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

tls: set TLS v1.2 as the default minimal version for servers #19330

Merged
merged 1 commit into from
Jan 18, 2022
Merged

tls: set TLS v1.2 as the default minimal version for servers #19330

merged 1 commit into from
Jan 18, 2022

Conversation

derekguo001
Copy link

Commit Message: Remove TLS 1.0 and 1.1 from the default server TLS versions. Users can still explicitly opt-in to 1.0 and 1.1 using tls_minimum_protocol_version.
Additional Description:
Risk Level: Low
Testing: updated
Docs Changes: updated
Release Notes: added
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] #5398 and checks off one box for #5401
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link

Hi @derekguo001, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #19330 was opened by derekguo001.

see: more, trace.

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #19330 was opened by derekguo001.

see: more, trace.

adisuissa
adisuissa previously requested changes Jan 4, 2022
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

API comments change LGTM, left a comment about the release notes.

I'm not sure about possible implications of this change, and leaving this up to senior reviewers.

docs/root/version_history/current.rst Outdated
@@ -24,6 +24,7 @@ Minor Behavior Changes
* quic: add back the support for IETF draft 29 which is guarded via ``envoy.reloadable_features.FLAGS_quic_reloadable_flag_quic_disable_version_draft_29``. It is off by default so Envoy only supports RFCv1 without flipping this runtime guard explicitly. Draft 29 is not recommended for use.
* router: take elapsed time into account when setting the x-envoy-expected-rq-timeout-ms header for retries, and never send a value that's longer than the request timeout. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.update_expected_rq_timeout_on_retry`` to false.
* stream_info: response code details with empty space characters (' ', '\t', '\f', '\v', '\n', '\r') is not accepted by the ``setResponseCodeDetails()`` API.
* tls: remove TLS 1.0 and 1.1 from server defaults
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* tls: remove TLS 1.0 and 1.1 from server defaults
* tls: set TLS v1.2 as the default minimal version for servers.

And please add a link here to tls_minimum_protocol_version (specifying how the default can be modified).

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition, this should be moved to Incompatible Behavior Changes for better visibility.

Copy link
Author

Choose a reason for hiding this comment

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

OK. Fixed.

ggreenway
ggreenway requested changes Jan 4, 2022
View reviewed changes
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

/wait

docs/root/version_history/current.rst Outdated
@@ -24,6 +24,7 @@ Minor Behavior Changes
* quic: add back the support for IETF draft 29 which is guarded via ``envoy.reloadable_features.FLAGS_quic_reloadable_flag_quic_disable_version_draft_29``. It is off by default so Envoy only supports RFCv1 without flipping this runtime guard explicitly. Draft 29 is not recommended for use.
* router: take elapsed time into account when setting the x-envoy-expected-rq-timeout-ms header for retries, and never send a value that's longer than the request timeout. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.update_expected_rq_timeout_on_retry`` to false.
* stream_info: response code details with empty space characters (' ', '\t', '\f', '\v', '\n', '\r') is not accepted by the ``setResponseCodeDetails()`` API.
* tls: remove TLS 1.0 and 1.1 from server defaults
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition, this should be moved to Incompatible Behavior Changes for better visibility.

ggreenway
ggreenway requested changes Jan 5, 2022
View reviewed changes
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

This looks pretty good, but you have a CI failure that needs to be resolved.

/wait

@derekguo001
Copy link
Author

Thanks @ggreenway . I updated the code and resolved the CI failure. But it looks like a new CI sub-task timed out.

@derekguo001 derekguo001 changed the title tls: remove TLS 1.0 and 1.1 from the defaults on the server-side tls: set TLS v1.2 as the default minimal version for servers Jan 6, 2022
adisuissa
adisuissa reviewed Jan 6, 2022
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

/lgtm api

Approval pending CI tests.
/retest

@repokitteh-read-only repokitteh-read-only bot removed the api label Jan 6, 2022
@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19330 (review) was submitted by @adisuissa.

see: more, trace.

@PiotrSikora
Copy link
Contributor

Drive-by comment: This should be fine, but I'd wait with merging this right after after the release is cut on 1/15. Merging this days before the release doesn't give people enough time to catch issues in the real world.

@derekguo001
Copy link
Author

OK. @PiotrSikora Thanks.

ggreenway
ggreenway requested changes Jan 11, 2022
View reviewed changes
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

This looks good and we can merge after the release goes out. It will require an update after the release notes get reset when the release is cut. But other than that, this looks good.

/wait

@derekguo001
Copy link
Author

@ggreenway Sure, I'll update it after that.

tls: set TLS v1.2 as the default minimal version for servers
b4e66dd 
Remove TLS 1.0 and 1.1 from the default server TLS versions. Users can
still explicitly opt-in to 1.0 and 1.1 using tls_minimum_protocol_version.

Signed-off-by: derekguo001 <[email protected]>
@derekguo001
Copy link
Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19330 (comment) was created by @derekguo001.

see: more, trace.

@derekguo001
Copy link
Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19330 (comment) was created by @derekguo001.

see: more, trace.

@ggreenway ggreenway dismissed adisuissa’s stale review January 18, 2022 16:15

concerns were addressed

@ggreenway ggreenway merged commit f8baa48 into envoyproxy:main Jan 18, 2022
@ggreenway ggreenway mentioned this pull request Jan 18, 2022
Closed
@derekguo001 derekguo001 deleted the remove_tls_old_versions branch January 19, 2022 07:45
joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
tls: set TLS v1.2 as the default minimal version for servers (envoypr…
753767b 
…oxy#19330)

Remove TLS 1.0 and 1.1 from the default server TLS versions. Users can
still explicitly opt-in to 1.0 and 1.1 using tls_minimum_protocol_version.

Signed-off-by: derekguo001 <[email protected]>
Signed-off-by: Josh Perry <[email protected]>
@mikemorris mikemorris mentioned this pull request Sep 19, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggreenway ggreenway ggreenway approved these changes

@asraa asraa Awaiting requested review from asraa

@lizan lizan Awaiting requested review from lizan

@adisuissa adisuissa Awaiting requested review from adisuissa

Assignees

@adisuissa adisuissa

@ggreenway ggreenway

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@derekguo001 @PiotrSikora @adisuissa @ggreenway