[PATCH] hg-ssh: more flexible permissions for hg-ssh

[Matt Mackall]

On Fri, 2012-05-18 at 14:21 -0700, David Schleimer wrote:
> # HG changeset patch
> # User David Schleimer <dschleimer at fb.com>
> # Date 1337376000 25200
> # Node ID cbfb26903ff5a57cc53e8abc5ca64a06c5fa0bb1
> # Parent  9acb5cd19162bdb1053429df9ce9a13c2d15aea8
> 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.

..and a bunch of other refactoring that makes this too complicated to
properly evaluate.

The very last thing I want to see is an obfuscated security-related
patch.

Please break this up:

- add a main()
- factor out a rejectpatch() helper
- etc.

> diff --git a/contrib/hg-ssh b/contrib/hg-ssh
> --- a/contrib/hg-ssh
> +++ b/contrib/hg-ssh
> @@ -33,25 +33,51 @@
>  
>  import sys, os, shlex
>  
> -cwd = os.getcwd()
> -allowed_paths = [os.path.normpath(os.path.join(cwd, os.path.expanduser(path)))
> -                 for path in sys.argv[1:]]
> -orig_cmd = os.getenv('SSH_ORIGINAL_COMMAND', '?')
> -try:
> -    cmdargv = shlex.split(orig_cmd)
> -except ValueError, e:
> -    sys.stderr.write('Illegal command "%s": %s\n' % (orig_cmd, e))
> -    sys.exit(255)
> +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 sys.argv[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 args]
> +    orig_cmd = os.getenv('SSH_ORIGINAL_COMMAND', '?')
> +    try:
> +        cmdargv = shlex.split(orig_cmd)
> +    except ValueError, e:
> +        sys.stderr.write('Illegal command "%s": %s\n' % (orig_cmd, e))
> +        sys.exit(255)
>  
> -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 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 or allrepos:
> +            cmd = ['-R', repo, 'serve', '--stdio']
> +            if readonly:
> +                cmd = ['--config',
> +                       'hooks.prechangegroup=python:__main__.rejectpush'] + cmd
> +            dispatch.dispatch(dispatch.request(cmd))
> +        else:
> +            sys.stderr.write('Illegal repository "%s"\n' % repo)
> +            sys.exit(255)
>      else:
> -        sys.stderr.write('Illegal repository "%s"\n' % repo)
> +        sys.stderr.write('Illegal command "%s"\n' % orig_cmd)
>          sys.exit(255)
> -else:
> -    sys.stderr.write('Illegal command "%s"\n' % orig_cmd)
> -    sys.exit(255)
>  
> +def rejectpush(ui, repo, hooktype):
> +    ui.warn('Permissions denied')
> +    # mercurial uses unix process conventions for hook return values
> +    # so a truthy return means failure
> +    return True
> +
> +if __name__ == '__main__':
> +    main()


-- 
Mathematics is the supreme nostalgia of our time.