-
Notifications
You must be signed in to change notification settings - Fork 975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTTPCLIENT-2358 Implement a mutual authentication capable SPNEGO scheme #615
base: master
Are you sure you want to change the base?
Conversation
@stoty I cannot contribute here much. My only wish, and I understand it is a big ask, to also create a compatibility test similar to those we have for BASIC / DIGEST with Squid and Apache HTTPD |
I was afraid you're going to say that... There are two ways to go about that:
I don't know which of the two is more work, but I know where to copy the test setup from for the second one. Do you have a preference ? |
@stoty Kek. Anyways, We already test for compatibility with Jetty in core by running it in a Docker container I t would be my preferred option but I do not know how difficult it is to pack extra Jetty dependencies into a Docker container. I was also hoping Apache HTTPD or Ngnix might have Kerberos support. If it is too much effort, disregard my request. |
It is a reasonable request, and it IS helpful to catch any regressions, etc. |
Apache Kerby implements Kerberos, you can get inspiration (copy) how they do it: https://directory.apache.org/kerby/ |
Kerby is great for in-JVM tests, but for dockerized tests it's probably easier to use MIT kerberos. |
After poking around a bit, it seems that neither Apache Httpd, not Nginx supports SPNEGO out of the box. For now, I plan to make a local test with Kerby + Jetty, without Docker. |
A full coverage test contains JGSS (via Tomcat my SpnegoAuthenticator), MIT Kerberos (via mod_auth_gssapi), Microsoft Kerberos (via IIS (SSPI)). Virtually impossible to automate. I will do manual testing anyway. I have everything in place at work to test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much less code review. Thanks. I already see some points.
httpclient5/src/main/java/org/apache/hc/client5/http/auth/MutualKerberosConfig.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/auth/MutualKerberosConfig.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/auth/MutualKerberosConfig.java
Outdated
Show resolved
Hide resolved
Thanks. While I don't use Tomcat, testing Jgss with Jetty is doable, in fact one of the ways I tested this was via the SPNEGO test in HttpCLient->Calcite Avatica->Phoenix PQS. |
be6688d
to
ce46c45
Compare
2d87e73
to
2d7dc5e
Compare
Fix GSSCredential handling
2d7dc5e
to
5a11bce
Compare
I have added tests for direct connection to httpd + mod_auth_spnego, @michael-o . The async client cannot use the Subject set with Subject.callAs to figure out the Kerberos credentials (probably due to the auth being run from a different thread) Async still works if we create and set KerbersCredentials. (as in the new tests I added) |
I have added tests for SPNEGO authentication for Squid. I feel that this is ready now, @michael-o . |
Which bug exactly? |
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/MutualGssSchemeBase.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am picking this up again and want to complete next month.
Structural issues:
- Since in both
auth
andimpl.path
packages contains multiple new classes there should be in respective new subpackagesgss
for tidiness. - Don't use the term "Kerberos" in classnames becausee we perform SPNEGO with GSS-API, correctly it should be called "Gss"
- SPNEGO in class names should be written consistently, In PascalCase and not mixed case.
- Drop the prefix "Mutual" in class names altogether because one can disable on request making it a contradiction and the other party could pontentially deny it.
I can do that, but that would mean the the three formerly mutual classes only differ in case and package from the old deprecated classes. I can see that causing a lot of gried for users down the line.
Sure.
Sure.
Only the old deprecated classes use SPNego, and fixing those would make it even easier to mix them up with the new ones. (apart from breaking backwards compatibility, if we care about that)
I can do that, but then then the only difference between the old and new classes will be the packages and the case. |
Thanks for the review. I have also left KerberosCredentials alone, because it is used by both the new and old code. |
Don't rename old ones, only new ones. If old ones are reused maye it would be better to copy them? |
Yes, please. Don't break old deprecated, but add new classes for the new impl. |
I wasn't clear. The classes are different, but their names are almost identical. As the Scheme is not enabled by default, users have to add some boilerplate code to enable SPNEGO at all. i have renamed KerberCredentials, and added a backwards compatibility subclass. |
httpclient5/src/main/java/org/apache/hc/client5/http/auth/KerberosCredentials.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/auth/gss/GssConfig.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/auth/KerberosConfig.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/auth/gss/GssConfig.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/auth/gss/GssConfig.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/gss/GssSchemeBase.java
Outdated
Show resolved
Hide resolved
case TOKEN_SENT: | ||
if (challenged) { | ||
state = State.TOKEN_READY; | ||
} else if (mutualAuth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong. The only loop condition is gssContext.isEstablished()
. The acceptor can send still a token, even if not mutual auth is peformed and even in an error condition an acceptor context can produce a token (e.g. MIT Kerberos), if necessary I can show code. HAve seen this years ago.
Note that requesting mutual auth is only a hint to GSS-API, wether the security context will provide is upto the mech.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should we handle the case when we request mutual auth, but don't get it ?
Should we just fail, or should we split the option , one to request it, and one to require that it succeeds ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read https://datatracker.ietf.org/doc/html/rfc7546, there is even code depicting how you handle the loop. It should answer almost all questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec kind of leaves this to the implementation:
If a flag
was requested but is not available and that feature is necessary for
the application protocol, the initiator must destroy the security
context and not use the security context for application traffic.
If I interpret that losely, this means that we could split the mutualAuth, because the caller may ask for mutual Auth, but may not consider it necessary (though admittedly this not make a lot of sense).
If the initiator is expecting a context token (that is, the previous
call to GSS_Init_sec_context() returned GSS_S_CONTINUE_NEEDED) but
does not receive such a token within a reasonable amount of time
after transmitting the previous output_token to the acceptor, the
initiator should assume that the acceptor's state is invalid and fail
the GSS negotiation. Again, it may be appropriate for the initiator
to report this error condition to the acceptor via the application's
communication channel.
This one is even more interesting, as if we implemented this strictly, we could never work with Squid (and I expect that that there are many more similarly broken servers.)
I lean towards not making mutual authentication optional, as the user has no way to check the status, and if it is optional, then why even bother in the first place ? It would catch misconfigured/malicious servers that send an incorrect response, but not ones that just plain ignore the request.
On the other hand, I lean towards adding a strict flag to the config, so that we require that the GSS negotiation completes, so that we have a standards-compliant mode. This of course is implied when mutual Auth is requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will read this one as well, check other impls and share my thoughts on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the current code should comply with that.
"ignoreMissingToken" is the non-strict flag (defaults to off), but I am open to a better name.
ignoreIncompleteSecurityContext
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about the warning, that can be a really high volume, and the user has already disabled it explicitly.
I am currently logging a DEBUG message for this.
Lets go for WARNING and see whether we will receive complaints, then we still can reduce. I'd eager to see who is faulty, this will give everyone a chance to report to the producer of the crap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old scheme had also other issues beside the mutual auth.
So you are saying, if we request mutual auth we shall check for it after the context has been established and if not then we should fail? Including a switch to make it soft?
Yes. currently these are requestMutualAuth and requireMutualAuth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about the warning, that can be a really high volume, and the user has already disabled it explicitly.
I am currently logging a DEBUG message for this.
Lets go for WARNING and see whether we will receive complaints, then we still can reduce. I'd eager to see who is faulty, this will give everyone a chance to report to the producer of the crap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR: I checked the source of Squid, man...
Look at https://github.com/squid-cache/squid/blob/0c90595b9d63930f07313a60eb3c7ec9859251bf/src/auth/negotiate/kerberos/negotiate_kerberos_auth.cc#L750-L754, completion is never really checked...
I also failed to find where the output token goes, it is simply not processed after it has been converted to Base 64, moreover they use a lot of raw Kerberos 5 APIs which aren't required at all, one can do everything through GSS-API these days, even obtaining the PAC data. For me: Squid is broken.
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/gss/GssSchemeBase.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/gss/SpnegoScheme.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public boolean isConnectionBased() { | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong and false. If there is only one roundtrip then you don't need to bind the GSS context to the connection otherwise you have to, see also https://github.com/gssapi/mod_auth_gssapi?tab=readme-ov-file#gssapiconnectionbound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, fixing.
Note that this was copied from the old one.
Should we fix it there as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, leave as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I misread. We should fix the copy, not the original one. My bad!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this again, This is not a problem when the completion done after one roundtrip. So I guess when the security context says continue needed than this needs to be bound to the connection. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I'm a bit hazy on what connectionBased does.
Looking at the main auth loop in ProtocolExec.execute(), It looks like AuthScheme (and with it GssContext) is bound to the whole authentication conversation until it either fails or succeeds, regardless of this setting.
Best I can tell, isConnectionBased is exclusively used for NTLM, where the server seems to bind the user authentication to the HTTP connection.
AFAIU, NTLM is the only auth that does this.
For Kerberos, we need to do full authentication for every single request (that's why it's so insanely slow and expensive, and that's why an AUTH cookie is usually created for the session after successfully authenticating).
Based on this, I think that the right setting is false, just like for every other method, excpet NTLM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Setting it to false causes some dockerized Proxy tests to fail.
I'm going to need to more time to investige.
I have fixed the small issues. |
…or non-compliant servers
I have added a new config option for non-standard servers, and rewritten the established checks. Please take another look @michael-o . |
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/gss/GssSchemeBase.java
Outdated
Show resolved
Hide resolved
Update some debug/exception messages
I have split the mutual auth config option to request and require. I can drop the request one, and reqest unconditionally if you prefer. |
I opened https://bugs.squid-cache.org/show_bug.cgi?id=5485 for the Squid issues. |
Thanks, very good. mod_auth_gssapi is the gold standard for me written by very knowledgeable people. |
import org.apache.hc.core5.annotation.ThreadingBehavior; | ||
|
||
/** | ||
* Immutable class encapsulating GSS configuration options for the new mutual auth capable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...for the new {@link SpnegoScheme}.
|
||
@Override | ||
public Principal getUserPrincipal() { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a TODO than the principal can be obtained from the GSSCredential
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
|
||
private static final Logger LOG = LoggerFactory.getLogger(GssSchemeBase.class); | ||
private static final String NO_TOKEN = ""; | ||
private static final String KERBEROS_SCHEME = "HTTP"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still open
|
||
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still open
try { | ||
hostname = dnsResolver.resolveCanonicalHostname(host.getHostName()); | ||
} catch (final UnknownHostException ignore) { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still open...
* SPNEGO (Simple and Protected GSSAPI Negotiation Mechanism) authentication | ||
* scheme. | ||
* <p> | ||
* This is the new mutual authentication capable Scheme which replaces the old deprecated non mutual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is more than enough to mention that this superseded the other class, not necessary to mention its capabilities here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
@Override | ||
public boolean isConnectionBased() { | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this again, This is not a problem when the completion done after one roundtrip. So I guess when the security context says continue needed than this needs to be bound to the connection. WDYT?
if (LOG.isDebugEnabled()) { | ||
final HttpClientContext clientContext = HttpClientContext.cast(context); | ||
final String exchangeId = clientContext.getExchangeId(); | ||
LOG.debug("{} GSS error: too many challenges received. Infinite loop ?", exchangeId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a good question. I have the feeling that this should be at warning lever since an upper layer exception cannot reflect this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
return; | ||
} | ||
|
||
final byte[] challengeToken = Base64.decodeBase64(authChallenge == null ? null : authChallenge.getValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why bother Base64 if we can do the ternary check outside?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean.
The ternary here prevents an NPE from authChallenge.getValue() is authChallange is null.
Base64.decodeBase64 returns null for null, so we don't have to check for a null value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authChallenge == null ? null : Base64.decodeBase64(authChallenge.getValue());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
case UNINITIATED: | ||
setGssCredential(HttpClientContext.cast(context).getCredentialsProvider(), host, context); | ||
queuedToken = generateToken(challengeToken, KERBEROS_SCHEME, gssHostname); | ||
if (challengeToken != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check should be done before the generateToken()
method, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't really matter, generateToken() does not change challengeToken, and this only gates some logging.
I will switch them around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense in a way because you avoid the object creation in the method.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, this violates, as you write, the RFC and shall fail. GSS-API is a client first approach. See Cyrus SASL:
Plugin "gs2" [loaded], API version: 4
SASL mechanism: GS2-IAKERB, best SSF: 0
security flags: NO_ANONYMOUS|NO_PLAINTEXT|NO_ACTIVE|PASS_CREDENTIALS|MUTUAL_AUTH
features: WANT_CLIENT_FIRST|NEED_SERVER_FQDN|GSS_FRAMING|CHANNEL_BINDING
Plugin "gs2" [loaded], API version: 4
SASL mechanism: GS2-KRB5, best SSF: 0
security flags: NO_ANONYMOUS|NO_PLAINTEXT|NO_ACTIVE|PASS_CREDENTIALS|MUTUAL_AUTH
features: WANT_CLIENT_FIRST|NEED_SERVER_FQDN|GSS_FRAMING|CHANNEL_BINDING
Plugin "gssapiv2" [loaded], API version: 4
SASL mechanism: GSS-SPNEGO, best SSF: 256
security flags: NO_ANONYMOUS|NO_PLAINTEXT|NO_ACTIVE|PASS_CREDENTIALS|MUTUAL_AUTH
features: WANT_CLIENT_FIRST|PROXY_AUTHENTICATION|NEED_SERVER_FQDN|SUPPORTS_HTTP
Plugin "gssapiv2" [loaded], API version: 4
SASL mechanism: GSSAPI, best SSF: 256
security flags: NO_ANONYMOUS|NO_PLAINTEXT|NO_ACTIVE|PASS_CREDENTIALS|MUTUAL_AUTH
features: WANT_CLIENT_FIRST|PROXY_AUTHENTICATION|NEED_SERVER_FQDN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
Will need to change test for this.
if (LOG.isDebugEnabled()) { | ||
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning, not debug as agreed.
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.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd drop the part from "...and". If not set explicitly, no need to mention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
} | ||
state = State.FAILED; | ||
throw new AuthenticationException( | ||
"Did not receive required challenge and GssConfig.ignoreMissingToken is false."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
state = State.FAILED; | ||
// TODO should we have specific exception(s) for these ? | ||
throw new AuthenticationException( | ||
"GSSContext is not established."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You haven't disposed the security context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a finally block for that.
LOG.debug("{} GSSContext MutualAuthState is false, but continuing because GssConfig.requireMutualAuth is false.", | ||
exchangeId); | ||
} | ||
state = State.FAILED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not fully understand this. If mutual auth isn't required, why fail if it is not available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug, I'm fixing it. (We don't have a test case for this path)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
I realize that I somehow didn't upload the changes for GSSSchemeBase in my last pach, that's why a lot of your earlier comments were not addressed. |
set isConnectionBased() to false fix logic in needsAuthentication()
I hope I got all the fixes we discussed, @michael-o . Based on my digging, I have set isConnectionBased() to false. I have also fixed the logic in the needAuthentication() to handle proxy + target authentication correctly. |
Were you able to check my latest patch @michael-o ? |
No description provided.