tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject svn commit: r1405370 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/authenticator/ java/org/apache/tomcat/util/http/parser/ test/org/apache/catalina/authenticator/ test/org/apache/tomcat/util/http/parser/ webapps/docs/
Date Sat, 03 Nov 2012 15:50:41 GMT
Author: markt
Date: Sat Nov  3 15:50:41 2012
New Revision: 1405370

URL: http://svn.apache.org/viewvc?rev=1405370&view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54060
Use new HTTP header parser to address issues in current regular expression based parser.
This roughly twice as fast and generates about a third of the garbage (based on profiling
the load unit test)

Added:
    tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser2.java
      - copied, changed from r1404773, tomcat/trunk/java/org/apache/tomcat/util/http/parser/HttpParser2.java
    tomcat/tc7.0.x/trunk/test/org/apache/tomcat/util/http/parser/TestHttpParser2.java
      - copied, changed from r1404773, tomcat/trunk/test/org/apache/tomcat/util/http/parser/TestHttpParser2.java
Modified:
    tomcat/tc7.0.x/trunk/   (props changed)
    tomcat/tc7.0.x/trunk/java/org/apache/catalina/authenticator/DigestAuthenticator.java
    tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TestDigestAuthenticator.java
    tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TesterDigestAuthenticatorPerformance.java
    tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml

Propchange: tomcat/tc7.0.x/trunk/
------------------------------------------------------------------------------
  Merged /tomcat/trunk:r1404773,1404917,1405133,1405168,1405321,1405353,1405357,1405364

Modified: tomcat/tc7.0.x/trunk/java/org/apache/catalina/authenticator/DigestAuthenticator.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/authenticator/DigestAuthenticator.java?rev=1405370&r1=1405369&r2=1405370&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/catalina/authenticator/DigestAuthenticator.java (original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/catalina/authenticator/DigestAuthenticator.java Sat
Nov  3 15:50:41 2012
@@ -20,6 +20,7 @@ package org.apache.catalina.authenticato
 
 
 import java.io.IOException;
+import java.io.StringReader;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 import java.security.Principal;
@@ -39,6 +40,7 @@ import org.apache.catalina.util.MD5Encod
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
 import org.apache.tomcat.util.buf.B2CConverter;
+import org.apache.tomcat.util.http.parser.HttpParser2;
 
 
 
@@ -554,48 +556,29 @@ public class DigestAuthenticator extends
             if (authorization == null) {
                 return false;
             }
-            if (!authorization.startsWith("Digest ")) {
+
+            Map<String,String> directives;
+            try {
+                directives = HttpParser2.parseAuthorizationDigest(
+                        new StringReader(authorization));
+            } catch (IOException e) {
                 return false;
             }
-            authorization = authorization.substring(7).trim();
 
-            // Bugzilla 37132: http://issues.apache.org/bugzilla/show_bug.cgi?id=37132
-            String[] tokens = authorization.split(",(?=(?:[^\"]*\"[^\"]*\")+$)");
+            if (directives == null) {
+                return false;
+            }
 
             method = request.getMethod();
-
-            for (int i = 0; i < tokens.length; i++) {
-                String currentToken = tokens[i];
-                if (currentToken.length() == 0)
-                    continue;
-
-                int equalSign = currentToken.indexOf('=');
-                if (equalSign < 0) {
-                    return false;
-                }
-                String currentTokenName =
-                    currentToken.substring(0, equalSign).trim();
-                String currentTokenValue =
-                    currentToken.substring(equalSign + 1).trim();
-                if ("username".equals(currentTokenName))
-                    userName = removeQuotes(currentTokenValue);
-                if ("realm".equals(currentTokenName))
-                    realmName = removeQuotes(currentTokenValue, true);
-                if ("nonce".equals(currentTokenName))
-                    nonce = removeQuotes(currentTokenValue);
-                if ("nc".equals(currentTokenName))
-                    nc = removeQuotes(currentTokenValue);
-                if ("cnonce".equals(currentTokenName))
-                    cnonce = removeQuotes(currentTokenValue);
-                if ("qop".equals(currentTokenName))
-                    qop = removeQuotes(currentTokenValue);
-                if ("uri".equals(currentTokenName))
-                    uri = removeQuotes(currentTokenValue);
-                if ("response".equals(currentTokenName))
-                    response = removeQuotes(currentTokenValue);
-                if ("opaque".equals(currentTokenName))
-                    opaqueReceived = removeQuotes(currentTokenValue);
-            }
+            userName = directives.get("username");
+            realmName = directives.get("realm");
+            nonce = directives.get("nonce");
+            nc = directives.get("nc");
+            cnonce = directives.get("cnonce");
+            qop = directives.get("qop");
+            uri = directives.get("uri");
+            response = directives.get("response");
+            opaqueReceived = directives.get("opaque");
 
             return true;
         }

Copied: tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser2.java (from
r1404773, tomcat/trunk/java/org/apache/tomcat/util/http/parser/HttpParser2.java)
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser2.java?p2=tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser2.java&p1=tomcat/trunk/java/org/apache/tomcat/util/http/parser/HttpParser2.java&r1=1404773&r2=1405370&rev=1405370&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/http/parser/HttpParser2.java (original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser2.java Sat Nov
 3 15:50:41 2012
@@ -52,7 +52,8 @@ public class HttpParser2 {
     private static final Integer FIELD_TYPE_LHEX = Integer.valueOf(3);
     private static final Integer FIELD_TYPE_QUOTED_LHEX = Integer.valueOf(4);
 
-    private static final Map<String,Integer> fieldTypes = new HashMap<>();
+    private static final Map<String,Integer> fieldTypes =
+            new HashMap<String,Integer>();
 
     private static final boolean isToken[] = new boolean[128];
     private static final boolean isHex[] = new boolean[128];
@@ -98,7 +99,8 @@ public class HttpParser2 {
      *
      * @param input The header value to parse
      *
-     * @return  A map of directives and values as {@link String}s. Although the
+     * @return  A map of directives and values as {@link String}s or
+     *          <code>null</code> if a parsing error occurs. Although the
      *          values returned are {@link String}s they will have been
      *          validated to ensure that they conform to RFC 2617.
      *
@@ -109,15 +111,22 @@ public class HttpParser2 {
     public static Map<String,String> parseAuthorizationDigest (
             StringReader input) throws IllegalArgumentException, IOException {
 
-        Map<String,String> result = new HashMap<>();
+        Map<String,String> result = new HashMap<String,String>();
 
-        swallowConstant(input, "Digest", false);
+        if (!skipConstant(input, "Digest", false)) {
+            return null;
+        }
         skipLws(input);
         // All field names are valid tokens
         String field = readToken(input);
-        while (field != null) {
+        if (field == null) {
+            return null;
+        }
+        while (!field.equals("")) {
             skipLws(input);
-            swallowConstant(input, "=", false);
+            if (!skipConstant(input, "=", false)) {
+                return null;
+            }
             skipLws(input);
             String value = null;
             Integer type = fieldTypes.get(field.toLowerCase(Locale.US));
@@ -152,83 +161,104 @@ public class HttpParser2 {
                             "TODO i18n: Unsupported type");
             }
 
+            if (value == null) {
+                return null;
+            }
             result.put(field, value);
 
             skipLws(input);
-            if (!swallowConstant(input, ",", true)) {
-                break;
+            if (!skipConstant(input, ",", true)) {
+                return null;
             }
             skipLws(input);
             field = readToken(input);
+            if (field == null) {
+                return null;
+            }
         }
 
         return result;
     }
 
-    private static boolean swallowConstant(StringReader input, String constant,
-            boolean optional) throws IOException {
+    /**
+     * @return  <code>true</code> if the constant is found or if no data is
+     *          present and EOF is allowed otherwise returns <code>false</code>
+     */
+    private static boolean skipConstant(StringReader input, String constant,
+            boolean eofOk) throws IOException {
         int len = constant.length();
 
         for (int i = 0; i < len; i++) {
             int c = input.read();
+            if (i == 0 && c == -1 && eofOk) {
+                return true;
+            }
             if (c != constant.charAt(i)) {
-                if (optional) {
-                    input.skip(i);
-                    return false;
-                } else {
-                    throw new IllegalArgumentException(
-                            "TODO I18N: Failed to parse input for [" + constant +
-                            "]");
-                }
+                return false;
             }
         }
         return true;
     }
 
     private static void skipLws(StringReader input) throws IOException {
-        char c = (char) input.read();
+        int c = input.read();
         while (c == 32 || c == 9) {
-            c = (char) input.read();
+            c = input.read();
         }
 
         // Skip back so non-LWS character is available for next read
         input.skip(-1);
     }
 
+    /**
+     * @return  the token if one was found, the empty string if no data was
+     *          available to read or <code>null</code> if data other than a
+     *          token was found
+     */
     private static String readToken(StringReader input) throws IOException {
         StringBuilder result = new StringBuilder();
 
-        char c = (char) input.read();
-        while (c != 65535 && isToken[c]) {
-            result.append(c);
-            c = (char) input.read();
+        int c = input.read();
+        while (c != -1 && isToken[c]) {
+            result.append((char) c);
+            c = input.read();
         }
         // Skip back so non-token character is available for next read
         input.skip(-1);
 
-        return result.toString();
+        if (c != -1 && result.length() == 0) {
+            return null;
+        } else {
+            return result.toString();
+        }
     }
 
+    /**
+     * @return the quoted string if one was found, null if data other than a
+     *         quoted string was found or null if the end of data was reached
+     *         before the quoted string was terminated
+     */
     private static String readQuotedString(StringReader input)
             throws IOException {
 
-        char c = (char) input.read();
+        int c = input.read();
         if (c != '"') {
-            throw new IllegalArgumentException(
-                    "TODO i18n: Quoted string must start with a quote");
+            return null;
         }
 
         StringBuilder result = new StringBuilder();
 
-        c = (char) input.read();
+        c = input.read();
         while (c != '"') {
-            if (c == '\\') {
-                c = (char) input.read();
+            if (c == -1) {
+                return null;
+            } else if (c == '\\') {
+                c = input.read();
                 result.append(c);
             } else {
-                result.append(c);
+                result.append((char) c);
             }
-            c = (char) input.read();
+            c = input.read();
         }
 
         return result.toString();
@@ -236,7 +266,7 @@ public class HttpParser2 {
 
     private static String readTokenOrQuotedString(StringReader input)
             throws IOException {
-        char c = (char) input.read();
+        int c = input.read();
         input.skip(-1);
 
         if (c == '"') {
@@ -246,30 +276,37 @@ public class HttpParser2 {
         }
     }
 
-    /*
+    /**
      * Parses lower case hex but permits upper case hex to be used (converting
      * it to lower case before returning).
+     *
+     * @return the lower case hex if present or <code>null</code> if data other
+     *         than lower case hex was found
      */
     private static String readLhex(StringReader input) throws IOException {
         StringBuilder result = new StringBuilder();
 
-        char c = (char) input.read();
-        while (isHex[c]) {
-            result.append(c);
-            c = (char) input.read();
+        int c = input.read();
+        while (c != -1 && isHex[c]) {
+            result.append((char) c);
+            c = input.read();
         }
         // Skip back so non-hex character is available for next read
         input.skip(-1);
 
-        return result.toString().toLowerCase();
+        if (result.length() == 0) {
+            return null;
+        } else {
+            return result.toString().toLowerCase();
+        }
     }
 
     private static String readQuotedLhex(StringReader input)
             throws IOException {
 
-        swallowConstant(input, "\"", false);
+        skipConstant(input, "\"", false);
         String result = readLhex(input);
-        swallowConstant(input, "\"", false);
+        skipConstant(input, "\"", false);
 
         return result;
     }

Modified: tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TestDigestAuthenticator.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TestDigestAuthenticator.java?rev=1405370&r1=1405369&r2=1405370&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TestDigestAuthenticator.java
(original)
+++ tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TestDigestAuthenticator.java
Sat Nov  3 15:50:41 2012
@@ -342,14 +342,13 @@ public class TestDigestAuthenticator ext
         auth.append(md5response);
         auth.append("\"");
         if (qop != null) {
-            auth.append(", qop=\"");
+            auth.append(", qop=");
             auth.append(qop);
-            auth.append("\"");
+            auth.append("");
         }
         if (nc != null) {
-            auth.append(", nc=\"");
+            auth.append(", nc=");
             auth.append(nc);
-            auth.append("\"");
         }
         if (cnonce != null) {
             auth.append(", cnonce=\"");

Modified: tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TesterDigestAuthenticatorPerformance.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TesterDigestAuthenticatorPerformance.java?rev=1405370&r1=1405369&r2=1405370&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TesterDigestAuthenticatorPerformance.java
(original)
+++ tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TesterDigestAuthenticatorPerformance.java
Sat Nov  3 15:50:41 2012
@@ -54,7 +54,7 @@ public class TesterDigestAuthenticatorPe
 
     @Test
     public void testSimple() throws Exception {
-        doTest(4, 100000);
+        doTest(4, 1000000);
     }
 
     public void doTest(int threadCount, int requestCount) throws Exception {
@@ -66,7 +66,8 @@ public class TesterDigestAuthenticatorPe
 
         // Create the runnables & threads
         for (int i = 0; i < threadCount; i++) {
-            runnables[i] = new TesterRunnable(nonce, requestCount);
+            runnables[i] =
+                    new TesterRunnable(authenticator, nonce, requestCount);
             threads[i] = new Thread(runnables[i]);
         }
 
@@ -124,7 +125,7 @@ public class TesterDigestAuthenticatorPe
     }
 
 
-    private class TesterRunnable implements Runnable {
+    private static class TesterRunnable implements Runnable {
 
         private String nonce;
         private int requestCount;
@@ -135,9 +136,22 @@ public class TesterDigestAuthenticatorPe
         private TesterDigestRequest request;
         private HttpServletResponse response;
         private LoginConfig config;
+        private DigestAuthenticator authenticator;
+
+        private static final String A1 = USER + ":" + REALM + ":" + PWD;
+        private static final String A2 = METHOD + ":" + CONTEXT_PATH + URI;
+
+        private static final String MD5A1 = MD5Encoder.encode(
+                ConcurrentMessageDigest.digest("MD5", A1.getBytes()));
+        private static final String MD5A2 = MD5Encoder.encode(
+                ConcurrentMessageDigest.digest("MD5", A2.getBytes()));
+
+
 
         // All init code should be in here. run() needs to be quick
-        public TesterRunnable(String nonce, int requestCount) throws Exception {
+        public TesterRunnable(DigestAuthenticator authenticator,
+                String nonce, int requestCount) throws Exception {
+            this.authenticator = authenticator;
             this.nonce = nonce;
             this.requestCount = requestCount;
 
@@ -181,16 +195,8 @@ public class TesterDigestAuthenticatorPe
                     Integer.valueOf(nonceCount.incrementAndGet()));
             String cnonce = "cnonce";
 
-            String a1 = USER + ":" + REALM + ":" + PWD;
-            String a2 = METHOD + ":" + CONTEXT_PATH + URI;
-
-            String md5a1 = MD5Encoder.encode(
-                    ConcurrentMessageDigest.digest("MD5", a1.getBytes()));
-            String md5a2 = MD5Encoder.encode(
-                    ConcurrentMessageDigest.digest("MD5", a2.getBytes()));
-
-            String response = md5a1 + ":" + nonce + ":" + ncString + ":" +
-                    cnonce + ":" + QOP + ":" + md5a2;
+            String response = MD5A1 + ":" + nonce + ":" + ncString + ":" +
+                    cnonce + ":" + QOP + ":" + MD5A2;
 
             String md5response = MD5Encoder.encode(
                     ConcurrentMessageDigest.digest("MD5", response.getBytes()));
@@ -209,12 +215,10 @@ public class TesterDigestAuthenticatorPe
             auth.append("\", response=\"");
             auth.append(md5response);
             auth.append("\"");
-            auth.append(", qop=\"");
+            auth.append(", qop=");
             auth.append(QOP);
-            auth.append("\"");
-            auth.append(", nc=\"");
+            auth.append(", nc=");
             auth.append(ncString);
-            auth.append("\"");
             auth.append(", cnonce=\"");
             auth.append(cnonce);
             auth.append("\"");

Copied: tomcat/tc7.0.x/trunk/test/org/apache/tomcat/util/http/parser/TestHttpParser2.java
(from r1404773, tomcat/trunk/test/org/apache/tomcat/util/http/parser/TestHttpParser2.java)
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/test/org/apache/tomcat/util/http/parser/TestHttpParser2.java?p2=tomcat/tc7.0.x/trunk/test/org/apache/tomcat/util/http/parser/TestHttpParser2.java&p1=tomcat/trunk/test/org/apache/tomcat/util/http/parser/TestHttpParser2.java&r1=1404773&r2=1405370&rev=1405370&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/tomcat/util/http/parser/TestHttpParser2.java (original)
+++ tomcat/tc7.0.x/trunk/test/org/apache/tomcat/util/http/parser/TestHttpParser2.java Sat
Nov  3 15:50:41 2012
@@ -113,4 +113,46 @@ public class TestHttpParser2 {
         Assert.assertEquals("auth", result.get("qop"));
         Assert.assertEquals("9926cb3c334ede11", result.get("cnonce"));
     }
+
+    @Test
+    public void testEndWithLhex() throws Exception {
+        String header = "Digest nc=00000001";
+
+        StringReader input = new StringReader(header);
+
+        Map<String,String> result = HttpParser2.parseAuthorizationDigest(input);
+
+        Assert.assertEquals("00000001", result.get("nc"));
+    }
+
+    @Test
+    public void testUnclosedQuotedString1() throws Exception {
+        String header = "Digest username=\"test";
+
+        StringReader input = new StringReader(header);
+
+        Map<String,String> result = HttpParser2.parseAuthorizationDigest(input);
+        Assert.assertNull(result);
+    }
+
+    @Test
+    public void testUnclosedQuotedString2() throws Exception {
+        String header = "Digest username=\"test\\";
+
+        StringReader input = new StringReader(header);
+
+        Map<String,String> result = HttpParser2.parseAuthorizationDigest(input);
+        Assert.assertNull(result);
+    }
+
+    @Test
+    public void testNonTokenDirective() throws Exception {
+        String header = "Digest user{name=\"test\"";
+
+        StringReader input = new StringReader(header);
+
+        Map<String,String> result = HttpParser2.parseAuthorizationDigest(input);
+        Assert.assertNull(result);
+    }
+
 }

Modified: tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml?rev=1405370&r1=1405369&r2=1405370&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml Sat Nov  3 15:50:41 2012
@@ -82,6 +82,12 @@
         multiple instances of the CGI servlet. (markt)
       </fix>
       <fix>
+        <bug>54060</bug>: Use a simple parser rather than a regular expression
+        to parse HTTP Digest authentication headers so the header is correctly
+        parsed. The new approach is also faster and generates less garbage.
+        (markt)
+      </fix>
+      <fix>
         <bug>54068</bug>: Rewrite the web fragment ordering algorithm to resolve
         multiple issues that resulted in incorrect ordering or failure to find
         a correct, valid order. (markt)



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message