Using hooks in the authorization process (of Phabricator)
Gregory Szorc
gregory.szorc at gmail.com
Wed Feb 21 05:04:53 UTC 2018
On Fri, Dec 15, 2017 at 9:16 AM, Harald Klimach <harald at klimachs.de> wrote:
> Hi there,
>
> I gave Phabricator a try with Mercurial repositories. Unfortunately, their
> support is somewhat broken,
> because they try to parse the data stream going into the server:
> https://secure.phabricator.com/T10382
>
> This results for example in no support for the bundle2 protocol:
> https://secure.phabricator.com/T10382,
> but as reported in the first ticket above it also fails even with bundle2
> protocol suppressed in new client/server
> configurations, at least sometimes.
>
> In any case, the point of parsing the stream serves the purpose to figure
> out whether this is a write operation
> or not. On the one side we need to know this, to authorize users with read
> but not write access to the repository,
> and on the other side this information is used to trigger updates for the
> webfrontend when necessary.
>
> durin42 suggested in the phabricator tickets to go with hooks, which they
> don’t seem to like, but I think
> it would be the best solution for my setup at least, so I gave it a try.
>
> The workflow is roughly like this:
> * The SSH server calls a php script, which takes care of the
> authentication (looking up proper keys).
> * The PHP process then starts an HG process and passes the streams in and
> out of it.
> * In the original version the stream is decoded and reassembled to extract
> which commands are passed in.
> Based on the found commands it is decided whether the user is allowed to
> do it or not.
>
> My take on this with a hook-based solution is to leave the stream
> completely untouched but communicate with
> the hook for the authorization. For the communication I use files based on
> the process ID of the Mercurial process.
> So far this seems to work quite fine, but I would be happy for any
> feedback on the workflow that I am now using,
> and whether something more elegant would be possible. Thanks a lot!
>
> So here is what I do now:
> * The PHP process starts the Mercurial serving process
> * I then generate a file with a name based on the PID of the child process
> (actually not directly the Mercurial process but a sudo to change the
> user). Into this file I write, whether the user is allowed to write to the
> repository (and if so, a file where Mercurial should
> write a reply, that the repository changed) and possibly an error message
> detailling why the access is denied.
> * I generate a second file, into which the Mercurial hook should write a
> reply, if data is to be written in the repository, this is only used
> to trigger an update of the webdata.
> * Then the PHP process starts to feed the streams in and out of the
> Mercurial process, where I use a pretxnchangegroup hook:
> pretxnchangegroup = python:/var/www/apps/phabricator/resources/hghook/
> phab_auth_hook.py:phab_authorization
> that looks like this:
>
> def phab_authorization(ui, repo, **kwargs):
> import os
> import stat
> parentPID = os.getppid()
> auth_fname = '/tmp/phabhgssh-auth_{0}'.format(parentPID)
> try:
> with open(auth_fname, 'r') as authfile:
> try:
> firstline = next(authfile)
> auth_string = ''.join(firstline.split())
> except StopIteration as e:
> auth_string = 'DENIED'
>
> if auth_string == 'CANWRITE':
> try:
> fline = next(authfile)
> except StopIteration as e:
> ui.warn('Do not know how to interact with
> Phabricator\n')
> ui.warn('Writefile info missing!\n')
> return True
> else:
> ui.warn('Not allowed to write to this repository:\n')
> for line in authfile:
> ui.warn(line)
> ui.warn('\n')
> return True
>
> wrote_fname = ''.join(fline.split())
>
> except IOError as e:
> ui.warn('Missing authorization file.\n')
> ui.warn('Not allowed to write to this repository!\n')
> return True
>
> if (wrote_fname):
> with open(wrote_fname, 'w') as wrotefile:
> wrotefile.write('writing to repo\n')
>
> return False
>
> * After the Mercurial process finishes the PHP process reads out the
> feedback file to see whether a change happend
> * It then unlinks the two generated files again
>
> Questions on my mind:
> Is this a sane approach? Utilizing files for this communication feels a
> little clumsy here.
> Is the pretxnchangegroup hook the best option for this kind of control? I
> don’t need the
> content of the changes to decide the authorization, so prechangegroup
> would be even
> better, but it is not a controlling hook. Maybe pre-push would be a good
> option? But the
> documentation advises to use the standard hooks.
> Are there any better options to achieve this?
>
> With this approach, there is no problem with the bundle2 protocol and
> recent Mercurial
> versions, I successfully can push to the repositories (if allowed), where
> it previously
> failed. (My client is on Mercurial 4.3.1, the server is on 4.3.2).
>
> Best regards and many thanks,
> Harald
>
> The PHP serving code, starting the Mercurial server and dealing with the
> files looks like this:
>
> protected function executeRepositoryOperations() {
> $repository = $this->getRepository();
> $args = $this->getArgs();
>
> if (!$args->getArg('stdio')) {
> throw new Exception(pht('Expected `%s`!', 'hg ... --stdio'));
> }
>
> if ($args->getArg('command') !== array('serve')) {
> throw new Exception(pht('Expected `%s`!', 'hg ... serve'));
> }
>
> if ($this->shouldProxy()) {
> $command = $this->getProxyCommand();
> } else {
> $bin = Filesystem::resolveBinary('hg');
> $command = csprintf(
> '%s -R %s serve --stdio',
> $bin,
> $repository->getLocalPath());
> }
> $command = PhabricatorDaemon::sudoCommandAsDaemonUser($command);
>
> $future = id(new ExecFuture('%C', $command))
> ->setEnv($this->getEnvironment());
>
> $io_channel = $this->getIOChannel();
> // Start the Mercurial server to get its PID.
> $hgproc = id($this->newPassthruCommand())
> ->setIOChannel($io_channel)
> ->setCommandChannelFromExecFuture($future);
> // Construct a file for the Mercurial hook to indicate whether
> // write should have happened.
> $wrote_fname = '/tmp/phabhgssh-didWrite_'.$future->getPID();
> $fh = fopen($wrote_fname, 'w');
> fwrite($fh, "\n");
> fclose($fh);
>
> // Create a file to indicate whether the user is authorized to
> // push to the repository or not.
> $auth_fname = '/tmp/phabhgssh-auth_'.$future->getPID();
> $fh = fopen($auth_fname, 'w');
> if (!$fh) {
> throw new Exception(
> pht('Could not properly open file to communicate authorization with
> mercurial hook.')
> );
> }
> if ($this->canWrite()) {
> fwrite($fh, "CANWRITE\n");
> fwrite($fh, $wrote_fname."\n");
> } else {
> fwrite($fh, "DENIED because:\n");
> fwrite($fh, $this->errorMsg);
> }
> fclose($fh);
> chmod($wrote_fname, 0666);
>
> // Feed the Mercurial process with the streams.
> $err = $hgproc->execute();
>
> // Remove the authorization file again.
> unlink($auth_fname);
>
> // Check whether Mercurial wanted to write and remove the
> // file for this again.
> $fh = fopen($wrote_fname, 'r');
> if (!$fh) {
> throw new Exception(
> pht('Could not properly open file to communicate repo change with
> mercurial hook.')
> );
> }
> $hgWriteLine = fgets($fh);
> fclose($fh);
> unlink($wrote_fname);
> $this->didSeeWrite = (str_replace(PHP_EOL, '', $hgWriteLine) ===
> 'writing to repo');
> if ($this->didSeeWrite) {
> // Throw errors if not allowed to write but did attempt to.
> $this->requireWriteAccess();
> }
>
> if (!$err && $this->didSeeWrite) {
> $repository->writeStatusMessage(
> PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE,
> PhabricatorRepositoryStatusMessage::CODE_OKAY);
> }
>
> return $err;
> }
>
>
>
> For completeness here is the full patch against Phabricator that I use:
>
> diff --git a/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php
> b/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php
> index 4508ae53d..24b4e176a 100644
> @@ -57,25 +59,58 @@ final class DiffusionMercurialServeSSHWorkflow
> ->setEnv($this->getEnvironment());
>
> $io_channel = $this->getIOChannel();
> - $protocol_channel = new DiffusionMercurialWireClientSS
> HProtocolChannel(
> - $io_channel);
> -
> - $err = id($this->newPassthruCommand())
> - ->setIOChannel($protocol_channel)
> - ->setCommandChannelFromExecFuture($future)
> - ->setWillWriteCallback(array($this, 'willWriteMessageCallback'))
> - ->execute();
> -
> - // TODO: It's apparently technically possible to communicate errors to
> - // Mercurial over SSH by writing a special "\n<error>\n-\n" string.
> However,
> - // my attempt to implement that resulted in Mercurial closing the
> socket and
> - // then hanging, without showing the error. This might be an issue on
> our
> - // side (we need to close our half of the socket?), or maybe the code
> - // for this in Mercurial doesn't actually work, or maybe something
> else
> - // is afoot. At some point, we should look into doing this more
> cleanly.
> - // For now, when we, e.g., reject writes for policy reasons, the user
> will
> - // see "abort: unexpected response: empty string" after the
> diagnostically
> - // useful, e.g., "remote: This repository is read-only over SSH."
> message.
> + // Start the Mercurial server to get its PID.
> + $hgproc = id($this->newPassthruCommand())
> + ->setIOChannel($io_channel)
> + ->setCommandChannelFromExecFuture($future);
> + // Construct a file for the Mercurial hook to indicate whether
> + // write should have happened.
> + $wrote_fname = '/tmp/phabhgssh-didWrite_'.$future->getPID();
> + $fh = fopen($wrote_fname, 'w');
> + fwrite($fh, "\n");
> + fclose($fh);
> +
> + // Create a file to indicate whether the user is authorized to
> + // push to the repository or not.
> + $auth_fname = '/tmp/phabhgssh-auth_'.$future->getPID();
> + $fh = fopen($auth_fname, 'w');
> + if (!$fh) {
> + throw new Exception(
> + pht('Could not properly open file to communicate authorization with
> mercurial hook.')
> + );
> + }
> + if ($this->canWrite()) {
> + fwrite($fh, "CANWRITE\n");
> + fwrite($fh, $wrote_fname."\n");
> + } else {
> + fwrite($fh, "DENIED because:\n");
> + fwrite($fh, $this->errorMsg);
> + }
> + fclose($fh);
> + chmod($wrote_fname, 0666);
> +
> + // Feed the Mercurial process with the streams.
> + $err = $hgproc->execute();
> +
> + // Remove the authorization file again.
> + unlink($auth_fname);
> +
> + // Check whether Mercurial wanted to write and remove the
> + // file for this again.
> + $fh = fopen($wrote_fname, 'r');
> + if (!$fh) {
> + throw new Exception(
> + pht('Could not properly open file to communicate repo change with
> mercurial hook.')
> + );
> + }
> + $hgWriteLine = fgets($fh);
> + fclose($fh);
> + unlink($wrote_fname);
> + $this->didSeeWrite = (str_replace(PHP_EOL, '', $hgWriteLine) ===
> 'writing to repo');
> + if ($this->didSeeWrite) {
> + // Throw errors if not allowed to write but did attempt to.
> + $this->requireWriteAccess();
> + }
>
> if (!$err && $this->didSeeWrite) {
> $repository->writeStatusMessage(
> diff --git a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php
> b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php
> index 0e5ad7bbe..93b02c4ca 100644
> --- a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php
> +++ b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php
> @@ -5,6 +5,7 @@ abstract class DiffusionSSHWorkflow extends
> PhabricatorSSHWorkflow {
> private $args;
> private $repository;
> private $hasWriteAccess;
> + protected $errorMsg = '';
> private $proxyURI;
> private $baseRequestPath;
>
> @@ -208,20 +209,20 @@ abstract class DiffusionSSHWorkflow extends
> PhabricatorSSHWorkflow {
> return $repository;
> }
>
> - protected function requireWriteAccess($protocol_command = null) {
> + protected function canWrite($protocol_command = null) {
> if ($this->hasWriteAccess === true) {
> - return;
> + return $this->hasWriteAccess;
> }
>
> $repository = $this->getRepository();
> $viewer = $this->getUser();
>
> if ($viewer->isOmnipotent()) {
> - throw new Exception(
> - pht(
> + $this->hasWriteAccess = false;
> + $this->errorMsg = pht(
> 'This request is authenticated as a cluster device, but is '.
> 'performing a write. Writes must be performed with a real '.
> - 'user account.'));
> + 'user account.');
> }
>
> $protocol = PhabricatorRepositoryURI::BUILTIN_PROTOCOL_SSH;
> @@ -230,27 +231,36 @@ abstract class DiffusionSSHWorkflow extends
> PhabricatorSSHWorkflow {
> $viewer,
> $repository,
> DiffusionPushCapability::CAPABILITY);
> - if (!$can_push) {
> - throw new Exception(
> - pht('You do not have permission to push to this repository.'));
> + if ($can_push) {
> + $this->hasWriteAccess = true;
> + } else {
> + $this->hasWriteAccess = false;
> + $this->errorMsg = pht('You do not have permission to push to this
> repository.');
> }
> } else {
> if ($protocol_command !== null) {
> - throw new Exception(
> - pht(
> + $this->hasWriteAccess = false;
> + $this->errorMsg = pht(
> 'This repository is read-only over SSH (tried to execute '.
> 'protocol command "%s").',
> - $protocol_command));
> + $protocol_command);
> } else {
> - throw new Exception(
> - pht('This repository is read-only over SSH.'));
> + $this->hasWriteAccess = false;
> + $this->errorMsg = pht('This repository is read-only over SSH.');
> }
> }
>
> - $this->hasWriteAccess = true;
> return $this->hasWriteAccess;
> }
>
> + protected function requireWriteAccess($protocol_command = null) {
> + if ($this->canWrite($protocol_command)) {
> + return $this->hasWriteAccess;
> + } else {
> + throw new Exception($this->errorMsg);
> + }
> + }
> +
> protected function shouldSkipReadSynchronization() {
> $viewer = $this->getUser();
>
I didn't read the patches in extreme detail, but the approach of having
Phabricator and Mercurial speak with each other to coordinate access seems
reasonable.
In addition to the pretxnchangegroup hook, you'll also want to intercept
the prepushkey hook. Modern versions of Mercurial should perform all writes
within a transaction, so a pretxnopen hook should always fire if a write is
being attempted.
I also think there is room in core Mercurial to control whether access
should be read-only or read-write. But getting this right 100% of the time
is hard: you are only ever one bug or one extension away from circumventing
permissions checks. Relying on filesystem permissions is much more robust.
Speaking of filesystem permissions, one thing you could do is start the
Mercurial server process as a different user depending on whether the
authenticated Phabricator user has write access. Or you could spawn the
process in a container with a read-only bind mount of the repository
directory. Lots of options for handling this at the OS level instead of the
application level. And those options will be much more robust in terms of
preventing writes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial/attachments/20180220/a90f8a60/attachment.html>
More information about the Mercurial
mailing list