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.

Sign up for GitHub

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

Jump to bottom

Issue #2886 Handle SNI with non SNI certificates #2888

Merged
merged 4 commits into from Nov 2, 2018
Merged

Issue #2886 Handle SNI with non SNI certificates #2888

merged 4 commits into from Nov 2, 2018

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Sep 5, 2018

Fix #2886 by modifying the #2010 fix so that a single SNI host will still trigger SNI if there are other certificates.

Signed-off-by: Greg Wilkins gregw@webtide.com

@gregw
Issue #2886 Handle SNI with non SNI certificates
1deb83b 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Copy link
Contributor Author

gregw commented Sep 5, 2018

@sbordet @joakime This looks a little bit wrong... I need brains trust!

joakime
joakime requested changes Sep 10, 2018
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change needs a test case to verify the bug and to prevent regression.

@joakime joakime mentioned this pull request Sep 10, 2018
Closed
@sbordet sbordet mentioned this pull request Nov 1, 2018
Closed
@sbordet
Copy link
Contributor

sbordet commented Nov 1, 2018

@gregw hold this until we have more information about #2886.
While I think the changes in this PR may be ok, they may not fix #2886.

@gregw
Copy link
Contributor Author

gregw commented Nov 1, 2018

@joakime can your re-review

@joakime joakime added this to the 9.4.x milestone Nov 1, 2018
joakime
joakime reviewed Nov 1, 2018
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename snikeystore_nowild to snikeystore_nowild.<type> (being <type> of either .jks or .p12) ?

@gregw
Copy link
Contributor Author

gregw commented Nov 1, 2018

@sbordet @joakime The build broke because of 2 unrelated flaky tests.
Am I good to merge?

@joakime
Copy link
Contributor

joakime commented Nov 1, 2018

@gregw +1 from me

@gregw gregw merged commit b69f8e4 into jetty-9.4.x Nov 2, 2018
@gregw gregw deleted the jetty-9.4.x-2886-SNI-Certs branch November 2, 2018 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joakime joakime joakime approved these changes

@sbordet sbordet Awaiting requested review from sbordet

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
9.4.x
Development

Successfully merging this pull request may close these issues.

SNI matching does not work in certain cases when there is only one CN certificate in the keystore
3 participants
@gregw @sbordet @joakime