Skip to content

Commit 8840f60

Browse files
author
epriestley
committed
Enable Mercurial reads and writes over SSH
Summary: Ref T2230. This is substantially more complicated than Git, but mostly because Mercurial's protocol is a like 50 ad-hoc extensions cobbled together. Because we must decode protocol frames in order to determine if a request is read or write, 90% of this is implementing a stream parser for the protocol. Mercurial's own parser is simpler, but relies on blocking reads. Since we don't even have methods for blocking reads right now and keeping the whole thing non-blocking is conceptually better, I made the parser nonblocking. It ends up being a lot of stuff. I made an effort to cover it reasonably well with unit tests, and to make sure we fail closed (i.e., reject requests) if there are any parts of the protocol I got wrong. A lot of the complexity is sharable with the HTTP stuff, so it ends up being not-so-bad, just very hard to verify by inspection as clearly correct. Test Plan: - Ran `hg clone` over SSH. - Ran `hg fetch` over SSH. - Ran `hg push` over SSH, to a read-only repo (error) and a read-write repo (success). Reviewers: btrahan, asherkin Reviewed By: btrahan CC: aran Maniphest Tasks: T2230 Differential Revision: https://secure.phabricator.com/D7553
1 parent ac7c739 commit 8840f60

14 files changed

+544
-42
lines changed

‎scripts/ssh/ssh-exec.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@
6161

6262
$workflows = array(
6363
new ConduitSSHWorkflow(),
64-
64+
new DiffusionSSHMercurialServeWorkflow(),
6565
new DiffusionSSHGitUploadPackWorkflow(),
6666
new DiffusionSSHGitReceivePackWorkflow(),
6767
);

‎src/__phutil_library_map__.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -543,6 +543,10 @@
543543
'DiffusionSSHGitReceivePackWorkflow' => 'applications/diffusion/ssh/DiffusionSSHGitReceivePackWorkflow.php',
544544
'DiffusionSSHGitUploadPackWorkflow' => 'applications/diffusion/ssh/DiffusionSSHGitUploadPackWorkflow.php',
545545
'DiffusionSSHGitWorkflow' => 'applications/diffusion/ssh/DiffusionSSHGitWorkflow.php',
546+
'DiffusionSSHMercurialServeWorkflow' => 'applications/diffusion/ssh/DiffusionSSHMercurialServeWorkflow.php',
547+
'DiffusionSSHMercurialWireClientProtocolChannel' => 'applications/diffusion/ssh/DiffusionSSHMercurialWireClientProtocolChannel.php',
548+
'DiffusionSSHMercurialWireTestCase' => 'applications/diffusion/ssh/__tests__/DiffusionSSHMercurialWireTestCase.php',
549+
'DiffusionSSHMercurialWorkflow' => 'applications/diffusion/ssh/DiffusionSSHMercurialWorkflow.php',
546550
'DiffusionSSHWorkflow' => 'applications/diffusion/ssh/DiffusionSSHWorkflow.php',
547551
'DiffusionServeController' => 'applications/diffusion/controller/DiffusionServeController.php',
548552
'DiffusionSetPasswordPanel' => 'applications/diffusion/panel/DiffusionSetPasswordPanel.php',
@@ -2808,6 +2812,10 @@
28082812
'DiffusionSSHGitReceivePackWorkflow' => 'DiffusionSSHGitWorkflow',
28092813
'DiffusionSSHGitUploadPackWorkflow' => 'DiffusionSSHGitWorkflow',
28102814
'DiffusionSSHGitWorkflow' => 'DiffusionSSHWorkflow',
2815+
'DiffusionSSHMercurialServeWorkflow' => 'DiffusionSSHMercurialWorkflow',
2816+
'DiffusionSSHMercurialWireClientProtocolChannel' => 'PhutilProtocolChannel',
2817+
'DiffusionSSHMercurialWireTestCase' => 'PhabricatorTestCase',
2818+
'DiffusionSSHMercurialWorkflow' => 'DiffusionSSHWorkflow',
28112819
'DiffusionSSHWorkflow' => 'PhabricatorSSHWorkflow',
28122820
'DiffusionServeController' => 'DiffusionController',
28132821
'DiffusionSetPasswordPanel' => 'PhabricatorSettingsPanel',

‎src/applications/diffusion/controller/DiffusionServeController.php

Lines changed: 2 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -228,40 +228,8 @@ private function isReadOnlyRequest(
228228
case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
229229
$cmd = $request->getStr('cmd');
230230
if ($cmd == 'batch') {
231-
// For "batch" we get a "cmds" argument like
232-
//
233-
// heads ;known nodes=
234-
//
235-
// We need to examine the commands (here, "heads" and "known") to
236-
// make sure they're all read-only.
237-
238-
$args = $this->getMercurialArguments();
239-
$cmds = idx($args, 'cmds');
240-
if ($cmds) {
241-
242-
// NOTE: Mercurial has some code to escape semicolons, but it does
243-
// not actually function for command separation. For example, these
244-
// two batch commands will produce completely different results (the
245-
// former will run the lookup; the latter will fail with a parser
246-
// error):
247-
//
248-
// lookup key=a:xb;lookup key=z* 0
249-
// lookup key=a:;b;lookup key=z* 0
250-
// ^
251-
// |
252-
// +-- Note semicolon.
253-
//
254-
// So just split unconditionally.
255-
256-
$cmds = explode(';', $cmds);
257-
foreach ($cmds as $sub_cmd) {
258-
$name = head(explode(' ', $sub_cmd, 2));
259-
if (!DiffusionMercurialWireProtocol::isReadOnlyCommand($name)) {
260-
return false;
261-
}
262-
}
263-
return true;
264-
}
231+
$cmds = idx($this->getMercurialArguments(), 'cmds');
232+
return DiffusionMercurialWireProtocol::isReadOnlyBatchCommand($cmds);
265233
}
266234
return DiffusionMercurialWireProtocol::isReadOnlyCommand($cmd);
267235
case PhabricatorRepositoryType::REPOSITORY_TYPE_SUBVERSION:

‎src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,4 +59,44 @@ public static function isReadOnlyCommand($command) {
5959
return isset($read_only[$command]);
6060
}
6161

62+
public static function isReadOnlyBatchCommand($cmds) {
63+
if (!strlen($cmds)) {
64+
// We expect a "batch" command to always have a "cmds" string, so err
65+
// on the side of caution and throw if we don't get any data here. This
66+
// either indicates a mangled command from the client or a programming
67+
// error in our code.
68+
throw new Exception("Expected nonempty 'cmds' specification!");
69+
}
70+
71+
// For "batch" we get a "cmds" argument like:
72+
//
73+
// heads ;known nodes=
74+
//
75+
// We need to examine the commands (here, "heads" and "known") to make sure
76+
// they're all read-only.
77+
78+
// NOTE: Mercurial has some code to escape semicolons, but it does not
79+
// actually function for command separation. For example, these two batch
80+
// commands will produce completely different results (the former will run
81+
// the lookup; the latter will fail with a parser error):
82+
//
83+
// lookup key=a:xb;lookup key=z* 0
84+
// lookup key=a:;b;lookup key=z* 0
85+
// ^
86+
// |
87+
// +-- Note semicolon.
88+
//
89+
// So just split unconditionally.
90+
91+
$cmds = explode(';', $cmds);
92+
foreach ($cmds as $sub_cmd) {
93+
$name = head(explode(' ', $sub_cmd, 2));
94+
if (!self::isReadOnlyCommand($name)) {
95+
return false;
96+
}
97+
}
98+
99+
return true;
100+
}
101+
62102
}

‎src/applications/diffusion/ssh/DiffusionSSHMercurialServeWorkflow.php

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
<?php
2+
3+
final class DiffusionSSHMercurialServeWorkflow
4+
extends DiffusionSSHMercurialWorkflow {
5+
6+
protected $didSeeWrite;
7+
8+
public function didConstruct() {
9+
$this->setName('hg');
10+
$this->setArguments(
11+
array(
12+
array(
13+
'name' => 'repository',
14+
'short' => 'R',
15+
'param' => 'repo',
16+
),
17+
array(
18+
'name' => 'stdio',
19+
),
20+
array(
21+
'name' => 'command',
22+
'wildcard' => true,
23+
),
24+
));
25+
}
26+
27+
public function getRequestPath() {
28+
return $this->getArgs()->getArg('repository');
29+
}
30+
31+
protected function executeRepositoryOperations(
32+
PhabricatorRepository $repository) {
33+
34+
$args = $this->getArgs();
35+
36+
if (!$args->getArg('stdio')) {
37+
throw new Exception("Expected `hg ... --stdio`!");
38+
}
39+
40+
if ($args->getArg('command') !== array('serve')) {
41+
throw new Exception("Expected `hg ... serve`!");
42+
}
43+
44+
$future = new ExecFuture(
45+
'hg -R %s serve --stdio',
46+
$repository->getLocalPath());
47+
48+
$io_channel = $this->getIOChannel();
49+
50+
$protocol_channel = new DiffusionSSHMercurialWireClientProtocolChannel(
51+
$io_channel);
52+
53+
$err = id($this->newPassthruCommand())
54+
->setIOChannel($protocol_channel)
55+
->setCommandChannelFromExecFuture($future)
56+
->setWillWriteCallback(array($this, 'willWriteMessageCallback'))
57+
->execute();
58+
59+
// TODO: It's apparently technically possible to communicate errors to
60+
// Mercurial over SSH by writing a special "\n<error>\n-\n" string. However,
61+
// my attempt to implement that resulted in Mercurial closing the socket and
62+
// then hanging, without showing the error. This might be an issue on our
63+
// side (we need to close our half of the socket?), or maybe the code
64+
// for this in Mercurial doesn't actually work, or maybe something else
65+
// is afoot. At some point, we should look into doing this more cleanly.
66+
// For now, when we, e.g., reject writes for policy reasons, the user will
67+
// see "abort: unexpected response: empty string" after the diagnostically
68+
// useful, e.g., "remote: This repository is read-only over SSH." message.
69+
70+
if (!$err && $this->didSeeWrite) {
71+
$repository->writeStatusMessage(
72+
PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE,
73+
PhabricatorRepositoryStatusMessage::CODE_OKAY);
74+
}
75+
76+
return $err;
77+
}
78+
79+
public function willWriteMessageCallback(
80+
PhabricatorSSHPassthruCommand $command,
81+
$message) {
82+
83+
$command = $message['command'];
84+
85+
// Check if this is a readonly command.
86+
87+
$is_readonly = false;
88+
if ($command == 'batch') {
89+
$cmds = idx($message['arguments'], 'cmds');
90+
if (DiffusionMercurialWireProtocol::isReadOnlyBatchCommand($cmds)) {
91+
$is_readonly = true;
92+
}
93+
} else if (DiffusionMercurialWireProtocol::isReadOnlyCommand($command)) {
94+
$is_readonly = true;
95+
}
96+
97+
if (!$is_readonly) {
98+
$this->requireWriteAccess();
99+
$this->didSeeWrite = true;
100+
}
101+
102+
// If we're good, return the raw message data.
103+
return $message['raw'];
104+
}
105+
106+
}

0 commit comments

Comments
 (0)