Add support for setting SNI hostname and set for client SSL handler #1698

lulf · 2016-11-03T12:57:33Z

No description provided.

lulf · 2016-11-03T13:05:18Z

I wasn't sure if this should be put in ClientOptionsBase instead, since I think it only makes sense for clients. But I came to the conclusion that I liked it better along with other ssl options.

vietj · 2016-11-23T22:08:22Z

I just checked and it supports only a single host, we want instead to support a mapping of domain->key/cert that is supported by Netty's io.netty.handler.ssl.SniHandler.

so it seems (as far as I understand after a quick look), we should use the SniHandler with a mapping of domain->SslContext to fully implement this feature.

the TCPSSLOptions would have a Map<String, KeyCertOptions> somehow.

lulf · 2016-12-08T13:40:18Z

@vietj I've updated the PR to use SniHandler for server side mapping of host -> SslContext instead of SNIMatcher directly. The end2end interaction is tested in the SSLEngineTest integration test, where I made it use a different certificate for the sni host.

I haven't paid much attention to how this is exposed in the options, so please comment on how you would like that.

vietj · 2016-12-08T15:12:26Z

@lulf thanks going to review it next week

vietj · 2017-01-03T15:47:12Z

src/main/java/io/vertx/core/http/HttpServerOptions.java

@@ -280,6 +281,13 @@ public HttpServerOptions addEnabledCipherSuite(String suite) {
  }

  @Override
+  @GenIgnore


I believe we should try to handle Map<String, Options> in codegen to not have GenIgnore

vietj · 2017-01-03T15:52:07Z

@lulf

instructions on how to create server-virt-cert.pem and the key are missing in src/test/resources/tls/ssl.txt
there should be test in HttpTLSTest with various formats keystore/pkcs12/pem

lulf · 2017-01-03T18:41:04Z

@vietj

Instructions added, and I added a single test to HttpTLSTest that verifies that the cert is working. It's generated the same way as the server-cert.pem, because for SNI I needed another cert that was not trusted by the default ssl context. I'm not sure if adding more tests to HttpTLSTest is necessary.

Also, there are some test failures I need to look into that I just discovered.

* Allow setting SNI hostname for clients * Allow adding different key store options for servers Signed-off-by: Ulf Lilleengen <ulf.lilleengen@gmail.com>

lulf · 2017-01-04T18:48:58Z

@vietj

In addition to addressing your comments, I had to add some more functionality to SSLHelper to be able to support listening for handshakes and setting SSLEngine properties as several tests were failing due to that. Now all the unit tests pass here.

vietj · 2017-01-04T20:47:41Z

@lulf has it been pushed ? can you update the issue and list all the changes ?

lulf · 2017-01-05T08:02:10Z

@vietj

All code is pushed to this branch yes. Not sure what issue you are referring to, but these are the changes I've done:

Add option for controller client side SNI host parameter
Add option for adding per-domain keyCert options
Add SNIhandler with domain -> context mapping that extends the netty SniHandler to support vert.x usage of listening to handshake and setting engine options.
Replaced SslHandler with SNIHandler for http and net servers implementations
Added another certificate with instructions to be used in SNI tests, added simple unit test to HttpTLSTest to verify cert is working
Add unit tests for ssl helper SNI stuff in SSLHelperTest
Add integration test for SNI support in SSLEngineTest

lulf · 2017-01-05T08:39:55Z

Ah, that issue :)

vietj · 2017-01-05T14:46:40Z

@lulf there are a few things to change but before continuing I would like to focus on the most important part:

The SSL test in HttpTLSTest is incorrect because it only tests the SNI certificate validity as a certificate and does not configure the SNI server. Also the HttpServer does not implement SNI, otherwise the added test in HttpTLSTest would fail and it passes.

Such test can only be added if the *Client supports SNI as well or if we use directly Netty to write the client part of the tests.

So right now we need to have at least the HttpServer implement SNI and to have this tested effectively.

lulf · 2017-01-05T14:50:08Z

@vietj

This is tested in src/test/java/io/vertx/test/it/SSLEngineTest.java . It uses HttpServer and I modified it to tests that a client sets SNI and that it works and doesn't work.

vietj · 2017-01-05T15:01:22Z

@lulf my bad I did not have the correct version of files I see now createSNIHandler called by HttpServer and client

still the test testTLSCLientTrustVirtServerCertPEM does not seem correct as the caller does not configure SNI in server/client

lulf · 2017-01-05T15:02:40Z

@vietj

That is why i was a bit puzzled why I should have to add that test, since I already tested it in SSLEngineTest. I thought it was just to test certificate validity. I can extend that test to use SNI also, although it would still test the same thing.

vietj · 2017-01-05T15:04:38Z

@lulf yes, I beleive the Trust / Cert classes should configure the TCPSSLOptions rather with somehting configure(TCPSSLOPtions) which means a bit of refactoring.

* Add a few testcases to test different combinations

lulf · 2017-01-05T19:07:34Z

@vietj

I modified the TLSTest to take a SNI parameter for client and for server, and extended with a few testcases.

I'll just create separate commits during the review and squash them at the end to make it easier to see what has changed.

vietj · 2017-01-06T16:42:38Z

src/main/java/io/vertx/core/net/impl/SSLHelper.java

+  /**
+   * Custom SNI handler that allows for listening to handshakes and configuring the SSLEngine.
+   */
+  public class SNIHandler extends SniHandler {


Make it a normal class (not inner) and rename VertxSniHandler

vietj · 2017-01-06T16:43:24Z

src/main/java/io/vertx/core/http/HttpClientOptions.java

@@ -292,6 +292,12 @@ public HttpClientOptions setSsl(boolean ssl) {
  }

  @Override
+  public HttpClientOptions setSNIServerName(String serverName) {


rename setSniServerName as the rest of the options tends to use non capital acronyms

vietj · 2017-01-10T07:57:00Z

@lulf so here is what we can do : add a setVirtualHost(String) on HttpClientRequest, when this setter is called then the value is used as SNI server name and it overrides the HTTP host value sent to the server, i.e it would call setHost() with the value automatically (but the user might override again the host value after with setHost if he wants to but has to do it explicitely).

vietj · 2017-01-12T17:43:25Z

@lulf any news for setVirtualHost ?

lulf · 2017-01-12T20:26:20Z

@vietj I've added the method to the interface but haven't been able to wire things in the implementation yet. A bit busy the coming days unfortunately.

vietj · 2017-01-12T22:15:59Z

@lulf thanks for the headsup

theoweiss · 2017-03-08T10:37:53Z

Is there any chance to get this feature into 3.4.1?

theoweiss · 2017-03-27T22:36:54Z

Sorry, this feature is really important for our services based on vertx. @lulf @vietj do you have a plan when this PR will be ready to merge?

lulf · 2017-03-28T09:38:40Z

@theoweiss Unfortunately I don't have any time left to work on this these days, and there seems to be a lot of code changes in the code that the PR touches, so I don't think this will be merged unless someone else picks up the pieces or restarts the effort.

vietj · 2017-03-28T13:34:35Z

the effort is interesting, I would really like to grap well how the SNI configuration side, both on the server and on the client.

for instance it seems possible with Java keystores to map SNI hostname using keystores aliases instead of having a Map<String, KeyCertOptions> / Map<String, TrustOptions> which would get the actual stuff simpler.

vietj · 2017-03-28T13:35:17Z

and I would like it to be done soon, perhaps in a Vert.x 3.4.2 release. I believe the technical bits are here, it's only a matter of providing good configuration imho.

vietj · 2017-04-14T09:41:15Z

I've started a new branch for SNI (reused a few bits of this PR, thanks) : https://github.com/eclipse/vert.x/tree/sni-support

At the moment it provides NetSocket support only, the main point is the extension of the KeyCertOptions interface that provides a mapper of serverName to Key+CertChain. The ssl context are cached in SSLHelper based on the certificate used for the handshake.

@xkr47 you are probably interested in following this branch

xkr47 · 2017-04-14T17:40:01Z

@vietj sure!
Some quick thoughts:

It's perfectly possible that a remote client gives our server a SNI hostname that doesn't exist, therefore mgr = keyCertOptions.keyManagerMapper(vertx).apply(serverName); may be null. In my DynamicCertManager solution I provide the option of choosing a default certificate, or to leave the server without a default, in which case the TLS handshake fails when client-speicifed hostname does not match any certificate. The latter is imo a good additional layer of defense agains attackers randomly bombing servers by IP; their attack surface will be a lot smaller if they don't know a valid hostname.
As a server implementor I would like to be able to query incoming connections/requests what certificate or hostname was used in the TLS handshake.

vietj · 2017-04-15T17:42:33Z

thanks for the feedback, will take it in account

vietj · 2017-04-15T17:55:13Z

you can know already the certificate on NetSocket, I believe we should add the same on HttpConnection and both should have the serverName

vietj · 2017-04-20T07:38:06Z

I've mostly finished the SNI support (not the docs yet) : you can have a look https://github.com/eclipse/vert.x/tree/sni-support

vietj · 2017-04-20T07:42:27Z

@lulf can you have a look at it and how it can be used for your use cases ?

xkr47 · 2017-04-20T12:45:41Z

I would propose that DEFAULT_SNI is changed to true as soon as the feature is considered stable; unsupporting servers will simply ignore it.

vietj · 2017-04-24T09:08:58Z

the sni-support branch has now a PR opened for review, you might want to have a look and provide feedback #1954

vietj · 2017-04-24T09:09:47Z

@xkr47 you mean for HttpClientOptions ?

xkr47 · 2017-04-24T12:16:28Z

@xkr47 you mean for HttpClientOptions ?

Yes

vietj · 2017-04-24T13:07:33Z

@xkr47 I propose to leave it as false for now for the release 3.4.x and switch it to true when we'll be on 3.5.0-SNAPSHOT

vietj · 2017-04-27T12:24:58Z

closing this PR, thanks @lulf for the contribution

xkr47 · 2017-04-27T17:22:15Z

Thanks @lulf!

vietj added the to review label Nov 3, 2016

lulf force-pushed the support-tls-sni-client-option branch from 4b2f3e5 to 1adc8b6 Compare November 3, 2016 13:03

lulf force-pushed the support-tls-sni-client-option branch from 1adc8b6 to 6642cc8 Compare November 22, 2016 09:57

lulf force-pushed the support-tls-sni-client-option branch from 6642cc8 to bc694f0 Compare December 8, 2016 13:34

lulf force-pushed the support-tls-sni-client-option branch from bc694f0 to 3ebf171 Compare December 8, 2016 13:47

vietj reviewed Jan 3, 2017

View reviewed changes

lulf force-pushed the support-tls-sni-client-option branch 2 times, most recently from fe6ce6c to 93fa66d Compare January 3, 2017 18:37

Add support for SNI

53e9bf2

* Allow setting SNI hostname for clients * Allow adding different key store options for servers Signed-off-by: Ulf Lilleengen <ulf.lilleengen@gmail.com>

lulf force-pushed the support-tls-sni-client-option branch from 93fa66d to 53e9bf2 Compare January 4, 2017 18:46

vietj mentioned this pull request Jan 5, 2017

Add SNI support #1477

Closed

Add support for SNI tests to TLS test

f7ff00e

* Add a few testcases to test different combinations

vietj reviewed Jan 6, 2017

View reviewed changes

vietj closed this Jan 12, 2017

vietj reopened this Jan 12, 2017

vietj added to review and removed to review labels Jan 12, 2017

vietj mentioned this pull request Feb 17, 2017

SSL server multiple certificates #1820

Closed

vietj closed this Apr 27, 2017

vietj removed the to review label Apr 27, 2017

lulf deleted the support-tls-sni-client-option branch April 27, 2017 13:00

Add support for setting SNI hostname and set for client SSL handler #1698

Add support for setting SNI hostname and set for client SSL handler #1698

Conversation

lulf commented Nov 3, 2016

lulf commented Nov 3, 2016

vietj commented Nov 23, 2016

lulf commented Dec 8, 2016

vietj commented Dec 8, 2016

vietj Jan 3, 2017

Choose a reason for hiding this comment

lulf Jan 3, 2017

Choose a reason for hiding this comment

vietj commented Jan 3, 2017

lulf commented Jan 3, 2017 • edited Loading

lulf commented Jan 4, 2017

vietj commented Jan 4, 2017

lulf commented Jan 5, 2017

lulf commented Jan 5, 2017

vietj commented Jan 5, 2017

lulf commented Jan 5, 2017 • edited Loading

vietj commented Jan 5, 2017

lulf commented Jan 5, 2017

vietj commented Jan 5, 2017

lulf commented Jan 5, 2017

vietj Jan 6, 2017

Choose a reason for hiding this comment

vietj Jan 6, 2017

Choose a reason for hiding this comment

vietj commented Jan 10, 2017 • edited Loading

vietj commented Jan 12, 2017

lulf commented Jan 12, 2017

vietj commented Jan 12, 2017

theoweiss commented Mar 8, 2017

theoweiss commented Mar 27, 2017 • edited Loading

lulf commented Mar 28, 2017

vietj commented Mar 28, 2017

vietj commented Mar 28, 2017

vietj commented Apr 14, 2017

xkr47 commented Apr 14, 2017

vietj commented Apr 15, 2017

vietj commented Apr 15, 2017

vietj commented Apr 20, 2017

vietj commented Apr 20, 2017 • edited Loading

xkr47 commented Apr 20, 2017

vietj commented Apr 24, 2017

vietj commented Apr 24, 2017

xkr47 commented Apr 24, 2017 • edited Loading

vietj commented Apr 24, 2017

vietj commented Apr 27, 2017

xkr47 commented Apr 27, 2017

lulf commented Jan 3, 2017 •

edited

Loading

lulf commented Jan 5, 2017 •

edited

Loading

vietj commented Jan 10, 2017 •

edited

Loading

theoweiss commented Mar 27, 2017 •

edited

Loading

vietj commented Apr 20, 2017 •

edited

Loading

xkr47 commented Apr 24, 2017 •

edited

Loading