trustee: Configure reference values based on all possible PCR combinations #116
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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bgartzi 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
force-pushed
the
pcr_combination
branch
from
69d8614 to
7a009f0
Compare
Minimum rust version was set to 1.85. Fedora is way above that threshold at the moment. Future EL releases will be above that as well. While on it, fix some of the linter errors that arise from the minimum version update. Signed-off-by: Beñat Gartzia Arruabarrena <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Modify the new TestContext function (and those below) to accept an arbitrary number of approved images for tests. In the setup! macro, hardcode the approved image that was used by tests until now. Also, add an alternate macro interface so it can accept an arbitrary numer of images. Signed-off-by: Beñat Gartzia Arruabarrena <[email protected]>
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 <[email protected]>
These could be used by other tests too. Make it a macro so it can be reused. Signed-off-by: Beñat Gartzia Arruabarrena <[email protected]>
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 <[email protected]>
It's impacted by a known compute-pcrs' combine logic bug: MokListTrusted
events are dropped during PCR combination. A fix is on its way:
trusted-execution-clusters/compute-pcrs#62
While it's merged, let's pretend the wrong pcr14 value is what we are
expecting just to get that green check.
bgartzi
force-pushed
the
pcr_combination
branch
from
7a009f0 to
377abcc
Compare
|
@bgartzi: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
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. |
3 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.