Details

      Description

      The authentication with authz_svn should allow wildcards like 
      
        [/]
        @team = rw
       
        [/*/tags/]
        @team = r
      
      In several projects, there can be folders named identically, and setting up 
      rules for each project is very cumbersome. Using wildcards like in the above 
      example would simplify the task of setting up security, and could fasten the 
      processing of the rules file by far.
      
      There was already a discussion about this: 
      http://subversion.tigris.org/servlets/ReadMsg?list=users&msgNo=35110
       
      Additionally, I found this thread:
      http://subversion.tigris.org/servlets/ReadMsg?listName=users&msgNo=27861
       
      Kind regards,
      
      Jochen Wendebaum
      

      Original issue reported by jwendebaum

      1. 1_record-line-numbers.diff
        6 kB
        Subversion Importer
      2. 2_non-recursive-wildcards.diff
        4 kB
        Subversion Importer
      3. 3_recursive-repo-walk.diff
        16 kB
        Subversion Importer
      4. 4_wildcards-tests.diff
        4 kB
        Subversion Importer

        Issue Links

          Activity

          Hide
          lgo Lieven Govaerts added a comment -

          David Anderson and I have been discussing this feature some time ago on the dev
          list:
          
          http://svn.haxx.se/dev/archive-2006-01/0072.shtml
          http://svn.haxx.se/users/archive-2005-12/0988.shtml
          

          Show
          lgo Lieven Govaerts added a comment - David Anderson and I have been discussing this feature some time ago on the dev list: http://svn.haxx.se/dev/archive-2006-01/0072.shtml http://svn.haxx.se/users/archive-2005-12/0988.shtml
          Hide
          hwright Hyrum Wright added a comment -

          See issue 2958, which introduces the idea of $user name substitution, a la:
          
          [/users/$user]
          $user = rw
          

          Show
          hwright Hyrum Wright added a comment - See issue 2958, which introduces the idea of $user name substitution, a la: [/users/$user] $user = rw
          Hide
          subversion-importer Subversion Importer added a comment -

          I'm goint to implement this functionality for a client, starting from the patch
          proposed by Josh Siegel in
          http://svn.haxx.se/dev/archive-2005-02/0631.shtml#start
          
          As soon as I have a working implementation I will post it here and propose it
          for inclusion.
          

          Original comment by rodrigo_gallardo

          Show
          subversion-importer Subversion Importer added a comment - I'm goint to implement this functionality for a client, starting from the patch proposed by Josh Siegel in http://svn.haxx.se/dev/archive-2005-02/0631.shtml#start As soon as I have a working implementation I will post it here and propose it for inclusion. Original comment by rodrigo_gallardo
          Hide
          subversion-importer Subversion Importer added a comment -

          In our application project managers have the responsibility for creating project
          subdirectories under their project areas e.g. /dev/managerXarea/newproject/
          whereas developers create the code underneath "newproject". Currently this means
          that the repository administrators need to add rules to authz every time the
          project manager creates a new project. It would be great to have this set up in
          authz as follows
          
          [groups]
          repositoryAdmins = admin1, admin2
          projectManagers = managerX, managerY
          managerXDevelopers = developer1, developer2
          managerYDevelopers = developer1, developer3
          
          [/]
          @repositoryAdmins = rw
          
          [/dev/managerXarea/]
          managerX = rw
          
          [/dev/managerXarea/*/]
          @managerXDevelopers = rw
          managerX = r
          
          [/dev/managerYarea/]
          managerY = rw
          
          [/dev/managerYarea/*/]
          @managerYDevelopers = rw
          managerY = r
          

          Original comment by carnun

          Show
          subversion-importer Subversion Importer added a comment - In our application project managers have the responsibility for creating project subdirectories under their project areas e.g. /dev/managerXarea/newproject/ whereas developers create the code underneath "newproject". Currently this means that the repository administrators need to add rules to authz every time the project manager creates a new project. It would be great to have this set up in authz as follows [groups] repositoryAdmins = admin1, admin2 projectManagers = managerX, managerY managerXDevelopers = developer1, developer2 managerYDevelopers = developer1, developer3 [/] @repositoryAdmins = rw [/dev/managerXarea/] managerX = rw [/dev/managerXarea/*/] @managerXDevelopers = rw managerX = r [/dev/managerYarea/] managerY = rw [/dev/managerYarea/*/] @managerYDevelopers = rw managerY = r Original comment by carnun
          Hide
          subversion-importer Subversion Importer added a comment -

          In our application project managers have the responsibility for creating project
          subdirectories under their project areas e.g. /dev/managerXarea/newproject/
          whereas developers create the code underneath "newproject". Currently this means
          that the repository administrators need to add rules to authz every time the
          project manager creates a new project. It would be great to have this set up in
          authz as follows
          
          [groups]
          repositoryAdmins = admin1, admin2
          projectManagers = managerX, managerY
          managerXDevelopers = developer1, developer2
          managerYDevelopers = developer1, developer3
          
          [/]
          @repositoryAdmins = rw
          
          [/dev/managerXarea/]
          managerX = rw
          
          [/dev/managerXarea/*/]
          @managerXDevelopers = rw
          managerX = r
          
          [/dev/managerYarea/]
          managerY = rw
          
          [/dev/managerYarea/*/]
          @managerYDevelopers = rw
          managerY = r
          

          Original comment by carnun

          Show
          subversion-importer Subversion Importer added a comment - In our application project managers have the responsibility for creating project subdirectories under their project areas e.g. /dev/managerXarea/newproject/ whereas developers create the code underneath "newproject". Currently this means that the repository administrators need to add rules to authz every time the project manager creates a new project. It would be great to have this set up in authz as follows [groups] repositoryAdmins = admin1, admin2 projectManagers = managerX, managerY managerXDevelopers = developer1, developer2 managerYDevelopers = developer1, developer3 [/] @repositoryAdmins = rw [/dev/managerXarea/] managerX = rw [/dev/managerXarea/*/] @managerXDevelopers = rw managerX = r [/dev/managerYarea/] managerY = rw [/dev/managerYarea/*/] @managerYDevelopers = rw managerY = r Original comment by carnun
          Hide
          subversion-importer Subversion Importer added a comment -

          Ok, here's my patch for this issue. There's still at least one problem
          with it, but otherwise it's ready and in use in a client's site. The
          patch was developed against 1.4.4 but this version is rebased against
          trunk revision 28157
          
          I've broken up the patch into 4 pieces to make it easier to understand
          and review.
          
          I believe there's no 'natural' way to resolve the conflicts created by
          having several globs match a single path, thus we opted for using
          "order of declaration" as precedence.
          
          The first part of the patch, record-line-numbers.diff, makes the
          config file mechanism record the starting line number of each section,
          for use by the rest.
          
          The second patch, non-recursive-wildcards.diff, handles matching
          single paths against the globs. I have not tested how this interacts
          with the new ~user and aliases syntaxes.
          
          The third patch, recursive-repo-walk.diff, implements walking the repo
          when recursive access is requested. *This patch modifies the API,*
          because in order to let it walk the repo the callers of
          svn_repos_authz_check_access need to provide a svn_fs_root_t*
          
          My patch currently is passing the root for the youngest revision in
          the repo. I'm not certain this is the best idea but can't think of a
          scenario where it would actually matter.
          
          And, svnserve doesn't work. Aparently I'm not passing the right root,
          but I haven't really understood the code enough to tell.
          
          The last part of the patch, wildcards-tests.diff, adds some simple
          excercising of the code to the test suite.
          
          The patch set compiles cleanly and the resulting code passes all of
          the original test suite + my new tests.
          
          I'll appreciate your reviews and comments.
          

          Original comment by rodrigo_gallardo

          Show
          subversion-importer Subversion Importer added a comment - Ok, here's my patch for this issue. There's still at least one problem with it, but otherwise it's ready and in use in a client's site. The patch was developed against 1.4.4 but this version is rebased against trunk revision 28157 I've broken up the patch into 4 pieces to make it easier to understand and review. I believe there's no 'natural' way to resolve the conflicts created by having several globs match a single path, thus we opted for using "order of declaration" as precedence. The first part of the patch, record-line-numbers.diff, makes the config file mechanism record the starting line number of each section, for use by the rest. The second patch, non-recursive-wildcards.diff, handles matching single paths against the globs. I have not tested how this interacts with the new ~user and aliases syntaxes. The third patch, recursive-repo-walk.diff, implements walking the repo when recursive access is requested. *This patch modifies the API,* because in order to let it walk the repo the callers of svn_repos_authz_check_access need to provide a svn_fs_root_t* My patch currently is passing the root for the youngest revision in the repo. I'm not certain this is the best idea but can't think of a scenario where it would actually matter. And, svnserve doesn't work. Aparently I'm not passing the right root, but I haven't really understood the code enough to tell. The last part of the patch, wildcards-tests.diff, adds some simple excercising of the code to the test suite. The patch set compiles cleanly and the resulting code passes all of the original test suite + my new tests. I'll appreciate your reviews and comments. Original comment by rodrigo_gallardo
          Hide
          subversion-importer Subversion Importer added a comment -

          Created an attachment (id=821)
          Record section's starting line numbers
          
          

          Original comment by rodrigo_gallardo

          Show
          subversion-importer Subversion Importer added a comment - Created an attachment (id=821) Record section's starting line numbers Original comment by rodrigo_gallardo
          Hide
          subversion-importer Subversion Importer added a comment -

          Attachment 1_record-line-numbers.diff has been added with description: Record section's starting line numbers

          Show
          subversion-importer Subversion Importer added a comment - Attachment 1_record-line-numbers.diff has been added with description: Record section's starting line numbers
          Hide
          subversion-importer Subversion Importer added a comment -

          Created an attachment (id=822)
          Enable globs in authz file for non recursive permission checking.
          
          

          Original comment by rodrigo_gallardo

          Show
          subversion-importer Subversion Importer added a comment - Created an attachment (id=822) Enable globs in authz file for non recursive permission checking. Original comment by rodrigo_gallardo
          Hide
          subversion-importer Subversion Importer added a comment -

          Attachment 2_non-recursive-wildcards.diff has been added with description: Enable globs in authz file for non recursive permission checking.

          Show
          subversion-importer Subversion Importer added a comment - Attachment 2_non-recursive-wildcards.diff has been added with description: Enable globs in authz file for non recursive permission checking.
          Hide
          subversion-importer Subversion Importer added a comment -

          Created an attachment (id=823)
          Walk repo tree for checking recursive access requests
          
          

          Original comment by rodrigo_gallardo

          Show
          subversion-importer Subversion Importer added a comment - Created an attachment (id=823) Walk repo tree for checking recursive access requests Original comment by rodrigo_gallardo
          Hide
          subversion-importer Subversion Importer added a comment -

          Attachment 3_recursive-repo-walk.diff has been added with description: Walk repo tree for checking recursive access requests

          Show
          subversion-importer Subversion Importer added a comment - Attachment 3_recursive-repo-walk.diff has been added with description: Walk repo tree for checking recursive access requests
          Hide
          subversion-importer Subversion Importer added a comment -

          Created an attachment (id=824)
          testsuite
          
          

          Original comment by rodrigo_gallardo

          Show
          subversion-importer Subversion Importer added a comment - Created an attachment (id=824) testsuite Original comment by rodrigo_gallardo
          Hide
          subversion-importer Subversion Importer added a comment -

          Attachment 4_wildcards-tests.diff has been added with description: testsuite

          Show
          subversion-importer Subversion Importer added a comment - Attachment 4_wildcards-tests.diff has been added with description: testsuite
          Hide
          subversion-importer Subversion Importer added a comment -

          The patch set was also sent to the mailing list:
          http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=133204
          

          Original comment by rodrigo_gallardo

          Show
          subversion-importer Subversion Importer added a comment - The patch set was also sent to the mailing list: http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=133204 Original comment by rodrigo_gallardo
          Hide
          kfogel Karl Fogel added a comment -

          Here's a "simple patch which seams to work for us in order to use 
          wildcards in authz access file paths":
          
             http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=139267
             From: Gerald Macinenti <gerald.macinenti@letitwave.fr>
             To: dev@subversion.tigris.org
             Subject: [PATCH] issue #2662, yet another authz wildcards support patch...
             Date: Wed, 28 May 2008 17:05:51 +0200
             Message-ID: <483D74CF.4070509@letitwave.fr>
          
          See the thread; I followed up with some brief review.
          

          Show
          kfogel Karl Fogel added a comment - Here's a "simple patch which seams to work for us in order to use wildcards in authz access file paths": http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=139267 From: Gerald Macinenti <gerald.macinenti@letitwave.fr> To: dev@subversion.tigris.org Subject: [PATCH] issue #2662, yet another authz wildcards support patch... Date: Wed, 28 May 2008 17:05:51 +0200 Message-ID: <483D74CF.4070509@letitwave.fr> See the thread; I followed up with some brief review.
          Hide
          maxb Max Bowsher added a comment -

          Triage: There's a lot of work gone into producing patches here, we ought to
          review them before 1.6.
          

          Show
          maxb Max Bowsher added a comment - Triage: There's a lot of work gone into producing patches here, we ought to review them before 1.6.
          Hide
          cmpilato C. Michael Pilato added a comment -

          Move all 1.6.0 issues to 1.6.x.  (The 1.6.0 ship has sailed.)
          

          Show
          cmpilato C. Michael Pilato added a comment - Move all 1.6.0 issues to 1.6.x. (The 1.6.0 ship has sailed.)
          Hide
          cmpilato C. Michael Pilato added a comment -

          Fix issue type.  We're discontinuing the use of PATCH.
          

          Show
          cmpilato C. Michael Pilato added a comment - Fix issue type. We're discontinuing the use of PATCH.
          Hide
          cmpilato C. Michael Pilato added a comment -

          Add 'patch' keyword to issues which have an attachment of the 'patch' type.
          

          Show
          cmpilato C. Michael Pilato added a comment - Add 'patch' keyword to issues which have an attachment of the 'patch' type.
          Hide
          subversion-importer Subversion Importer added a comment -

          I'm running the patch from the discussion in this message: 
          http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=34162 against svn 1.6.12 
          and it seems to work fine.
          
          This is a really sorely missed feature. People ask for it on the mailing lists quite a bit.
          
          

          Original comment by jon

          Show
          subversion-importer Subversion Importer added a comment - I'm running the patch from the discussion in this message: http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=34162 against svn 1.6.12 and it seems to work fine. This is a really sorely missed feature. People ask for it on the mailing lists quite a bit. Original comment by jon
          Hide
          stsp Stefan Sperling added a comment -

          Jon, I'm glad you're testing this patch and showing interest. The patch looks
          fine to me, and it has already received quite a bit of review when it was first
          posted.
          
          However, the final statement in the thread was that this patch was still waiting
          for a regression test to be written. I also think we should have a test in our
          test suite that makes sure that the feature continues to work in the face of
          future changes people make to Subversion. You might know that the authz feature
          has historically been a bit of a blind spot in our testing (see issue #3242,
          which I personally attribute to lack of testing).
          
          Would you have time and energy to invest into sending a patch for a regression
          test? You could extend the existing set of authz tests in the file
          subversion/tests/cmdline/authz_tests.py. If you have any questions about writing
          tests or sending patches in general please just ask on the developer mailing
          list. You can also refer to our community guide:
          http://subversion.apache.org/docs/community-guide/
          If you're not interested in writing a test, that's fine. Please let us know so
          that someone else will hopefully pick up the ball. Thanks!
          

          Show
          stsp Stefan Sperling added a comment - Jon, I'm glad you're testing this patch and showing interest. The patch looks fine to me, and it has already received quite a bit of review when it was first posted. However, the final statement in the thread was that this patch was still waiting for a regression test to be written. I also think we should have a test in our test suite that makes sure that the feature continues to work in the face of future changes people make to Subversion. You might know that the authz feature has historically been a bit of a blind spot in our testing (see issue #3242, which I personally attribute to lack of testing). Would you have time and energy to invest into sending a patch for a regression test? You could extend the existing set of authz tests in the file subversion/tests/cmdline/authz_tests.py. If you have any questions about writing tests or sending patches in general please just ask on the developer mailing list. You can also refer to our community guide: http://subversion.apache.org/docs/community-guide/ If you're not interested in writing a test, that's fine. Please let us know so that someone else will hopefully pick up the ball. Thanks!
          Hide
          stsp Stefan Sperling added a comment -

          I've taken a look at Gerald Macinenti's simple patch. It doesn't properly
          address the problem described by Greg Hudson in this post to dev@ in 2006:
          http://svn.haxx.se/dev/archive-2006-05/0018.shtml
          Quote:
          "If I have a rule denying read access to */tags, and a caller wants to
          know if the user has recursive read access to /project/foo, doesn't the
          answer depend on whether /project/foo contains a "tags" somewhere? How
          can you know if the */tags wildcard entry is relevant without knowing
          what paths exist inside the tree?"
          
          Rodrigo Gallardo's patch (which I haven't looked at in detail) seems to address
          this problem, however.
          

          Show
          stsp Stefan Sperling added a comment - I've taken a look at Gerald Macinenti's simple patch. It doesn't properly address the problem described by Greg Hudson in this post to dev@ in 2006: http://svn.haxx.se/dev/archive-2006-05/0018.shtml Quote: "If I have a rule denying read access to */tags, and a caller wants to know if the user has recursive read access to /project/foo, doesn't the answer depend on whether /project/foo contains a "tags" somewhere? How can you know if the */tags wildcard entry is relevant without knowing what paths exist inside the tree?" Rodrigo Gallardo's patch (which I haven't looked at in detail) seems to address this problem, however.
          Hide
          cmpilato C. Michael Pilato added a comment -

          We don't add new features and functionality in patch releases.  Bumping this to
          1.7-consid.
          

          Show
          cmpilato C. Michael Pilato added a comment - We don't add new features and functionality in patch releases. Bumping this to 1.7-consid.
          Hide
          cmpilato C. Michael Pilato added a comment -

          Bump some stuff out of 1.7 that, at this point, we shouldn't really be considering.
          

          Show
          cmpilato C. Michael Pilato added a comment - Bump some stuff out of 1.7 that, at this point, we shouldn't really be considering.
          Hide
          subversion-importer Subversion Importer added a comment -

          It looks like this issue has not seen any activity since May, 2011.  Is there 
          still work being done to include this in a release?  Our organization would 
          benefit greatly from this feature.
          

          Original comment by dabrink2

          Show
          subversion-importer Subversion Importer added a comment - It looks like this issue has not seen any activity since May, 2011. Is there still work being done to include this in a release? Our organization would benefit greatly from this feature. Original comment by dabrink2
          Hide
          stsp Stefan Sperling added a comment -

          Sorry Doug, it looks like the existing resources the project has available are
          fully occupied with other areas of development at the moment. At least I myself
          not aware of anybody pursuing this right now. As usual, we welcome new
          contributors with a desire to tackle unsolved problems such as this one.
          
          A note to whoever would like to tackle this:
          One major roadblock for this issue is the question I linked to from desc20.
          We'll need a way to efficiently match patterns like */tags, without scanning
          every path in the repository. Scanning every path is a very expensive operation,
          possibly expensive enough to render this feature unusable in practice. I am not
          sure but perhaps Branko Čibej's recent experiments with a directory index could
          be leveraged:
          http://svn.apache.org/viewvc/subversion/trunk/notes/directory-index/?pathrev=1306839
          

          Show
          stsp Stefan Sperling added a comment - Sorry Doug, it looks like the existing resources the project has available are fully occupied with other areas of development at the moment. At least I myself not aware of anybody pursuing this right now. As usual, we welcome new contributors with a desire to tackle unsolved problems such as this one. A note to whoever would like to tackle this: One major roadblock for this issue is the question I linked to from desc20. We'll need a way to efficiently match patterns like */tags, without scanning every path in the repository. Scanning every path is a very expensive operation, possibly expensive enough to render this feature unusable in practice. I am not sure but perhaps Branko Čibej's recent experiments with a directory index could be leveraged: http://svn.apache.org/viewvc/subversion/trunk/notes/directory-index/?pathrev=1306839
          Hide
          stsp Stefan Sperling added a comment -

          And of course, it would help if somebody made an effort to port Rodrigo
          Gallardo's patch to the current trunk code and run some performance benchmarks
          on that code so we can get an impression of how it will scale to large repositories.
          

          Show
          stsp Stefan Sperling added a comment - And of course, it would help if somebody made an effort to port Rodrigo Gallardo's patch to the current trunk code and run some performance benchmarks on that code so we can get an impression of how it will scale to large repositories.
          Hide
          subversion-importer Subversion Importer added a comment -

          Comment regarding:
          "If I have a rule denying read access to */tags, and a caller wants to
          know if the user has recursive read access to /project/foo, doesn't the
          answer depend on whether /project/foo contains a "tags" somewhere? How
          can you know if the */tags wildcard entry is relevant without knowing
          what paths exist inside the tree?"
          
          I would read "*/tags" as "define the access rights to any path's (*) sub-folder 
          named 'tags' in my repo".  I would not want to be denied rights 
          to "/project/foo" in this scenario - I would want to be denied access only to 
          the "tags" sub-folder (/project/foo/myapp/tags).  Am I missing something? In 
          what scenario would someone want to deny access to a parent folder just because 
          it happens to contain a sub-folder with a specific name?
          

          Original comment by dabrink2

          Show
          subversion-importer Subversion Importer added a comment - Comment regarding: "If I have a rule denying read access to */tags, and a caller wants to know if the user has recursive read access to /project/foo, doesn't the answer depend on whether /project/foo contains a "tags" somewhere? How can you know if the */tags wildcard entry is relevant without knowing what paths exist inside the tree?" I would read "*/tags" as "define the access rights to any path's (*) sub-folder named 'tags' in my repo". I would not want to be denied rights to "/project/foo" in this scenario - I would want to be denied access only to the "tags" sub-folder (/project/foo/myapp/tags). Am I missing something? In what scenario would someone want to deny access to a parent folder just because it happens to contain a sub-folder with a specific name? Original comment by dabrink2
          Hide
          stsp Stefan Sperling added a comment -

          Doug, that depends on the how we interpret '*'.
          
          If we look at the fnmatch() function, there is an option flag (FNM_PATHNAME)
          that makes it behave as you suggest (a slash would not match the *, so to match
          'foo' at 3 levels deep you'd need */*/*/foo). I agree that using
          FNM_PATHNAME-like behaviour would probably simplify the implementation of this
          feature. If we interpret */foo as "anything at any level"/foo then the amount of
          directories to scan is of course much larger. However, it can still be very
          large even with FNM_PATHNAME-like behaviour because the user can specify an
          arbitrary amount of patterns in the authz file.
          
          Generally, we have to be very careful when adding features that might cause the
          server to perform a lot of work to satisfy a single request.
          

          Show
          stsp Stefan Sperling added a comment - Doug, that depends on the how we interpret '*'. If we look at the fnmatch() function, there is an option flag (FNM_PATHNAME) that makes it behave as you suggest (a slash would not match the *, so to match 'foo' at 3 levels deep you'd need */*/*/foo). I agree that using FNM_PATHNAME-like behaviour would probably simplify the implementation of this feature. If we interpret */foo as "anything at any level"/foo then the amount of directories to scan is of course much larger. However, it can still be very large even with FNM_PATHNAME-like behaviour because the user can specify an arbitrary amount of patterns in the authz file. Generally, we have to be very careful when adding features that might cause the server to perform a lot of work to satisfy a single request.
          Hide
          subversion-importer Subversion Importer added a comment -

          I could really use this feature.  Basically at the particular directory level
          and above, only management/architects will "configure" the tree. Anything below
          that directory would be writable by developers.  Please reconsider the feature
          

          Original comment by bedub58

          Show
          subversion-importer Subversion Importer added a comment - I could really use this feature. Basically at the particular directory level and above, only management/architects will "configure" the tree. Anything below that directory would be writable by developers. Please reconsider the feature Original comment by bedub58
          Hide
          kfogel Karl Fogel added a comment -

          Just to point out some related behaviors that make the lack of this feature
          particularly surprising (even forehead-slapworthy) in some use cases:
          
          Imagine a user U who can ls, checkout, etc a directory (say, /trunk).  That
          directory has a subdirectory that is invisible to U (say, /trunk/private).  U
          will discover to her surprise that she cannot branch /trunk. So U can check out
          trunk, but then U can't make a branch, even though U supposedly has write access
          to /branches and, as far as U knows, read access to /trunk.  Unexpected, eh? :-)
          
          Here's an IRC conversation (massively edited for clarity/flow) about it.  Thanks
          to danielsh and philipm for thinking through the implementation oddity that
          would be involved in fixing this -- clearly the "needsdesign" tag on this bug
          report remains accurate.
          
           <kfogel>   suppose I have "* = rw" for / of repos R, and a sub-authz
                      rule for "R:/trunk/private" that says "* = " with the
                      next line saying "@privategroup = rw".
           
           <kfogel>   Now, suppose I have a user U, who is *not* a member
                      of @privategroup.  U tries to make a branch of trunk (and
                      trunk has a 'private' subdirectory at the time U tries to
                      make the branch):
           
           <kfogel>     svn --username U cp -m "Seeing what happens."       \
                        http://svn.red-bean.com/repos/R/trunk               \
                        http://svn.red-bean.com/repos/R/branches/newbranch
           
           <kfogel>   Now U will get this error:
           
           <kfogel>     svn: access to '/repos/R/!svn/bc/2494/trunk' forbidden
           
           <kfogel>   Should U be able to make the branch, and just not have a
                      private/ subdir in the branch?  Or is an error the right
                      reaction?  (In which case, the error is quite confusing,
                      but then again, it kind of has to be, because it can't
                      reveal the existence of the private subdirectory.)
           
           <danielsh> kfogel: if you allow the branch to be created, what would
                      'svn log -qvl1' on the resulting revision print for that
                      user? 
           
           <danielsh> kfogel: point is that branches/newbranch/private is NOT
                      covered by authz rules
           
           <kfogel>   The resulting revision (the one that created the branch)
                      would not *contain* the private/ directory, so there's no
                      log adjustment to be made anyway.  It should just print
                      normal results.
           
           <kfogel>   (We already have an accepted 'svn log' behavior for
                      revisions that contain changes to paths one isn't allowed
                      to see.  When I have U do 'svn log' on a revision that
                      modified something in /trunk/private/, svn has a reasonable
                      behavior right now -- namely, to show me nothing).
           
           <kfogel>   danielsh: if some other user (one who was a member of
                      @privategroup) had done the cp, things might be different
           
           <kfogel>   danielsh: in that case, when that privileged user does "svn
                      log", they'll see everything, and the private/ dir will be
                      in the branch; and when user U (unprivileged) does 'svn
                      log' on that revision, they will see whatever they would
                      see on any other revision that involves a mixture of
                      visible and invisible things.
           
           <danielsh> kfogel: but if the revision doesn't contain the private/
                      dir, you have to do a "delete private/" in the FS, and then
                      you have in the revision a readable path with unreadable
                      copyfrom... and I'm just not sure how we handle that.
           
           <philipm>  How could the user make a copy that doesn't include the
                      authz prohibited child?
           
           <danielsh> kfogel: I'm tryig to see if allowing the copy to go through
                      (sans private/) would make sense 
           
           <kfogel>   danielsh: asking more about desired behavior than implementation
           
           <kfogel>   philipm: right now, they can do a checkout of trunk/.
           
           <kfogel>   philipm: so, my point is, they reasonably expect that they
                      can do a cp of something they can already do a checkout or
                      an ls of.
           
           <kfogel>   philipm: then they try it, and get a weird error.
           
           <philipm>  Can they copy the working copy?
           
           <kfogel>   philipm: sure... heh, ummm... I wonder if that works right.
                      Testing now!
           
           <philipm>  If you try to commit the copy and delete the excluded child
                      the FS will reject the commit.
           
           <philipm>  It rejects it at the copy stage.
           
           <philipm>  It wouldn't be a cheap copy. It would be a cheap copy with
                      modifications.
           
           <danielsh> it should let the copy go through, but delete the private/
                      child, and hide the existence of the deletion from
                      unauthn'd users.
           
           <kfogel>     $ svn cp --username U -m                         \
                          "Creating a branch from wc this time."         \
                          . http://svn.red-bean.com/repos/R/branches/ug
           
                        svn: Directory 'private' is missing
                        svn: Directory 'private' is missing
           
           <kfogel>   philipm, danielsh: (yes, it printed that twice)
           
           <kfogel>   philipm, danielsh: so, in 1.6.7, the server reveals to the
                      client the name of the forbidden directory.  If that's
                      still true in 1.7.x, we have a problem.
           
           <philipm>  They have always been revealed
           
           <kfogel>   philipm: I didn't realize that.
           
           <danielsh> 19:11:42 @danielsh | it should let the copy go
                      through, but delete the private/ child, and hide
                      the existence of the deletion from unauthn'd users.
           
           <danielsh> kfogel, * : ^^^ is that implementable?  Would it work? 
           
           <kfogel>   danielsh: in my opinion, that's the right user-visible
                      behavior.  How hard it is to implement, I have no idea.
           
           <kfogel>   danielsh: (I am going to note all this in #2662, though, so
                      at least we don't have to think it all through again)
           
           <danielsh> kfogel: you would create the revision as a copy + delete of
                      a child, and then hide paths that have unreadable
                      copyfrom... ? 
           
           <danielsh> not sure if that works, easy to implement, makes sense, etc
           
           <danielsh> keywords.add("needsdesign") # :-) 
           
           <kfogel>   danielsh: Mmmmm.  I think that's right.  In other words, if
                      user U does 'svn log' on the new revision, U doesn't see
                      the deletion of the privileged path.  However, if a
                      privileged user does 'svn log', they can see that deletion
                      too.  Agree about the "needsdesign" -- #2662 is already
                      marked with that :-).
           
           <danielsh> kfogel: I'm sure there is a boatload of corner cases in
                      this back-of-the-envelope design ... 
           
           <kfogel>   danielsh: yup
          
          

          Show
          kfogel Karl Fogel added a comment - Just to point out some related behaviors that make the lack of this feature particularly surprising (even forehead-slapworthy) in some use cases: Imagine a user U who can ls, checkout, etc a directory (say, /trunk). That directory has a subdirectory that is invisible to U (say, /trunk/private). U will discover to her surprise that she cannot branch /trunk. So U can check out trunk, but then U can't make a branch, even though U supposedly has write access to /branches and, as far as U knows, read access to /trunk. Unexpected, eh? :-) Here's an IRC conversation (massively edited for clarity/flow) about it. Thanks to danielsh and philipm for thinking through the implementation oddity that would be involved in fixing this -- clearly the "needsdesign" tag on this bug report remains accurate. <kfogel> suppose I have "* = rw" for / of repos R, and a sub-authz rule for "R:/trunk/private" that says "* = " with the next line saying "@privategroup = rw". <kfogel> Now, suppose I have a user U, who is *not* a member of @privategroup. U tries to make a branch of trunk (and trunk has a 'private' subdirectory at the time U tries to make the branch): <kfogel> svn --username U cp -m "Seeing what happens." \ http://svn.red-bean.com/repos/R/trunk \ http://svn.red-bean.com/repos/R/branches/newbranch <kfogel> Now U will get this error: <kfogel> svn: access to '/repos/R/!svn/bc/2494/trunk' forbidden <kfogel> Should U be able to make the branch, and just not have a private/ subdir in the branch? Or is an error the right reaction? (In which case, the error is quite confusing, but then again, it kind of has to be, because it can't reveal the existence of the private subdirectory.) <danielsh> kfogel: if you allow the branch to be created, what would 'svn log -qvl1' on the resulting revision print for that user? <danielsh> kfogel: point is that branches/newbranch/private is NOT covered by authz rules <kfogel> The resulting revision (the one that created the branch) would not *contain* the private/ directory, so there's no log adjustment to be made anyway. It should just print normal results. <kfogel> (We already have an accepted 'svn log' behavior for revisions that contain changes to paths one isn't allowed to see. When I have U do 'svn log' on a revision that modified something in /trunk/private/, svn has a reasonable behavior right now -- namely, to show me nothing). <kfogel> danielsh: if some other user (one who was a member of @privategroup) had done the cp, things might be different <kfogel> danielsh: in that case, when that privileged user does "svn log", they'll see everything, and the private/ dir will be in the branch; and when user U (unprivileged) does 'svn log' on that revision, they will see whatever they would see on any other revision that involves a mixture of visible and invisible things. <danielsh> kfogel: but if the revision doesn't contain the private/ dir, you have to do a "delete private/" in the FS, and then you have in the revision a readable path with unreadable copyfrom... and I'm just not sure how we handle that. <philipm> How could the user make a copy that doesn't include the authz prohibited child? <danielsh> kfogel: I'm tryig to see if allowing the copy to go through (sans private/) would make sense <kfogel> danielsh: asking more about desired behavior than implementation <kfogel> philipm: right now, they can do a checkout of trunk/. <kfogel> philipm: so, my point is, they reasonably expect that they can do a cp of something they can already do a checkout or an ls of. <kfogel> philipm: then they try it, and get a weird error. <philipm> Can they copy the working copy? <kfogel> philipm: sure... heh, ummm... I wonder if that works right. Testing now! <philipm> If you try to commit the copy and delete the excluded child the FS will reject the commit. <philipm> It rejects it at the copy stage. <philipm> It wouldn't be a cheap copy. It would be a cheap copy with modifications. <danielsh> it should let the copy go through, but delete the private/ child, and hide the existence of the deletion from unauthn'd users. <kfogel> $ svn cp --username U -m \ "Creating a branch from wc this time." \ . http://svn.red-bean.com/repos/R/branches/ug svn: Directory 'private' is missing svn: Directory 'private' is missing <kfogel> philipm, danielsh: (yes, it printed that twice) <kfogel> philipm, danielsh: so, in 1.6.7, the server reveals to the client the name of the forbidden directory. If that's still true in 1.7.x, we have a problem. <philipm> They have always been revealed <kfogel> philipm: I didn't realize that. <danielsh> 19:11:42 @danielsh | it should let the copy go through, but delete the private/ child, and hide the existence of the deletion from unauthn'd users. <danielsh> kfogel, * : ^^^ is that implementable? Would it work? <kfogel> danielsh: in my opinion, that's the right user-visible behavior. How hard it is to implement, I have no idea. <kfogel> danielsh: (I am going to note all this in #2662, though, so at least we don't have to think it all through again) <danielsh> kfogel: you would create the revision as a copy + delete of a child, and then hide paths that have unreadable copyfrom... ? <danielsh> not sure if that works, easy to implement, makes sense, etc <danielsh> keywords.add("needsdesign") # :-) <kfogel> danielsh: Mmmmm. I think that's right. In other words, if user U does 'svn log' on the new revision, U doesn't see the deletion of the privileged path. However, if a privileged user does 'svn log', they can see that deletion too. Agree about the "needsdesign" -- #2662 is already marked with that :-). <danielsh> kfogel: I'm sure there is a boatload of corner cases in this back-of-the-envelope design ... <kfogel> danielsh: yup
          Hide
          kfogel Karl Fogel added a comment -

          The reason the above phenomenon (in #desc29) is related, by the way, is that
          once wildcards or regexps _are_ working, people will start doing things like
          
            [repos:/branches/*/private]
            * = 
            @privilegedgroup = rw
          
            [repos:/tags/*/private]
            * = 
            @privilegedgroup = rw
          
          etc, and expect that now branching or tagging trunk/ will Just Work.  What
          #desc29 is saying is, it won't work, at least not for unprivileged users.  Not
          merely that it won't work consistently with how it works for privileged users,
          but that it won't work at all.
          

          Show
          kfogel Karl Fogel added a comment - The reason the above phenomenon (in #desc29) is related, by the way, is that once wildcards or regexps _are_ working, people will start doing things like [repos:/branches/*/private] * = @privilegedgroup = rw [repos:/tags/*/private] * = @privilegedgroup = rw etc, and expect that now branching or tagging trunk/ will Just Work. What #desc29 is saying is, it won't work, at least not for unprivileged users. Not merely that it won't work consistently with how it works for privileged users, but that it won't work at all.
          Hide
          kfogel Karl Fogel added a comment -

          Okay, the comments from #desc29 and #desc30 are now filed as separate issue
          #4322.  Thanks to Doug Brinkmeier for pointing out that a new issue would really
          be appropriate.
          

          Show
          kfogel Karl Fogel added a comment - Okay, the comments from #desc29 and #desc30 are now filed as separate issue #4322. Thanks to Doug Brinkmeier for pointing out that a new issue would really be appropriate.
          Hide
          cmpilato C. Michael Pilato added a comment -

          1.8.0 is stabilizing for release right now.  As such, the branch is no longer
          open to non-defect-related issue work.  Bumping FEATURE and ENHANCEMENT issues
          scheduled for 1.8-consider to 1.9-consider instead.
          

          Show
          cmpilato C. Michael Pilato added a comment - 1.8.0 is stabilizing for release right now. As such, the branch is no longer open to non-defect-related issue work. Bumping FEATURE and ENHANCEMENT issues scheduled for 1.8-consider to 1.9-consider instead.
          Hide
          stsp Stefan Sperling added a comment -

          This issue was briefly discussed at the 2013 Subversion Hackathon between Ben
          Reser, Prabhu Gnana Sundar, Johan Corveleyn, and myself.
          
          One of the questions considered was whether a '*' should match one or more
          directory levels. We concluded that having a '*' match just one directory level
          won't cut it. Users would still need to list a lot of patterns in some cases,
          and the idea of wildcards is to reduce the number of path listed in authz files.
          
          There is an attack that works around some patterns and exposes secret paths.
          Consider an authz rule such as:
          [/A]
          attacker = r
          [/A/*/secret]
          trusteduser = r
          * =
          [/B]
          attacker = rw
          
          Assuming a naive implementation of a wildecard authz feature, the attacker is
          able to obtain access to the secret path by running:
            svn copy ^/A ^/B/A-copy
          The secret is copied into /B/A-copy/ and will no longer be protected.
          
          To prevent this attack, Subversion would need to walk the repository tree and
          match every path inside of it against wildcard patterns. This could be very
          slow. We concluded that this feature is worth implementing anyway. We can alert
          users to the fact that the feature can reduce server performance in some cases,
          and could even be used for denial of service attacks (as already mentioned in
          above comments to this issue). It's reasonable to assume that users who want an
          authz wildcard feature are willing to pay the price, especially in controlled
          environments where servers are not on the public internet and all users are
          known and trusted.
          

          Show
          stsp Stefan Sperling added a comment - This issue was briefly discussed at the 2013 Subversion Hackathon between Ben Reser, Prabhu Gnana Sundar, Johan Corveleyn, and myself. One of the questions considered was whether a '*' should match one or more directory levels. We concluded that having a '*' match just one directory level won't cut it. Users would still need to list a lot of patterns in some cases, and the idea of wildcards is to reduce the number of path listed in authz files. There is an attack that works around some patterns and exposes secret paths. Consider an authz rule such as: [/A] attacker = r [/A/*/secret] trusteduser = r * = [/B] attacker = rw Assuming a naive implementation of a wildecard authz feature, the attacker is able to obtain access to the secret path by running: svn copy ^/A ^/B/A-copy The secret is copied into /B/A-copy/ and will no longer be protected. To prevent this attack, Subversion would need to walk the repository tree and match every path inside of it against wildcard patterns. This could be very slow. We concluded that this feature is worth implementing anyway. We can alert users to the fact that the feature can reduce server performance in some cases, and could even be used for denial of service attacks (as already mentioned in above comments to this issue). It's reasonable to assume that users who want an authz wildcard feature are willing to pay the price, especially in controlled environments where servers are not on the public internet and all users are known and trusted.
          Hide
          stefan2 Stefan Fuhrmann added a comment -

          For some time now, there is branch for faster authz while supporting wildcards
          at the same time: ^/subversion/authzperf .
          

          Show
          stefan2 Stefan Fuhrmann added a comment - For some time now, there is branch for faster authz while supporting wildcards at the same time: ^/subversion/authzperf .
          Hide
          stefan2 Stefan Fuhrmann added a comment -

          Mark as started. We expect it to become part of 1.10.
          

          Show
          stefan2 Stefan Fuhrmann added a comment - Mark as started. We expect it to become part of 1.10.
          Hide
          stsp Stefan Sperling added a comment -

          The authzperf branch was merged to trunk in https://svn.apache.org/r1776832 and the result of this work will be released in 1.10.

          Closing this issue.

          Show
          stsp Stefan Sperling added a comment - The authzperf branch was merged to trunk in https://svn.apache.org/r1776832 and the result of this work will be released in 1.10. Closing this issue.

            People

            • Assignee:
              Unassigned
              Reporter:
              subversion-importer Subversion Importer
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development