Make the pkcs11 config more generic #199

vakwetu · 2025-01-13T17:17:17Z

Made the pkcs11 more generic so that hopefully we will be able to support many HSMs and configurations without
too many changes.

Jira: https://issues.redhat.com/browse/OSPRH-12972

pkg/barbican/volumes.go

pkg/barbican/p11_prep.go

api/v1beta1/barbican_webhook.go

controllers/barbican_controller.go

api/v1beta1/common_types.go

templates/barbican/config/barbican-api-config.json

templates/barbican/config/barbican-p11-prep-config.json

api/v1beta1/common_types.go

controllers/barbican_controller.go

controllers/barbicanworker_controller.go

controllers/barbican_controller.go

controllers/barbicanworker_controller.go

controllers/barbicanapi_controller.go

controllers/barbicankeystonelistener_controller.go

controllers/barbicanapi_controller.go

stuggi · 2025-01-22T08:12:48Z

controllers/barbican_controller.go

 )

 var (
-	commonWatchFields = []string{
+	workerWatchFields = []string{


adding it here as you can not comment on not changed parts in the file. since this barbican controller renders the main config, it should also index/reconcile when the p11 secrets change. otherwise the config won't get updated. Needs that added in SetupWithManager

This is a larger question that this, I think.

The config that is generated in the top-level controller is passed down to the sub-component controllers (BarbicanAPI, BarbicanWorker, BarbicanKeystoneListener) - and the reconciliation that takes placed based on those watches is done in those components.

This is similar to what is done in cinder: https://github.com/search?q=repo%3Aopenstack-k8s-operators%2Fcinder-operator%20commonWatchFields&type=code

So, if there is a change in any of the watched things, then there will be a reconciliation triggered in the sub-components.

If there is something that is lacking here, then we need a separate change to reconcile all of the common secrets in the top-level controller. (not just pk11 secrets).

I'm not sure what effect that will have - and whether we'll end up with cascading reconciliations.
The top-level controller only initiates some setup jobs (dbsync, p11-prep). Not sure if we want to rerun these jobs when the config changes.

If something needs to change here, then we should open a Jira for this and address it separately. (And we'd probably need to fix it in cinder too!)

yes, lets follow up on this. here in barbican it is, right now, not an issue, since all controller render the global config, due to the link used for the templates directory. if the components would just consume the global config ,rendered by the main controller, and this one does not index/watch properly the input secret, the component controllers would reconcile, but not getting an updated rendered global config of the updated secret since the main controller did not reconcile. so something to keep in mind when removing the link as mentioned bellow, since there could be a situation where the user updated the config secret, but still using the old config since it has not rendered again.

pkg/barbican/volumes.go

templates/barbican/bin/generate_p11_keys.sh

templates/barbican/config/barbican-p11-prep-config.json

stuggi · 2025-01-22T09:57:27Z

api/v1beta1/common_types.go


+// BarbicanPKCS11Template - Includes common HSM properties
+type BarbicanPKCS11Template struct {


Do we need BarbicanPKCS11Template as part of the of the component api at all [11]. it would probably be enough to have it just on the barbican as we fetch the global config from there. But since this was done from beginning with other global parameters, I am not sure if you want to do such a change at this point for P11, we can not change it for the params which are there since GA.

[1] https://github.com/openstack-k8s-operators/barbican-operator/blob/main/api/v1beta1/common_types.go#L54C9-L54C15

I'm not entirely clear as to what you're proposing here, but yeah, I think we want to follow the pattern we've been using from the beginning.

just wanted to mention that the component controllers right now have parameters in their api which they probably won't need, since its a global thing would only required to be rendered in the main controller, where the sub components consume the global config for the generated secret.

stuggi · 2025-01-22T10:00:18Z

controllers/barbicanapi_controller.go

+		if err != nil {
+			return err
+		}
+		customData[barbican.CustomConfigFileName] = string(barbicanSecret.Data[barbican.CustomConfigFileName])


since we render and get the main config from the secret the barbican controller renders, I think we might want to consider to no longer use links for the component template directories? similar how it is done in cinder

$ ll templates/ barbican barbicanapi -> barbican barbicankeystonelistener -> barbican barbicanworker -> barbican

Otherwise we'd still render the configs and have the global one + the local rendered? if we make dedicated directories for each one and just have the component specific config as part of its directory, we won't have to check for the global secrets and set templateParameters.

agreed. We should do this. But I think we should do this as a separate change. That way we can focus on making sure that particular change doesn't break anything.

Lets open a separate Jira for this.

ack sounds good

api/v1beta1/common_types.go

api/v1beta1/barbican_webhook.go

api/v1beta1/common_types.go

config/samples/pods_with_pkcs11-luna.yaml

dmendiza · 2025-01-22T19:26:26Z

api/v1beta1/common_types.go

-	AlwaysSetCKASensitive bool `json:"alwaysSetCKASensitive"`
+        // +kubebuilder:validation:Required
+        // The OpenShift secret that stores the HSM client data.
+        // These will be mounted to /var/lib/config-data/hsm


Where does /var/lib/config-data/hsm come from?

Per design discussion, we will mount the data in a standard location and use kolla to move the data to the location designated by ClientDataPath.

The path we currently use for kolla stuff is /var/lib/config-data/default. This is just a branch close to this.

The reason for this is that if we mount the secret to ClientDataPath directly, then we end up overwriting whatever is already there. So, if we wanted to use /usr/local/luna is the client path, we'd end up deleting everything in that directory and only having the secrets there.

dmendiza · 2025-01-22T19:29:12Z

controllers/barbican_controller.go

+	}
+
+	// check for PKCS11 secret holding the PKCS11 Crypto Login
+	if slices.Contains(instance.Spec.EnabledSecretStores, barbicanv1beta1.SecretStorePKCS11) && instance.Spec.PKCS11 != nil {


Should we test here for len(instance.Spec.EnabledSecretStores) == 0 as above?

I don't think so. If instance.Spec.EnabledSecretStores is [], then the first term here will fail without throwing any error - as we'd expect.

The length check above is so that we default to simple_crypto in case EnabledSecretStores is undefined.

controllers/barbican_controller.go

config/samples/pods_with_pkcs11-luna.yaml

Modify the PKCS11 support to make it more generic and hopefully make it easier to then support different HSM configurations. Updated the PKCS11 functional tests. Fix tls-e kuttl tests

vakwetu · 2025-01-23T21:01:39Z

Just merged all the changes into a single commit to trigger new kuttl test run now that openshift/release#60903 has merged.

stuggi · 2025-01-27T07:30:42Z

/override ci/prow/precommit-check

openshift-ci · 2025-01-27T07:30:47Z

@stuggi: Overrode contexts on behalf of stuggi: ci/prow/precommit-check

In response to this:

/override ci/prow/precommit-check

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-sigs/prow repository.

stuggi

/lgtm lets address things we discussed in this PR and not yet in, in follow ups

openshift-ci · 2025-01-27T07:32:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stuggi, vakwetu

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [stuggi,vakwetu]

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

vakwetu requested review from dmendiza, stuggi and mauricioharley January 13, 2025 17:17

openshift-ci bot requested a review from afaranha January 13, 2025 17:17

openshift-ci bot added the approved label Jan 13, 2025

vakwetu force-pushed the simplify_luna_deploy branch from d035b77 to 6f7cd13 Compare January 13, 2025 17:27

stuggi reviewed Jan 14, 2025

View reviewed changes

vakwetu force-pushed the simplify_luna_deploy branch 2 times, most recently from 4f21104 to f3e693b Compare January 15, 2025 15:45

stuggi reviewed Jan 15, 2025

View reviewed changes

controllers/barbican_controller.go Outdated Show resolved Hide resolved

controllers/barbican_controller.go Outdated Show resolved Hide resolved

controllers/barbican_controller.go Outdated Show resolved Hide resolved

controllers/barbicanworker_controller.go Outdated Show resolved Hide resolved

vakwetu force-pushed the simplify_luna_deploy branch 2 times, most recently from 0e3b919 to 4cc913e Compare January 16, 2025 22:34

stuggi reviewed Jan 17, 2025

View reviewed changes

controllers/barbican_controller.go Show resolved Hide resolved

vakwetu force-pushed the simplify_luna_deploy branch from 4cc913e to 03e16f4 Compare January 20, 2025 21:06

vakwetu changed the title ~~Initial change to simplify the config~~ Make the pkcs11 config more generic Jan 20, 2025

vakwetu force-pushed the simplify_luna_deploy branch from 03e16f4 to a1fe6b5 Compare January 21, 2025 04:46

stuggi reviewed Jan 22, 2025

View reviewed changes

dmendiza reviewed Jan 22, 2025

View reviewed changes

config/samples/pods_with_pkcs11-luna.yaml Outdated Show resolved Hide resolved

config/samples/pods_with_pkcs11-luna.yaml Outdated Show resolved Hide resolved

vakwetu force-pushed the simplify_luna_deploy branch 2 times, most recently from 6c8fc91 to c16a257 Compare January 23, 2025 04:48

Make PKCS11 support more generic

772d803

Modify the PKCS11 support to make it more generic and hopefully make it easier to then support different HSM configurations. Updated the PKCS11 functional tests. Fix tls-e kuttl tests

vakwetu force-pushed the simplify_luna_deploy branch from c16a257 to 772d803 Compare January 23, 2025 20:59

stuggi approved these changes Jan 27, 2025

View reviewed changes

openshift-ci bot assigned stuggi Jan 27, 2025

openshift-ci bot added the lgtm label Jan 27, 2025

openshift-merge-bot bot merged commit cf83ce8 into openstack-k8s-operators:main Jan 27, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the pkcs11 config more generic #199

Make the pkcs11 config more generic #199

vakwetu commented Jan 13, 2025 •

edited

Loading

stuggi Jan 22, 2025

vakwetu Jan 22, 2025

stuggi Jan 23, 2025

stuggi Jan 22, 2025

vakwetu Jan 22, 2025

stuggi Jan 23, 2025

stuggi Jan 22, 2025

vakwetu Jan 22, 2025

vakwetu Jan 22, 2025

stuggi Jan 23, 2025

dmendiza Jan 22, 2025

vakwetu Jan 22, 2025

dmendiza Jan 22, 2025

vakwetu Jan 22, 2025

vakwetu commented Jan 23, 2025

stuggi commented Jan 27, 2025

openshift-ci bot commented Jan 27, 2025

stuggi left a comment

openshift-ci bot commented Jan 27, 2025


		// BarbicanPKCS11Template - Includes common HSM properties
		type BarbicanPKCS11Template struct {

Make the pkcs11 config more generic #199

Make the pkcs11 config more generic #199

Conversation

vakwetu commented Jan 13, 2025 • 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

vakwetu commented Jan 23, 2025

stuggi commented Jan 27, 2025

openshift-ci bot commented Jan 27, 2025

stuggi left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 27, 2025

vakwetu commented Jan 13, 2025 •

edited

Loading