Avoid TLS fallback when protocol is not ambiguous #9283
Merged
Conversation
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
AkihiroSuda
added
cherry-pick/1.6.x
cherry-pick/1.7.x
AkihiroSuda
reviewed
Oct 23, 2023
AkihiroSuda
reviewed
Oct 23, 2023
akhilerm
reviewed
Oct 23, 2023
dmcgowan
force-pushed
the
tls-default-behavior
branch
2 times, most recently
from
28f2339 to
b458aa2
Compare
dmcgowan
force-pushed
the
tls-default-behavior
branch
from
b458aa2 to
f7f8ac3
Compare
ktock
reviewed
Oct 24, 2023
dmcgowan
force-pushed
the
tls-default-behavior
branch
from
f7f8ac3 to
c75b129
Compare
akhilerm
reviewed
Oct 24, 2023
mikebrow
reviewed
Oct 24, 2023
Member
mikebrow
left a comment
mikebrow
left a comment
There was a problem hiding this comment.
nit and a question regarding error messaging to explain the changes...
| // the protocol from the port number is ambiguous, use the | ||
| // docker.HTTPFallback roundtripper to catch TLS errors and re-attempt the | ||
| // request as http. This allows preference for https when configured but | ||
| // also catches TLS errors early enough in the request to avoid sending |
Member
There was a problem hiding this comment.
will this "flavor" pass through to enhance the error messaging and let the error show as trying https first based on (tryTLSFirst) is enabled?
would be nice if the error messaging was obvious telling the user what to do to address config changes.. for each testable situation
Member
Author
There was a problem hiding this comment.
Added an info log for this case
dmcgowan
force-pushed
the
tls-default-behavior
branch
from
c75b129 to
d31bd98
Compare
dmcgowan
force-pushed
the
tls-default-behavior
branch
from
d31bd98 to
a7c832f
Compare
The TLS fallback should only be used when the protocol is ambiguous due to provided TLS configurations and defaulting to http. Do not add TLS configurations when defaulting to http. When the port is 80 or will be defaulted to 80, there is no protocol ambiguity and TLS fallback should not be used. Signed-off-by: Derek McGowan <[email protected]>
dmcgowan
force-pushed
the
tls-default-behavior
branch
from
a7c832f to
d48ceb6
Compare
akhilerm
approved these changes
Oct 25, 2023
samuelkarp
approved these changes
Oct 25, 2023
This was referenced Oct 26, 2023
dmcgowan
added
cherry-picked/1.6.x
dmcgowan
added
the
cherry-picked/1.7.x
kiashok
added a commit
to kiashok/containerd
that referenced
this pull request
Oct 23, 2024
Update fork-external main with upstream main @ 452ec25 Related work items: containerd#5890, containerd#7647, containerd#9218, containerd#9233, containerd#9258, containerd#9270, containerd#9274, containerd#9279, containerd#9283, containerd#9286, containerd#9289, containerd#9290, containerd#9294, containerd#9295, containerd#9297, containerd#9305, containerd#9306, containerd#9308, containerd#9316, containerd#9317, containerd#9319, containerd#9320, containerd#9321
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
The TLS fallback should only be used when the protocol is ambiguous due to provided TLS configurations and defaulting to http. Do not add TLS configurations when defaulting to http. When the port is 80 or will be defaulted to 80, there is no protocol ambiguity and TLS fallback should not be used.
Alternative to #9269