Skip to content

crypto: Port over test_igvm_agent#3497

Closed
smalis-msft wants to merge 6 commits into
microsoft:mainfrom
smalis-msft:aes-kwp
Closed

crypto: Port over test_igvm_agent#3497
smalis-msft wants to merge 6 commits into
microsoft:mainfrom
smalis-msft:aes-kwp

Conversation

@smalis-msft
Copy link
Copy Markdown
Contributor

@smalis-msft smalis-msft commented May 15, 2026

Port the test_igvm_agent to the crypto crate, using the Rust backend as it's test only. Along the way, cleanup the AES key wrap module and add a RustCrypto backend.

Currently blocked on some other work.

Copilot AI review requested due to automatic review settings May 15, 2026 17:26
@smalis-msft smalis-msft requested a review from a team as a code owner May 15, 2026 17:26
@smalis-msft smalis-msft enabled auto-merge (squash) May 15, 2026 17:26
@smalis-msft smalis-msft marked this pull request as draft May 15, 2026 17:27
auto-merge was automatically disabled May 15, 2026 17:27

Pull request was converted to draft

@github-actions github-actions Bot added the unsafe Related to unsafe code label May 15, 2026
@github-actions
Copy link
Copy Markdown

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR ports the test-only test_igvm_agent cryptography helpers to the shared support/crypto crate (using the Rust backend per the PR description) and refactors the AES key wrap implementation/module naming.

Changes:

  • Removed the bespoke test-only crypto implementations from test_igvm_agent_lib and switched it to support/crypto RSA + AES key wrap usage.
  • Added RsaPublicKey::from_components(n, e) across crypto backends and updated consumers to use it.
  • Renamed/cleaned up AES key wrap module usage (aes_key_wrapaes_kwp) and adjusted OpenSSL wrap/unwrap implementation.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
vm/devices/get/test_igvm_agent_lib/src/test_crypto.rs Deletes the local test-only SHA1/AES/AES-KWP/RNG implementations.
vm/devices/get/test_igvm_agent_lib/src/lib.rs Switches key generation/encryption/wrapping to crypto crate APIs and getrandom.
vm/devices/get/test_igvm_agent_lib/Cargo.toml Replaces rsa/sha2 deps with crypto (rust backend) + getrandom.
support/crypto/src/x509/symcrypt_rust.rs Refactors RSA public key construction to use RsaPublicKey::from_components.
support/crypto/src/rsa/symcrypt.rs Adds RsaPublicKeyInner::from_components for SymCrypt backend.
support/crypto/src/rsa/rust.rs Adds RsaPublicKeyInner::from_components for RustCrypto backend.
support/crypto/src/rsa/ossl.rs Adds RsaPublicKeyInner::from_components for OpenSSL backend.
support/crypto/src/rsa/mod.rs Exposes RsaPublicKey::from_components in the public API.
support/crypto/src/lib.rs Renames AES key wrap module export to aes_kwp.
support/crypto/src/aes_kwp/ossl.rs Changes OpenSSL KWP wrap/unwrap to use cipher_update_vec + cipher_final_vec.
support/crypto/src/aes_kwp/mod.rs Minor attribute/comment cleanup in AES-KWP error type.
openhcl/underhill_attestation/src/secure_key_release.rs Updates AES-KWP module path to crypto::aes_kwp.
openhcl/underhill_attestation/src/key_protector.rs Updates AES-KWP module path to crypto::aes_kwp.
Cargo.lock Updates dependencies for test_igvm_agent_lib (adds crypto and getrandom, removes rsa/sha2).
Comments suppressed due to low confidence (1)

support/crypto/src/x509/symcrypt_rust.rs:63

  • key.try_into().unwrap() can panic on malformed/invalid RSA public key components even if the PKCS#1 DER parse succeeded. Since X.509 certificates can be attacker-controlled, this should return a structured error instead of panicking (e.g. map the conversion error into X509Error via der_err/rsa_der_err).
        #[cfg(rust)]
        let key = key.try_into().unwrap();
        Ok(crate::rsa::RsaPublicKey(
            crate::rsa::sys::RsaPublicKeyInner(key),

Comment thread vm/devices/get/test_igvm_agent_lib/src/lib.rs
Comment thread support/crypto/src/x509/symcrypt_rust.rs
@smalis-msft smalis-msft marked this pull request as ready for review May 15, 2026 17:58
Copilot AI review requested due to automatic review settings May 15, 2026 17:58
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.

Comment thread vm/devices/get/test_igvm_agent_lib/Cargo.toml Outdated
.map_err(|e| err(e, "constructing RSA public key"))?;
.map_err(|e| X509Error(e.0));
#[cfg(rust)]
let key = key.try_into().unwrap();
Comment thread vm/devices/get/test_igvm_agent_lib/src/lib.rs
Copilot AI review requested due to automatic review settings May 15, 2026 19:30
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

vm/devices/get/test_igvm_agent_rpc_server/Cargo.toml:40

  • This crate adds a direct crypto dependency purely to force-enable the rust backend feature and then suppresses it via package.metadata.xtask.unused-deps. If test_igvm_agent_lib selects the Windows-only backend feature itself (so all consumers build correctly), this extra dependency + unused-deps override can be removed to reduce duplication.
[target.'cfg(windows)'.dependencies]
crypto = { workspace = true, features = ["rust"] }
guid.workspace = true
parking_lot.workspace = true
test_igvm_agent_lib.workspace = true
tracing.workspace = true
tracing-subscriber = { workspace = true, features = ["env-filter"] }
windows-sys = { workspace = true, features = [
    "Win32_Foundation",
    "Win32_System_Memory",
    "Win32_System_Rpc",
    "Win32_System_Console",
    "Win32_System_Com",
] }

[build-dependencies]
cc.workspace = true
serde_json = { workspace = true, features = ["std"] }

[lints]
workspace = true

[package.metadata.xtask.unused-deps]
# keep the crypto dep so we can specify the backend feature
ignored = ["crypto"]

Comment thread vm/devices/get/test_igvm_agent_lib/Cargo.toml
Comment on lines +25 to +27
16 => Self::Aes128(KwpAes128::new_from_slice(key).unwrap()),
24 => Self::Aes192(KwpAes192::new_from_slice(key).unwrap()),
32 => Self::Aes256(KwpAes256::new_from_slice(key).unwrap()),
@smalis-msft smalis-msft marked this pull request as draft May 18, 2026 15:18
/// A backend cryptographic error occurred.
#[cfg(rust)]
#[error("AES key wrap error during {1}: {0}")]
Backend(String, &'static str),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

note to self: hide variants

@smalis-msft smalis-msft deleted the aes-kwp branch May 20, 2026 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

unsafe Related to unsafe code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants