-
Notifications
You must be signed in to change notification settings - Fork 19
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
Support for IBM SE #39
Conversation
README.md
Outdated
type IbmSEConfigSpec struct { | ||
// configPvcName is the name of the PeristentVolumeClaim where certificates/keys are mounted | ||
// +optional | ||
ConfigPvcName string `json:"configPvcName,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be the name can be changed to reflect the usage as well.. eg CertStorePvc
accessModes: | ||
- ReadOnlyMany | ||
storageClassName: "" | ||
local: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how ReadOnlyMany
will be populated. It assumes the storage is pre-populated and then shared afaik. An admin can create a dynamic PVC as well if default storage class exists. In such a case it's unclear how the certs will be populated.
@huoqifeng can you please help with instructions on populating the required certs for SE on a non local storage based PV, ie not using a directory on the worker node.
Also have you considered alternative approaches. For example can we use a side car (or initContainer) to populate the required certs in a persistent volume.
Also, let's say for test env if PV is not used, then can we use the sidecar (or initContainer) approach to simply download the certs and place it in a well-defined path? (eg /opt/confidential-containers/ibmse)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bpradipt I did not try PV approach. The code does hint the materials need be populated beforehand.
I think initContainer/sidecar approach should be OK. the initContainer/sidecar can retrieve all the materials and put it on a shared folder with KBS container following https://github.com/confidential-containers/trustee/blob/main/deps/verifier/src/se/README.md#deployment-of-kbs-with-ibm-se-verifier, like /opt/confidential-containers/ibmse, and then start the KBS/AS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK downloading all the materials is not easy, each IBM node would require its specific keys to run (the certificates should be ok). This is why I was suggesting to decouple the process of keys/certs preparation from the actual trustee deployment.
What is the advantage of using a sidecar approach? Would you need to create a PV anyway and then copy contents to the containers, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK downloading all the materials is not easy, each IBM node would require its specific keys to run (the certificates should be ok).
I didn't get this. How is key materials on IBM nodes (I think you meant worker nodes here) be relevant to key materials on KBS side? I understand they need to be same, but the setup is entirely different. KBS just needs to have the key materials for all the worker nodes that it needs to validate. Am I missing something ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why I was suggesting to decouple the process of keys/certs preparation from the actual trustee deployment.
It needs to be de-coupled for sure. However trustee operator needs to have an opinionated way to use the certificates along with admin instructions on how to make those keys available to Trustee. Currently this not clear.
The PV example using the worker node path is making it strongly coupled to IBM nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get this. How is key materials on IBM nodes (I think you meant worker nodes here) be relevant to key materials on KBS side? I understand they need to be same, but the setup is entirely different. KBS just needs to have the key materials for all the worker nodes that it needs to validate. Am I missing something ?
I was mentioning the worker node as a convenient way to prepare a local directory for the required certs/keys to be mounted as a PV/PVC in kbs, just for test purposes. In a production environment, as you said, we could have a non-local storage PV instead. Yes, I think the kbs should have access to all certs/keys for all the worker nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be de-coupled for sure. However trustee operator needs to have an opinionated way to use the certificates along with admin instructions on how to make those keys available to Trustee. Currently this not clear.
The PV example using the worker node path is making it strongly coupled to IBM nodes.
In the current implementation, the trustee container just needs to mount a PVC (by name), no other dependencies on local dirs or the way the PV is created, the storage, etc.
What matters is that a PV is created by admin in advance with all the data trustee is expecting.
Would it be worth to mention the directory structure in the documentation?
For example:
├── certs
│ ├── ibm-z-host-key-signing-gen2.crt
| └── DigiCertCA.crt
├── crls
│ └── ibm-z-host-key-gen2.crl
│ └── DigiCertTrustedRootG4.crl
│ └── DigiCertTrustedG4CodeSigningRSA4096SHA3842021CA1.crl
├── hdr
│ └── hdr.bin
├── hkds
│ └── HKD-3931-0275D38.crt
└── rsa
├── encrypt_key.pem
└── encrypt_key.pub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's add these details in the readme. Even add the pv yaml example using local storage in the readme for IBM SE. The operator will simply expect a PVC name which is pre-populated with the required data.
New option in the KbsConfig for attaching to a PersistedVolumeClaim. The volume should be created manually by admin and it contains all the required IBM certificates and keys. Signed-off-by: Leonardo Milleri <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thanks @lmilleri
…references/main chore(deps): update konflux references Removes task sbom-json-check as requested for migration.
New option in the KbsConfig for attaching to a PersistedVolumeClaim. The volume should be created manually by admin and it contains all the required IBM certificates and keys.