On 13.05.2015 18:18, C. Michael Pilato wrote:
> On 05/13/2015 10:35 AM, Branko Čibej wrote:
>> On 13 May 2015 at 15:24, C. Michael Pilato <cmpilato_at_collab.net> <mailto:cmpilato_at_collab.net> wrote:
>>>
>>> Well, the use-case being broken here is kinda the obvious one: I
>>> shouldn't have permission to create/delete some path /foo/bar (it's a
>>> system-critical file that shouldn't go away, or a password-bearing file
>>> path blocked by the VC system because devs keep stupidly committing
>>> them, or...), but you've provided and end-around for me to do so. :-)
>>> In the copy and move scenarios, you're allowing me to create a path I
>>> can't later delete or further modify. Now, while this is a very handy
>>> semantic for tag creation[1], it's also something an administrator
>>> should be able to make a conscious decision about, not just an
>>> unfortunate side-effect of an effort to make the code faster.
>> This proposal really has nothing to do with performance; it's about
>> intended/consistent authz semantics, which we never really bothered to
>> define, let alone prove. IMO, the fact that you can have authz rules
>> that allow you to create a directory but not its children is
>> unintended. I bet no-one thought about it during the initial
>> development of mod_authz_svn.
>
> Dude. The Subversion authz subsystem was discussed nearly *to death*
> before its implementation. I sat less than 10 feet away from the two
> guys who drove the formulation and implementation of both the public
> authz subsystem and CollabNet's private manifestation thereof. I have
> the T-shirt. And the scars. :-) This ground was covered.
I stand corrected.
> As you might recall, though, there was really no attempt to model
> in-filesystem ACLs, so you'll find all kinds of disparity with such a
> system when you compare it to ours. Create a directory but not its
> children? Obviously silly in an ACL system. Less so in a system
> where paths themselves are considered a part of the information whose
> access is controlled. But, seriously, what could be more consistent
> than the policy Subversion has right now? "If I don't have write
> access to a path, I can't modify (add, delete, change) anything at
> that path" seems about as straightforward as is semantically possible.
>
> But I need to back up a bit and confess that I failed to process a bit
> of the original mail, namely the bit about how the authz code naively
> disallows a recursive write if there are any rules that would block
> writes to some path deep in that tree *without even seeing if there
> actually is/would-be such a path*. I mean, that's the claim, right?
Yup. That's the case, for both the source (recursive read for copy,
recursive write for move) and the target (recursive write) of the
copy/move operation.
> If so, that's ... weird. And yeah, wildly inconsistent with ...
> really any sane reasoning. CollabNet's authz module did not behave
> this way -- we accepted the recursive tree walk as a necessary evil.
> Though, we did at least try to optimize it a bit: you can check read
> access at the source and write access at the target of a copy with the
> same walk, using a bit of path-math; and you can of course check
> multiple permissions -- read, add, modify, delete, copy -- as needed
> at the same time, too.
Ack. Note that on the authzperf branch -- which is, in some ways,
equivalent to your authz-improvements branch but with (initially) a
different set of priorities -- we still don't do tree walks, but the
authz parser and internal representation do predict, propagate and
separate the individual access rights. Not that we have more than two
there yet.
>>> I lean towards thinking
>>> that falls outside the scope of acceptable changes in behavior in the
>>> 1.x line *unless* you can find a way to, via configuration, allow
>>> administrators to explicitly toggle this new paradigm.
>> Assuming we agree that it's not a new paradigm, would proper
>> documentation, as I proposed, be sufficient?
>
> I was going to suggest that perhaps you tie the change in behavior to
> the use of wildcard rules, but as was explained in the bit I failed to
> process originally, this problem predates the new syntax. It still
> reads to me as if you're trying to fix a relatively straightforward
> problem (we do recursive path checks without actually considering the
> actual paths) in a way that runs counter to the original authz design
> principles (and by that, I mean authz_policy.txt, not the much-younger
> libsvn_repos/authz.c) just to avoid the more-expensive-yet-correct fix
> predicted in that very file:
>
> 3. Manually implement authz when receiving network requests that
> represent calls to a commit editor:
>
> - do write checks for most editor operations
> - do read *and* write checks for copy operations.
>
> (Note that doing full-out authz on whole trees fundamentally
> contradicts Subversion's O(1) copy philosophy; in practice,
> however, specific authz implementations are able to get the same
> effect while being less expensive than O(N).)
>
> In other words: "To fully implement the write side of this policy,
> you have to be recursive -- but maybe you'll find a way to avoid a
> tree walk. Good luck!"
There are ways to prevent a whole-tree walk, but these dwindle amazingly
quickly as soon as you have any kind of pattern matching in the rules.
> Now, I'm a bit disconnected from the project these days, so I accept a
> reduction in the weight of my vote.
Eh? There's no reduction, unless you want to exercise it yourself. :)
> I'm confident that the world will continue to spin regardless of
> what you guys decide. But just to be clear, you are proposing to
> silently remove existing access control protections that have been in
> place for over a decade, and introduce a change of behavior that
> contradicts the written authz_policy.txt document, and then cover for
> it with just a bit of documentation that sits on a website far removed
> from nearly everyone's package distribution system. :-)
I'm very acutely aware of this, yes ... in this case, the "we told you
so!" argument isn't very solid.
> SIDEBAR: I didn't see any changes in the patch to mod_authz_svn's use
> of recursive checks in these same ways. If you move forward with this
> change, don't forget to update that code as necessary, too.
Other than not checking for recursive write on the target of a copy? No;
if mod_authz_svn, by itself or with the "help" of mod_dav, do the
recursive walk, nothing would change.
-- Brane
Received on 2015-05-13 20:48:12 CEST