Skip to content

Conversation

@kukrimate
Copy link
Member

Local unit test runner is breaking for some reason, so submitting it just to test.

It should be done modulo I still need to add some tests that actually verify functionality with the NameAlg != SHA256

@kukrimate kukrimate marked this pull request as draft January 29, 2025 17:06
@chrisccoulson
Copy link
Collaborator

Ah, I think a few recent PRs of mine that landed today broke this and it looks like it needs a rebase. Also, for ease of review and because there's not really any dependencies between them, it might be better to base this on master rather than the other PR that you're working on. This one should be fairly quick to review and land :)

@kukrimate kukrimate force-pushed the name-alg branch 2 times, most recently from c08015a to 5bb979c Compare January 30, 2025 11:39
… keys

This is accomplished by adding the new field 'NameAlg' to the structure
'ProtectKeyParams'.

This means callers of these external APIs will need to fill in the new field
(affected tests):
 - NewExternalTPMProtectedKey (platform_test.go)
 - NewTPMProtectedKey (seal_test.go, update_test.go, platform_test.go)
 - NewTPMPassphraseProtectedKey (platform_test.go)

Signed-off-by: Mate Kukri <mate.kukri@canonical.com>
@kukrimate kukrimate marked this pull request as ready for review January 30, 2025 15:26
Copy link
Collaborator

@chrisccoulson chrisccoulson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I thought I'd reviewed this, but it turns out that I'd added some comments inline and then forgotten to submit the review.

I've deleted my comments now though as I have been thinking a bit more about whether this is the right thing to do. The preinstall checks will determine which is the best PCR bank to use, but the PCR bank is independent of the algorithm used for the final computed policy, which is defined by the object's name algorithm. Perhaps, rather than making the name algorithm selectable via a new field in ProtectKeyParams, and making it the same as the selected PCR bank, it might be better to automatically select the strongest out of SHA-256, SHA-384 or SHA-512 that is supported by the TPM. You could implement the selection in a new helper function, which could make use of TPMContext.IsAlgorithmSupported for testing the presence of each of the SHA2 digest algorithms in size order from largest to shortest, returning the first supported algorithm to use as the name algorithm. That should be fairly easy to write.

I need to have a think about how to test this though, especially as the current simulator we test against only has SHA256 enabled. I'm in the process of switching to a newer simulator which will have both SHA256 and SHA384 enabled, and in this case, it may be possible to test by manipulating the response with testutil.Transport.ResponseIntercept

NewExternalTPMProtectedKey won't be able to do this though because it has no TPM to access - in this case, it should just use the same algorithm that the supplied parent key uses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants