apply discussed fixes

stoty · stoty · commit 1e1b79f55f0a · 2025-10-20T20:24:03.000+02:00
set isConnectionBased() to false
fix logic in needsAuthentication()
diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestSpnegoScheme.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestSpnegoScheme.java
@@ -291,6 +291,7 @@ public AuthScheme create(final HttpContext context) {
      * Tests that the client will stop connecting to the server if
      * the server still keep asking for a valid ticket.
      */
+    //@Disabled
     @Test
     void testDontTryToAuthenticateEndlessly() throws Exception {
         configureServer(t -> {
@@ -310,42 +311,16 @@ void testDontTryToAuthenticateEndlessly() throws Exception {
         final HttpHost target = startServer();
         final String s = "/path";
         final HttpGet httpget = new HttpGet(s);
-        client().execute(target, httpget, response -> {
-            EntityUtils.consume(response.getEntity());
-            Assertions.assertEquals(HttpStatus.SC_UNAUTHORIZED, response.getCode());
-            return null;
-        });
-    }
-
-    /**
-     * Javadoc specifies that {@link GSSContext#initSecContext(byte[], int, int)} can return null
-     * if no token is generated. Client should be able to deal with this response.
-     */
-    @Test
-    void testNoTokenGeneratedError() throws Exception {
-        configureServer(t -> {
-            t.register("*", new PleaseNegotiateService());
-        });
-
-        final AuthSchemeFactory nsf = new TestAuthSchemeFactory(new NegotiateSchemeWithMockGssManager());
-        final Registry<AuthSchemeFactory> authSchemeRegistry = RegistryBuilder.<AuthSchemeFactory>create()
-            .register(StandardAuthScheme.SPNEGO, nsf)
-            .build();
-        configureClient(t -> {
-            t.setTargetAuthenticationStrategy(spnegoAuthenticationStrategy);
-            t.setDefaultAuthSchemeRegistry(authSchemeRegistry);
-            t.setDefaultCredentialsProvider(jaasCredentialsProvider);
-        });
-
-        final HttpHost target = startServer();
-        final String s = "/path";
-        final HttpGet httpget = new HttpGet(s);
-        client().execute(target, httpget, response -> {
-            EntityUtils.consume(response.getEntity());
-            Assertions.assertEquals(HttpStatus.SC_UNAUTHORIZED, response.getCode());
-            return null;
-        });
-
+        try {
+            client().execute(target, httpget, response -> {
+                EntityUtils.consume(response.getEntity());
+                Assertions.assertEquals(HttpStatus.SC_UNAUTHORIZED, response.getCode());
+                return null;
+            });
+            Assertions.fail();
+        } catch (final IllegalStateException e) {
+            // Expected
+        }
     }
 
     /**
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/auth/gss/GssConfig.java b/httpclient5/src/main/java/org/apache/hc/client5/http/auth/gss/GssConfig.java
@@ -32,7 +32,7 @@
 
 /**
  * Immutable class encapsulating GSS configuration options for the new mutual auth capable
- * SpnegoScheme.
+ * for the new {@link SpnegoScheme}.
  *
  * Unlike the deprecated {@link KerberosConfig}, this class uses explicit defaults, and
  * primitive booleans.
@@ -49,14 +49,14 @@ public class GssConfig implements Cloneable {
 
     public static final GssConfig DEFAULT = new Builder().build();
     public static final GssConfig LEGACY =
-            new Builder().setIgnoreMissingToken(true).setRequireMutualAuth(false).build();
+            new Builder().setIgnoreIncompleteSecurityContext(true).setRequireMutualAuth(false).build();
 
     private final boolean addPort;
     private final boolean useCanonicalHostname;
     private final boolean requestMutualAuth;
     private final boolean requireMutualAuth;
     private final boolean requestDelegCreds;
-    private final boolean ignoreMissingToken;
+    private final boolean ignoreIncompleteSecurityContext;
 
     /**
      * Intended for CDI compatibility
@@ -71,14 +71,14 @@ protected GssConfig() {
             final boolean requestMutualAuth,
             final boolean requireMutualAuth,
             final boolean requestDelegCreds,
-            final boolean ignoreMissingToken) {
+            final boolean ignoreIncompleteSecurityContext) {
         super();
         this.addPort = addPort;
         this.useCanonicalHostname = useCanonicalHostname;
         this.requestMutualAuth = requestMutualAuth;
         this.requireMutualAuth = requireMutualAuth;
         this.requestDelegCreds = requestDelegCreds;
-        this.ignoreMissingToken = ignoreMissingToken;
+        this.ignoreIncompleteSecurityContext = ignoreIncompleteSecurityContext;
     }
 
     public boolean isAddPort() {
@@ -101,8 +101,8 @@ public boolean isRequireMutualAuth() {
         return requireMutualAuth;
     }
 
-    public boolean isIgnoreMissingToken() {
-        return ignoreMissingToken;
+    public boolean isIgnoreIncompleteSecurityContext() {
+        return ignoreIncompleteSecurityContext;
     }
 
     @Override
@@ -119,7 +119,7 @@ public String toString() {
         builder.append(", requestDelegCreds=").append(requestDelegCreds);
         builder.append(", requestMutualAuth=").append(requestMutualAuth);
         builder.append(", requireMutualAuth=").append(requireMutualAuth);
-        builder.append(", ignoreMissingToken=").append(ignoreMissingToken);
+        builder.append(", ignoreIncompleteSecurityContext=").append(ignoreIncompleteSecurityContext);
         builder.append("]");
         return builder.toString();
     }
@@ -135,7 +135,7 @@ public static GssConfig.Builder copy(final GssConfig config) {
                 .setRequestDelegCreds(config.isRequestDelegCreds())
                 .setRequireMutualAuth(config.isRequireMutualAuth())
                 .setRequestMutualAuth(config.isRequestMutualAuth())
-                .setIgnoreMissingToken(config.isIgnoreMissingToken());
+                .setIgnoreIncompleteSecurityContext(config.isIgnoreIncompleteSecurityContext());
     }
 
     public static class Builder {
@@ -145,7 +145,7 @@ public static class Builder {
         private boolean requestMutualAuth = true;
         private boolean requireMutualAuth = true;
         private boolean requestDelegCreds = false;
-        private boolean ignoreMissingToken = false;
+        private boolean ignoreIncompleteSecurityContext = false;
 
 
         Builder() {
@@ -177,14 +177,14 @@ public Builder setRequestDelegCreds(final boolean requuestDelegCreds) {
             return this;
         }
 
-        public Builder setIgnoreMissingToken(final boolean ignoreMissingToken) {
-            this.ignoreMissingToken = ignoreMissingToken;
+        public Builder setIgnoreIncompleteSecurityContext(final boolean ignoreIncompleteSecurityContext) {
+            this.ignoreIncompleteSecurityContext = ignoreIncompleteSecurityContext;
             return this;
         }
 
         public GssConfig build() {
-            if (requireMutualAuth && ignoreMissingToken) {
-                throw new IllegalArgumentException("If requireMutualAuth is set then ignoreMissingToken must not be set");
+            if (requireMutualAuth && ignoreIncompleteSecurityContext) {
+                throw new IllegalArgumentException("If requireMutualAuth is set then ignoreIncompleteSecurityContext must not be set");
             }
             if (requireMutualAuth && !requestMutualAuth) {
                 throw new IllegalArgumentException("If requireMutualAuth is set then requestMutualAuth must also be set");
@@ -195,7 +195,7 @@ public GssConfig build() {
                     requestMutualAuth,
                     requireMutualAuth,
                     requestDelegCreds,
-                    ignoreMissingToken
+                    ignoreIncompleteSecurityContext
                     );
         }
 
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/auth/gss/GssCredentials.java b/httpclient5/src/main/java/org/apache/hc/client5/http/auth/gss/GssCredentials.java
@@ -63,6 +63,7 @@ public GSSCredential getGSSCredential() {
 
     @Override
     public Principal getUserPrincipal() {
+        // TODO Can we extract this somehow ?
         return null;
     }
 
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java
@@ -334,26 +334,25 @@ private boolean needAuthentication(
                 }
             }
 
+            boolean targetNeedsAuth = false;
+            boolean proxyNeedsAuth = false;
             if (targetAuthRequested || targetMutualAuthRequired) {
-                final boolean updated = authenticator.handleResponse(target, ChallengeType.TARGET, response,
+                targetNeedsAuth = authenticator.handleResponse(target, ChallengeType.TARGET, response,
                         targetAuthStrategy, targetAuthExchange, context);
 
                 if (authCacheKeeper != null) {
                     authCacheKeeper.updateOnResponse(target, pathPrefix, targetAuthExchange, context);
                 }
-
-                return updated;
             }
             if (proxyAuthRequested || proxyMutualAuthRequired) {
-                final boolean updated = authenticator.handleResponse(proxy, ChallengeType.PROXY, response,
+                proxyNeedsAuth = authenticator.handleResponse(proxy, ChallengeType.PROXY, response,
                         proxyAuthStrategy, proxyAuthExchange, context);
 
                 if (authCacheKeeper != null) {
                     authCacheKeeper.updateOnResponse(proxy, null, proxyAuthExchange, context);
                 }
-
-                return updated;
             }
+            return targetNeedsAuth || proxyNeedsAuth;
         }
         return false;
     }
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/AuthenticationHandler.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/AuthenticationHandler.java
@@ -363,7 +363,7 @@ public boolean handleResponse(
 
     /**
      * Generates a response to the authentication challenge based on the actual {@link AuthExchange} state
-     * and adds it to the given {@link HttpRequest} message .
+     * and adds it to the given {@link HttpRequest} message.
      *
      * @param host the hostname of the opposite endpoint.
      * @param challengeType the challenge type (target or proxy).
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/gss/GssSchemeBase.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/gss/GssSchemeBase.java
@@ -77,15 +77,16 @@ enum State {
     }
 
     private static final Logger LOG = LoggerFactory.getLogger(GssSchemeBase.class);
-    private static final String KERBEROS_SCHEME = "HTTP";
+    private static final String PEER_SERVICE_NAME = "HTTP";
 
     // The GSS spec does not specify how long the conversation can be. This should be plenty.
     // Realistically, we get one initial token, then one maybe one more for mutual authentication.
+    // TODO In the future this might need to be configurable with the upcoming IAKerb support
     private static final int MAX_GSS_CHALLENGES = 3;
     private final GssConfig config;
     private final DnsResolver dnsResolver;
     private final boolean requireMutualAuth;
-    private final boolean ignoreMissingToken;
+    private final boolean ignoreIncompleteSecurityContext;
     private int challengesLeft = MAX_GSS_CHALLENGES;
 
     /** Authentication process state */
@@ -100,10 +101,24 @@ enum State {
         this.config = config != null ? config : GssConfig.DEFAULT;
         this.dnsResolver = dnsResolver != null ? dnsResolver : SystemDefaultDnsResolver.INSTANCE;
         this.requireMutualAuth = config.isRequireMutualAuth();
-        this.ignoreMissingToken = config.isIgnoreMissingToken();
+        this.ignoreIncompleteSecurityContext = config.isIgnoreIncompleteSecurityContext();
         this.state = State.UNINITIATED;
     }
 
+    private void dispose() {
+        // remove sensitive information from memory
+        // cleaning up the credential is the caller's job
+        try {
+            if (gssContext != null) {
+                gssContext.dispose();
+            }
+        } catch (final Exception e) {
+            if (LOG.isWarnEnabled()) {
+                LOG.warn("Exception caught while calling gssContext.dispose()", e);
+            }
+        }
+    }
+
     GssSchemeBase(final GssConfig config) {
         this(config, SystemDefaultDnsResolver.INSTANCE);
     }
@@ -136,24 +151,28 @@ public void processChallenge(
             ) throws AuthenticationException {
 
         if (challengesLeft-- <= 0 ) {
-            if (LOG.isDebugEnabled()) {
+            if (LOG.isWarnEnabled()) {
                 final HttpClientContext clientContext = HttpClientContext.cast(context);
                 final String exchangeId = clientContext.getExchangeId();
-                LOG.debug("{} GSS error: too many challenges received. Infinite loop ?", exchangeId);
+                LOG.warn("{} GSS error: too many challenges received. Infinite loop ?", exchangeId);
             }
-            // TODO: Should we throw an exception ? There is a test for this behaviour.
             state = State.FAILED;
             return;
         }
 
-        final byte[] challengeToken = Base64.decodeBase64(authChallenge == null ? null : authChallenge.getValue());
+        final byte[] challengeToken = (authChallenge == null) ? null : Base64.decodeBase64(authChallenge.getValue());
 
         final String gssHostname;
         String hostname = host.getHostName();
         if (config.isUseCanonicalHostname()) {
             try {
                  hostname = dnsResolver.resolveCanonicalHostname(host.getHostName());
             } catch (final UnknownHostException ignore) {
+                if (LOG.isWarnEnabled()) {
+                    final HttpClientContext clientContext = HttpClientContext.cast(context);
+                    final String exchangeId = clientContext.getExchangeId();
+                    LOG.warn("{} Could not canonicalize hostname {}, using as is.", exchangeId, host.getHostName());
+                }
             }
         }
         if (config.isAddPort()) {
@@ -171,42 +190,42 @@ public void processChallenge(
             switch (state) {
             case UNINITIATED:
                 setGssCredential(HttpClientContext.cast(context).getCredentialsProvider(), host, context);
-                queuedToken = generateToken(challengeToken, KERBEROS_SCHEME, gssHostname);
-                if (challengeToken != null) {
+                if (challengeToken == null) {
+                    queuedToken = generateToken(challengeToken, PEER_SERVICE_NAME, gssHostname);
+                    state = State.TOKEN_READY;
+                } else {
                     if (LOG.isDebugEnabled()) {
                         final HttpClientContext clientContext = HttpClientContext.cast(context);
                         final String exchangeId = clientContext.getExchangeId();
                         LOG.debug("{} Internal GSS error: token received when none was sent yet: {}", exchangeId, challengeToken);
                     }
-                    // TODO Should we fail ? That would break existing tests that send a token
-                    // in the first response, which is against the RFC.
+                    state = State.FAILED;
                 }
-                state = State.TOKEN_READY;
                 break;
             case TOKEN_SENT:
                 if (challengeToken == null) {
-                    if (!challenged && ignoreMissingToken) {
+                    if (!challenged && ignoreIncompleteSecurityContext) {
                         // Got a Non 401/407 code without a challenge. Old non RFC compliant server.
-                        if (LOG.isDebugEnabled()) {
+                        if (LOG.isWarnEnabled()) {
                             final HttpClientContext clientContext = HttpClientContext.cast(context);
                             final String exchangeId = clientContext.getExchangeId();
-                            LOG.debug("{} GSS Context is not established, but continuing because GssConfig.ignoreMissingToken is true.", exchangeId);
+                            LOG.warn("{} GSS Context is not established, but continuing because GssConfig.ignoreIncompleteSecurityContext is true.", exchangeId);
                         }
                         state = State.SUCCEEDED;
                         break;
                     } else {
                         if (LOG.isDebugEnabled()) {
                             final HttpClientContext clientContext = HttpClientContext.cast(context);
                             final String exchangeId = clientContext.getExchangeId();
-                            LOG.debug("{} Did not receive required challenge and GssConfig.ignoreMissingToken is false.",
+                            LOG.debug("{} Did not receive required challenge.",
                                 exchangeId);
                         }
                         state = State.FAILED;
                         throw new AuthenticationException(
-                                "Did not receive required challenge and GssConfig.ignoreMissingToken is false.");
+                                "Did not receive required challenge.");
                     }
                 }
-                queuedToken = generateToken(challengeToken, KERBEROS_SCHEME, gssHostname);
+                queuedToken = generateToken(challengeToken, PEER_SERVICE_NAME, gssHostname);
                 if (challenged) {
                     state = State.TOKEN_READY;
                 } else if (!gssContext.isEstablished()) {
@@ -237,16 +256,16 @@ public void processChallenge(
                             LOG.debug("{} GSSContext MutualAuthState is false, but continuing because GssConfig.requireMutualAuth is false.",
                                 exchangeId);
                         }
-                        state = State.FAILED;
+                        state = State.SUCCEEDED;
                     }
                 } else {
                     state = State.SUCCEEDED;
                 }
                 break;
             default:
+                final State prevState = state;
                 state = State.FAILED;
-                throw new IllegalStateException("Illegal state: " + state);
-
+                throw new IllegalStateException("Illegal state: " + prevState);
             }
         } catch (final GSSException gsse) {
             state = State.FAILED;
@@ -264,6 +283,10 @@ public void processChallenge(
             }
             // other error
             throw new AuthenticationException(gsse.getMessage(), gsse);
+        } finally {
+            if ((state == State.FAILED || state == State.SUCCEEDED) && gssContext != null) {
+                dispose();
+            }
         }
     }
 
@@ -336,9 +359,6 @@ public boolean isResponseReady(
     protected void setGssCredential(final CredentialsProvider credentialsProvider,
             final HttpHost host,
             final HttpContext context) {
-        if (this.gssCredential != null) {
-            return;
-        }
         final Credentials credentials =
                 credentialsProvider.getCredentials(new AuthScope(host, null, getName()), context);
         if (credentials instanceof org.apache.hc.client5.http.auth.gss.GssCredentials) {
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/gss/SpnegoScheme.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/gss/SpnegoScheme.java
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java

Original file line number	Diff line number	Diff line change
`@@ -63,6 +63,7 @@ public GSSCredential getGSSCredential() {`
`63`	`63`
`64`	`64`	`@Override`
`65`	`65`	`public Principal getUserPrincipal() {`
	`66`	`+ // TODO Can we extract this somehow ?`
`66`	`67`	`return null;`
`67`	`68`	`}`
`68`	`69`
Original file line number	Diff line number	Diff line change
`@@ -334,26 +334,25 @@ private boolean needAuthentication(`
`334`	`334`	`}`
`335`	`335`	`}`
`336`	`336`
	`337`	`+ boolean targetNeedsAuth = false;`
	`338`	`+ boolean proxyNeedsAuth = false;`
`337`	`339`	`if (targetAuthRequested \|\| targetMutualAuthRequired) {`
`338`		`- final boolean updated = authenticator.handleResponse(target, ChallengeType.TARGET, response,`
	`340`	`+ targetNeedsAuth = authenticator.handleResponse(target, ChallengeType.TARGET, response,`
`339`	`341`	`targetAuthStrategy, targetAuthExchange, context);`
`340`	`342`
`341`	`343`	`if (authCacheKeeper != null) {`
`342`	`344`	`authCacheKeeper.updateOnResponse(target, pathPrefix, targetAuthExchange, context);`
`343`	`345`	`}`
`344`		`-`
`345`		`- return updated;`
`346`	`346`	`}`
`347`	`347`	`if (proxyAuthRequested \|\| proxyMutualAuthRequired) {`
`348`		`- final boolean updated = authenticator.handleResponse(proxy, ChallengeType.PROXY, response,`
	`348`	`+ proxyNeedsAuth = authenticator.handleResponse(proxy, ChallengeType.PROXY, response,`
`349`	`349`	`proxyAuthStrategy, proxyAuthExchange, context);`
`350`	`350`
`351`	`351`	`if (authCacheKeeper != null) {`
`352`	`352`	`authCacheKeeper.updateOnResponse(proxy, null, proxyAuthExchange, context);`
`353`	`353`	`}`
`354`		`-`
`355`		`- return updated;`
`356`	`354`	`}`
	`355`	`+ return targetNeedsAuth \|\| proxyNeedsAuth;`
`357`	`356`	`}`
`358`	`357`	`return false;`
`359`	`358`	`}`
Original file line number	Diff line number	Diff line change
`@@ -363,7 +363,7 @@ public boolean handleResponse(`
`363`	`363`
`364`	`364`	`/**`
`365`	`365`	`* Generates a response to the authentication challenge based on the actual {@link AuthExchange} state`
`366`		`- * and adds it to the given {@link HttpRequest} message .`
	`366`	`+ * and adds it to the given {@link HttpRequest} message.`
`367`	`367`	`*`
`368`	`368`	`* @param host the hostname of the opposite endpoint.`
`369`	`369`	`* @param challengeType the challenge type (target or proxy).`