-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨ Allow TLS minimum version to be configured #1548
✨ Allow TLS minimum version to be configured #1548
Conversation
Welcome @willthames! |
Hi @willthames. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
One could argue that only 1.3 makes any real sense but I've gone with the same TLS version options as go's crypto package |
/ok-to-test @willthames You'll need to rebase/amend to remove the As for default version, I would be fine setting it to 1.3 if all recent kube-apiservers support that. That's the only thing which actually matters as a client. |
Some environments have automated security scans that trigger on TLS versions or insecure cipher suites. Setting TLS to 1.3 would solve both problems (setting to 1.2 only solves the former as the default 1.2 cipher suites are insecure). Default TLS minimum version of 1.0 remains.
4e0c31b
to
c8ecd75
Compare
See #1431 for related issue |
@coderanger TLS 1.3 support was enabled by default with go 1.13 (https://golang.org/doc/go1.13#tls_1_3) but was available in go 1.12 through a GODEBUG setting. Looks like kubernetes builds started to use go 1.13 from kubernetes 1.17 (from kubernetes/kubernetes@e3ff39f) AWS still supports 1.16 but kubernetes itself doesn't. If you're happy for me to make 1.3 the default I can make that change (it'll make this PR a lot simpler!) |
I would want to make sure that's okay with OperatorSDK crew because they have to support older stuff than the rest of us (usually) but I think 1.2 as the default would definitely make sense no matter what, and maybe 1.3 if we can swing it. I think the overall structure of your change is good though, just playing with which is the default value. |
It is afaik also still supported on GKE. Many ppl use one of the two so I don't think it is a good idea to default to tls 1.3 just yet. |
pkg/webhook/server.go
Outdated
s.tlsMinVersion = tls.VersionTLS12 | ||
case "1.3": | ||
s.tlsMinVersion = tls.VersionTLS13 | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO we should error if TLSMinVersion
is set but not in the list, silently picking an old version is not a good idea IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind suggesting a mechanism for this (setDefaults doesn't currently have any error handling so I'm not sure what pattern to follow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just move this into Start
. The only thing that really qualifies as defaulting is to set it to tls.VersionTLS10
if s.TLSMinVersion is an empty string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for that suggestion - hopefully this iteration does what you're after.
Also nit, but I don't think this qualifies as a bugfix, a bug is IMHO something that was already built and supposed to work but didn't, since we never had support for configuring the TLS version this is IMHO a feature. |
@alvaroaleman the description for bug also says patch (although I guess any change is a patch), and there is an issue raised for this - that was my reasoning but I'm not against changing it to feature |
@alvaroaleman changed title to use ✨ |
0477503
to
233c740
Compare
233c740
to
acdafe1
Compare
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, willthames 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 |
thanks! |
@alvaroaleman Is there a reason we didn't make 1.2 the default? |
Yeah, to not change what is currently happening, golang defaults to 1.0:
|
Sure but that seems like a bad default because kube-apiserver should support 1.2 in all cases? 1.3 was questionable but I don't think there's a reason to not pin up to 1.2 to block some funky MITM attacks? |
So I am not opposed to changing the default, golang defaulting to 1.0 is quite terrible actually. It just happens to be a change that might cause issues (I don't know if it does) and this change right here seemed to me just about adding a knob. |
Legit, I'll open a new PR to debate changing the default :) |
This field has been added in kubernetes-sigs#1548 It then turned out that people want to configure more parts of the TLSConfig and the generic TLSOpts was added in kubernetes-sigs#1897 Deprecate TLSMinVersion in favor of the more generic TLSOpts.
This field has been added in kubernetes-sigs#1548 It then turned out that people want to configure more parts of the TLSConfig and the generic TLSOpts was added in kubernetes-sigs#1897 Deprecate TLSMinVersion in favor of the more generic TLSOpts.
Some environments have automated security scans that trigger
on TLS versions or insecure cipher suites. Setting TLS to 1.3
would solve both problems (setting to 1.2 only solves the former
as the default 1.2 cipher suites are insecure).
Default TLS minimum version of 1.0 remains.