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

a better default configuration for TLS connection of http2 #59

Closed
jmuk opened this issue May 2, 2017 · 3 comments
Closed

a better default configuration for TLS connection of http2 #59

jmuk opened this issue May 2, 2017 · 3 comments

Comments

@jmuk
Copy link
Contributor

jmuk commented May 2, 2017

It seems the TLS socket is initialized as tls.connect(port, host, options) in http2.connect. Some existing HTTP2 servers require some TLS options to set up the connection properly. More specifically:

  • servername (for Server Name Indication)
  • ALPN / NPN, specifying 'h2'

I believe those should be set by defaults, then passing a URL string will just work.

Also, maybe it's better to use secureConnect event instead of connect event for the TLS sockets for setting up a session -- so that it won't go into the session if some TLS setup failure happens.

@jasnell
Copy link
Member

jasnell commented May 2, 2017

The ALPN/NPN bit should already be handled internally by the code. If it's not, then that's definitely a bug. For the SNI, that should be able to be passed in on the options but I haven't tested it yet.

@jmuk
Copy link
Contributor Author

jmuk commented May 2, 2017

I think you're talking about the servers, but I was talking about the clients (sorry I wasn't clear).

The http2 client sockets are initialized around https://github.com/nodejs/http2/blob/master/lib/internal/http2/core.js#L1238 but no ALPN/NPN are specified yet, if I understand correctly.

SNI can be specified through options indeed, but (for the clients) I think it's better to specify the host name by default, otherwise an http2 connection request will look like http2.connect("https://http2.golang.org/", {servername: 'http2.golang.org'}) which are really redundant.

@jasnell
Copy link
Member

jasnell commented May 2, 2017

Ok. I'll verify shortly and will make the sni change. Thank you!

jmuk added a commit to jmuk/http2 that referenced this issue May 2, 2017
jmuk added a commit to jmuk/http2 that referenced this issue May 2, 2017
@jasnell jasnell closed this as completed in 4038ea0 May 7, 2017
jasnell pushed a commit that referenced this issue May 19, 2017
fixes: #59

Also, add a testcase for http2/TLS secure connection.
This verifies to send the server name and ALPN protocols
by default.

PR-URL: #61
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue May 31, 2017
fixes: #59

Also, add a testcase for http2/TLS secure connection.
This verifies to send the server name and ALPN protocols
by default.

PR-URL: #61
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 issue Jun 22, 2017
fixes: nodejs#59

Also, add a testcase for http2/TLS secure connection.
This verifies to send the server name and ALPN protocols
by default.

PR-URL: nodejs#61
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 issue Jul 10, 2017
fixes: nodejs#59

Also, add a testcase for http2/TLS secure connection.
This verifies to send the server name and ALPN protocols
by default.

PR-URL: nodejs#61
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 issue Jul 14, 2017
fixes: nodejs#59

Also, add a testcase for http2/TLS secure connection.
This verifies to send the server name and ALPN protocols
by default.

PR-URL: nodejs#61
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 a pull request may close this issue.

2 participants