You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2012/11/03 16:50:41 UTC
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/
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