Compute PCRs #12

Jakob-Naucke · 2025-08-21T13:39:55Z

Run job to run confidential-clusters/compute-pcrs. Supersedes the reference value input workflow. Hard-coded for FCOS 42. Supersedes #11.

@alicefr

Permission might be overly permissive -- any hints or is this fine?
We could move to an official registry for the compute-pcrs image after merge of Add Containerfile compute-pcrs#19.

cc @bgartzi

alicefr · 2025-08-25T06:56:03Z

operator/src/reference_values.rs

+use crate::macros::info_if_exists;
+
+const BOOT_IMAGE: &str = "quay.io/fedora/fedora-coreos:42.20250705.3.0";
+const COMPUTE_IMAGE: &str = "quay.io/jnaucke/compute-pcrs:latest";


We should probably build the image as part of this repo

alicefr · 2025-08-25T07:03:33Z

@Jakob-Naucke instead of reading the job pod output what about if we include the compute-pcrs binary in a new container image, and you add a new binary which launches the calculation and then update the config maps of trustee? I think it will be a cleaner

Jakob-Naucke · 2025-08-25T07:18:53Z

add a new binary which launches the calculation and then update the config maps of trustee? I think it will be a cleaner

Hmm, I have two concerns about it being cleaner.

new binary

Extra Rust crate or jq-manglery in Shell? Neither seems very low friction.

update the config maps

Is API access nearly as easy as what we have from the operator?

alicefr · 2025-08-25T07:24:10Z

add a new binary which launches the calculation and then update the config maps of trustee? I think it will be a cleaner

Hmm, I have two concerns about it being cleaner.

new binary

Extra Rust crate or jq-manglery in Shell? Neither seems very low friction.

Well, if we convert the compute-pcrs into a library, it removes all those frictions. IMO, a library what we should get from the compute-pcrs repository. It should also pretty straightforward to convert the binary into a library, and then the new binary can rely on this library.

update the config maps

Is API access nearly as easy as what we have from the operator?

Well, the operator would need to fetch the logs and patch the reference values config maps, so why not let the job doing it immediately

Jakob-Naucke · 2025-08-25T07:31:51Z

Extra Rust crate or jq-manglery in Shell? Neither seems very low friction.

Well, if we convert the compute-pcrs into a library, it removes all those frictions. IMO, a library what we should get from the compute-pcrs repository. It should also pretty straightforward to convert the binary into a library, and then the new binary can rely on this library.

oh yeah that's cleaner indeed. Didn't realize you meant that based on

include the compute-pcrs binary in a new container image

Is API access nearly as easy as what we have from the operator?

Well, the operator would need to fetch the logs and patch the reference values config maps, so why not let the job doing it immediately

What I meant was: The job needs all the k8s API interaction logic again. That's probably fine though.

alicefr · 2025-08-25T07:36:41Z

Is API access nearly as easy as what we have from the operator?

Well, the operator would need to fetch the logs and patch the reference values config maps, so why not let the job doing it immediately

What I meant was: The job needs all the k8s API interaction logic again. That's probably fine though.

Yes, it requires the k8s client and the rbac for updating the config map. But it is a self-contained job with a clear scope, so I personally prefer it rather then parsing the pod output.
You could also let the job create a config map in the operator with the reference values and then the operator update the reference values in the trustee namespace, but imo, it is a bit overkilled.

bgartzi · 2025-08-25T08:25:30Z

@alicefr:

Well, if we convert the compute-pcrs into a library, it removes all those frictions. IMO, a library what we should get from the compute-pcrs repository. It should also pretty straightforward to convert the binary into a library, and then the new binary can rely on this library.

The source code is already divided into cli and lib. Is that what you meant? Is there something else we should do on the compute-pcrs side?

Jakob-Naucke · 2025-08-25T09:45:53Z

The source code is already divided into cli and lib. Is that what you meant? Is there something else we should do on the compute-pcrs side?

I plan to make a new binary crate that uses compute-pcrs-lib. I think that side is fine as is.

Jakob-Naucke · 2025-08-26T13:49:43Z

untested, pushed to the wrong remote, bound to happen. converting to draft. sorry for the noise.

Jakob-Naucke · 2025-08-26T15:51:38Z

@alicefr now tested and ready for review, comment on permissions still standing though

alicefr · 2025-08-27T06:56:33Z

Makefile

 REGISTRY ?= quay.io
 OPERATOR_IMAGE=$(REGISTRY)/confidential-clusters/cocl-operator:latest
+COMPUTE_PCRS_IMAGE=$(REGISTRY)/confidential-clusters/compute-pcrs:latest
+PCRS_BOOT_IMAGE=quay.io/fedora/fedora-coreos:42.20250705.3.0


I wouldn't put this image as part of the manifests. For now, it is fine to have it hardcoded, but it is something we need to infer from the cluster

Are you saying

cluster inference should be implemented before merge, or

we can merge this but it will need to be changed, or

cluster inference is not necessary now, but even then we should have something else (what?)?

this logic will be part of the operator not the manifests. So, I would avoid to put this as part of the manifests generation. You can, for now, just hardcode the fedora version in the operator

alicefr · 2025-08-27T06:56:43Z

Makefile

-		--trustee-namespace operators
+		--trustee-namespace operators \
+		--pcrs-compute-image $(COMPUTE_PCRS_IMAGE) \
+		--pcrs-boot-image $(PCRS_BOOT_IMAGE)


As comment as above

alicefr · 2025-08-27T07:01:04Z

compute-pcrs/Containerfile

+# Hack: Set compute-pcrs as sole member to avoid needing to copy other crates.
+# In that case, a rebuild would be triggered upon any change in those crates.
+RUN sed -i 's/members =.*/members = ["compute-pcrs"]/' Cargo.toml && \


cannot you copy the Cargo.toml from compute-pcrs instead?

No, it has workspace dependencies and I prefer workspace dependencies over potentially deviating versions

alicefr · 2025-08-27T07:08:29Z

compute-pcrs/Containerfile

+    "--efivars", "/reference-values/efivars/qemu-ovmf/fcos-42", \
+    "--mokvars", "/reference-values/mok-variables/fcos-42"]


nit: usually the args are specified at container creation

no this is intentional. These arguments are dependent on the path of reference-values, which is "known" in this Containerfile, not when the other arguments are fed.

Kubernetes overwrites the entrypoint in any case. What I don't like is that there is fcos-42 hardcoded there

Kubernetes overwrites the entrypoint in any case.

Not when you use args instead of command 🙂

What I don't like is that there is fcos-42 hardcoded there

I can see that. I'm considering adding a "reference values base directory" flag to the compute-pcrs binary though so this information isn't spread out. Or we clone with an init container. WDYT?

Has a further decision been taken on this topic? I see this is one of the only references to reference-values/mok-variables/fcos-42 which I would like to update to reference-values/mok-variables/fedora-42 for the sake of simplicity.

Are we safe if I move trusted-execution-clusters/reference-values#3 on?

@bgartzi go ahead.

alicefr · 2025-08-27T07:09:41Z

compute-pcrs/Containerfile

+    git clone --depth 1 https://github.com/confidential-clusters/reference-values && \
+    cargo build --release -p compute-pcrs
+
+FROM docker.io/library/debian:trixie


can we use fedora as base image?

Hmm, I used Debian because docker.io/library/rust only has Debian (or Alpine) as base and a Debian base will be 100% ABI compatible for execution. I can check if Fedora works though (or base the build container on Fedora too, probably requires an explicit Rust installation).

Well, it can be built on the debian rust image, but you should copy it in a fedora base image. Or are you afraid that the dynamic library won't match?

Or are you afraid that the dynamic library won't match?

Yes I was, but maybe it's fine. I'll test.

Nobody asked, but, would a similar approach to the one proposed in trusted-execution-clusters/compute-pcrs/pull/19 work for this?

i.e. fedora as builder, install needed dependencies (a subset of the compute-pcrs https://github.com/confidential-clusters/compute-pcrs/blob/main/.github/Containerfile.buildroot), build binary, then copy the binary to a clean fedora image?

or use compute-pcrs's image? 🙃

alicefr · 2025-08-27T07:14:35Z

compute-pcrs/src/main.rs

+    let client = Client::try_default().await?;
+    let config_maps: Api<ConfigMap> = Api::namespaced(client, &args.namespace);
+    match config_maps
+        .create(&PostParams::default(), &config_map)
+        .await
+    {
+        Ok(_) => info!("Create ConfigMap {}", args.configmap),
+        Err(kube::Error::Api(ae)) if ae.code == 409 => {
+            info!("ConfigMap {} already exists", args.configmap)
+        }
+        Err(e) => return Err(e.into()),
+    }


what if the config map already exists? You probably wants to retrieve its value check if it is different from the reference values, and if not then not update it. Right now, it make little sense but when we have more coreos versions to handle then the logic will become useful

as per #13, the RVs will be computed statelessly, and the config map will be overwritten. the code that I wrote for this which I momentarily refuse to delete is at Jakob-Naucke:shelved-append-rvs. I'm in favor of merging this PR first and moving on from there.

alicefr · 2025-08-27T07:17:12Z

manifest-gen/src/main.rs

+    )]
+    pcrs_compute_image: String,
+
+    #[arg(long, default_value = "quay.io/fedora/fedora-coreos:42.20250705.3.0")]


I would remove this and hardcoded directly in the compute-pcrs. We will then add a logic to detect the coreos version to calculate

hardcoded directly in the compute-pcrs

This info is required when defining the container and its image volume, it's nothing that compute-pcrs lib/bin/image can influence

Yes, but the job with the image volume is created by the operator, so you don't need this in the manifests

alicefr · 2025-08-27T07:20:57Z

operator/src/trustee.rs

-            name: Some(name.to_string()),
-            namespace: Some(namespace.to_string()),
+    let pod_spec = PodSpec {
+        service_account_name: Some("cocl-operator".to_string()),


I would create a separate service account for the job only with a separate Role to only be able to create and modify the config maps in the trustee namespace

alicefr · 2025-08-27T07:22:41Z

@Jakob-Naucke as far as it regards the permission, I think you are referring to job RBAC, right? I think you should split the permission for the job only as I mentioned already here

alicefr · 2025-09-02T06:16:09Z

scripts/clean-cluster-kind.sh

 	fi
 done
 kubectl delete deploy cocl-operator -n confidential-clusters || true
+kubectl delete job compute-pcrs -n confidential-clusters || true


I think should be handled by the operator

If the job completes, the operator can then remove it

alicefr · 2025-09-03T06:25:24Z

operator/src/trustee.rs

+    let create = jobs.create(&PostParams::default(), &job).await;
+    info_if_exists!(create, "Job", job_name);
+    let completed = await_condition(jobs.clone(), job_name, is_job_completed());
+    let _ = timeout(Duration::from_secs(900), completed).await?;


Is this blocking until it completes?
The usual way in kubernetes is watching objects with a controller and then trigger a reconciliation loop if there is a change in the state. Can we implement something like this here? See the rust documentation for the Controller

Signed-off-by: Jakob Naucke <jnaucke@redhat.com>

Signed-off-by: Jakob Naucke <jnaucke@redhat.com> Based-on-patch-by: Alice Frosi <alicefr@redhat.com>

Signed-off-by: Jakob Naucke <jnaucke@redhat.com> Based-on-patch-by: Alice Frosi <afrosi@redhat.com>

in place of ClusterRoleBindings Signed-off-by: Jakob Naucke <jnaucke@redhat.com> Based-on-patch-by: Alice Frosi <afrosi@redhat.com>

and some extra dependency cleanups Signed-off-by: Jakob Naucke <jnaucke@redhat.com>

Create new binary to use confidential-clusters/compute-pcrs-lib and write the configmap. Build image with it and extend cocl spec with the respective image fields. Run with a job. Supersedes the reference value input workflow. Signed-off-by: Jakob Naucke <jnaucke@redhat.com>

alicefr · 2025-09-11T07:01:30Z

operator/src/trustee.rs

+    Action::requeue(Duration::from_secs(60))
+}
+
+pub async fn launch_rv_job_controller(client: Client, namespace: &str) {


Very nice, yes I meant exactly this, thx!

Jakob-Naucke mentioned this pull request Aug 21, 2025

Add reference-values-in.json file #11

Merged

alicefr reviewed Aug 25, 2025

View reviewed changes

Jakob-Naucke force-pushed the compute-pcrs branch from 8a82c23 to e4efa66 Compare August 26, 2025 13:48

Jakob-Naucke marked this pull request as draft August 26, 2025 13:49

Jakob-Naucke force-pushed the compute-pcrs branch from e4efa66 to d26af80 Compare August 26, 2025 15:50

Jakob-Naucke marked this pull request as ready for review August 26, 2025 15:51

alicefr reviewed Aug 27, 2025

View reviewed changes

travier mentioned this pull request Aug 27, 2025

Design / flow for reference values #13

Open

Jakob-Naucke force-pushed the compute-pcrs branch 2 times, most recently from ef749e3 to e517c6c Compare August 28, 2025 12:48

Jakob-Naucke requested a review from alicefr August 28, 2025 15:10

Jakob-Naucke mentioned this pull request Aug 29, 2025

Compute PCRs, using labels and caches #14

Merged

alicefr reviewed Sep 2, 2025

View reviewed changes

Jakob-Naucke force-pushed the compute-pcrs branch from e517c6c to 2b06f5d Compare September 2, 2025 15:25

Jakob-Naucke requested a review from alicefr September 2, 2025 15:25

alicefr reviewed Sep 3, 2025

View reviewed changes

bgartzi mentioned this pull request Sep 8, 2025

fedora: Rename directories from fcos to fedora trusted-execution-clusters/reference-values#3

Merged

Jakob-Naucke force-pushed the compute-pcrs branch from 2b06f5d to bd7749d Compare September 9, 2025 17:25

Jakob-Naucke requested a review from alicefr September 9, 2025 17:25

Jakob-Naucke and others added 6 commits September 9, 2025 19:29

nit: Run cargo clippy some more

862f0dc

Signed-off-by: Jakob Naucke <jnaucke@redhat.com>

Namespace in operator::main arguments

35d1b2d

Signed-off-by: Jakob Naucke <jnaucke@redhat.com> Based-on-patch-by: Alice Frosi <alicefr@redhat.com>

Prepare Makefile for more than one image

8ab04d6

Signed-off-by: Jakob Naucke <jnaucke@redhat.com> Based-on-patch-by: Alice Frosi <afrosi@redhat.com>

Move to RoleBindings

99bfe29

in place of ClusterRoleBindings Signed-off-by: Jakob Naucke <jnaucke@redhat.com> Based-on-patch-by: Alice Frosi <afrosi@redhat.com>

Workspace dependencies

fc1b803

and some extra dependency cleanups Signed-off-by: Jakob Naucke <jnaucke@redhat.com>

Jakob-Naucke force-pushed the compute-pcrs branch from bd7749d to a5ed684 Compare September 9, 2025 17:33

alicefr reviewed Sep 11, 2025

View reviewed changes

alicefr merged commit ed73907 into trusted-execution-clusters:main Sep 11, 2025
5 checks passed

Jakob-Naucke deleted the compute-pcrs branch September 11, 2025 07:22

		"--efivars", "/reference-values/efivars/qemu-ovmf/fcos-42", \
		"--mokvars", "/reference-values/mok-variables/fcos-42"]

Compute PCRs #12

Compute PCRs #12

Uh oh!

Conversation

Jakob-Naucke commented Aug 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alicefr commented Aug 25, 2025

Uh oh!

Jakob-Naucke commented Aug 25, 2025 • edited by alicefr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alicefr commented Aug 25, 2025

Uh oh!

Jakob-Naucke commented Aug 25, 2025

Uh oh!

alicefr commented Aug 25, 2025

Uh oh!

bgartzi commented Aug 25, 2025

Uh oh!

Jakob-Naucke commented Aug 25, 2025

Uh oh!

Jakob-Naucke commented Aug 26, 2025

Uh oh!

Jakob-Naucke commented Aug 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alicefr commented Aug 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Jakob-Naucke commented Aug 25, 2025 •

edited by alicefr

Loading