From 9b359affe7b5904a1cf07adc781ccf72b6720f0b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 28 Jan 2015 10:18:07 -0800 Subject: [PATCH] Prepare SSH connections for proxying Summary: Ref T7034. In a cluster environment, when a user connects with a VCS request over SSH (like `git pull`), the receiving server may need to proxy it to a server which can actually satisfy the request. In order to proxy the request, we need to know which repository the user is interested in accessing. Split the SSH workflow into two steps: # First, identify the repository. # Then, execute the operation. In the future, this will allow us to put a possible "proxy the whole thing somewhere else" step in the middle, mirroring the behavior of Conduit. This is trivially easy in `git` and `hg`. Both identify the repository on the commmand line. This is fiendishly complex in `svn`, for the same reasons that hosting SVN was hard in the first place. Specifically: - The client doesn't tell us what it's after. - To get it to tell us, we have to send it a server capabilities string //first//. - We can't just start an `svnserve` process and read the repository out after a little while, because we may need to proxy the request once we figure out the repository. - We can't consume the client protocol frame that tells us what the client wants, because when we start the real server request it won't know what the client is after if it never receives that frame. - On the other hand, we must consume the second copy of the server protocol frame that would be sent to the client, or they'll get two "HELLO" messages and not know what to do. The approach here is straightforward, but the implementation is not trivial. Roughly: - Start `svnserve`, read the "hello" frame from it. - Kill `svnserve`. - Send the "hello" to the client. - Wait for the client to send us "I want repository X". - Save the message it sent us in the "peekBuffer". - Return "this is a request for repository X", so we can proxy it. Then, to continue the request: - Start the real `svnserve`. - Read the "hello" frame from it and throw it away. - Write the data in the "peekBuffer" to it, as though we'd just received it from the client. - State of the world is normal again, so we can continue. Also fixed some other issues: - SVN could choke if `repository.default-local-path` contained extra slashes. - PHP might emit some complaints when executing the commit hook; silence those. Test Plan: Pushed and pulled repositories in SVN, Mercurial and Git. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7034 Differential Revision: https://secure.phabricator.com/D11541 --- resources/celerity/map.php | 10 +- scripts/repository/commit_hook.php | 11 +- .../DiffusionGitReceivePackSSHWorkflow.php | 4 +- .../diffusion/ssh/DiffusionGitSSHWorkflow.php | 6 + .../ssh/DiffusionGitUploadPackSSHWorkflow.php | 4 +- .../DiffusionMercurialServeSSHWorkflow.php | 7 +- .../diffusion/ssh/DiffusionSSHWorkflow.php | 21 ++- .../DiffusionSubversionServeSSHWorkflow.php | 171 +++++++++++++++++- .../ssh/PhabricatorSSHPassthruCommand.php | 22 ++- 9 files changed, 223 insertions(+), 33 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 3b914fbb39..7442ca2abd 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => '04a24e98', + 'core.pkg.css' => '8815f87d', 'core.pkg.js' => 'efa12ecc', 'darkconsole.pkg.js' => '8ab24e01', 'differential.pkg.css' => '8af45893', @@ -125,7 +125,7 @@ return array( 'rsrc/css/phui/phui-action-list.css' => '9ee9910a', 'rsrc/css/phui/phui-box.css' => '7b3a2eed', 'rsrc/css/phui/phui-button.css' => '008ba5e2', - 'rsrc/css/phui/phui-crumbs-view.css' => '3e362700', + 'rsrc/css/phui/phui-crumbs-view.css' => '646a8830', 'rsrc/css/phui/phui-document.css' => 'bbeb1890', 'rsrc/css/phui/phui-feed-story.css' => 'c9f3a0b5', 'rsrc/css/phui/phui-fontkit.css' => '9c3d2dce', @@ -137,7 +137,7 @@ return array( 'rsrc/css/phui/phui-info-panel.css' => '27ea50a1', 'rsrc/css/phui/phui-list.css' => '53deb25c', 'rsrc/css/phui/phui-object-box.css' => '0d47b3c8', - 'rsrc/css/phui/phui-object-item-list-view.css' => '832c58fe', + 'rsrc/css/phui/phui-object-item-list-view.css' => '2686a80e', 'rsrc/css/phui/phui-pinboard-view.css' => '3dd4a269', 'rsrc/css/phui/phui-property-list-view.css' => '51480060', 'rsrc/css/phui/phui-remarkup-preview.css' => '19ad512b', @@ -770,7 +770,7 @@ return array( 'phui-calendar-day-css' => 'de035c8a', 'phui-calendar-list-css' => 'c1d0ca59', 'phui-calendar-month-css' => 'a92e47d2', - 'phui-crumbs-view-css' => '3e362700', + 'phui-crumbs-view-css' => '646a8830', 'phui-document-view-css' => 'bbeb1890', 'phui-feed-story-css' => 'c9f3a0b5', 'phui-font-icon-base-css' => '3dad2ae3', @@ -783,7 +783,7 @@ return array( 'phui-info-panel-css' => '27ea50a1', 'phui-list-view-css' => '53deb25c', 'phui-object-box-css' => '0d47b3c8', - 'phui-object-item-list-view-css' => '832c58fe', + 'phui-object-item-list-view-css' => '2686a80e', 'phui-pinboard-view-css' => '3dd4a269', 'phui-property-list-view-css' => '51480060', 'phui-remarkup-preview-css' => '19ad512b', diff --git a/scripts/repository/commit_hook.php b/scripts/repository/commit_hook.php index 4132df941e..035e03ef29 100755 --- a/scripts/repository/commit_hook.php +++ b/scripts/repository/commit_hook.php @@ -1,6 +1,15 @@ -#!/usr/bin/env php +#!/usr/bin/env TERM=dumb php getArgs(); - $path = head($args->getArg('dir')); - $repository = $this->loadRepository($path); + $repository = $this->getRepository(); // This is a write, and must have write access. $this->requireWriteAccess(); diff --git a/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php index a577700f78..4857aa8aa3 100644 --- a/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php @@ -7,6 +7,12 @@ abstract class DiffusionGitSSHWorkflow extends DiffusionSSHWorkflow { return parent::writeError($message."\n"); } + protected function identifyRepository() { + $args = $this->getArgs(); + $path = head($args->getArg('dir')); + return $this->loadRepositoryWithPath($path); + } + protected function waitForGitClient() { $io_channel = $this->getIOChannel(); diff --git a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php index 8934c4ac9b..beea59edb0 100644 --- a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php @@ -14,9 +14,7 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow { } protected function executeRepositoryOperations() { - $args = $this->getArgs(); - $path = head($args->getArg('dir')); - $repository = $this->loadRepository($path); + $repository = $this->getRepository(); $command = csprintf('git-upload-pack -- %s', $repository->getLocalPath()); $command = PhabricatorDaemon::sudoCommandAsDaemonUser($command); diff --git a/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php index cbd1d1a752..6afa70bf9c 100644 --- a/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php @@ -24,11 +24,14 @@ final class DiffusionMercurialServeSSHWorkflow )); } - protected function executeRepositoryOperations() { + protected function identifyRepository() { $args = $this->getArgs(); $path = $args->getArg('repository'); - $repository = $this->loadRepository($path); + return $this->loadRepositoryWithPath($path); + } + protected function executeRepositoryOperations() { + $repository = $this->getRepository(); $args = $this->getArgs(); if (!$args->getArg('stdio')) { diff --git a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php index fda7f16f94..4b0ec5d190 100644 --- a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php @@ -8,11 +8,16 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { public function getRepository() { if (!$this->repository) { - throw new Exception('Call loadRepository() before getRepository()!'); + throw new Exception(pht('Repository is not available yet!')); } return $this->repository; } + private function setRepository(PhabricatorRepository $repository) { + $this->repository = $repository; + return $this; + } + public function getArgs() { return $this->args; } @@ -33,6 +38,10 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { return $env; } + /** + * Identify and load the affected repository. + */ + abstract protected function identifyRepository(); abstract protected function executeRepositoryOperations(); protected function writeError($message) { @@ -43,6 +52,12 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { final public function execute(PhutilArgumentParser $args) { $this->args = $args; + $repository = $this->identifyRepository(); + $this->setRepository($repository); + + // TODO: Here, we would make a proxying decision, had I implemented + // proxying yet. + try { return $this->executeRepositoryOperations(); } catch (Exception $ex) { @@ -51,7 +66,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { } } - protected function loadRepository($path) { + protected function loadRepositoryWithPath($path) { $viewer = $this->getUser(); $regex = '@^/?diffusion/(?P[A-Z]+)(?:/|\z)@'; @@ -88,8 +103,6 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { pht('This repository is not available over SSH.')); } - $this->repository = $repository; - return $repository; } diff --git a/src/applications/diffusion/ssh/DiffusionSubversionServeSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionSubversionServeSSHWorkflow.php index 2d3a3280ea..b7f16f6561 100644 --- a/src/applications/diffusion/ssh/DiffusionSubversionServeSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionSubversionServeSSHWorkflow.php @@ -20,6 +20,12 @@ final class DiffusionSubversionServeSSHWorkflow private $internalBaseURI; private $externalBaseURI; + private $peekBuffer; + private $command; + + private function getCommand() { + return $this->command; + } protected function didConstruct() { $this->setName('svnserve'); @@ -32,7 +38,107 @@ final class DiffusionSubversionServeSSHWorkflow )); } + protected function identifyRepository() { + // NOTE: In SVN, we need to read the first few protocol frames before we + // can determine which repository the user is trying to access. We're + // going to peek at the data on the wire to identify the repository. + + $io_channel = $this->getIOChannel(); + + // Before the client will send us the first protocol frame, we need to send + // it a connection frame with server capabilities. To figure out the + // correct frame we're going to start `svnserve`, read the frame from it, + // send it to the client, then kill the subprocess. + + // TODO: This is pretty inelegant and the protocol frame will change very + // rarely. We could cache it if we can find a reasonable way to dirty the + // cache. + + $command = csprintf('svnserve -t'); + $command = PhabricatorDaemon::sudoCommandAsDaemonUser($command); + $future = new ExecFuture('%C', $command); + $exec_channel = new PhutilExecChannel($future); + $exec_protocol = new DiffusionSubversionWireProtocol(); + + while (true) { + PhutilChannel::waitForAny(array($exec_channel)); + $exec_channel->update(); + + $exec_message = $exec_channel->read(); + if ($exec_message !== null) { + $messages = $exec_protocol->writeData($exec_message); + if ($messages) { + $message = head($messages); + $raw = $message['raw']; + + // Write the greeting frame to the client. + $io_channel->write($raw); + + // Kill the subprocess. + $future->resolveKill(); + break; + } + } + + if (!$exec_channel->isOpenForReading()) { + throw new Exception( + pht( + 'svnserve subprocess exited before emitting a protocol frame.')); + } + } + + $io_protocol = new DiffusionSubversionWireProtocol(); + while (true) { + PhutilChannel::waitForAny(array($io_channel)); + $io_channel->update(); + + $in_message = $io_channel->read(); + if ($in_message !== null) { + $this->peekBuffer .= $in_message; + if (strlen($this->peekBuffer) > (1024 * 1024)) { + throw new Exception( + pht( + 'Client transmitted more than 1MB of data without transmitting '. + 'a recognizable protocol frame.')); + } + + $messages = $io_protocol->writeData($in_message); + if ($messages) { + $message = head($messages); + $struct = $message['structure']; + + // This is the: + // + // ( version ( cap1 ... ) url ... ) + // + // The `url` allows us to identify the repository. + + $uri = $struct[2]['value']; + $path = $this->getPathFromSubversionURI($uri); + + return $this->loadRepositoryWithPath($path); + } + } + + if (!$io_channel->isOpenForReading()) { + throw new Exception( + pht( + 'Client closed connection before sending a complete protocol '. + 'frame.')); + } + + // If the client has disconnected, kill the subprocess and bail. + if (!$io_channel->isOpenForWriting()) { + throw new Exception( + pht( + 'Client closed connection before receiving response.')); + } + } + } + protected function executeRepositoryOperations() { + $repository = $this->getRepository(); + $args = $this->getArgs(); if (!$args->getArg('tunnel')) { throw new Exception('Expected `svnserve -t`!'); @@ -48,12 +154,15 @@ final class DiffusionSubversionServeSSHWorkflow $this->inProtocol = new DiffusionSubversionWireProtocol(); $this->outProtocol = new DiffusionSubversionWireProtocol(); - $err = id($this->newPassthruCommand()) + $this->command = id($this->newPassthruCommand()) ->setIOChannel($this->getIOChannel()) ->setCommandChannelFromExecFuture($future) ->setWillWriteCallback(array($this, 'willWriteMessageCallback')) - ->setWillReadCallback(array($this, 'willReadMessageCallback')) - ->execute(); + ->setWillReadCallback(array($this, 'willReadMessageCallback')); + + $this->command->setPauseIOReads(true); + + $err = $this->command->execute(); if (!$err && $this->didSeeWrite) { $this->getRepository()->writeStatusMessage( @@ -161,6 +270,19 @@ final class DiffusionSubversionServeSSHWorkflow switch ($this->outPhaseCount) { case 0: // This is the "greeting", which announces capabilities. + + // We already sent this when we were figuring out which + // repository this request is for, so we aren't going to send + // it again. + + // Instead, we're going to replay the client's response (which + // we also already read). + + $command = $this->getCommand(); + $command->writeIORead($this->peekBuffer); + $command->setPauseIOReads(false); + + $message_raw = null; break; case 1: // This responds to the client greeting, and announces auth. @@ -203,7 +325,9 @@ final class DiffusionSubversionServeSSHWorkflow } - $result[] = $message_raw; + if ($message_raw !== null) { + $result[] = $message_raw; + } } if (!$result) { @@ -213,7 +337,7 @@ final class DiffusionSubversionServeSSHWorkflow return implode('', $result); } - private function makeInternalURI($uri_string) { + private function getPathFromSubversionURI($uri_string) { $uri = new PhutilURI($uri_string); $proto = $uri->getProtocol(); @@ -223,11 +347,10 @@ final class DiffusionSubversionServeSSHWorkflow 'Protocol for URI "%s" MUST be "svn+ssh".', $uri_string)); } - $path = $uri->getPath(); // Subversion presumably deals with this, but make sure there's nothing - // skethcy going on with the URI. + // sketchy going on with the URI. if (preg_match('(/\\.\\./)', $path)) { throw new Exception( pht( @@ -235,8 +358,17 @@ final class DiffusionSubversionServeSSHWorkflow $uri_string)); } - $repository = $this->loadRepository($path); + $path = $this->normalizeSVNPath($path); + return $path; + } + + private function makeInternalURI($uri_string) { + $uri = new PhutilURI($uri_string); + + $repository = $this->getRepository(); + + $path = $this->getPathFromSubversionURI($uri_string); $path = preg_replace( '(^/diffusion/[A-Z]+)', rtrim($repository->getLocalPath(), '/'), @@ -246,14 +378,25 @@ final class DiffusionSubversionServeSSHWorkflow $path = rtrim($path, '/'); } + // NOTE: We are intentionally NOT removing username information from the + // URI. Subversion retains it over the course of the request and considers + // two repositories with different username identifiers to be distinct and + // incompatible. + $uri->setPath($path); // If this is happening during the handshake, these are the base URIs for // the request. if ($this->externalBaseURI === null) { $pre = (string)id(clone $uri)->setPath(''); - $this->externalBaseURI = $pre.'/diffusion/'.$repository->getCallsign(); - $this->internalBaseURI = $pre.rtrim($repository->getLocalPath(), '/'); + + $external_path = '/diffusion/'.$repository->getCallsign(); + $external_path = $this->normalizeSVNPath($external_path); + $this->externalBaseURI = $pre.$external_path; + + $internal_path = rtrim($repository->getLocalPath(), '/'); + $internal_path = $this->normalizeSVNPath($internal_path); + $this->internalBaseURI = $pre.$internal_path; } return (string)$uri; @@ -270,4 +413,12 @@ final class DiffusionSubversionServeSSHWorkflow return $uri; } + private function normalizeSVNPath($path) { + // Subversion normalizes redundant slashes internally, so normalize them + // here as well to make sure things match up. + $path = preg_replace('(/+)', '/', $path); + + return $path; + } + } diff --git a/src/infrastructure/ssh/PhabricatorSSHPassthruCommand.php b/src/infrastructure/ssh/PhabricatorSSHPassthruCommand.php index 5724728676..f27d44660d 100644 --- a/src/infrastructure/ssh/PhabricatorSSHPassthruCommand.php +++ b/src/infrastructure/ssh/PhabricatorSSHPassthruCommand.php @@ -43,6 +43,7 @@ final class PhabricatorSSHPassthruCommand extends Phobject { private $execFuture; private $willWriteCallback; private $willReadCallback; + private $pauseIOReads; public function setCommandChannelFromExecFuture(ExecFuture $exec_future) { $exec_channel = new PhutilExecChannel($exec_future); @@ -78,6 +79,11 @@ final class PhabricatorSSHPassthruCommand extends Phobject { $this->errorChannel->write($data); } + public function setPauseIOReads($pause) { + $this->pauseIOReads = $pause; + return $this; + } + public function execute() { $command_channel = $this->commandChannel; $io_channel = $this->ioChannel; @@ -140,16 +146,15 @@ final class PhabricatorSSHPassthruCommand extends Phobject { $done = !$command_channel->isOpenForReading() && $command_channel->isReadBufferEmpty(); - $in_message = $io_channel->read(); - if ($in_message !== null) { - $in_message = $this->willWriteData($in_message); + if (!$this->pauseIOReads) { + $in_message = $io_channel->read(); if ($in_message !== null) { - $command_channel->write($in_message); + $this->writeIORead($in_message); } } $out_message = $command_channel->read(); - if ($out_message !== null) { + if (strlen($out_message)) { $out_message = $this->willReadData($out_message); if ($out_message !== null) { $io_channel->write($out_message); @@ -185,6 +190,13 @@ final class PhabricatorSSHPassthruCommand extends Phobject { return $err; } + public function writeIORead($in_message) { + $in_message = $this->willWriteData($in_message); + if (strlen($in_message)) { + $this->commandChannel->write($in_message); + } + } + public function willWriteData($message) { if ($this->willWriteCallback) { return call_user_func($this->willWriteCallback, $this, $message);