On 25/04/12 12:32, Mark Thomas wrote:
On 25/04/2012 12:03, Brian Burch wrote:
On 24/04/12 21:34, Mark Thomas wrote:
On 24/04/2012 21:11, Mark Thomas wrote:
On 24/04/2012 20:51, Brian Burch wrote:
Sorry I haven't been able to quote the details of this commit made by
markt a month ago, but I didn't keep a copy in my inbox.
I previously submitted an enhancement to the corresponding test suite
https://issues.apache.org/bugzilla/show_bug.cgi?id=53096
I fully expected all my test cases would succeed against mark's trivial
bugfix.
I recently brought my trunk sandbox up to svn: r1329909 and was puzzled
to discover the relevant test case still FAILED!
I've done a lot of digging and debugging, and come to the conclusion
this problem is more subtle than originally thought. Does anyone know
whether the current fix has been validated against a real android
device? I suspect it doesn't work.
I certainly didn't at the time, but I can quite easily.
The issues are:
1. the one line change:
- String md5a1 = getDigest(username, realm);
+ // In digest auth, digests are always lower case
+ String md5a1 = getDigest(username,
realm).toLowerCase(Locale.ENGLISH);
If I remember correctly, the intention was to be make tomcat more
tolerant of clients that presented digest strings with upper case
hexadecimal digits provided they were otherwise correct. The change in
r1329909 seems to me as if it does nothing of relevance to that
objective.
Agreed.
(if that was the objective).
However, that was not the objective. The objective was to handle
programs that created hashes for the user database that used capitals.
The Android DIGEST auth issue was related to URIs. See
https://issues.apache.org/bugzilla/show_bug.cgi?id=52954
Let me see what happens with 2.3.5 and 4.0.3 and decide then.
Watch this space...
BZ 52954 is not an issue in Android 4.0.3 but it is in 2.3.5. Given that
2.3.x is by far the most prevalent Android version at the moment we
should certainly take a look at fixing BZ 52954.
Upper/lower case digests from android clients is a red herring.
I don't understand exactly what you mean by that. Didn't the original
report say android http clients were sending HTTP Digest hex strings
with upper case A-F, or was I just dreaming?
You were dreaming. Go read BZ 52964.
Not quite dreaming... it seems I created an imaginary cocktail of two bugs:-
BZ 52953 Unlike BASIC Authentication, DIGEST mode does not work if the
hash is stored in uppercase
and
BZ 52954 Allowing for broken android HTTP DIGEST support
I thought (wrongly) they were both referring to android clients.
r1329909 implied it was trying to resolve that issue, even if it was
also resolving others. Unfortunately, r1329909 pointlessly manipulates a
hex digest string that is already certain to contain lower case hex digits.
No it didn't. And that isn't the right revision number either. We are
discussing this:
http://svn.apache.org/viewvc?view=revision&revision=r1303339
Careless of me to imply the change in question was the latest at the
time I did my update. I'm glad you were paying attention and I hope I
didn't waste any of your time.
It now makes sense to me why your r1303339 hit the server-side
recreation of the expected client digest. You were not addressing the
case where an upper-case hex digit had arrived from an http client. I
apologise for muddying the water.
For what it is worth, here is my patch to achieve the "apparently
desired" objective (my "situation 1"). You'll see that it also cleans up
a debug parameter list by a) removing duplication of clientDigest and b)
fixing a typo in a field label.
That isn't the problem. Android has no problems sending digests in lower
case s required by the spec.
My suggested patch had three objectives (I assumed you would break it
into smaller commits):
1. Would you please clean up the debug message parameters (as proposed
in my diff)? It seems overkill to open a new bug for this trivial change.
2. In the case where an http client has sent an invalid digest string
(anything that is not a lower case hex digit) rfc2617 seems to me to
mandate http status 400 (syntax error, do not retry), but my tests show
tomcat is sending 401 (unauthorized) when I inject "A" (or even "G")
into the digest string. That looks like a new bug to me - what do you think?
3. My revised opinion is that r1303339 is effective, but tackles the
user database problem too far down the track. RealmBase.getDigest(String
username, String realmName) has two cases. When a cleartext password is
available it can be trusted to return an rfc-compliant digest from
org.apache.catalina.util.MD5Encoder.encode(). However, if the concrete
implementation of getPassword returns a pre-digested password, getDigest
should not simply trust the returned String to be rfc-compliant. I think
it should a) validate the string and throw some sort of exception if it
is not a meaningful md5 digest, and perhaps b) force it to lower case if
appropriate. Maybe this could be accomplished neatly in a new
MD5Encoder.validate(String) method?
Regards,
Brian
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
