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 TLSConfiguration to Kubevirt #8252

Merged
merged 7 commits into from Sep 10, 2022

Conversation

fossedihelm
Copy link
Contributor

What this PR does / why we need it:
Add TLS Configuration to Kubevirt spec.
This configuration allows specifying minTLSVersion and ciphers that will be used for the client/server communication.
TLS golang crypto package [1] allows overriding the Config that will be used for the communication specifying the GetConfigForClient function. Reading tlsConfiguration from kubevirt CR inside this function allows us to always have updated information.
Possible values for minTLSVersion are:

  • VersionTLS10
  • VersionTLS11
  • VersionTLS12
  • VersionTLS13

Values for ciphers are strings using the IANA syntax, and more specifically these:

General note about crypto package:

  • server always gives precedence to the maximum supported version provided by the client
  • it is not possible to specify ciphers when minTLSVersion is 1.3
    This means that, since we only specify minTLSVersion, communications between components (virt-handler/virt-api/etc) will be performed with TLS 1.3.

[1] https://pkg.go.dev/crypto/tls#Config
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Add `tlsConfiguration` to Kubevirt Configuration

@kubevirt-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XL kind/build-change Categorizes PRs as related to changing build files of virt-* components labels Aug 6, 2022
@fossedihelm
Copy link
Contributor Author

/test all

@fossedihelm
Copy link
Contributor Author

/test all

@fossedihelm
Copy link
Contributor Author

/cc @xpivarc @davidvossel

@fossedihelm fossedihelm marked this pull request as ready for review August 6, 2022 07:26
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2022
@stu-gott
Copy link
Member

stu-gott commented Aug 8, 2022

/retest-required

@tiraboschi
Copy link
Member

Personally I still think that letting the admin simply choose between a small set of predefined profiles, with the capability of eventually set a custom configuration, is an interesting idea.
Mozilla Foundation's Server Side TLS profiles are well known and pretty accepted as a de facto standard, although not so directly coupled in k8s ecosystem.

Copy link
Contributor

@akalenyu akalenyu left a comment

Choose a reason for hiding this comment

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

Looks good, I also want to make sure that following Simone's comment we are fine with not using the existing profile API.
(Would have loved to know if these profiles are something people from the security realm strongly expect or not)

BTW should we also add the TLS configuration to the export proxy components?

TLSConfig: &tls.Config{

@fossedihelm
Copy link
Contributor Author

fossedihelm commented Aug 9, 2022

BTW should we also add the TLS configuration to the export proxy components?

TLSConfig: &tls.Config{

@akalenyu You're right, the export proxy was added later my initial PR and I forgot to address it. Thanks for notice it.
Will adjust the commit!

@fossedihelm
Copy link
Contributor Author

/retest-required

Copy link
Contributor

@enp0s3 enp0s3 left a comment

Choose a reason for hiding this comment

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

@fossedihelm Hi! Thank you for the PR! Looks well arranged, I have a few questions below

cmd/virt-handler/virt-handler.go Show resolved Hide resolved
pkg/util/webhooks/tls.go Outdated Show resolved Hide resolved
pkg/util/webhooks/tls_test.go Outdated Show resolved Hide resolved
tests/infra_test.go Outdated Show resolved Hide resolved
tests/utils.go Outdated Show resolved Hide resolved
@fossedihelm
Copy link
Contributor Author

@xpivarc Pushed requested changes. PTAL

@xpivarc
Copy link
Member

xpivarc commented Sep 7, 2022

/lgtm
/hold
@enp0s3 This changed a little bit. Remove hold if you are still ok with this.

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2022
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2022
@fossedihelm
Copy link
Contributor Author

/retest

@enp0s3
Copy link
Contributor

enp0s3 commented Sep 7, 2022

/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2022
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fossedihelm
Copy link
Contributor Author

/retest-required

4 similar comments
@fossedihelm
Copy link
Contributor Author

/retest-required

@fossedihelm
Copy link
Contributor Author

/retest-required

@fossedihelm
Copy link
Contributor Author

/retest-required

@fossedihelm
Copy link
Contributor Author

/retest-required

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fossedihelm
Copy link
Contributor Author

/retest-required

1 similar comment
@fossedihelm
Copy link
Contributor Author

/retest-required

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Sep 10, 2022

@fossedihelm: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-fossa f2812d3 link false /test pull-kubevirt-fossa
pull-kubevirt-goveralls f2812d3 link false /test pull-kubevirt-goveralls

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. I understand the commands that are listed here.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot merged commit 3621975 into kubevirt:main Sep 10, 2022
@fossedihelm
Copy link
Contributor Author

/cherrypick release-0.53

@kubevirt-bot
Copy link
Contributor

@fossedihelm: #8252 failed to apply on top of branch "release-0.53":

Applying: Add TLSConfiguration to kubevirt
Applying: Refactoring: extract verifyPeerCert function
Applying: Use kubevirt tlsConfiguration in TLS config
Using index info to reconstruct a base tree...
M	cmd/virt-handler/virt-handler.go
M	pkg/util/webhooks/BUILD.bazel
M	pkg/util/webhooks/tls_test.go
M	pkg/virt-api/api.go
M	pkg/virt-controller/watch/application.go
M	pkg/virt-operator/application.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-operator/application.go
Auto-merging pkg/virt-controller/watch/application.go
Auto-merging pkg/virt-api/api.go
Auto-merging pkg/util/webhooks/tls_test.go
Auto-merging pkg/util/webhooks/BUILD.bazel
Auto-merging cmd/virt-handler/virt-handler.go
Applying: Add validation for TLSConfiguration
Using index info to reconstruct a base tree...
M	pkg/virt-operator/webhooks/kubevirt-update-admitter.go
M	pkg/virt-operator/webhooks/kubevirt-update-admitter_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-operator/webhooks/kubevirt-update-admitter_test.go
CONFLICT (content): Merge conflict in pkg/virt-operator/webhooks/kubevirt-update-admitter_test.go
Auto-merging pkg/virt-operator/webhooks/kubevirt-update-admitter.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0004 Add validation for TLSConfiguration
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-0.53

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/build-change Categorizes PRs as related to changing build files of virt-* components lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants