Fix SCRAM final response handling

Author: arturobernalg

httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultAuthenticationStrategy.java

@@ -69,7 +69,6 @@ public class DefaultAuthenticationStrategy implements AuthenticationStrategy {
     private static final List<String> DEFAULT_SCHEME_PRIORITY =
             Collections.unmodifiableList(Arrays.asList(
                     StandardAuthScheme.BEARER,
-                    StandardAuthScheme.SCRAM_SHA_256,
                     StandardAuthScheme.DIGEST,
                     StandardAuthScheme.BASIC));
 

httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/AuthenticationHandler.java

@@ -227,6 +227,19 @@ public boolean handleResponse(
         }
 
         final Map<String, AuthChallenge> challengeMap = extractChallengeMap(challengeType, response, clientContext);
+        if (challengeMap.isEmpty() && !challenged && isChallengeExpected) {
+            final AuthScheme authScheme = authExchange.getAuthScheme();
+            if (authScheme != null) {
+                MessageSupport.parseHeaders(
+                        response,
+                        challengeType == ChallengeType.PROXY ? "Proxy-Authentication-Info" : "Authentication-Info",
+                        (buffer, cursor) -> {
+                            final String schemeName = authScheme.getName();
+                            final AuthChallenge authChallenge = parser.parse(challengeType, schemeName, buffer, cursor);
+                            challengeMap.put(schemeName.toLowerCase(Locale.ROOT), authChallenge);
+                        });
+            }
+        }
 
         if (challengeMap.isEmpty()) {
             if (LOG.isDebugEnabled()) {

httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/ScramScheme.java

@@ -80,6 +80,9 @@ public final class ScramScheme implements AuthScheme {
 
     private static final Logger LOG = LoggerFactory.getLogger(ScramScheme.class);
 
+    private static final int DEFAULT_WARN_MIN_ITERATIONS = 4096;
+    private static final int DEFAULT_MAX_ITERATIONS_ALLOWED = 100000;
+
     // RFC 7804 / RFC 5802 fixed no-CB GS2 header and its base64 value for 'c='
     private static final String GS2_HEADER = "n,,";
     private static final String C_BIND_B64 = "biws"; // base64("n,,")
@@ -100,6 +103,7 @@ private enum State {
     private final SecureRandom secureRandom;
     private final int warnMinIterations;
     private final int minIterationsRequired;
+    private final int maxIterationsAllowed;
 
     private State state = State.INIT;
     private boolean complete;
@@ -128,7 +132,7 @@ private enum State {
      * @since 5.6
      */
     public ScramScheme() {
-        this(4096, 0, null);
+        this(DEFAULT_WARN_MIN_ITERATIONS, 0, DEFAULT_MAX_ITERATIONS_ALLOWED, null);
     }
 
     /**
@@ -140,8 +144,26 @@ public ScramScheme() {
      * @since 5.6
      */
     public ScramScheme(final int warnMinIterations, final int minIterationsRequired, final SecureRandom rnd) {
+        this(warnMinIterations, minIterationsRequired, DEFAULT_MAX_ITERATIONS_ALLOWED, rnd);
+    }
+
+    /**
+     * Constructor with custom iteration policy.
+     *
+     * @param warnMinIterations     warn if iteration count is lower than this (0 disables warnings)
+     * @param minIterationsRequired fail if iteration count is lower than this (0 disables enforcement)
+     * @param maxIterationsAllowed  fail if iteration count is greater than this (must be positive)
+     * @param rnd                   optional secure random source (null uses system default)
+     * @since 5.6
+     */
+    public ScramScheme(
+            final int warnMinIterations,
+            final int minIterationsRequired,
+            final int maxIterationsAllowed,
+            final SecureRandom rnd) {
         this.warnMinIterations = Math.max(0, warnMinIterations);
         this.minIterationsRequired = Math.max(0, minIterationsRequired);
+        this.maxIterationsAllowed = Args.positive(maxIterationsAllowed, "Max iterations allowed");
         this.secureRandom = rnd != null ? rnd : new SecureRandom();
     }
 
@@ -206,9 +228,10 @@ public void processChallenge(
         Args.notNull(context, "HTTP context");
 
         if (authChallenge == null) {
-            if (!challenged) {
-                // Final response with no Authentication-Info: nothing to do
-                return;
+            if (!challenged && this.state == State.CLIENT_FINAL_SENT) {
+                zeroAndClearExpectedV();
+                this.state = State.FAILED;
+                throw new AuthenticationException("Missing SCRAM Authentication-Info");
             }
             throw new MalformedChallengeException("Null SCRAM challenge");
         }
@@ -232,10 +255,32 @@ public void processChallenge(
                 return;
             }
 
+            final String sid = params.get("sid");
+            if (sid == null || sid.isEmpty()) {
+                zeroAndClearExpectedV();
+                this.state = State.FAILED;
+                throw new MalformedChallengeException("SCRAM server-first missing sid");
+            }
+
             // server-first (data present)
-            final String decoded = b64ToString(data);
+            final String decoded;
+            try {
+                decoded = b64ToString(data);
+            } catch (final MalformedChallengeException ex) {
+                zeroAndClearExpectedV();
+                this.state = State.FAILED;
+                throw ex;
+            }
             this.serverFirstRaw = decoded;
-            final Map<String, String> attrs = parseAttrs(decoded);
+
+            final Map<String, String> attrs;
+            try {
+                attrs = parseAttrs(decoded);
+            } catch (final MalformedChallengeException ex) {
+                zeroAndClearExpectedV();
+                this.state = State.FAILED;
+                throw ex;
+            }
 
             final String r = attrs.get("r");
             final String s = attrs.get("s");
@@ -249,7 +294,6 @@ public void processChallenge(
                 throw new AuthenticationException("SCRAM server nonce does not start with client nonce");
             }
 
-            this.sid = params.get("sid");
             try {
                 this.salt = B64D.decode(s);
                 if (this.salt.length == 0) {
@@ -274,25 +318,66 @@ public void processChallenge(
                 throw new AuthenticationException(
                         "SCRAM iteration count below required minimum: " + this.iterations + " < " + this.minIterationsRequired);
             }
+            if (this.iterations > this.maxIterationsAllowed) {
+                this.state = State.FAILED;
+                throw new AuthenticationException(
+                        "SCRAM iteration count above allowed maximum: " + this.iterations + " > " + this.maxIterationsAllowed);
+            }
             if (this.warnMinIterations > 0 && this.iterations < this.warnMinIterations && LOG.isWarnEnabled()) {
                 LOG.warn("SCRAM iteration count ({}) lower than recommended ({})", this.iterations, warnMinIterations);
             }
 
+            this.sid = sid;
             this.serverNonce = r;
             this.state = State.SERVER_FIRST_RCVD;
             this.complete = false;
             zeroAndClearExpectedV();
             return;
         }
 
+        if (this.state != State.CLIENT_FINAL_SENT) {
+            final State currentState = this.state;
+            zeroAndClearExpectedV();
+            this.state = State.FAILED;
+            throw new AuthenticationException("SCRAM final response out of sequence: " + currentState);
+        }
+
+        final String sid = params.get("sid");
+        if (sid == null || sid.isEmpty()) {
+            zeroAndClearExpectedV();
+            this.state = State.FAILED;
+            throw new MalformedChallengeException("SCRAM Authentication-Info missing sid");
+        }
+        if (this.sid == null || !this.sid.equals(sid)) {
+            zeroAndClearExpectedV();
+            this.state = State.FAILED;
+            throw new AuthenticationException("SCRAM sid mismatch");
+        }
+
         // --- final-response path (Authentication-Info on any status) ---
         // For Authentication-Info, RFC 7804 does NOT mandate a scheme token; do NOT enforce scheme name here.
         final String data = params.get("data");
         if (data == null) {
-            return;
+            zeroAndClearExpectedV();
+            this.state = State.FAILED;
+            throw new MalformedChallengeException("SCRAM Authentication-Info missing data");
+        }
+        final String decoded;
+        try {
+            decoded = b64ToString(data);
+        } catch (final MalformedChallengeException ex) {
+            zeroAndClearExpectedV();
+            this.state = State.FAILED;
+            throw ex;
+        }
+        final Map<String, String> attrs;
+        try {
+            attrs = parseAttrs(decoded);
+        } catch (final MalformedChallengeException ex) {
+            zeroAndClearExpectedV();
+            this.state = State.FAILED;
+            throw ex;
         }
-        final String decoded = b64ToString(data);
-        final Map<String, String> attrs = parseAttrs(decoded);
         final String err = attrs.get("e");
         if (err != null) {
             this.state = State.FAILED;
@@ -303,7 +388,9 @@ public void processChallenge(
         }
         final String vB64 = attrs.get("v");
         if (vB64 == null) {
-            return;
+            zeroAndClearExpectedV();
+            this.state = State.FAILED;
+            throw new MalformedChallengeException("SCRAM Authentication-Info missing v");
         }
 
         // compare 'v' in constant time; treat bad base64 for v as a signature mismatch (tests expect "signature")
@@ -335,7 +422,7 @@ public void processChallenge(
      */
     @Override
     public boolean isChallengeComplete() {
-        return this.complete || this.state == State.COMPLETE || this.state == State.FAILED;
+        return this.state == State.FAILED;
     }
 
     /**
@@ -486,9 +573,7 @@ private String buildClientFinalAndExpectV() throws AuthenticationException {
 
             final StringBuilder sb = new StringBuilder(64);
             sb.append(StandardAuthScheme.SCRAM_SHA_256).append(' ');
-            if (this.sid != null) {
-                sb.append("sid=").append(quoteParam(this.sid)).append(", ");
-            }
+            sb.append("sid=").append(quoteParam(this.sid)).append(", ");
             sb.append("data=").append(quoteParam(data)); // quoted
 
             this.state = State.CLIENT_FINAL_SENT;

httpclient5/src/test/java/org/apache/hc/client5/http/examples/ClientScramAuthentication.java

@@ -0,0 +1,81 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation.  For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.hc.client5.http.examples;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.hc.client5.http.auth.StandardAuthScheme;
+import org.apache.hc.client5.http.classic.methods.HttpGet;
+import org.apache.hc.client5.http.config.RequestConfig;
+import org.apache.hc.client5.http.impl.auth.CredentialsProviderBuilder;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
+import org.apache.hc.client5.http.impl.classic.HttpClients;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.hc.core5.http.message.StatusLine;
+
+/**
+ * An example of how to explicitly enable {@code SCRAM-SHA-256} authentication.
+ * SCRAM-SHA-256 is not part of the default auth scheme preference list and
+ * must be opted into by placing it on the preferred auth schemes list of
+ * the {@link RequestConfig}.
+ *
+ * @since 5.7
+ */
+public class ClientScramAuthentication {
+
+    public static void main(final String[] args) throws Exception {
+        final List<String> priority = Collections.unmodifiableList(Arrays.asList(
+                StandardAuthScheme.SCRAM_SHA_256,
+                StandardAuthScheme.BEARER));
+
+        final HttpHost target = new HttpHost("http", "httpbin.org", 80);
+
+        try (final CloseableHttpClient httpclient = HttpClients.custom()
+                .setDefaultRequestConfig(RequestConfig.custom()
+                        .setTargetPreferredAuthSchemes(priority)
+                        .setProxyPreferredAuthSchemes(priority)
+                        .build())
+                .setDefaultCredentialsProvider(CredentialsProviderBuilder.create()
+                        .add(target, "user", "passwd".toCharArray())
+                        .build())
+                .build()) {
+
+            final HttpGet httpget = new HttpGet("http://httpbin.org/basic-auth/user/passwd");
+
+            System.out.println("Executing request " + httpget.getMethod() + " " + httpget.getUri());
+            httpclient.execute(httpget, response -> {
+                System.out.println("----------------------------------------");
+                System.out.println(httpget + "->" + new StatusLine(response));
+                EntityUtils.consume(response.getEntity());
+                return null;
+            });
+        }
+    }
+}

httpclient5/src/test/java/org/apache/hc/client5/http/impl/auth/TestAuthenticationHandler.java

@@ -29,6 +29,7 @@
 import java.util.LinkedList;
 import java.util.Queue;
 
+import org.apache.hc.client5.http.auth.AuthChallenge;
 import org.apache.hc.client5.http.auth.AuthExchange;
 import org.apache.hc.client5.http.auth.AuthScheme;
 import org.apache.hc.client5.http.auth.AuthSchemeFactory;
@@ -48,6 +49,7 @@
 import org.apache.hc.core5.http.HttpRequest;
 import org.apache.hc.core5.http.HttpResponse;
 import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.NameValuePair;
 import org.apache.hc.core5.http.config.Lookup;
 import org.apache.hc.core5.http.config.RegistryBuilder;
 import org.apache.hc.core5.http.message.BasicHeader;
@@ -58,6 +60,7 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.Answers;
+import org.mockito.ArgumentCaptor;
 import org.mockito.Mockito;
 
 class TestAuthenticationHandler {
@@ -481,4 +484,47 @@ void testAuthSuccessConnectionBased() throws Exception {
                 Mockito.any(HttpContext.class));
     }
 
+    private static String getParam(final AuthChallenge authChallenge, final String name) {
+        for (final NameValuePair param : authChallenge.getParams()) {
+            if (param.getName().equalsIgnoreCase(name)) {
+                return param.getValue();
+            }
+        }
+        return null;
+    }
+
+    @Test
+    void testAuthenticationInfoProcessedOnSuccessResponse() throws Exception {
+        final HttpHost host = new HttpHost("somehost", 80);
+        final HttpResponse response = new BasicHttpResponse(HttpStatus.SC_OK, "OK");
+        response.addHeader(new BasicHeader("Authentication-Info", "sid=\"sid-1\", data=\"dj1hYmM\""));
+
+        final AuthScheme authScheme = Mockito.mock(AuthScheme.class);
+        Mockito.when(authScheme.getName()).thenReturn(StandardAuthScheme.SCRAM_SHA_256);
+        Mockito.when(authScheme.isChallengeExpected()).thenReturn(Boolean.TRUE);
+        Mockito.when(authScheme.isChallengeComplete()).thenReturn(Boolean.FALSE);
+
+        this.authExchange.select(authScheme);
+        this.authExchange.setState(AuthExchange.State.HANDSHAKE);
+
+        final DefaultAuthenticationStrategy authStrategy = new DefaultAuthenticationStrategy();
+
+        Assertions.assertFalse(this.httpAuthenticator.handleResponse(
+                host, ChallengeType.TARGET, response, authStrategy, this.authExchange, this.context));
+        Assertions.assertEquals(AuthExchange.State.SUCCESS, this.authExchange.getState());
+
+        final ArgumentCaptor<AuthChallenge> challengeCaptor = ArgumentCaptor.forClass(AuthChallenge.class);
+        Mockito.verify(authScheme).processChallenge(
+                Mockito.eq(host),
+                Mockito.eq(false),
+                challengeCaptor.capture(),
+                Mockito.same(this.context));
+
+        final AuthChallenge challenge = challengeCaptor.getValue();
+        Assertions.assertNotNull(challenge);
+        Assertions.assertEquals(StandardAuthScheme.SCRAM_SHA_256, challenge.getSchemeName());
+        Assertions.assertEquals("sid-1", getParam(challenge, "sid"));
+        Assertions.assertEquals("dj1hYmM", getParam(challenge, "data"));
+    }
+
 }