Initial implementation of all core functionality of TPM 2.0#1
Initial implementation of all core functionality of TPM 2.0#1hyperfinitism wants to merge 1 commit intomainfrom
Conversation
hyperfinitism
force-pushed
the
feature/core-functionality
branch
from
f5bf612 to
7ef75b7
Compare
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers an initial implementation of a Rust-based command-line tool for TPM 2.0, providing a broad range of functionalities for interacting with and managing TPM devices. It aims to offer a secure and reliable alternative to existing tools, leveraging Rust's safety features. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ef75b734d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| if let Some(ref path) = self.public_out { | ||
| std::fs::write(path, format!("{:?}", result.out_public))?; |
There was a problem hiding this comment.
The create command writes out_public using Debug formatting, so the --public output is text instead of TPM marshaled bytes. That breaks the documented create → load flow because load expects PublicBuffer::try_from(pub_bytes) binary data (src/cmd/load.rs:72), causing invalid public buffer when users try to load keys created by this command.
Useful? React with 👍 / 👎.
| let tss_ticket: TPMT_TK_HASHCHECK = | ||
| unsafe { std::ptr::read(ticket_data.as_ptr() as *const TPMT_TK_HASHCHECK) }; | ||
| HashcheckTicket::try_from(tss_ticket) |
There was a problem hiding this comment.
This reads ticket bytes with ptr::read into TPMT_TK_HASHCHECK, which treats file contents as native in-memory layout (endianness/alignment/padding dependent) rather than TPM marshaled format. In practice, tickets produced by other TPM tooling or different architectures can be misparsed and rejected, so restricted-key signing with --ticket becomes unreliable.
Useful? React with 👍 / 👎.
| } else if self.hmac_session || self.audit_session { | ||
| SessionType::Hmac |
There was a problem hiding this comment.
The CLI advertises --audit-session as “HMAC with audit flag”, but this branch only changes the session type to Hmac and never sets the session audit attribute before saving the context. As a result, sessions created with --audit-session behave like regular HMAC sessions and do not produce audit behavior the option name promises.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of command-line tools for interacting with TPM 2.0, covering a wide range of core functionalities. The overall structure is well-organized, with clear separation of concerns into modules for CLI parsing, command implementation, and various utilities. The use of helpers for session management, handle loading, and raw ESYS FFI calls is commendable. However, there are several areas for improvement regarding configuration, consistency, and correctness that should be addressed.
Note: Security Review is unavailable for this PR.
| fn credentialed_context_source(&self) -> anyhow::Result<ContextSource> { | ||
| match (&self.credentialed_context, self.credentialed_context_handle) { | ||
| (Some(path), None) => Ok(ContextSource::File(path.clone())), | ||
| (None, Some(handle)) => Ok(ContextSource::Handle(handle)), | ||
| _ => anyhow::bail!( | ||
| "exactly one of --credentialedkey-context or --credentialedkey-context-handle must be provided" | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
The error message "exactly one of ... must be provided" is incorrect for the case where neither --credentialedkey-context nor --credentialedkey-context-handle is provided. clap's conflicts_with attribute prevents both from being used, but doesn't enforce that at least one is present. The logic should be updated to provide a more accurate error message when neither argument is given. This pattern is repeated for credential_key_context_source in this file and for similar context/handle source functions in many other command files.
| fn credentialed_context_source(&self) -> anyhow::Result<ContextSource> { | |
| match (&self.credentialed_context, self.credentialed_context_handle) { | |
| (Some(path), None) => Ok(ContextSource::File(path.clone())), | |
| (None, Some(handle)) => Ok(ContextSource::Handle(handle)), | |
| _ => anyhow::bail!( | |
| "exactly one of --credentialedkey-context or --credentialedkey-context-handle must be provided" | |
| ), | |
| } | |
| } | |
| fn credentialed_context_source(&self) -> anyhow::Result<ContextSource> { | |
| match (&self.credentialed_context, self.credentialed_context_handle) { | |
| (Some(path), None) => Ok(ContextSource::File(path.clone())), | |
| (None, Some(handle)) => Ok(ContextSource::Handle(handle)), | |
| (None, None) => anyhow::bail!( | |
| "one of --credentialedkey-context or --credentialedkey-context-handle must be provided" | |
| ), | |
| (Some(_), Some(_)) => unreachable!("clap should prevent both arguments from being used"), | |
| } | |
| } |
README.md
Outdated
|
|
||
| - **Extended context file support**: Some arguments in `tpm2-tools` accept only a TPM handle in hex string form without an apparent reason. `rust-tpm2-cli` removes this restriction and allows a context file to be specified wherever it is semantically appropriate. | ||
|
|
||
| - **Subcommand splitting**: Subcommands that conflate distinct operations have been separated. For example, the `encryptdecrypt` subcommand of `tpm2-tools` is split into two dedicated subcommands. |
There was a problem hiding this comment.
The documentation states that the encryptdecrypt subcommand is split into two dedicated subcommands (encrypt and decrypt). However, the encryptdecrypt subcommand is still present in the CLI implementation alongside the new ones. This is confusing for users. Please either remove the encryptdecrypt subcommand to align with the documentation or update the documentation to clarify its purpose (e.g., if it's kept for compatibility with tpm2-tools).
| /// Encrypt data with a symmetric TPM key | ||
| Encrypt(cmd::encrypt::EncryptCmd), | ||
| /// Encrypt or decrypt data with a symmetric key | ||
| Encryptdecrypt(cmd::encryptdecrypt::EncryptDecryptCmd), |
There was a problem hiding this comment.
The encryptdecrypt subcommand appears to be redundant, as separate encrypt and decrypt subcommands are also implemented. As mentioned in the README.md, splitting this command improves clarity. To maintain a clean and unambiguous API, consider removing the Encryptdecrypt variant.
| "ecc" => { | ||
| let params = PublicEccParametersBuilder::new() | ||
| .with_ecc_scheme(EccScheme::EcDsa(HashScheme::new(hash_alg))) | ||
| .with_curve(EccCurve::NistP256) |
There was a problem hiding this comment.
The ECC curve is hardcoded to NistP256. This limits the command's utility. It would be better to allow users to specify the curve via a command-line argument, for example --curve.
| fn build_ak_public(alg: &str, hash_alg: HashingAlgorithm) -> anyhow::Result<Public> { | ||
| let attributes = ObjectAttributesBuilder::new() | ||
| .with_fixed_tpm(true) | ||
| .with_fixed_parent(true) | ||
| .with_sensitive_data_origin(true) | ||
| .with_user_with_auth(true) | ||
| .with_sign_encrypt(true) | ||
| .with_restricted(true) | ||
| .build() | ||
| .context("failed to build object attributes")?; | ||
|
|
||
| let builder = PublicBuilder::new() | ||
| .with_name_hashing_algorithm(HashingAlgorithm::Sha256) | ||
| .with_object_attributes(attributes); | ||
|
|
||
| match alg.to_lowercase().as_str() { | ||
| "rsa" => { | ||
| let params = PublicRsaParametersBuilder::new() | ||
| .with_scheme(RsaScheme::RsaSsa(HashScheme::new(hash_alg))) | ||
| .with_key_bits(RsaKeyBits::Rsa2048) | ||
| .with_exponent(RsaExponent::default()) | ||
| .with_is_signing_key(true) | ||
| .with_restricted(true) | ||
| .with_symmetric(SymmetricDefinitionObject::Null) | ||
| .build() | ||
| .context("failed to build RSA parameters")?; | ||
|
|
||
| builder | ||
| .with_public_algorithm(PublicAlgorithm::Rsa) | ||
| .with_rsa_parameters(params) | ||
| .with_rsa_unique_identifier(Default::default()) | ||
| .build() | ||
| .context("failed to build RSA AK public") | ||
| } | ||
| "ecc" => { | ||
| let params = PublicEccParametersBuilder::new() | ||
| .with_ecc_scheme(EccScheme::EcDsa(HashScheme::new(hash_alg))) | ||
| .with_curve(EccCurve::NistP256) | ||
| .with_is_signing_key(true) | ||
| .with_restricted(true) | ||
| .with_symmetric(SymmetricDefinitionObject::Null) | ||
| .with_key_derivation_function_scheme(KeyDerivationFunctionScheme::Null) | ||
| .build() | ||
| .context("failed to build ECC parameters")?; | ||
|
|
||
| builder | ||
| .with_public_algorithm(PublicAlgorithm::Ecc) | ||
| .with_ecc_parameters(params) | ||
| .with_ecc_unique_identifier(Default::default()) | ||
| .build() | ||
| .context("failed to build ECC AK public") | ||
| } | ||
| _ => bail!("unsupported AK algorithm: {alg}; supported: rsa, ecc"), | ||
| } | ||
| } |
There was a problem hiding this comment.
The build_ak_public function hardcodes several parameters, such as the name hashing algorithm (Sha256), RSA key size (Rsa2048), and ECC curve (NistP256). This reduces the flexibility of the createak command. Consider adding command-line arguments to allow users to configure these parameters.
| fn build_ek_public(alg: &str) -> anyhow::Result<Public> { | ||
| let auth_policy = Digest::try_from(EK_AUTH_POLICY_SHA256.as_slice()) | ||
| .map_err(|e| anyhow::anyhow!("invalid auth policy: {e}"))?; | ||
|
|
||
| let attributes = ObjectAttributesBuilder::new() | ||
| .with_fixed_tpm(true) | ||
| .with_fixed_parent(true) | ||
| .with_sensitive_data_origin(true) | ||
| .with_admin_with_policy(true) | ||
| .with_restricted(true) | ||
| .with_decrypt(true) | ||
| .build() | ||
| .context("failed to build object attributes")?; | ||
|
|
||
| let builder = PublicBuilder::new() | ||
| .with_name_hashing_algorithm(HashingAlgorithm::Sha256) | ||
| .with_object_attributes(attributes) | ||
| .with_auth_policy(auth_policy); | ||
|
|
||
| match alg.to_lowercase().as_str() { | ||
| "rsa" => { | ||
| let params = PublicRsaParametersBuilder::new() | ||
| .with_scheme(RsaScheme::Null) | ||
| .with_key_bits(RsaKeyBits::Rsa2048) | ||
| .with_exponent(RsaExponent::default()) | ||
| .with_is_decryption_key(true) | ||
| .with_restricted(true) | ||
| .with_symmetric(SymmetricDefinitionObject::AES_128_CFB) | ||
| .build() | ||
| .context("failed to build RSA parameters")?; | ||
|
|
||
| builder | ||
| .with_public_algorithm(PublicAlgorithm::Rsa) | ||
| .with_rsa_parameters(params) | ||
| .with_rsa_unique_identifier(Default::default()) | ||
| .build() | ||
| .context("failed to build RSA EK public template") | ||
| } | ||
| "ecc" => { | ||
| let params = PublicEccParametersBuilder::new() | ||
| .with_ecc_scheme(EccScheme::Null) | ||
| .with_curve(EccCurve::NistP256) | ||
| .with_is_decryption_key(true) | ||
| .with_restricted(true) | ||
| .with_symmetric(SymmetricDefinitionObject::AES_128_CFB) | ||
| .with_key_derivation_function_scheme(KeyDerivationFunctionScheme::Null) | ||
| .build() | ||
| .context("failed to build ECC parameters")?; | ||
|
|
||
| builder | ||
| .with_public_algorithm(PublicAlgorithm::Ecc) | ||
| .with_ecc_parameters(params) | ||
| .with_ecc_unique_identifier(Default::default()) | ||
| .build() | ||
| .context("failed to build ECC EK public template") | ||
| } | ||
| _ => bail!("unsupported EK algorithm: {alg}; supported: rsa, ecc"), | ||
| } | ||
| } |
There was a problem hiding this comment.
The build_ek_public function hardcodes the RSA key size to Rsa2048 and the ECC curve to NistP256. To increase the command's flexibility, these should be configurable via command-line arguments.
| #[arg(short = 's', long = "setup-parameters")] | ||
| pub setup_parameters: bool, |
There was a problem hiding this comment.
The help text for --setup-parameters implies that --max-tries, --recovery-time, and --lockout-recovery-time are required. However, the implementation provides default values if they are not specified. This should be made consistent. You can either update the help text, or enforce that the arguments are provided when --setup-parameters is used.
For example, you could make the arguments required using clap attributes:
#[arg(long = "max-tries", required_if_eq("setup_parameters", "true"))]
pub max_tries: Option<u32>,And then unwrap() them in execute.
Implements 100 subcommands covering all core functionality of TPM 2.0, including cryptography, key management, PCR bank, NV storage, attestation, session and policy management, and utilities. Signed-off-by: Takuma IMAMURA <209989118+hyperfinitism@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
hyperfinitism
force-pushed
the
feature/core-functionality
branch
from
7ef75b7 to
5078aa5
Compare
1 participant
This PR implements 100 subcommands covering all core functionality of TPM 2.0, including cryptography, key management, PCR bank, NV storage, attestation, session and policy management, and utilities.