underhill_attestation: refactoring#3500
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors underhill_attestation to break the very large lib.rs into focused modules and tighten error-handling patterns, with no intended behavior change. The key-derivation flow (get_derived_keys) and its byId variant move to a new derived_keys module; tests and TEE mocks move to tests.rs / test_helpers.rs. The (Error, bool) retry tuples are replaced by a Retryable<E> struct, KeyProtectorSettings/KeyProtectorById are split into clearer types (KeyProtectorActions, GspTypeRecord, and a Found/NotFound enum), and several IgvmAttestRequestHelper and key-unwrap routines are decomposed into helpers. Many error variants are renamed to lower-case and converted to named fields; tests now use matches! against enum variants instead of stringified messages.
Changes:
- Move
get_derived_keys,get_derived_keys_by_id, and supporting types out oflib.rsinto a newderived_keysmodule; split TEE mocks/new_key_protectorhelper intotest_helpers.rsand tests intotests.rs. - Introduce
Retryable<E>,KeyProtectorActions,GspTypeRecord, and aKeyProtectorById::{Found,NotFound}enum to replace ad-hoc tuples/sentinel-bool patterns; passIgvmAttestRequestTypeper call tocreate_requestinstead of via stateful setter. - Normalize errors (consistent lower-case messages, named struct fields,
return Err(...)overErr(...)?) and rewrite tests to match on error variants instead of message strings.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| openhcl/underhill_attestation/src/lib.rs | Trims lib.rs from a monolith to orchestration; introduces Retryable, KeyProtectorActions, GspTypeRecord, and KeyProtectorById enum. |
| openhcl/underhill_attestation/src/derived_keys.rs | New module hosting get_derived_keys/get_derived_keys_by_id and decomposed helpers (attempt_gsp, try_hardware_unseal_ingress_key, etc.). |
| openhcl/underhill_attestation/src/tests.rs | New file containing all orchestration-level tests, updated to use the new types and matches!-based assertions. |
| openhcl/underhill_attestation/src/test_helpers.rs | Adds new_key_protector(active_kp) helper and MockTeeCall/MockTeeCallNoGetDerivedKey mocks moved from lib.rs. |
| openhcl/underhill_attestation/src/vmgs.rs | Tightens visibility to pub(crate), converts Err(...)? to return Err(...), and updates tests to match on error variants. |
| openhcl/underhill_attestation/src/key_protector.rs | Splits unwrap_and_rotate_keys into focused helpers (unwrap_des_key, unwrap_ingress_dek, unwrap_existing_egress_dek, wrap_and_store_new_egress_key), adds dek_is_present. |
| openhcl/underhill_attestation/src/hardware_key_sealing.rs | Renames error variants to named fields; fixes typos in error/doc strings; updates test import. |
| openhcl/underhill_attestation/src/secure_key_release.rs | Switches retry tuples to Retryable, extracts notify_wrapped_key_required, and passes IgvmAttestRequestType per call. |
| openhcl/underhill_attestation/src/igvm_attest/mod.rs | Removes stateful set_request_type; passes request_type to create_request; adds doc comments and return Err(...). |
| openhcl/underhill_attestation/src/igvm_attest/{ak_cert,key_release,wrapped_key}.rs | Doc comments; convert Err(...)? to return Err(...). |
| openhcl/underhill_attestation/src/jwt.rs | Collapses single-variant JwtSignatureVerificationError to a tuple struct; switches From to map_err for cert-chain errors. |
| openhcl/underhill_core/src/emuplat/tpm.rs | Updates AK_CERT call site to pass IgvmAttestRequestType::AK_CERT_REQUEST explicitly. |
Signed-off-by: Ming-Wei Shih <mishih@microsoft.com>
546f958 to
7808bd2
Compare
| pub const RSA_WRAPPED_AES_KEY_LENGTH: usize = 256; | ||
|
|
||
| /// Returns `true` if the DEK buffer contains any non-zero bytes. | ||
| pub(crate) fn dek_is_present(dek: &DekKp) -> bool { |
There was a problem hiding this comment.
Is there some way we could model this as an Option instead? Would that be any cleaner?
There was a problem hiding this comment.
Probably not because the data source is VMGS file and we always parse the data into memory regardless the slot presents or not.
An iteration of code refactoring. No functional changes. The details include