HTTPCLIENT-1625 Completely overhaul GSS-API-based authentication backend #577
Conversation
stoty
force-pushed
the
HTTPCLIENT-1625
branch
2 times, most recently
from
01354e2
to
cf04092
Compare
httpclient5/src/main/java/org/apache/hc/client5/http/auth/AuthScheme.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/auth/AuthSchemeV2.java
Outdated
Show resolved
Hide resolved
| private final Option stripPort; | ||
| private final Option useCanonicalHostname; | ||
| private final Option requestDelegCreds; | ||
| private final Option stripPort; //Effective default is ENABLE |
There was a problem hiding this 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.
There was a problem hiding this comment.
That's certainly possible, but that would introduce an incompatibility with the 5.2 behaviour.
| 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 |
There was a problem hiding this comment.
I would like to see a usecase why this should not be on by default without an option at all.
There was a problem hiding this comment.
All I can came up with is:
- GSS does not default to mutual auth
- Backwards compatibility
I would at least leave an option to disable this, so that there is an out for very broken cases.
Of course there is also a case for forcing users to clean up their potentially broken setup.
There was a problem hiding this comment.
I have looked at this, and the current API is really built around using the gss library default settings.
Since we are making incompatible changes anyway, if we want to set non-default gss options by default, (and keep them configurable), then we should remove the default options, and use simple booleans with explicit true/false defaults.
httpclient5/src/main/java/org/apache/hc/client5/http/auth/StandardAuthScheme.java
Show resolved
Hide resolved
| throw new AuthenticationException(gsse.getMessage(), gsse); | ||
| } | ||
| // other error | ||
| throw new AuthenticationException(gsse.getMessage(), gsse); |
There was a problem hiding this comment.
Won't those duplicate the message?
There was a problem hiding this comment.
Not really.
But the Auth. execption would have the same message as the cause.
We can write some new message for the Auth. exception, I just really hate when a library swallows the original exception instead of wrapping it.
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/GGSSchemeBase.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/GGSSchemeBase.java
Outdated
Show resolved
Hide resolved
| */ | ||
| @Deprecated |
| */ | ||
| @Deprecated | ||
| @Experimental | ||
| public class SPNegoScheme extends GGSSchemeBase { |
There was a problem hiding this comment.
Should also go in favor of SpnegoScheme
|
Thanks for the initial review @michael-o . Regarding the renames and changing the defaults, I have specifically tried to touch as little as possible because much of the GSS/Kerberos code was not removed from codebase when disabling SPNEGO, which I assumed was for brackwards compatibility reasons. I'm fine with your proposed changes, but those may theoretically cause issues with pre-5.3 apps, and will cause the api compare plugin to go wild. |
Will engage next week again. |
|
I believe I have addressed your comments which do not change the API. |
|
Can you take another look at the current patch @michael-o ? |
Will pick end of this week or next week. |
stoty
force-pushed
the
HTTPCLIENT-1625
branch
from
99bb75c
to
de8dd8d
Compare
|
Rebased to current master |
|
@ok2c @stoty I gave this another conceptual thought: We said that we are going to remove the current code for good, but this PR reuses it and makes it partially undeprecated. For me, this is not straight forward. My gut feeling says that we should remove completely first and the @stoty can readd the code he thinks is required to satisfy this PR. WDYT? |
|
@michael-o We cannot remove old implementation unless we are willing to go to the major release cycle and release these changes as 6.0. I suggest that the old classes are left deprecated as there might still be users that rely on them and new implementation is added in parallel (with different names or in a different package). |
I see, than we need new packages names. Agreed. |
stoty
force-pushed
the
HTTPCLIENT-1625
branch
from
de8dd8d
to
5b5564b
Compare
|
I have reverted the removals. I don't see what alternative package name we could use instead of "org.apache.hc.client5.http.auth" Maybe I could prefix the new classes with "Mutual" ? The other classes are strictly backwards compatible, like KerberosCredentials, and KerberosConfig. |
I'll get back to this question this week, have some serious trouble at $work I need to solve first. |
|
I don't think that removing deprecation is problem, especially as these classes were deprecated without providing a replacement, not because they were replaced by something. |
@stoty Un-deprecate those classes that can be kept fully backward compatible and create new classes for those that cannot |
|
rebased |
|
I have made the discussed changes. |
|
@stoty @michael-o Folks, what is the reason this PR is stuck? |
I am busy, can only peek once in a while. Hopefully the upcoming weeks will be better. |
stoty
force-pushed
the
HTTPCLIENT-1625
branch
from
e698321
to
6e1e009
Compare
and mark HttpAuthenticator @internal
|
@stoty If you want to expedite the review process we could do the following. Once you are more or less happy with the state of things, submit changes to |
@stoty There is still enough code in HC that goes back to version 3 and 2. I am fine with deprecating the present |
|
I don't think that duplicating HttpAuthenticator is a good idea. For the authentication shemes, a case could be made for duplicating them, as those are pluggable components. However, HttpAuthenticator is a purely internal class, and no sane external code is expected to call it. The only way I could think that HttpAuthenticator would be called by external code, is if someone fully re-implemented a new (Closable...)Client class (and the corresponding ...Exec class ), which is not something that I think should be covered by backwards compatibility. The end result would be replacing HttpAuthenticator with HttpAuthenticator2, and having a dangling HttpAuthenticator, which doesn't work with the other classes anyway (and it would possibly have to be modified anyway just to compile). |
|
@stoty Please disable |
|
Thank you. |
| * @throws AuthenticationException in case the authentication process is unsuccessful. | ||
| * @since 5.5 | ||
| */ | ||
| void processChallenge( |
There was a problem hiding this comment.
When processing a response with mutual a auth token, we don't send a new request, hence we must be able to signal the authentication failiure to the caller, and MalformedChallengeException is not semantically correct for that.
We also need additional information, the HttpContext, and the challenged flag to be able to verify the received mutual auth token.
The old method did not verify anything, it just stored the token, an deferred processing to the generateAuthResponse() method.
| @@ -151,11 +159,17 @@ public Builder setRequestDelegCreds(final Option requestDelegCreds) { | |||
| return this; | |||
| } | |||
|
|
|||
| public Builder setRequestMutualAuth(final Option requestMutualAuth) { | |||
There was a problem hiding this comment.
This the new option to enable mutual auth.
Michal suggested changing this API, which is certainly possible, especially now, that we have duplicated the Scheme objects, and require changes in callers anyway.
However, this configuration API is indenpendent of the deeper changes.
| @@ -95,7 +99,7 @@ public List<AuthScheme> select( | |||
| Collection<String> authPrefs = challengeType == ChallengeType.TARGET ? | |||
| config.getTargetPreferredAuthSchemes() : config.getProxyPreferredAuthSchemes(); | |||
| if (authPrefs == null) { | |||
| authPrefs = DEFAULT_SCHEME_PRIORITY; | |||
| authPrefs = getSchemePriority(); | |||
There was a problem hiding this comment.
This is a convenience change, to make it easier to re-add the SpnegoScheme without having to fully duplicate this class.
| @@ -515,10 +517,11 @@ private boolean needAuthentication( | |||
| final AuthExchange proxyAuthExchange, | |||
| final HttpHost proxy, | |||
| final HttpResponse response, | |||
| final HttpClientContext context) { | |||
| final HttpClientContext context) throws AuthenticationException, MalformedChallengeException { | |||
There was a problem hiding this comment.
This is corresponds to to the processChallenge() change in AuthScheme2, we need to be able to signal the authentication failure to the caller even when the server considers the authentication complete, and does not challenge us.
There was a problem hiding this comment.
I am bit hazy on Connection oriented Auth schemes, but I think that in reality, there is no valid combination where a connection oriented scheme is used with Spengo, so this is only about conforming to the interface change.
| * @return true if responses with non 401/407 response codes must be processed by the scheme. | ||
| * @since 5.5 | ||
| */ | ||
| boolean isChallengeExpected(); |
There was a problem hiding this comment.
This is the other half of the mutual auth handling.
If mutual auth is requested, then the server MUST send a challenge with the mutual auth token (which is not really a "challenge", as a response is not expected).
If this returns true, but the server does not send a challenge, then mutual authentication failed, and an Exception is thrown to the user.
| authExchange.reset(); | ||
| return false; | ||
| if (!isChallengeExpected) { | ||
| authExchange.reset(); |
There was a problem hiding this comment.
Previously, if there was no challenge, then we could return here indicating that the authentication process has finished.
However, if we have mutual auth enabled, but didn't get a challenge, then the authentication is not finished, and must continue processing.
| } | ||
|
|
||
| switch (authExchange.getState()) { | ||
| case FAILURE: | ||
| return false; | ||
| case SUCCESS: | ||
| authExchange.reset(); | ||
| break; | ||
| if (!isChallengeExpected) { |
There was a problem hiding this comment.
AuthChallenge.SUCCESS only indicates that the server hasn't sent a response.
If the server should have sent a response, then we must continue processing.
| if (authScheme != null) { | ||
| final String schemeName = authScheme.getName(); | ||
| final AuthChallenge challenge = challengeMap.get(schemeName.toLowerCase(Locale.ROOT)); | ||
| if (challenge != null) { | ||
| if (challenge != null || isChallengeExpected) { |
There was a problem hiding this comment.
Same old change, in mutual auth mode we must make sure to push down the non-existent challenge to AuthScheme2, which is going to throw an exception.
| authScheme.processChallenge(challenge, context); | ||
| } catch (final MalformedChallengeException ex) { | ||
| if (authScheme instanceof AuthScheme2) { | ||
| ((AuthScheme2)authScheme).processChallenge(host, challenge, context, challenged); |
| } | ||
| authExchange.reset(); | ||
| authExchange.setState(AuthExchange.State.FAILURE); | ||
| return false; | ||
| if (isChallengeExpected) { | ||
| throw ex; |
There was a problem hiding this comment.
Mutual auth case, throw exception, because the server has sent a 200 or equvalent code, and we will simply return the response to the client otherwise, even if the AuthExchange.STATE is Failure.
No description provided.