This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2871

wireproto: service multiple command requests per HTTP request
ClosedPublic

Authored by indygreg on Mar 14 2018, 9:10 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHGbbea991635d0: wireproto: service multiple command requests per HTTP request
Summary

Now that our new frame-based protocol server can understand how
to ingest multiple, possibly interleaved, command requests, let's
hook it up to the HTTP server.

The code on the HTTP side of things is still a bit hacky. We need
a bit of work around error handling, content types, etc. But it's
a start.

Among the added tests, we demonstrate that a client can send frames
for multiple commands iterleaved with each other and that a later
issued command can respond before the first one has finished
sending. This makes our multi-request model technically superior
to the previous "batch" command.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Mar 14 2018, 9:10 PM
indygreg updated this revision to Diff 7147.Mar 19 2018, 7:59 PM
durin42 added a subscriber: durin42.Mar 21 2018, 9:26 PM
durin42 added inline comments.
mercurial/wireprotoserver.py
508

worth not using assert here? I don't think this is attacker-controlled?

durin42 accepted this revision.Mar 21 2018, 9:33 PM
This revision is now accepted and ready to land.Mar 21 2018, 9:33 PM
Closed by commit rHGbbea991635d0: wireproto: service multiple command requests per HTTP request (authored by indygreg). · Explain WhyMar 21 2018, 9:41 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Mar 25 2018, 12:38 AM
yuja added inline comments.
mercurial/wireprotoserver.py
557

Nit: return False instead of returning None?

Revision Contents
Changeset List

Diff 7235

mercurial/help/internals/wireprotocol.txt

TODO finalize the media type. For now, it is defined in wireprotoserver.py. TODO finalize the media type. For now, it is defined in wireprotoserver.py.
Servers receiving requests without an ``Accept`` header SHOULD respond with Servers receiving requests without an ``Accept`` header SHOULD respond with
an HTTP 406. an HTTP 406.
Servers receiving requests with an invalid ``Content-Type`` header SHOULD Servers receiving requests with an invalid ``Content-Type`` header SHOULD
respond with an HTTP 415. 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 SSH Protocol
============ ============
Handshake Handshake
--------- ---------
For all clients, the handshake consists of the client sending 1 or more For all clients, the handshake consists of the client sending 1 or more
commands to the server using version 1 of the transport. Servers respond commands to the server using version 1 of the transport. Servers respond

mercurial/wireprotoserver.py

res.setbodybytes('permission denied') res.setbodybytes('permission denied')
return return
# We have a special endpoint to reflect the request back at the client. # We have a special endpoint to reflect the request back at the client.
if command == b'debugreflect': if command == b'debugreflect':
_processhttpv2reflectrequest(rctx.repo.ui, rctx.repo, req, res) _processhttpv2reflectrequest(rctx.repo.ui, rctx.repo, req, res)
return 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.status = b'404 Not Found'
res.headers[b'Content-Type'] = b'text/plain' res.headers[b'Content-Type'] = b'text/plain'
res.setbodybytes(_('unknown wire protocol command: %s\n') % command) res.setbodybytes(_('unknown wire protocol command: %s\n') % command)
return return
repo = rctx.repo repo = rctx.repo
ui = repo.ui ui = repo.ui
proto = httpv2protocolhandler(req, ui) 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.status = b'404 Not Found'
res.headers[b'Content-Type'] = b'text/plain' res.headers[b'Content-Type'] = b'text/plain'
res.setbodybytes(_('invalid wire protocol command: %s') % command) res.setbodybytes(_('invalid wire protocol command: %s') % command)
return return
if req.headers.get(b'Accept') != FRAMINGTYPE: if req.headers.get(b'Accept') != FRAMINGTYPE:
res.status = b'406 Not Acceptable' res.status = b'406 Not Acceptable'
res.headers[b'Content-Type'] = b'text/plain' res.headers[b'Content-Type'] = b'text/plain'
break break
action, meta = reactor.onframerecv(*frame) action, meta = reactor.onframerecv(*frame)
if action == 'wantframe': if action == 'wantframe':
# Need more data before we can do anything. # Need more data before we can do anything.
continue continue
elif action == 'runcommand': elif action == 'runcommand':
# We currently only support running a single command per sentoutput = _httpv2runcommand(ui, repo, req, res, authedperm,
# HTTP request. reqcommand, reactor, meta,
if seencommand: issubsequent=seencommand)
# TODO define proper error mechanism.
res.status = b'200 OK' if sentoutput:
res.headers[b'Content-Type'] = b'text/plain'
res.setbodybytes(_('support for multiple commands per request '
'not yet implemented'))
return return
_httpv2runcommand(ui, repo, req, res, authedperm, reqcommand, seencommand = True
reactor, meta)
elif action == 'error': elif action == 'error':
# TODO define proper error mechanism. # TODO define proper error mechanism.
res.status = b'200 OK' res.status = b'200 OK'
res.headers[b'Content-Type'] = b'text/plain' res.headers[b'Content-Type'] = b'text/plain'
res.setbodybytes(meta['message'] + b'\n') res.setbodybytes(meta['message'] + b'\n')
return return
else: else:
res.setbodygen(meta['framegen']) res.setbodygen(meta['framegen'])
elif action == 'noop': elif action == 'noop':
pass pass
else: else:
raise error.ProgrammingError('unhandled action from frame processor: %s' raise error.ProgrammingError('unhandled action from frame processor: %s'
% action) % action)
def _httpv2runcommand(ui, repo, req, res, authedperm, reqcommand, reactor, def _httpv2runcommand(ui, repo, req, res, authedperm, reqcommand, reactor,
command): command, issubsequent):
"""Dispatch a wire protocol command made from HTTPv2 requests. """Dispatch a wire protocol command made from HTTPv2 requests.
The authenticated permission (``authedperm``) along with the original The authenticated permission (``authedperm``) along with the original
command from the URL (``reqcommand``) are passed in. command from the URL (``reqcommand``) are passed in.
""" """
# We already validated that the session has permissions to perform the # We already validated that the session has permissions to perform the
# actions in ``authedperm``. In the unified frame protocol, the canonical # actions in ``authedperm``. In the unified frame protocol, the canonical
# command to run is expressed in a frame. However, the URL also requested # command to run is expressed in a frame. However, the URL also requested
# to run a specific command. We need to be careful that the command we # to run a specific command. We need to be careful that the command we
# run doesn't have permissions requirements greater than what was granted # run doesn't have permissions requirements greater than what was granted
# by ``authedperm``. # by ``authedperm``.
# #
# For now, this is no big deal, as we only allow a single command per # Our rule for this is we only allow one command per HTTP request and
# request and that command must match the command in the URL. But when # that command must match the command in the URL. However, we make
# things change, we need to watch out... # an exception for the ``multirequest`` URL. This URL is allowed to
if reqcommand != command['command']: # execute multiple commands. We double check permissions of each command
# TODO define proper error mechanism # as it is invoked to ensure there is no privilege escalation.
res.status = b'200 OK' # TODO consider allowing multiple commands to regular command URLs
res.headers[b'Content-Type'] = b'text/plain' # iff each command is the same.
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.
proto = httpv2protocolhandler(req, ui, args=command['args']) proto = httpv2protocolhandler(req, ui, args=command['args'])
assert wireproto.commands.commandavailable(command['command'], proto)
wirecommand = wireproto.commands[command['command']] 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') assert authedperm in (b'ro', b'rw')
durin42Unsubmitted
Not Done

worth not using assert here? I don't think this is attacker-controlled?

durin42: worth not using assert here? I don't think this is attacker-controlled?
wirecommand = wireproto.commands[command['command']]
assert wirecommand.permission in ('push', 'pull') assert wirecommand.permission in ('push', 'pull')
# We already checked this as part of the URL==command check, but if authedperm == b'ro' and wirecommand.permission != 'pull':
# permissions are important, so do it again. # TODO proper error mechanism
if authedperm == b'ro': res.status = b'403 Forbidden'
assert wirecommand.permission == 'pull' res.headers[b'Content-Type'] = b'text/plain'
elif authedperm == b'rw': res.setbodybytes(_('insufficient permissions to execute '
# We are allowed to access read-only commands under the rw URL. 'command: %s') % command['command'])
assert wirecommand.permission in ('push', 'pull') 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']) rsp = wireproto.dispatch(repo, proto, command['command'])
res.status = b'200 OK' res.status = b'200 OK'
res.headers[b'Content-Type'] = FRAMINGTYPE res.headers[b'Content-Type'] = FRAMINGTYPE
if isinstance(rsp, wireprototypes.bytesresponse): if isinstance(rsp, wireprototypes.bytesresponse):
action, meta = reactor.onbytesresponseready(command['requestid'], action, meta = reactor.onbytesresponseready(command['requestid'],
rsp.data) rsp.data)
else: else:
action, meta = reactor.onapplicationerror( action, meta = reactor.onapplicationerror(
_('unhandled response type from wire proto command')) _('unhandled response type from wire proto command'))
if action == 'sendframes': if action == 'sendframes':
res.setbodygen(meta['framegen']) res.setbodygen(meta['framegen'])
return True
elif action == 'noop': elif action == 'noop':
pass pass
yujaUnsubmitted
Not Done

Nit: return False instead of returning None?

yuja: Nit: `return False` instead of returning None?
else: else:
raise error.ProgrammingError('unhandled event from reactor: %s' % raise error.ProgrammingError('unhandled event from reactor: %s' %
action) action)
# Maps API name to metadata so custom API can be registered. # Maps API name to metadata so custom API can be registered.
API_HANDLERS = { API_HANDLERS = {
HTTPV2: { HTTPV2: {
'config': ('experimental', 'web.api.http-v2'), 'config': ('experimental', 'web.api.http-v2'),

tests/test-http-api-httpv2.t

s> ["wantframe", {"state": "command-receiving"}]\n s> ["wantframe", {"state": "command-receiving"}]\n
s> received: 2 0 1 \x03\x00\x04\x00fooval1\n s> received: 2 0 1 \x03\x00\x04\x00fooval1\n
s> ["wantframe", {"state": "command-receiving"}]\n s> ["wantframe", {"state": "command-receiving"}]\n
s> received: 2 2 1 \x04\x00\x03\x00bar1val\n s> received: 2 2 1 \x04\x00\x03\x00bar1val\n
s> ["runcommand", {"args": {"bar1": "val", "foo": "val1"}, "command": "command1", "data": null, "requestid": 1}]\n s> ["runcommand", {"args": {"bar1": "val", "foo": "val1"}, "command": "command1", "data": null, "requestid": 1}]\n
s> received: <no frame>\n s> received: <no frame>\n
s> {"action": "noop"} 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 $ cat error.log