Comment 11 for bug 109143

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 should give 'access denied' or similar
* don't do homedir expansion

>
> 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.

It may be, but it seems like a cleaner separation to me.

> 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 effort would probably be
> about the same.  (And if we do implement such at thing later, it would be pretty
> easy to convert my existing proposal to use that instead while keep
> backwards-compat of the API, I think.)

I think that sounds good - or perhaps there should in fact be a more
separated transport namespace class.

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