8326949: Authorization header is removed when a proxy Authenticator is set on HttpClient #21249

Michael-Mc-Mahon · 2024-09-29T16:46:06Z

This fix relaxes the constraints on user set authentication headers. Currently, any user set authentication headers are filtered out, if the HttpClient has an Authenticator set. The reason being that the authenticator is expected to manage authentication. With this fix, it will be possible to use pre-emptive authentication through user set headers, even if an authenticator is set. The expected use case is where the authenticator would manage either proxy or server authentication and the user set headers would manage server authentication if the authenticator is managing proxy (or vice versa).
If the pre-emptive authentication fails, then this behavior is disabled on further retries and it would be up to the authenticator to provide the right credentials then.

Thanks,
Michael

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8326949: Authorization header is removed when a proxy Authenticator is set on HttpClient (Enhancement - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21249/head:pull/21249
$ git checkout pull/21249

Update a local copy of the PR:
$ git checkout pull/21249
$ git pull https://git.openjdk.org/jdk.git pull/21249/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21249

View PR using the GUI difftool:
$ git pr show -t 21249

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21249.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-29T16:46:46Z

👋 Welcome back michaelm! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-09-29T16:47:37Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-09-29T16:48:28Z

@Michael-Mc-Mahon The following label will be automatically applied to this pull request:

net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-09-29T16:51:46Z

Webrevs

00: Full (fab3a25e)

AlanBateman · 2024-09-29T18:29:27Z

Would there be any merit to adding something to Builder.authenticator(Authenticator) to document the behavior? I don't know if this would be an API note or impNote. I'm mostly wondering where a developer might read about how/when it is used.

dfuch

The test seems to be missing a scenario where the provided preemptive credentials are wrong (or obsolete) and the stack then defaults to using the provided authenticator.

dfuch · 2024-09-30T12:52:10Z

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java

+            var plainCreds = "user:pwd";
+            var encoded = java.util.Base64.getEncoder().encodeToString(plainCreds.getBytes(US_ASCII));
+            var badCreds = "user:wrong";
+            var encoded1 = java.util.Base64.getEncoder().encodeToString(badCreds.getBytes(US_ASCII));


Is some part of the test missing? I don't see where encoded1 is used.
Also should there be some assertion as to whether ProxyAuth() was called or not?

dfuch · 2024-09-30T12:53:33Z

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java

+        try {
+
+            var client = HttpClient.newBuilder()


Ideally should use try-with-resource to also close the client.

dfuch · 2024-09-30T12:56:09Z

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java

+        try {
+            var client = HttpClient.newBuilder()
+                .version(java.net.http.HttpClient.Version.HTTP_1_1)
+                .build();


should use try-with-resource to close the client

dfuch · 2024-09-30T12:56:37Z

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java

+        try {
+            var client = HttpClient.newBuilder()


should use try-with-resource to close the client

dfuch · 2024-09-30T12:59:21Z

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java

+        }
+        @Override
+        public List<Proxy> select(URI uri) {
+          return List.of(new Proxy(Proxy.Type.HTTP, new InetSocketAddress("localhost", port)));


It would be better to use InetAddress.getLoopbackAddress() rather than "localhost"

dfuch · 2024-09-30T13:00:20Z

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java

+        @Override
+        protected PasswordAuthentication getPasswordAuthentication() {
+            if (getRequestorType() != RequestorType.SERVER) {
+                // We only want to handle proxy authentication here


Suggested change

// We only want to handle proxy authentication here

// We only want to handle server authentication here

dfuch · 2024-09-30T13:05:02Z

src/java.net.http/share/classes/jdk/internal/net/http/common/Utils.java

+/*
+        return (k, v) -> client.authenticator().isEmpty()
+            || (!k.equalsIgnoreCase("Authorization")
+                && !k.equalsIgnoreCase("Proxy-Authorization"))
+
+            // flag may be true for first attempt, and will be false
+            // for subsequent attempts if the first attempt failed
+            // due to 401/407
+
+            || req.tryUserSetAuthorization();
+*/


Shouldn't this commented code be removed?

dfuch · 2024-09-30T13:07:42Z

test/jdk/java/net/httpclient/UserAuthWithAuthenticator.java

+        }
+
+        public String baseURL() {
+            return "http://127.0.0.1:" + getPort();


Should use URIBuilder here to deal with possible IPv6-only envs

dfuch · 2024-09-30T13:14:03Z

I agree with Alan that it would be good to document how we use the Authenticator in HttpClient.Builder.
In which case this will probably require a CSR, or at least a release note.

Michael-Mc-Mahon · 2024-09-30T14:59:55Z

Would there be any merit to adding something to Builder.authenticator(Authenticator) to document the behavior? I don't know if this would be an API note or impNote. I'm mostly wondering where a developer might read about how/when it is used.

That would be useful. It definitely should be documented somewhere.

Michael-Mc-Mahon · 2024-09-30T15:01:03Z

Quite a few existing authentication regression tests are failing with the change. So, I need to investigate them and will address the other PR comments after that.

Michael-Mc-Mahon added 7 commits September 23, 2024 10:28

impl for fix

6e0b69a

Merge branch 'master' into 8326949-authorize

2bd4ce0

initial impl with test

bfd1653

test update

8d6a7fc

test update

c206705

test update

874529f

test update

fab3a25

openjdk bot added the rfr Pull request is ready for review label Sep 29, 2024

openjdk bot added the net [email protected] label Sep 29, 2024

dfuch reviewed Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8326949: Authorization header is removed when a proxy Authenticator is set on HttpClient #21249

8326949: Authorization header is removed when a proxy Authenticator is set on HttpClient #21249

Michael-Mc-Mahon commented Sep 29, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 29, 2024

openjdk bot commented Sep 29, 2024

openjdk bot commented Sep 29, 2024

mlbridge bot commented Sep 29, 2024

AlanBateman commented Sep 29, 2024 •

edited

Loading

dfuch left a comment

dfuch Sep 30, 2024 •

edited

Loading

dfuch Sep 30, 2024

dfuch Sep 30, 2024

dfuch Sep 30, 2024

dfuch Sep 30, 2024

dfuch Sep 30, 2024

dfuch Sep 30, 2024

dfuch Sep 30, 2024

dfuch commented Sep 30, 2024

Michael-Mc-Mahon commented Sep 30, 2024

Michael-Mc-Mahon commented Sep 30, 2024

	// We only want to handle proxy authentication here
	// We only want to handle server authentication here

8326949: Authorization header is removed when a proxy Authenticator is set on HttpClient #21249

Are you sure you want to change the base?

8326949: Authorization header is removed when a proxy Authenticator is set on HttpClient #21249

Conversation

Michael-Mc-Mahon commented Sep 29, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewing

Webrev

bridgekeeper bot commented Sep 29, 2024

openjdk bot commented Sep 29, 2024

openjdk bot commented Sep 29, 2024

mlbridge bot commented Sep 29, 2024

Webrevs

AlanBateman commented Sep 29, 2024 • edited Loading

dfuch left a comment

Choose a reason for hiding this comment

dfuch Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

dfuch Sep 30, 2024

Choose a reason for hiding this comment

dfuch Sep 30, 2024

Choose a reason for hiding this comment

dfuch Sep 30, 2024

Choose a reason for hiding this comment

dfuch Sep 30, 2024

Choose a reason for hiding this comment

dfuch Sep 30, 2024

Choose a reason for hiding this comment

dfuch Sep 30, 2024

Choose a reason for hiding this comment

dfuch Sep 30, 2024

Choose a reason for hiding this comment

dfuch commented Sep 30, 2024

Michael-Mc-Mahon commented Sep 30, 2024

Michael-Mc-Mahon commented Sep 30, 2024

Michael-Mc-Mahon commented Sep 29, 2024 •

edited by openjdk bot

Loading

AlanBateman commented Sep 29, 2024 •

edited

Loading

dfuch Sep 30, 2024 •

edited

Loading