[PATCH 2 of 2 v2] hg-ssh: more flexible permissions for hg-ssh

Mads mads at kiilerich.com
Mon May 21 20:02:54 CDT 2012


On 22/05/12 01:29, David Schleimer wrote:
> # HG changeset patch
> # User David Schleimer<dschleimer at fb.com>
> # Date 1337642370 25200
> # Node ID 6be86d4b3a0c424272600164500b6329b43ab946
> # Parent  b52b7fe0dd08b257dfc69c8a5de503cec94f4b76
> hg-ssh: more flexible permissions for hg-ssh
>
> This allows more flexible control over the permissions granted to a
> ssh key when using hg-ssh as the command in an authorized_keys file.
>
> Specifically, it allows you to restrict a key to read-only access, as
> well as allowing you to grant a key access to any repo, instead of
> needing to whitelist repos.
>
> diff --git a/contrib/hg-ssh b/contrib/hg-ssh
> --- a/contrib/hg-ssh
> +++ b/contrib/hg-ssh
> @@ -35,9 +35,21 @@
>
>   def main():
>       cwd = os.getcwd()
> +    allrepos = False
> +    readonly = False
> +    args = sys.argv[1:]
> +    while len(args):
> +        if args[0] == '--all-repos':
> +            allrepos = True
> +            args.pop(0)
> +        elif args[0] == '--read-only':
> +            readonly = True
> +            args.pop(0)
> +        else:
> +            break
>       allowed_paths = [os.path.normpath(os.path.join(cwd,
>                                                      os.path.expanduser(path)))
> -                     for path in sys.argv[1:]]
> +                     for path in args]
>       orig_cmd = os.getenv('SSH_ORIGINAL_COMMAND', '?')
>       try:
>           cmdargv = shlex.split(orig_cmd)
> @@ -48,10 +60,12 @@
>       if cmdargv[:2] == ['hg', '-R'] and cmdargv[3:] == ['serve', '--stdio']:
>           path = cmdargv[2]
>           repo = os.path.normpath(os.path.join(cwd, os.path.expanduser(path)))
> -        if repo in allowed_paths:
> -            dispatch.dispatch(dispatch.request(['-R', repo,
> -                                                'serve',
> -                                                '--stdio']))
> +        if repo in allowed_paths or allrepos:
> +            cmd = ['-R', repo, 'serve', '--stdio']
> +            if readonly:
> +                cmd += ['--config',
> +                        'hooks.prechangegroup=python:__main__.rejectpush']
> +            dispatch.dispatch(dispatch.request(cmd))
>           else:
>               sys.stderr.write('Illegal repository "%s"\n' % repo)
>               sys.exit(255)

I am not sure what the use case for read-only is - it seems like the 
same control could be achieved with sshd configuration and unix permissions?

I also notice that the configuration is quite different from the hgweb 
allow_push / deny_push functionality. That might be ok, but I wonder if 
it wouldn't be more useful if the configuration was per repo and with 
more fine grained per user access.

The all-repos functionality seems more questionable. It seems like it 
will allow a remote user to probe any file system path - for example in 
/proc and on nfs automounts and temporary repos in /tmp. That is quite 
different from the completely locked down sandbox the usual 
white-listing gives and might have unexpected security implications. I 
think we will  have to sanitize the paths ... but that is non-trivial 
and we would rather avoid doing that. But again: What is the usecase for 
this? Do you have a setup where whitelisting based on shell globbing 
isn't feasible?

Now we have two features in one patch. I think the read-only feature 
would be ok if it was a separate patch ;-)

> +def rejectpush(ui, **kwargs):
> +    ui.warn("Permission denied\n")
> +    # mercurial hooks use unix process conventions for hook return values
> +    # so a truthy return means failure
> +    return True

Returning a Boolean in place of an exit code / error code / failure code 
is a bit misleading and confusing and do that the comment is necessary. 
I think the code would be more readable if it just returned a 'random' 
integer as error code.

/Mads



More information about the Mercurial-devel mailing list