cluster-api icon indicating copy to clipboard operation
cluster-api copied to clipboard

Published 20 hours ago • verified publisher icon kubernetes-sigs

Allow configuration of TLS Version for Webhook servers

Open altonf4 opened this issue 3 years ago • 8 comments

The current MinTLSVersion that the webhook servers (cabpk/capi/kcp) start with is 1.0. This is a potential security vulnerability.

With controller-runtime v0.9.x+(https://github.com/kubernetes-sigs/controller-runtime/pull/1620/), the controller-runtime Manager supports configuration of this via MinTLSVersion in the webhook.Server values. Leveraging this directly via CAPI's webhook servers isn't possible as this isn't configurable via env args or manager args.

This should be extended to allow users to define the appropriate MinTLSVersions that are secure for their environments/use cases.

altonf4 avatar May 12 '22 22:05 altonf4

/milestone v1.2

In order to fix this in CAPI (and in the other controllers) IMO there should be a change in controller runtime allowing to set the flags introduced in https://github.com/kubernetes-sigs/controller-runtime/pull/1620 for the default webhook server, ideally via new flags added to the manager.Options struct similarly to what we already have for WebHook Port and Host

fabriziopandini avatar May 13 '22 10:05 fabriziopandini

The controller runtime v0.13.0 already has the fix

lubronzhan avatar Oct 12 '22 00:10 lubronzhan

I think it would be good if we would implement the flag like this:

  • It should be ~ consistent with the tls-min-version flag of the kube-apiserver regarding flag name + possible values
  • I think we should default to 1.2. (as far as I can tell Kubernetes defaults to 1.2 as well https://github.com/kubernetes/component-base/blob/master/cli/flag/ciphersuites_flag.go#L130-L147)

I think it would be good to have this flag consistent across providers and document it accordingly (like some others we already have for e.g. the metrics endpoint), otherwise it's bad for users.

sbueringer avatar Oct 12 '22 09:10 sbueringer

/triage accepted

fabriziopandini avatar Oct 12 '22 12:10 fabriziopandini

/help

fabriziopandini avatar Oct 12 '22 12:10 fabriziopandini

@fabriziopandini: This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to this:

/help

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.

k8s-ci-robot avatar Oct 12 '22 12:10 k8s-ci-robot

/assign

srm09 avatar Oct 12 '22 17:10 srm09

Opened https://github.com/kubernetes-sigs/controller-runtime/issues/2020 as per @fabriziopandini's first comment above.

srm09 avatar Oct 12 '22 18:10 srm09

Waiting for a controller-runtime version to be available with the fix before creating a PR.

srm09 avatar Oct 25 '22 18:10 srm09

I think this is a good place to also think about exposing the --tls-cipher-suites flag in a similar fashion.

srm09 avatar Nov 02 '22 16:11 srm09

Just for reference: Requiring TLS >= 1.2 will not cause any issues for FIPS compliance. FIPS already requires 1.2 as the minimum version.

The clearest reference I found is this second-hand AWS announcement.

dlipovetsky avatar Nov 02 '22 17:11 dlipovetsky