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

internal/envoy: Allow TLSv1.3 for xDS connection. #4081

Merged
merged 2 commits into from
Oct 7, 2021
Merged

internal/envoy: Allow TLSv1.3 for xDS connection. #4081

merged 2 commits into from
Oct 7, 2021

Conversation

tsaarni
Copy link
Member

@tsaarni tsaarni commented Oct 7, 2021 •
edited
Loading

This change sets the maximum TLS version to TLSv1.3 in the Envoy bootstrap config for the xDS connection. It means that TLSv1.3 will be selected from now on, since Contour already accepts TLSv1.3. This change will also make it easier to enforce TLSv1.3-only policy at some later point in time.

Previously Envoy defaulted to TLSv1.2 for the xDS connection.

Updates #3518

Signed-off-by: Tero Saarni [email protected]

@tsaarni tsaarni requested a review from a team as a code owner October 7, 2021 05:51
@tsaarni
Copy link
Member Author

tsaarni commented Oct 7, 2021 •
edited
Loading

xref #4065 (comment)

@codecov
Copy link

codecov bot commented Oct 7, 2021 •
edited
Loading

Codecov Report

Merging #4081 (2f0b454) into main (db9e15d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4081      +/-   ##
==========================================
+ Coverage   75.36%   75.38%   +0.01%     
==========================================
  Files         111      111              
  Lines        9407     9413       +6     
==========================================
+ Hits         7090     7096       +6     
  Misses       2167     2167              
  Partials      150      150
Impacted Files Coverage Δ
internal/envoy/v3/bootstrap.go 92.12% <100.00%> (+0.22%) ⬆️

@tsaarni
Copy link
Member Author

tsaarni commented Oct 7, 2021

Some background for reviewers:

Normally TLS implementations will have TLSv1.3 enabled by default, but Envoy currently caps the max version to TLSv1.2 at the client end due to retry related corner cases that might happen when proxying envoyproxy/envoy#9300. I believe that this is not a valid concern for the xDS gRPC client, but since we never changed the default in bootstrap config, it practically prevented automatic negotiation with TLSv1.3.

skriss
skriss approved these changes Oct 7, 2021
View reviewed changes
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM, makes sense as a step forward.

@skriss
Copy link
Member

skriss commented Oct 7, 2021

Will need to get rebased and get a changelog added to pass the new changelog checks.

@tsaarni
internal/envoy: Allow TLSv1.3 for xDS connection.
df55e02 
This change sets the maximum TLS version to TLSv1.3 in the Envoy bootstrap
config for the xDS connection.  It means that TLSv1.3 will be selected from
now on, since Contour already accepts TLSv1.3.

Previously Envoy defaulted to TLSv1.2 for the xDS connection.

Updates projectcontour#3518

Signed-off-by: Tero Saarni <[email protected]>
@sunjayBhatia sunjayBhatia added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Oct 7, 2021
@tsaarni
Added changelog entry.
2f0b454 
Signed-off-by: Tero Saarni <[email protected]>
@tsaarni tsaarni added release-note/small A small change that needs one line of explanation in the release notes. and removed release-note/minor A minor change that needs about a paragraph of explanation in the release notes. labels Oct 7, 2021
@tsaarni
Copy link
Member Author

tsaarni commented Oct 7, 2021

I immediately confused minor and small when creating the changelog file :-D
I think this should be small.

@skriss skriss added this to the 1.19.0 milestone Oct 7, 2021
@skriss skriss merged commit 7963cce into projectcontour:main Oct 7, 2021
@youngnick youngnick mentioned this pull request Nov 2, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunjayBhatia sunjayBhatia sunjayBhatia approved these changes

@skriss skriss skriss approved these changes

@stevesloka stevesloka Awaiting requested review from stevesloka

@youngnick youngnick Awaiting requested review from youngnick

Assignees
No one assigned
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Milestone
1.19.0
Development

Successfully merging this pull request may close these issues.
3 participants
@tsaarni @skriss @sunjayBhatia