Skip to content

skip origin Authorization on CONNECT proxy request#2234

Merged
hyperxpro merged 2 commits into
AsyncHttpClient:mainfrom
madib06ops:connect-strip-origin-auth
Jul 4, 2026
Merged

skip origin Authorization on CONNECT proxy request#2234
hyperxpro merged 2 commits into
AsyncHttpClient:mainfrom
madib06ops:connect-strip-origin-auth

Conversation

@madib06ops

@madib06ops madib06ops commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

newNettyRequest adds the origin Authorization header to every request it builds, including the CONNECT that opens a proxy tunnel. That CONNECT goes to the proxy in the clear before the TLS tunnel exists, so preemptive Basic or Digest credentials meant for the origin are handed to the proxy. Gate the origin Authorization on the non-CONNECT case, matching the proxy-auth line right below it; the credentials still travel on the request sent once the tunnel is up.

NTLM/Kerberos/SPNEGO realms take a different path: their header is attached in NettyRequestSender.sendRequestWithNewChannel via perConnectionAuthorizationHeader, applied to future.getNettyRequest(), which is the CONNECT on the tunnel path. Same leak, so that call is now guarded on the request method too. Added ConnectRequestNtlmAuthorizationTest, which drives a preemptive NTLM request through an HTTP proxy and asserts the CONNECT the proxy receives carries no Authorization header (it fails without the sender-side guard).

Behavior note: connect is derived from the method, so a caller using prepareConnect() directly with a preemptive realm previously got an Authorization header on that CONNECT and no longer does. That's the intended fix (the header should never ride the plaintext CONNECT), but it's an observable change worth calling out for release notes.

@hyperxpro hyperxpro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch on the preemptive Basic/Digest case. The tunneled request still picks the header back up after the CONNECT succeeds, so nothing breaks there.

This doesn't fully close the leak, though. In NettyRequestSender.sendRequestWithNewChannel around line 413:

requestFactory.addAuthorizationHeader(headers, perConnectionAuthorizationHeader(request, proxy, realm));

On the tunnel path, future.getNettyRequest() is the CONNECT request, so preemptive NTLM/Kerberos/SPNEGO realms still attach the origin Authorization header to the plaintext CONNECT. It's the same bug through a different code path. Could you guard this as well? An NTLM test would also be a nice addition to keep this path covered.

NTLM/Kerberos/SPNEGO preemptive realms attach the origin Authorization
in NettyRequestSender via perConnectionAuthorizationHeader, applied to
future.getNettyRequest(), which is the CONNECT on the tunnel path. Gate
it on the request method so those credentials stay off the plaintext
CONNECT and only travel on the tunneled request.
@madib06ops

Copy link
Copy Markdown
Contributor Author

You're right, the per-connection path was the same bug through a different door. I've guarded the addAuthorizationHeader call in sendRequestWithNewChannel on the request method, so preemptive NTLM/Kerberos/SPNEGO no longer attach the origin header to the CONNECT. New ConnectRequestNtlmAuthorizationTest covers that path and fails without the guard. Existing HttpsProxyTest/HttpsProxyBasicTest/NTLMProxyTest still pass, and I noted the prepareConnect() behavior change in the description.

@hyperxpro hyperxpro merged commit d07dbc7 into AsyncHttpClient:main Jul 4, 2026
13 checks passed
@hyperxpro

Copy link
Copy Markdown
Member

Thanks a lot!

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.

2 participants