You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2013/06/03 23:14:47 UTC
serf error handling for locks without authn
http://subversion.tigris.org/issues/show_bug.cgi?id=4368
Locking with anonymous http fails because there is no username. At
present mod_dav_svn returns "401 Unathorized" however
http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.2
says that a 401 response MUST include a WWW-Authenticate header and
apache's response doesn't include such as header as authn is not
configured. That looks as if mod_dav_svn is returning invalid
HTTP. Should we return something else? Perhaps "501 Not Implemented"?
The WebDAV RFC 2518 gives "412 Precondition Failed" as a possible error,
although I'm not clear whether that would be suitable for this request.
If I change mod_dav_svn to return a 412 instead of 401 I see what looks
like a ra_serf bug:
$ svn lock http://localhost:8888/obj/repo/A/f
'f' locked by user '(null)'.
The 412 is treated as success! The code in
libsvn_ra_serf/locks.c:determine_error looks dodgy:
213 static svn_error_t *
214 determine_error(svn_ra_serf__handler_t *handler,
215 svn_error_t *err)
216 {
217 {
218 apr_status_t errcode;
219
220 if (handler->sline.code == 423)
221 errcode = SVN_ERR_FS_PATH_ALREADY_LOCKED;
222 else if (handler->sline.code == 403)
223 errcode = SVN_ERR_RA_DAV_FORBIDDEN;
224 else
225 return err;
226
227 /* Client-side or server-side error already. Return it. */
228 if (err != NULL)
229 return err;
230
231 /* The server did not send us a detailed human-readable error.
232 Provide a generic error. */
233 err = svn_error_createf(errcode, NULL,
234 _("Lock request failed: %d %s"),
235 handler->sline.code,
236 handler->sline.reason);
237 }
238
239 return err;
240 }
Lines 224-225 mean that all the following code is never executed and
anything other than 423 or 403 is going to get lost. I can't simply
remove those two lines because errcode needs a value for
svn_error_createf.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
AW: serf error handling for locks without authn
Posted by Markus Schaber <m....@codesys.com>.
Hi,
Von: Philip Martin [mailto:philip.martin@wandisco.com]
> Greg Stein <gs...@gmail.com> writes:
> > On Mon, Jun 3, 2013 at 5:14 PM, Philip Martin
> > <ph...@wandisco.com> wrote:
> >> http://subversion.tigris.org/issues/show_bug.cgi?id=4368
> >> Locking with anonymous http fails because there is no username. At
> > That should be allowed. A WebDAV lock does not require an owner.
> svn_fs_lock requires a username.
>
> * @a fs must have a username associated with it (see
> * #svn_fs_access_t), else return #SVN_ERR_FS_NO_USER. Set the
> * 'owner' field in the new lock to the fs username.
>
> What is mod_dav_svn going to do, invent a username?
I'd say: yes.
Probably something based on the ip or hostname requesting the lock, or "anonymous_webdav_user".
Best regards
Markus Schaber
CODESYS(r) a trademark of 3S-Smart Software Solutions GmbH
Inspiring Automation Solutions
3S-Smart Software Solutions GmbH
Dipl.-Inf. Markus Schaber | Product Development Core Technology
Memminger Str. 151 | 87439 Kempten | Germany
Tel. +49-831-54031-979 | Fax +49-831-54031-50
E-Mail: m.schaber@codesys.com | Web: http://www.codesys.com | CODESYS store: http://store.codesys.com
CODESYS forum: http://forum.codesys.com
Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner | Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
Re: serf error handling for locks without authn
Posted by Ben Reser <be...@reser.org>.
On Mon, Jun 3, 2013 at 5:24 PM, Konstantin Kolinko
<kn...@gmail.com> wrote:
> If you are considering 501 then there also exists 405 Method Not Allowed
> An implementation difference though is that 405 must be accompanied by
> an Allow header.
>
> Difference between 405 and 501 is also specified in ch.5.1.1 of RFC
> 2616 (HTTP/1.1),
> Quoting from there:
> [[[
> The list of methods allowed by a resource can be specified in an
> Allow header field (section 14.7). The return code of the response
> always notifies the client whether a method is currently allowed on a
> resource, since the set of allowed methods can change dynamically. An
> origin server SHOULD return the status code 405 (Method Not Allowed)
> if the method is known by the origin server but not allowed for the
> requested resource, and 501 (Not Implemented) if the method is
> unrecognized or not implemented by the origin server.
> ]]]
>
> On a generic HTTP server if authentication is required but is not configured
> I would consider returning 403.
> I do not know whether that would work for Subversion in this case though.
Yeah 405 is just as annoying for us to return because it's hard for us
to provide that list of allowed methods, since what really is allowed
can be configured by the user.
403 would be alright, but I'm inclined to think 501 is better in this
case since in practically all cases the server is misconfigured.
Outside of us developers nobody in practice runs a Subversion server
and allows anonymous commits. We do it from time to time because
setting up authn/authz is annoying and we just want to test something
unrelated to that.
Re: serf error handling for locks without authn
Posted by Konstantin Kolinko <kn...@gmail.com>.
2013/6/4 Greg Stein <gs...@gmail.com>:
> On Mon, Jun 3, 2013 at 7:10 PM, Greg Stein <gs...@gmail.com> wrote:
>> On Mon, Jun 3, 2013 at 6:48 PM, Ben Reser <be...@reser.org> wrote:
>>...
>>> I'd argue that we should return a 500 range error since the problem
>>> here is that the server is not properly configured. There is really
>>
>> Nah. 500 means there is nothing the client can do, which isn't quite
>> accurate. A client *could* go ahead and fill in an Authorization:
>> header. (tho I don't know if Apache will parse it, without a config
>> there)
>>
>>> nothing a client can do to resolve the issue other than to
>>> authenticate, which our client is only going to do if the server is
>>> setup properly. So I'd vote for returning HTTP_INTERNAL_SERVER_ERROR.
>
> On IRC, Ben and I tossed this around. The short answer is "the server
> is not configured to allow a LOCK operation." 501 (Not Implemented)
> states it is an appropriate status when the server is unable to
> support the request method.
>
> We can also adjust the error string in append_locks() to something like:
> "Anonymous lock creation is not allowed. The server configuration
> will not allow a LOCK."
>
> That points to the configuration problem. And the 501 is pretty darned
> close to what we want.
>
> As Ben noted on IRC, a 4xx response implies the client got something
> wrong. The root problem is on the server, and that implies a 5xx
> response.
>
If you are considering 501 then there also exists 405 Method Not Allowed
An implementation difference though is that 405 must be accompanied by
an Allow header.
Difference between 405 and 501 is also specified in ch.5.1.1 of RFC
2616 (HTTP/1.1),
Quoting from there:
[[[
The list of methods allowed by a resource can be specified in an
Allow header field (section 14.7). The return code of the response
always notifies the client whether a method is currently allowed on a
resource, since the set of allowed methods can change dynamically. An
origin server SHOULD return the status code 405 (Method Not Allowed)
if the method is known by the origin server but not allowed for the
requested resource, and 501 (Not Implemented) if the method is
unrecognized or not implemented by the origin server.
]]]
On a generic HTTP server if authentication is required but is not configured
I would consider returning 403.
I do not know whether that would work for Subversion in this case though.
Best regards,
Konstantin Kolinko
Re: serf error handling for locks without authn
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Jun 3, 2013 at 7:47 PM, Philip Martin
<ph...@wandisco.com> wrote:
>...
> However there is another HTTP_UNAUTHORIZED: mod_dav_svn returns it from
> lock.c:refresh_locks:
>
> /* Sanity check: does the incoming token actually represent the
> current lock on the incoming resource? */
> if ((! slock)
> || (strcmp(token->uuid_str, slock->token) != 0))
> return dav_svn__new_error(resource->pool, HTTP_UNAUTHORIZED,
> DAV_ERR_LOCK_SAVE_LOCK,
> "Lock refresh request doesn't match existing "
> "lock.");
>
> I think this is a client sending the wrong lock token in an If: header,
> so perhaps "412 Precondition Failed" is correct here?
Yeah. That would work great.
Cheers,
-g
Re: serf error handling for locks without authn
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> On IRC, Ben and I tossed this around. The short answer is "the server
> is not configured to allow a LOCK operation." 501 (Not Implemented)
> states it is an appropriate status when the server is unable to
> support the request method.
>
> We can also adjust the error string in append_locks() to something like:
> "Anonymous lock creation is not allowed. The server configuration
> will not allow a LOCK."
>
> That points to the configuration problem. And the 501 is pretty darned
> close to what we want.
>
> As Ben noted on IRC, a 4xx response implies the client got something
> wrong. The root problem is on the server, and that implies a 5xx
> response.
501 was my first suggestion, it works quite well for the anonymous lock
attempt and the other cases where SVN_ERR_FS_NO_USER is currently
converted into 401. For a 501 the client gives
svn: warning: apr_err=SVN_ERR_RA_DAV_REQUEST_FAILED
svn: warning: W175002: LOCK request on '/obj/repo/A/f' failed: 501 Method Not Implemented
and the server logs "Anonymous lock creation is not allowed".
However there is another HTTP_UNAUTHORIZED: mod_dav_svn returns it from
lock.c:refresh_locks:
/* Sanity check: does the incoming token actually represent the
current lock on the incoming resource? */
if ((! slock)
|| (strcmp(token->uuid_str, slock->token) != 0))
return dav_svn__new_error(resource->pool, HTTP_UNAUTHORIZED,
DAV_ERR_LOCK_SAVE_LOCK,
"Lock refresh request doesn't match existing "
"lock.");
I think this is a client sending the wrong lock token in an If: header,
so perhaps "412 Precondition Failed" is correct here?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: serf error handling for locks without authn
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Jun 3, 2013 at 7:10 PM, Greg Stein <gs...@gmail.com> wrote:
> On Mon, Jun 3, 2013 at 6:48 PM, Ben Reser <be...@reser.org> wrote:
>...
>> I'd argue that we should return a 500 range error since the problem
>> here is that the server is not properly configured. There is really
>
> Nah. 500 means there is nothing the client can do, which isn't quite
> accurate. A client *could* go ahead and fill in an Authorization:
> header. (tho I don't know if Apache will parse it, without a config
> there)
>
>> nothing a client can do to resolve the issue other than to
>> authenticate, which our client is only going to do if the server is
>> setup properly. So I'd vote for returning HTTP_INTERNAL_SERVER_ERROR.
On IRC, Ben and I tossed this around. The short answer is "the server
is not configured to allow a LOCK operation." 501 (Not Implemented)
states it is an appropriate status when the server is unable to
support the request method.
We can also adjust the error string in append_locks() to something like:
"Anonymous lock creation is not allowed. The server configuration
will not allow a LOCK."
That points to the configuration problem. And the 501 is pretty darned
close to what we want.
As Ben noted on IRC, a 4xx response implies the client got something
wrong. The root problem is on the server, and that implies a 5xx
response.
Cheers,
-g
Re: serf error handling for locks without authn
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Jun 3, 2013 at 6:48 PM, Ben Reser <be...@reser.org> wrote:
> On Mon, Jun 3, 2013 at 3:31 PM, Greg Stein <gs...@gmail.com> wrote:
>...
>> Yeah. HTTP_CONFLICT should be correct, there.
>
> Well technically 401 is right but we have now way of filling in the
> proper WWW-Authenticate header. HTTP_CONFLICT doesn't sound
> particularly great either because that's supposed to be describing an
> issue with the state of the resource.
"This resource needs a username" or something like that :-)
> I'd argue that we should return a 500 range error since the problem
> here is that the server is not properly configured. There is really
Nah. 500 means there is nothing the client can do, which isn't quite
accurate. A client *could* go ahead and fill in an Authorization:
header. (tho I don't know if Apache will parse it, without a config
there)
> nothing a client can do to resolve the issue other than to
> authenticate, which our client is only going to do if the server is
> setup properly. So I'd vote for returning HTTP_INTERNAL_SERVER_ERROR.
"our client" won't do anything. Doesn't mean another one would.
500 is really for "holy crap. something failed, and I have NO IDEA
what is going on. so just go away" That isn't really true in this
case.
Cheers,
-g
Re: serf error handling for locks without authn
Posted by Philip Martin <ph...@wandisco.com>.
Ben Reser <be...@reser.org> writes:
> That's my fault, I fixed the anonymous locking segfault and chose a
> 401 to return but I forgot about the WWW-Authenticate header
> requirement.
Not really your fault, even before your r1455352 mod_dav_svn returned
401. Without r1455352 append_locks calls svn_repos_fs_lock and gets
SVN_ERR_FS_NO_USER which it converts into HTTP_UNAUTHORIZED.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: serf error handling for locks without authn
Posted by Ben Reser <be...@reser.org>.
On Mon, Jun 3, 2013 at 3:31 PM, Greg Stein <gs...@gmail.com> wrote:
> On Mon, Jun 3, 2013 at 6:09 PM, Philip Martin
> <ph...@wandisco.com> wrote:
>> Greg Stein <gs...@gmail.com> writes:
>>
>>> On Mon, Jun 3, 2013 at 5:14 PM, Philip Martin
>>> <ph...@wandisco.com> wrote:
>>>> http://subversion.tigris.org/issues/show_bug.cgi?id=4368
>>>>
>>>> Locking with anonymous http fails because there is no username. At
That's my fault, I fixed the anonymous locking segfault and chose a
401 to return but I forgot about the WWW-Authenticate header
requirement.
>> What is mod_dav_svn going to do, invent a username?
>
> Ah. Well... I would say "yes, invent a username". "_unknown_" or somesuch.
>
> But that's just me :-)
That's problematic, some people want to enforce that only the lock
owner can remove the locks and so on. I also think supporting
entirely anonymous locks is an almost entirely pointless exercise
given our current limitations.
> I'm not sure if we can determine whether authn/authz is configured for
> the particular path.
>
> Oh. Wait. The config should prevent the LOCK even *before* it gets to
> our code. IOW, if our code is reached *without* a username, then
> authn/authz is not configured.
This is indeed correct. When authentication is configured correctly
(even on anonymous read allowed setups) a LOCK will be rejected before
it even gets to us. The anonymous lock issue only happens when you
have a repo setup to allow entirely anonymous commits, which is
effectively nobody. At least until we have something like obliterate
where cleaning up the crap people would add to your repo is possible.
> 412 (Precondition Failed) is not appropriate, as there are no
> preconditions defined in the headers (eg. an "If" header). The proper
> response is 409 (Conflict). The client *may* be able to proactively
> provide authentication information, but we can't use 401 to automate
> the authn provision (for the lack of WWW-Authenticate, as you noted).
Agreed.
>>>> present mod_dav_svn returns "401 Unathorized" however
>>>
>>> Do you know where/why this is happening?
>>
>> mod_dav_svn/locks.c:append_locks.
>
> Yeah. HTTP_CONFLICT should be correct, there.
Well technically 401 is right but we have now way of filling in the
proper WWW-Authenticate header. HTTP_CONFLICT doesn't sound
particularly great either because that's supposed to be describing an
issue with the state of the resource.
I'd argue that we should return a 500 range error since the problem
here is that the server is not properly configured. There is really
nothing a client can do to resolve the issue other than to
authenticate, which our client is only going to do if the server is
setup properly. So I'd vote for returning HTTP_INTERNAL_SERVER_ERROR.
Re: serf error handling for locks without authn
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Jun 3, 2013 at 6:09 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>
>> On Mon, Jun 3, 2013 at 5:14 PM, Philip Martin
>> <ph...@wandisco.com> wrote:
>>> http://subversion.tigris.org/issues/show_bug.cgi?id=4368
>>>
>>> Locking with anonymous http fails because there is no username. At
>>
>> That should be allowed. A WebDAV lock does not require an owner.
>
> svn_fs_lock requires a username.
>
> * @a fs must have a username associated with it (see
> * #svn_fs_access_t), else return #SVN_ERR_FS_NO_USER. Set the
> * 'owner' field in the new lock to the fs username.
>
> What is mod_dav_svn going to do, invent a username?
Ah. Well... I would say "yes, invent a username". "_unknown_" or somesuch.
But that's just me :-)
I'm not sure if we can determine whether authn/authz is configured for
the particular path.
Oh. Wait. The config should prevent the LOCK even *before* it gets to
our code. IOW, if our code is reached *without* a username, then
authn/authz is not configured.
412 (Precondition Failed) is not appropriate, as there are no
preconditions defined in the headers (eg. an "If" header). The proper
response is 409 (Conflict). The client *may* be able to proactively
provide authentication information, but we can't use 401 to automate
the authn provision (for the lack of WWW-Authenticate, as you noted).
>>> present mod_dav_svn returns "401 Unathorized" however
>>
>> Do you know where/why this is happening?
>
> mod_dav_svn/locks.c:append_locks.
Yeah. HTTP_CONFLICT should be correct, there.
>...
And back to the client: yeah, determine_error() looks very wrong.
There should be some kind of fallback value for errcode.
Cheers,
-g
Re: serf error handling for locks without authn
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> On Mon, Jun 3, 2013 at 5:14 PM, Philip Martin
> <ph...@wandisco.com> wrote:
>> http://subversion.tigris.org/issues/show_bug.cgi?id=4368
>>
>> Locking with anonymous http fails because there is no username. At
>
> That should be allowed. A WebDAV lock does not require an owner.
svn_fs_lock requires a username.
* @a fs must have a username associated with it (see
* #svn_fs_access_t), else return #SVN_ERR_FS_NO_USER. Set the
* 'owner' field in the new lock to the fs username.
What is mod_dav_svn going to do, invent a username?
>> present mod_dav_svn returns "401 Unathorized" however
>
> Do you know where/why this is happening?
mod_dav_svn/locks.c:append_locks.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: serf error handling for locks without authn
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Jun 3, 2013 at 5:14 PM, Philip Martin
<ph...@wandisco.com> wrote:
> http://subversion.tigris.org/issues/show_bug.cgi?id=4368
>
> Locking with anonymous http fails because there is no username. At
That should be allowed. A WebDAV lock does not require an owner.
> present mod_dav_svn returns "401 Unathorized" however
Do you know where/why this is happening?
> http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.2
>
> says that a 401 response MUST include a WWW-Authenticate header and
While true, we need to avoid the 401 in the first place.
>...
Cheers,
-g
Re: serf error handling for locks without authn
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> I'm trying to work out what this code should look like. This function
> is called after LOCK and PROPFIND, but not UNLOCK which has its own
> code. I think the function should be something like:
>
> apr_status_t errcode;
>
> if (err)
> return err;
>
> if (handler->sline.code == 200 || handler->sline.code == 207)
> return SVN_NO_ERROR;
> else if (handler->sline.code == 423)
> errcode = SVN_ERR_FS_PATH_ALREADY_LOCKED;
> else if (handler->sline.code == 403)
> errcode = SVN_ERR_RA_DAV_FORBIDDEN;
> else
> errcode = SVN_ERR_RA_DAV_REQUEST_FAILED;
>
> return svn_error_createf(errcode, NULL,
> _("Lock request failed: %d %s"),
> handler->sline.code,
> handler->sline.reason ? handler->sline.reason : "");
>
>
> where a non-NULL err is given precedence otherwise the status line is
> examined. 200 is not an error and the regression tests require 207 to
> be treated as success as well.
>
> I'm not confident about changing this code but I think we have to do
> something for 1.8 as the current code simply drops status line errors if
> err is NULL.
I committed this to trunk. I've triggered the bug with a modified
server but I can't see how to get an unmodified server to do it. So I
see no immediate need to backport the change to 1.8.x.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: serf error handling for locks without authn
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> The code in libsvn_ra_serf/locks.c:determine_error looks dodgy:
>
> 213 static svn_error_t *
> 214 determine_error(svn_ra_serf__handler_t *handler,
> 215 svn_error_t *err)
> 216 {
> 217 {
> 218 apr_status_t errcode;
> 219
> 220 if (handler->sline.code == 423)
> 221 errcode = SVN_ERR_FS_PATH_ALREADY_LOCKED;
> 222 else if (handler->sline.code == 403)
> 223 errcode = SVN_ERR_RA_DAV_FORBIDDEN;
> 224 else
> 225 return err;
> 226
> 227 /* Client-side or server-side error already. Return it. */
> 228 if (err != NULL)
> 229 return err;
> 230
> 231 /* The server did not send us a detailed human-readable error.
> 232 Provide a generic error. */
> 233 err = svn_error_createf(errcode, NULL,
> 234 _("Lock request failed: %d %s"),
> 235 handler->sline.code,
> 236 handler->sline.reason);
> 237 }
> 238
> 239 return err;
> 240 }
>
> Lines 224-225 mean that all the following code is never executed and
> anything other than 423 or 403 is going to get lost. I can't simply
> remove those two lines because errcode needs a value for
> svn_error_createf.
I'm trying to work out what this code should look like. This function
is called after LOCK and PROPFIND, but not UNLOCK which has its own
code. I think the function should be something like:
apr_status_t errcode;
if (err)
return err;
if (handler->sline.code == 200 || handler->sline.code == 207)
return SVN_NO_ERROR;
else if (handler->sline.code == 423)
errcode = SVN_ERR_FS_PATH_ALREADY_LOCKED;
else if (handler->sline.code == 403)
errcode = SVN_ERR_RA_DAV_FORBIDDEN;
else
errcode = SVN_ERR_RA_DAV_REQUEST_FAILED;
return svn_error_createf(errcode, NULL,
_("Lock request failed: %d %s"),
handler->sline.code,
handler->sline.reason ? handler->sline.reason : "");
where a non-NULL err is given precedence otherwise the status line is
examined. 200 is not an error and the regression tests require 207 to
be treated as success as well.
I'm not confident about changing this code but I think we have to do
something for 1.8 as the current code simply drops status line errors if
err is NULL.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download