Implement KBS API integration for LUKS key and AK management #242
Implement KBS API integration for LUKS key and AK management #242iroykaufman wants to merge 5 commits intotrusted-execution-clusters:mainfrom
Conversation
Signed-off-by: Roy Kaufman <rkaufman@redhat.com>
- Implemented `send_secret` function in `trustee.rs` to send secrets to the KBS API. - Updated `compute-pcrs-lib` dependency to a specific revision for stability. - Added installation of `perl-FindBin` and `perl-core` and update dependency. for kbs-client building requirements. Signed-off-by: Roy Kaufman <rkaufman@redhat.com>
Signed-off-by: Roy Kaufman <rkaufman@redhat.com>
Signed-off-by: Roy Kaufman <rkaufman@redhat.com>
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: iroykaufman The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
PR needs rebase. DetailsInstructions 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. |
| # In debug builds, build dependencies to avoid full rebuild. | ||
| RUN if [ "$build_type" = debug ]; then cargo build -p operator; fi | ||
|
|
||
| RUN dnf install -y perl-FindBin perl-core |
There was a problem hiding this comment.
Why do we need these dependecies?
There was a problem hiding this comment.
The openssl-sys fails when this dependency is missing. This is because I bumped all the dependency versions, the upstream uses openssl-sys 0.9.112, and here it is openssl-sys 0.9.113. This is the error message:
cargo:warning=configuring OpenSSL build: 'perl' reported failure with exit status: 2
cargo:warning=openssl-src: failed to build OpenSSL from source
--- stderr
Can't locate FindBin.pm in @INC (you may need to install the FindBin module) (@INC entries checked: /usr/local/lib64/perl5/5.42 /usr/local/share/perl5/5.42 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5) at ./Configure line 15.
BEGIN failed--compilation aborted at ./Configure line 15.
| .context("Secret missing root key")? | ||
| .0 | ||
| .clone(); | ||
| let url = format!("http://{TRUSTEE_SERVICE}:{TRUSTEE_PORT}"); |
There was a problem hiding this comment.
We definitely https here, when #196 is merged, you should test it with it
|
I would love to see some integration tests here that creates multiple machine and try to restart the trustee pod and see if the secrets are correctly present also after the restart. Also a test where an secret was present before the restart of the trustee pod and that after a secret is dynamicall added and then deleted. |
Signed-off-by: Roy Kaufman <rkaufman@redhat.com>
|
|
||
| [admin] | ||
| insecure_api = true | ||
| auth_public_key="/key/public.pub" |
There was a problem hiding this comment.
nit: space between the 2 string and the =
| serde_json.workspace = true | ||
| thiserror = "2.0.18" | ||
| tokio.workspace = true | ||
| kbs-client = {git = "https://github.com/iroykaufman/trustee/", branch = "ak-registration"} |
There was a problem hiding this comment.
Any reason for using your fork?
There was a problem hiding this comment.
Yes, currently registering AK using the KBS API is not supported. I created this PR#1306 to include support.
| .filter(|secret| { | ||
| secret.metadata.deletion_timestamp.is_none() | ||
| && secret | ||
| .metadata | ||
| .owner_references | ||
| .as_ref() | ||
| .map(|owners| owners.iter().any(|owner| owner.kind == "Machine")) | ||
| .unwrap_or(false) | ||
| }) |
There was a problem hiding this comment.
Instead of filtering by owner, we can introduce a label and mark the secret owned by a machine. In this way, the filtering is doing by kubernetes api
| .and_then(|s| s.replicas) | ||
| .unwrap_or(1); | ||
| let ready = status.ready_replicas.unwrap_or(0); | ||
| if ready >= desired && desired > 0 { |
There was a problem hiding this comment.
why do we need the desired? Cannot we simply evaluate ready > 1 ?
| let now = Utc::now().to_rfc3339(); | ||
| let patch = json!({ | ||
| "spec": { | ||
| "template": { | ||
| "metadata": { | ||
| "annotations": { | ||
| "kubectl.kubernetes.io/restartedAt": now | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
cannot we simply delete the kbs pod?
|
can we also test the deletion of a secret in one of the integration tests |
Currently, every time the LUKS key or the AK is updated, the operator patches the trustee deployment, which causes a restart of the pod. This PR introduces a way to avoid this by setting the AK and LUKS using the KBS API.
Core implementation points:
Tests: