commit 76d95baf664502e93e703fe1d76e9a7304b36d9d
author David Drysdale <drysdale@google.com> Wed Sep 25 14:00:10 2024 +0100
committer David Drysdale <drysdale@google.com> Wed Sep 25 14:00:10 2024 +0100
tree bacc0752aa8a00254bbfb6ab202c3187f361120f
parent 94b1285f47007190c767bffd65feb35480156fe8

Remove incorrect/confusing Certificate use

The `Certificate` type from the KeyMint AIDL is intended to hold a
single DER-encoded certificate, but some of the RKP-handling code
re-uses it to hold a concatenated cert chain.

Remove as many of these incorrect/misleading uses of the AIDL
`Certificate` type as possible, and add comments for the ones remaining.

Flag: none, comments + pure refactor
Test: keystore2_client_tests
Change-Id: Id159078f31dd892d51596cc67308ced27fadd968

keystore2/src/security_level.rs

diff --git a/keystore2/src/security_level.rs b/keystore2/src/security_level.rs
index bd20afb..89c0e97 100644
--- a/keystore2/src/security_level.rs
+++ b/keystore2/src/security_level.rs
@@ -49,7 +49,7 @@
 };
 use crate::{globals::get_keymint_device, id_rotation::IdRotationState};
 use android_hardware_security_keymint::aidl::android::hardware::security::keymint::{
-    Algorithm::Algorithm, AttestationKey::AttestationKey,
+    Algorithm::Algorithm, AttestationKey::AttestationKey, Certificate::Certificate,
     HardwareAuthenticatorType::HardwareAuthenticatorType, IKeyMintDevice::IKeyMintDevice,
     KeyCreationResult::KeyCreationResult, KeyFormat::KeyFormat,
     KeyMintHardwareInfo::KeyMintHardwareInfo, KeyParameter::KeyParameter,
@@ -131,11 +131,21 @@
             certificateChain: mut certificate_chain,
         } = creation_result;
 
+        // Unify the possible contents of the certificate chain.  The first entry in the `Vec` is
+        // always the leaf certificate (if present), but the rest of the chain may be present as
+        // either:
+        //  - `certificate_chain[1..n]`: each entry holds a single certificate, as returned by
+        //    KeyMint, or
+        //  - `certificate[1`]: a single `Certificate` from RKP that actually (and confusingly)
+        //    holds the DER-encoded certs of the chain concatenated together.
         let mut cert_info: CertificateInfo = CertificateInfo::new(
+            // Leaf is always a single cert in the first entry, if present.
             match certificate_chain.len() {
                 0 => None,
                 _ => Some(certificate_chain.remove(0).encodedCertificate),
             },
+            // Remainder may be either `[1..n]` individual certs, or just `[1]` holding a
+            // concatenated chain. Convert the former to the latter.
             match certificate_chain.len() {
                 0 => None,
                 _ => Some(
@@ -622,7 +632,14 @@
                     log_security_safe_params(&params)
                 ))
                 .map(|(mut result, _)| {
-                    result.certificateChain.push(attestation_certs);
+                    // The `certificateChain` in a `KeyCreationResult` should normally have one
+                    // `Certificate` for each certificate in the chain. To avoid having to
+                    // unnecessarily parse the RKP chain (which is concatenated DER-encoded certs),
+                    // stuff the whole concatenated chain into a single `Certificate`.
+                    // This is untangled by `store_new_key()`.
+                    result
+                        .certificateChain
+                        .push(Certificate { encodedCertificate: attestation_certs });
                     result
                 })
             }