Skip to content
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 matching does not work in certain cases when there is only one CN certificate in the keystore #2886

Closed
ancwrd1 opened this issue Sep 5, 2018 · 6 comments · Fixed by #2888
Assignees

Comments

@ancwrd1
Copy link

ancwrd1 commented Sep 5, 2018

Jetty version 9.4.12.

Conditions:

  • keystore contains two certificates: one CN certificate with subjectAltNames configured and alias "cert1", and one certificate without CN, with alias "default".
  • SslContextFactory configured with certAlias set to "default", so it will fallback if SNI matching fails.

Expected behaviour:
SNI matching works and "cert1" is selected correctly when connecting to it's host name.

Observed behaviour:
"default" certificate is always selected unless a second CN certificate is added to the keystore.

I think this is related to commit 8660055.

@gregw
Copy link
Contributor

gregw commented Sep 5, 2018

Thanks for this report and the diagnosis of #2010 ! looking...

@sbordet
Copy link
Contributor

sbordet commented Nov 1, 2018

@Dremon I don't think we have enough information.

For commit 8660055 to make a difference for your case, you would need to have exactly 1 non-wildcard host and zero wildcard hosts, so that before commit 8660055 the expression would evaluate to true and after would evaluate to false.
I doubt that you have such case with 2 certificates in the keystore.

@gregw while #2888 may be an improvement for that expression, but I'm not sure it changes anything for the OP of this issue.

@gregw
Copy link
Contributor

gregw commented Nov 1, 2018

I think we do have enough info. The current code will enable SNI matching IFF:

if (!_certWilds.isEmpty() || _certHosts.size()>1)

or to paraphrase: SNI will be used to select a certificate IFF there are some wild card certificates OR more than 1 certificate with a host name.
However, the OP has 2 certificates: neither with a wild card and only one that has a hast name, so neither of these apply and SNI matching is not enabled. So SNI is not used to select the certificate and the default certificate may be selected (probably dependent on indeterminate ordering) even if the other certificate would have matched the host name.

My proposed change is:

!_certWilds.isEmpty() || _certHosts.size()>1 || _certHosts.size()==1 && _aliasX509.size()>1

which can be paraphrased as: SNI will be used to select a certificate IFF there are some wild card certificates OR more than 1 certificate with a host name OR (there are more than 1 certificates AND one of them has a host name ).
This last clause exactly matches the OPs situation so SNI matching will be enabled...... hmmm now I do have a doubt.... perhaps we will then not select the default certificate for a non-matching host.

I guess I just have to create a unit test to confirm... stand by....

@sbordet
Copy link
Contributor

sbordet commented Nov 1, 2018

@gregw how can a certificate with CN and subjectAltNames and another with no CN (but presumably subjectAltNames) yield just one entry in _certHosts?
The only possibility is that they all point to the same host name, and so they are misconfigured: if there is only one name there is no need for SNI.

@ancwrd1
Copy link
Author

ancwrd1 commented Nov 1, 2018

To clarify:

keystore contains 1 SNI certificate (CN + subjectAltNames) and 1 non-SNI certificate (CN=localhost, no subjectAltNames).

This non-SNI certificate is always selected (incorrectly) even when connecting to the host name which matches first SNI certificate, unless a second SNI certificate is added (then it works).

@gregw
Copy link
Contributor

gregw commented Nov 1, 2018

@Dremon if you could test the PR #2888 that would be good. Although we have added a unit test that duplicated your problem so we are moderately confident we've fixed it.

gregw added a commit that referenced this issue Nov 2, 2018
* Issue #2886 Handle SNI with non SNI certificates

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #2886 Single SNI with default certificate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants