Re: [PATCH] Authz support in Svnserve

kfogel@collab.net wrote:
> Since I know you like thoroughness, I'll include nits of all sizes in
> this review (some of which may duplicate what Greg Hudson already
> noticed). Although I lead off with some silly grammar mistakes,
> please note that there are more weighty comments later on.

Ooh, lots of things to correct. Consider stuff where I don't
additionally comment has been seen and corrected. I'm applying the
deltas from all three reviews at the same time, starting with comments
on Karl's (he asks the most questions :-) ), then following up on Greg.

> I have to admit, it confused me to see that we're passing a "read
> callback" to something called svn_repos_fs_change_rev_prop2 :-). I'll
> see if I can figure that out when I get to the code...

This is documented in the public API definition of
svn_repos_fs_change_rev_prop2(), in svn_repos.h . The read callback is
used to ensure that all changed-paths in the revision being modified are
readable by the client making the revprop change. If one of the
changed-paths is unreadable, the revpropset fails with an authz access
denied error. This behaviour conforms to the policy for revision
properties outlined in notes/authz-policy.txt .

In fact, I separated this function change out, because it is slightly
special: in svnserve, it requires manual write access control, as well
as read access verification by the svn_repos function. It is the only
command in the svnserve commandset that requires both a callback and
explicit verification. I was considering documenting this in
authz-policy.txt, but decided that this was because of the specific
behaviour of svnserve, and shouldn't be a general authz implementation
policy.

So, to sum it up, we are passing a read access callback to a write
operation, but the revpropset operation does indeed use it to verify
readability. It just uses that as a condition for success or failure of
the write operation later.

> Please do two spaces after a sentence-ending period above (as you do
> below).

Corrected throughout the code where you found it. I wish to plead
europeanness, where the Two Spaces are not the norm. I've been trying
to get used to this in my email conversations, but sometimes the old
reflex still steps in.

> However, perhaps the descriptive text above should mention that the
> file format is the same as that used by mod_authz_svn in http://
> access, and that in fact the two servers can share the same file.
> That's something a reader of the config file would want to know.

> Same thing here -- since the format is compatible w/ mod_authz_svn,
> say so.

Actually, I was going to submit a patch to the svnbook (outside SoC),
updating the documentation concerning path-based access control,
defining it as a common feature to both RA methods, with a note to the
effect of interoperability. Would that not be a better place to say
that? Or perhaps mention it in both?

> This isn't really due to your change, but I find both the comment and
> the names of these constants a bit misleading. These files are just
> what is looked for *by default*, right? Depending on how the admin
> configures things for run-time, totally different files might be
> looked for. I wish the comment and the names expressed this better.

Actually, I'm looking at my code with a puzzled expression on my face,
as I realize I defined this constant by miming the definition for the
'passwd' symbol, but I don't actually use it anywhere in the code. In
this case, it can go, unless you want to leave it for consistency? I'll
leave it in my changeset; if you want it out, it's simple enough to hack
out of the patch.

What is certain is that if no authz file is specified in
svnserve.conf, svnserve doesn't try to be clever: it assumes no authz
and carries on.

> Also like Greg, this feels a result of squishiness elsewhere in the
> system to me. At least clarify what you mean by "malformed" -- it
> seems like you're saying that the path is malformed w.r.t. authz if
> it does not have a leading "/", right?

Paths passed to authz have to be absolute paths in the repository being
queried. This is not a problem given that the API doesn't give any way
of specifying an absolute "base path" and subsequent paths relative to
this base. So all authz lookups use absolute paths, everywhere in the code.

The problem is that some bits of svnserve (from what I recall, the
target URL during a copy operation is one) strip URLs a little too
aggressively, and turn svn://server/repos/path/inside to path/inside.
They then pass this "absoluteish" path to authz, which throws an
infinite loop because it's stripping the path down to "" looking for the
final "/" state.

> But more to the point, which authz routines go into an infinite loop
> if passed the empty string instead of "/"? Can we fix them?

The authz routine that fails is the one invoked for any lookup. It is
actually the one doing a nonrecursive lookup (authz_get_path_access off
the top of my head). It strips back the path, looking after each split
if the new parent path determined has any applicable rules. The stop
condition for that loop is when the string before splitting is "/",
meaning we have traversed right back to the root and found no applicable
rules.

The proper way to fix this is, in my opinion, not to add a check for "/
or the empty string" in authz code, but to fix up svnserve so that it
splits less aggressively where appropriate and always passes real
absolute paths into the authz routines (and possibly add a single check
to verify that path[0] == '/' in the authz code).

The fix I have inserted (which you're commenting on) was meant to be
temporary, because time for my SoC is running a little short. Yes,
temporary fixes are evil, but this one isn't that evil, because it works
consistently (that function is the central hub for all authz requests in
svnserve, and the problem is localized in svnserve for now) and at least
has the merit of documenting the existence of a problem. A more
thorough investigation of svnserve and more permanent fix to this path
problem is also high on my post-soc todo.

So, to sum up all this verbosity: if you're okay with a firm kind of
squishiness for the time being, I'd like to leave it at that for this
patch. Once this patch has gone in, I'll investigate the matter further
and attempt to produce a more localized and permanent fix. Given the
dates, unless this patch goes through after this first review, I
probably won't have time to do it on SoC time, if that's okay (just
signed for a flat, moving homes, that kind of stuff).

> The parameter's name is REQUIRED, not REQUIRED_ACCESS. How is the
> NEEDS_USERNAME parameter used? The doc string doesn't mention it,
> nor does it mention B, though B's function can be deduced more easily
> (it's the source of the "authz directives" referred to earlier,
> presumably).

Roger, the docstrings are a little out of sync with the prototypes all
over, because of several refactoring passes that changed things a
little. I'm not yet up to speed with updating the docstring the second
I change the function I'm afraid. Corrected.

>> + /* Get authz's opinion on the access. If authz is disabled in
>> the + server config, this will just allow access blindly. This
>> will + make the function only consider blanket access rules
>> when + deciding what to do. */
>
> I didn't really understand the last sentence in that comment.

Greg commented on this and said to remove it. That comment is made with
the assumption the reader knows what the code looked like before the
addition of authz, which is bad. Removed. Actually, removed everything
but the first sentence. The blurb about the default authz-less
behaviour is documented in the callee's docstring.

> Ben Collins-Sussman appreciates the reminder :-).

My pleasure :-).

> There are a lot of parameters here; planning to document them? :-)
> (Documenting by reference to another function is fine, just don't
> leave the reader wondering.)

Again, I plead an incompletely trained habit of documenting synchronously.

> By the way, why the extra blank line " *\n"? :-)

The Voices told me to do it.

Okay, after a quick talk around the coffee machine, the Voices say that
my evil plans can be put into action some other way, and that the extra
blank line is no longer necessary for our operation. I have therefore
dispatched an assassin accordingly.

> Why is it obvious that ANONYMOUS won't work? Just because it's write
> access? If so, that would seem like a site policy decision, not an
> assumption for us to hardcode in.

Because of a combination of things that means that the client is
currently (ie. before this authentication attempt) running on the
ANONYMOUS mechanism (because ! b->user, and no auth mechanism currently
supported doesn't provide a username for the use of svnserve). So,
sending a reply "Hey, your ANONYMOUS access mechanism was unable to get
you access to this resource. You can try me again with a different
mechanism from the following list: ANONYMOUS CRAM-MD5." sounds a little
surrealist.

But granted the comment could be clearer on the reasons. And it is now so.

> I thought it's called must_have_access() now? And how does this code
> even compile? :-)

Strange thing is that here it does compile, link, pass tests, what have
you. And no, it shouldn't compile. Grmbl. I'll put it down to the bad
agricultural results of a small planet somewhere in the vicinity of
Betelgeuse and check my compile environment for explanations.

After some hours of investigation, my compile environment was borked.
This also made some tests pass when they should have revealed two
serious pool lifetime management bugs in the locking code I introduced.
  These are now corrected.

> Minor style nit: try to put a callback and its baton on the same
> line, especially if other pairs are being kept together elsewhere in
> the argument list.

Noted.

> I'm a little confused about when SVN_CMD_ERR is appropriate and when
> SVN_ERR is appropriate, in this function, but I think that's most
> likely due to my ignorance...

This is a little confusing, and quite uselessly. Before, commit()
called add_lock_tokens wrapped in a SVN_CMD_ERR, and used SVN_ERR inside
the function. At one point, I called a function in add_lock_tokens that
could also return a non-client error, and so broke the outer SVN_CMD_ERR
down to SVN_ERR and made returns inside the function either SVN_ERR or
SVN_CMD_ERR, depending on the context.

Now however, it looks like that function (a proto-lookup_access()) no
longer returns any errors that justify this change (worse, I'd forgotten
to wrap the malformed data error back to the client), so I'll undo it.

> Is there a reason we don't say more in this error, for example the
> path?

The error in question is a client command error, and is therefore
wrapped back across the network to the client. In this case, the svn
guidelines (and authz guidelines) are clear: leak as little information
as possible when it comes to authorizations. In this case, saying
"access denied for path /foo/bar" could reveal the existence of such a
path. I'll admit I must have played it overly safe in places, but best
too paranoid imho (but I wouldn't say that if They weren't after me).

Plus, when logging is available, such paths and details could be logged
server side. The client would then receive a non-revealing "access
denied" error, and the admin would know why and what to correct if need be.

Okay, that's Karl's review parsed and processed. Looking back through
Greg's: I've corrected the things you've pointed out, and have no
specific comment on the changes.

So, here is the corrected changeset. This compiles, links and tests
fine on my machine, which, as we have seen above, means absolutely
diddley squat. Please, do try on your setups.

Thanks all for the careful reviews!
- Dave.

[[[
Make svnserve perform authz lookups during the processing of client
operations.

* notes/authz_policy.txt: Update the list of functions that take authz
     callbacks. Update the list of operations that require manual
     authz lookups.

* subversion/include/svn_config.h
   (SVN_CONFIG_OPTION_AUTHZ_DB): New define.

* subversion/libsvn_repos/repos.c
   (create_conf): update the contents of svnserve.conf to document the
     new configuration option for authz. Create an 'authz'
     configuration file with documentation and a few authz setting
     examples.

* subversion/libsvn_repos/repos.c
   (SVN_REPOS__CONF_AUTHZ): New define.

* subversion/svnserve/serve.c
   (server_baton_t): Add an authz configuration handle, and the
     repository name, both for authz lookups.
   (authz_check_access, authz_check_access_cb, lookup_access): New
     functions.
   (must_have_write_access): Rename to must_have_access.
   (must_have_access): Add optional authz lookups if a specific path is
     passed. All callers updated.
   (change_rev_prop): Check for write access to the repository and pass
     an authz read callback to svn_repos_fs_change_rev_prop2.
   (accept_report, rev_proplist, rev_prop, log_cmd, get_locations,
    get_file_revs): Pass an authz read callback to libsvn_repos
     functions.
   (add_lock_tokens): Perform authz lookups during the processing of
     lock tokens. Fix a returned error message.
   (commit): Pass an authz read/write callback to the commit editor.
   (get_file, get_dir, check_path, stat, lock, lock_many, unlock,
    unlock_many, get_lock, get_locks): Check authz on the requested
     path(s), either manually or by passing the authz read callback.
   (find_repos): Read the authz configuration if present and add the
     necessary information to the server baton.
]]]

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org