[PATCH 3 of 7] dispatch: rework the serve --stdio safe argument checks
Boris FELD
boris.feld at octobus.net
Sun Jul 15 10:33:48 UTC 2018
Hello,
This topic is important to us, I will try to provide more context about
our needsand constraintsin order to reach a consensus.
Our ultimate goal is to activate various debugging output (profile,
debug log...) on the server via the client. We have already used this
series in real condition by having a custom Mercurial version deployed.
The data we get from using this feature proved to be very valuable and
speed up our works a lot. Weidentified the source of multipleperformance
issuesthanks to it.
The properties that are important for us are the following and we will
explain why a bit further:
   - We want to activate it as soon as possible (ssh: dispatch level,
http: wsgi server),
   - Controlled by the client,
   - Security managed by core.
Yuya would like to move the implementation outside of core dispatch, for
security and maintenance concerns. Either, into custom ssh script, or at
the protocol level. Neither of these two locations fits our needs (and
constraints).
So we need to find a way to do this early extra validation of parameter
in a way that is good enough in terms of security and maintenance.
# Longer explanation
## Why does it has to be set up so early?
A significant part of the slowness we observe happen very early, during
the initial setup (extensions setup, various data loading, custom
extensions code, etc...). So the earlier the extra debug/profile is
setup, the more useful data we gather.
Doing this at the protocol level would be too late, as a lot of the
important time-consuming activities have already happened.
## Why control it from the client instead of being a custom ssh script?
Some of the people we work with have a high level of security (eg: we
don't have a full shell access on their server and we only have access
to a subset of their repositories).
This means multiples things:
* We still want some security management, using `hg debugserve` would
allow arbitrary code to be executed, something we still want to prevent.
* Deploying a new ssh script is something complex that takes time.
Admins have to look at the new script and deploy it. It can take
multiple days, making us unable to collect these valuable data (in time).
* If we, mercurial core developers, are afraid of getting the validation
logic in core wrong, I don't think we should expect people not familiar
with Mercurial to be able to get that right. So moving the
responsibility to validate input to admins seems dangerous.
* Having an example script in `contrib/` would not help much as people
usually already have some custom script.
* Controlling option from the client is critical. When investigating
something, we try various options to look at the issue from different
angles (profile cpu vs time). Having to redeploy a new script every time
we want to change an option break our ability to investigate quickly as
it often requires synchronization between different teams.
## Why do you want this into core
* Make it much simpler for us to use it at more place (including to help
random people on the list and bug tracker). Having to deploy a custom
Mercurial binaries on the server is a strong barrier.
* Given how valuable this is to us, we expect it to be valuable for
others. In fact server-side debug message is something various users
asked for in the past.
* Maintaining a custom series of patch to be moved around is time
wasting. We would prefer focusing our work on upstreaming code for the
benefits of all of us rather than maintaining a custom patch series. Our
work days are more about upstreaming code into core for the benefit of
all of us rather than the other way around.
# What next?
With the current constraints, the proposed approach was the best
solution we came up with. Admins still have the control of the feature
with few configuration knobs and turning them on is quick and easy.
Debug outputs are provided on demand so we can test against servers in
real conditions without impacting others users and it's enabled early
enough so we could gather valuable data.
It helped us to identify existing issues and send appropriate patches.
For example, the fncache improvement that recently landed originated
from this.
However, we understand Yuya's concerns about security. Maybe we can
rework the command line parsing tooling to extract a small kernel that
would fit our need here while being easier to validate and test security
wise?
We are willing to discuss potential other solutions, we may have
missedone that fits our needs.
Cheers,
--
Boris Feld
On 26/06/2018 13:53, Yuya Nishihara wrote:
> On Mon, 25 Jun 2018 19:51:52 +0200, Paul Morelle wrote:
>> On 22/06/18 13:15, Yuya Nishihara wrote:
>>> On Thu, 21 Jun 2018 19:46:24 +0200, Paul Morelle wrote:
>>>> On 21/06/18 13:53, Yuya Nishihara wrote:
>>>>> On Wed, 20 Jun 2018 18:36:24 +0200, Paul Morelle wrote:
>>>>>>> # HG changeset patch
>>>>>> # User Boris Feld <boris.feld at octobus.net>
>>>>>> # Date 1529489906 -7200
>>>>>> # Wed Jun 20 12:18:26 2018 +0200
>>>>>> # Node ID 81edf3431b95d57257c690f7fe125c6676a78e18
>>>>>> # Parent b7051e4bf783c844f705473a2396458acecc59dc
>>>>>> # EXP-Topic remote-debug
>>>>>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>>>>>> # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 81edf3431b95
>>>>>> dispatch: rework the serve --stdio safe argument checks
>>>>>>
>>>>>> We prepare the code to check for arguments after the mandatory ones.
>>>>>>
>>>>>> We want to relax this check to allow for wider argument passing in certain
>>>>>> conditions (example --debug). We make preliminary refactoring in different
>>>>>> changesets for clarity.
>>>>>>
>>>>>> diff -r b7051e4bf783 -r 81edf3431b95 mercurial/dispatch.py
>>>>>> --- a/mercurial/dispatch.py Wed Jun 20 12:16:48 2018 +0200
>>>>>> +++ b/mercurial/dispatch.py Wed Jun 20 12:18:26 2018 +0200
>>>>>> @@ -285,12 +285,15 @@
>>>>>> def unsafe():
>>>>>> msg = _('potentially unsafe serve --stdio invocation: %s')
>>>>>> raise error.Abort(msg % (stringutil.pprint(req.args),))
>>>>>> - if (len(req.args) != 4 or
>>>>>> + if (len(req.args) < 4 or
>>>>>> req.args[0] != '-R' or
>>>>>> req.args[1].startswith('--') or
>>>>>> req.args[2] != 'serve' or
>>>>>> req.args[3] != '--stdio'):
>>>>>> unsafe()
>>>>>> + other_args = req.args[4:]
>>>>>> + if other_args:
>>>>>> + unsafe()
>>>>> It's a bit scary to extend this just for debugging aids because argument
>>>>> parsing at this phase has to be ad-hoc. Can't you instead use the
>>>>> ssh/authorized_keys file to redirect a server session to 'hg debugserve'?
>>>>>
>>>>> Alternatively, we could add a wrapper script like hg-ssh.
>>>> If I have correctly understood, your proposition is to keep the
>>>> client-side version of this, and move the permission management to the
>>>> sshkey/ssh-server level. Is this right?
>>> Yes.
>>>
>>>> Something we could do in this area is to replace the call to sshpeer by
>>>> `hg debugserve …` when we need the remote-debugging feature.
>>>> However, exposing a "debug" command at the wireprotocol level seems bad
>>>> ; maybe we could introduce a `--remote-argument` flag that would lift
>>>> the check with a permission handling similar to what we have today (in
>>>> patch 4).
>>>>
>>>> However, having all checks disabled (debugserve) is quite different than
>>>> what is proposed in this series, which only allows for more information
>>>> to be retrieved, and that's it.
>>>> Fully bypassing the argument check would allow the client do a full
>>>> range of actions (including arbitrary code execution). This is a much
>>>> different access level, and in my current use case it would be a problem.
>>> Instead of using debugserve, the wrapper script can set ui.debug/traceback
>>> flag just like contrib/hg-ssh does for hooks.
>> Doing this at the authorized_keys level is not an option for us. In one
>> of our target environment for this kind of debugging, any change to the
>> ssh script requires validation by admin people that can take from a
>> couple of days to a couple of weeks, making it impossible to turn
>> debugging on for a couple of commands, or easily compare various settings.
> IMHO, that doesn't justify why we have to put everything in hg core.
> "hg serve" itself is an SSH script in security POV.
>
>> The ability to control the used debug options from the client is very
>> valuable in this case. We have already gathered many useful information
>> from it!
>>
>> I agree that ad-hoc argument parsing is less than optimal. Handling
>> three extra flags (--debug, --profile and --traceback) is not too awful,
>> but then we need to pass some associated configuration option (e.g.
>> profiling.time-track=real) which will make things even more verbose.
>>
>> Maybe we could leverage the contents of `mercurial/configitems.py` by
>> adding a "serversafe" attribute to the configuration items? Such config
>> item could be specified on the client side (if the user is allowed to).
> No, that sounds worse than the original patch. IIUC, the reason why we have
> hard-coded check for req.args is that our option parser isn't strict and is
> likely to allow command-line injection. Making the whitelist fully configurable
> would be dangerous.
>
>>>> I don't think that moving the permission handling outside of Mercurial
>>>> would be a good move : implementing similar features for HTTP would
>>>> still require some Mercurial-side restrictions and permissions handling
>>>> anyway. It seems more consistent to implement it on the Mercurial side
>>>> for all protocols.
>>> I agree it should be handled at protocol level, and I think it should be
>>> a wire command/option rather than the command argument hack. IMHO, it's
>>> way safer than constructing/parsing shell command arguments carefully,
>>> and HTTP will need that anyway.
>> The life cycle of the server is quite different in ssh and http, and
>> this has different implications.
>> For the ssh client, there are multiple initializations and cachings that
>> are worth tracking, so tracking the full run is what is most appropriate
>> for our needs.
>> For http, the arguments will have to be processed at the hgweb level
>> (instead of the protocol level).
> I know the situation of HTTP and SSH is slightly different, but how important
> it is to catch the early-stage thingy by extending the SSH command-line
> interface, which we have to support forever? I think wire-protocol level
> implementation will be good enough, and better in the long run.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20180715/df856f4d/attachment.html>
More information about the Mercurial-devel
mailing list