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

Allow configuration of TLS Version for Webhook servers #6511

Closed
altonf4 opened this issue May 12, 2022 · 11 comments · Fixed by #7483
Closed

Allow configuration of TLS Version for Webhook servers #6511

altonf4 opened this issue May 12, 2022 · 11 comments · Fixed by #7483
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@altonf4
Copy link

altonf4 commented May 12, 2022

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+(kubernetes-sigs/controller-runtime#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.

@fabriziopandini
Copy link
Member

/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 kubernetes-sigs/controller-runtime#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

@k8s-ci-robot k8s-ci-robot added this to the v1.2 milestone May 13, 2022
@fabriziopandini fabriziopandini added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jul 29, 2022
@fabriziopandini fabriziopandini removed this from the v1.2 milestone Jul 29, 2022
@fabriziopandini fabriziopandini removed the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jul 29, 2022
@lubronzhan
Copy link
Contributor

The controller runtime v0.13.0 already has the fix

@sbueringer
Copy link
Member

sbueringer commented Oct 12, 2022

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

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.

@fabriziopandini
Copy link
Member

/triage accepted

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Oct 12, 2022
@fabriziopandini
Copy link
Member

/help

@k8s-ci-robot
Copy link
Contributor

@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 k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Oct 12, 2022
@srm09
Copy link
Contributor

srm09 commented Oct 12, 2022

/assign

@srm09
Copy link
Contributor

srm09 commented Oct 12, 2022

Opened kubernetes-sigs/controller-runtime#2020 as per @fabriziopandini's first comment above.

@srm09
Copy link
Contributor

srm09 commented Oct 25, 2022

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

@srm09
Copy link
Contributor

srm09 commented Nov 2, 2022

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

@dlipovetsky
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants