feat: validator: Load TLS configuration from ConfigMap #1119

akrejcir · 2024-11-01T10:27:38Z

What this PR does / why we need it:
This PR moves the TLS configuration of template-validator to a ConfigMap. It is mounded as a file in the pod, and validator is able to update its configuration without restarting the pod.

Which issue(s) this PR fixes:
Jira: https://issues.redhat.com/browse/CNV-28716

Release note:

None

These methods are not used from outside the package. Signed-off-by: Andrej Krejcir <[email protected]>

Do not create or use the struct when using HTTP. Signed-off-by: Andrej Krejcir <[email protected]>

This package was copied directly from vm-console-proxy: https://github.com/kubevirt/vm-console-proxy/tree/main/pkg/filewatch In a future commit, it will replace file watch logic in internal/template-validator/tlsinfo. We do this to simplify watching multiple directories with TRS certificate and TLS configuration. Signed-off-by: Andrej Krejcir <[email protected]>

Runs all callbacks before processing watch events. This means that callbacks will have a change to notice the files after the watch was started, but no events happened yet. Signed-off-by: Andrej Krejcir <[email protected]>

kubevirt-bot · 2024-11-01T10:27:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from akrejcir. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

akrejcir · 2024-11-01T10:28:20Z

/cc @codingben @jcanocan @ksimon1

codingben · 2024-11-03T18:26:31Z

@akrejcir Is it simialr to the implementation as in kubevirt/vm-console-proxy, e.g. filewatch.go? Maybe you can reuse this existing file to from kubevirt/vm-console-proxy to avoid code duplication?

akrejcir · 2024-11-04T07:45:56Z

It is exactly the same package. I've mentioned it in the commit message: 3b2e200

Do you mean that SSP should import vm-console-proxy? I would rather avoid it, because it will cause unnecessary indirect dependencies.

internal/template-validator/tlsinfo/tlsinfo.go

internal/template-validator/validator/app.go

internal/template-validator/tlsinfo/tlsinfo.go

internal/operands/template-validator/resources.go

internal/template-validator/tlsinfo/tlsinfo.go

0xFelix · 2024-11-05T16:03:25Z

internal/template-validator/tlsinfo/tlsinfo_test.go

-	})
+		It("should load certificate", func() {
+			Expect(tlsInfo.Init()).To(Succeed())
+			defer tlsInfo.Clean()


defer vs DeferCleanup?

defer is called when func() {} returns. DeferCleanup() is called after the It() test case, similarly to AfterEach(). It is useful to call in BeforeEach() to register cleanups that will be called after the test.

Does it make a difference using DeferCleanup here?

I think only a performance difference.

Can it be moved to AfterEach()?

Yes, but I prefer it this way. It is also how it's used in the validator code app.go.

internal/template-validator/tlsinfo/tlsinfo_test.go

Using the filewatch package in template validator will make it easier to watch multiple directories in a future commit. Signed-off-by: Andrej Krejcir <[email protected]>

The GetClientConfig() will be used in future commit to set TLS options. Signed-off-by: Andrej Krejcir <[email protected]>

The TLS configuration is read from a ConfigMap that is mounted as a file. This allows updating the configuration without restarting the pod. Signed-off-by: Andrej Krejcir <[email protected]>

Signed-off-by: Andrej Krejcir <[email protected]>

This will make it easier to modify the tests in future commit. Signed-off-by: Andrej Krejcir <[email protected]>

Check TLS policy of template validator pod. Signed-off-by: Andrej Krejcir <[email protected]>

sonarcloud · 2024-11-07T10:37:58Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

0xFelix · 2024-11-08T09:48:22Z

internal/template-validator/tlsinfo/tlsinfo.go

+	}
+
+	ti.minTLSVersion = minVersion
+	ti.cipherSuites = common.CipherIDs(tlsOptions.OpenSSLCipherNames, nil)


What happens if this leaves us with an empty slice in case of user misconfiguration?

Good point. If user passes unknown cipher names, they will be silently left out. That is how it is implemented in the library function crypto.OpenSSLToIANACipherSuites(). I will add code to return an error instead.

If ti.cipherSuites is empty slice, then no cipher suites are enabled, except TLS 1.3 ciphers, which cannot be disabled.

Thinking about this, it may be desired behavior. Usually, on TCP connection, the server gives client a list of supported ciphers and the client chooses. Any unknown ciphers are ignored.
Maybe it is ok to use the same approach for configuration file. User could specify multiple ciphers, and it is not an error if the validator does not know some of them.

What do you think?

I was thinking what happens if you configure the API server with an empty slice if using TLS 1.1/1.2? Will it fallback to default ciphers? I'm not sure we should swallow this error silently. It is misconfiguration of the API server, not a failure in negotiating a cipher during a handshake.

If we configure the server ciphers with empty slice [] and not use TLS 1.3, then no ciphers will be enabled, so it will not accept any connection. If we configure it with nil slice, then it will use default ciphers.
This is IMHO a bad API design in the standard library. Usually in other places there is no functional difference between [] and nil.

Ok, I will change the code, so it fails with error if ciphers in config file are not known.

I've implemented the error checking and noticed that OpenShift's TLS security profiles also contain ciphers that are not implemented in the TLS in golang and they are silently ignored.
For example, here is the TLS Intermediate profile:
https://github.com/openshift/api/blob/a2817b89f7e0989016967055a142d5e88bed18de/config/v1/types_tlssecurityprofile.go#L288-L303

TLSProfileIntermediateType: { Ciphers: []string{ "TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", "TLS_CHACHA20_POLY1305_SHA256", "ECDHE-ECDSA-AES128-GCM-SHA256", "ECDHE-RSA-AES128-GCM-SHA256", "ECDHE-ECDSA-AES256-GCM-SHA384", "ECDHE-RSA-AES256-GCM-SHA384", "ECDHE-ECDSA-CHACHA20-POLY1305", "ECDHE-RSA-CHACHA20-POLY1305", "DHE-RSA-AES128-GCM-SHA256", "DHE-RSA-AES256-GCM-SHA384", }, MinTLSVersion: VersionTLS12, },

It contains ciphers DHE-RSA-AES128-GCM-SHA256 and DHE-RSA-AES256-GCM-SHA384, which are not available in the golang TLS library.
The code that does the translation between names and cipher IDs ignores them: https://github.com/openshift/library-go/blob/64780a4acea5ee3e565683ad96b1f4f0799f0bd2/pkg/crypto/crypto.go#L149-L180
https://github.com/openshift/library-go/blob/64780a4acea5ee3e565683ad96b1f4f0799f0bd2/pkg/crypto/crypto.go#L300-L314

So it is not trivial to check if a cipher name is valid. We don't want it to fail if users set one of the OpenShift's profiles, but they already contain unknown ciphers. So we would need to specifically allow them, even if they are not implemented.

What do you think?

As long as an empty slice that is not nil does not result in default values being used, I'm fine with it.

But strange that these profiles ship ciphers that are not supported. Are they supported in downstream distributions of golang?

internal/template-validator/tlsinfo/tlsinfo_test.go

0xFelix · 2024-11-08T09:49:58Z

internal/template-validator/tlsinfo/tlsinfo_test.go

-	})
+		It("should load certificate", func() {
+			Expect(tlsInfo.Init()).To(Succeed())
+			defer tlsInfo.Clean()


Does it make a difference using DeferCleanup here?

tests/crypto_policy_test.go

akrejcir added 4 commits October 23, 2024 16:08

chore: template-validator: Make 2 methods private

8a9eec1

These methods are not used from outside the package. Signed-off-by: Andrej Krejcir <[email protected]>

chore: validator: Use TLSInfo only when cert dir is specified

2d0a747

Do not create or use the struct when using HTTP. Signed-off-by: Andrej Krejcir <[email protected]>

feat: filewatch: Run callbacks before processing watch events

432908b

Runs all callbacks before processing watch events. This means that callbacks will have a change to notice the files after the watch was started, but no events happened yet. Signed-off-by: Andrej Krejcir <[email protected]>

kubevirt-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 1, 2024

kubevirt-bot requested a review from 0xFelix November 1, 2024 10:27

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Nov 1, 2024

kubevirt-bot requested a review from jcanocan November 1, 2024 10:27

kubevirt-bot added the size/XL label Nov 1, 2024

kubevirt-bot requested review from codingben and ksimon1 November 1, 2024 10:28

akrejcir force-pushed the validator-tls-config-map branch from 874c9bc to 520ceb4 Compare November 1, 2024 12:40

0xFelix reviewed Nov 4, 2024

View reviewed changes

akrejcir force-pushed the validator-tls-config-map branch from 520ceb4 to 6ef7c2f Compare November 5, 2024 11:39

0xFelix reviewed Nov 5, 2024

View reviewed changes

akrejcir added 6 commits November 7, 2024 11:32

feat: Use filewatch package in validator

799eea4

Using the filewatch package in template validator will make it easier to watch multiple directories in a future commit. Signed-off-by: Andrej Krejcir <[email protected]>

chore: validator: Set GetClientConfig() function of TLSConfig

d425b66

The GetClientConfig() will be used in future commit to set TLS options. Signed-off-by: Andrej Krejcir <[email protected]>

feat: validator: read TLS options from file

92e07c1

The TLS configuration is read from a ConfigMap that is mounted as a file. This allows updating the configuration without restarting the pod. Signed-off-by: Andrej Krejcir <[email protected]>

feat: Create ConfigMap for validator TLS options

3bfef3f

Signed-off-by: Andrej Krejcir <[email protected]>

cleanup: tests: TLS policy test functions return an error

d21bdfd

This will make it easier to modify the tests in future commit. Signed-off-by: Andrej Krejcir <[email protected]>

tests: Modify TLS policy tests to check validator

d1bdb12

Check TLS policy of template validator pod. Signed-off-by: Andrej Krejcir <[email protected]>

akrejcir force-pushed the validator-tls-config-map branch from 6ef7c2f to d1bdb12 Compare November 7, 2024 10:37

0xFelix reviewed Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: validator: Load TLS configuration from ConfigMap #1119

feat: validator: Load TLS configuration from ConfigMap #1119

akrejcir commented Nov 1, 2024

kubevirt-bot commented Nov 1, 2024

akrejcir commented Nov 1, 2024

codingben commented Nov 3, 2024

akrejcir commented Nov 4, 2024

0xFelix Nov 5, 2024

akrejcir Nov 6, 2024 •

edited

Loading

0xFelix Nov 8, 2024

akrejcir Nov 8, 2024

0xFelix Nov 11, 2024

akrejcir Nov 11, 2024

sonarcloud bot commented Nov 7, 2024

0xFelix Nov 8, 2024

akrejcir Nov 8, 2024 •

edited

Loading

akrejcir Nov 8, 2024 •

edited

Loading

0xFelix Nov 11, 2024

akrejcir Nov 11, 2024

akrejcir Nov 12, 2024 •

edited

Loading

0xFelix Nov 14, 2024

0xFelix Nov 8, 2024

feat: validator: Load TLS configuration from ConfigMap #1119

Are you sure you want to change the base?

feat: validator: Load TLS configuration from ConfigMap #1119

Conversation

akrejcir commented Nov 1, 2024

kubevirt-bot commented Nov 1, 2024

akrejcir commented Nov 1, 2024

codingben commented Nov 3, 2024

akrejcir commented Nov 4, 2024

Choose a reason for hiding this comment

akrejcir Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 7, 2024

Quality Gate passed

Choose a reason for hiding this comment

akrejcir Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

akrejcir Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrejcir Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrejcir Nov 6, 2024 •

edited

Loading

akrejcir Nov 8, 2024 •

edited

Loading

akrejcir Nov 8, 2024 •

edited

Loading

akrejcir Nov 12, 2024 •

edited

Loading