[release/1.7] remotes: always try to establish tls connection when tls configured#9188
Merged
estesp merged 1 commit intocontainerd:release/1.7from Oct 4, 2023
Conversation
When a endpoint is configured for http and has a tls configuration, always try to the tls connection and fallback to http when the tls connections fails from receiving an http response. This fixes an issue with default localhost endpoints which get defaulted to http with insecure tls also configured but are using tls. Signed-off-by: Derek McGowan <[email protected]> (cherry picked from commit 79772a0) Signed-off-by: Derek McGowan <[email protected]>
Member
|
/retest |
dcantah
approved these changes
Oct 4, 2023
estesp
approved these changes
Oct 4, 2023
Member
|
This caused a regression: |
AkihiroSuda
reviewed
Oct 10, 2023
|
|
||
| var defaultTLSConfig *tls.Config | ||
| if options.DefaultTLS != nil { | ||
| explicitTLS = true |
Member
There was a problem hiding this comment.
Probably this shouldn't be true when DefaultScheme is "http"
Contributor
There was a problem hiding this comment.
Whether the InsecureSkipVerify field should be checked?
eg:
if options.DefaultTLS != nil {
if !options.DefaultTLS.InsecureSkipVerify {
explicitTLS = true
}
defaultTLSConfig = options.DefaultTLS
}
Member
Author
There was a problem hiding this comment.
Is the DefaultScheme only "http" there when explicitly set by the client? The "http" fallback should always be safe, but the tests might not expect that extra connection attempt. If the behavior is explicit from the client, we can keep as false, otherwise maybe we should see if the test could ignore the tls connection attempt.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
When a endpoint is configured for http and has a tls configuration, always try to the tls connection and fallback to http when the tls connections fails from receiving an http response. This fixes an issue with default localhost endpoints which get defaulted to http with insecure tls also configured but are using tls.
Backport of #9182
Fixes a regression in how localhost is handled with tls