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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to