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 14 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
|
[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 |
|
@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. |
|
Now, I believe this is ready for another look @alicefr unless something in CI fails. @Jakob-Naucke you're also invited ofc. |
Bumps the rust group with 3 updates: [uuid](https://github.com/uuid-rs/uuid), [toml](https://github.com/toml-rs/toml) and [regex](https://github.com/rust-lang/regex). Updates `uuid` from 1.23.4 to 1.23.5 - [Release notes](https://github.com/uuid-rs/uuid/releases) - [Commits](uuid-rs/uuid@v1.23.4...v1.23.5) Updates `toml` from 1.1.2+spec-1.1.0 to 1.1.3+spec-1.1.0 - [Commits](toml-rs/toml@toml-v1.1.2...toml-v1.1.3) Updates `regex` from 1.12.4 to 1.13.0 - [Release notes](https://github.com/rust-lang/regex/releases) - [Changelog](https://github.com/rust-lang/regex/blob/master/CHANGELOG.md) - [Commits](rust-lang/regex@1.12.4...1.13.0) --- updated-dependencies: - dependency-name: uuid dependency-version: 1.23.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: rust - dependency-name: toml dependency-version: 1.1.3+spec-1.1.0 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: rust - dependency-name: regex dependency-version: 1.13.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: rust ... Signed-off-by: dependabot[bot] <support@github.com>
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. As we now deal with multiple images, and "coreos" is a bit to vague in that case, approved-images now accept both the image reference and a name, so we can identify them easily. Instead of calling "coreos" to the existing one, we now call it "coreos-approved-primary". 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>
|
Of course I moved some constant around and changed their name but forgot to update their reference in the document that covered them. Fixed now. |
|
Last failure was about kubevirt installation timing out. Trying again. |
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.