OpenStack client TLS configuration flags

[tuminoid]

/kind feature

Describe the solution you'd like
CAPO calls OpenStack API as client. That client connection is using TLSconfig that is only setting TLSminversion of TLS 1.2. As follow-up to TLS configuration flags for CAPO webhook I think it would be nice to make that client connection TLS configurable as well.

Anything else you would like to add:
Before implementing it, I'd like to hear how maintainers would like to see it done. Namely if the client TLS connections should honor the same configuration flags as webhooks (--tls-min-version, --tls-max-version), or if the client connection should have its own flags, something like --tls-client-min-version or --tls-min-version-client etc.

[EmilienM]

I don't have much opinions yet but in general for these things I like when it's consistent with other providers. I know you worked on another provider, maybe we can just propose the same pattern and see how it works for us?

[tuminoid]

I don't have much opinions yet but in general for these things I like when it's consistent with other providers. I know you worked on another provider, maybe we can just propose the same pattern and see how it works for us?

Issue here is that many/most k8s components are either server or client, but not both in the same code base, especially towards external systems that might be configured differently than the k8s components. Enforcing TLS 1.3 is a bit niche feature still, so quickly checking other providers the only ones having the configuration is the ones we've contributed it to so far. For these reasons I'd lean towards separate flags personally.

@mdbooth @lentzi90 your thoughts?

[lentzi90]

I'd say the flags should be separate. I would expect that the user does not have control over the OpenStack provider and will have to live with whatever TLS versions they support. Therefore we could easily have a situation where the client config is "forced" and I think we should then still allow the user to configure the webhook as they like.

[tuminoid]

I'd say the flags should be separate. I would expect that the user does not have control over the OpenStack provider and will have to live with whatever TLS versions they support. Therefore we could easily have a situation where the client config is "forced" and I think we should then still allow the user to configure the webhook as they like.

Thanks, I agree with this.

Follow-up: which wording would you prefer? --tls-client-min-version or --tls-min-version-client (or something more explicit like --tls-openstack-client-min-version)?

[lentzi90]

Before we jump to that I would like to understand it a bit better. We use a clouds.yaml file to configure part of this already and I wonder if that already has some fields for TLS config that we are just not using. It has at least something for CA and client certificates so not that far fetched I guess.

I got curious and did some digging. This is where the current configuration is taking place:

cluster-api-provider-openstack/pkg/scope/provider.go

Lines 225 to 282 in 4d81532

	func NewProviderClient(cloud clientconfig.Cloud, caCert []byte, logger logr.Logger) (*gophercloud.ProviderClient, *clientconfig.ClientOpts, string, error) {
	clientOpts := new(clientconfig.ClientOpts)
	if cloud.AuthInfo != nil {
	clientOpts.AuthInfo = cloud.AuthInfo
	clientOpts.AuthType = cloud.AuthType
	clientOpts.RegionName = cloud.RegionName
	clientOpts.EndpointType = cloud.EndpointType
	}
	opts, err := clientconfig.AuthOptions(clientOpts)
	if err != nil {
	return nil, nil, "", fmt.Errorf("auth option failed for cloud %v: %v", cloud.Cloud, err)
	}
	opts.AllowReauth = true
	provider, err := openstack.NewClient(opts.IdentityEndpoint)
	if err != nil {
	return nil, nil, "", fmt.Errorf("create providerClient err: %v", err)
	}
	ua := gophercloud.UserAgent{}
	ua.Prepend(fmt.Sprintf("cluster-api-provider-openstack/%s", version.Get().String()))
	provider.UserAgent = ua
	config := &tls.Config{
	MinVersion: tls.VersionTLS12,
	}
	if cloud.Verify != nil {
	config.InsecureSkipVerify = !*cloud.Verify
	}
	if caCert != nil {
	config.RootCAs = x509.NewCertPool()
	ok := config.RootCAs.AppendCertsFromPEM(caCert)
	if !ok {
	// If no certificates were successfully parsed, set RootCAs to nil to use the host's root CA
	config.RootCAs = nil
	}
	}
	provider.HTTPClient.Transport = &http.Transport{Proxy: http.ProxyFromEnvironment, TLSClientConfig: config}
	if klog.V(6).Enabled() {
	provider.HTTPClient.Transport = &osclient.RoundTripper{
	Rt: provider.HTTPClient.Transport,
	Logger: &gophercloudLogger{logger},
	}
	}
	err = openstack.Authenticate(provider, *opts)
	if err != nil {
	return nil, nil, "", fmt.Errorf("providerClient authentication err: %v", err)
	}
	projectID, err := getProjectIDFromAuthResult(provider.GetAuthResult())
	if err != nil {
	return nil, nil, "", err
	}
	return provider, clientOpts, projectID, nil
	}

[tuminoid]

That would've been desired and cleaner option, but unfortunately OS does not allow configuring TLS via clouds.yaml : os configuration show gives you what you can configure. OS in very reliant on what Python is doing with TLS and Python 3.7 or newer is basically always serving TLS 1.2/1.3 auto-negoatiate.

That said, default would stay as 1.2/1.3 auto-negotiate here, and only if the user knows their could can do TLS 1.3 and they wish to use only it, they could flip the switch in CAPO client too. We have some enterprise use-cases where this would be desired functionality.

Implementation wise, reading off the flags and placing them into tls.Config on L249 should be quite simple. We can reuse the testing from webhook TLS to verify the flag parsing. Maybe we could have some e2e test where TLS 1.3 would be enabled as well?

[lentzi90]

Sounds good to me. To make it perfectly clear, the flags would enforce the client configuration for all CAPO clusters, so it would not be possible to enforce TLS 1.3 for only a subset of clusters for example.

I don't have a strong opinion on the wording for the flags. Both options look good

[tuminoid]

On the second thought, it doesn't make a lot of sense to define these flags. While the actual implementation wouldn't be too complicated, testing this is annoying and the benefits are questionable:

1. If the target OS API supports TLS 1.3, current implementation already auto-negotiates TLS 1.3 connection
2. If the target OS API does not support TLS 1.3, forcing TLS 1.3 client does not make sense
3. Attacker cannot downgrade someone else's connection to TLS 1.2, only their own
4. Downgrading your own connection does not reveal anything server-side

Hence, I'm going to save the effort and close this.

/close

[k8s-ci-robot]

@tuminoid: Closing this issue.

In response to this:

On the second thought, it doesn't make a lot of sense to define these flags. While the actual implementation wouldn't be too complicated, testing this is annoying and the benefits are questionable:

1. If the target OS API supports TLS 1.3, current implementation already auto-negotiates TLS 1.3 connection
2. If the target OS API does not support TLS 1.3, forcing TLS 1.3 client does not make sense
3. Attacker cannot downgrade someone else's connection to TLS 1.2, only their own
4. Downgrading your own connection does not reveal anything server-side

Hence, I'm going to save the effort and close this.

/close

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.