D2871: wireproto: service multiple command requests per HTTP request
indygreg (Gregory Szorc)
phabricator at mercurial-scm.org
Mon Mar 19 20:01:15 EDT 2018
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
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
============
To: indygreg, #hg-reviewers
Cc: mercurial-devel
More information about the Mercurial-devel
mailing list