-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Sni support #1954
Sni support #1954
Conversation
…cateChain() on HttpConnection
… for the HTTP server
if (keyMgrFactory == null) { | ||
throw new VertxException("Key/certificate is mandatory for SSL"); | ||
if (mgr != null) { | ||
builder = SslContextBuilder.forServer(mgr.getPrivateKey(null), "wibble", mgr.getCertificateChain(null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is wibble
magic value required here? Or would it be possible to use other passwords too =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch :-)
@vietj This is one of those 'what it says its for, vs what you want to do' situations, I'm not sure there is an answer that pleases everyone. In adding to such not-FQDN peer host names, the main other issue with sending a value all the time is going to be when the 'hostname' given to connect to is actually an IP address. Those don't make sense to send for SNI (and are expressly forbidden by the RFC) and thus the implicit SNI wont send them, whereas the explicit way presumably would (not tried). For non-HTTP cases I think I'd do what it is now doing (and presumably did before) i.e doing implicit SNI, and then have a way to provide the desired value explicitly for any less usual cases where folks want to control the value. If you provide it explicitly then it gets sent explicitly, if not then the implicit SNI happens or not based on the hostname value given. For the HTTP case, its a little more interesting, but I think the same approach might be sufficient..i.e maybe all thats needed is renaming the new 'sni' option to something like 'force sni' and clarifying that SNI will happen in most cases, but this will force it to send the associated value. |
@gemmellr thanks for the feedback About forceSNI, it's a good tradeoff, we can leave it aside for now (i.e do implicit SNI) and add it later if there is demand for it. WDYT ? |
@vietj leaving the option out until someone says they need it also sounds reasonable, if you think most folks can do without it. I'm perhaps not the best person to ask though since I've never actually used the HTTP client and don't entirely know how behaviour has changed on that side of things :) |
@vietj Overriding the SNI value explicitly is very useful in an integration test setting, which is the reason for me wanting this feature at least. |
@lulf ok so let's keep it :-) . however we were talking of HttpClient and I believe you are mostly using porton based on NetClient which has it. But we'll have a forceSNI flag instead. |
Spending a bit more time looking at the code, for the HTTP client it seems it will use the host header value for the SSLEngine host if specified, in which case it effectively already has an SNI hostname value override, right? The thing a 'force' option would further let you do is make SNI happen even where the peer host or host header values specified were either non-FQDN or an IP address? |
The override is taken indeed from the The value should always been used from the host header as the host header is also used for pooling (so the request uses the right SSL connection and the correct certificate) So the force would just force the SSLEngine to use the In case of |
well and actually the primary purpose of SNI is for virtual hosts with SSL so well :-) |
Yep for NetClient in some cases you just might want to provoke use of different backend services fronted on the same DNS hostname (or IP address) and port, and SNI is a convenient way to do it, especially where there are multiple protocols involved too as you dont even need to inspect them then. That said, other protocols also include hostnames too though besides HTTP, such as AMQP 1.0, which is where some of the interest in this comes from for vertx-proton. |
@xkr47 you haven't replied to my question about bombing. I was thinking perhaps to handle it differently:
|
Sorry, I failed to associate that comment with me. So "bombing" is just something I happened to call it.. I got the idea when I accidentally returned null from So imagine you have a malicious client that keeps opening TLS connections to random IPs for scanning purposes (without SNI hostname in handshake). You therefore have no particular interest in serving the client. So if you decide to NOT have a default certificate in your server, the malicious client's attack will be cut short with a TLS handshake error. Only if the attacker knows a SNI hostname that your server actually accepts can he/she/it proceed. So this provides a little bit of additional security against these kinds of attacks. It however has the downside of rejecting any client NOT supplying a SNI hostname. There will therefore certainly be users that cannot use this option. A variant of this would be that the attacker client DOES send a SNI hostname in the handshake, but one that doesn't match any certificate in the server's KeyStore. In this case we can also (separately) decide whether to reply with a default certificate or not. Not reporting with a default certificate in this case is less likely to cause issues but it's still possible that e.g. hostnames not listed in any certificates' SAN are used if configured in DNS.
For the two cases I listed above, the first case is handled solely by the The second case would solely be handled by the
.. and allow the alias/index to be null to not have a default certificate? I guess the Generally I guess most users won't care about configuring the "default hostname for non-SNI connections" and "default hostname for SNI connections with mismatching hostname" cases separately; therefore the provided JksOptions, PfxOptions and PemKeyCertOptions can provide just one default setting covering both cases. People can still implement their own KeyCertOptions classes to get the functionality. This brings me to one final thing that annoys me a little (not directly related to the SNI support but still); the fact that JksOptions, PfxOptions and PemKeyCertOptions are merely data objects and their logic have been embedded into KeyStoreHelper using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good; I think you missed my comment in src/main/java/io/vertx/core/http/package-info.java
?
@vietj if the host header contains an IP address, will you be able to detect that in order not to use it as a SNI hostname? Or how are you planning on handling IPs? (also IPv6:s) |
this case is handled by setting an host header explicitly.
client.get(port, port, uri).setHost(“the_host);
… On Apr 27, 2017, at 1:55 AM, Jonas Berlin ***@***.***> wrote:
The override is taken indeed from the host header.
@vietj <https://github.com/vietj> if the host header contains an IP address, will you be able to detect that in order not to use it as a SNI hostname? Or how are you planning on handling IPs? (also IPv6:s)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#1954 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANxinwqdhn9XbsfVQ6YVKh7f7VK7yaTks5rz9mDgaJpZM4NF00u>.
|
thanks all for the review, your contribution definitely made a difference! |
@vietj So you didn't end up adding support for specifying fallback certificate? And no link from http/package-info to net/package-info? |
I thought about it. The issue with specifying a fallback certificate is that a client presenting a server name should get by default a certificate. However there is no way to have a good default value because we don't know the alias name the user would use in the existing keystore files. That being said SNI on the server is not enabled by default, so it means that SNI needs explicit configuration and the fallback certificate should be configured as well. So I do have mixed feelings about it, it would be interesting to see what other thinks. for the link : provide a PR :-) |
@vietj the 'forceSni' update and related test changes etc look good to me |
The default behaviour is to use the implicit SNI that Java does, it doesn't send the SNI hostname if the value given is an IP or non-FQDN. |
motivation : SNI is an important features for virtual hosting
changes: provides SNI support for HttpClient/HttpServer/NetClient/NetServer. On the server certificates are matched using the certificate CN or SAN DNS, the
PemKeyCertOptions
has been extended to support multiple entries for this matter. For HttpClient, the host header value is sent as SNI server name, for NetClient theconnect
method is overloaded to provide the server name in addition of the connect host.please review @pmlopes @alexlehm @cescoffier