Re: Recursive operations and authz

 On 05/13/2015 10:35 AM, Branko Čibej wrote:

On 13 May 2015 at 15:24, C. Michael Pilato <cmpilato_at_collab.net>
<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.

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

 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!"

Now, I'm a bit disconnected from the project these days, so I accept a
reduction in the weight of my vote. 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. :-)

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.

-- 
C. Michael Pilato <cmpilato_at_collab.net> <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development