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 <cmpil...@collab.net> >> <mailto:cmpil...@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
