Skip to content

Commit 3b6359b

Browse files
stotystoty
committed
rewrite GSS established logic, and add new option for compatibility for non-compliant servers
1 parent fb3b9d2 commit 3b6359b

File tree

8 files changed

+82
-46
lines changed

8 files changed

+82
-46
lines changed

‎httpclient5-testing/src/test/java/org/apache/hc/client5/testing/compatibility/async/HttpAsyncClientCompatibilityTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ public HttpAsyncClientCompatibilityTest(
8787
secretPath = "/private_spnego/big-secret.txt";
8888
this.clientResource.configure(builder -> builder
8989
.setTargetAuthenticationStrategy(new SpnegoAuthenticationStrategy())
90-
.setDefaultAuthSchemeRegistry(SpnegoTestUtil.getSpnegoSchemeRegistry()));
90+
.setDefaultAuthSchemeRegistry(SpnegoTestUtil.getDefaultSpnegoSchemeRegistry()));
9191
}
9292
}
9393
if (proxy != null) {
@@ -100,7 +100,7 @@ public HttpAsyncClientCompatibilityTest(
100100
// but that's not a problem as SPNEGO cannot be proxied anyway.
101101
this.clientResource.configure(builder ->
102102
builder.setProxyAuthenticationStrategy(new SpnegoAuthenticationStrategy())
103-
.setDefaultAuthSchemeRegistry(SpnegoTestUtil.getSpnegoSchemeRegistryNoMutual()));
103+
.setDefaultAuthSchemeRegistry(SpnegoTestUtil.getLegacySpnegoSchemeRegistry()));
104104
}
105105
}
106106
}

‎httpclient5-testing/src/test/java/org/apache/hc/client5/testing/compatibility/spnego/SpnegoTestUtil.java‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,8 @@
4646
import javax.security.auth.login.LoginContext;
4747
import javax.security.auth.login.LoginException;
4848

49-
import org.apache.hc.client5.http.SystemDefaultDnsResolver;
5049
import org.apache.hc.client5.http.auth.AuthSchemeFactory;
5150
import org.apache.hc.client5.http.auth.StandardAuthScheme;
52-
import org.apache.hc.client5.http.auth.gss.GssConfig;
5351
import org.apache.hc.client5.http.auth.gss.GssCredentials;
5452
import org.apache.hc.client5.http.impl.auth.BasicSchemeFactory;
5553
import org.apache.hc.client5.http.impl.auth.BearerSchemeFactory;
@@ -65,11 +63,6 @@
6563

6664
public class SpnegoTestUtil {
6765

68-
static private final GssConfig NO_MUTUAL_KERBEROS_CONFIG =
69-
GssConfig.custom().setRequestMutualAuth(false).build();
70-
static private final SpnegoSchemeFactory NO_MUTUAL_SCHEME_FACTORY =
71-
new SpnegoSchemeFactory(NO_MUTUAL_KERBEROS_CONFIG, SystemDefaultDnsResolver.INSTANCE);
72-
7366
public static GssCredentials createCredentials(final Subject subject) {
7467
return SecurityUtils.callAs(subject, new Callable<GssCredentials>() {
7568
@Override
@@ -89,7 +82,7 @@ public static Path createKeytabDir() {
8982
}
9083
}
9184

92-
public static Registry<AuthSchemeFactory> getSpnegoSchemeRegistry() {
85+
public static Registry<AuthSchemeFactory> getDefaultSpnegoSchemeRegistry() {
9386
return RegistryBuilder.<AuthSchemeFactory>create()
9487
.register(StandardAuthScheme.BEARER, BearerSchemeFactory.INSTANCE)
9588
.register(StandardAuthScheme.BASIC, BasicSchemeFactory.INSTANCE)
@@ -100,12 +93,12 @@ public static Registry<AuthSchemeFactory> getSpnegoSchemeRegistry() {
10093
}
10194

10295
//Squid does not support mutual auth
103-
public static Registry<AuthSchemeFactory> getSpnegoSchemeRegistryNoMutual() {
96+
public static Registry<AuthSchemeFactory> getLegacySpnegoSchemeRegistry() {
10497
return RegistryBuilder.<AuthSchemeFactory>create()
10598
.register(StandardAuthScheme.BEARER, BearerSchemeFactory.INSTANCE)
10699
.register(StandardAuthScheme.BASIC, BasicSchemeFactory.INSTANCE)
107100
.register(StandardAuthScheme.DIGEST, DigestSchemeFactory.INSTANCE)
108-
.register(StandardAuthScheme.SPNEGO, NO_MUTUAL_SCHEME_FACTORY)
101+
.register(StandardAuthScheme.SPNEGO, SpnegoSchemeFactory.LEGACY)
109102
// register other schemes as needed
110103
.build();
111104
}

‎httpclient5-testing/src/test/java/org/apache/hc/client5/testing/compatibility/sync/HttpClientCompatibilityTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ public HttpClientCompatibilityTest(final HttpHost target, final Credentials targ
7272
secretPath = "/private_spnego/big-secret.txt";
7373
this.clientResource.configure(builder -> builder
7474
.setTargetAuthenticationStrategy(new SpnegoAuthenticationStrategy())
75-
.setDefaultAuthSchemeRegistry(SpnegoTestUtil.getSpnegoSchemeRegistry()));
75+
.setDefaultAuthSchemeRegistry(SpnegoTestUtil.getDefaultSpnegoSchemeRegistry()));
7676
}
7777
}
7878
if (proxy != null) {
@@ -85,7 +85,7 @@ public HttpClientCompatibilityTest(final HttpHost target, final Credentials targ
8585
// but that's not a problem as SPNEGO cannot be proxied anyway.
8686
this.clientResource.configure(builder ->
8787
builder.setProxyAuthenticationStrategy(new SpnegoAuthenticationStrategy())
88-
.setDefaultAuthSchemeRegistry(SpnegoTestUtil.getSpnegoSchemeRegistryNoMutual()));
88+
.setDefaultAuthSchemeRegistry(SpnegoTestUtil.getLegacySpnegoSchemeRegistry()));
8989
}
9090
}
9191
}

‎httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestSpnegoScheme.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -420,7 +420,7 @@ void testMutualFailureNoToken() throws Exception {
420420
Assertions.assertTrue(e.getCause() instanceof AuthenticationException);
421421
}
422422

423-
Mockito.verify(mockAuthScheme.context, Mockito.atLeastOnce()).isEstablished();
423+
Mockito.verify(mockAuthScheme.context, Mockito.never()).isEstablished();
424424
Mockito.verify(mockAuthScheme.context, Mockito.never()).getMutualAuthState();
425425
}
426426

‎httpclient5/src/main/java/org/apache/hc/client5/http/auth/gss/GssConfig.java‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,29 +48,34 @@ public class GssConfig implements Cloneable {
4848

4949

5050
public static final GssConfig DEFAULT = new Builder().build();
51+
public static final GssConfig LEGACY =
52+
new Builder().setIgnoreMissingToken(true).setRequestMutualAuth(false).build();
5153

5254
private final boolean addPort;
5355
private final boolean useCanonicalHostname;
5456
private final boolean requestMutualAuth;
5557
private final boolean requestDelegCreds;
58+
private final boolean ignoreMissingToken;
5659

5760
/**
5861
* Intended for CDI compatibility
5962
*/
6063
protected GssConfig() {
61-
this(false, false, true, false);
64+
this(false, false, true, false, false);
6265
}
6366

6467
GssConfig(
6568
final boolean addPort,
6669
final boolean useCanonicalHostname,
6770
final boolean requestMutualAuth,
68-
final boolean requestDelegCreds) {
71+
final boolean requestDelegCreds,
72+
final boolean ignoreMissingToken) {
6973
super();
7074
this.addPort = addPort;
7175
this.useCanonicalHostname = useCanonicalHostname;
7276
this.requestMutualAuth = requestMutualAuth;
7377
this.requestDelegCreds = requestDelegCreds;
78+
this.ignoreMissingToken = ignoreMissingToken;
7479
}
7580

7681
public boolean isAddPort() {
@@ -89,6 +94,10 @@ public boolean isRequestMutualAuth() {
8994
return requestMutualAuth;
9095
}
9196

97+
public boolean isIgnoreMissingToken() {
98+
return ignoreMissingToken;
99+
}
100+
92101
@Override
93102
protected GssConfig clone() throws CloneNotSupportedException {
94103
return (GssConfig) super.clone();
@@ -102,6 +111,7 @@ public String toString() {
102111
builder.append(", useCanonicalHostname=").append(useCanonicalHostname);
103112
builder.append(", requestDelegCreds=").append(requestDelegCreds);
104113
builder.append(", requestMutualAuth=").append(requestMutualAuth);
114+
builder.append(", ignoreMissingToken=").append(ignoreMissingToken);
105115
builder.append("]");
106116
return builder.toString();
107117
}
@@ -115,7 +125,8 @@ public static GssConfig.Builder copy(final GssConfig config) {
115125
.setAddPort(config.isAddPort())
116126
.setUseCanonicalHostname(config.isUseCanonicalHostname())
117127
.setRequestDelegCreds(config.isRequestDelegCreds())
118-
.setRequestMutualAuth(config.isRequestMutualAuth());
128+
.setRequestMutualAuth(config.isRequestMutualAuth())
129+
.setIgnoreMissingToken(config.isIgnoreMissingToken());
119130
}
120131

121132
public static class Builder {
@@ -124,6 +135,8 @@ public static class Builder {
124135
private boolean useCanonicalHostname = false;
125136
private boolean requestMutualAuth = true;
126137
private boolean requestDelegCreds = false;
138+
private boolean ignoreMissingToken = false;
139+
127140

128141
Builder() {
129142
super();
@@ -149,12 +162,21 @@ public Builder setRequestDelegCreds(final boolean requuestDelegCreds) {
149162
return this;
150163
}
151164

165+
public Builder setIgnoreMissingToken(final boolean ignoreMissingToken) {
166+
this.ignoreMissingToken = ignoreMissingToken;
167+
return this;
168+
}
169+
152170
public GssConfig build() {
171+
if (requestMutualAuth && ignoreMissingToken) {
172+
throw new IllegalArgumentException("If requestMutualAuth is set then ignoreMissingToken must not be set");
173+
}
153174
return new GssConfig(
154175
addPort,
155176
useCanonicalHostname,
156177
requestMutualAuth,
157-
requestDelegCreds
178+
requestDelegCreds,
179+
ignoreMissingToken
158180
);
159181
}
160182

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,7 @@ public boolean isChallenged(
121121
}
122122

123123
/**
124-
* Determines whether the given response represents an authentication challenge, without
125-
* changing the {@link AuthExchange} state.
124+
* Determines whether the response is 401/407 response depending to the challengeType
126125
*
127126
* @param challengeType the challenge type (target or proxy).
128127
* @param response the response message head.

‎httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/gss/GssSchemeBase.java‎

Lines changed: 44 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ enum State {
7777
}
7878

7979
private static final Logger LOG = LoggerFactory.getLogger(GssSchemeBase.class);
80-
private static final String NO_TOKEN = "";
8180
private static final String KERBEROS_SCHEME = "HTTP";
8281

8382
// The GSS spec does not specify how long the conversation can be. This should be plenty.
@@ -86,6 +85,7 @@ enum State {
8685
private final GssConfig config;
8786
private final DnsResolver dnsResolver;
8887
private final boolean mutualAuth;
88+
private final boolean ignoreMissingToken;
8989
private int challengesLeft = MAX_GSS_CHALLENGES;
9090

9191
/** Authentication process state */
@@ -100,6 +100,7 @@ enum State {
100100
this.config = config != null ? config : GssConfig.DEFAULT;
101101
this.dnsResolver = dnsResolver != null ? dnsResolver : SystemDefaultDnsResolver.INSTANCE;
102102
this.mutualAuth = config.isRequestMutualAuth();
103+
this.ignoreMissingToken = config.isIgnoreMissingToken();
103104
this.state = State.UNINITIATED;
104105
}
105106

@@ -167,10 +168,10 @@ public void processChallenge(
167168
LOG.debug("{} GSS init {}", exchangeId, gssHostname);
168169
}
169170
try {
170-
setGssCredential(HttpClientContext.cast(context).getCredentialsProvider(), host, context);
171-
queuedToken = generateToken(challengeToken, KERBEROS_SCHEME, gssHostname);
172171
switch (state) {
173172
case UNINITIATED:
173+
setGssCredential(HttpClientContext.cast(context).getCredentialsProvider(), host, context);
174+
queuedToken = generateToken(challengeToken, KERBEROS_SCHEME, gssHostname);
174175
if (challengeToken != null) {
175176
if (LOG.isDebugEnabled()) {
176177
final HttpClientContext clientContext = HttpClientContext.cast(context);
@@ -183,35 +184,54 @@ public void processChallenge(
183184
state = State.TOKEN_READY;
184185
break;
185186
case TOKEN_SENT:
186-
if (challenged) {
187-
state = State.TOKEN_READY;
188-
} else if (mutualAuth) {
189-
// We should have received a valid mutualAuth token
190-
if (!gssContext.isEstablished()) {
187+
if (challengeToken == null) {
188+
if (!challenged && ignoreMissingToken) {
189+
// Got a 200 without a challenge. Old non RFC compliant server.
191190
if (LOG.isDebugEnabled()) {
192-
final HttpClientContext clientContext =
193-
HttpClientContext.cast(context);
191+
final HttpClientContext clientContext = HttpClientContext.cast(context);
194192
final String exchangeId = clientContext.getExchangeId();
195-
LOG.debug("{} GSSContext is not established ", exchangeId);
193+
LOG.debug("{} GSS Context is not established, but continuing because GssConfig.ignoreMissingToken is true.", exchangeId);
196194
}
197-
state = State.FAILED;
198-
// TODO should we have specific exception(s) for these ?
199-
throw new AuthenticationException(
200-
"requireMutualAuth is set but GSSContext is not established");
201-
} else if (!gssContext.getMutualAuthState()) {
195+
state = State.SUCCEEDED;
196+
break;
197+
} else {
202198
if (LOG.isDebugEnabled()) {
203-
final HttpClientContext clientContext =
204-
HttpClientContext.cast(context);
199+
final HttpClientContext clientContext = HttpClientContext.cast(context);
205200
final String exchangeId = clientContext.getExchangeId();
206-
LOG.debug("{} requireMutualAuth is set but GSSAUthContext does not have"
207-
+ " mutualAuthState set", exchangeId);
201+
LOG.debug("{} Did not receive required challenge and GssConfig.ignoreMissingToken is false.",
202+
exchangeId);
208203
}
209204
state = State.FAILED;
210205
throw new AuthenticationException(
211-
"requireMutualAuth is set but GSSContext mutualAuthState is not set");
212-
} else {
213-
state = State.SUCCEEDED;
206+
"Did not receive required challenge and GssConfig.ignoreMissingToken is false.");
207+
}
208+
}
209+
queuedToken = generateToken(challengeToken, KERBEROS_SCHEME, gssHostname);
210+
if (challenged) {
211+
state = State.TOKEN_READY;
212+
} else if (!gssContext.isEstablished()) {
213+
if (LOG.isDebugEnabled()) {
214+
final HttpClientContext clientContext = HttpClientContext.cast(context);
215+
final String exchangeId = clientContext.getExchangeId();
216+
LOG.debug("{} GSSContext is not established ", exchangeId);
217+
}
218+
state = State.FAILED;
219+
// TODO should we have specific exception(s) for these ?
220+
throw new AuthenticationException(
221+
"requireMutualAuth is set but GSSContext is not established");
222+
} else if (mutualAuth && !gssContext.getMutualAuthState()) {
223+
if (LOG.isDebugEnabled()) {
224+
final HttpClientContext clientContext = HttpClientContext.cast(context);
225+
final String exchangeId = clientContext.getExchangeId();
226+
LOG.debug("{} requireMutualAuth is set but GSSAUthContext does not have"
227+
+ " mutualAuthState set",
228+
exchangeId);
214229
}
230+
state = State.FAILED;
231+
throw new AuthenticationException(
232+
"requireMutualAuth is set but GSSContext mutualAuthState is not set");
233+
} else {
234+
state = State.SUCCEEDED;
215235
}
216236
break;
217237
default:
@@ -289,7 +309,7 @@ public boolean isChallengeComplete() {
289309

290310
@Override
291311
public boolean isChallengeExpected() {
292-
return state == State.TOKEN_SENT && mutualAuth;
312+
return state == State.TOKEN_SENT;
293313
}
294314

295315
@Override
@@ -301,7 +321,6 @@ public boolean isResponseReady(
301321
Args.notNull(host, "Auth host");
302322
Args.notNull(credentialsProvider, "CredentialsProvider");
303323

304-
setGssCredential(credentialsProvider, host, context);
305324
return true;
306325
}
307326

‎httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/gss/SpnegoSchemeFactory.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,9 @@ public class SpnegoSchemeFactory implements AuthSchemeFactory {
5656
public static final SpnegoSchemeFactory DEFAULT = new SpnegoSchemeFactory(org.apache.hc.client5.http.auth.gss.GssConfig.DEFAULT,
5757
SystemDefaultDnsResolver.INSTANCE);
5858

59+
public static final SpnegoSchemeFactory LEGACY = new SpnegoSchemeFactory(org.apache.hc.client5.http.auth.gss.GssConfig.LEGACY,
60+
SystemDefaultDnsResolver.INSTANCE);
61+
5962
private final org.apache.hc.client5.http.auth.gss.GssConfig config;
6063
private final DnsResolver dnsResolver;
6164

0 commit comments

Comments
 (0)