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

TLS NPN support #1524

Closed
wants to merge 2 commits into from
Closed

TLS NPN support #1524

wants to merge 2 commits into from

Conversation

crazyproger
Copy link

On my production system found that clients with TLS NPN can not connect to envoy. This patch fixes that, tested on my production system.

@RomanDzhabarov
Copy link
Member

@PiotrSikora do you mind taking a look? :-)

@PiotrSikora
Copy link
Contributor

I'll test this in a bit, but at the very least, it looks that the NPN support is added only on the downstream side and not the upstream. I'm not sure if we want to have this asymmetry.

@crazyproger
Copy link
Author

You are absolutely right - I've only tested downstream part, since my infrastructure does not use TLS to upstreams. If you suggest some code point where it should be implemented for upstreams - I will try to fix it tomorrow.

@crazyproger
Copy link
Author

After some investigation I think that process should be "mirrored" in ClientContextImpl. So in constructor callback should be installed through SSL_CTX_set_next_proto_select_cb() which will be very similar to alpnSelectCallback(). But one thing that I've still didn't found in OpenSSL documentation is - how to switch between ALPN and NPN on-the-fly(and ideally prefer ALPN?), since by RFCs negotiation variant is initiated by client in first ClientHello message.

@crazyproger crazyproger reopened this Aug 24, 2017
@mattklein123
Copy link
Member

@crazyproger @PiotrSikora no objection from me for supporting NPN (though it's been deprecated). But we should support both server and client. Also, you will need to add tests. Testing will be a little complicated insofar as without a config option, it will be difficult to differentiate NPN from ALPN. @PiotrSikora thoughts on what to do about tests?

@PiotrSikora
Copy link
Contributor

@crazyproger for the client side, you need to install callback using SSL_CTX_set_next_proto_select_cb() in ClientContextImpl::ClientContextImpl(). In the callback, you can use SSL_select_next_proto() to find the first shared protocol.

As for the choice between ALPN & NPN - BoringSSL will ignore NPN if ALPN was also announced by the client, and client will reject connection if server incorrectly negotiated both, so we should be covered there.

@mattklein123 We can't really test it without #186 (soon...), not without adding knobs to the config, which we shouldn't do.

@mattklein123
Copy link
Member

OK fair enough, I'm willing to let tests slide on this one once we add the client side code.

@mattklein123
Copy link
Member

Going to close this for now. We can reopen when ready to finish it up.

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.

4 participants