| Submitter | Durham Goode |
|---|---|
| Date | April 27, 2017, 1:24 a.m. |
| Message ID | <b1964bbc387fb53b4152.1493256292@dev111.prn1.facebook.com> |
| Download | mbox | patch |
| Permalink | /patch/20303/ |
| State | Deferred |
| Headers | show |
Comments
On Wed, Apr 26, 2017 at 06:24:52PM -0700, Durham Goode wrote: > # HG changeset patch > # User Durham Goode <durham@fb.com> > # Date 1493255976 25200 > # Wed Apr 26 18:19:36 2017 -0700 > # Branch stable > # Node ID b1964bbc387fb53b4152f19d6e929309e3f21ac6 > # Parent 6e0368b6e0bb2aa5210daec091c0200583553a78 > serve: move hg-ssh readonly logic into hg serve This looks good, but I'm very hesitant to add a new feature like this 4 days before a release. Does anyone feel strongly that this should be in 4.2?
Augie Fackler <raf@durin42.com> writes: > On Wed, Apr 26, 2017 at 06:24:52PM -0700, Durham Goode wrote: >> # HG changeset patch >> # User Durham Goode <durham@fb.com> >> # Date 1493255976 25200 >> # Wed Apr 26 18:19:36 2017 -0700 >> # Branch stable >> # Node ID b1964bbc387fb53b4152f19d6e929309e3f21ac6 >> # Parent 6e0368b6e0bb2aa5210daec091c0200583553a78 >> serve: move hg-ssh readonly logic into hg serve > > This looks good, but I'm very hesitant to add a new feature like this > 4 days before a release. Does anyone feel strongly that this should be > in 4.2? I have no strong opinions one way or the other.
On Thu, 27 Apr 2017 10:13:21 -0400, Augie Fackler wrote: > On Wed, Apr 26, 2017 at 06:24:52PM -0700, Durham Goode wrote: > > # HG changeset patch > > # User Durham Goode <durham@fb.com> > > # Date 1493255976 25200 > > # Wed Apr 26 18:19:36 2017 -0700 > > # Branch stable > > # Node ID b1964bbc387fb53b4152f19d6e929309e3f21ac6 > > # Parent 6e0368b6e0bb2aa5210daec091c0200583553a78 > > serve: move hg-ssh readonly logic into hg serve > > This looks good, but I'm very hesitant to add a new feature like this > 4 days before a release. Does anyone feel strongly that this should be > in 4.2? Agreed. The feature seems good, but no need to hurry to pick it up. > + if realcmd == 'serve' and '--read-only' in req.args: > + req.args.remove('--read-only') This increases the risk of wrong command parsing. We can't say --read-only is always a flag. > + req.ui.setconfig('hooks', 'pretxnopen.readonlyrejectpush', > + rejectpush, 'dispatch') > + req.ui.setconfig('hooks', 'prepushkey.readonlyrejectpush', > + rejectpush, 'dispatch') And --read-only won't work as expected in command server since there are write operations other than push.
On 4/28/17 6:27 AM, Yuya Nishihara wrote: > On Thu, 27 Apr 2017 10:13:21 -0400, Augie Fackler wrote: >> On Wed, Apr 26, 2017 at 06:24:52PM -0700, Durham Goode wrote: >>> # HG changeset patch >>> # User Durham Goode <durham@fb.com> >>> # Date 1493255976 25200 >>> # Wed Apr 26 18:19:36 2017 -0700 >>> # Branch stable >>> # Node ID b1964bbc387fb53b4152f19d6e929309e3f21ac6 >>> # Parent 6e0368b6e0bb2aa5210daec091c0200583553a78 >>> serve: move hg-ssh readonly logic into hg serve >> >> This looks good, but I'm very hesitant to add a new feature like this >> 4 days before a release. Does anyone feel strongly that this should be >> in 4.2? > > Agreed. The feature seems good, but no need to hurry to pick it up. > >> + if realcmd == 'serve' and '--read-only' in req.args: >> + req.args.remove('--read-only') > > This increases the risk of wrong command parsing. We can't say --read-only > is always a flag. > >> + req.ui.setconfig('hooks', 'pretxnopen.readonlyrejectpush', >> + rejectpush, 'dispatch') >> + req.ui.setconfig('hooks', 'prepushkey.readonlyrejectpush', >> + rejectpush, 'dispatch') > > And --read-only won't work as expected in command server since there are write > operations other than push. Are there write operations that wouldn't trigger the pretxnopen hook? Alternatively, I could rename this flag to be --add-hgssh-push-blocking-hooks, remove it from the hg server command definition entirely (just watch for it at the dispatch level and strip it like I currently do). That way we can maintain the current pattern while letting it be a special path for hg-ssh only. And if someone passes --add-hgssh-push-blocking-hooks to hg serve in a position that isn't a flag, well it'll just break for them. Thoughts?
On Tue, 9 May 2017 15:48:50 -0700, Durham Goode wrote: > On 4/28/17 6:27 AM, Yuya Nishihara wrote: > > On Thu, 27 Apr 2017 10:13:21 -0400, Augie Fackler wrote: > >> On Wed, Apr 26, 2017 at 06:24:52PM -0700, Durham Goode wrote: > >>> # HG changeset patch > >>> # User Durham Goode <durham@fb.com> > >>> # Date 1493255976 25200 > >>> # Wed Apr 26 18:19:36 2017 -0700 > >>> # Branch stable > >>> # Node ID b1964bbc387fb53b4152f19d6e929309e3f21ac6 > >>> # Parent 6e0368b6e0bb2aa5210daec091c0200583553a78 > >>> serve: move hg-ssh readonly logic into hg serve > >> > >> This looks good, but I'm very hesitant to add a new feature like this > >> 4 days before a release. Does anyone feel strongly that this should be > >> in 4.2? > > > > Agreed. The feature seems good, but no need to hurry to pick it up. > > > >> + if realcmd == 'serve' and '--read-only' in req.args: > >> + req.args.remove('--read-only') > > > > This increases the risk of wrong command parsing. We can't say --read-only > > is always a flag. > > > >> + req.ui.setconfig('hooks', 'pretxnopen.readonlyrejectpush', > >> + rejectpush, 'dispatch') > >> + req.ui.setconfig('hooks', 'prepushkey.readonlyrejectpush', > >> + rejectpush, 'dispatch') > > > > And --read-only won't work as expected in command server since there are write > > operations other than push. > > Are there write operations that wouldn't trigger the pretxnopen hook? Perhaps, "hg rollback" ? > Alternatively, I could rename this flag to be > --add-hgssh-push-blocking-hooks, remove it from the hg server command > definition entirely (just watch for it at the dispatch level and strip > it like I currently do). That way we can maintain the current pattern > while letting it be a special path for hg-ssh only. > > And if someone passes --add-hgssh-push-blocking-hooks to hg serve in a > position that isn't a flag, well it'll just break for them. Another idea is adding --unsafe-stdio option so the frontend script may do anything on the 'hg serve --stdio' process.
On 5/10/17 7:29 AM, Yuya Nishihara wrote: > On Tue, 9 May 2017 15:48:50 -0700, Durham Goode wrote: >> On 4/28/17 6:27 AM, Yuya Nishihara wrote: >>> On Thu, 27 Apr 2017 10:13:21 -0400, Augie Fackler wrote: >>>> On Wed, Apr 26, 2017 at 06:24:52PM -0700, Durham Goode wrote: >>>>> # HG changeset patch >>>>> # User Durham Goode <durham@fb.com> >>>>> # Date 1493255976 25200 >>>>> # Wed Apr 26 18:19:36 2017 -0700 >>>>> # Branch stable >>>>> # Node ID b1964bbc387fb53b4152f19d6e929309e3f21ac6 >>>>> # Parent 6e0368b6e0bb2aa5210daec091c0200583553a78 >>>>> serve: move hg-ssh readonly logic into hg serve >>>> >>>> This looks good, but I'm very hesitant to add a new feature like this >>>> 4 days before a release. Does anyone feel strongly that this should be >>>> in 4.2? >>> >>> Agreed. The feature seems good, but no need to hurry to pick it up. >>> >>>> + if realcmd == 'serve' and '--read-only' in req.args: >>>> + req.args.remove('--read-only') >>> >>> This increases the risk of wrong command parsing. We can't say --read-only >>> is always a flag. >>> >>>> + req.ui.setconfig('hooks', 'pretxnopen.readonlyrejectpush', >>>> + rejectpush, 'dispatch') >>>> + req.ui.setconfig('hooks', 'prepushkey.readonlyrejectpush', >>>> + rejectpush, 'dispatch') >>> >>> And --read-only won't work as expected in command server since there are write >>> operations other than push. >> >> Are there write operations that wouldn't trigger the pretxnopen hook? > > Perhaps, "hg rollback" ? > >> Alternatively, I could rename this flag to be >> --add-hgssh-push-blocking-hooks, remove it from the hg server command >> definition entirely (just watch for it at the dispatch level and strip >> it like I currently do). That way we can maintain the current pattern >> while letting it be a special path for hg-ssh only. >> >> And if someone passes --add-hgssh-push-blocking-hooks to hg serve in a >> position that isn't a flag, well it'll just break for them. > > Another idea is adding --unsafe-stdio option so the frontend script may do > anything on the 'hg serve --stdio' process. I'm not sure I understand this proposal. If we have --unsafe-stdio, doesn't that imply --stdio is safe and can block write operations? It sounds like we don't have a way to block things like rollback, so we couldn't guarantee --stdio is safe.
On Wed, 10 May 2017 08:50:38 -0700, Durham Goode wrote: > On 5/10/17 7:29 AM, Yuya Nishihara wrote: > > On Tue, 9 May 2017 15:48:50 -0700, Durham Goode wrote: > >> On 4/28/17 6:27 AM, Yuya Nishihara wrote: > >>> On Thu, 27 Apr 2017 10:13:21 -0400, Augie Fackler wrote: > >>>> On Wed, Apr 26, 2017 at 06:24:52PM -0700, Durham Goode wrote: > >>>>> # HG changeset patch > >>>>> # User Durham Goode <durham@fb.com> > >>>>> # Date 1493255976 25200 > >>>>> # Wed Apr 26 18:19:36 2017 -0700 > >>>>> # Branch stable > >>>>> # Node ID b1964bbc387fb53b4152f19d6e929309e3f21ac6 > >>>>> # Parent 6e0368b6e0bb2aa5210daec091c0200583553a78 > >>>>> serve: move hg-ssh readonly logic into hg serve > >>>> > >>>> This looks good, but I'm very hesitant to add a new feature like this > >>>> 4 days before a release. Does anyone feel strongly that this should be > >>>> in 4.2? > >>> > >>> Agreed. The feature seems good, but no need to hurry to pick it up. > >>> > >>>> + if realcmd == 'serve' and '--read-only' in req.args: > >>>> + req.args.remove('--read-only') > >>> > >>> This increases the risk of wrong command parsing. We can't say --read-only > >>> is always a flag. > >>> > >>>> + req.ui.setconfig('hooks', 'pretxnopen.readonlyrejectpush', > >>>> + rejectpush, 'dispatch') > >>>> + req.ui.setconfig('hooks', 'prepushkey.readonlyrejectpush', > >>>> + rejectpush, 'dispatch') > >>> > >>> And --read-only won't work as expected in command server since there are write > >>> operations other than push. > >> > >> Are there write operations that wouldn't trigger the pretxnopen hook? > > > > Perhaps, "hg rollback" ? > > > >> Alternatively, I could rename this flag to be > >> --add-hgssh-push-blocking-hooks, remove it from the hg server command > >> definition entirely (just watch for it at the dispatch level and strip > >> it like I currently do). That way we can maintain the current pattern > >> while letting it be a special path for hg-ssh only. > >> > >> And if someone passes --add-hgssh-push-blocking-hooks to hg serve in a > >> position that isn't a flag, well it'll just break for them. > > > > Another idea is adding --unsafe-stdio option so the frontend script may do > > anything on the 'hg serve --stdio' process. > > I'm not sure I understand this proposal. If we have --unsafe-stdio, > doesn't that imply --stdio is safe and can block write operations? It > sounds like we don't have a way to block things like rollback, so we > couldn't guarantee --stdio is safe. I meant we could add --unsafe-stdio (or --unchecked-stdio) as an alias of --stdio. A frontend script (like hg-ssh) may bypass the constraints of 'serve --stdio' by using 'serve --unsafe-stdio'. This might sound evil, but I think it's okay as long as 'serve <whatever>' is not an API of the ssh wire protocol.
Patch
diff --git a/contrib/hg-ssh b/contrib/hg-ssh --- a/contrib/hg-ssh +++ b/contrib/hg-ssh @@ -32,7 +32,7 @@ command="hg-ssh --read-only repos/*" # enable importing on demand to reduce startup time from mercurial import demandimport; demandimport.enable() -from mercurial import dispatch, ui as uimod +from mercurial import dispatch import sys, os, shlex @@ -61,14 +61,9 @@ def main(): repo = os.path.normpath(os.path.join(cwd, os.path.expanduser(path))) if repo in allowed_paths: cmd = ['-R', repo, 'serve', '--stdio'] + if readonly: + cmd.append('--read-only') req = dispatch.request(cmd) - if readonly: - if not req.ui: - req.ui = uimod.ui.load() - req.ui.setconfig('hooks', 'pretxnopen.hg-ssh', - 'python:__main__.rejectpush', 'hg-ssh') - req.ui.setconfig('hooks', 'prepushkey.hg-ssh', - 'python:__main__.rejectpush', 'hg-ssh') dispatch.dispatch(req) else: sys.stderr.write('Illegal repository "%s"\n' % repo) diff --git a/mercurial/commands.py b/mercurial/commands.py --- a/mercurial/commands.py +++ b/mercurial/commands.py @@ -4619,8 +4619,9 @@ def root(ui, repo): ('t', 'templates', '', _('web templates to use'), _('TEMPLATE')), ('', 'style', '', _('template style to use'), _('STYLE')), ('6', 'ipv6', None, _('use IPv6 in addition to IPv4')), - ('', 'certificate', '', _('SSL certificate file'), _('FILE'))] - + subrepoopts, + ('', 'certificate', '', _('SSL certificate file'), _('FILE')), + ('', 'read-only', None, _('only allow read operations')), + ] + subrepoopts, _('[OPTION]...'), optionalrepo=True) def serve(ui, repo, **opts): diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py --- a/mercurial/dispatch.py +++ b/mercurial/dispatch.py @@ -214,6 +214,17 @@ def _runcatch(req): # it's not a command that server operators expect to # be safe to offer to users in a sandbox. pass + + if realcmd == 'serve' and '--read-only' in req.args: + req.args.remove('--read-only') + + if not req.ui: + req.ui = uimod.ui.load() + req.ui.setconfig('hooks', 'pretxnopen.readonlyrejectpush', + rejectpush, 'dispatch') + req.ui.setconfig('hooks', 'prepushkey.readonlyrejectpush', + rejectpush, 'dispatch') + if realcmd == 'serve' and '--stdio' in cmdargs: # We want to constrain 'hg serve --stdio' instances pretty # closely, as many shared-ssh access tools want to grant @@ -993,3 +1004,9 @@ def handlecommandexception(ui): ui.log("commandexception", "%s\n%s\n", warning, traceback.format_exc()) ui.warn(warning) return False # re-raise the exception + +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 diff --git a/tests/test-completion.t b/tests/test-completion.t --- a/tests/test-completion.t +++ b/tests/test-completion.t @@ -181,6 +181,7 @@ Show the options for the "serve" command --prefix --profile --quiet + --read-only --repository --stdio --style @@ -227,7 +228,7 @@ Show all commands + options pull: update, force, rev, bookmark, branch, ssh, remotecmd, insecure push: force, rev, bookmark, branch, new-branch, ssh, remotecmd, insecure remove: after, force, subrepos, include, exclude - serve: accesslog, daemon, daemon-postexec, errorlog, port, address, prefix, name, web-conf, webdir-conf, pid-file, stdio, cmdserver, templates, style, ipv6, certificate, subrepos + serve: accesslog, daemon, daemon-postexec, errorlog, port, address, prefix, name, web-conf, webdir-conf, pid-file, stdio, cmdserver, templates, style, ipv6, certificate, read-only, subrepos status: all, modified, added, removed, deleted, clean, unknown, ignored, no-status, copies, print0, rev, change, include, exclude, subrepos, template summary: remote update: clean, check, merge, date, rev, tool diff --git a/tests/test-ssh-bundle1.t b/tests/test-ssh-bundle1.t --- a/tests/test-ssh-bundle1.t +++ b/tests/test-ssh-bundle1.t @@ -410,9 +410,9 @@ Test hg-ssh in read-only mode: pushing to ssh://user@dummy/*/remote (glob) searching for changes remote: Permission denied - remote: abort: pretxnopen.hg-ssh hook failed + remote: abort: pretxnopen.readonlyrejectpush hook failed remote: Permission denied - remote: pushkey-abort: prepushkey.hg-ssh hook failed + remote: pushkey-abort: prepushkey.readonlyrejectpush hook failed updating 6c0482d977a3 to public failed! [1] diff --git a/tests/test-ssh.t b/tests/test-ssh.t --- a/tests/test-ssh.t +++ b/tests/test-ssh.t @@ -427,7 +427,7 @@ Test hg-ssh in read-only mode: pushing to ssh://user@dummy/*/remote (glob) searching for changes remote: Permission denied - remote: pretxnopen.hg-ssh hook failed + remote: pretxnopen.readonlyrejectpush hook failed abort: push failed on remote [255]