trustee: Configure reference values based on all possible PCR combinations#116
trustee: Configure reference values based on all possible PCR combinations#116bgartzi wants to merge 13 commits into
Conversation
bgartzi
force-pushed
the
pcr_combination
branch
from
39cefcc to
0c7a05f
Compare
|
@bgartzi because the recent changes to compute-pcrs-lib are incompatible, I am ignoring it for dependabot or it keeps updating (see #121). Hoping that none of us forget it, reminder for you and myself to unignore it after this PR ( |
bgartzi
force-pushed
the
pcr_combination
branch
from
0c7a05f to
69d8614
Compare
bgartzi
force-pushed
the
pcr_combination
branch
from
69d8614 to
7a009f0
Compare
bgartzi
force-pushed
the
pcr_combination
branch
from
7a009f0 to
377abcc
Compare
|
Except for the flagged commit that I will remove as soon as the fix (trusted-execution-clusters/compute-pcrs#62) lands compute-pcrs and I update the pointer to the right lib version, I think this is finally ready for review. |
bgartzi
changed the title
|
Note that the followed approach just impacts the amount of reference values. It does not impact the amount of pcrs stored in the image-pcrs configmap. Although it gets the job done, it's not trivial to track where those reference values came from. Not sure if this is what you had in mind, or populating both rvs and image-pcrs with all possible combinations would be preferred. |
Jakob-Naucke
left a comment
Jakob-Naucke
left a comment
There was a problem hiding this comment.
Very nice. A few comments.
| [workspace.package] | ||
| edition = "2024" | ||
| rust-version = "1.85" | ||
| rust-version = "1.88" |
There was a problem hiding this comment.
We usually sync buildroot, so I'm open to moving to 1.92 right away.
There was a problem hiding this comment.
On this regard: I needed to also update some Containerfiles to build onto buildroot:fedora instead of just buildroot (which I think implicitly implied buildroot:latest).
bgartzi
force-pushed
the
pcr_combination
branch
from
377abcc to
78ffc8d
Compare
bgartzi
force-pushed
the
pcr_combination
branch
from
78ffc8d to
3332d06
Compare
|
Fixing latest conflicts... |
|
|
||
| ARG build_type | ||
| FROM ghcr.io/trusted-execution-clusters/buildroot AS builder | ||
| FROM ghcr.io/trusted-execution-clusters/buildroot:fedora AS builder |
There was a problem hiding this comment.
:latest and :fedora point to Fedora 43 images that are apparently different, but I don't even know how they're different. Is there a difference that matters to this PR?
There was a problem hiding this comment.
There was not, until I set the minimum rust version to 1.92: :latest fails to build as it still holds 1.91.
There was a problem hiding this comment.
Yes. Not for this PR but frankly we should yank a :latest that isn't being updated
bgartzi
force-pushed
the
pcr_combination
branch
from
3332d06 to
3b3b660
Compare
bgartzi
force-pushed
the
pcr_combination
branch
from
3b3b660 to
fb1321d
Compare
|
This version:
Still, the buildroot:latest vs buildroot:fedora discussion and research remains. |
|
|
||
| ARG build_type | ||
| FROM ghcr.io/trusted-execution-clusters/buildroot AS builder | ||
| FROM ghcr.io/trusted-execution-clusters/buildroot:fedora AS builder |
There was a problem hiding this comment.
Yes. Not for this PR but frankly we should yank a :latest that isn't being updated
|
Now extra approved images can be also pre-pulled, defaults to the secondary approved image pulled in the combination integration test and integration tests in github pre-pull images first. |
Jakob-Naucke
left a comment
Jakob-Naucke
left a comment
There was a problem hiding this comment.
urgh, two start failures in the test (probably not your fault) and a stop failure (fix is coming soon). I'll try and improve these soon.
| pull_approved_image "$APPROVED_IMAGE" $index | ||
|
|
||
| for image in $EXTRA_APPROVED_IMAGES; do |
There was a problem hiding this comment.
I'm not a fan of the extra env. I'd rather keep these out of the Makefile, put a const for the test image at the top of tests/trusted_execution_cluster.rs, and grep-sed for that here, WDYT?
| pull_approved_image "$APPROVED_IMAGE" $index | |
| for image in $EXTRA_APPROVED_IMAGES; do | |
| combined_pcrs_test_image=$(grep "const COMBINED_PCRS_TEST_IMAGE" tests/trusted_execution_cluster.rs | \ | |
| sed -E 's/.*"(.*)";/\1/') | |
| for image in "$APPROVED_IMAGE" "$combined_pcrs_test_image"; do |
There was a problem hiding this comment.
skipped the for, but I can put it there. You tell me.
Jakob-Naucke
left a comment
Jakob-Naucke
left a comment
There was a problem hiding this comment.
no, I like it. @alicefr PTAL too bc it's pretty big
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bgartzi, Jakob-Naucke 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 |
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: fmt.Sprintf("coreos-%d", i), |
There was a problem hiding this comment.
can we use something different then the index? maybe the instead the approvedImage with the / and :replaced by -. Or do we have a limit in the name lenght?
There was a problem hiding this comment.
Also coreos wasn't great by the way
| #[macro_export] | ||
| macro_rules! expected_pcr7_hash { | ||
| () => {{ "b3a56a06c03a65277d0a787fcabc1e293eaa5d6dd79398f2dda741f7b874c65d" }}; | ||
| } | ||
|
|
There was a problem hiding this comment.
One question, do we expect these constant to remain always the same or do we need to modify them for example when we upgrade the fedora version of the approved image?
There was a problem hiding this comment.
They might change with approved images, specially pcr4. If we'd like to parametrize this so we could hardcode this upper in the stack, such as in the makefile, the problem wouldn't be passing pcr values, but the events of each of them.
| named_test! { | ||
| async fn test_combined_image_pcrs_configmap_updates() -> anyhow::Result<()> { | ||
| let test_ctx = setup!([ | ||
| "quay.io/trusted-execution-clusters/fedora-coreos@sha256:372a5db90a8695fafc2869d438bacd7f0ef7fd84f63746a450bfcd4b8b64ae83", |
There was a problem hiding this comment.
Question, how do we upgrade this? Should it make sense to move the definition out of the test and put an env variable. Same question, how do we get the expected pcr value.
There was a problem hiding this comment.
I mean, it's just a second version for testing that combination works, we can probably leave it for a few years. It's not a moving target like FCOS versions.
There was a problem hiding this comment.
As soon as we want to write another test that handles 2 approved images and their upgrade path, yes. We can get ahead of time and put it that way if you want. I don't have a personal choice.
When it comes to upgrades, if we would like to change the image to a completely new one, we need to boot a VM with that image, check the tpm event log and look for shim, grub and kernel related events. Then we need to compute possible combinations, which include primary bootloader + secondary kernel and secondary bootloader + primary kernel.
There was a problem hiding this comment.
can we document in a comment how we build this image and describe how we extracted the pcrs values. At least, we won't forget it
| use trusted_cluster_operator_test_utils::constants::*; | ||
| use trusted_cluster_operator_test_utils::*; | ||
|
|
||
| const COMBINE_PCRS_UPDATE_TEST_IMAGE: &str = "quay.io/trusted-execution-clusters/fedora-coreos@sha256:372a5db90a8695fafc2869d438bacd7f0ef7fd84f63746a450bfcd4b8b64ae83"; |
There was a problem hiding this comment.
This is make sense to squash the definition of this image in the commit where this was introduced
There was a problem hiding this comment.
Did you also mean the pre-pull logic? Just the const definition, right?
|
@bgartzi I did a first pass, the only part that worries me is how we update the various hardcoded pcrs values and the second image when we upgrade the whole setup |
|
New changes are detected. LGTM label has been removed. |
|
still struggling to remember I have to sign commits sorry.
The problem is we could automate this using the compute-pcrs library somewhere in CI or makefile, but that would somehow break the purpose of the end to end test. If we really wish to test the whole stack, we need to boot a VM with the tested images, extract all pcr events and hashes, then update the values. We could automate it, but it is not precisely trivial. |
And update those crates that were making use of it. Later we will also use it in some other crates. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
I broke some of the compute-pcrs' lib APIs, so apart from just pointing into a newer commit, we also need to tweak a couple of things here and there. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
trustee tests were relying on some values that didn't hold any TPMEvent or any PCR value that related to them. This commit turns those dummy values into something that are closer to something we could expect in reality. The main reason to do this is that the compute-pcrs logic does not like empty event PCRs, as it's based on that to reconstruct them. This goes against the purpose of a unit test, as we should actually mock that external part so as not to rely on it, but decided this was simpler at least as an initial proposal. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
Update trustee reference values with all possible combinations between approved PCR values, not just those that come directly from image PCRs. This computes reference values for possible stages during node updates in which a node could be booting some components from image A and some other from image B. This commit also adds a test to check that pcr4 is well covered in front of bootloader and kernel updates. The test is closer to an integration test than to an unit-test as it relies on values that are close to real values, and how the compute-pcrs lib's combine_images processes them. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
Support the possibility of generating CRs for multiple approved images. These are stored sepparately in different yaml files. This changes the way trusted-cluster-gen names approved_image_cr.yaml files, as it's not just one, but many of them that can be marshalled. That's why test dependencies are updated to also be able to handle multiple approved image CRs. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
Refactor the logic that was verifying expected image pcrs in test_image_pcrs_configmap_updates as a method of TestContext. It also slightly changes the logic of the poller to stop when the length of pcrs equals the one expected. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
The compute pcrs job was computing pcr7 based on the guest OS information. However, PCR7 is mainly affected by the EFI variables, which are configured by the platform, or virtualization host. As we lack a proper way to retrieve the EFI vars of the platform that nodes are running on, we don't attest pcr7, so we should just not compute it. This commit removes the bits that computed pcr7 in the compute-pcrs job and those that checked it during integration tests. It leaves the test logic covering pcr7 in operator/src/trustee.rs as it should be agnostic to whatever pcr prediction comes in. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
These could be used by other tests too. Make it a macro so it can be reused, and define a few more macros for standard TPMEvents and PCR values that are constant across integration tests at the moment, such as PCR7 and PCR14. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
Add an integration test in which 2 approved images with different bootloader and kernel are added to the cluster. This emulates the situation in which a coreos image could be undergoing a bootloader and kernel update. The test checks that 2 images are added to the image pcr config map, and then checks that the reference values contain all possible pcr4 combinations. pcr7 and pcr14 are constant in this case, so there are not combinations possible (apart from the original value). Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
The logic checking that the image-pcrs configmap was populated moved into using await_condition instead of a poller. Some conditions were checked twice inside of the actual condition and subsequent assertions. This commit removes those assertions in favor of the await call. Moreover, it makes more sense from a stability point of view to wait for the whole condition to happen, as it could be the case that a Nth image is approved, and we run all the assertions (and fail) because only one of the N images was found in the configmap. This way, the tests will wait until the expected images of all approved images are present. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
Some integration tests might run on more approved images. Find which it is based on the test file then pre-pull it. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com> Assisted-By: Claude Opus 4.6 <noreply@anthropic.com>
This help not hitting some test timeouts due to images not being pulled quickly enough. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
Covering the primary and secondary approved images and also possible combinations between the two of them. Signed-off-by: Beñat Gartzia Arruabarrena <bgartzia@redhat.com>
|
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. |
4 participants
Now the compute-pcrs library supports computing PCR value combinations. In other words, given all events from image A and image B (where some of the components got updated), it computes all possible PCRs of the intermediate states that a node could go through during an update from image A to image B.
While implementing this I found a compute-pcrs bug that this PR relies on: trusted-execution-clusters/compute-pcrs#56
For now, I added a workaround commit that is not signed-off-by me. Once the compute-pcrs library gets merged, I will update this PR.
Another discussion topic that comes to my mind is how unit tests are implemented to face this new integration. They sure could be way more isolated from compute-pcrs, which I will update if relevant.