D2718: wireproto: declare permissions requirements in @wireprotocommand (API)
indygreg (Gregory Szorc)
phabricator at mercurial-scm.org
Thu Mar 8 00:25:06 UTC 2018
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.
REVISION SUMMARY
With the security patches from 4.5.2 merged into default, we now
have a per-command attribute defining what permissions are needed
to run that command. We now have a richer @wireprotocommand that
can be extended to record additional command metadata. So we
port the permissions mechanism to be based on @wireprotocommand.
.. api::
hgweb_mod.perms and wireproto.permissions have been removed. Wire
protocol commands should declare their required permissions in the
@wireprotocommand decorator.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D2718
AFFECTED FILES
hgext/largefiles/uisetup.py
mercurial/hgweb/hgweb_mod.py
mercurial/wireproto.py
mercurial/wireprotoserver.py
tests/test-http-permissions.t
CHANGE DETAILS
diff --git a/tests/test-http-permissions.t b/tests/test-http-permissions.t
--- a/tests/test-http-permissions.t
+++ b/tests/test-http-permissions.t
@@ -21,12 +21,10 @@
> @wireproto.wireprotocommand('customwritenoperm')
> def customwritenoperm(repo, proto):
> return b'write command no defined permissions\n'
- > wireproto.permissions['customreadwithperm'] = 'pull'
- > @wireproto.wireprotocommand('customreadwithperm')
+ > @wireproto.wireprotocommand('customreadwithperm', permission='pull')
> def customreadwithperm(repo, proto):
> return b'read-only command w/ defined permissions\n'
- > wireproto.permissions['customwritewithperm'] = 'push'
- > @wireproto.wireprotocommand('customwritewithperm')
+ > @wireproto.wireprotocommand('customwritewithperm', permission='push')
> def customwritewithperm(repo, proto):
> return b'write command w/ defined permissions\n'
> EOF
diff --git a/mercurial/wireprotoserver.py b/mercurial/wireprotoserver.py
--- a/mercurial/wireprotoserver.py
+++ b/mercurial/wireprotoserver.py
@@ -242,11 +242,7 @@
'over HTTP'))
return []
- # Assume commands with no defined permissions are writes /
- # for pushes. This is the safest from a security perspective
- # because it doesn't allow commands with undefined semantics
- # from bypassing permissions checks.
- checkperm(wireproto.permissions.get(cmd, 'push'))
+ checkperm(wireproto.commands[cmd].permission)
rsp = wireproto.dispatch(repo, proto, cmd)
diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -592,10 +592,12 @@
class commandentry(object):
"""Represents a declared wire protocol command."""
- def __init__(self, func, args='', transports=None):
+ def __init__(self, func, args='', transports=None,
+ permission='push'):
self.func = func
self.args = args
self.transports = transports or set()
+ self.permission = permission
def _merge(self, func, args):
"""Merge this instance with an incoming 2-tuple.
@@ -605,7 +607,8 @@
data not captured by the 2-tuple and a new instance containing
the union of the two objects is returned.
"""
- return commandentry(func, args=args, transports=set(self.transports))
+ return commandentry(func, args=args, transports=set(self.transports),
+ permission=self.permission)
# Old code treats instances as 2-tuples. So expose that interface.
def __iter__(self):
@@ -643,7 +646,8 @@
else:
# Use default values from @wireprotocommand.
v = commandentry(v[0], args=v[1],
- transports=set(wireprototypes.TRANSPORTS))
+ transports=set(wireprototypes.TRANSPORTS),
+ permission='push')
else:
raise ValueError('command entries must be commandentry instances '
'or 2-tuples')
@@ -672,12 +676,8 @@
commands = commanddict()
-# Maps wire protocol name to operation type. This is used for permissions
-# checking. All defined @wireiprotocommand should have an entry in this
-# dict.
-permissions = {}
-
-def wireprotocommand(name, args='', transportpolicy=POLICY_ALL):
+def wireprotocommand(name, args='', transportpolicy=POLICY_ALL,
+ permission='push'):
"""Decorator to declare a wire protocol command.
``name`` is the name of the wire protocol command being provided.
@@ -688,6 +688,12 @@
``transportpolicy`` is a POLICY_* constant denoting which transports
this wire protocol command should be exposed to. By default, commands
are exposed to all wire protocol transports.
+
+ ``permission`` defines the permission type needed to run this command.
+ Can be ``push`` or ``pull``. These roughly map to read-write and read-only,
+ respectively. Default is to assume command requires ``push`` permissions
+ because otherwise commands not declaring their permissions could modify
+ a repository that is supposed to be read-only.
"""
if transportpolicy == POLICY_ALL:
transports = set(wireprototypes.TRANSPORTS)
@@ -701,14 +707,18 @@
raise error.Abort(_('invalid transport policy value: %s') %
transportpolicy)
+ if permission not in ('push', 'pull'):
+ raise error.Abort(_('invalid wire protocol permission; got %s; '
+ 'expected "push" or "pull"') % permission)
+
def register(func):
- commands[name] = commandentry(func, args=args, transports=transports)
+ commands[name] = commandentry(func, args=args, transports=transports,
+ permission=permission)
return func
return register
# TODO define a more appropriate permissions type to use for this.
-permissions['batch'] = 'pull'
- at wireprotocommand('batch', 'cmds *')
+ at wireprotocommand('batch', 'cmds *', permission='pull')
def batch(repo, proto, cmds, others):
repo = repo.filtered("served")
res = []
@@ -725,11 +735,9 @@
# checking on each batched command.
# TODO formalize permission checking as part of protocol interface.
if util.safehasattr(proto, 'checkperm'):
- # Assume commands with no defined permissions are writes / for
- # pushes. This is the safest from a security perspective because
- # it doesn't allow commands with undefined semantics from
- # bypassing permissions checks.
- proto.checkperm(permissions.get(op, 'push'))
+ perm = commands[op].permission
+ assert perm in ('push', 'pull')
+ proto.checkperm(perm)
if spec:
keys = spec.split()
@@ -758,18 +766,17 @@
return bytesresponse(';'.join(res))
-permissions['between'] = 'pull'
- at wireprotocommand('between', 'pairs', transportpolicy=POLICY_V1_ONLY)
+ at wireprotocommand('between', 'pairs', transportpolicy=POLICY_V1_ONLY,
+ permission='pull')
def between(repo, proto, pairs):
pairs = [decodelist(p, '-') for p in pairs.split(" ")]
r = []
for b in repo.between(pairs):
r.append(encodelist(b) + "\n")
return bytesresponse(''.join(r))
-permissions['branchmap'] = 'pull'
- at wireprotocommand('branchmap')
+ at wireprotocommand('branchmap', permission='pull')
def branchmap(repo, proto):
branchmap = repo.branchmap()
heads = []
@@ -780,18 +787,17 @@
return bytesresponse('\n'.join(heads))
-permissions['branches'] = 'pull'
- at wireprotocommand('branches', 'nodes', transportpolicy=POLICY_V1_ONLY)
+ at wireprotocommand('branches', 'nodes', transportpolicy=POLICY_V1_ONLY,
+ permission='pull')
def branches(repo, proto, nodes):
nodes = decodelist(nodes)
r = []
for b in repo.branches(nodes):
r.append(encodelist(b) + "\n")
return bytesresponse(''.join(r))
-permissions['clonebundles'] = 'pull'
- at wireprotocommand('clonebundles', '')
+ at wireprotocommand('clonebundles', '', permission='pull')
def clonebundles(repo, proto):
"""Server command for returning info for available bundles to seed clones.
@@ -843,24 +849,23 @@
# If you are writing an extension and consider wrapping this function. Wrap
# `_capabilities` instead.
-permissions['capabilities'] = 'pull'
- at wireprotocommand('capabilities')
+ at wireprotocommand('capabilities', permission='pull')
def capabilities(repo, proto):
return bytesresponse(' '.join(_capabilities(repo, proto)))
-permissions['changegroup'] = 'pull'
- at wireprotocommand('changegroup', 'roots', transportpolicy=POLICY_V1_ONLY)
+ at wireprotocommand('changegroup', 'roots', transportpolicy=POLICY_V1_ONLY,
+ permission='pull')
def changegroup(repo, proto, roots):
nodes = decodelist(roots)
outgoing = discovery.outgoing(repo, missingroots=nodes,
missingheads=repo.heads())
cg = changegroupmod.makechangegroup(repo, outgoing, '01', 'serve')
gen = iter(lambda: cg.read(32768), '')
return streamres(gen=gen)
-permissions['changegroupsubset'] = 'pull'
@wireprotocommand('changegroupsubset', 'bases heads',
- transportpolicy=POLICY_V1_ONLY)
+ transportpolicy=POLICY_V1_ONLY,
+ permission='pull')
def changegroupsubset(repo, proto, bases, heads):
bases = decodelist(bases)
heads = decodelist(heads)
@@ -870,16 +875,15 @@
gen = iter(lambda: cg.read(32768), '')
return streamres(gen=gen)
-permissions['debugwireargs'] = 'pull'
- at wireprotocommand('debugwireargs', 'one two *')
+ at wireprotocommand('debugwireargs', 'one two *',
+ permission='pull')
def debugwireargs(repo, proto, one, two, others):
# only accept optional args from the known set
opts = options('debugwireargs', ['three', 'four'], others)
return bytesresponse(repo.debugwireargs(one, two,
**pycompat.strkwargs(opts)))
-permissions['getbundle'] = 'pull'
- at wireprotocommand('getbundle', '*')
+ at wireprotocommand('getbundle', '*', permission='pull')
def getbundle(repo, proto, others):
opts = options('getbundle', gboptsmap.keys(), others)
for k, v in opts.iteritems():
@@ -945,14 +949,12 @@
return streamres(gen=chunks, prefer_uncompressed=not prefercompressed)
-permissions['heads'] = 'pull'
- at wireprotocommand('heads')
+ at wireprotocommand('heads', permission='pull')
def heads(repo, proto):
h = repo.heads()
return bytesresponse(encodelist(h) + '\n')
-permissions['hello'] = 'pull'
- at wireprotocommand('hello')
+ at wireprotocommand('hello', permission='pull')
def hello(repo, proto):
"""Called as part of SSH handshake to obtain server info.
@@ -967,14 +969,12 @@
caps = capabilities(repo, proto).data
return bytesresponse('capabilities: %s\n' % caps)
-permissions['listkeys'] = 'pull'
- at wireprotocommand('listkeys', 'namespace')
+ at wireprotocommand('listkeys', 'namespace', permission='pull')
def listkeys(repo, proto, namespace):
d = sorted(repo.listkeys(encoding.tolocal(namespace)).items())
return bytesresponse(pushkeymod.encodekeys(d))
-permissions['lookup'] = 'pull'
- at wireprotocommand('lookup', 'key')
+ at wireprotocommand('lookup', 'key', permission='pull')
def lookup(repo, proto, key):
try:
k = encoding.tolocal(key)
@@ -986,14 +986,12 @@
success = 0
return bytesresponse('%d %s\n' % (success, r))
-permissions['known'] = 'pull'
- at wireprotocommand('known', 'nodes *')
+ at wireprotocommand('known', 'nodes *', permission='pull')
def known(repo, proto, nodes, others):
v = ''.join(b and '1' or '0' for b in repo.known(decodelist(nodes)))
return bytesresponse(v)
-permissions['pushkey'] = 'push'
- at wireprotocommand('pushkey', 'namespace key old new')
+ at wireprotocommand('pushkey', 'namespace key old new', permission='push')
def pushkey(repo, proto, namespace, key, old, new):
# compatibility with pre-1.8 clients which were accidentally
# sending raw binary nodes rather than utf-8-encoded hex
@@ -1014,17 +1012,15 @@
output = output.getvalue() if output else ''
return bytesresponse('%d\n%s' % (int(r), output))
-permissions['stream_out'] = 'pull'
- at wireprotocommand('stream_out')
+ at wireprotocommand('stream_out', permission='pull')
def stream(repo, proto):
'''If the server supports streaming clone, it advertises the "stream"
capability with a value representing the version and flags of the repo
it is serving. Client checks to see if it understands the format.
'''
return streamres_legacy(streamclone.generatev1wireproto(repo))
-permissions['unbundle'] = 'push'
- at wireprotocommand('unbundle', 'heads')
+ at wireprotocommand('unbundle', 'heads', permission='push')
def unbundle(repo, proto, heads):
their_heads = decodelist(heads)
diff --git a/mercurial/hgweb/hgweb_mod.py b/mercurial/hgweb/hgweb_mod.py
--- a/mercurial/hgweb/hgweb_mod.py
+++ b/mercurial/hgweb/hgweb_mod.py
@@ -37,7 +37,6 @@
templater,
ui as uimod,
util,
- wireproto,
wireprotoserver,
)
@@ -47,9 +46,6 @@
wsgicgi,
)
-# Aliased for API compatibility.
-perms = wireproto.permissions
-
archivespecs = util.sortdict((
('zip', ('application/zip', 'zip', '.zip', None)),
('gz', ('application/x-gzip', 'tgz', '.tar.gz', None)),
diff --git a/hgext/largefiles/uisetup.py b/hgext/largefiles/uisetup.py
--- a/hgext/largefiles/uisetup.py
+++ b/hgext/largefiles/uisetup.py
@@ -164,21 +164,18 @@
overrides.openlargefile)
# create the new wireproto commands ...
- wireproto.wireprotocommand('putlfile', 'sha')(proto.putlfile)
- wireproto.wireprotocommand('getlfile', 'sha')(proto.getlfile)
- wireproto.wireprotocommand('statlfile', 'sha')(proto.statlfile)
- wireproto.wireprotocommand('lheads', '')(wireproto.heads)
+ wireproto.wireprotocommand('putlfile', 'sha', permission='push')(
+ proto.putlfile)
+ wireproto.wireprotocommand('getlfile', 'sha', permission='pull')(
+ proto.getlfile)
+ wireproto.wireprotocommand('statlfile', 'sha', permission='pull')(
+ proto.statlfile)
+ wireproto.wireprotocommand('lheads', '', permission='pull')(
+ wireproto.heads)
# ... and wrap some existing ones
wireproto.commands['heads'].func = proto.heads
- # make putlfile behave the same as push and {get,stat}lfile behave
- # the same as pull w.r.t. permissions checks
- wireproto.permissions['putlfile'] = 'push'
- wireproto.permissions['getlfile'] = 'pull'
- wireproto.permissions['statlfile'] = 'pull'
- wireproto.permissions['lheads'] = 'pull'
-
extensions.wrapfunction(webcommands, 'decodepath', overrides.decodepath)
extensions.wrapfunction(wireproto, '_capabilities', proto._capabilities)
To: indygreg, #hg-reviewers
Cc: mercurial-devel
More information about the Mercurial-devel
mailing list