This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2837

wireproto: require POST for all HTTPv2 requests
ClosedPublic

Authored by indygreg on Mar 12 2018, 10:16 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHGfc5e261915b9: wireproto: require POST for all HTTPv2 requests
Summary

Wire protocol version 1 transfers argument data via request
headers by default. This has historically caused problems because
servers institute limits on the length of individual HTTP headers
as well as the total size of all request headers. Mercurial servers
can advertise the maximum length of an individual header. But
there's no guarantee any intermediate HTTP agents will accept
headers up to that length.

In the existing wire protocol, server operators typically also
key off the HTTP request method to implement authentication.
For example, GET requests translate to read-only requests and
can be allowed. But read-write commands must use POST and require
authentication. This has typically worked because the only wire
protocol commands that use POST modify the repo (e.g. the
"unbundle" command).

There is an experimental feature to enable clients to transmit
argument data via POST request bodies. This is technically a
better and more robust solution. But we can't enable it by default
because of servers assuming POST means write access.

In version 2 of the wire protocol, the permissions of a request
are encoded in the URL. And with it being a new protocol in a new
URL space, we're not constrained by backwards compatibility
requirements.

This commit adopts the technically superior mechanism of using
HTTP request bodies to send argument data by requiring POST for
all commands. Strictly speaking, it may be possible to send
request bodies on GET requests. But my experience is that not all
HTTP stacks support this. POST pretty much always works. Using POST
for read-only operations does sacrifice some RESTful design
purity. But this API cares about practicality, not about being
in Roy T. Fielding's REST ivory tower.

There's a chance we may relax this restriction in the future. But
for now, I want to see how far we can get with a POST only API.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Mar 12 2018, 10:16 PM
indygreg updated this revision to Diff 7000.Mar 13 2018, 3:11 PM
indygreg updated this revision to Diff 7015.Mar 13 2018, 10:49 PM
indygreg updated this revision to Diff 7136.Mar 19 2018, 7:59 PM
durin42 accepted this revision.Mar 21 2018, 3:18 PM
This revision is now accepted and ready to land.Mar 21 2018, 3:18 PM
indygreg updated this revision to Diff 7196.Mar 21 2018, 6:19 PM
durin42 accepted this revision.Mar 21 2018, 6:21 PM
Closed by commit rHGfc5e261915b9: wireproto: require POST for all HTTPv2 requests (authored by indygreg). · Explain WhyMar 21 2018, 8:58 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 7196

mercurial/help/internals/wireprotocol.txt

HTTP Version 2 Transport HTTP Version 2 Transport
------------------------ ------------------------
**Experimental - feature under active development** **Experimental - feature under active development**
Version 2 of the HTTP protocol is exposed under the ``/api/*`` URL space. Version 2 of the HTTP protocol is exposed under the ``/api/*`` URL space.
It's final API name is not yet formalized. It's final API name is not yet formalized.
Commands are triggered by sending HTTP requests against URLs of the Commands are triggered by sending HTTP POST requests against URLs of the
form ``<permission>/<command>``, where ``<permission>`` is ``ro`` or form ``<permission>/<command>``, where ``<permission>`` is ``ro`` or
``rw``, meaning read-only and read-write, respectively and ``<command>`` ``rw``, meaning read-only and read-write, respectively and ``<command>``
is a named wire protocol command. is a named wire protocol command.
Non-POST request methods MUST be rejected by the server with an HTTP
405 response.
Commands that modify repository state in meaningful ways MUST NOT be Commands that modify repository state in meaningful ways MUST NOT be
exposed under the ``ro`` URL prefix. All available commands MUST be exposed under the ``ro`` URL prefix. All available commands MUST be
available under the ``rw`` URL prefix. available under the ``rw`` URL prefix.
Server adminstrators MAY implement blanket HTTP authentication keyed Server adminstrators MAY implement blanket HTTP authentication keyed
off the URL prefix. For example, a server may require authentication off the URL prefix. For example, a server may require authentication
for all ``rw/*`` URLs and let unauthenticated requests to ``ro/*`` for all ``rw/*`` URLs and let unauthenticated requests to ``ro/*``
URL proceed. A server MAY issue an HTTP 401, 403, or 407 response URL proceed. A server MAY issue an HTTP 401, 403, or 407 response

mercurial/wireprotoserver.py

permission, command = urlparts[0:2] permission, command = urlparts[0:2]
if permission not in (b'ro', b'rw'): if permission not in (b'ro', b'rw'):
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 permission: %s') % permission) res.setbodybytes(_('unknown permission: %s') % permission)
return return
if req.method != 'POST':
res.status = b'405 Method Not Allowed'
res.headers[b'Allow'] = b'POST'
res.setbodybytes(_('commands require POST requests'))
return
# At some point we'll want to use our own API instead of recycling the # At some point we'll want to use our own API instead of recycling the
# behavior of version 1 of the wire protocol... # behavior of version 1 of the wire protocol...
# TODO return reasonable responses - not responses that overload the # TODO return reasonable responses - not responses that overload the
# HTTP status line message for error reporting. # HTTP status line message for error reporting.
try: try:
checkperm(rctx, req, 'pull' if permission == b'ro' else 'push') checkperm(rctx, req, 'pull' if permission == b'ro' else 'push')
except hgwebcommon.ErrorResponse as e: except hgwebcommon.ErrorResponse as e:
res.status = hgwebcommon.statusmessage(e.code, pycompat.bytestr(e)) res.status = hgwebcommon.statusmessage(e.code, pycompat.bytestr(e))

tests/test-http-api-httpv2.t

> EOF > EOF
$ hg -R server serve -p $HGPORT -d --pid-file hg.pid $ hg -R server serve -p $HGPORT -d --pid-file hg.pid
$ cat hg.pid > $DAEMON_PIDS $ cat hg.pid > $DAEMON_PIDS
Request to read-only command works out of the box Request to read-only command works out of the box
$ send << EOF $ send << EOF
> httprequest GET api/$HTTPV2/ro/customreadonly > httprequest POST api/$HTTPV2/ro/customreadonly
> user-agent: test > user-agent: test
> EOF > EOF
using raw connection to peer using raw connection to peer
s> GET /api/exp-http-v2-0001/ro/customreadonly HTTP/1.1\r\n s> POST /api/exp-http-v2-0001/ro/customreadonly HTTP/1.1\r\n
s> Accept-Encoding: identity\r\n s> Accept-Encoding: identity\r\n
s> user-agent: test\r\n s> user-agent: test\r\n
s> host: $LOCALIP:$HGPORT\r\n (glob) s> host: $LOCALIP:$HGPORT\r\n (glob)
s> \r\n s> \r\n
s> makefile('rb', None) s> makefile('rb', None)
s> HTTP/1.1 200 OK\r\n s> HTTP/1.1 200 OK\r\n
s> Server: testing stub value\r\n s> Server: testing stub value\r\n
s> Date: $HTTP_DATE$\r\n s> Date: $HTTP_DATE$\r\n
s> Content-Type: text/plain\r\n s> Content-Type: text/plain\r\n
s> Content-Length: 18\r\n s> Content-Length: 18\r\n
s> \r\n s> \r\n
s> ro/customreadonly\n s> ro/customreadonly\n
Request to unknown command yields 404 Request to unknown command yields 404
$ send << EOF $ send << EOF
> httprequest GET api/$HTTPV2/ro/badcommand > httprequest POST api/$HTTPV2/ro/badcommand
> user-agent: test > user-agent: test
> EOF > EOF
using raw connection to peer using raw connection to peer
s> GET /api/exp-http-v2-0001/ro/badcommand HTTP/1.1\r\n s> POST /api/exp-http-v2-0001/ro/badcommand HTTP/1.1\r\n
s> Accept-Encoding: identity\r\n s> Accept-Encoding: identity\r\n
s> user-agent: test\r\n s> user-agent: test\r\n
s> host: $LOCALIP:$HGPORT\r\n (glob) s> host: $LOCALIP:$HGPORT\r\n (glob)
s> \r\n s> \r\n
s> makefile('rb', None) s> makefile('rb', None)
s> HTTP/1.1 404 Not Found\r\n s> HTTP/1.1 404 Not Found\r\n
s> Server: testing stub value\r\n s> Server: testing stub value\r\n
s> Date: $HTTP_DATE$\r\n s> Date: $HTTP_DATE$\r\n
s> Content-Type: text/plain\r\n s> Content-Type: text/plain\r\n
s> Content-Length: 42\r\n s> Content-Length: 42\r\n
s> \r\n s> \r\n
s> unknown wire protocol command: badcommand\n s> unknown wire protocol command: badcommand\n
Request to read-write command fails because server is read-only by default GET to read-only command yields a 405
GET to read-write request not allowed
$ send << EOF $ send << EOF
> httprequest GET api/$HTTPV2/rw/customreadonly > httprequest GET api/$HTTPV2/ro/customreadonly
> user-agent: test > user-agent: test
> EOF > EOF
using raw connection to peer using raw connection to peer
s> GET /api/exp-http-v2-0001/rw/customreadonly HTTP/1.1\r\n s> GET /api/exp-http-v2-0001/ro/customreadonly HTTP/1.1\r\n
s> Accept-Encoding: identity\r\n s> Accept-Encoding: identity\r\n
s> user-agent: test\r\n s> user-agent: test\r\n
s> host: $LOCALIP:$HGPORT\r\n (glob) s> host: $LOCALIP:$HGPORT\r\n (glob)
s> \r\n s> \r\n
s> makefile('rb', None) s> makefile('rb', None)
s> HTTP/1.1 405 push requires POST request\r\n s> HTTP/1.1 405 Method Not Allowed\r\n
s> Server: testing stub value\r\n s> Server: testing stub value\r\n
s> Date: $HTTP_DATE$\r\n s> Date: $HTTP_DATE$\r\n
s> Content-Length: 17\r\n s> Allow: POST\r\n
s> Content-Length: 30\r\n
s> \r\n s> \r\n
s> permission denied s> commands require POST requests
Even for unknown commands Request to read-write command fails because server is read-only by default
GET to read-write request yields 405
$ send << EOF $ send << EOF
> httprequest GET api/$HTTPV2/rw/badcommand > httprequest GET api/$HTTPV2/rw/customreadonly
> user-agent: test > user-agent: test
> EOF > EOF
using raw connection to peer using raw connection to peer
s> GET /api/exp-http-v2-0001/rw/badcommand HTTP/1.1\r\n s> GET /api/exp-http-v2-0001/rw/customreadonly HTTP/1.1\r\n
s> Accept-Encoding: identity\r\n s> Accept-Encoding: identity\r\n
s> user-agent: test\r\n s> user-agent: test\r\n
s> host: $LOCALIP:$HGPORT\r\n (glob) s> host: $LOCALIP:$HGPORT\r\n (glob)
s> \r\n s> \r\n
s> makefile('rb', None) s> makefile('rb', None)
s> HTTP/1.1 405 push requires POST request\r\n s> HTTP/1.1 405 Method Not Allowed\r\n
s> Server: testing stub value\r\n s> Server: testing stub value\r\n
s> Date: $HTTP_DATE$\r\n s> Date: $HTTP_DATE$\r\n
s> Content-Length: 17\r\n s> Allow: POST\r\n
s> Content-Length: 30\r\n
s> \r\n s> \r\n
s> permission denied s> commands require POST requests
SSL required by default Even for unknown commands
$ send << EOF $ send << EOF
> httprequest POST api/$HTTPV2/rw/customreadonly > httprequest GET api/$HTTPV2/rw/badcommand
> user-agent: test > user-agent: test
> EOF > EOF
using raw connection to peer using raw connection to peer
s> POST /api/exp-http-v2-0001/rw/customreadonly HTTP/1.1\r\n s> GET /api/exp-http-v2-0001/rw/badcommand HTTP/1.1\r\n
s> Accept-Encoding: identity\r\n s> Accept-Encoding: identity\r\n
s> user-agent: test\r\n s> user-agent: test\r\n
s> host: $LOCALIP:$HGPORT\r\n (glob) s> host: $LOCALIP:$HGPORT\r\n (glob)
s> \r\n s> \r\n
s> makefile('rb', None) s> makefile('rb', None)
s> HTTP/1.1 403 ssl required\r\n s> HTTP/1.1 405 Method Not Allowed\r\n
s> Server: testing stub value\r\n s> Server: testing stub value\r\n
s> Date: $HTTP_DATE$\r\n s> Date: $HTTP_DATE$\r\n
s> Content-Length: 17\r\n s> Allow: POST\r\n
s> Content-Length: 30\r\n
s> \r\n s> \r\n
s> permission denied s> commands require POST requests
Restart server to allow non-ssl read-write operations SSL required by default
$ killdaemons.py
$ cat > server/.hg/hgrc << EOF
> [experimental]
> web.apiserver = true
> web.api.http-v2 = true
> [web]
> push_ssl = false
> EOF
$ hg -R server serve -p $HGPORT -d --pid-file hg.pid
$ cat hg.pid > $DAEMON_PIDS
Server insists on POST for read-write commands
$ send << EOF $ send << EOF
> httprequest GET api/$HTTPV2/rw/customreadonly > httprequest POST api/$HTTPV2/rw/customreadonly
> user-agent: test > user-agent: test
> EOF > EOF
using raw connection to peer using raw connection to peer
s> GET /api/exp-http-v2-0001/rw/customreadonly HTTP/1.1\r\n s> POST /api/exp-http-v2-0001/rw/customreadonly HTTP/1.1\r\n
s> Accept-Encoding: identity\r\n s> Accept-Encoding: identity\r\n
s> user-agent: test\r\n s> user-agent: test\r\n
s> host: $LOCALIP:$HGPORT\r\n (glob) s> host: $LOCALIP:$HGPORT\r\n (glob)
s> \r\n s> \r\n
s> makefile('rb', None) s> makefile('rb', None)
s> HTTP/1.1 405 push requires POST request\r\n s> HTTP/1.1 403 ssl required\r\n
s> Server: testing stub value\r\n s> Server: testing stub value\r\n
s> Date: $HTTP_DATE$\r\n s> Date: $HTTP_DATE$\r\n
s> Content-Length: 17\r\n s> Content-Length: 17\r\n
s> \r\n s> \r\n
s> permission denied s> permission denied
Restart server to allow non-ssl read-write operations
$ killdaemons.py $ killdaemons.py
$ cat > server/.hg/hgrc << EOF $ cat > server/.hg/hgrc << EOF
> [experimental] > [experimental]
> web.apiserver = true > web.apiserver = true
> web.api.http-v2 = true > web.api.http-v2 = true
> [web] > [web]
> push_ssl = false > push_ssl = false
> allow-push = * > allow-push = *