While reviewing a Patch to include MD5-sess into the Digest Authentication Scheme I came across a few flaws in that class. I suggest the following changes (see attached patch):

- The qop Parameter must be parsed correctly and not just be ignored
- The fact that it is legal to have a missing qop must not be ignored
- The class should be prepared to handle the auth-int qop option
(even though an implementation is not possible with the current design)
- The public interface of this class is narrowed (as it is not needed by the tests any more)
- The test cases should check the actual result rather than checking for equality after another run through the same logic. Note: This is not simple for requests that require the client to generate a cnonce.


The patch is against HEAD. The 2.0 branch would be unaffected by these changes.

Odi
Index: java/org/apache/commons/httpclient/auth/DigestScheme.java
===================================================================
RCS file: 
/home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/auth/DigestScheme.java,v
retrieving revision 1.8
diff -u -r1.8 DigestScheme.java
--- java/org/apache/commons/httpclient/auth/DigestScheme.java   11 Sep 2003 09:09:42 
-0000      1.8
+++ java/org/apache/commons/httpclient/auth/DigestScheme.java   11 Sep 2003 11:48:09 
-0000
@@ -1,7 +1,7 @@
 /*
- * $Header: 
/home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/auth/DigestScheme.java,v
 1.8 2003/09/11 09:09:42 oglueck Exp $
- * $Revision: 1.8 $
- * $Date: 2003/09/11 09:09:42 $
+ * $Header: 
/home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/auth/DigestScheme.java,v
 1.7 2003/09/08 02:00:03 mbecke Exp $
+ * $Revision: 1.7 $
+ * $Date: 2003/09/08 02:00:03 $
  *
  * ====================================================================
  *
@@ -63,7 +63,7 @@
 
 package org.apache.commons.httpclient.auth;
 
-import java.util.Map;
+import java.util.StringTokenizer;
 import java.security.MessageDigest;
 
 import org.apache.commons.httpclient.HttpConstants;
@@ -102,6 +102,14 @@
         '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 
         'e', 'f'
     };
+    
+    //@TODO: supply a real nonce-count, currently a server will interprete a repeated 
request as a replay  
+    private static final String NC = "00000001"; //nonce-count is always 1
+    private static final int QOP_MISSING = 0;
+    private static final int QOP_AUTH_INT = 1;
+    private static final int QOP_AUTH = 2;
+
+    private int qopVariant = QOP_MISSING;
 
     /**
      * Gets an ID based upon the realm and the nonce value.  This ensures that 
requests
@@ -130,7 +138,21 @@
     public DigestScheme(final String challenge) 
       throws MalformedChallengeException {
         super(challenge);
-        this.getParameters().put("nc", "00000001");
+        
+        // qop parsing
+        String qop = getParameter("qop");
+        if (qop != null) {
+            StringTokenizer tok = new StringTokenizer(qop,",");
+            while (tok.hasMoreTokens()) {
+                String variant = tok.nextToken().trim();
+                if (variant.equals("auth")) {
+                    qopVariant = QOP_AUTH;
+                    break; //that's our favourite, because auth-int is unsupported
+                } else if (variant.equals("auth-int")) {
+                    qopVariant = QOP_AUTH_INT;               
+                }     
+            }
+        }        
     }
 
 
@@ -174,37 +196,13 @@
              "Credentials cannot be used for digest authentication: " 
               + credentials.getClass().getName());
         }
-        this.getParameters().put("cnonce", createCnonce());
         this.getParameters().put("methodname", method);
         this.getParameters().put("uri", uri);
-        return DigestScheme.authenticate(usernamepassword, getParameters());
-    }
-
-    /**
-     * Produces a digest authorization string for the given set of 
-     * [EMAIL PROTECTED] UsernamePasswordCredentials} and authetication parameters.
-     *
-     * @param credentials Credentials to create the digest with
-     * @param params The authetication parameters. The following
-     *  parameters are expected: <code>uri</code>, <code>realm</code>, 
-     *  <code>nonce</code>, <code>nc</code>, <code>cnonce</code>, 
-     *  <code>qop</code>, <code>methodname</code>.
-     * 
-     * @return a digest authorization string
-     * 
-     * @throws AuthenticationException if authorization string cannot 
-     *   be generated due to an authentication failure
-     */
-    public static String authenticate(UsernamePasswordCredentials credentials,
-            Map params) throws AuthenticationException {
+        String digest = createDigest(usernamepassword.getUserName(),
+                usernamepassword.getPassword());
 
-        LOG.trace("enter DigestScheme.authenticate(UsernamePasswordCredentials, 
Map)");
-
-        String digest = createDigest(credentials.getUserName(),
-                credentials.getPassword(), params);
-
-        return "Digest " + createDigestHeader(credentials.getUserName(),
-                params, digest);
+        return "Digest " + createDigestHeader(usernamepassword.getUserName(),
+                             digest);
     }
 
     /**
@@ -221,30 +219,31 @@
      *         value in the Authentication HTTP header.
      * @throws AuthenticationException when MD5 is an unsupported algorithm
      */
-    public static String createDigest(String uname, String pwd,
-            Map params) throws AuthenticationException {
+    private String createDigest(String uname, String pwd) throws 
AuthenticationException {
 
         LOG.trace("enter DigestScheme.createDigest(String, String, Map)");
 
         final String digAlg = "MD5";
 
         // Collecting required tokens
-        String uri = (String) params.get("uri");
-        String realm = (String) params.get("realm");
-        String nonce = (String) params.get("nonce");
-        String nc = (String) params.get("nc");
-        String cnonce = (String) params.get("cnonce");
-        String qop = (String) params.get("qop");
-        String method = (String) params.get("methodname");
-        String algorithm = (String) params.get("algorithm");
+        String uri = getParameter("uri");
+        String realm = getParameter("realm");
+        String nonce = getParameter("nonce");
+        String cnonce = getParameter("cnonce");
+        String qop = getParameter("qop");
+        String method = getParameter("methodname");
+        String algorithm = getParameter("algorithm");
 
         // If an algorithm is not specified, default to MD5.
         if(algorithm == null) {
             algorithm="MD5";
         }
 
-        if (qop != null) {
-            qop = "auth";
+
+        if (qopVariant == QOP_AUTH_INT) {
+            LOG.warn("qop=auth-int is not supported");
+            throw new AuthenticationException(
+                "Unsupported qop in HTTP Digest authentication");   
         }
 
         MessageDigest md5Helper;
@@ -257,8 +256,7 @@
                + digAlg);
         }
 
-        // Calculating digest according to rfc 2617
-
+        // 3.2.2.2: Calculating digest
         String a1 = null;
         if(algorithm.equals("MD5")) {
             // unq(username-value) ":" unq(realm-value) ":" passwd
@@ -279,16 +277,25 @@
         String md5a1 = encode(md5Helper.digest(HttpConstants.getBytes(a1)));
         String serverDigestValue;
 
-        String a2 = method + ":" + uri;
+        String a2 = null;
+        if (qopVariant == QOP_AUTH_INT) {
+            LOG.error("Unhandled qop auth-int");
+            //we do not have access to the entity-body or its hash
+            //@TODO: add Method ":" digest-uri-value ":" H(entity-body)      
+        } else {
+            a2 = method + ":" + uri;
+        }
         String md5a2 = encode(md5Helper.digest(HttpConstants.getBytes(a2)));
 
-        if (qop == null) {
+        // 3.2.2.1
+        if (qopVariant == QOP_MISSING) {
             LOG.debug("Using null qop method");
             serverDigestValue = md5a1 + ":" + nonce + ":" + md5a2;
         } else {
             LOG.debug("Using qop method " + qop);
-            serverDigestValue = md5a1 + ":" + nonce + ":" + nc + ":" + cnonce
-                                + ":" + qop + ":" + md5a2;
+            String qopOption = getQopVariantString();
+            serverDigestValue = md5a1 + ":" + nonce + ":" + NC + ":" + cnonce
+                                + ":" + qopOption + ":" + md5a2;
         }
 
         String serverDigest =
@@ -301,48 +308,52 @@
      * Creates digest-response header as defined in RFC2617.
      * 
      * @param uname Username
-     * @param params The parameters necessary to construct the digest header. 
-     *  The following parameters are expected: <code>uri</code>, 
-     *  <code>realm</code>, <code>nonce</code>, <code>nc</code>, 
-     *  <code>cnonce</code>, <code>qop</code>, <code>methodname</code>.
      * @param digest The response tag's value as String.
      * 
      * @return The digest-response as String.
      */
-    public static String createDigestHeader(String uname, Map params,
-            String digest) {
+    private String createDigestHeader(String uname, String digest) throws 
AuthenticationException {
 
         LOG.trace("enter DigestScheme.createDigestHeader(String, Map, "
             + "String)");
 
         StringBuffer sb = new StringBuffer();
-        String uri = (String) params.get("uri");
-        String realm = (String) params.get("realm");
-        String nonce = (String) params.get("nonce");
-        String nc = (String) params.get("nc");
-        String cnonce = (String) params.get("cnonce");
-        String opaque = (String) params.get("opaque");
+        String uri = getParameter("uri");
+        String realm = getParameter("realm");
+        String nonce = getParameter("nonce");
+        String nc = getParameter("nc");
+        String opaque = getParameter("opaque");
         String response = digest;
-        String qop = (String) params.get("qop");
-        String algorithm = (String) params.get("algorithm");
-
-        if (qop != null) {
-            qop = "auth"; //we only support auth
-        }
+        String qop = getParameter("qop");
+        String algorithm = getParameter("algorithm");
 
         sb.append("username=\"" + uname + "\"")
           .append(", realm=\"" + realm + "\"")
           .append(", nonce=\"" + nonce + "\"").append(", uri=\"" + uri + "\"")
-          .append(((qop == null) ? "" : ", qop=\"" + qop + "\""))
-          .append(", algorithm=\"" + algorithm + "\"")
-          .append(((qop == null) ? "" : ", nc=" + nc))
-          .append(((qop == null) ? "" : ", cnonce=\"" + cnonce + "\""))
-          .append(", response=\"" + response + "\"")
-          .append((opaque == null) ? "" : ", opaque=\"" + opaque + "\"");
-
+          .append(", response=\"" + response + "\"");
+        if (qopVariant != QOP_MISSING) {
+            sb.append(", qop=\"" + getQopVariantString() + "\"")
+              .append(", nc="+ NC)
+              .append(", cnonce=\"" + createCnonce() + "\"");
+        }
+        if (algorithm != null) {
+            sb.append(", algorithm=\"" + algorithm + "\"");
+        }    
+        if (opaque != null) {
+            sb.append(", opaque=\"" + opaque + "\"");
+        }
         return sb.toString();
     }
 
+    private String getQopVariantString() {
+        String qopOption;
+        if (qopVariant == QOP_AUTH_INT) {
+            qopOption = "auth-int";   
+        } else {
+            qopOption = "auth";
+        }
+        return qopOption;            
+    }
 
     /**
      * Encodes the 128 bit (16 bytes) MD5 digest into a 32 characters long 
Index: test/org/apache/commons/httpclient/TestAuthenticator.java
===================================================================
RCS file: 
/home/cvspublic/jakarta-commons/httpclient/src/test/org/apache/commons/httpclient/TestAuthenticator.java,v
retrieving revision 1.30
diff -u -r1.30 TestAuthenticator.java
--- test/org/apache/commons/httpclient/TestAuthenticator.java   11 Sep 2003 09:09:42 
-0000      1.30
+++ test/org/apache/commons/httpclient/TestAuthenticator.java   11 Sep 2003 11:48:12 
-0000
@@ -1,7 +1,7 @@
 /*
- * $Header: 
/home/cvspublic/jakarta-commons/httpclient/src/test/org/apache/commons/httpclient/TestAuthenticator.java,v
 1.30 2003/09/11 09:09:42 oglueck Exp $
- * $Revision: 1.30 $
- * $Date: 2003/09/11 09:09:42 $
+ * $Header: 
/home/cvspublic/jakarta-commons/httpclient/src/test/org/apache/commons/httpclient/TestAuthenticator.java,v
 1.29 2003/09/08 02:00:03 mbecke Exp $
+ * $Revision: 1.29 $
+ * $Date: 2003/09/08 02:00:03 $
  * ====================================================================
  *
  * The Apache Software License, Version 1.1
@@ -63,11 +63,10 @@
 package org.apache.commons.httpclient;
 
 import junit.framework.Test;
-import junit.framework.TestCase;
 import junit.framework.TestSuite;
 
-import java.util.Hashtable;
-import java.util.StringTokenizer;
+import java.util.Map;
+
 import org.apache.commons.httpclient.auth.*;
 import org.apache.commons.httpclient.util.Base64;
 
@@ -76,7 +75,7 @@
  *
  * @author Rodney Waldhoff
  * @author <a href="mailto:[EMAIL PROTECTED]">Jeff Dever</a>
- * @version $Id: TestAuthenticator.java,v 1.30 2003/09/11 09:09:42 oglueck Exp $
+ * @version $Id: TestAuthenticator.java,v 1.29 2003/09/08 02:00:03 mbecke Exp $
  */
 public class TestAuthenticator extends TestNoHostBase {
 
@@ -91,29 +90,6 @@
         junit.textui.TestRunner.main(testCaseName);
     }
 
-    // ------------------------------------------------------- Utility Methods
-
-    private void checkAuthorization(UsernamePasswordCredentials cred, String 
methodName, String auth) throws Exception {
-        Hashtable table = new Hashtable();
-        StringTokenizer tokenizer = new StringTokenizer(auth, ",=\"");
-        while(tokenizer.hasMoreTokens()){
-            String key = null;
-            String value = null;
-            if(tokenizer.hasMoreTokens())
-                key = tokenizer.nextToken();
-            if(tokenizer.hasMoreTokens())
-                value = tokenizer.nextToken();
-            if(key != null && value != null){
-                table.put(key.trim(),value.trim());
-            }
-        }
-        String response = (String) table.get("response");
-        table.put( "methodname", methodName );
-        String digest = 
DigestScheme.createDigest(cred.getUserName(),cred.getPassword(), table);
-        assertEquals(response, digest);
-    }
-
-
     // ------------------------------------------------------- TestCase Methods
 
     public static Test suite() {
@@ -332,7 +308,7 @@
 
 
     public void testDigestAuthenticationWithDefaultCreds() throws Exception {
-        String challenge = "Digest realm=\"realm1\"";
+        String challenge = "Digest realm=\"realm1\", 
nonce=\"f2a3f18799759d4f1a1c068b92b573cb\"";
         HttpState state = new HttpState();
         UsernamePasswordCredentials cred = new 
UsernamePasswordCredentials("username","password");
         state.setCredentials(null, null, cred);
@@ -340,11 +316,16 @@
         AuthScheme authscheme = new DigestScheme(challenge);
         assertTrue(HttpAuthenticator.authenticate(authscheme, method, null, state));
         assertTrue(null != method.getRequestHeader("Authorization"));
-        checkAuthorization(cred, method.getName(), 
method.getRequestHeader("Authorization").getValue());
+        Map table = 
AuthChallengeParser.extractParams(method.getRequestHeader("Authorization").getValue());
+        assertEquals("username", table.get("username"));
+        assertEquals("realm1", table.get("realm"));
+        assertEquals("/", table.get("uri"));
+        assertEquals("f2a3f18799759d4f1a1c068b92b573cb", table.get("nonce"));
+        assertEquals("e95a7ddf37c2eab009568b1ed134f89a", table.get("response"));
     }
 
     public void testDigestAuthentication() throws Exception {
-        String challenge = "Digest realm=\"realm1\"";
+        String challenge = "Digest realm=\"realm1\", 
nonce=\"f2a3f18799759d4f1a1c068b92b573cb\"";
         HttpState state = new HttpState();
         UsernamePasswordCredentials cred = new 
UsernamePasswordCredentials("username","password");
         state.setCredentials(null, null, cred);
@@ -352,7 +333,12 @@
         AuthScheme authscheme = new DigestScheme(challenge);
         assertTrue(HttpAuthenticator.authenticate(authscheme, method, null, state));
         assertTrue(null != method.getRequestHeader("Authorization"));
-        checkAuthorization(cred, method.getName(), 
method.getRequestHeader("Authorization").getValue());
+        Map table = 
AuthChallengeParser.extractParams(method.getRequestHeader("Authorization").getValue());
+        assertEquals("username", table.get("username"));
+        assertEquals("realm1", table.get("realm"));
+        assertEquals("/", table.get("uri"));
+        assertEquals("f2a3f18799759d4f1a1c068b92b573cb", table.get("nonce"));
+        assertEquals("e95a7ddf37c2eab009568b1ed134f89a", table.get("response"));
     }
 
     public void testDigestAuthenticationWithStaleNonce() throws Exception {
@@ -386,12 +372,17 @@
         SimpleHttpMethod method = new SimpleHttpMethod();
         method.setDoAuthentication(true);
         assertEquals("Authentication failed", 200, client.executeMethod(method));
-        checkAuthorization(cred, method.getName(), 
method.getRequestHeader("Authorization").getValue());
+        Map table = 
AuthChallengeParser.extractParams(method.getRequestHeader("Authorization").getValue());
+        assertEquals("username", table.get("username"));
+        assertEquals("realm1", table.get("realm"));
+        assertEquals("/", table.get("uri"));
+        assertEquals("321CBA", table.get("nonce"));
+        assertEquals("7f5948eefa115296e9279225041527b3", table.get("response"));
     }
 
     public void testDigestAuthenticationWithMultipleRealms() throws Exception {
-        String challenge1 = "Digest realm=\"realm1\"";
-        String challenge2 = "Digest realm=\"realm2\"";
+        String challenge1 = "Digest realm=\"realm1\", nonce=\"abcde\"";
+        String challenge2 = "Digest realm=\"realm2\", nonce=\"123546\"";
         HttpState state = new HttpState();
         UsernamePasswordCredentials cred = new 
UsernamePasswordCredentials("username","password");
         state.setCredentials("realm1", null, cred);
@@ -403,13 +394,23 @@
             HttpMethod method = new SimpleHttpMethod(new 
Header("WWW-Authenticate",challenge1));
             assertTrue(HttpAuthenticator.authenticate(authscheme1, method, null, 
state));
             assertTrue(null != method.getRequestHeader("Authorization"));
-            checkAuthorization(cred, method.getName(), 
method.getRequestHeader("Authorization").getValue());
+            Map table = 
AuthChallengeParser.extractParams(method.getRequestHeader("Authorization").getValue());
+            assertEquals("username", table.get("username"));
+            assertEquals("realm1", table.get("realm"));
+            assertEquals("/", table.get("uri"));
+            assertEquals("abcde", table.get("nonce"));
+            assertEquals("786f500303eac1478f3c2865e676ed68", table.get("response"));
         }
         {
             HttpMethod method = new SimpleHttpMethod(new 
Header("WWW-Authenticate",challenge2));
             assertTrue(HttpAuthenticator.authenticate(authscheme2, method, null, 
state));
             assertTrue(null != method.getRequestHeader("Authorization"));
-            checkAuthorization(cred2, method.getName(), 
method.getRequestHeader("Authorization").getValue());
+            Map table = 
AuthChallengeParser.extractParams(method.getRequestHeader("Authorization").getValue());
+            assertEquals("uname2", table.get("username"));
+            assertEquals("realm2", table.get("realm"));
+            assertEquals("/", table.get("uri"));
+            assertEquals("123546", table.get("nonce"));
+            assertEquals("0283edd9ef06a38b378b3b74661391e9", table.get("response"));
         }
     }
 
@@ -425,7 +426,7 @@
         String nonce="e273f1776275974f1a120d8b92c5b3cb";
 
         String challenge="Digest realm=\"" + realm + "\", "
-            + nonce + "\"" + nonce + "\", "
+            + "nonce=\"" + nonce + "\", "
             + "opaque=\"SomeString\", "
             + "stale=false, "
             + "algorithm=MD5-sess, "
@@ -441,9 +442,61 @@
         assertTrue(HttpAuthenticator.authenticate(
             authscheme, method, null, state));
         assertTrue(null != method.getRequestHeader("Authorization"));
-        checkAuthorization(cred, method.getName(),
-            method.getRequestHeader("Authorization").getValue());
+        Map table = 
AuthChallengeParser.extractParams(method.getRequestHeader("Authorization").getValue());
+        assertEquals(username, table.get("username"));
+        assertEquals(realm, table.get("realm"));
+        assertEquals("MD5-sess", table.get("algorithm"));
+        assertEquals("/", table.get("uri"));
+        assertEquals(nonce, table.get("nonce"));
+        assertEquals(1, Integer.parseInt((String) table.get("nc"),16));
+        assertTrue(null != table.get("cnonce"));
+        assertEquals("SomeString", table.get("opaque"));
+        assertEquals("auth", table.get("qop"));
+        //@TODO: add better check
+        assertTrue(null != table.get("response")); 
     }
+    
+    /** 
+     * Test digest authentication using the MD5-sess algorithm.
+     */
+    public void testDigestAuthenticationMD5SessNoQop() throws Exception {
+        // Example using Digest auth with MD5-sess
+
+        String realm="realm";
+        String username="username";
+        String password="password";
+        String nonce="e273f1776275974f1a120d8b92c5b3cb";
+
+        String challenge="Digest realm=\"" + realm + "\", "
+            + "nonce=\"" + nonce + "\", "
+            + "opaque=\"SomeString\", "
+            + "stale=false, "
+            + "algorithm=MD5-sess";
+
+        HttpState state = new HttpState();
+        UsernamePasswordCredentials cred =
+            new UsernamePasswordCredentials(username, password);
+        state.setCredentials(realm, null, cred);
+        AuthScheme authscheme = new DigestScheme(challenge);
+        HttpMethod method =
+            new SimpleHttpMethod(new Header("WWW-Authenticate", challenge));
+        assertTrue(HttpAuthenticator.authenticate(
+            authscheme, method, null, state));
+        assertTrue(null != method.getRequestHeader("Authorization"));
+        Map table = 
AuthChallengeParser.extractParams(method.getRequestHeader("Authorization").getValue());
+        assertEquals(username, table.get("username"));
+        assertEquals(realm, table.get("realm"));
+        assertEquals("MD5-sess", table.get("algorithm"));
+        assertEquals("/", table.get("uri"));
+        assertEquals(nonce, table.get("nonce"));
+        assertTrue(null == table.get("nc"));
+        assertEquals("SomeString", table.get("opaque"));
+        assertTrue(null == table.get("qop"));
+        //@TODO: add better check
+        assertTrue(null != table.get("response")); 
+    }
+
+    
 
     // --------------------------------- Test Methods for NTLM Authentication
 

---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to