Skip to content
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

Closed
wants to merge 13 commits into from

Conversation

stoty
Copy link
Contributor

@stoty stoty commented Jan 21, 2025

I have kept the old commit chain for ease of review.

@stoty
Copy link
Contributor Author

stoty commented Jan 21, 2025

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authentication?

Copy link
Contributor Author

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.

Copy link
Member

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.

@michael-o
Copy link
Member

michael-o commented Jan 21, 2025

@stoty
Copy link
Contributor Author

stoty commented Jan 21, 2025

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 ?

@michael-o
Copy link
Member

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.

@ok2c
Copy link
Member

ok2c commented Jan 21, 2025

@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.

@stoty
Copy link
Contributor Author

stoty commented Jan 22, 2025

Option 1 is fine, thank you @ok2c .

@stoty
Copy link
Contributor Author

stoty commented Jan 22, 2025

I have looked into rfc7615 some more, @michael-o .

To support it properly, IMO

  • the new headers should be first added to httpcore to sync it to rfc9110,
  • then we need a new httpcore release , and update httpclient to use it,
  • then HttpAuthenticator needs to be modified to check them to when extracting the challanges

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.

@ok2c
Copy link
Member

ok2c commented Jan 22, 2025

Superseded by #614

@ok2c ok2c closed this Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants