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

feature/alpn support #143

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ibaniski
Copy link

Provide support for ALPN extensions (HTTP/2)

Ilija Baniski added 2 commits July 15, 2016 14:38
To propagate the original ALPN extension we needed to make changes at
four points:

1. On original ClientHello. Here we piggyback on the early peak into the
ClientHello message to extract SNI and also extract the full ALPN
extension. We are not being too smart about it; if it is not fully
available, we just skip it (thus falling back to the original
behaviour). We store this in the ctx.

2. On "fake" ClientHello to the original destination, we set the ALPN we
extracted during (1) above (available in the ctx).

3. On "fake" ServerHello from the original destination, we extract the
ALPN that the real server accepted. We store this in the ctx, same
location as (1) above (so original list is gone, but we don't need it
anymore).

4. On original connection establishment, we add a new callback (as the
"fake" server) to choose protocol from the ALPN list provided by the
original ClientHello (1). Here we just set whatever the real server
chose in (3).

We rely on libssl 1.0.2 for ALPN functions and callbacks. OPENSSL
version checks added to toggle ALPN support.

Change-Id: Iee7f240d98ae0d1af52e09ae1010242b9d4b9217
Existing tests in the ssl_tls_client_hello_parse suite were modified to
use the new interface of ssl_tls_clienthello_parse().

Additional test was added to specifically exercise extraction of ALPN
from a ClientHello message.

Change-Id: Id7bfb3701f8db9b2b9bfb9fdbff3d217fe9dd4d6
@droe
Copy link
Owner

droe commented Jul 23, 2016

Thanks for the contribution. Have you tested this patch under conditions where browser and server actually do select h2 or spdy and you use a https proxyspec? I expect changes to the protocol parsing to be required in order to properly support HTTP/2.

Related to #62, #89, #91.

@droe droe added the feature label Jul 23, 2016
@ibaniski
Copy link
Author

You are correct, it does not work correctly when using the https proxyspec.

However, I am using it with ssl proxyspec where it works as expected. I have a modification where it only relays ALPN in non-http proxyspec. Let me know if you are interested in taking a look. With the patch, as far as I can tell there are no concerns regarding backwards compatibility.

Also, I could take a look at implementing what may be necessary to make http/2 work in https proxyspec. Do you know, at a high level, what would need to be supported? I see in the pxy_http_reqhdr_filter_line and pxy_http_resphdr_filter_line functions that some headers are modified/removed, but without more context I can't tell how that would translate to http/2.

In http/https proxyspec modes sslsplit parses HTTP and performs certain
modifications (mostly in the HTTP headers). This parsing fails for
HTTP/2 (as expected).

Only perform ALPN forwarding for ssl proxyspec, where raw data is just
proxied across the two connections without any modifications.

Change-Id: I40ed058ffbf273d98bd8214ae654a8202dc5c5a8
@droe droe mentioned this pull request Feb 10, 2018
@droe droe added the priority label Feb 10, 2018
@droe droe added this to the 0.5.3 milestone Feb 10, 2018
@droe droe self-assigned this Feb 10, 2018
@phlantin
Copy link

phlantin commented Jul 6, 2018

This patch is very useful to inspect http/2 content, thank you. I look forward to seeing formal http/2 support to be able to observe the headers which are compressed.

@droe droe removed this from the 0.5.3 milestone Jul 16, 2018
@droe
Copy link
Owner

droe commented Jul 16, 2018

Would you be willing to also update the manual page with information on how the different modes behave with this patch regarding ALPN relaying and support for funky protocols?

Is there a need to make ALPN relaying configurable?

@droe droe removed their assignment Aug 1, 2018
@droe droe added this to To do in H1 Priority Roadmap Aug 25, 2018
@droe droe removed this from To do in H1 Priority Roadmap Aug 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants