Issue #3863 - Enforce use of SNI

[sbordet]

Introduced SslContextFactory.rejectUnmatchedSNIHost (default false)
so that if no SNI is sent, or SNI does not match a certificate,
then the TLS handshake is aborted.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

[sbordet]

@gregw the logic is now that if there is no SNI or the SNI does not match, we abort the TLS handshake.

I don't know if it's worth to split those two cases.

While seems evident that if we connect to google.com but we send SNI wikipedia.org we should abort the TLS handshake, if we don't send SNI is less evident that we should abort.

So perhaps 2 booleans or an enum?

[gregw]

LGTM

[gregw]

After discussion, I think we need to do more here... including in documentation on how to use these modes.
The issue is that we need to consider various situations with the keystore as well:

• Keystore only has only one cert, which has SNI - simple decision for connections without SNI - match on cypher or fail with no SNI.
• Keystore has multiple certs all with SNI and no other certs - I think we must always fail a connection with no SNI in this case
• Keystore has multiple certs, sone with SNI and some without - A connection without SNI should only match certificates without SNI

So perhaps a single boolean is enough... but the name might not be right?

[sbordet]

@gregw you got it incorrect.

Client sends SNI and it's a single string.

Server certs may have a CN (Common Name) and possibly SANs (Subject Alternative Names).
And they can also be wildcards. Let's just call it SAN to indicate one or more server names.
Server always have at least 1 name.

So we have a table with clients with 3 states: no SNI, a matching SNI and a non-matching SNI.

And the server has states depending on the keystore: 1 exact SAN, 1 wild SAN, N exact SAN, N wild SAN + M exact SAN.

Let's build the table and see how many cases we have.

[gregw]

@sbordet I'm not sure that it matters if a cert match is a CN, SAN or wild-SAN match - that counts as a name match. I'm just saying that we need to simplify and consider just if we have a name match or not - regardless of the type of name match.

So firstly let's work out what information we have. I think it is correct that when we call engine.getSSLParameters().getSNIMatchers() we will get back:

• null SNI if there was no SNI name in the connection
• empty SNI collection if there was an SNI name, but it did not match anything
• SNI collection if there was a name match (and we tunnel the cert/alias via each match)

We then get a set of aliases that have matching cyphers, which I guess we could break into those 3 sets: those with no-SAN, those with matching SAN and those without matching SAN. So can we break it does like this:

1. null SNI
   a. if noSAN cert, then use that
   b. matching SAN must be empty because no SNI
   c. non matching SAN can be used by configuration if there is only 1 choice.
2. empty SNI set
   a. if noSAN cert, then use that
   b. matching SAN - must be empty because no matching SNI
   c. non matching SAN - never used.
3. SNI set
   a. of noSAN cert, then use if matching SAN set is empty or by configuration???
   b. matching SAN - use it!
   c. non matching SAN - never used

So we only need to consider configuration for 1.c and maybe 3.a. The current proposed option is called rejectUnmatchedSNIHost which I think works fine for both. If it is true then we reject at 1.c and reject at 3.1 if there are SAN matches. If it is false, then we use the cert at 1.c and 3.a

Or we could have two booleans?

[sbordet]

@gregw I don't understand what you mean by "noSAN cert". Every cert has a SAN (either only CN or CN+SAN).

1. no SNI (i.e. no SNI for CN=jetty.org,SAN=jetty.net)
   a. reject1 ? Reject at TLS level : send first certificate (defaults to send);
   Note that apps can configure SslContextFactory.certAlias to pick a specific certificate.
   Perhaps we may even send a 400 here.

2. unmatched SNI (i.e. SNI=foo.com for CN=jetty.org,SAN=jetty.net)
   a. reject2 ? Reject : send first certificate (defaults to reject);
   Reject could be at TLS level or at HTTP level (400) - I would prefer a 400 - note that e.g. google.com sends back a 200 with its certificate, so more choices here.

3. matched SNI (i.e. SNI of jetty.net for CN=jetty.org,SAN=jetty.net)
   a. send certificate

So we have 2 cases: reject1 => "reject missing SNI" and reject2 => "reject unmatched SNI".
The latter can be "reject at TLS", "reject at HTTP", "don't reject".

So perhaps we have one 3 state enum (REJECT_TLS, REJECT_HTTP, ALLOW), and 2 configurations (missingSNI, unmatchedSNI).

[gregw]

@sbordet I was thinking that we should consider certificates with CN but no SAN differently to ones with CN and SAN... but thinking about that now, I see no real reason to do so.

So reducing the choices we need to consider even more.... we need to work out what to do when we have no SNI match. We have no SNI match in two cases: 1. there was no SNI sent ; 2. there was an SNI sent, but it didn't match.

For case 1, I can see reason to not reject, but the question is which certificates should we allow a match with: Any? ; only ones without SAN?; only ones that are somehow otherwise tagged as default certificates?

For case 2. I really don't see a good reason for ever not rejecting... perhaps other than debugging?? But perhaps we can have a configuration of rejectMismatchedSNI. If rejecting, I think a TLS reject is the right thing to do, because it is still very strange to use a non matching certificate to send a 400... and then perhaps it is best to leave any HTTP rejection up to the virtual host mechanism? If we are not rejecting, then we still have the issue of what certificates to consider using?

Perhaps the configuration should not be a boolean? Perhaps we need to configure a set of aliases that are permissible to use for no SNI provided and for an SNI mismatch?

So lets say we configure with:

NoSniAlias: default.com
MismatchedSniAlias: whosthat.com
certificates: default.com, whosthat.com, jetty.org, *.jetty.org

We would then get

SNI	Candidate Certs	comment
none	default.com	response determined by handlers (maybe 400)
jetty.org	jetty.org, *.jetty.org
www.jetty.org	*.jetty.org
nasa.org	whosthat.com	response determined by handlers (maybe 400)

Now this handling kind of assumes that the request inside a connection will actually have a host header that matches. We do have a SniHostCheck in the SecureRequestCustomizer and it will send a 400 if the host does not match. I think we need to review that gives all the right behaviours in the cases above?

[sbordet]

@gregw we need to hangout about this to find the common ground.

I don't think we need to add more alias configurations to SslContextFactory. It currently has one, and in more complicated cases it is overridable enough to write the behavior you want.

This leaves us with 2 booleans: rejectMissingSNI and rejectUnmatchedSNI, where the reject action happens at TLS level (not HTTP).

Alternatively, we need an Enum for each of those properties, where the Enum is:

enum RejectType { NO_REJECT, REJECT_TLS, REJECT_HTTP }

and the properties are now not booleans by RejectTypes.

[gregw]

@sbordet Sure let's hangout.
However, I don't see how booleans configuration can answer the question of what certificates are OK to use when there is no SNI with an inbound connection. Sure we can come up with some heuristics that might get close to what we think are reasonable cases, but allowing a list of aliases to be configured will be a complete an final solution rather than a best guess.

[gregw]

Almost a LGTM. Mostly just comments and a few style suggestions for clarity.

[sbordet]

@gregw after our review I'm still not satisfied and many tests fail out of the box, so while the tests may have assumptions and the keystores are old and not really production-precise, I guess it may break people.

Old behavior

At SslContextFactory start we look for certAlias. If it was non-null, we wrap the key manager with AliasedX509ExtendedKeyManager.
Then we look at the keystore and map aliases and certificates, and (if needed) wrap again with SniX509ExtendedKeyManager.
So we have sniKM(aliasKM(jdkKM)).

The logic for choosing the server alias was:

• aliases = getServerAliases(keyType, issuers) which would go to the jdkKM
• if no aliases return null
• find the SNI and associated X509 (which may be null e.g. in case of present but mismatched host name)
• if x509.getAlias() was among the aliases, return that
• if there was a X509 but not in aliases return null
• no X509, return NO_MATCHERS.

Returning null was to ask the JDK to call again, and if no other call was done, terminate TLS.
Returning NO_MATCHERS was causing to delegate the call, which would go to aliasKM and return the certificate associated with certAlias or delegate to jdkKM which would return the first alias (and never return null).

This logic was also paired with the SecureRequestCustomizer because if sniKM chose a X509 we would have it as a SSLSession attribute.
If, however, some default from aliasKM or jdkKM was returned, that attribute was absent.

A call with no SNI would have had no X509, would have returned NO_MATCHERS; the call was delegated and either aliasKM or jdkKM would have returned a certificate; since sniKM did not choose the alias, SecureRequestCustomizer would have no X509 so would skip the check.
Same for a call with wrong SNI.
A call with SNI would have had sniKM to find the alias, a X509 as SSLSession attribute, and a check (always passing) in SecureRequestCustomizer.

New behavior

We still have the wrapping of key managers, but now it's not really needed because we don't delegate anymore.

The logic for choosing the server alias was:

• aliases = getServerAliases(keyType, issuers) which would go to the jdkKM
• if no aliases return null
• find the SNI (may not find it)
• call the SNISelector implementation with a subset of X509 (from the valid aliases)
• if we get a non-null X509, return that
• otherwise return null

The difference is that now sniKM always picks a X509, so the SSLSession attribute is always present. This always forces the check in SecureRequestCustomizer.
If we got some default from either certAlias or by picking the first certificate (in sniKM not by delegating), very likely the check in SecureRequestCustomizer fails. We don't have a way to say to SecureRequestCustomizer "this is a default pick, skip the check".
We have added SecureRequestCustomizer.rejectMissingSniHost in case SecureRequestCustomizer did not find the X509 SSLSession attribute, but now this never happens, as the X509 attribute is always there (or the communication is terminated at the TLS level).

Handling of multiple calls from the JDK

There is no way to know whether the call to chooseServerAlias() is the last one or not.
When we are called, we can either return null to the JDK (and be prepared that the JDK may terminate at TLS level), or we can return a default.
In the old behavior, NO_MATCHERS indicated a default and it was returned as soon as we had at least one alias but no X509, no matter the keyType.
There was no "wait for the next call with possibly a different keyType".
It had the nice side effect that would not put the X509 in the SSLSession attributes, even if a default was chosen.

Now we have SNISelector; we need to return the information of whether the X509 that was picked by the SNISelector implementation is a default or not, to convey this information to SecureRequestCustomizer that will skip the SNI host check.
Not doing that will result in a behavior change and possibly a lot of TLS failures where before it was just returning a certificate (albeit a wrong one, but the client could have been configured to ignore certificates).

I see 2 ways:

1. SNISelector should return a 3-state: null, X509 or default. This will mimic the old behavior, and we would reuse the wrapping and delegate to aliasKM and then to jdkKM.
2. SNISelector does all the logic (including picking a default), but has to return a tuple (X509, boolean) where the boolean says whether it's a default or not. This will likely require allocation to the return tuple, but it's the most flexible solution. To force implementation to add a "default" attribute to SSLSession seems too much to me (easily forgotten, not API friendly, looks like a hack).

The difficult case would be a keystore with an EC certificate for *.jetty.org and a RSA certificate for *.jetty.org and we want to return the EC certificate when SNI is strong.jetty.org.
We get called with keyType=RSA, one alias and SNI strong.jetty.org.
In general we don't know if we get called again. But the SNISelector implementation may know what's in the keystore and return null because it knows it will be called again with keyType=EC.
If it does not know if it will be called again it would have to return one X509 (or otherwise risk TLS termination), but likely not the right one.

@gregw other thoughts?

[gregw]

@sbordet good analysis.

I think that we have to assume that the JVM will call us in some priority order - probably the clients preferred order, so the intention of the API is that we return an alias as soon as there is an acceptable certificate. The problem in our heads is the what-if we are called with in a way that a no-SNI default weak alias can be returned, but that if we didn't then we would be called again with params that can match strong SNI alias. I think we can't worry about that... not least because I don't think that technically there is anything we can do, but mostly because if the API calls us in an order then that is the order and the first acceptable certificate will win.

So I think:

• we should create a dummy x509 called DELEGATE which the SNISelector can return if it wants a default non-sni certificate to be picked.
• our implementation of SNISelector should return any SNI match it finds, otherwise it returns DELEGATE if certAlias is not null, otherwise it returns null
• I don't think we need to distinguish between noSNI name and noSNI match
• The x509 is now only set if there is an SNI match. The SecureRequestCustomize should be modified to have a boolean sniRequired that fails requests (with close) if there is no x509, this is in addition to the current optional sniHostCheck that checks the name in the request matches the x509

[janbartel]

Changes look plausible, I have a few coding quibbles, but generally fine.

[sbordet]

Please add a test for SniSelector that tests this: rejects no SNI at TLS and rejects existing wrong SNI with 400.
Possibly the custom SniSelector should call the default implementation in SslContextFactory, don't copy/paste.