Re: svn commit: r1064093 -/subversion/trunk/subversion/libsvn_repos/authz.c
On Thu, Jan 27, 2011 at 11:14 AM, C. Michael Pilato wrote: > If we have have the option of moving towards case-sensitivity -- that is, a > *more*-precise authz policy -- that seems like a good thing. I'd even be in > favor of making this behavior optional (like the force_username_case option > we already have). Given that we do wonky things on the client side when cases are mixed in the same path, I think it'd be very weird to make this behavior optional. It's so highly unlikely that anyone is going to want apply rules to "AuthZ" but not to "authz" - especially given that it wouldn't have worked *at all* before anyway. My $.02. -- justin
Re: svn commit: r1064093 -/subversion/trunk/subversion/libsvn_repos/authz.c
C. Michael Pilato wrote on Thu, Jan 27, 2011 at 15:33:17 -0500: > On 01/27/2011 02:18 PM, Daniel Shahaf wrote: > > C. Michael Pilato wrote on Thu, Jan 27, 2011 at 14:14:11 -0500: > >> If we have have the option of moving towards case-sensitivity -- that is, a > >> *more*-precise authz policy -- that seems like a good thing. I'd even be > >> in > >> favor of making this behavior optional (like the force_username_case option > >> we already have). > >> > > > > Could you clarify? What modes are you suggesting to support, and which > > would be the default one? > > Well, I was thinking about "authz-section-case-sensitive = true|false" in > svnserve.conf, and "SVNAuthzSectionCaseSensitive on|off" for the DAV > modules. Enabling this gives us case sensitivity in our treatment of those > authz section names (which include repository names and paths); disabling > makes it case-insensitive. > > But I confess that I'm only considering this as a way to avoid breaking > existing authz files that are exploiting the case-insensitivity we have > today. If we all agree that case-*sensitivity* (or case-insensitivity) is > The Way To Go, and that the realistic user effect of consistification here > is negligible, then toss the "make it optional" suggestion altogether. > I've already expressed my opinion (+1 to casefulness), so I'll let others chime in now. In any case, is anyone interested in writing a patch in this direction (once we agree what the direction is)? /me looking at Arwin > > Also: where is force_username_case documented? The default > > svnserve.conf doesn't mention it. > > Really? No, sorry. (I used my new-test-repository script instead of plain 'svnadmin create'.)
Re: svn commit: r1064093 -/subversion/trunk/subversion/libsvn_repos/authz.c
On 01/27/2011 02:18 PM, Daniel Shahaf wrote: > C. Michael Pilato wrote on Thu, Jan 27, 2011 at 14:14:11 -0500: >> On 01/27/2011 01:46 PM, Daniel Shahaf wrote: >>> Personally I'm +0.5, since IMO we should be treating pathnames >>> case-insensitively. >> >> (Did you mean "case-sensitively"?) >> > > Yes. > >> If we have have the option of moving towards case-sensitivity -- that is, a >> *more*-precise authz policy -- that seems like a good thing. I'd even be in >> favor of making this behavior optional (like the force_username_case option >> we already have). >> > > Could you clarify? What modes are you suggesting to support, and which > would be the default one? Well, I was thinking about "authz-section-case-sensitive = true|false" in svnserve.conf, and "SVNAuthzSectionCaseSensitive on|off" for the DAV modules. Enabling this gives us case sensitivity in our treatment of those authz section names (which include repository names and paths); disabling makes it case-insensitive. But I confess that I'm only considering this as a way to avoid breaking existing authz files that are exploiting the case-insensitivity we have today. If we all agree that case-*sensitivity* (or case-insensitivity) is The Way To Go, and that the realistic user effect of consistification here is negligible, then toss the "make it optional" suggestion altogether. > Also: where is force_username_case documented? The default > svnserve.conf doesn't mention it. Really? I see it in my default svnserve.conf under the [general] section. Though, it's "force-username-case" (hyphens, not underscores; sorry). -- C. Michael Pilato CollabNet <> www.collab.net <> Distributed Development On Demand signature.asc Description: OpenPGP digital signature
Re: svn commit: r1064093 -/subversion/trunk/subversion/libsvn_repos/authz.c
C. Michael Pilato wrote on Thu, Jan 27, 2011 at 14:14:11 -0500: > On 01/27/2011 01:46 PM, Daniel Shahaf wrote: > > Personally I'm +0.5, since IMO we should be treating pathnames > > case-insensitively. > > (Did you mean "case-sensitively"?) > Yes. > If we have have the option of moving towards case-sensitivity -- that is, a > *more*-precise authz policy -- that seems like a good thing. I'd even be in > favor of making this behavior optional (like the force_username_case option > we already have). > Could you clarify? What modes are you suggesting to support, and which would be the default one? Also: where is force_username_case documented? The default svnserve.conf doesn't mention it. > -- > C. Michael Pilato > CollabNet <> www.collab.net <> Distributed Development On Demand >
Re: svn commit: r1064093 -/subversion/trunk/subversion/libsvn_repos/authz.c
On 01/27/2011 01:46 PM, Daniel Shahaf wrote: > Personally I'm +0.5, since IMO we should be treating pathnames > case-insensitively. (Did you mean "case-sensitively"?) If we have have the option of moving towards case-sensitivity -- that is, a *more*-precise authz policy -- that seems like a good thing. I'd even be in favor of making this behavior optional (like the force_username_case option we already have). -- C. Michael Pilato CollabNet <> www.collab.net <> Distributed Development On Demand signature.asc Description: OpenPGP digital signature
RE: svn commit: r1064093-/subversion/trunk/subversion/libsvn_repos/authz.c
>-Original Message- >From: Daniel Shahaf [mailto:d...@daniel.shahaf.name] >Sent: Fri 1/28/2011 12:16 AM >To: Kamesh Jayachandran; Arwin Arni Nandagopal >Cc: dev@subversion.apache.org >Subject: Re: svn commit: >r1064093-/subversion/trunk/subversion/libsvn_repos/authz.c >Kamesh Jayachandran wrote on Fri, Jan 28, 2011 at 00:14:35 +0530: >> >> >> It might be a rare situation, >> >> but *if* folks have come to depend on the case-sensitivity of these >> >> checks, >> >> they need to prepare for the fallout of this loosening of the policy. >> >> >> >> >Sorry for not thinking about this earlier, but: >> >Why was the fix to make all checks case-INsensitive, as opposed to >> >making all checks case-sensitive? >> >> We use svn_config_read to parse the authz which always stores the section >> names in *lower* case in its internal hash table. >> >> See subversion/libsvn_subr/config.c:make_hash_key(). >> >> We need to rev svn_config_read to accept bool case_sensitive_section_parser. >> Then we can change the behaviour to always case-sensitive approach as >> against today's case-insensitive approach. >> >Thanks for the reminder. >Do you think it's a good idea? >Personally I'm +0.5, since IMO we should be treating pathnames >case-insensitively. Same case with me. With regards Kamesh Jayachandran
Re: svn commit: r1064093 -/subversion/trunk/subversion/libsvn_repos/authz.c
Kamesh Jayachandran wrote on Fri, Jan 28, 2011 at 00:14:35 +0530: > > >> It might be a rare situation, > >> but *if* folks have come to depend on the case-sensitivity of these checks, > >> they need to prepare for the fallout of this loosening of the policy. > >> > > >Sorry for not thinking about this earlier, but: > >Why was the fix to make all checks case-INsensitive, as opposed to > >making all checks case-sensitive? > > We use svn_config_read to parse the authz which always stores the section > names in *lower* case in its internal hash table. > > See subversion/libsvn_subr/config.c:make_hash_key(). > > We need to rev svn_config_read to accept bool case_sensitive_section_parser. > Then we can change the behaviour to always case-sensitive approach as against > today's case-insensitive approach. > Thanks for the reminder. Do you think it's a good idea? Personally I'm +0.5, since IMO we should be treating pathnames case-insensitively. Daniel > With regards > Kamesh Jayachandran
RE: svn commit: r1064093 -/subversion/trunk/subversion/libsvn_repos/authz.c
>> It might be a rare situation, >> but *if* folks have come to depend on the case-sensitivity of these checks, >> they need to prepare for the fallout of this loosening of the policy. >> >Sorry for not thinking about this earlier, but: >Why was the fix to make all checks case-INsensitive, as opposed to >making all checks case-sensitive? We use svn_config_read to parse the authz which always stores the section names in *lower* case in its internal hash table. See subversion/libsvn_subr/config.c:make_hash_key(). We need to rev svn_config_read to accept bool case_sensitive_section_parser. Then we can change the behaviour to always case-sensitive approach as against today's case-insensitive approach. With regards Kamesh Jayachandran
Re: svn commit: r1064093 - /subversion/trunk/subversion/libsvn_repos/authz.c
C. Michael Pilato wrote on Thu, Jan 27, 2011 at 10:23:02 -0500:
> We should probably release-note this change.
+1
> It might be a rare situation,
> but *if* folks have come to depend on the case-sensitivity of these checks,
> they need to prepare for the fallout of this loosening of the policy.
>
Sorry for not thinking about this earlier, but:
Why was the fix to make all checks case-INsensitive, as opposed to
making all checks case-sensitive?
(If this was discussed before, I'd appreciate a pointer to the
discussion.)
>
> On 01/27/2011 06:41 AM, kame...@apache.org wrote:
> > Author: kameshj
> > Date: Thu Jan 27 11:41:43 2011
> > New Revision: 1064093
> >
> > URL: http://svn.apache.org/viewvc?rev=1064093&view=rev
> > Log:
> > Fix for Issue #3781 repo prefix rules in authz section is checked case
> > sensitively for write operations
> >
> > * subversion/libsvn_repos/authz.c
> > (authz_get_any_access_parser_cb): Use strncasecmp() instead of strncmp()
> >
> > Patch by: me
> > Arwin Arni
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_repos/authz.c
> >
> > Modified: subversion/trunk/subversion/libsvn_repos/authz.c
> > URL:
> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/authz.c?rev=1064093&r1=1064092&r2=1064093&view=diff
> > ==
> > --- subversion/trunk/subversion/libsvn_repos/authz.c (original)
> > +++ subversion/trunk/subversion/libsvn_repos/authz.c Thu Jan 27 11:41:43
> > 2011
> > @@ -398,8 +398,8 @@ authz_get_any_access_parser_cb(const cha
> >
> >/* Does the section apply to the query? */
> >if (section_name[0] == '/'
> > - || strncmp(section_name, b->repos_path,
> > - strlen(b->repos_path)) == 0)
> > + || strncasecmp(section_name, b->repos_path,
> > + strlen(b->repos_path)) == 0)
> > {
> >b->allow = b->deny = svn_authz_none;
> >
> >
> >
>
>
> --
> C. Michael Pilato
> CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r1064093 - /subversion/trunk/subversion/libsvn_repos/authz.c
On 01/27/2011 10:26 AM, Mark Phippard wrote: > On Thu, Jan 27, 2011 at 10:23 AM, C. Michael Pilato > wrote: >> We should probably release-note this change. It might be a rare situation, >> but *if* folks have come to depend on the case-sensitivity of these checks, >> they need to prepare for the fallout of this loosening of the policy. > > Can you envision a scenario where the current behavior works? I guess > the only thing I can think of is if someone wanted a repository to > just be read-only but incorrectly had it set to read-write and it was > only not allowing the write because of case-sensitivity. > > This just seems like an obvious bug to me that anyone would have run > into and changed their rules (to get the case right) until it worked > properly. I'm not saying it's a common situation. I'm simply saying that it's better to advise about this kind of thing up front rather than looking -- if only to the 1 person in the world for whom it matters -- as if we didn't think about the possible ramifications of the change. -- C. Michael Pilato CollabNet <> www.collab.net <> Distributed Development On Demand signature.asc Description: OpenPGP digital signature
Re: svn commit: r1064093 - /subversion/trunk/subversion/libsvn_repos/authz.c
On Thu, Jan 27, 2011 at 10:23 AM, C. Michael Pilato wrote: > We should probably release-note this change. It might be a rare situation, > but *if* folks have come to depend on the case-sensitivity of these checks, > they need to prepare for the fallout of this loosening of the policy. Can you envision a scenario where the current behavior works? I guess the only thing I can think of is if someone wanted a repository to just be read-only but incorrectly had it set to read-write and it was only not allowing the write because of case-sensitivity. This just seems like an obvious bug to me that anyone would have run into and changed their rules (to get the case right) until it worked properly. -- Thanks Mark Phippard http://markphip.blogspot.com/
Re: svn commit: r1064093 - /subversion/trunk/subversion/libsvn_repos/authz.c
We should probably release-note this change. It might be a rare situation,
but *if* folks have come to depend on the case-sensitivity of these checks,
they need to prepare for the fallout of this loosening of the policy.
On 01/27/2011 06:41 AM, kame...@apache.org wrote:
> Author: kameshj
> Date: Thu Jan 27 11:41:43 2011
> New Revision: 1064093
>
> URL: http://svn.apache.org/viewvc?rev=1064093&view=rev
> Log:
> Fix for Issue #3781 repo prefix rules in authz section is checked case
> sensitively for write operations
>
> * subversion/libsvn_repos/authz.c
> (authz_get_any_access_parser_cb): Use strncasecmp() instead of strncmp()
>
> Patch by: me
> Arwin Arni
>
> Modified:
> subversion/trunk/subversion/libsvn_repos/authz.c
>
> Modified: subversion/trunk/subversion/libsvn_repos/authz.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/authz.c?rev=1064093&r1=1064092&r2=1064093&view=diff
> ==
> --- subversion/trunk/subversion/libsvn_repos/authz.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/authz.c Thu Jan 27 11:41:43 2011
> @@ -398,8 +398,8 @@ authz_get_any_access_parser_cb(const cha
>
>/* Does the section apply to the query? */
>if (section_name[0] == '/'
> - || strncmp(section_name, b->repos_path,
> - strlen(b->repos_path)) == 0)
> + || strncasecmp(section_name, b->repos_path,
> + strlen(b->repos_path)) == 0)
> {
>b->allow = b->deny = svn_authz_none;
>
>
>
--
C. Michael Pilato
CollabNet <> www.collab.net <> Distributed Development On Demand
