✨ Allow TLS minimum version to be configured

[willthames]

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.

[k8s-ci-robot]

Welcome @willthames!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[willthames]

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

[coderanger]

/ok-to-test

@willthames You'll need to rebase/amend to remove the Fixes from the commit message. Because of how Kubernetes vendoring works, it can get noisy to leave those in the commits themselves.

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.

[willthames]

See #1431 for related issue

[willthames]

@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!)

[coderanger]

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.

[alvaroaleman]

AWS still supports 1.16 but kubernetes itself doesn't.

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.

[alvaroaleman]

IMHO we should error if TLSMinVersion is set but not in the list, silently picking an old version is not a good idea IMHO

[willthames]

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)

[alvaroaleman]

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

[willthames]

thanks for that suggestion - hopefully this iteration does what you're after.

[alvaroaleman]

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.

[willthames]

@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

[willthames]

@alvaroaleman changed title to use ✨

[willthames]

/retest

[k8s-ci-robot]

[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:

• OWNERS [alvaroaleman]

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

[alvaroaleman]

thanks!

[coderanger]

@alvaroaleman Is there a reason we didn't make 1.2 the default?

[alvaroaleman]

@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:

 // If zero, TLS 1.0 is currently taken as the minimum.

[coderanger]

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?

[alvaroaleman]

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.

[coderanger]

Legit, I'll open a new PR to debate changing the default :)