Patchwork [V2,STABLE] serve: move hg-ssh readonly logic into hg serve

login
register
mail settings
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

Durham Goode - April 27, 2017, 1:24 a.m.
# 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

Recently hg-ssh was changed to block writes via in-memory hook configuration
instead of by passing config hooks, and dispatch.py blocks any invocation of hg
serve --stdio that has options passed. We have infrastructure that sets up read
only serve processes without using hg-ssh, and it was broken by this change.

Let's add a --read-only option to hg serve so non-hg-ssh solutions can still
launch hg in read-only mode. This makes it also work with non-stdio serve
processes as well.
Augie Fackler - April 27, 2017, 2:13 p.m.
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?
Sean Farley - April 27, 2017, 5:48 p.m.
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.
Yuya Nishihara - April 28, 2017, 1:27 p.m.
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.
Durham Goode - May 9, 2017, 10:48 p.m.
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?
Yuya Nishihara - May 10, 2017, 2:29 p.m.
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.
Durham Goode - May 10, 2017, 3:50 p.m.
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.
Yuya Nishihara - May 11, 2017, 2:03 p.m.
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]