Skip to content
This repository has been archived by the owner on Jul 6, 2018. It is now read-only.

http2: Reject incompatible TLS ALPN handshakes #144

Closed
wants to merge 4 commits into from

Conversation

sebdeckers
Copy link
Contributor

  1. Only announce http/1.1 in ALPN when the fallback is allowed.
  2. Immediately destroy connections that do not support HTTP/2 when the HTTP/1 fallback is not allowed. Previously these connections would instantiate an Http2Session and linger until they timed out or caused a protocol error.

Note: The Node.js API does not seem to allow rejecting TLS handshakes at the ALPN-level (as described in RFC 7301 3.2). Therefore we do not differentiate between TLS clients that failed to match an ALPN protocol, and clients that simply do not support ALPN. In practice this should make little, if any, difference. Should consider adding strict ALPN negotiation as an option to Node.js core TLS though.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

sebdeckers

This comment was marked as off-topic.

@jasnell
Copy link
Member

jasnell commented May 31, 2017

Updated master from nodejs/node master. This will need a rebase

@sebdeckers
Copy link
Contributor Author

@jasnell rebased 👍

mcollina

This comment was marked as off-topic.

@sebdeckers
Copy link
Contributor Author

@mcollina @jasnell As per discussion in the outdated code review I've changed this to emit an error instead of silently destroying the connection. See b900d2d. This could be pretty annoying to users, I fear, since must listen for socketError events or risk uncaught error events being thrown on a the socket (which is only accessible through the secureConnection event). Is that by design?

@mcollina
Copy link
Member

mcollina commented Jun 2, 2017

I do not think we should emit an error event. This is a very specific case, and we should be more diligent. So maybe we should use an "unknownProtocol" event passing the socket, and if there are no listeners, we silently drop. If there is a listener (check the return value of emit()), we do nothing and it would be up to the user to decide what they want to do with this.

@sebdeckers
Copy link
Contributor Author

@mcollina That sounds good to me. See ad8bb9d

@sebdeckers
Copy link
Contributor Author

@jasnell Rebased and ready for review. Minor patch but introduces a new event as per above discussion.

@mcollina
Copy link
Member

mcollina commented Jun 6, 2017

Still LGTM :).

jasnell

This comment was marked as off-topic.

jasnell pushed a commit that referenced this pull request Jun 6, 2017
Emit `unknownProtocol` event or silently destroy the socket

PR-URL: #144
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 6, 2017

Landed!

@jasnell jasnell closed this Jun 6, 2017
@sebdeckers sebdeckers deleted the reject-non-h2-alpn branch June 20, 2017 00:49
jasnell pushed a commit to jasnell/http2-1 that referenced this pull request Jun 22, 2017
Emit `unknownProtocol` event or silently destroy the socket

PR-URL: nodejs#144
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit to jasnell/http2-1 that referenced this pull request Jul 10, 2017
Emit `unknownProtocol` event or silently destroy the socket

PR-URL: nodejs#144
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit to jasnell/http2-1 that referenced this pull request Jul 14, 2017
Emit `unknownProtocol` event or silently destroy the socket

PR-URL: nodejs#144
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants