Using hooks in the authorization process (of Phabricator)
Harald Klimach
harald at klimachs.de
Fri Dec 15 17:16:00 UTC 2017
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 DiffusionMercurialWireClientSSHProtocolChannel(
- $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();
More information about the Mercurial
mailing list