Support ECC storage keys in create_activation - #7
Merged
Conversation
TPMT_PUBLIC::create_activation rejected anything that was not an RSA key, so a TPM whose endorsement key is ECC could not be issued a credential at all. This adds the ECC path. The two algorithms differ only in how the seed reaches the TPM, which is now isolated in produce_seed. With RSA the seed is random and the secret is that seed under OAEP. With ECC nothing is transported: both sides derive the seed from an ECDH agreement, and the secret is the ephemeral public point the TPM needs to repeat it. Everything after the seed is defined on the seed alone and is shared unchanged. The crypto layer gains KDFe, the SP800-56A concatenation KDF that TPM 2.0 Part 1 section 11.4.10.3 defines for this derivation. It is easy to confuse with KDFa: it hashes rather than HMACs, and it does not hash the requested length. Getting either wrong yields output of the right shape and the wrong value. Only the ephemeral agreement is delegated to the provider, and it returns the raw agreed value. Deriving from it is specification behaviour rather than backend behaviour, so it stays in Crypto. Some platform APIs offer to perform the concatenation themselves, but at least one silently ignores the requested hash algorithm and always uses SHA-256, which no interoperability failure would attribute to the KDF. Coordinate width is answered by Crypto rather than by a provider, because a TPM may drop leading zero bytes from a coordinate it marshals into a TPM2B while KDFe hashes coordinates at full width. A curve outside the TCG registry is an error rather than a guess, since guessing produces a plausible key that simply does not match the peer's. The software provider implements the agreement over P-256, P-384 and P-521. Barreto-Naehrig and SM2 curves are rejected rather than approximated with a NIST curve. Tested by a round trip against a stand-in that repeats the agreement, on three curves across three nameAlgs. Mutation testing confirms the round trip fails if partyU and partyV are swapped, or if the KDFe label is changed, on one side only. It cannot catch a mistake made identically on both sides, since both ends call the same KDFe, so KDFe is additionally pinned to an independently computed vector.
There was a problem hiding this comment.
Pull request overview
Adds ECC storage-key support to Rust credential activation by introducing ECDH seed agreement and TPM KDFe derivation while preserving the existing RSA flow.
Changes:
- Refactors activation seed production into shared RSA/ECC handling.
- Adds ECC provider APIs and RustCrypto implementations for P-256, P-384, and P-521.
- Adds KDFe, curve-coordinate handling, dependencies, and ECC activation tests.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
TSS.Rust/src/tpm_type_extensions.rs |
Adds ECC activation seed production and round-trip tests. |
TSS.Rust/src/crypto/software_provider.rs |
Implements ephemeral ECDH for supported NIST curves. |
TSS.Rust/src/crypto/provider.rs |
Extends the provider contract with ECC operations. |
TSS.Rust/src/crypto/mod.rs |
Adds ECC helpers and KDFe derivation. |
TSS.Rust/Cargo.toml |
Adds optional RustCrypto ECC dependencies. |
TSS.Rust/Cargo.lock |
Locks the new ECC dependency graph. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Four defects found in review of the previous commit, two of them behavioural and two in what the code claims about itself. KDFa and KDFe are specified in bits but truncated to whole octets, so a request that is not a multiple of eight returned too many significant bits instead of the leftmost bits right aligned. TSS.NET and TSS.CPP shift the stream to correct this. Both KDFs now share one helper rather than being fixed apart, since the two disagreeing about what a bit count means is worse than the original bug. KDFa was affected identically and is fixed here even though the previous commit did not touch it. This is unreachable through TPM 2.0 itself, where every KDF request is a digest or symmetric key size and therefore already octet aligned. It is fixed so that a caller using the library directly agrees with the other stacks. The shift is applied to the retained octets rather than to the whole stream, which is equivalent because the octets a full shift would produce beyond the requested length are discarded in either order. Where the request is octet aligned the operation is byte for byte the previous truncation, confirmed by forcing the shift to zero and observing that only the new test fails. The agreed ECDH value was held in an ordinary Vec and printed by a derived Debug, while the seed derived from it was being zeroized. Wrapping it in Zeroizing alone would have left the printing, since Zeroizing implements Debug whenever its contents do, so Debug is now written by hand and shows the public coordinates in full while withholding the secret. That keeps the value useful for diagnosing an encoding mismatch, which is what it is for. EccEphemeralAgreement documented all three of its fields as KDFe inputs. Only z and the ephemeral X reach the KDF; the ephemeral Y travels to the peer inside the marshalled point. The correction says why Y is still padded, since a reader told only that it is unhashed could conclude it needs no padding, and a TPM parsing the point is entitled to a full width coordinate. The same claim appeared in a comment in produce_seed and is corrected there too. Coordinate restoration had no coverage. Points built from a SEC1 encoding carry full width coordinates, so the padding was exercised only as a no op and removing it entirely passed the whole suite. A round trip is added against a key whose public X carries a leading zero, stripped from the fixture the way a TPM marshalling a TPM2B may strip it, along with a rejection case for a coordinate wider than its curve: trimming an oversized coordinate to fit would agree with a point the peer never held and fail only at the TPM. That fixture uses a fixed scalar. One key in 256 has a leading zero octet, and generating that many in an unoptimised test build cost more than the rest of the suite together. The test asserts the coordinate is short before relying on it, so a fixture that stopped being short would fail rather than quietly revert to a no op. The KDF test pins both functions at 250 bits against independently computed vectors. The two are unrelated to each other because KDFa hashes the requested length into every iteration and KDFe does not, so KDFa at 250 bits is a different stream rather than a shortened one.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
TSS.Rust/src/crypto/mod.rs:406
zis wrapped inZeroizingbecause it is sensitive, but each KDFe iteration copies it into this ordinaryVec, which is dropped without being wiped. That leaves the agreed secret in a heap allocation after the protected provider result is erased. Use a zeroizing temporary (or incremental hashing) for the assembled hash input.
let mut to_hash = Vec::new();
TSS.Rust/src/crypto/mod.rs:166
- The generated curve registry also defines
TPM_ECC_CURVE::TEST_P192(tpm_types.rs:349), but this helper falls through and reports its known coordinate width as unknown. Since this API promises registry-based widths, handle the test P-192 identifier as a 24-byte coordinate too; provider support can still be rejected separately.
TPM_ECC_CURVE::NIST_P192 => Ok(24),
TSS.Rust/Cargo.toml:54
- These curve crates retain their default features, so the lockfile now pulls in the ECDSA/RFC6979/signature stack even though this provider only performs ECDH. Disabling defaults keeps the new backend's compile-time dependency surface aligned with the operation it implements.
p256 = { version = "0.13", features = ["ecdh"], optional = true }
p384 = { version = "0.13", features = ["ecdh"], optional = true }
p521 = { version = "0.13", features = ["ecdh"], optional = true }
Wipe the KDF intermediates. Both KDFs accumulated the derived key in a plain Vec, and KDFe additionally copied the agreed value into a plain per-iteration buffer, so several copies of key material were freed unwiped. All are now Zeroizing, and both are reserved to their final size up front because a Vec that grows frees its old allocation without wiping it, which would have left copies behind despite the wrapper. KDFa's hashed input is deliberately left plain: everything in it is public and the key travels separately as the HMAC key. Give TEST_P192 its coordinate width. It was the only registry curve other than NONE falling through to the unknown arm. This function is a registry lookup, so it answers for any curve the registry names; whether a backend will agree over that curve is answered separately. The new sweep test enumerates the registry through the generated try_from rather than a hand-written list, so a curve added upstream fails the test instead of silently falling through. Stop pulling default features from the curve crates. They brought in the ECDSA and RFC6979 crates for a provider that only ever agrees. The graph goes from 70 crates to 68; the PKCS#8 and PEM crates stay, since the rsa crate needs them independently.
Every other use of this family is written out in full: std::mem::size_of in device.rs, tpm2_helpers.rs and twice in tpm_buffer.rs. This one line was the only unqualified use, so it now matches the other four. This is a consistency change rather than a fix. size_of_val has been in the prelude since Rust 1.80, the crate declares no MSRV below that, and the unqualified form built and passed the suite; a review comment claiming otherwise was mistaken. The _val form is kept rather than size_of::<u32>() so that the reservation follows the counter's type if it ever changes, which is what makes the reservation correct.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
TSS.Rust/src/tpm_type_extensions.rs:261
- This sentence is missing a relative clause, making the coordinate-padding explanation difficult to parse.
// KDFe hashes this coordinate, so one the TPM marshalled without its leading zeroes
TSS.Rust/src/crypto/mod.rs:865
- “Octet bits” mixes byte and bit units; this assertion is checking six unused bits.
"the 6 octet bits that were not requested should be zero"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TPMT_PUBLIC::create_activation rejected anything that was not an RSA key, so a TPM whose endorsement key is ECC could not be issued a credential at all. This adds the ECC path.
The two algorithms differ only in how the seed reaches the TPM, which is now isolated in produce_seed. With RSA the seed is random and the secret is that seed under OAEP. With ECC nothing is transported: both sides derive the seed from an ECDH agreement, and the secret is the ephemeral public point the TPM needs to repeat it. Everything after the seed is defined on the seed alone and is shared unchanged.
The crypto layer gains KDFe, the SP800-56A concatenation KDF that TPM 2.0 Part 1 section 11.4.10.3 defines for this derivation. It is easy to confuse with KDFa: it hashes rather than HMACs, and it does not hash the requested length. Getting either wrong yields output of the right shape and the wrong value.
Only the ephemeral agreement is delegated to the provider, and it returns the raw agreed value. Deriving from it is specification behaviour rather than backend behaviour, so it stays in Crypto. Some platform APIs offer to perform the concatenation themselves, but at least one silently ignores the requested hash algorithm and always uses SHA-256, which no interoperability failure would attribute to the KDF.
Coordinate width is answered by Crypto rather than by a provider, because a TPM may drop leading zero bytes from a coordinate it marshals into a TPM2B while KDFe hashes coordinates at full width. A curve outside the TCG registry is an error rather than a guess, since guessing produces a plausible key that simply does not match the peer's.
The software provider implements the agreement over P-256, P-384 and P-521. Barreto-Naehrig and SM2 curves are rejected rather than approximated with a NIST curve.
Tested by a round trip against a stand-in that repeats the agreement, on three curves across three nameAlgs. Mutation testing confirms the round trip fails if partyU and partyV are swapped, or if the KDFe label is changed, on one side only. It cannot catch a mistake made identically on both sides, since both ends call the same KDFe, so KDFe is additionally pinned to an independently computed vector.