-
Notifications
You must be signed in to change notification settings - Fork 974
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-2356 Extend AuthScheme API and Authentication Logic to Enable SPNEGO Mutual Authentication #612
Conversation
and mark HttpAuthenticator @internal
The japicmp disable is not meant to be commited. it's for letting the test suite run. |
final HttpClientContext clientContext = HttpClientContext.cast(context); | ||
final String exchangeId = clientContext.getExchangeId(); | ||
// The mutual auth may still fail | ||
LOG.debug("{} Server has accepted authorization", 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.
authentication?
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.
Technically, the client sends WWW-Authenticate headers, but the client sends Authorization headers.
At this point, the server has authenticated AND authorized the client, so I think that the message is correct.
But we can of course change the text.
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 know, but then it is inconsistent with other log messages.
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/HttpAuthenticator.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/HttpAuthenticator.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/HttpAuthenticator.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java
Outdated
Show resolved
Hide resolved
Thanks for the review @michael-o . I have fixed the comments. Regarding rfc7615 It looks interesting, and makes more sense than sending the WWW-Authenticate: header with a 200 response, but SPENGO does not use it, and it would be very to hard to test its implementation without a method that does use it. Maybe open a follow-up ticket for that ? |
I just wanted to make you aware of the RFC. I know that SPNEGO over HTTP predates this new RFC from @reschke. As long as someone else wants to hook in something which uses the RFC defined header this PR should support it. No need to modify for SPNEGO anything explicitly. |
@stoty Generally the change-set looks good to me. I only would like to make a few really minor code tweaks and, naturally, fix API incompatibility. There are two options. 1. I can take over from here, make those changes and let you test and review the version that will go into master 2. You can continue working on your branch and I will just comment on things that I would like changed or done. |
Option 1 is fine, thank you @ok2c . |
I have looked into rfc7615 some more, @michael-o . To support it properly, IMO
Alternatively, extracting the challanges could be pushed down to the Scheme API as a new method, but I'm not sure if that's worth it. |
Superseded by #614 |
I have kept the old commit chain for ease of review.