Edgewall Software
Modify

Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#10208 closed defect (fixed)

AuthzSourcePolicy does not conform to mod_authz_svn rules

Reported by: dmcr@… Owned by: Remy Blank
Priority: normal Milestone: 0.12.3
Component: version control Version: 0.12.1
Severity: normal Keywords: authzpolicy patch verify
Cc: itamarost@…, jordi@… Branch:
Release Notes:

AuthzSourcePolicy conforms better to mod_authz_svn rules

API Changes:

versioncontrol: use the requested (path, rev) for the identity of a Node instead of (created_path, created_rev) (r10914)

Internal Changes:

Description

Hi,

I noticed a discrepancy between AuthzSourcePolicy and mod_authz_svn that I think qualifies as a bug in the AuthzSourcePolicy implementation.

We have a user that wants to give universal read-only access to his repo's 'tags' project, but only give specific user access to the 'trunk' and 'branches' projects. In the simplest case, if I give someone r or rw access to tags, but not to trunk and branches, nor to the parent level, they can see all the files in the tags project (and none of the others) when accessing the repo using mod_authz_svn, by using a browser for example.

Example:

[myrepo:/] user1 = r

[myrepo:/trunk] user2 = r

[myrepo:/tags] user3 = r

In this example, user1 and user3 can both see the contents of the tags project when using a browser to directly view the svn 'myrepo' repo. But user3 can't see the trunk project while user1 can. This is what you would expect.

But when they try to browse from inside the associated Trac project, user3 can't see any files or directories in the svn 'tags' project, whereas user1 can. This is because the files in the tags project are the result of 'svn copy' operations from the trunk project to the tags project. Presumably, this does not copy the actual data, but instead provides a pointer in the tags project to the data residing in the trunk project, since the log file shows permission failures for trunk-resident files when browsing the tags project. So it would appear that AuthzSourcePolicy is coded to check permissions on the pointers, which it should *not* do, since the internal arrangement of data should be of no concern to the user.

But as a result, my Trac administrator has to choose between not giving the world read-only access to his tags project, or also giving the world read-only access to his trunk project, which he does not want to do since some of the code there is confidential and also not ready to run.

So it would be very helpful if AuthzSourcePolicy were to conform to the behavior of mod_authz_svn in this regard.

Thanks,

Dennis

Attachments (3)

authz-policy-10208-itamaro-v1.patch (701 bytes ) - added by Itamar Ostricher 12 years ago.
suggested patch to resolve the issue
10208-requested-rev-r10910.patch (6.6 KB ) - added by Remy Blank 12 years ago.
Use .path and .rev for permission checks.
10208-requested-rev-2-r10910.patch (12.6 KB ) - added by Remy Blank 12 years ago.
Fix a few issues in the source browser.

Download all attachments as: .zip

Change History (25)

comment:1 by dmcr@…, 13 years ago

Hi again,

I know you guys are very busy, but I was wondering if someone was likely to be working on this ticket. I myself am not familiar with the internals of the svn native filesystem, and was hoping that someone at your end was.

Thanks,

Dennis

comment:2 by Remy Blank, 13 years ago

Milestone: next-minor-0.12.x

Nobody is currently working on it, but we should definitely take a look.

comment:3 by Itamar Ostricher, 12 years ago

Cc: itamarost@… added
Keywords: authzpolicy added

Just ran into the same issue today (also with trac-0.12.stable).

Did not have any luck figuring out why it happens. Hope I'll find some time taking a deeper look…

comment:4 by Itamar Ostricher, 12 years ago

OK, here's what I came up with:

The AuthzSourcePolicy::check_permission() method uses resource.id as the node path in checking against the authz file, but the Node class sets the resource.id to be the created_path property of the Node and not the path property, which makes the AuthzSourcePolicy to return the permission according to the created_path instead of the current actual path.

The fix is obvious (let resource.id be path instead of created_path), but there must be some other reason the resource.id was set this way… What is that reason? How can this be fixed?

by Itamar Ostricher, 12 years ago

suggested patch to resolve the issue

comment:5 by Itamar Ostricher, 12 years ago

Keywords: patch verify added

I have cofirmed my concern that this issue also manifests as a security vulnrability. To illustrate, assume I have this repository:

- Private
- Public
  - TestDir
    - test_file

And my SVN authz file gives global read for /Public, and no access to everything else.

If I svn-move TestDir from Public to Private, I can still direct my browser to /browser/Private/TestDir/test_file, and get the file!

I have attached a patch to this ticket that resolves the issue.

comment:6 by Remy Blank, 12 years ago

Milestone: next-minor-0.12.x0.12.3
Owner: set to Remy Blank

I have also always wondered why we use created_*. Can you please confirm that svn_authz_mod does the right thing with the test repository you created?

comment:7 by Christian Boos, 12 years ago

Btw, I did the same change when working on #9976, so I'm also OK for the change.

in reply to:  6 comment:8 by Itamar Ostricher, 12 years ago

Replying to rblank:

I have also always wondered why we use created_*. Can you please confirm that svn_authz_mod does the right thing with the test repository you created?

Confirmed

in reply to:  7 comment:9 by Christian Boos, 12 years ago

Replying to cboos:

Btw, I did the same change when working on #9976, so I'm also OK for the change.

In order to answer Remy's question about why we use created_, I re-read #3340, #7744 and r8797 and now I have a doubt: we still have self.rev = self.created_rev in SubversionNode.__init__, so the change I talked about in #3340 never happened. As a consequence, (self.path, self.rev) can be inconsistent and we should use (self.path, self._requested_rev). And in trunk we can probably make the switch to self.rev = self._requested_rev.

comment:10 by Dennis McRitchie <dmcr@…>, 12 years ago

So Christian, are you saying that 0.12.1 can be patched with the fix for #9976, plus the patch for this ticket as changed below?

resource = property(lambda self: Resource('source', self.path,

version=self.requested_rev, parent=self.repos.resource))

Also, I'm not sure what your last sentence means: does it refer to an additional change that's needed for the fix? If so, where does it go?

Thanks, Dennis

comment:11 by Christian Boos, 12 years ago

version=self._requested_rev, yes. But I can't test at the moment, so maybe Itamar could confirm…

The last two sentences just mean that the above looks a bit ugly, and while it's OK for 0.12-stable we can probably do better on trunk and do the "planned" change to fix the Node.rev at the version which was requested.

in reply to:  11 ; comment:12 by Itamar Ostricher, 12 years ago

Replying to cboos:

version=self._requested_rev, yes. But I can't test at the moment, so maybe Itamar could confirm…

I think this change might be problematic. The _requested_rev property is specific to the SubversionNode implementation, while the resource property is defined in the api, which means this change should work fine with SVN repositories, but will fail with other implementations that do not define _requested_rev

I don't known why the self.rev = self.created_rev line exists, but I would like to go with the switch to self.rev = self._requested_rev in the stable branch, unless I'm missing deeper repercussions…

in reply to:  12 comment:13 by Christian Boos, 12 years ago

Replying to itamaro:

Replying to cboos:

version=self._requested_rev, yes. But I can't test at the moment, so maybe Itamar could confirm…

I think this change might be problematic. The _requested_rev property is specific to the SubversionNode implementation, while the resource property is defined in the api, which means this change should work fine with SVN repositories, but will fail with other implementations that do not define _requested_rev

Sure, sorry for the approximations, I didn't test.

I don't known why the self.rev = self.created_rev line exists, but I would like to go with the switch to self.rev = self._requested_rev in the stable branch, unless I'm missing deeper repercussions…

I'm not sure about possible other repercussions either, but just seeing that I wanted to do that for quite some time and that it nevertheless "resisted" a few major releases, I don't feel inclined to do it now on stable, especially on a .3 that will get released "soon" ;-)

comment:14 by Jordi Mallach, 12 years ago

Cc: jordi@… added

comment:15 by Remy Blank, 12 years ago

I would like to finish this, but I have a hard time understanding what the solution should be. Actually, I don't understand how .created_path can be different from .path: for a normal commit, they must be identical, and for a copy, it should be the path of the destination, hence also identical to the path passed to the constructor. Or would this be the case only if the copy also had a change to the file's content? So they would differ only if the operation was a pure copy? Even in that case, I don't see the use of .created_path.

I do see the use of .created_rev, so that one should certainly stay. But maybe we should simply remove .created_path (or set it equal to .path for backward compatibility). For the .resource property, I would keep .created_rev (it won't make any difference for authz, as the revision isn't used, but it feels more correct).

by Remy Blank, 12 years ago

Use .path and .rev for permission checks.

comment:16 by Remy Blank, 12 years ago

10208-requested-rev-r10910.patch includes Itamar's patch changing .resource to use .path and .rev instead of .created_path and .created_rev, and also fixes SubversionNode to set .rev to the requested revision instead of .created_rev (and incidentally removes ._requested_rev.

Of course, this breaks some tests that check the value of .rev, these are fixed in the patch as well. All tests pass now. On to some more testing, possibly with more unit tests. A second pair of eyes appreciated, too.

comment:17 by Itamar Ostricher, 12 years ago

Tested Remy's updated patch. Works for me, with my test cases.

comment:18 by Christian Boos, 12 years ago

There are a few issues with the patch, as I feared in the TracBrowser:

  • when visiting a "deleted" file, starting from the changeset which deleted it, one can see its last revision but from there we can't navigate to previous revisions … forget it, also happens without the patch ;-)
  • same thing for moves actually, when viewing a file or a folder below a directory which has been copied, Last Change goes to a non-existing path (this problem only with the patch!)

Bottom line: the change is very positive (e.g. #10358 verified to be fixed), but there are some more adaptations needed in the browser to cope with it, at least for the Last Change link.

by Remy Blank, 12 years ago

Fix a few issues in the source browser.

comment:19 by Remy Blank, 12 years ago

10208-requested-rev-2-r10910.patch fixes a few issues I found in the browser, for example all files listed in a directory were showing the same revision. I wasn't able to reproduce the issue described in comment:18, though. Can you please give some details about the repository "shape" and the actions you execute?

It looks like there are quite a few places in the browser where .rev and .created_rev were used interchangeably, and I'm not all too familiar with the code, so I wouldn't mind a little help with polishing this patch (also, I'm very tired ATM, which doesn't help, so I'll stop for now).

To find the remaining issues, perhaps it would make sense to write a small script that scrapes a set of pages, once without and once with the patch, and shows the diffs, possibly hiding some known differences. This could even be generally useful, e.g. for refactoring.

comment:20 by Christian Boos, 12 years ago

My issue with Last Change is gone with the new patch. The problem was actually happening for a move operation (e.g. svn mv trunk/dirA trunk/dirB, commit 10) then the Last Change for a dirB/sub path was pointing to /changeset/<requested-rev>/<created-path> (i.e. /changeset/10/trunk/dirA/sub) which didn't exist (anymore).

Looks like you nailed all the cases, as I couldn't trigger another issue. Only further testing will tell (we have given ourselves a week for this, so I think it's quite safe to commit 10208-requested-rev-2-r10910.patch now).

comment:21 by Remy Blank, 12 years ago

Resolution: fixed
Status: newclosed

Patch applied in [10914]. It may be a good idea to add a specific note about this fix in the release notes for 0.13.2rc1, and ask people to specifically test for regressions in the source browser (and possibly the log and changeset views, too).

comment:22 by Christian Boos, 12 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.