michael-o commented on code in PR #577:
URL:
https://github.com/apache/httpcomponents-client/pull/577#discussion_r1746998248
##########
httpclient5/src/main/java/org/apache/hc/client5/http/auth/AuthScheme.java:
##########
@@ -86,6 +86,10 @@
* containing the terminal authorization response, the scheme is considered
unsuccessful
* and in FAILED state.
* </p>
+ * <p>
+ * This interface cannot correctly handle some authentication methods, like
SPENGO.
Review Comment:
Typo: SPNEGO
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/GGSSchemeBase.java:
##########
@@ -60,69 +60,167 @@
*
* @since 4.2
*
- * @deprecated Do not use. The GGS based experimental authentication schemes
are no longer
- * supported. Consider using Basic or Bearer authentication with TLS instead.
*/
-@Deprecated
-public abstract class GGSSchemeBase implements AuthScheme {
+// FIXME The class name looks like a Typo. Rename in 6.0 ?
+public abstract class GGSSchemeBase implements AuthSchemeV2 {
Review Comment:
It is a typo, I reported this. We can safely rename it.
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/SPNegoScheme.java:
##########
@@ -41,14 +41,7 @@
* </p>
*
* @since 4.2
- *
- * @deprecated Do not use. The GGS based experimental authentication schemes
are no longer
- * supported. Consider using Basic or Bearer authentication with TLS instead.
- *
- * @see BasicScheme
- * @see BearerScheme
*/
-@Deprecated
@Experimental
public class SPNegoScheme extends GGSSchemeBase {
Review Comment:
Should also go in favor of `SpnegoScheme`
##########
httpclient5/src/main/java/org/apache/hc/client5/http/auth/KerberosConfig.java:
##########
@@ -53,25 +49,28 @@ public enum Option {
public static final KerberosConfig DEFAULT = new Builder().build();
- private final Option stripPort;
- private final Option useCanonicalHostname;
- private final Option requestDelegCreds;
+ private final Option stripPort; //Effective default is ENABLE
+ private final Option useCanonicalHostname; //Effective default is ENABLE
+ private final Option requestDelegCreds; //Effective default is DISABLE
+ private final Option requestMutualAuth; //Effective default is DISABLE
Review Comment:
I would like to see a usecase why this should not be on by default without
an option at all.
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultAuthenticationStrategy.java:
##########
@@ -68,6 +68,8 @@ public class DefaultAuthenticationStrategy implements
AuthenticationStrategy {
private static final List<String> DEFAULT_SCHEME_PRIORITY =
Collections.unmodifiableList(Arrays.asList(
+ StandardAuthScheme.SPNEGO,
Review Comment:
I would not even enable it by default because JGSS is so crappy that it
takes forever to fails if not creds are there.
##########
httpclient5/src/main/java/org/apache/hc/client5/http/auth/AuthSchemeV2.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * ====================================================================
+ * 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.auth;
+
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.protocol.HttpContext;
+
+/**
+ * This is an improved version of the {@link AuthScheme} interface, amended to
be able to handle
+ * a conversation involving multiple challenge-response transactions and
adding the ability to check
+ * the results of a final token sent together with the successful HTTP request
as required by
+ * RFC 4559 and RFC 7546.
+ *
+ * @since 5.5
+ */
+public interface AuthSchemeV2 extends AuthScheme {
Review Comment:
I think it should be just `AuthScheme2` or `AuthSchemeEx`
##########
httpclient5/src/main/java/org/apache/hc/client5/http/auth/KerberosConfig.java:
##########
@@ -53,25 +49,28 @@ public enum Option {
public static final KerberosConfig DEFAULT = new Builder().build();
- private final Option stripPort;
- private final Option useCanonicalHostname;
- private final Option requestDelegCreds;
+ private final Option stripPort; //Effective default is ENABLE
Review Comment:
I would invert this. For the past 15 years, I haven't seen a single
deployment using the port with `HTTP` service class, especially because that
major clients do not even support this.
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/KerberosScheme.java:
##########
@@ -40,14 +40,7 @@
* </p>
*
* @since 4.2
- *
- * @deprecated Do not use. The GGS based experimental authentication schemes
are no longer
- * supported. Consider using Basic or Bearer authentication with TLS instead.
- *
- * @see BasicScheme
- * @see BearerScheme
*/
-@Deprecated
Review Comment:
Will go
##########
httpclient5/src/main/java/org/apache/hc/client5/http/auth/StandardAuthScheme.java:
##########
@@ -66,20 +66,12 @@ private StandardAuthScheme() {
/**
* SPNEGO authentication scheme as defined in RFC 4559 and RFC 4178.
- *
- * @deprecated Do not use. The GGS based experimental authentication
schemes are no longer
- * supported. Consider using Basic or Bearer authentication with TLS
instead.
*/
- @Deprecated
public static final String SPNEGO = "Negotiate";
/**
* Kerberos authentication scheme as defined in RFC 4120.
- *
- * @deprecated Do not use. The GGS based experimental authentication
schemes are no longer
- * supported. Consider using Basic or Bearer authentication with TLS
instead.
*/
- @Deprecated
public static final String KERBEROS = "Kerberos";
Review Comment:
No, this can be dropped. There is no RFC for Kerberos over HTTP. It has
never been standardized.
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/GGSSchemeBase.java:
##########
@@ -138,7 +236,9 @@ protected byte[] generateGSSToken(
final GSSManager manager = getManager();
final GSSName serverName = manager.createName(serviceName + "@" +
authServer, GSSName.NT_HOSTBASED_SERVICE);
Review Comment:
Well, that is not a server name, but a SPN...
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/GGSSchemeBase.java:
##########
@@ -138,7 +236,9 @@ protected byte[] generateGSSToken(
final GSSManager manager = getManager();
final GSSName serverName = manager.createName(serviceName + "@" +
authServer, GSSName.NT_HOSTBASED_SERVICE);
Review Comment:
I must admit that `authServer` is a horrible variable name.
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/GGSSchemeBase.java:
##########
@@ -60,69 +60,167 @@
*
* @since 4.2
*
- * @deprecated Do not use. The GGS based experimental authentication schemes
are no longer
- * supported. Consider using Basic or Bearer authentication with TLS instead.
*/
-@Deprecated
-public abstract class GGSSchemeBase implements AuthScheme {
+// FIXME The class name looks like a Typo. Rename in 6.0 ?
+public abstract class GGSSchemeBase implements AuthSchemeV2 {
enum State {
UNINITIATED,
- CHALLENGE_RECEIVED,
- TOKEN_GENERATED,
+ TOKEN_READY,
+ TOKEN_SENT,
+ SUCCEEDED,
FAILED,
}
private static final Logger LOG =
LoggerFactory.getLogger(GGSSchemeBase.class);
private static final String NO_TOKEN = "";
private static final String KERBEROS_SCHEME = "HTTP";
- private final org.apache.hc.client5.http.auth.KerberosConfig config;
+
+ // 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.
+ private static final int MAX_GSS_CHALLENGES = 3;
Review Comment:
Why? I don't see a need and have never seen such a boundary. The context is
complete when it is complete. In the past 15 years I haven't seen more than one
roundtrip for SPNEGO. This can only happen when IAKerb is in place...
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/GGSSchemeBase.java:
##########
@@ -60,69 +60,167 @@
*
* @since 4.2
*
- * @deprecated Do not use. The GGS based experimental authentication schemes
are no longer
- * supported. Consider using Basic or Bearer authentication with TLS instead.
*/
-@Deprecated
-public abstract class GGSSchemeBase implements AuthScheme {
+// FIXME The class name looks like a Typo. Rename in 6.0 ?
+public abstract class GGSSchemeBase implements AuthSchemeV2 {
enum State {
UNINITIATED,
- CHALLENGE_RECEIVED,
- TOKEN_GENERATED,
+ TOKEN_READY,
+ TOKEN_SENT,
+ SUCCEEDED,
FAILED,
}
private static final Logger LOG =
LoggerFactory.getLogger(GGSSchemeBase.class);
private static final String NO_TOKEN = "";
private static final String KERBEROS_SCHEME = "HTTP";
- private final org.apache.hc.client5.http.auth.KerberosConfig config;
+
+ // 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.
+ private static final int MAX_GSS_CHALLENGES = 3;
+ private final KerberosConfig config;
private final DnsResolver dnsResolver;
+ private final boolean mutualAuth;
+ private int challengesLeft = MAX_GSS_CHALLENGES;
/** Authentication process state */
private State state;
private GSSCredential gssCredential;
+ private GSSContext gssContext;
private String challenge;
- private byte[] token;
+ private byte[] queuedToken = new byte[0];
- GGSSchemeBase(final org.apache.hc.client5.http.auth.KerberosConfig config,
final DnsResolver dnsResolver) {
+ GGSSchemeBase(final KerberosConfig config, final DnsResolver dnsResolver) {
super();
- this.config = config != null ? config :
org.apache.hc.client5.http.auth.KerberosConfig.DEFAULT;
+ this.config = config != null ? config : KerberosConfig.DEFAULT;
this.dnsResolver = dnsResolver != null ? dnsResolver :
SystemDefaultDnsResolver.INSTANCE;
+ this.mutualAuth = config.getRequestMutualAuth() ==
KerberosConfig.Option.ENABLE;
this.state = State.UNINITIATED;
}
- GGSSchemeBase(final org.apache.hc.client5.http.auth.KerberosConfig config)
{
+ GGSSchemeBase(final KerberosConfig config) {
this(config, SystemDefaultDnsResolver.INSTANCE);
}
GGSSchemeBase() {
- this(org.apache.hc.client5.http.auth.KerberosConfig.DEFAULT,
SystemDefaultDnsResolver.INSTANCE);
+ this(KerberosConfig.DEFAULT, SystemDefaultDnsResolver.INSTANCE);
}
@Override
public String getRealm() {
return null;
}
+ // The AuthScheme API maps awkwardly to GSSAPI, where proccessChallange
and generateAuthResponse
+ // map to the same single method call. Hence the generated token is only
stored in this method.
@Override
public void processChallenge(
+ final HttpHost host,
final AuthChallenge authChallenge,
- final HttpContext context) throws MalformedChallengeException {
- Args.notNull(authChallenge, "AuthChallenge");
-
- this.challenge = authChallenge.getValue() != null ?
authChallenge.getValue() : NO_TOKEN;
+ final HttpContext context,
+ final boolean challenged) throws AuthenticationException {
- if (state == State.UNINITIATED) {
- token = Base64.decodeBase64(challenge.getBytes());
- state = State.CHALLENGE_RECEIVED;
- } else {
+ if (challengesLeft-- <= 0 ) {
if (LOG.isDebugEnabled()) {
final HttpClientContext clientContext =
HttpClientContext.cast(context);
final String exchangeId = clientContext.getExchangeId();
- LOG.debug("{} Authentication already attempted", exchangeId);
+ LOG.debug("{} 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 String authServer;
+ String hostname = host.getHostName();
+ if (config.getUseCanonicalHostname() != KerberosConfig.Option.DISABLE){
+ try {
+ hostname =
dnsResolver.resolveCanonicalHostname(host.getHostName());
+ } catch (final UnknownHostException ignore){
+ }
+ }
+ if (config.getStripPort() != KerberosConfig.Option.DISABLE) {
+ authServer = hostname;
+ } else {
+ authServer = hostname + ":" + host.getPort();
+ }
+
+ if (LOG.isDebugEnabled()) {
+ final HttpClientContext clientContext =
HttpClientContext.adapt(context);
+ final String exchangeId = clientContext.getExchangeId();
+ LOG.debug("{} GSS init {}", exchangeId, authServer);
+ }
+ try {
+ queuedToken = generateToken(challengeToken, KERBEROS_SCHEME,
authServer);
+ switch (state) {
+ case UNINITIATED:
+ if (challenge != NO_TOKEN) {
+ if (LOG.isDebugEnabled()) {
+ final HttpClientContext clientContext =
HttpClientContext.adapt(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.TOKEN_READY;
+ break;
+ case TOKEN_SENT:
+ if (challenged) {
+ state = State.TOKEN_READY;
+ } else if (mutualAuth){
+ // We should have received a valid mutualAuth token
+ if (!gssContext.isEstablished()) {
+ if (LOG.isDebugEnabled()) {
+ final HttpClientContext clientContext =
+ HttpClientContext.adapt(context);
+ final String exchangeId =
clientContext.getExchangeId();
+ LOG.debug("{} GSSContext is not established ",
exchangeId);
+ }
+ state = State.FAILED;
+ // TODO should we have specific exception(s) for these
?
+ throw new AuthenticationException(
+ "requireMutualAuth is set but GSSContext is
not established");
+ } else if (!gssContext.getMutualAuthState()) {
+ if (LOG.isDebugEnabled()) {
+ final HttpClientContext clientContext =
+ HttpClientContext.adapt(context);
+ final String exchangeId =
clientContext.getExchangeId();
+ LOG.debug("{} requireMutualAuth is set but
GSSAUthContext does not have"
+ + " mutualAuthState set", exchangeId);
+ }
+ state = State.FAILED;
+ throw new AuthenticationException(
+ "requireMutualAuth is set but GSSContext
mutualAuthState is not set");
+ } else {
+ state = State.SUCCEEDED;
+ }
+ }
+ break;
+ default:
+ state = State.FAILED;
+ throw new IllegalStateException("Illegal state: " + state);
+
+ }
+ } catch (final GSSException gsse) {
+ state = State.FAILED;
+ if (gsse.getMajor() == GSSException.DEFECTIVE_CREDENTIAL
+ || gsse.getMajor() == GSSException.CREDENTIALS_EXPIRED) {
+ throw new InvalidCredentialsException(gsse.getMessage(), gsse);
+ }
+ if (gsse.getMajor() == GSSException.NO_CRED) {
+ throw new InvalidCredentialsException(gsse.getMessage(), gsse);
+ }
+ if (gsse.getMajor() == GSSException.DEFECTIVE_TOKEN
+ || gsse.getMajor() == GSSException.DUPLICATE_TOKEN
+ || gsse.getMajor() == GSSException.OLD_TOKEN) {
+ throw new AuthenticationException(gsse.getMessage(), gsse);
+ }
+ // other error
+ throw new AuthenticationException(gsse.getMessage(), gsse);
Review Comment:
Won't those duplicate the message?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org
For additional commands, e-mail: dev-h...@hc.apache.org
