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

Change minimum TLS version to 1.3 for internal encryption (between activator and queue-proxy) #13887

Merged
merged 2 commits into from
Jun 3, 2023
Merged

Change minimum TLS version to 1.3 for internal encryption (between activator and queue-proxy) #13887

merged 2 commits into from
Jun 3, 2023

Conversation

izabelacg
Copy link
Member

@izabelacg izabelacg commented Apr 14, 2023 •
edited
Loading

Proposed Changes

  • Change minimum TLS version (from 1.2 to 1.3) when internal encryption is activated: between activator and queue-proxy.

TLS 1.3 comes with numerous enhancements, such as a quicker TLS handshake and more secure cipher suites.

Release Note

* Activator uses TLS 1.3 as the minimum version when internal encryption is activated for communication with queue-proxy

@knative-prow knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/API API objects and controllers area/autoscale area/networking labels Apr 14, 2023
@codecov
Copy link

codecov bot commented Apr 14, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (d07bf78) 86.21% compared to head (480b706) 86.21%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #13887   +/-   ##
=======================================
  Coverage   86.21%   86.21%           
=======================================
  Files         199      199           
  Lines       14767    14767           
=======================================
  Hits        12731    12731           
  Misses       1734     1734           
  Partials      302      302
Impacted Files Coverage Δ
pkg/activator/certificate/cache.go 41.81% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

nak3
nak3 reviewed Apr 19, 2023
View reviewed changes
cmd/activator/main.go Outdated
@@ -285,7 +285,7 @@ func main() {
name, server := "https", pkgnet.NewServer(":"+strconv.Itoa(networking.BackendHTTPSPort), ah)
go func(name string, s *http.Server) {
s.TLSConfig = &tls.Config{
MinVersion: tls.VersionTLS12,
MinVersion: tls.VersionTLS13,
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems TLS 1.3 between Ingress -> activator has an issue - probably related to envoyproxy/envoy#9300

Could you try to revert this line if it will make CI pass? I think we should change this TLS min version carefully or at least by separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Testing this suggestion: tests running.

@skonto
Copy link
Contributor

skonto commented Apr 28, 2023 •
edited
Loading

@izabelacg @nak3 It seems that the latest update is that it works (TLS 1.3 is used) if you set both the minimum and the maximum version. Istio sets the same. Could we try that eg. set MaxVersion=tls.VersionTLS13 as well in the tls config?

@izabelacg izabelacg changed the title Change minimum TLS version to 1.3 for internal encryption [WIP] Change minimum TLS version to 1.3 for internal encryption Apr 28, 2023
@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2023
@izabelacg
Copy link
Member Author

Testing tls max version suggestion in this PR: #13930

nak3
nak3 reviewed May 1, 2023
View reviewed changes
cmd/activator/main.go Outdated
@@ -285,6 +285,7 @@ func main() {
name, server := "https", pkgnet.NewServer(":"+strconv.Itoa(networking.BackendHTTPSPort), ah)
go func(name string, s *http.Server) {
s.TLSConfig = &tls.Config{
//MinVersion: tls.VersionTLS13,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! Could you open an github issue for the tracker and add a comment with the issue number?
(If you can fix this soon, you don't need to open the issue, though.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened issue: #14057

@nak3
Copy link
Contributor

nak3 commented May 1, 2023

@izabelacg @nak3 It seems that the latest update is that it works (TLS 1.3 is used) if you set both the minimum and the maximum version. Istio sets the same. Could we try that eg. set MaxVersion=tls.VersionTLS13 as well in the tls config?

I think the workaround MaxVersion=tls.VersionTLS13 is supposed to be used on client side - Ingress (e.g. contour, kourier controller) for our case.
(And you want to use the workaround on Ingress, we might want to use the workaround on networking conformance test as well.)

@izabelacg izabelacg mentioned this pull request Jun 2, 2023
Closed
@izabelacg izabelacg changed the title [WIP] Change minimum TLS version to 1.3 for internal encryption [WIP] Change minimum TLS version to 1.3 for internal encryption (between activator and queue-proxy) Jun 2, 2023
@izabelacg izabelacg changed the title [WIP] Change minimum TLS version to 1.3 for internal encryption (between activator and queue-proxy) Change minimum TLS version to 1.3 for internal encryption (between activator and queue-proxy) Jun 2, 2023
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 2, 2023
@nak3
Copy link
Contributor

nak3 commented Jun 3, 2023

/lgtm
/approve

Thank you!

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2023
@knative-prow
Copy link

knative-prow bot commented Jun 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: izabelacg, nak3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2023
@knative-prow knative-prow bot merged commit 349b2d6 into knative:main Jun 3, 2023
skonto pushed a commit to skonto/serving that referenced this pull request Oct 5, 2023
@izabelacg @skonto
Change minimum TLS version to 1.3 for internal encryption (between ac…
a92cd2f 
…tivator and queue-proxy) (knative#13887)

* change mininum TLS version for when internal encryption is activated

* revert tls1.3 for activator - main.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nak3 nak3 nak3 left review comments

@jsanin-vmw jsanin-vmw Awaiting requested review from jsanin-vmw

@pradnyavmw pradnyavmw Awaiting requested review from pradnyavmw

Assignees

@nak3 nak3

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/autoscale area/networking lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@izabelacg @skonto @nak3