search

kubernetes-sigs / cluster-api-provider-openstack

Cluster API implementation for OpenStack
https://cluster-api-openstack.sigs.k8s.io/
Apache License 2.0
289 stars 253 forks source link

Static analyser gets confused about minimum TLS version #2034

Closed mdbooth closed 5 months ago

mdbooth commented 5 months ago

/kind bug

The OpenShift build system is complaining at us because tls-min-version defaults to TLS 1.2, which has a bunch of known issues. I recommend we default it to TLS 1.3, same as the maximum. In all likelihood nobody is ever going to need to update this: what kube-apiserver isn't going to support TLS 1.3 these days?

If anybody did really need this there's an option so it remains possible to allow it.

Added in #1867

This wasn't the problem at all. The issue was that the static analyser saw that there was a code path (specifically the error path 🤦) where the variable which was used to set the TLS version was set to zero. Lets try to write something not too horrible which makes it easier for a static analyser to verify that tls version can't be set to any unacceptable value.

cc @tuminoid

tuminoid commented 5 months ago

Golang since 1.13 (properly since 1.14) has supported TLS 1.3, so some 4 years ago. That'd be taken in k8s 1.19 or so, long out of support by now. Any reasonable kubernetes installation that would take in new CAPO that has TLS 1.3 as default is zero. And you can configure it back to TLS 1.2 as you said.

OTOH, since kube-apiserver is TLS 1.3 enabled and CAPO is TLS 1.3 enabled, they will always negotiate a TLS 1.3 between them. Forcing TLS 1.3 as minimum affects only scenario where malicious user talks to CAPO webhook, and only accepts TLS 1.2. Not exactly sure what the risk here is but I also don't see what the harm would be in bumping the minimum.

Hence, 👍 from me. Would you like me to push a PR for this?

edit after I found the failure you were referring to: this complaint

 ✗ [Medium] Insecure TLS Configuration
   ID: dc3cc197-4289-49fb-9a65-14830b47ae6b 
   Path: main.go, line 391 
   Info: Insecure TLS configuration is found to be in use. MinVersion is set to a deprecated SSL/TLS version

I cannot find any new sources that TLS 1.2 would be insecure or deprecated. We have some vendors deprecating some TLS 1.2 cipher suites, and some vendors are considering dropping TLS 1.2 in future, but TLS 1.2 is not broken. For example, Microsoft Azure will enforce TLS 1.2 as minimum in the coming October. I don't think they'd be doing that if it was insecure.

I think the failure from OpenShift is false positive or they have really forward-looking tests.

MaysaMacedo commented 5 months ago

@tuminoid I'm working on it, should we assume that 1.3 is the only version supported?

tuminoid commented 5 months ago

No, we need to keep the flags, just change the default to min to 1.3, so it can be configured to accept 1.2 if needed.

Before changing it, it would be great to hear why OpenShift is flagging it as insecure. L391 is setting the min version based on config options, which explicitly warn if user enables insecure ciphers that are not enabled by default for TLS1.2 and do not allow TLS 1.1 which would actually be deprecated.

MaysaMacedo commented 5 months ago

@tuminoid So, in theory only this should have worked right? https://github.com/openshift/cluster-api-provider-openstack/pull/305/commits/480ac8bca46b95a40bd4b15b2ba5b66cc8b32d23

tuminoid commented 5 months ago

That does set TLS minimum to 1.3, so if it still triggers the failure, then it is a false positive. Like said in the previous comment, even our TLS 1.2 default should not trigger any failure IMO. Scanner in question seems to be Snyk, and the rule 83 does not give any indication what they expect.

mdbooth commented 5 months ago

@tuminoid It turns out that TLS 1.2 is fine. @pierreprinetti discovered what the static analyser was actually unhappy about. I've updated the description.

mdbooth commented 5 months ago

That does set TLS minimum to 1.3, so if it still triggers the failure, then it is a false positive. Like said in the previous comment, even our TLS 1.2 default should not trigger any failure IMO. Scanner in question seems to be Snyk, and the rule 83 does not give any indication what they expect.

Yeah, it's Snyk. Its output and documentation are both chocolate teapots.

pierreprinetti commented 5 months ago

That does set TLS minimum to 1.3, so if it still triggers the failure, then it is a false positive. Like said in the previous comment, even our TLS 1.2 default should not trigger any failure IMO. Scanner in question seems to be Snyk, and the rule 83 does not give any indication what they expect.

Yeah, it's Snyk. Its output and documentation are both chocolate teapots.

FYI I have opened a Snyk ticket with a link to the PR and a minimal reproducer

tuminoid commented 5 months ago

@tuminoid It turns out that TLS 1.2 is fine. @pierreprinetti discovered what the static analyser was actually unhappy about. I've updated the description.

OK, I can see why the static analyser is confused. In reality, you couldn't end up in a situation where you would've set 0 to TLSConfig. To me, that is still a false positive, but its good to get it fixed.

That said, your original point of having TLS 1.3 as default is still valid. Do you still plan to make that happen?