You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Brian Burch <br...@pingtoo.com> on 2012/04/24 21:51:15 UTC
Re: svn commit: r1303339 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/realm/RealmBase.java
webapps/docs/changelog.xml
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.
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.
2. Client digest responses require an outer hash which will wrap one or
two inner hashes... /if/ the inner hashes are (incorrectly) represented
with upper case hex digits, then the outer hash will inevitably be
different to the (correct situation) where they are represented with
lower case hex digits. This difference is independent of whether the
HTTP Digest header delivers its hashes using lower or upper case hex digits.
3. I have reworked the TestDigestAuthenticator class to explore these
alternatives. I have also drafted a change to RealmBase that "fixes"
situation 1 above, but it still fails with both cases of situation 2.
4. My suspicion is that /if/ the android digest algorithm (incorrectly)
uses upper case hex digits with its outer hashes, it will probably also
do the same with its inner hashes. If I am right, the two failing tests
in situation 3 will not work until RealmBase has even more compensatory
logic.
I can explain and explore this in more detail, but I need a strategic
decision first: do we back out the change and go to wontfix (i.e. wont
be tolerant), or do we proceed with trying to be flexible? There must be
a lot of androids out there carrying the "non standard" DIGEST logic
(whatever that might really be).
Regards,
Brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1303339 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/realm/RealmBase.java
webapps/docs/changelog.xml
Posted by Brian Burch <br...@pingtoo.com>.
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-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1303339 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/realm/RealmBase.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
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.
> 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
> 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.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1303339 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/realm/RealmBase.java
webapps/docs/changelog.xml
Posted by Brian Burch <br...@pingtoo.com>.
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?
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.
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.
Index: java/org/apache/catalina/realm/RealmBase.java
===================================================================
--- java/org/apache/catalina/realm/RealmBase.java (revision 1330187)
+++ java/org/apache/catalina/realm/RealmBase.java (working copy)
@@ -383,7 +383,7 @@
String md5a2) {
// In digest auth, digests are always lower case
- String md5a1 = getDigest(username,
realm).toLowerCase(Locale.ENGLISH);
+ String md5a1 = getDigest(username, realm);
if (md5a1 == null)
return null;
String serverDigestValue;
@@ -409,14 +409,14 @@
}
if (log.isDebugEnabled()) {
- log.debug("Digest : " + clientDigest + " Username:" + username
- + " ClientSigest:" + clientDigest + " nonce:" + nonce
+ log.debug("ClientDigest: " + clientDigest +
+ " Username:" + username + " nonce:" + nonce
+ " nc:" + nc + " cnonce:" + cnonce + " qop:" + qop
+ " realm:" + realm + "md5a2:" + md5a2
- + " Server digest:" + serverDigest);
+ + " ServerDigest:" + serverDigest);
}
- if (serverDigest.equals(clientDigest)) {
+ if (serverDigest.equalsIgnoreCase(clientDigest)) {
return getPrincipal(username);
}
I hope I'm not treading on your toes!
BTW I have a new version of the unit test class that examines all of my
"situations", but I'll hang onto it until you have clarified the real
problem - obviously we don't need any failing test cases for situations
we don't intend to support!
Regards,
Brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1303339 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/realm/RealmBase.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
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.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1303339 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/realm/RealmBase.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
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.
> 2. Client digest responses require an outer hash which will wrap one or
> two inner hashes... /if/ the inner hashes are (incorrectly) represented
> with upper case hex digits, then the outer hash will inevitably be
> different to the (correct situation) where they are represented with
> lower case hex digits. This difference is independent of whether the
> HTTP Digest header delivers its hashes using lower or upper case hex
> digits.
>
> 3. I have reworked the TestDigestAuthenticator class to explore these
> alternatives. I have also drafted a change to RealmBase that "fixes"
> situation 1 above, but it still fails with both cases of situation 2.
>
> 4. My suspicion is that /if/ the android digest algorithm (incorrectly)
> uses upper case hex digits with its outer hashes, it will probably also
> do the same with its inner hashes. If I am right, the two failing tests
> in situation 3 will not work until RealmBase has even more compensatory
> logic.
>
> I can explain and explore this in more detail, but I need a strategic
> decision first: do we back out the change and go to wontfix (i.e. wont
> be tolerant), or do we proceed with trying to be flexible? There must be
> a lot of androids out there carrying the "non standard" DIGEST logic
> (whatever that might really be).
Let me see what happens with 2.3.5 and 4.0.3 and decide then.
Watch this space...
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org