WIP direct TLS connection to database service #306

dciabrin · 2023-09-05T09:23:04Z

This adds the ability to configure oslo.db/pymysql to connect to the database service over TLS.
It requires adding TLS options to bind-mount a CA that can validate the TLS certificate exposed by the database service.

openshift-ci · 2023-09-05T09:23:08Z

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

openshift-ci · 2023-09-05T09:23:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dciabrin
Once this PR has been reviewed and has the lgtm label, please assign abays for approval. 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

gibizer · 2023-09-06T16:37:34Z

pkg/keystone/initcontainer.go

@@ -52,6 +53,14 @@ func initContainer(init APIDetails) []corev1.Container {
 	envVars["DatabaseUser"] = env.SetValue(init.DatabaseUser)
 	envVars["DatabaseName"] = env.SetValue(init.DatabaseName)

+	var clientConfig string
+	if init.DatabaseUseTLS {


Sorry I have limited TLS knowledge.
What will happen If we don't set the CA cert in our mysql client config but the mysql server is configured with TLS?
a) the client will fail to connect as the servers TLS cert cannot be validated
b) the client will accept the servers TLS cert blindly

When a CA certificate is not specified in the MySQL client configuration, but the MySQL server is configured with TLS, two things can happen depending on client's configuration.

The client will fail to connect as the server's TLS certificate cannot be validated - This scenario will occur if the client is configured to enforce certificate validation. Without the CA certificate to verify the server's certificate, the client will reject the connection.

The client will accept the server's TLS certificate blindly - This will occur if the client is configured to allow connections without validating the server's certificate. The connection can be established, but it is insecure, because there is no way how to validate server's certificate without the CA, and therefore the connection can be compromised by man-in-the-middle.

So, both (a) and (b) can be true as MySQL client can be configured in either way (some client's services this don't provide, as far as I remember memcached do not accept server's TLS handshake, when there's no way how to validate its certificate).

@Deydra71 thanks for the explanation. Do we have preference over configuring the client for option a) or b) here?

The preference is to have CA certs available everywhere, so the certs can always be verified. So the option to accept TLS connection blindly is discouraged.

Sure, but this code now allows not to have CA configured for KeystoneAPI CR but have TLS certs on the mysql server. So my question here is: do we want keyston-operator to enforce option a) ? If so the I think we need to change this PR.

Enabling TLS in PyMySQL enforces certificate verification, I'm not sure that we can disable this verification.
I think ultimately, once all Openstack clients can connect to the database via TLS, when the galera CR is configured to use TLS, we should create database users with a 'REQUIRE SSL' grant that would only allow connection over TLS.

One potential issue is if keystone is not configured for TLS, but the database is and enforces connection over TLS. If so, keystone will never be able to connect.

Maybe for these two cases we could add a specific condition DatabaseReady like the MemcachedReady one? And give a specific error when a TLS config mismatch is detected by the keystone operator?

I think ultimately, once all Openstack clients can connect to the database via TLS, when the galera CR is configured to use TLS, we should create database users with a 'REQUIRE SSL' grant that would only allow connection over TLS.

I think this is a good idea.

One potential issue is if keystone is not configured for TLS, but the database is and enforces connection over TLS.

I think this is something that only the openstack-operator can check effectively as that is the only place where both the Gal;era and the KeystoneAPI CR is visible. We can add a validation webhook check there that rejects an OpenStackControlPlane CR that configures the Galera part with TLS but not the KeystoneAPI (and other service CR) part.

Maybe for these two cases we could add a specific condition DatabaseReady like the MemcachedReady one? And give a specific error when a TLS config mismatch is detected by the keystone operator?

Can keystone-operator really detect it? If so then sure we can add a condition for it.

gibizer · 2023-09-06T16:45:46Z

pkg/keystone/initcontainer.go

@@ -52,6 +53,14 @@ func initContainer(init APIDetails) []corev1.Container {
 	envVars["DatabaseUser"] = env.SetValue(init.DatabaseUser)
 	envVars["DatabaseName"] = env.SetValue(init.DatabaseName)

+	var clientConfig string
+	if init.DatabaseUseTLS {
+		clientConfig = "ssl=1\nssl-ca=/etc/ipa/ca.crt'"


This is a bit unreadable. I'm tempted ask for a my.cnf template that is rendered and mounted to the pod. Similarly how https://github.com/openstack-k8s-operators/keystone-operator/blob/main/templates/keystoneapi/config/keystone.conf is handled at

keystone-operator/controllers/keystoneapi_controller.go

Lines 912 to 921 in d73d07c

// ConfigMap

{

Name: fmt.Sprintf("%s-config-data", instance.Name),

Namespace: instance.Namespace,

Type: util.TemplateTypeConfig,

InstanceType: instance.Kind,

CustomData: customData,

ConfigOptions: templateParameters,

Labels: cmLabels,

},

Fair enough. I guess that template would ultimately be the same for all openstack clients? So we might want to have its skeleton in lib-common?

It can be lib-common or even mariadb-operator/api module. But I'm not 100% sure how can we store a template file there that is directly usable for the current templating mechanism. I think that is simply loads anything that is in the ./templates directory of the operator. So we have to make sure that the template file gets copied to bundle during build if it is not in ./templates.

gibizer · 2023-09-06T16:48:03Z

api/v1beta1/keystoneapi_types.go

+// TLSSpec defines the TLS options
+type TLSSpec struct {
+	// Secret in the same namespace containing the server private key (tls.key) and public cert (tls.crt) for TLS
+	SecretName string `json:"secretName,omitempty"`


Do I understand it correctly that

this is unused in the current PR.

it will be used later when we want to add a TLS cert for the keystone-api service.

and 2. are correct. I just copied this TLSSpec struct from what we have in @olliewalsh PoC in Add TLS support mariadb-operator#23 as this seems to be what other TLS PoC are based upon. This should probably be exposed in lib-common.

Cool. I agree that if this is a common struct then we can add it to lib-common to avoid repetition and eventual divergence.

I have questions on the generality of the struct:

do we ever need a single CA cert per service CR? I guess this will be the deployment internal CA as we do re-encryption at the routes. Am I correct that we never need to use in the pods some human operator provided CA along with our internal CA? I mean is it enough to have a single Secret for the CA cert?

Does every service CR requires only a single TLS cert? I hope we model one service per service CR so the answer is probably yes.

Ahh on 1) I think we have a complication. We have aggregate CRDs, like Nova CRD, that holds the config for multiple services. Like one NovaAPI and potentially one NovaMetadata per cell. So if we only ever need to use a single CA cert (our internal one) but we want TLS cert per service then Nova CRD needs a single CA cert field, but needs per service service cert field. So modelling TLSSpec as one CA cert and one service cert might not be reusable in the Nova CRD.

I think we have to be able that the human operator need to pass a custom CA on top of ours. e.g. if there is some storage backend using a custom CA outside of our management.

gibizer · 2023-09-06T16:57:51Z

controllers/keystoneapi_controller.go

+	// . To check whether the server is configured with TLS, look for
+	// for a secret CR that has a label `mariadb-ref` that references the
+	// server CR.


I think every operator needs to do this so:

lets move this logic to lib-common

enhance the MariaDBDatabase CR to make it simpler to decide if it is configured with TLS.
2.1. Could we look at the MariaDBDatabase.Spec.TLS.SecretName field to decide if the DB is configured with TLS or not?
2.2. An alternative would be to let the openstack-operator push down a flag to KeystoneAPI to indicate that the database is configured with TLS.

Yes fully agree.

I have a preference for 2.1 as it allows us to implement TLS for rabbit/memcached/redis incrementally, without requiring a new flag to be managed/passed by the openstack-operator

gibizer · 2023-09-06T16:58:38Z

controllers/keystoneapi_controller.go

+	}
+
+	if len(secretList.Items) > 0 {
+		return "ssl=1\nssl-ca=/etc/ipa/ca.crt'", nil


I lean towards asking for a tempalted my.cnf instead

gibizer · 2023-09-06T16:59:29Z

pkg/keystone/dbsync.go

 	initContainerDetails := APIDetails{
 		ContainerImage:       instance.Spec.ContainerImage,
 		DatabaseHost:         instance.Status.DatabaseHostname,
 		DatabaseUser:         instance.Spec.DatabaseUser,
 		DatabaseName:         DatabaseName,
+		DatabaseUseTLS:       instance.Spec.TLS.CaSecretName != "",


I guess this will change when getClientConfigForDatabase will be used

gibizer · 2023-09-06T17:01:12Z

pkg/keystone/volumes.go

+						Items: []corev1.KeyToPath{
+							{
+								Key:  "ca.crt",
+								Path: "ca.crt",
+							},


I'm wondering if we need this translation or if this is already the default behavior

gibizer · 2023-09-06T17:02:08Z

templates/keystoneapi/bin/init.sh

+# DB-specific config
+cat >${SVC_DB_CFG} <<EOF
+[client]
+${DBCLIENTCONFIG}
+EOF


I would be happier to move away from init.sh and instead render templates into CMs or Secrets in go and mount them to the pod directly.

Deydra71 · 2023-09-25T08:00:36Z

pkg/keystone/volumes.go

@@ -91,11 +105,17 @@ func getVolumes(name string) []corev1.Volume {
 		},
 	}

+	if instance.Spec.TLS != nil {
+		caVolumes := tls.CreateVolumes(instance.Spec.TLS)


Because of recent changes to the TLS structure, this line should be modified to: caVolumes := tls.CreateVolumes(&instance.Spec.TLS.Service, []tls.TLSCa{instance.Spec.TLS.Ca})

We're now utilizing two distinct structures:

TLSService for server-specific TLS settings and secrets

TLSCA for CA certificates

dciabrin · 2023-10-06T12:49:23Z

This PR now depends on openstack-k8s-operators/lib-common#357 to generate a mysql client config for keystone, as well as openstack-k8s-operators/mariadb-operator#119 for a getting a FQDN as the database host to validate TLS certificate.

gibizer

I have nothing major against this PR. It would be really nice to get envtest coverage for it though.

gibizer · 2023-10-09T14:17:50Z

controllers/keystoneapi_controller.go

+	if instance.Spec.TLS != nil {
+		mysqlTLSConfig = instance.Spec.TLS.CreateDatabaseClientConfig()
+	} else {
+		mysqlTLSConfig = ""


nit: This branch isn't needed mysqlTLSConfig is initalized to "" at L951 anyhow

This adds the ability to configure oslo.db/pymysql to connect to the database service over TLS. It requires adding TLS options to bind-mount a CA that can validate the TLS certificate exposed by the database service.

stuggi · 2024-03-13T16:22:28Z

closing this with #383 merged

openshift-ci bot added the do-not-merge/work-in-progress label Sep 5, 2023

dciabrin requested review from stuggi, Deydra71, gibizer and abays September 5, 2023 09:23

gibizer reviewed Sep 6, 2023

View reviewed changes

dciabrin force-pushed the keystone-tls-db branch from 05370df to b0669fb Compare September 18, 2023 08:55

openshift-merge-robot added the needs-rebase label Sep 18, 2023

Deydra71 reviewed Sep 25, 2023

View reviewed changes

dciabrin force-pushed the keystone-tls-db branch from b0669fb to 1e32653 Compare October 5, 2023 09:29

openshift-merge-robot removed the needs-rebase label Oct 5, 2023

dciabrin force-pushed the keystone-tls-db branch from 1e32653 to f5716a5 Compare October 5, 2023 20:11

openshift-merge-robot added the needs-rebase label Oct 5, 2023

dciabrin force-pushed the keystone-tls-db branch from f5716a5 to c6c3745 Compare October 5, 2023 20:25

openshift-merge-robot removed the needs-rebase label Oct 5, 2023

dciabrin force-pushed the keystone-tls-db branch from c6c3745 to 6acf310 Compare October 6, 2023 09:58

gibizer reviewed Oct 9, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label Oct 12, 2023

TLS connection to database service

0f03872

This adds the ability to configure oslo.db/pymysql to connect to the database service over TLS. It requires adding TLS options to bind-mount a CA that can validate the TLS certificate exposed by the database service.

dciabrin force-pushed the keystone-tls-db branch from 6acf310 to 0f03872 Compare November 20, 2023 10:13

openshift-merge-robot removed the needs-rebase label Nov 20, 2023

stuggi closed this Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP direct TLS connection to database service #306

WIP direct TLS connection to database service #306

dciabrin commented Sep 5, 2023

openshift-ci bot commented Sep 5, 2023

openshift-ci bot commented Sep 5, 2023

gibizer Sep 6, 2023

Deydra71 Sep 6, 2023

gibizer Sep 7, 2023

Deydra71 Sep 7, 2023

gibizer Sep 7, 2023

dciabrin Sep 7, 2023

gibizer Sep 13, 2023

gibizer Sep 6, 2023

dciabrin Sep 7, 2023

gibizer Sep 7, 2023 •

edited

Loading

gibizer Sep 6, 2023

dciabrin Sep 7, 2023

gibizer Sep 7, 2023

gibizer Sep 7, 2023

gibizer Sep 7, 2023

stuggi Sep 7, 2023

gibizer Sep 6, 2023

dciabrin Sep 7, 2023

gibizer Sep 6, 2023

gibizer Sep 6, 2023

gibizer Sep 6, 2023

gibizer Sep 6, 2023

Deydra71 Sep 25, 2023

dciabrin commented Oct 6, 2023

gibizer left a comment

gibizer Oct 9, 2023

stuggi commented Mar 13, 2024

	// ConfigMap
	{
	Name: fmt.Sprintf("%s-config-data", instance.Name),
	Namespace: instance.Namespace,
	Type: util.TemplateTypeConfig,
	InstanceType: instance.Kind,
	CustomData: customData,
	ConfigOptions: templateParameters,
	Labels: cmLabels,
	},

WIP direct TLS connection to database service #306

WIP direct TLS connection to database service #306

Conversation

dciabrin commented Sep 5, 2023

openshift-ci bot commented Sep 5, 2023

openshift-ci bot commented Sep 5, 2023

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

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

gibizer Sep 7, 2023 • 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

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

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

dciabrin commented Oct 6, 2023

gibizer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuggi commented Mar 13, 2024

gibizer Sep 7, 2023 •

edited

Loading