tls: Fall back to certificate keyed by empty name (fixes #2035) #2037
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This should only happen for sites defined with an empty hostname (like ":8080") and which are using self-signed certificates or some other funky self-managed certificate. But that certificate should arguably be used for all incoming SNI names.
|
@benjamin-demarteau I'm gonna merge this soon, unless you find that it doesn't work for you. (But I'm sure it will, if I understood your situation correctly.) |
|
Sorry, I'm not a golang dev, I couldn't get it to build.
…On 22 Feb 2018 02:29, "Matt Holt" ***@***.***> wrote:
@benjamin-demarteau <https://github.com/benjamin-demarteau> I'm gonna
merge this soon, unless you find that it doesn't work for you. (But I'm
sure it will, if I understood your situation correctly.)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2037 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/Af34SbRB9woj7rPX-QZfDz9ZiaROxn6yks5tXMLjgaJpZM4SMIaJ>
.
|
|
Hmm, while I think this fixes the issue, I think my implementation is slightly wrong on other edge cases. Will be pushing a new commit later... |
Also fix self-signed certs to include IP addresses in their name if they are configured to serve an IP address
It would be good to revisit this in the future.
|
I think this PR is in a better state now. I've reverted the behavior related to empty SNI, where we would only serve a "default" certificate if the client doesn't have SNI. Because sometimes (mostly self-signed situations) the site owner wants a certificate even if it's the wrong one. (But in the future I really want that to be opt-in rather than the default!) Ready for a review. Or will assume a good state and merge in a couple days. |
This was referenced Nov 12, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
1. What does this change do, exactly?
Serves a certificate keyed by an empty hostname for any SNI name if
there were no other matches.
This should only happen for sites defined with an empty hostname (like
":8080") and which are using self-signed certificates or some other
funky self-managed certificate. But that certificate should arguably
be used for all incoming SNI names.
2. Please link to the relevant issues.
Fixes #2035
3. Which documentation changes (if any) need to be made because of this PR?
Probably none
4. Checklist
@benjamin-demarteau Please take a look, let me know how this works for you!