A Subversion user (Klaus Welch <klaus.welch@advantest.com>) reported on the users@subversion.apache.org list that Subversion 1.8.0-rc2 was returning a Serf context error while trying to do a simple 'svn info' against a server configured for digest authentication. (See http://svn.haxx.se/users/archive-2013-05/0323.shtml for the discussion.)
I was able to reproduce this problem, too.
Upon debugging, I find that Serf is raising an error in the following condition (in auth_digest.c:serf__validate_response_digest_auth()):
/* Incorrect response-digest in Authentication-Info header. */
if (strcmp(rspauth, resp_hdr_hex) != 0) {
return SERF_ERROR_AUTHN_FAILED;
}
Given the mismatched MD5's per the condition, I examined a wiretrace to see if any information there was out of sorts. All of the visible components (cnonce, rspauth, nc, etc.) checkout out fine. That left the calculated ones: ha1 and ha2. So I added some printf's and re-ran the operation:
build_digest_ha1: username(cmpilato), realm_name(Subversion Repository (Digest)), password(cmpilato) build_digest_ha2: method(OPTIONS), uri(/tests-digest/repos) build_digest_ha2: method(), uri(/tests-digest/repos) build_digest_ha2: method(OPTIONS), uri(/tests-digest/repos) build_digest_ha2: method(), uri(/tests-digest/repos) build_digest_ha2: method(PROPFIND), uri(/tests-digest/repos/!svn/rvr/27) build_digest_ha2: method(), uri(/tests-digest/repos)
That final build_digest_ha2() call is the one that comes shortly before the error condition is raise. I was able to determine that, had build_digest_ha2() been called with the URI "/tests-digest/repos/!svn/rvr/27" (instead of "/tests-digest/repos"), the MD5 calculations which make use of the ha2 would have yielded the correct value.
I don't know Serf particularly well, and I know Digest auth even less, but it seems odd to me that the one HA2 value would be generated using the actual URI of the request, but in the validation step, the URI that's used is pulled from conn->host_info (which I must assume is setup per-connection, not per-request).
In my opinion, this is a showstopper for the Subversion 1.8.0 release, which depends on Serf for HTTP communication.
Comment #1
Posted on May 31, 2013 by Swift LionI'll have a look.
Comment #2
Posted on May 31, 2013 by Grumpy HippoSIDEBAR: It appears that this bit of code is the only code that tries to use conn->host_info.path. But when conn->host_url -- which presumably is supposed to represent the same information as conn->host_info, just in another format -- is built, the path information is stripped (per the APR_URI_UNP_OMITPATHINFO flag to apr_uri_unparse()).
I might suggest that conn->host_info should be re-parsed from the host_url to ensure that the two stay in sync?
{{{
Index: outgoing.c
--- outgoing.c (revision 1877) +++ outgoing.c (working copy) @@ -1227,7 +1227,7 @@ c->host_url = apr_uri_unparse(c->pool, &host_info, APR_URI_UNP_OMITPATHINFO); - c->host_info = host_info; + (void)apr_uri_parse(c->pool, c->host_url, &(c->host_info));
*conn = c;
}}}
Comment #3
Posted on May 31, 2013 by Swift LionShould be fixed in r1885. The response digest in the Authentication-Info header is indeed calculated with the uri of the original request. (RFC 2617, 3.2.3).
When you say: " I was able to determine that, had build_digest_ha2() been called with the URI "/tests-digest/repos/!svn/rvr/27" (instead of "/tests-digest/repos"), the MD5 calculations which make use of the ha2 would have yielded the correct value.", I suppose you tested this by changing this uri in gdb? Because, in the response handler the uri of the request is not directly available, so I had to work around this a bit.
FYI: when I originally implemented digest this worked ok with subversion, but serf didn't validate the successful response headers yet. I've fixed this in r1669 for auth_kerb, but then broke auth_digest as that code path had never been tested before.
L.
Comment #4
Posted on Jun 1, 2013 by Grumpy HippoI didn't change the URI in GDB, no. Rather, I manually calculate the checksums.
"What would happen if HA2 was md5(method + ':' + '/correct/uri') instead? Aha! The result is what's expected!"
Either way, glad you found a solution. It's the same solution I was thinking to implement, but I second-guessed myself. "Surely they'd have stored the URI associated with the request_t if it wasn't just outright wrong to do so..." Guess not. :-)
Status: Fixed
Labels:
Type-Defect
Priority-High