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.

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

Add support for TLS 1.3 ciphers. #90843

Merged
merged 1 commit into from May 12, 2020
Merged

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented May 7, 2020

What type of PR is this?
/kind bug

What this PR does / why we need it:

Adds support for TLS 1.3 ciphers as recommended by the security audit.

Which issue(s) this PR fixes:

Part of #81145

Special notes for your reviewer:
This change expands the list of possible TLS Ciphers that can be used within Kubernetes by adding 3 recommended ciphers for TLS 1.3, namely:

TLS_AES_128_GCM_SHA256
TLS_CHACHA20_POLY1305_SHA256
TLS_AES_256_GCM_SHA384

Does this PR introduce a user-facing change?:

Add support for TLS 1.3 ciphers: TLS_AES_128_GCM_SHA256, TLS_CHACHA20_POLY1305_SHA256 and TLS_AES_256_GCM_SHA384.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


/sig api-machinery
/sig auth
/area security
/priority backlog

@k8s-ci-robot k8s-ci-robot added release-note kind/bug size/XS sig/api-machinery sig/auth area/security cncf-cla: yes priority/backlog sig/cluster-lifecycle labels May 7, 2020
@k8s-ci-robot k8s-ci-robot requested review from dixudx and stewart-yu May 7, 2020
@pjbgf

This comment has been minimized.

1 similar comment
@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 7, 2020

/test pull-kubernetes-e2e-kind

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 7, 2020

/test pull-kubernetes-kubemark-e2e-gce-big

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 7, 2020

/test pull-kubernetes-e2e-kind

1 similar comment
@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 7, 2020

/test pull-kubernetes-e2e-kind

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 8, 2020

@cji I am adding you here as you created the issue.

/assign cji

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 10, 2020

/assign @luxas

@neolit123
Copy link
Member

@neolit123 neolit123 commented May 10, 2020

@liggitt
Copy link
Member

@liggitt liggitt commented May 10, 2020

hmm, I thought there was a unit test ensuring all supported ciphers were available in this map already… I would have expected a unit test failure if that was not the case

@@ -86,7 +86,8 @@ func TestConstantMaps(t *testing.T) {
if strings.HasPrefix(declName, "VersionTLS") {
discoveredVersions[declName] = true
}
if strings.HasPrefix(declName, "TLS_RSA_") || strings.HasPrefix(declName, "TLS_ECDHE_") {
if strings.HasPrefix(declName, "TLS_RSA_") || strings.HasPrefix(declName, "TLS_ECDHE_") ||
strings.HasPrefix(declName, "TLS_AES_") || strings.HasPrefix(declName, "TLS_CHACHA20_") {
Copy link
Member Author

@pjbgf pjbgf May 10, 2020

Choose a reason for hiding this comment

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

@liggitt I added the prefix of TLS1.3 ciphers to ensure the test will cover them going forward.

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 10, 2020

hmm, I thought there was a unit test ensuring all supported ciphers were available in this map already… I would have expected a unit test failure if that was not the case

@liggitt that's right, but the test was only covering ciphers that started with TLS_RSA_ or TLS_ECDHE, so they would skip the ciphers on this PR. PTAL

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 11, 2020

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-verify

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 11, 2020

/test pull-kubernetes-verify

1 similar comment
@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 11, 2020

/test pull-kubernetes-verify

@liggitt
Copy link
Member

@liggitt liggitt commented May 11, 2020

verify is failing because gofmt needs to be run. Run hack/update-gofmt.sh and update with the result

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 11, 2020

Thank you for pointing that out, @liggitt - just fixed it.

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 11, 2020

/test pull-kubernetes-e2e-kind-ipv6

@liggitt
Copy link
Member

@liggitt liggitt commented May 11, 2020

/approve

thanks, go ahead and squash to a single commit, then lgtm

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented May 11, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, pjbgf

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

@k8s-ci-robot k8s-ci-robot added the approved label May 11, 2020
@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 11, 2020

/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-kind-ipv6

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 11, 2020

/test pull-kubernetes-verify
/test pull-kubernetes-e2e-kind-ipv6

@liggitt liggitt added the tide/merge-method-squash label May 11, 2020
@liggitt
Copy link
Member

@liggitt liggitt commented May 11, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 11, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm label May 11, 2020
@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 11, 2020

/lgtm

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented May 11, 2020

@pjbgf: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@liggitt
Copy link
Member

@liggitt liggitt commented May 11, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 11, 2020
@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 11, 2020

/test pull-kubernetes-integration

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 11, 2020

/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-integration

@fejta-bot
Copy link

@fejta-bot fejta-bot commented May 12, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 12, 2020

/test pull-kubernetes-e2e-gce-100-performance

@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 12, 2020

/test pull-kubernetes-verify

1 similar comment
@pjbgf
Copy link
Member Author

@pjbgf pjbgf commented May 12, 2020

/test pull-kubernetes-verify

@k8s-ci-robot k8s-ci-robot merged commit 34226de into kubernetes:master May 12, 2020
17 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone May 12, 2020
@pjbgf pjbgf deleted the add-tls13-ciphers branch May 14, 2020
@pjbgf pjbgf restored the add-tls13-ciphers branch May 14, 2020
@pjbgf pjbgf deleted the add-tls13-ciphers branch Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/security cncf-cla: yes kind/bug lgtm priority/backlog release-note sig/api-machinery sig/auth sig/cluster-lifecycle size/XS tide/merge-method-squash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants