From ee09904809c7e4c4da340dc62a58b84837afbaa1 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 17 Apr 2018 17:04:44 +1000 Subject: [PATCH 1/4] Reworked the way authentication parameters are parsed. Removed the regex to separate out the realm parameter and instead parse it with the other parameters into HeaderInfo. Changed HeaderInfo to store the parsed parameters as a Map instead of the un-parsed parameters in a string. The parsing of the parameters is now done in AuthenticationProtocolHandler.newHeaderInfo(String) and then passed into the HeaderInfo instead of Parsing it in DigestAuthentication. Replaced the usage of splitParams(String) with QuotedCSV used to parse the parameters. Added test to check the ordering of parameters doesn't matter. Allow not to have a realm parameter, changed DigestAuthentication.matches() to not match if realm is null, so that Digest Authentication requires realm parameter but any Basic Authentication can be done without it. There is currently no tests for this. Signed-off-by: Lachlan Roberts --- .../client/AuthenticationProtocolHandler.java | 67 +++++++++++++---- .../jetty/client/api/Authentication.java | 31 +++++--- .../client/util/DigestAuthentication.java | 72 ++++--------------- .../client/HttpClientAuthenticationTest.java | 31 ++++++++ 4 files changed, 117 insertions(+), 84 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java b/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java index 6d3a245dce6..8c9fc433222 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java @@ -20,12 +20,14 @@ package org.eclipse.jetty.client; import java.net.URI; import java.util.ArrayList; -import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.eclipse.jetty.client.api.Authentication; +import org.eclipse.jetty.client.api.Authentication.HeaderInfo; import org.eclipse.jetty.client.api.Connection; import org.eclipse.jetty.client.api.ContentProvider; import org.eclipse.jetty.client.api.ContentResponse; @@ -35,6 +37,8 @@ import org.eclipse.jetty.client.api.Result; import org.eclipse.jetty.client.util.BufferingResponseListener; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.http.QuotedCSV; +import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; @@ -42,8 +46,10 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler { public static final int DEFAULT_MAX_CONTENT_LENGTH = 16*1024; public static final Logger LOG = Log.getLogger(AuthenticationProtocolHandler.class); - private static final Pattern AUTHENTICATE_PATTERN = Pattern.compile("([^\\s]+)\\s(.*,\\s*)?realm=\"([^\"]*)\"\\s*,?\\s*(.*)", Pattern.CASE_INSENSITIVE); + private static final Pattern PARAM_PATTERN = Pattern.compile("([^=]+)=(.*)"); + private static final Pattern TYPE_PATTERN = Pattern.compile("([^\\s]+)\\s+(.*)"); + private final HttpClient client; private final int maxContentLength; private final ResponseNotifier notifier; @@ -74,6 +80,43 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler // Return new instances every time to keep track of the response content return new AuthenticationListener(); } + + + protected HeaderInfo newHeaderInfo(String value) throws IllegalArgumentException + { + String type; + Map params; + + Matcher m = TYPE_PATTERN.matcher(value); + if (m.matches()) + { + type = m.group(1); + params = parseParameters(m.group(2)); + } + else + { + throw new IllegalArgumentException("Invalid Authentication Format"); + } + + return new HeaderInfo(getAuthorizationHeader(), type, params); + } + + protected Map parseParameters(String wwwAuthenticate) + { + Map result = new HashMap<>(); + QuotedCSV parts = new QuotedCSV(false, wwwAuthenticate); + for (String part : parts) + { + Matcher matcher = PARAM_PATTERN.matcher(part); + if (matcher.matches()) + { + String name = StringUtil.asciiToLowerCase(matcher.group(1)); + String value = matcher.group(2); + result.put(name, value); + } + } + return result; + } private class AuthenticationListener extends BufferingResponseListener { @@ -234,23 +277,19 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler { // TODO: these should be ordered by strength List result = new ArrayList<>(); - List values = Collections.list(response.getHeaders().getValues(header.asString())); + List values = response.getHeaders().getValuesList(header); for (String value : values) { - Matcher matcher = AUTHENTICATE_PATTERN.matcher(value); - if (matcher.matches()) + try { - String type = matcher.group(1); - String realm = matcher.group(3); - String params; - if(matcher.group(2) != null) - params = matcher.group(2) + matcher.group(4); - else - params = matcher.group(4); - - Authentication.HeaderInfo headerInfo = new Authentication.HeaderInfo(type, realm, params, getAuthorizationHeader()); + Authentication.HeaderInfo headerInfo = newHeaderInfo(value); result.add(headerInfo); } + catch(IllegalArgumentException e) + { + if (LOG.isDebugEnabled()) + LOG.debug("Failed to parse authentication header", e); + } } return result; } diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/api/Authentication.java b/jetty-client/src/main/java/org/eclipse/jetty/client/api/Authentication.java index 34bed19f306..ff1355479d2 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/api/Authentication.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/api/Authentication.java @@ -19,9 +19,11 @@ package org.eclipse.jetty.client.api; import java.net.URI; +import java.util.Map; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.util.Attributes; +import org.eclipse.jetty.util.StringUtil; /** * {@link Authentication} represents a mechanism to authenticate requests for protected resources. @@ -76,19 +78,18 @@ public interface Authentication */ public static class HeaderInfo { - private final String type; - private final String realm; - private final String params; private final HttpHeader header; + private final String type; + private final Map params; - public HeaderInfo(String type, String realm, String params, HttpHeader header) + + public HeaderInfo(HttpHeader header, String type, Map params) throws IllegalArgumentException { - this.type = type; - this.realm = realm; - this.params = params; this.header = header; + this.type = type; + this.params = params; } - + /** * @return the authentication type (for example "Basic" or "Digest") */ @@ -98,20 +99,28 @@ public interface Authentication } /** - * @return the realm name + * @return the realm name or null if there is no realm parameter */ public String getRealm() { - return realm; + return params.get("realm"); } /** * @return additional authentication parameters */ - public String getParameters() + public Map getParameters() { return params; } + + /** + * @return specified authentication parameter or null if does not exist + */ + public String getParameter(String paramName) + { + return params.get(StringUtil.asciiToLowerCase(paramName)); + } /** * @return the {@code Authorization} (or {@code Proxy-Authorization}) header diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/util/DigestAuthentication.java b/jetty-client/src/main/java/org/eclipse/jetty/client/util/DigestAuthentication.java index 7a5e16bedcf..fc2fb8363d6 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/util/DigestAuthentication.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/util/DigestAuthentication.java @@ -22,15 +22,11 @@ import java.net.URI; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; -import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Random; import java.util.concurrent.atomic.AtomicInteger; -import java.util.regex.Matcher; -import java.util.regex.Pattern; import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.api.AuthenticationStore; @@ -40,6 +36,7 @@ import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.util.Attributes; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.TypeUtil; +import org.eclipse.jetty.util.log.Log; /** * Implementation of the HTTP "Digest" authentication defined in RFC 2617. @@ -50,8 +47,6 @@ import org.eclipse.jetty.util.TypeUtil; */ public class DigestAuthentication extends AbstractAuthentication { - private static final Pattern PARAM_PATTERN = Pattern.compile("([^=]+)=(.*)"); - private final String user; private final String password; @@ -74,10 +69,21 @@ public class DigestAuthentication extends AbstractAuthentication return "Digest"; } + + @Override + public boolean matches(String type, URI uri, String realm) + { + // digest authenication requires a realm + if (realm == null) + return false; + + return super.matches(type,uri,realm); + } + @Override public Result authenticate(Request request, ContentResponse response, HeaderInfo headerInfo, Attributes context) { - Map params = parseParameters(headerInfo.getParameters()); + Map params = headerInfo.getParameters(); String nonce = params.get("nonce"); if (nonce == null || nonce.length() == 0) return null; @@ -105,58 +111,6 @@ public class DigestAuthentication extends AbstractAuthentication return new DigestResult(headerInfo.getHeader(), response.getContent(), realm, user, password, algorithm, nonce, clientQOP, opaque); } - private Map parseParameters(String wwwAuthenticate) - { - Map result = new HashMap<>(); - List parts = splitParams(wwwAuthenticate); - for (String part : parts) - { - Matcher matcher = PARAM_PATTERN.matcher(part); - if (matcher.matches()) - { - String name = matcher.group(1).trim().toLowerCase(Locale.ENGLISH); - String value = matcher.group(2).trim(); - if (value.startsWith("\"") && value.endsWith("\"")) - value = value.substring(1, value.length() - 1); - result.put(name, value); - } - } - return result; - } - - private List splitParams(String paramString) - { - List result = new ArrayList<>(); - int start = 0; - for (int i = 0; i < paramString.length(); ++i) - { - int quotes = 0; - char ch = paramString.charAt(i); - switch (ch) - { - case '\\': - ++i; - break; - case '"': - ++quotes; - break; - case ',': - if (quotes % 2 == 0) - { - String element = paramString.substring(start, i).trim(); - if (element.length() > 0) - result.add(element); - start = i + 1; - } - break; - default: - break; - } - } - result.add(paramString.substring(start, paramString.length()).trim()); - return result; - } - private MessageDigest getMessageDigest(String algorithm) { try diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java index 3dd0d4a1c99..578752c4c63 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java @@ -43,6 +43,7 @@ import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.client.api.Response; import org.eclipse.jetty.client.api.Response.Listener; import org.eclipse.jetty.client.api.Result; +import org.eclipse.jetty.client.api.Authentication.HeaderInfo; import org.eclipse.jetty.client.util.BasicAuthentication; import org.eclipse.jetty.client.util.DeferredContentProvider; import org.eclipse.jetty.client.util.DigestAuthentication; @@ -613,4 +614,34 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest }; } } + + @Test + public void testParamOrdering() { + AuthenticationProtocolHandler aph = new WWWAuthenticationProtocolHandler(client); + + HeaderInfo headerInfo = aph.newHeaderInfo("Digest realm=\"thermostat\", qop=\"auth\", nonce=\"1523430383\""); + Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Digest")); + Assert.assertTrue(headerInfo.getParameter("qop").equals("auth")); + Assert.assertTrue(headerInfo.getParameter("realm").equals("thermostat")); + Assert.assertTrue(headerInfo.getParameter("nonce").equals("1523430383")); + + headerInfo = aph.newHeaderInfo("Digest qop=\"auth\", realm=\"thermostat\", nonce=\"1523430383\""); + Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Digest")); + Assert.assertTrue(headerInfo.getParameter("qop").equals("auth")); + Assert.assertTrue(headerInfo.getParameter("realm").equals("thermostat")); + Assert.assertTrue(headerInfo.getParameter("nonce").equals("1523430383")); + + headerInfo = aph.newHeaderInfo("Digest qop=\"auth\", nonce=\"1523430383\", realm=\"thermostat\""); + Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Digest")); + Assert.assertTrue(headerInfo.getParameter("qop").equals("auth")); + Assert.assertTrue(headerInfo.getParameter("realm").equals("thermostat")); + Assert.assertTrue(headerInfo.getParameter("nonce").equals("1523430383")); + + headerInfo = aph.newHeaderInfo("Digest qop=\"auth\", nonce=\"1523430383\""); + Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Digest")); + Assert.assertTrue(headerInfo.getParameter("qop").equals("auth")); + Assert.assertTrue(headerInfo.getParameter("realm") == null); + Assert.assertTrue(headerInfo.getParameter("nonce").equals("1523430383")); + + } } From 5a1325949eac48b7bf0eee7a8a896f6b3549f690 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 18 Apr 2018 16:02:33 +1000 Subject: [PATCH 2/4] Changes to allow for other formats specified in RFC7235 - Multiple challanges in the same header can now be parsed successfully. - Will now allow a base64 value after the auth-scheme instead of parameters. Which can be used for the Negotiate auth-scheme. - Added more in depth testing for tricky cases. Signed-off-by: Lachlan Roberts --- .../client/AuthenticationProtocolHandler.java | 55 ++++++++--- .../jetty/client/api/Authentication.java | 8 ++ .../client/HttpClientAuthenticationTest.java | 97 ++++++++++++++++++- 3 files changed, 143 insertions(+), 17 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java b/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java index 8c9fc433222..9a765f3990d 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java @@ -47,9 +47,11 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler public static final int DEFAULT_MAX_CONTENT_LENGTH = 16*1024; public static final Logger LOG = Log.getLogger(AuthenticationProtocolHandler.class); - private static final Pattern PARAM_PATTERN = Pattern.compile("([^=]+)=(.*)"); - private static final Pattern TYPE_PATTERN = Pattern.compile("([^\\s]+)\\s+(.*)"); - + private static final Pattern PARAM_PATTERN = Pattern.compile("([^=]+)=([^=]+)?"); + private static final Pattern TYPE_PATTERN = Pattern.compile("([^\\s]+)(\\s+(.*))?"); + private static final Pattern MULTIPLE_CHALLENGE_PATTERN = Pattern.compile("(.*),\\s*([^=\\s,]+(\\s+[^=\\s].*)?)\\s*"); + private static final Pattern BASE64_PATTERN = Pattern.compile("([^\\s,=]+=*)\\s*"); + private final HttpClient client; private final int maxContentLength; private final ResponseNotifier notifier; @@ -81,17 +83,35 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler return new AuthenticationListener(); } + protected List getHeaderInfo(String value) throws IllegalArgumentException + { + Matcher m = MULTIPLE_CHALLENGE_PATTERN.matcher(value); + if (m.matches()) + { + List l = new ArrayList<>(); + l.addAll(getHeaderInfo(m.group(1))); + l.addAll(getHeaderInfo(m.group(2))); + return l; + } + else + { + List l = new ArrayList<>(); + l.add(newHeaderInfo(value)); + return l; + } + } protected HeaderInfo newHeaderInfo(String value) throws IllegalArgumentException { String type; - Map params; + Map params = new HashMap<>(); Matcher m = TYPE_PATTERN.matcher(value); if (m.matches()) { type = m.group(1); - params = parseParameters(m.group(2)); + if (m.group(2) != null) + params = parseParameters(m.group(3)); } else { @@ -101,19 +121,31 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler return new HeaderInfo(getAuthorizationHeader(), type, params); } - protected Map parseParameters(String wwwAuthenticate) + protected Map parseParameters(String wwwAuthenticate) throws IllegalArgumentException { Map result = new HashMap<>(); + + Matcher b64 = BASE64_PATTERN.matcher(wwwAuthenticate); + if (b64.matches()) + { + result.put("base64", b64.group(1)); + return result; + } + QuotedCSV parts = new QuotedCSV(false, wwwAuthenticate); for (String part : parts) { - Matcher matcher = PARAM_PATTERN.matcher(part); - if (matcher.matches()) + Matcher params = PARAM_PATTERN.matcher(part); + if (params.matches()) { - String name = StringUtil.asciiToLowerCase(matcher.group(1)); - String value = matcher.group(2); + String name = StringUtil.asciiToLowerCase(params.group(1)); + String value = (params.group(2)==null)?"":params.group(2); result.put(name, value); } + else + { + throw new IllegalArgumentException("Invalid Authentication Format"); + } } return result; } @@ -282,8 +314,7 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler { try { - Authentication.HeaderInfo headerInfo = newHeaderInfo(value); - result.add(headerInfo); + result.addAll(getHeaderInfo(value)); } catch(IllegalArgumentException e) { diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/api/Authentication.java b/jetty-client/src/main/java/org/eclipse/jetty/client/api/Authentication.java index ff1355479d2..6333a377f35 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/api/Authentication.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/api/Authentication.java @@ -106,6 +106,14 @@ public interface Authentication return params.get("realm"); } + /** + * @return the base64 content as a string if it exists otherwise null + */ + public String getBase64() + { + return params.get("base64"); + } + /** * @return additional authentication parameters */ diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java index 578752c4c63..8bd2333dc8b 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java @@ -23,6 +23,7 @@ import java.io.IOException; import java.net.URI; import java.nio.ByteBuffer; import java.util.Iterator; +import java.util.List; import java.util.NoSuchElementException; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -64,6 +65,7 @@ import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.util.security.Constraint; import org.eclipse.jetty.util.ssl.SslContextFactory; +import org.hamcrest.Matchers; import org.junit.Assert; import org.junit.Test; @@ -616,32 +618,117 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest } @Test - public void testParamOrdering() { + public void testTestHeaderInfoParsing() { AuthenticationProtocolHandler aph = new WWWAuthenticationProtocolHandler(client); - HeaderInfo headerInfo = aph.newHeaderInfo("Digest realm=\"thermostat\", qop=\"auth\", nonce=\"1523430383\""); + HeaderInfo headerInfo = aph.getHeaderInfo("Digest realm=\"thermostat\", qop=\"auth\", nonce=\"1523430383\"").get(0); Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Digest")); Assert.assertTrue(headerInfo.getParameter("qop").equals("auth")); Assert.assertTrue(headerInfo.getParameter("realm").equals("thermostat")); Assert.assertTrue(headerInfo.getParameter("nonce").equals("1523430383")); - headerInfo = aph.newHeaderInfo("Digest qop=\"auth\", realm=\"thermostat\", nonce=\"1523430383\""); + headerInfo = aph.getHeaderInfo("Digest qop=\"auth\", realm=\"thermostat\", nonce=\"1523430383\"").get(0); Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Digest")); Assert.assertTrue(headerInfo.getParameter("qop").equals("auth")); Assert.assertTrue(headerInfo.getParameter("realm").equals("thermostat")); Assert.assertTrue(headerInfo.getParameter("nonce").equals("1523430383")); - headerInfo = aph.newHeaderInfo("Digest qop=\"auth\", nonce=\"1523430383\", realm=\"thermostat\""); + headerInfo = aph.getHeaderInfo("Digest qop=\"auth\", nonce=\"1523430383\", realm=\"thermostat\"").get(0); Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Digest")); Assert.assertTrue(headerInfo.getParameter("qop").equals("auth")); Assert.assertTrue(headerInfo.getParameter("realm").equals("thermostat")); Assert.assertTrue(headerInfo.getParameter("nonce").equals("1523430383")); - headerInfo = aph.newHeaderInfo("Digest qop=\"auth\", nonce=\"1523430383\""); + headerInfo = aph.getHeaderInfo("Digest qop=\"auth\", nonce=\"1523430383\"").get(0); Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Digest")); Assert.assertTrue(headerInfo.getParameter("qop").equals("auth")); Assert.assertTrue(headerInfo.getParameter("realm") == null); Assert.assertTrue(headerInfo.getParameter("nonce").equals("1523430383")); + + // test multiple authentications + List headerInfoList = aph.getHeaderInfo("Digest qop=\"auth\", realm=\"thermostat\", nonce=\"1523430383\", " + + "Digest realm=\"thermostat2\", qop=\"auth2\", nonce=\"4522530354\", " + + "Digest qop=\"auth3\", nonce=\"9523570528\", realm=\"thermostat3\", " + + "Digest qop=\"auth4\", nonce=\"3526435321\""); + + Assert.assertTrue(headerInfoList.get(0).getType().equalsIgnoreCase("Digest")); + Assert.assertTrue(headerInfoList.get(0).getParameter("qop").equals("auth")); + Assert.assertTrue(headerInfoList.get(0).getParameter("realm").equals("thermostat")); + Assert.assertTrue(headerInfoList.get(0).getParameter("nonce").equals("1523430383")); + + Assert.assertTrue(headerInfoList.get(1).getType().equalsIgnoreCase("Digest")); + Assert.assertTrue(headerInfoList.get(1).getParameter("qop").equals("auth2")); + Assert.assertTrue(headerInfoList.get(1).getParameter("realm").equals("thermostat2")); + Assert.assertTrue(headerInfoList.get(1).getParameter("nonce").equals("4522530354")); + + Assert.assertTrue(headerInfoList.get(2).getType().equalsIgnoreCase("Digest")); + Assert.assertTrue(headerInfoList.get(2).getParameter("qop").equals("auth3")); + Assert.assertTrue(headerInfoList.get(2).getParameter("realm").equals("thermostat3")); + Assert.assertTrue(headerInfoList.get(2).getParameter("nonce").equals("9523570528")); + + Assert.assertTrue(headerInfoList.get(3).getType().equalsIgnoreCase("Digest")); + Assert.assertTrue(headerInfoList.get(3).getParameter("qop").equals("auth4")); + Assert.assertTrue(headerInfoList.get(3).getParameter("realm") == null); + Assert.assertTrue(headerInfoList.get(3).getParameter("nonce").equals("3526435321")); + + List headerInfos = aph.getHeaderInfo("Newauth realm=\"apps\", type=1, title=\"Login to \\\"apps\\\"\", Basic realm=\"simple\""); + Assert.assertTrue(headerInfos.get(0).getType().equalsIgnoreCase("Newauth")); + Assert.assertTrue(headerInfos.get(0).getParameter("realm").equals("apps")); + Assert.assertTrue(headerInfos.get(0).getParameter("type").equals("1")); + Assert.assertThat(headerInfos.get(0).getParameter("title"), Matchers.equalTo("Login to \"apps\"")); + Assert.assertTrue(headerInfos.get(1).getType().equalsIgnoreCase("Basic")); + Assert.assertTrue(headerInfos.get(1).getParameter("realm").equals("simple")); + } + + @Test + public void testTestHeaderInfoParsingUnusualCases() { + AuthenticationProtocolHandler aph = new WWWAuthenticationProtocolHandler(client); + + HeaderInfo headerInfo = aph.getHeaderInfo("Scheme").get(0); + Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Scheme")); + Assert.assertTrue(headerInfo.getParameter("realm") == null); + + List headerInfos = aph.getHeaderInfo("Scheme1 , Scheme2 "); + Assert.assertEquals(headerInfos.size(), 2); + Assert.assertTrue(headerInfos.get(0).getType().equalsIgnoreCase("Scheme1")); + Assert.assertTrue(headerInfos.get(1).getType().equalsIgnoreCase("Scheme2")); + + headerInfo = aph.getHeaderInfo("Scheme name=\"value\", other=\"value2\"").get(0); + Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Scheme")); + Assert.assertTrue(headerInfo.getParameter("name").equals("value")); + Assert.assertTrue(headerInfo.getParameter("other").equals("value2")); + + headerInfo = aph.getHeaderInfo("Scheme name = value , other = \"value2\" ").get(0); + Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Scheme")); + Assert.assertTrue(headerInfo.getParameter("name").equals("value")); + Assert.assertTrue(headerInfo.getParameter("other").equals("value2")); + + headerInfos = aph.getHeaderInfo("Scheme name=value, Scheme2 name=value2"); + Assert.assertEquals(headerInfos.size(), 2); + Assert.assertTrue(headerInfos.get(0).getType().equalsIgnoreCase("Scheme")); + Assert.assertTrue(headerInfos.get(0).getParameter("nAmE").equals("value")); + Assert.assertThat(headerInfos.get(1).getType(), Matchers.equalToIgnoringCase("Scheme2")); + Assert.assertTrue(headerInfos.get(1).getParameter("nAmE").equals("value2")); + + headerInfos = aph.getHeaderInfo("Scheme , ,, ,, name=value, Scheme2 name=value2"); + Assert.assertEquals(headerInfos.size(), 2); + Assert.assertTrue(headerInfos.get(0).getType().equalsIgnoreCase("Scheme")); + Assert.assertTrue(headerInfos.get(0).getParameter("name").equals("value")); + Assert.assertTrue(headerInfos.get(1).getType().equalsIgnoreCase("Scheme2")); + Assert.assertTrue(headerInfos.get(1).getParameter("name").equals("value2")); + + //Negotiate with base64 Content + headerInfo = aph.getHeaderInfo("Negotiate TlRMTVNTUAABAAAAB4IIogAAAAAAAAAAAAAAAAAAAAAFAs4OAAAADw==").get(0); + Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Negotiate")); + Assert.assertTrue(headerInfo.getBase64().equals("TlRMTVNTUAABAAAAB4IIogAAAAAAAAAAAAAAAAAAAAAFAs4OAAAADw==")); + + headerInfos = aph.getHeaderInfo("Negotiate TlRMTVNTUAABAAAAAAAAAFAs4OAAAADw==, " + + "Negotiate YIIJvwYGKwYBBQUCoIIJszCCCa+gJDAi="); + Assert.assertTrue(headerInfos.get(0).getType().equalsIgnoreCase("Negotiate")); + Assert.assertTrue(headerInfos.get(0).getBase64().equals("TlRMTVNTUAABAAAAAAAAAFAs4OAAAADw==")); + + Assert.assertTrue(headerInfos.get(1).getType().equalsIgnoreCase("Negotiate")); + Assert.assertTrue(headerInfos.get(1).getBase64().equals("YIIJvwYGKwYBBQUCoIIJszCCCa+gJDAi=")); } } From fc5c438d7217bbb7e96ac111075c739cd1d4942d Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 24 Apr 2018 11:16:31 +1000 Subject: [PATCH 3/4] Changes after review 1 Changed the base64 pattern to only accept token68 pattern from rfc7235#appendix-C Add limit to recusion depth of multiple challange matching to stop any vulnerablilties related to malicious server overflowing client stack Regex no longer allows trailing whitespace Signed-off-by: Lachlan Roberts --- .../client/AuthenticationProtocolHandler.java | 21 +++++++++++++------ .../client/HttpClientAuthenticationTest.java | 2 +- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java b/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java index 9a765f3990d..0fb88500c9d 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java @@ -49,8 +49,9 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler private static final Pattern PARAM_PATTERN = Pattern.compile("([^=]+)=([^=]+)?"); private static final Pattern TYPE_PATTERN = Pattern.compile("([^\\s]+)(\\s+(.*))?"); - private static final Pattern MULTIPLE_CHALLENGE_PATTERN = Pattern.compile("(.*),\\s*([^=\\s,]+(\\s+[^=\\s].*)?)\\s*"); - private static final Pattern BASE64_PATTERN = Pattern.compile("([^\\s,=]+=*)\\s*"); + private static final Pattern MULTIPLE_CHALLENGE_PATTERN = Pattern.compile("(.*),\\s*([^=\\s,]+(\\s+[^=\\s].*)?)"); + private static final Pattern BASE64_PATTERN = Pattern.compile("[\\+\\-\\.\\/\\dA-Z_a-z~]+=*"); + private static final int MAX_DEPTH = 10; private final HttpClient client; private final int maxContentLength; @@ -85,12 +86,20 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler protected List getHeaderInfo(String value) throws IllegalArgumentException { + return getHeaderInfo(value, 0); + } + + protected List getHeaderInfo(String value, int depth) throws IllegalArgumentException + { + if(depth > MAX_DEPTH) + throw new IllegalStateException("too many challanges"); + Matcher m = MULTIPLE_CHALLENGE_PATTERN.matcher(value); if (m.matches()) { List l = new ArrayList<>(); - l.addAll(getHeaderInfo(m.group(1))); - l.addAll(getHeaderInfo(m.group(2))); + l.addAll(getHeaderInfo(m.group(1), depth+1)); + l.addAll(getHeaderInfo(m.group(2), depth+1)); return l; } else @@ -128,7 +137,7 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler Matcher b64 = BASE64_PATTERN.matcher(wwwAuthenticate); if (b64.matches()) { - result.put("base64", b64.group(1)); + result.put("base64", wwwAuthenticate); return result; } @@ -139,7 +148,7 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler if (params.matches()) { String name = StringUtil.asciiToLowerCase(params.group(1)); - String value = (params.group(2)==null)?"":params.group(2); + String value = (params.group(2)==null) ? "" : params.group(2); result.put(name, value); } else diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java index 8bd2333dc8b..c3f04e82e28 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java @@ -689,7 +689,7 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Scheme")); Assert.assertTrue(headerInfo.getParameter("realm") == null); - List headerInfos = aph.getHeaderInfo("Scheme1 , Scheme2 "); + List headerInfos = aph.getHeaderInfo("Scheme1 , Scheme2"); Assert.assertEquals(headerInfos.size(), 2); Assert.assertTrue(headerInfos.get(0).getType().equalsIgnoreCase("Scheme1")); Assert.assertTrue(headerInfos.get(1).getType().equalsIgnoreCase("Scheme2")); From 04ae33c4ac9821b49e071b4f25f843e56a2cb748 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 25 Apr 2018 16:09:55 +1000 Subject: [PATCH 4/4] Switched getHeaderInfo from recursion to iteration. Signed-off-by: Lachlan Roberts --- .../client/AuthenticationProtocolHandler.java | 40 +++++++++---------- .../client/HttpClientAuthenticationTest.java | 5 ++- 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java b/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java index 0baec4a7407..c6a160ea214 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java @@ -49,9 +49,8 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler private static final Pattern PARAM_PATTERN = Pattern.compile("([^=]+)=([^=]+)?"); private static final Pattern TYPE_PATTERN = Pattern.compile("([^\\s]+)(\\s+(.*))?"); - private static final Pattern MULTIPLE_CHALLENGE_PATTERN = Pattern.compile("(.*),\\s*([^=\\s,]+(\\s+[^=\\s].*)?)"); + private static final Pattern MULTIPLE_CHALLENGE_PATTERN = Pattern.compile("(.*?)\\s*,\\s*([^=\\s,]+(\\s+[^=\\s].*)?)"); private static final Pattern BASE64_PATTERN = Pattern.compile("[\\+\\-\\.\\/\\dA-Z_a-z~]+=*"); - private static final int MAX_DEPTH = 10; private final HttpClient client; private final int maxContentLength; @@ -84,30 +83,29 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler return new AuthenticationListener(); } + + protected List getHeaderInfo(String value) throws IllegalArgumentException { - return getHeaderInfo(value, 0); - } - - protected List getHeaderInfo(String value, int depth) throws IllegalArgumentException - { - if(depth > MAX_DEPTH) - throw new IllegalStateException("too many challanges"); + String header = value; + List headerInfos = new ArrayList<>(); - Matcher m = MULTIPLE_CHALLENGE_PATTERN.matcher(value); - if (m.matches()) + while(true) { - List l = new ArrayList<>(); - l.addAll(getHeaderInfo(m.group(1), depth+1)); - l.addAll(getHeaderInfo(m.group(2), depth+1)); - return l; - } - else - { - List l = new ArrayList<>(); - l.add(newHeaderInfo(value)); - return l; + Matcher m = MULTIPLE_CHALLENGE_PATTERN.matcher(header); + if (m.matches()) + { + headerInfos.add(newHeaderInfo(m.group(1))); + header = m.group(2); + } + else + { + headerInfos.add(newHeaderInfo(header)); + break; + } } + + return headerInfos; } protected HeaderInfo newHeaderInfo(String value) throws IllegalArgumentException diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java index c3f04e82e28..ccaacd51dbd 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java @@ -689,10 +689,11 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Scheme")); Assert.assertTrue(headerInfo.getParameter("realm") == null); - List headerInfos = aph.getHeaderInfo("Scheme1 , Scheme2"); - Assert.assertEquals(headerInfos.size(), 2); + List headerInfos = aph.getHeaderInfo("Scheme1 , Scheme2 , Scheme3"); + Assert.assertEquals(3, headerInfos.size()); Assert.assertTrue(headerInfos.get(0).getType().equalsIgnoreCase("Scheme1")); Assert.assertTrue(headerInfos.get(1).getType().equalsIgnoreCase("Scheme2")); + Assert.assertTrue(headerInfos.get(2).getType().equalsIgnoreCase("Scheme3")); headerInfo = aph.getHeaderInfo("Scheme name=\"value\", other=\"value2\"").get(0); Assert.assertTrue(headerInfo.getType().equalsIgnoreCase("Scheme"));