HttpProxyHandler generates invalid CONNECT url and Host header when address is resolved by slandelle · Pull Request #7784 · netty/netty

slandelle · 2018-03-15T15:05:24Z

Motivation:

HttpProxyHandler uses NetUtil#toSocketAddressString to compute
CONNECT url and Host header.

The url is correct when the address is unresolved, as
NetUtil#toSocketAddressString will then use
getHoststring/getHostname. If the address is already resolved, the
url will be based on the IP instead of the hostname.

There’s an additional minor issue with the Host header: default port
443 should be omitted.

Modifications:

Only use NetUtil.toSocketAddressString when address is unresolved,
use getHostname otherwise
Don’t append port to Host header when it’s 443

Result:

HttpProxyHandler performs properly when connecting to a resolved address

normanmaurer · 2018-03-15T15:31:07Z

@slandelle this produced multiple test errors... please check

slandelle · 2018-03-15T15:40:28Z

Yes, I'll have a look.

But I think those tests are broken.
For example, setting the SslHandler before the HttpProxyHandler in the pipeline causes the CONNECT request to be sent over HTTPS instead of clear HTTP.
Those tests only pass because the proxy is a fake one and is broken too.

normanmaurer · 2018-03-15T16:33:55Z

@slandelle then fix the broken tests ;)

normanmaurer · 2018-03-16T10:36:44Z

@slandelle should we make this a utility method in the http package ? Like in HttpUtils ? I think this is something that is useful in general.

Can do, sure. But does it make sense to introduce this now while work on Netty 5 targeting Java 8 is about to start?

@slandelle I think it still does as netty 5 is most likely still be 1 year away

This utility would be better suited for NetUtil than HttpUtil, don't you think?

Not sure... isnt it kind of related how you format stuff for the http header ?

Ah, ok, I thought you were only talking about the first line above your comment, not the whole block. Will do.

slandelle · 2018-03-16T10:42:09Z

@normanmaurer @trustin Note that tests passes, but I didn't fix the logic in ProxyHandlerTest that looks broken to me. That would be a separate issue.

normanmaurer · 2018-03-16T10:42:36Z

@slandelle sounds good... if you have time we love PRs as you know ;)

kachayev · 2018-03-16T11:58:49Z

@slandelle @normanmaurer I have a question regarding

default port 443 should be omitted

RFC 7230 and older RFC2616 on Host header and RFC7231 on CONNECT method do not require excluding 443 port from the header.

RFC2616 says

A "host" without any trailing port information implies the default port for the service requested

Which might be applied to 80 and 443 if we still honor 2616, but not exclusively to 443.

slandelle · 2018-03-16T12:08:15Z

@kachayev From my experience with AHC), there are some servers that really expect with port. And that's how browsers compute Host headers too.

fenik17 · 2018-03-16T12:28:11Z

Why did you make this change?

kachayev · 2018-03-16T12:40:50Z

@slandelle With the latest changes getHostHeader implementation makes perfect sense and correlates with RFCs (excluding both default ports). But here you always call the method with secure set to true, when in fact I can use http-only proxy. And here url is calculated by checking only 443 (my point that it's better either honor both default ports or none of them).

slandelle · 2018-03-16T12:53:07Z

@kachayev You wouldn't use this handler for http-only proxy, the mechanism is different and doesn't use a CONNECT request (directly connect to proxy and send absolute url instead of relative). I have to check for WebSockets then.

kachayev · 2018-03-16T13:21:17Z

@slandelle "wouldn't use" doesn't mean it's not allowed. RFC7231 I mentioned above does not specify tunnel to be used only with a secure connection. CURL, for example, does not establish tunnel when connecting to HTTP proxy by default but you still can force this:

$ curl -vvv -o /dev/null -x http://localhost:8888 --proxytunnel http://www.google.com
* Rebuilt URL to: http://www.google.com/
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8888 (#0)
* Establish HTTP proxy tunnel to www.google.com:80
> CONNECT www.google.com:80 HTTP/1.1
> Host: www.google.com:80
> User-Agent: curl/7.54.0
> Proxy-Connection: Keep-Alive
>
< HTTP/1.1 200 Connection established
<
* Proxy replied OK to CONNECT request
> GET / HTTP/1.1
> Host: www.google.com
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 200 OK

slandelle · 2018-03-16T14:39:43Z

@kachayev Which proxy do you use? Then, what do you suggest? Ignore 80 and 443? It's not possible to guess the actual scheme and current API doesn't let you specify.

kachayev · 2018-03-16T14:57:05Z

@slandelle Charles for Mac, but that's irrelevant because a lot of proxies do support CONNECT over plain HTTP. My suggestion is not to ignore even default ports (neither 443 nor 80) as it might break functionality in some cases (at least we cannot guarantee that it would not).

slandelle · 2018-03-16T15:10:51Z

My suggestion is not to ignore even default ports (neither 443 nor 80) as it might break functionality in some cases (at least we cannot guarantee that it would not).

From my experience, it's the other way around: things broke when the default port was there. And that's what AHC and browser does. I'm fine with not ignoring by default, but I'll introduce an option to override.

slandelle · 2018-03-16T15:44:42Z

@normanmaurer PTAL again

kachayev · 2018-03-16T15:56:16Z

@slandelle 👍

normanmaurer · 2018-03-16T20:15:05Z

@slandelle you need to revert all the removals of throws... as its a breaking change. People who may have override the method and have throws on it will be in trouble.

slandelle · 2018-03-16T20:46:40Z

@normanmaurer fixed

carl-mastrangelo · 2018-03-16T23:14:54Z

cc: @zpencer

normanmaurer · 2018-03-20T11:54:06Z

@carl-mastrangelo @ejona86 @trustin any of you can verify this one ?

zpencer · 2018-03-20T16:32:23Z

I can take a look

ejona86 · 2018-03-20T19:10:02Z

FWIW, the getHostString changes all look good. It's the ignoreDefaultPortsInConnectHostHeader that I needed to look at more (including the discussion here) before feeling comfortable approving.

zpencer · 2018-03-20T17:21:40Z

Let's add a test for this case.

@slandelle can you do this ?

I'm not convinced it would make sense to test this single line of code.
Then, url and Host header computations are entangled so I can save allocations.
I can't test HttpProxyHandler#newInitialMessage because I would need to override destinationAddress() which is final.
I'm going to extract CONNECT request creation in a static method.

Actually, it should be fine. Stay tuned.

slandelle · 2018-03-22T09:42:46Z

@normanmaurer @zpencer Host header tests added, PTAL again.

normanmaurer · 2018-03-22T13:40:07Z

@trustin any concerns ?

trustin · 2018-03-22T16:20:04Z

Sorry! Will take a look tomorrow. 2018년 3월 22일 (목) 오후 10:40, Norman Maurer <notifications@github.com>님이 작성:

…

@trustin <https://github.com/trustin> any concerns ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7784 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKnXhyQ5NRLXdsIT21nlNMK5kckjDa5ks5tg6nlgaJpZM4SsUfR> .

normanmaurer

A few nits... otherwise LGTM

normanmaurer · 2018-03-22T19:49:05Z

normanmaurer · 2018-03-22T19:49:28Z

nit: you can remove the else as you return anyway.

normanmaurer · 2018-03-22T19:49:59Z

nit: add . on EOL.

slandelle · 2018-03-22T20:00:08Z

@normanmaurer done

trustin

Just a small comment.

trustin · 2018-03-23T07:01:49Z

I guess a user of this method may think the return value will contain the port number. How about adding a boolean parameter withPort, so that this method adds :<port> when it's true?

What about trying to find a better name instead? Maybe formatHostnameForHttp?
I'm not fond of expanding an API beyond what's being used internally so it can serve for whatever other purpose we're not aware of.
WDYT?

That's also fine.

…ddress is resolved Motivation: HttpProxyHandler uses `NetUtil#toSocketAddressString` to compute CONNECT url and Host header. The url is correct when the address is unresolved, as `NetUtil#toSocketAddressString` will then use `getHoststring`/`getHostname`. If the address is already resolved, the url will be based on the IP instead of the hostname. There’s an additional minor issue with the Host header: default port 443 should be omitted. Modifications: * Introduce NetUtil#getHostname * Introduce HttpUtil#formatHostnameForHttp to format an InetSocketAddress to HTTP format * Change url computation to favor hostname instead of IP * Introduce HttpProxyHandler ignoreDefaultPortsInConnectHostHeader parameter to ignore 80 and 443 ports in Host header Result: HttpProxyHandler performs properly when connecting to a resolved address

normanmaurer · 2018-03-27T07:43:18Z

@slandelle thanks!

slandelle · 2018-03-27T07:51:50Z

@normanmaurer Thanks for merging

normanmaurer requested changes Mar 16, 2018

View reviewed changes

fenik17 reviewed Mar 16, 2018

View reviewed changes

normanmaurer requested changes Mar 16, 2018

View reviewed changes

normanmaurer requested review from carl-mastrangelo, ejona86 and trustin March 16, 2018 20:16

normanmaurer approved these changes Mar 16, 2018

View reviewed changes

zpencer reviewed Mar 20, 2018

View reviewed changes

normanmaurer requested changes Mar 22, 2018

View reviewed changes

trustin reviewed Mar 23, 2018

View reviewed changes

normanmaurer approved these changes Mar 27, 2018

View reviewed changes

normanmaurer merged commit d60cd02 into netty:4.1 Mar 27, 2018

normanmaurer added this to the 4.1.23.Final milestone Mar 27, 2018

Uh oh!

Conversation

slandelle commented Mar 15, 2018

Uh oh!

normanmaurer commented Mar 15, 2018

Uh oh!

slandelle commented Mar 15, 2018

Uh oh!

normanmaurer commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slandelle commented Mar 16, 2018

Uh oh!

normanmaurer commented Mar 16, 2018

Uh oh!

kachayev commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slandelle commented Mar 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kachayev commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slandelle commented Mar 16, 2018

Uh oh!

kachayev commented Mar 16, 2018

Uh oh!

slandelle commented Mar 16, 2018

Uh oh!

kachayev commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slandelle commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slandelle commented Mar 16, 2018

Uh oh!

kachayev commented Mar 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slandelle commented Mar 16, 2018

Uh oh!

carl-mastrangelo commented Mar 16, 2018

Uh oh!

normanmaurer commented Mar 20, 2018

Uh oh!

zpencer commented Mar 20, 2018

Uh oh!

ejona86 commented Mar 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slandelle commented Mar 22, 2018

Uh oh!

normanmaurer commented Mar 22, 2018

Uh oh!

kachayev commented Mar 16, 2018 •

edited

Loading

kachayev commented Mar 16, 2018 •

edited

Loading

kachayev commented Mar 16, 2018 •

edited

Loading

slandelle commented Mar 16, 2018 •

edited

Loading