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.
2009/9/10 Andrew Bennetts <email address hidden>: est's constructor giving the relative path to use to
> Martin Pool wrote:
>> 2009/9/10 Andrew Bennetts <email address hidden>:
> [...]
>> > I think the way to do this is to add another argument to
>> > SmartServerRequ
> [...]
>>
>> 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 //devpad/ ~mbp/foo" , or for that matter file:///~spiv/foo.
speculative to say people might want to say "please have a look at
bzr+ssh:
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
> expander( chroot( actual) ) est.translate_ client_ path, though.
> 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-
>
> Implementing a whole transport decorator seemed likely to be more work than just
> extending the logic in SmartServerRequ
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.
-- launchpad. net/~mbp/>
Martin <http://