hpss does not support ~ (tilde) for home dir access on bzr:// or bzr+ssh://

Hmm, I think this is the sort of UI polish that's overdue. I'd like to try fix this for 1.1.

Hi!

Pls. add support for expanding ~!

It's so tedious to write the absolute paths :-/

Sincerely,
Gour

I have a quick proof-of-concept patch that implements this. It needs some thought and testing, but it seems to work basically ok. It makes bzr+ssh:// invoke "bzr serve ... --directory=." if the URL looks like "bzr+ssh://host/~/...".

Fix sent to the list for review.

Putting this back to inprogress because the patch was knocked back in review.

I guess doing it at the ssh layer, though not ideal, is a step forward. If the patch is technically OK I'd be ok to merge it and open a separate bug asking for it to be done in the rpcs rather than when starting the server.

On Thu, 2009-06-11 at 02:53 +0000, Martin Pool wrote:
> I guess doing it at the ssh layer, though not ideal, is a step forward.
> If the patch is technically OK I'd be ok to merge it and open a separate
> bug asking for it to be done in the rpcs rather than when starting the
> server.

If doing it at the ssh layer will interact badly with later doing it per
RPC then perhaps waiting for a per rpc approach.

-Rob

2009/6/11 Robert Collins <email address hidden>:
> If doing it at the ssh layer will interact badly with later doing it per
> RPC then perhaps waiting for a per rpc approach.

I agree, but I don't think it will cause problems in future. New
servers will still accept the command line option (or refuse it), and
new clients just won't send.

--
Martin <http://launchpad.net/~mbp/>

Ok, I'm working on this again. The main difficulty I see with translating ~/ on the server is making sure we don't accidentally allow access to $HOME if it wouldn't be allowed without this feature. e.g. adding this feature shouldn't silently cause existing inetd configurations (or OpenSSH authorized_keys) that specify --directory=/foo/bar to start serving something outside /foo/bar.

I think the way to do this is to add another argument to SmartServerRequest's constructor giving the relative path to use to translate an initial ~ path segment. If not specified, ~ will not be treated specially (which still leaves the possibility for a custom transport implementation for backing transport to handle it specially if desired). Then "bzr serve" can look to see if $HOME is inside the specified --directory, and if so, pass this extra argument.

This will satisfy the main motiviation, which is to make bzr+ssh://host/~/... do something useful automatically. It may also be useful for bzr:// and bzr+http too, which is nice.

Perhaps we will want to allow users to pass --homedir to bzr serve rather than hardcoding to $HOME, but we can do that easily enough later if/when we get some use cases for more control.

2009/9/10 Andrew Bennetts <email address hidden>:
> Ok, I'm working on this again.  The main difficulty I see with
> translating ~/ on the server is making sure we don't accidentally allow
> access to $HOME if it wouldn't be allowed without this feature.  e.g.
> adding this feature shouldn't silently cause existing inetd
> configurations (or OpenSSH authorized_keys) that specify
> --directory=/foo/bar to start serving something outside /foo/bar.

Good point. You should test it, too, when the time comes.

> I think the way to do this is to add another argument to
> SmartServerRequest's constructor giving the relative path to use to
> translate an initial ~ path segment.  If not specified, ~ will not be
> treated specially (which still leaves the possibility for a custom
> transport implementation for backing transport to handle it specially if
> desired).  Then "bzr serve" can look to see if $HOME is inside the
> specified --directory, and if so, pass this extra argument.

That seems ok, except a bit specific to ~. We might want to add
support for say ~spiv later on.

It seems to me that we want to put a limitation on any transport
constructed in that context, and a good way to do that would be by
hooking in at the get_transport factory level. We could pass in a
transport factory to the request, but don't we already have some kind
of jail on what kind of requests they can do?

--
Martin <http://launchpad.net/~mbp/>

Martin Pool wrote:
> 2009/9/10 Andrew Bennetts <email address hidden>:
[...]
> > I think the way to do this is to add another argument to
> > SmartServerRequest's constructor giving the relative path to use to
[...]
>
> That seems ok, except a bit specific to ~. We might want to add
> support for say ~spiv later on.

Well, in the API you can already pass an arbitrary backing_transport to the
server. It's not particularly hard to implement arbitrary translations by
implementing your own transport. This is how Launchpad's code hosting works for
instance; it has a server side plugin that adds an "lp-serve" command that uses
a custom backing transport that does translations like ~spiv/foo/bar ->
ab/cd/ef/01.

So features like ~user are already possible, for advanced users anyway. So I'm
a bit hesitant to add new features to the core based on speculation that it
might be used by someone... perhaps there's a bug report/feature request I'm not
aware of?

A plugin that implements the "expand /~user/ in bzr serve" feature might make a
good example, and be useful to the hypothetical people that want this feature,
so perhaps we should write that? I guess that deserves its own bug.

> It seems to me that we want to put a limitation on any transport
> constructed in that context, and a good way to do that would be by
> hooking in at the get_transport factory level. We could pass in a
> transport factory to the request, but don't we already have some kind
> of jail on what kind of requests they can do?

Right. We have two limitations here already:

  1. The backing_transport, which is a chrooted transport made by cmd_serve
     (although plugins have some control over this).
  2. The per-thread BzrDir.open jail, which prevents accidental opening of
     bzrdirs outside the backing transport, e.g. due to automatic attempts to
     open stacked branches or by hooks from incautious plugins. (Again, plugins
     may relax the jail if they choose.)

So, a naïve implementation of /~/ expansion that tries to access /home/spiv when
the backing_transport is chrooted at /srv/public_repo/ would probably trip over
both existing safeguards.

That said, I'd rather a solution that never tried to do something insecure in
the first place. :)

Also, a path that attempts to violate the existing restrictions results in a
different error to a simple “path not found”, IIRC. I think that's reasonable,
but at the same time I think we would want /~/ to be a simple “path not found”
if it isn't accessible for a given server. So that's also a reason to only
perform /~/ expansion if we expect it will work.

I did consider implementing this via wrapping the backing transport, rather than
via extending SmartServerRequest. So then you'd (typically, ignoring wacky
plugins or wacky bzr+http glue) have several layers of transport decorator:

   homedir-expander(chroot(actual))

Implementing a whole transport decorator seemed likely to be more work than just
extending the logic in SmartServerRequest.translate_client_path, though.

If we already had a path-filtering Transport base class with a single method to
override to vet all paths that got through it, then the effo...

2009/9/10 Andrew Bennetts <email address hidden>:
> Martin Pool wrote:
>> 2009/9/10 Andrew Bennetts <email address hidden>:
> [...]
>> > I think the way to do this is to add another argument to
>> > SmartServerRequest's constructor giving the relative path to use to
> [...]
>>
>> That seems ok, except a bit specific to ~.  We might want to add
>> support for say ~spiv later on.
>
> Well, in the API you can already pass an arbitrary backing_transport to the
> server.  It's not particularly hard to implement arbitrary translations by
> implementing your own transport.  This is how Launchpad's code hosting works for
> instance; it has a server side plugin that adds an "lp-serve" command that uses
> a custom backing transport that does translations like ~spiv/foo/bar ->
> ab/cd/ef/01.
>
> So features like ~user are already possible, for advanced users anyway.  So I'm
> a bit hesitant to add new features to the core based on speculation that it
> might be used by someone... perhaps there's a bug report/feature request I'm not
> aware of?

~user is supported by ssh and rsync. I don't think it's super
speculative to say people might want to say "please have a look at
bzr+ssh://devpad/~mbp/foo", or for that matter file:///~spiv/foo.

I just don't see it as inherently specific to hpss.

>
> A plugin that implements the "expand /~user/ in bzr serve" feature might make a
> good example, and be useful to the hypothetical people that want this feature,
> so perhaps we should write that?  I guess that deserves its own bug.
>
>> It seems to me that we want to put a limitation on any transport
>> constructed in that context, and a good way to do that would be by
>> hooking in at the get_transport factory level.  We could pass in a
>> transport factory to the request, but don't we already have some kind
>> of jail on what kind of requests they can do?
>
> Right.  We have two limitations here already:
>
>  1. The backing_transport, which is a chrooted transport made by cmd_serve
>     (although plugins have some control over this).
>  2. The per-thread BzrDir.open jail, which prevents accidental opening of
>     bzrdirs outside the backing transport, e.g. due to automatic attempts to
>     open stacked branches or by hooks from incautious plugins.  (Again, plugins
>     may relax the jail if they choose.)
>
> So, a naïve implementation of /~/ expansion that tries to access /home/spiv when
> the backing_transport is chrooted at /srv/public_repo/ would probably trip over
> both existing safeguards.
>
> That said, I'd rather a solution that never tried to do something insecure in
> the first place. :)
>
> Also, a path that attempts to violate the existing restrictions results in a
> different error to a simple “path not found”, IIRC.  I think that's reasonable,
> but at the same time I think we would want /~/ to be a simple “path not found”
> if it isn't accessible for a given server.  So that's also a reason to only
> perform /~/ expansion if we expect it will work.

I'm not sure, it seems like the server could in principle be
configured in either of two directions:

* only allow access to /srv/bzr --> attempted access to home
directories sho...

> ~user is supported by ssh and rsync. I don't think it's super
> speculative to say people might want to say "please have a look at
> bzr+ssh://devpad/~mbp/foo", or for that matter file:///~spiv/foo.

I have use for bzr+ssh://.../~user/ on a regular basis.

lp:~spiv/bzr/bzr-ssh-homedir-take-3 has a fix for this, which expands both /~/... and /~user/... paths in the server, using a transport decorator that is automatically configured by bzr serve.

Andrew, your branch is merged; should the bug be closed?

Robert: yes! \o/

It will be in the 2.1 release.