Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Barbican Support for Luna HSM #168

Conversation

mauricioharley
Copy link
Contributor

@mauricioharley mauricioharley commented Oct 9, 2024

This patch is a Work in Progress to support Thales Luna HSMs on Barbican v18.

It templates the Luna HSM configuration into Barbican variables.

api/v1beta1/common_types.go Outdated Show resolved Hide resolved
api/v1beta1/common_types.go Outdated Show resolved Hide resolved
api/v1beta1/common_types.go Outdated Show resolved Hide resolved
Copy link
Contributor

@vakwetu vakwetu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next step of course is to take what you have here and add to the relevant specs.

Then you need to find where the config files are generated/templated and make the changes. See https://github.com/openstack-k8s-operators/keystone-operator/pull/479/files# as an example of what Dave is doing on the federation side.

@mauricioharley mauricioharley changed the title Initial support for Thales Luna HSM Barbican Support for Luna HSM Oct 11, 2024
// +kubebuilder:validation:Optional
// The HSM certificates. The map's key is the OpenShift secret storing the certificate, and
// the value is the mounting point (e.g., "luna-certificates": "/usr/local/luna/config/certs").
HSMCertificates map[string]string `json:"hsmCertificates"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can there be multiple hsm certificates involved? because if I read the PR correct, only one would be mounted . If if only one cert secret is used, can't we just mount them to a fixed patch, instead of making the path configurable?
If we need it to be configurable, why not have a list of a sub struct type with secretName and mountPath ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also what is the expected content of the secret? specific keys for referencing cert/key?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been modified. We can have multiple certs but they will all be contained in a single secret - which will be mounted to a specific directory (mountpoint). The contents of the secret will be all the files that will be created in that directory.

api/v1beta1/barbican_types.go Outdated Show resolved Hide resolved
api/v1beta1/common_types.go Outdated Show resolved Hide resolved
api/v1beta1/common_types.go Show resolved Hide resolved
pkg/barbican/volumes.go Show resolved Hide resolved
@mauricioharley mauricioharley force-pushed the add_thales_luna_support branch 3 times, most recently from ef4921e to 434b386 Compare November 15, 2024 16:06
@vakwetu
Copy link
Contributor

vakwetu commented Nov 15, 2024

Looks like kuttl tests are passing. but the deploy-test there is failing while waiting on galera.

@vakwetu
Copy link
Contributor

vakwetu commented Nov 15, 2024

/test barbican-operator-build-deploy-kuttl

1 similar comment
@vakwetu
Copy link
Contributor

vakwetu commented Nov 18, 2024

/test barbican-operator-build-deploy-kuttl

@mauricioharley
Copy link
Contributor Author

/test barbican-operator-build-deploy-kuttl

@mauricioharley
Copy link
Contributor Author

/retest

@mauricioharley mauricioharley force-pushed the add_thales_luna_support branch 3 times, most recently from 2723ff1 to 693987b Compare December 2, 2024 19:39
@vakwetu
Copy link
Contributor

vakwetu commented Dec 2, 2024

/test barbican-operator-build-deploy-kuttl

hold-the-node
Signed-off-by: Mauricio Harley <[email protected]>
Co-authored-by: Ade Lee <[email protected]>
Co-authored-by: Grzegorz Grasza <[email protected]>
@xek xek force-pushed the add_thales_luna_support branch from 693987b to d84132c Compare December 4, 2024 11:42
@@ -16,6 +16,7 @@ for crd in config/crd/bases/*.yaml; do
mkdir -p "$(dirname "$TMP_DIR/$crd")"
git show "$BASE_REF:$crd" > "$TMP_DIR/$crd"
$CHECKER check-manifests \
--disabled-validators=NoBools,NoNewRequiredFields \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - so this was just for testing. We want to remove this and then explicitly waive the test failures.

Copy link
Contributor

@xek xek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 6, 2024
Copy link

openshift-ci bot commented Dec 6, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mauricioharley, xek

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:

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

@openshift-merge-bot openshift-merge-bot bot merged commit cc2b30a into openstack-k8s-operators:main Dec 6, 2024
6 checks passed
@vakwetu
Copy link
Contributor

vakwetu commented Dec 6, 2024

Just to add a comment here about the CRD checker.

This patch was accidentally merged without removing the changes to the crd-schema-checker. We will re-enable them in a follow-on commit.

The checker flagged a bunch of bools that were added in the pkcs11 spec. These bools are in fact really bools that would be set as such in the barbican.conf file. So the errors here would have been waived.

Further, the checker flagged a bunch of new required fields in the PKCS11 struct. This is OK because the PKCS11 struct itself is new and it is included into our existing structs as an optional pointer to a PKCS11 struct. Existing CRs will not have this field defined, and upon update, will instantiate that pointer as a nil value. Which won't trigger any issues with required fields in the struct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants