Patchwork D2871: wireproto: service multiple command requests per HTTP request

login
register
mail settings
Submitter phabricator
Date March 20, 2018, 12:01 a.m.
Message ID <6a137640872fe70ab25c63e9cd0d24ba@localhost.localdomain>
Download mbox | patch
Permalink /patch/29657/
State Not Applicable
Headers show

Comments

phabricator - March 20, 2018, 12:01 a.m.
indygreg updated this revision to Diff 7147.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D2871?vs=7055&id=7147

REVISION DETAIL
  https://phab.mercurial-scm.org/D2871

AFFECTED FILES
  mercurial/help/internals/wireprotocol.txt
  mercurial/wireprotoserver.py
  tests/test-http-api-httpv2.t

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel

Patch

diff --git a/tests/test-http-api-httpv2.t b/tests/test-http-api-httpv2.t
--- a/tests/test-http-api-httpv2.t
+++ b/tests/test-http-api-httpv2.t
@@ -412,4 +412,153 @@ 
   s>     received: <no frame>\n
   s>     {"action": "noop"}
 
+Multiple requests to regular command URL are not allowed
+
+  $ send << EOF
+  > httprequest POST api/$HTTPV2/ro/customreadonly
+  >     accept: $MEDIATYPE
+  >     content-type: $MEDIATYPE
+  >     user-agent: test
+  >     frame 1 command-name eos customreadonly
+  >     frame 3 command-name eos customreadonly
+  > EOF
+  using raw connection to peer
+  s>     POST /api/exp-http-v2-0001/ro/customreadonly HTTP/1.1\r\n
+  s>     Accept-Encoding: identity\r\n
+  s>     accept: application/mercurial-exp-framing-0002\r\n
+  s>     content-type: application/mercurial-exp-framing-0002\r\n
+  s>     user-agent: test\r\n
+  s>     content-length: 40\r\n
+  s>     host: $LOCALIP:$HGPORT\r\n (glob)
+  s>     \r\n
+  s>     \x0e\x00\x00\x01\x00\x11customreadonly\x0e\x00\x00\x03\x00\x11customreadonly
+  s> makefile('rb', None)
+  s>     HTTP/1.1 200 OK\r\n
+  s>     Server: testing stub value\r\n
+  s>     Date: $HTTP_DATE$\r\n
+  s>     Content-Type: text/plain\r\n
+  s>     Content-Length: 46\r\n
+  s>     \r\n
+  s>     multiple commands cannot be issued to this URL
+
+Multiple requests to "multirequest" URL are allowed
+
+  $ send << EOF
+  > httprequest POST api/$HTTPV2/ro/multirequest
+  >     accept: $MEDIATYPE
+  >     content-type: $MEDIATYPE
+  >     user-agent: test
+  >     frame 1 command-name eos customreadonly
+  >     frame 3 command-name eos customreadonly
+  > EOF
+  using raw connection to peer
+  s>     POST /api/exp-http-v2-0001/ro/multirequest HTTP/1.1\r\n
+  s>     Accept-Encoding: identity\r\n
+  s>     accept: application/mercurial-exp-framing-0002\r\n
+  s>     content-type: application/mercurial-exp-framing-0002\r\n
+  s>     user-agent: test\r\n
+  s>     *\r\n (glob)
+  s>     host: $LOCALIP:$HGPORT\r\n (glob)
+  s>     \r\n
+  s>     \x0e\x00\x00\x01\x00\x11customreadonly\x0e\x00\x00\x03\x00\x11customreadonly
+  s> makefile('rb', None)
+  s>     HTTP/1.1 200 OK\r\n
+  s>     Server: testing stub value\r\n
+  s>     Date: $HTTP_DATE$\r\n
+  s>     Content-Type: application/mercurial-exp-framing-0002\r\n
+  s>     Transfer-Encoding: chunked\r\n
+  s>     \r\n
+  s>     *\r\n (glob)
+  s>     \x1d\x00\x00\x01\x00Bcustomreadonly bytes response
+  s>     \r\n
+  s>     23\r\n
+  s>     \x1d\x00\x00\x03\x00Bcustomreadonly bytes response
+  s>     \r\n
+  s>     0\r\n
+  s>     \r\n
+
+Interleaved requests to "multirequest" are processed
+
+  $ send << EOF
+  > httprequest POST api/$HTTPV2/ro/multirequest
+  >     accept: $MEDIATYPE
+  >     content-type: $MEDIATYPE
+  >     user-agent: test
+  >     frame 1 command-name have-args listkeys
+  >     frame 3 command-name have-args listkeys
+  >     frame 3 command-argument eoa \x09\x00\x09\x00namespacebookmarks
+  >     frame 1 command-argument eoa \x09\x00\x0a\x00namespacenamespaces
+  > EOF
+  using raw connection to peer
+  s>     POST /api/exp-http-v2-0001/ro/multirequest HTTP/1.1\r\n
+  s>     Accept-Encoding: identity\r\n
+  s>     accept: application/mercurial-exp-framing-0002\r\n
+  s>     content-type: application/mercurial-exp-framing-0002\r\n
+  s>     user-agent: test\r\n
+  s>     content-length: 85\r\n
+  s>     host: $LOCALIP:$HGPORT\r\n (glob)
+  s>     \r\n
+  s>     \x08\x00\x00\x01\x00\x12listkeys\x08\x00\x00\x03\x00\x12listkeys\x16\x00\x00\x03\x00"	\x00	\x00namespacebookmarks\x17\x00\x00\x01\x00"	\x00\n
+  s>     \x00namespacenamespaces
+  s> makefile('rb', None)
+  s>     HTTP/1.1 200 OK\r\n
+  s>     Server: testing stub value\r\n
+  s>     Date: $HTTP_DATE$\r\n
+  s>     Content-Type: application/mercurial-exp-framing-0002\r\n
+  s>     Transfer-Encoding: chunked\r\n
+  s>     \r\n
+  s>     6\r\n
+  s>     \x00\x00\x00\x03\x00B
+  s>     \r\n
+  s>     24\r\n
+  s>     \x1e\x00\x00\x01\x00Bbookmarks	\n
+  s>     namespaces	\n
+  s>     phases	
+  s>     \r\n
+  s>     0\r\n
+  s>     \r\n
+
+Restart server to disable read-write access
+
+  $ killdaemons.py
+  $ cat > server/.hg/hgrc << EOF
+  > [experimental]
+  > web.apiserver = true
+  > web.api.debugreflect = true
+  > web.api.http-v2 = true
+  > [web]
+  > push_ssl = false
+  > EOF
+
+  $ hg -R server serve -p $HGPORT -d --pid-file hg.pid -E error.log
+  $ cat hg.pid > $DAEMON_PIDS
+
+Attempting to run a read-write command via multirequest on read-only URL is not allowed
+
+  $ send << EOF
+  > httprequest POST api/$HTTPV2/ro/multirequest
+  >     accept: $MEDIATYPE
+  >     content-type: $MEDIATYPE
+  >     user-agent: test
+  >     frame 1 command-name eos unbundle
+  > EOF
+  using raw connection to peer
+  s>     POST /api/exp-http-v2-0001/ro/multirequest HTTP/1.1\r\n
+  s>     Accept-Encoding: identity\r\n
+  s>     accept: application/mercurial-exp-framing-0002\r\n
+  s>     content-type: application/mercurial-exp-framing-0002\r\n
+  s>     user-agent: test\r\n
+  s>     content-length: 14\r\n
+  s>     host: $LOCALIP:$HGPORT\r\n (glob)
+  s>     \r\n
+  s>     \x08\x00\x00\x01\x00\x11unbundle
+  s> makefile('rb', None)
+  s>     HTTP/1.1 403 Forbidden\r\n
+  s>     Server: testing stub value\r\n
+  s>     Date: $HTTP_DATE$\r\n
+  s>     Content-Type: text/plain\r\n
+  s>     Content-Length: 53\r\n
+  s>     \r\n
+  s>     insufficient permissions to execute command: unbundle
+
   $ cat error.log
diff --git a/mercurial/wireprotoserver.py b/mercurial/wireprotoserver.py
--- a/mercurial/wireprotoserver.py
+++ b/mercurial/wireprotoserver.py
@@ -327,7 +327,12 @@ 
         _processhttpv2reflectrequest(rctx.repo.ui, rctx.repo, req, res)
         return
 
-    if command not in wireproto.commands:
+    # Extra commands that we handle that aren't really wire protocol
+    # commands. Think extra hard before making this hackery available to
+    # extension.
+    extracommands = {'multirequest'}
+
+    if command not in wireproto.commands and command not in extracommands:
         res.status = b'404 Not Found'
         res.headers[b'Content-Type'] = b'text/plain'
         res.setbodybytes(_('unknown wire protocol command: %s\n') % command)
@@ -338,7 +343,8 @@ 
 
     proto = httpv2protocolhandler(req, ui)
 
-    if not wireproto.commands.commandavailable(command, proto):
+    if (not wireproto.commands.commandavailable(command, proto)
+        and command not in extracommands):
         res.status = b'404 Not Found'
         res.headers[b'Content-Type'] = b'text/plain'
         res.setbodybytes(_('invalid wire protocol command: %s') % command)
@@ -434,18 +440,14 @@ 
             # Need more data before we can do anything.
             continue
         elif action == 'runcommand':
-            # We currently only support running a single command per
-            # HTTP request.
-            if seencommand:
-                # TODO define proper error mechanism.
-                res.status = b'200 OK'
-                res.headers[b'Content-Type'] = b'text/plain'
-                res.setbodybytes(_('support for multiple commands per request '
-                                   'not yet implemented'))
+            sentoutput = _httpv2runcommand(ui, repo, req, res, authedperm,
+                                           reqcommand, reactor, meta,
+                                           issubsequent=seencommand)
+
+            if sentoutput:
                 return
 
-            _httpv2runcommand(ui, repo, req, res, authedperm, reqcommand,
-                              reactor, meta)
+            seencommand = True
 
         elif action == 'error':
             # TODO define proper error mechanism.
@@ -471,7 +473,7 @@ 
                                      % action)
 
 def _httpv2runcommand(ui, repo, req, res, authedperm, reqcommand, reactor,
-                      command):
+                      command, issubsequent):
     """Dispatch a wire protocol command made from HTTPv2 requests.
 
     The authenticated permission (``authedperm``) along with the original
@@ -484,34 +486,57 @@ 
     # run doesn't have permissions requirements greater than what was granted
     # by ``authedperm``.
     #
-    # For now, this is no big deal, as we only allow a single command per
-    # request and that command must match the command in the URL. But when
-    # things change, we need to watch out...
-    if reqcommand != command['command']:
-        # TODO define proper error mechanism
-        res.status = b'200 OK'
-        res.headers[b'Content-Type'] = b'text/plain'
-        res.setbodybytes(_('command in frame must match command in URL'))
-        return
-
-    # TODO once we get rid of the command==URL restriction, we'll need to
-    # revalidate command validity and auth here. checkperm,
-    # wireproto.commands.commandavailable(), etc.
+    # Our rule for this is we only allow one command per HTTP request and
+    # that command must match the command in the URL. However, we make
+    # an exception for the ``multirequest`` URL. This URL is allowed to
+    # execute multiple commands. We double check permissions of each command
+    # as it is invoked to ensure there is no privilege escalation.
+    # TODO consider allowing multiple commands to regular command URLs
+    # iff each command is the same.
 
     proto = httpv2protocolhandler(req, ui, args=command['args'])
-    assert wireproto.commands.commandavailable(command['command'], proto)
-    wirecommand = wireproto.commands[command['command']]
 
-    assert authedperm in (b'ro', b'rw')
-    assert wirecommand.permission in ('push', 'pull')
+    if reqcommand == b'multirequest':
+        if not wireproto.commands.commandavailable(command['command'], proto):
+            # TODO proper error mechanism
+            res.status = b'200 OK'
+            res.headers[b'Content-Type'] = b'text/plain'
+            res.setbodybytes(_('wire protocol command not available: %s') %
+                             command['command'])
+            return True
+
+        assert authedperm in (b'ro', b'rw')
+        wirecommand = wireproto.commands[command['command']]
+        assert wirecommand.permission in ('push', 'pull')
 
-    # We already checked this as part of the URL==command check, but
-    # permissions are important, so do it again.
-    if authedperm == b'ro':
-        assert wirecommand.permission == 'pull'
-    elif authedperm == b'rw':
-        # We are allowed to access read-only commands under the rw URL.
-        assert wirecommand.permission in ('push', 'pull')
+        if authedperm == b'ro' and wirecommand.permission != 'pull':
+            # TODO proper error mechanism
+            res.status = b'403 Forbidden'
+            res.headers[b'Content-Type'] = b'text/plain'
+            res.setbodybytes(_('insufficient permissions to execute '
+                               'command: %s') % command['command'])
+            return True
+
+        # TODO should we also call checkperm() here? Maybe not if we're going
+        # to overhaul that API. The granted scope from the URL check should
+        # be good enough.
+
+    else:
+        # Don't allow multiple commands outside of ``multirequest`` URL.
+        if issubsequent:
+            # TODO proper error mechanism
+            res.status = b'200 OK'
+            res.headers[b'Content-Type'] = b'text/plain'
+            res.setbodybytes(_('multiple commands cannot be issued to this '
+                               'URL'))
+            return True
+
+        if reqcommand != command['command']:
+            # TODO define proper error mechanism
+            res.status = b'200 OK'
+            res.headers[b'Content-Type'] = b'text/plain'
+            res.setbodybytes(_('command in frame must match command in URL'))
+            return True
 
     rsp = wireproto.dispatch(repo, proto, command['command'])
 
@@ -527,6 +552,7 @@ 
 
     if action == 'sendframes':
         res.setbodygen(meta['framegen'])
+        return True
     elif action == 'noop':
         pass
     else:
diff --git a/mercurial/help/internals/wireprotocol.txt b/mercurial/help/internals/wireprotocol.txt
--- a/mercurial/help/internals/wireprotocol.txt
+++ b/mercurial/help/internals/wireprotocol.txt
@@ -203,6 +203,28 @@ 
 Servers receiving requests with an invalid ``Content-Type`` header SHOULD
 respond with an HTTP 415.
 
+The command to run is specified in the POST payload as defined by the
+*Unified Frame-Based Protocol*. This is redundant with data already
+encoded in the URL. This is by design, so server operators can have
+better understanding about server activity from looking merely at
+HTTP access logs.
+
+In most circumstances, the command specified in the URL MUST match
+the command specified in the frame-based payload or the server will
+respond with an error. The exception to this is the special
+``multirequest`` URL. (See below.) In addition, HTTP requests
+are limited to one command invocation. The exception is the special
+``multirequest`` URL.
+
+The ``multirequest`` command endpoints (``ro/multirequest`` and
+``rw/multirequest``) are special in that they allow the execution of
+*any* command and allow the execution of multiple commands. If the
+HTTP request issues multiple commands across multiple frames, all
+issued commands will be processed by the server. Per the defined
+behavior of the *Unified Frame-Based Protocol*, commands may be
+issued interleaved and responses may come back in a different order
+than they were issued. Clients MUST be able to deal with this.
+
 SSH Protocol
 ============