-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ek, ak abstractions: allow to specficy exact key type #546
ek, ak abstractions: allow to specficy exact key type #546
Conversation
I need to rebase it on #547 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. 👍
@THS-on could you review if this solves your use case reported in #414 (comment) ? 👋
9c5eea2
to
b51ce33
Compare
b51ce33
to
b4cdbda
Compare
@Superhepper I'll develop try to develop the fix for this issue against this branch and will report back. Thanks for backporting it! |
When using
Can we just add this to the |
Nah, I think it can be added. It has been added in the main branch probably just me who missed it. And the customization would work anyway. Because you can just overwrite it using the key customization. |
9d306a0
to
b5601bc
Compare
Now, I just need to figure out why these changes made the tests fail on Fedora .. |
b5601bc
to
0984f34
Compare
@Superhepper for reference, this is probably the change that we then also want to fully backport: #464 |
I will try to add that as well. |
01ac482
to
1e8633f
Compare
@THS-on If you missed it this PR has now been updated with the requested changes so it should work better now I hope. Please try it out. |
It now seems to work, but I get a miss match between the EK generated and in the EK cert on the Keylime verifier side. Still need to check where this actually goes wrong, somewhere between rust-tss-esapi, agent and verifier (which implements its own parser for TPM data structures). Once I checked that its not in rust-tss-esapi this PR can be merged 👍 |
I tried to write a simple example to compare the generated key with the one in the certificate, but I cannot figure out why it doesn't find the TryFrom for SubjectPublicKeyInfo. Which is kinda weird as in the Keylime codebase it just works.
|
I think you have jus
I just think you have forgotten to add |
Edit: issue seems to be somewhere in tss-esapi Old:
|
In my test script I only compared the bits and not the fully key. There seems to be still an issue in tss-esapi. Reproducer
Cargo.toml
Tested with libvrit VM using swtpm version 0.8.2. Validation that the EK and EK certificate actually match using tpm2-tools:
|
Found the issue. The hashAlg and authPolicy are different for each ECC curve (e.g. section B.4.6 of https://trustedcomputinggroup.org/wp-content/uploads/TCG-EK-Credential-Profile-V-2.5-R2_published.pdf). We hard code them to sha256. I'll create a PR probably next week that fixes this in the main branch that we then can backport. |
Changes are implemented in #552 |
I will add the changes #552 to this PR when I got the time. |
This takes the following PRs and from the main branch and adapts them so that they can be merged into the 7.x.y branch: \parallaxsecond#464 (By Ionut Mihalcea <[email protected]>) \parallaxsecond#414 (By Thore Sommer <[email protected]>) \parallaxsecond#552 (By Thore Sommer <[email protected]>) Co-authored-by: Jesper Brynolf <[email protected]> Co-authored-by: Thore Sommer <[email protected]> Co-authored-by: Ionut Mihalcea <[email protected]> Signed-off-by: Jesper Brynolf <[email protected]>
1e8633f
to
7c4dcee
Compare
@THS-on I have added your latest patch this one so please try it out again. |
@Superhepper currently fails with the following in Keylime:
I'll try to build a minimal reproducer. |
I will make a more thorough check to see if I messed something up when I added the latest commit yesterday. |
@THS-on - if you can share the code you used to get the above error, that would be good enough too. |
@Superhepper @ionut-arm sorry for the noise regarding this one. It has nothing to do with the rust bindings itself. Also for activate credential we need to set the policy accordingly for the upper profiles, we are currently are not: https://github.com/keylime/rust-keylime/blob/a3bfd5aa2c3ecea078ac0262821c3878cbdc8b94/keylime/src/tpm.rs#L1205 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
EccCurve::NistP256 => ( | ||
HashingAlgorithm::Sha256, | ||
Digest::try_from(AUTH_POLICY_A_SHA256.as_slice())?, | ||
SymmetricDefinitionObject::AES_128_CFB, | ||
32, | ||
), | ||
EccCurve::NistP384 => ( | ||
HashingAlgorithm::Sha384, | ||
Digest::try_from(AUTH_POLICY_B_SHA384.as_slice())?, | ||
SymmetricDefinitionObject::AES_256_CFB, | ||
0, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to cover PolicyA for P256 and PolicyB for P384+. What about PolicyB for P256 (and also RSA 2048)?
cc @THS-on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P256 and RSA 2048 are the default algorithms in the lower profile which uses PolicyA. Technically also a higher profile exists for them using PolicyB, but we have no way knowing which to pick based on the key size/curve alone.
This mimics the behavior also found in tpm2-tools
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated Keylime to set the correct policy for authorizing the EK (might make sense to put this also in as an abstraction in the future).
Now everything seems to work. LGTM! @Superhepper thanks for backporting it!
This adds support other keys than RSA2048 and ECC Nist P256.
This takes the following PRs and from the main
branch and adapts them so that they can be merged
into the 7.x.y branch:
#464 (By @ionut-arm )
#414 (By @THS-on )
#552 (By @THS-on )