-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
common/tls: Allow specifying SNI hostnames #7897
Merged
sspaink
merged 4 commits into
influxdata:master
from
antifuchs:use-sni-in-http-response
Dec 23, 2020
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
43f6526
tls_config: Allow specifying SNI hostnames
antifuchs 8beb583
Adjust the x509_cert to allow usage of tls_server_name
antifuchs ceb9c49
Improve documentation on what we try to accomplish in the nil return
antifuchs f96b6f5
Remove unused struct field in tests
antifuchs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,10 +15,11 @@ var pki = testutil.NewPKI("../../../testutil/pki") | |
|
||
func TestClientConfig(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
client tls.ClientConfig | ||
expNil bool | ||
expErr bool | ||
name string | ||
client tls.ClientConfig | ||
expNil bool | ||
expErr bool | ||
serverName string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, vestige of a previous idea for a test - Removing it now. |
||
}{ | ||
{ | ||
name: "unset", | ||
|
@@ -86,6 +87,14 @@ func TestClientConfig(t *testing.T) { | |
SSLKey: pki.ClientKeyPath(), | ||
}, | ||
}, | ||
{ | ||
name: "set SNI server name", | ||
client: tls.ClientConfig{ | ||
ServerName: "foo.example.com", | ||
}, | ||
expNil: false, | ||
expErr: false, | ||
}, | ||
} | ||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
|
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 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 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct. If you neither specify a CA nor a KEY or CERT but a
ServerName
then this check will not trigger but I doubt it makes sense to check the servername without validating the certificate!?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
ServerName
directive does not affect only the host name in certificate checking: It's also the hostname requested via SNI in the TLSClientHello
message - that's the thing that I need to customize that prompted me to submit the PR in the first place (:Even if that weren't the case, an empty TLS Config is documented to use the host's root CA set if no root CAs are provided, so setting the ServerName value on an otherwise-empty TLS config would be meaningful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please put a comment there linking to the docu you mentioned. Just for the next one asking himself why this should make sense!? Thanks for the explanation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, so I re-read the TODO comment above & started auditing our code's usage of
TLSConfig()
, where I found out that the only case where a nil tls.Config matters (because the stdlib function tls.Client requires a non-nil value) is the x509_cert.go code... where we override the ServerName based on a configuration setting (incompatibly with the "common" ServerName setting now)!So I'll have to fix the ServerName usage in x509_cert.go, but more importantly: I think we should remove that TODO, as it could be a pretty meaningful distinction in terms of "were there any settings that the operator customized" (but ultimately, it looks like there's not much of a difference). What do you think?