skip origin Authorization on CONNECT proxy request#2234
Conversation
hyperxpro
left a comment
There was a problem hiding this comment.
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.
|
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. |
|
Thanks a lot! |
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.sendRequestWithNewChannelviaperConnectionAuthorizationHeader, applied tofuture.getNettyRequest(), which is the CONNECT on the tunnel path. Same leak, so that call is now guarded on the request method too. AddedConnectRequestNtlmAuthorizationTest, 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:
connectis derived from the method, so a caller usingprepareConnect()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.