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
diff --git a/keystore2/src/attestation_key_utils.rs b/keystore2/src/attestation_key_utils.rs
index 184b3cb..4a8923c 100644
--- a/keystore2/src/attestation_key_utils.rs
+++ b/keystore2/src/attestation_key_utils.rs
@@ -23,7 +23,7 @@
use crate::remote_provisioning::RemProvState;
use crate::utils::check_key_permission;
use android_hardware_security_keymint::aidl::android::hardware::security::keymint::{
- AttestationKey::AttestationKey, Certificate::Certificate, KeyParameter::KeyParameter, Tag::Tag,
+ AttestationKey::AttestationKey, KeyParameter::KeyParameter, Tag::Tag,
};
use android_system_keystore2::aidl::android::system::keystore2::{
Domain::Domain, KeyDescriptor::KeyDescriptor, ResponseCode::ResponseCode,
@@ -37,7 +37,8 @@
pub enum AttestationKeyInfo {
RkpdProvisioned {
attestation_key: AttestationKey,
- attestation_certs: Certificate,
+ /// Concatenated chain of DER-encoded certificates (ending with the root).
+ attestation_certs: Vec<u8>,
},
UserGenerated {
key_id_guard: KeyIdGuard,
diff --git a/keystore2/src/remote_provisioning.rs b/keystore2/src/remote_provisioning.rs
index cda93b3..2bdafd4 100644
--- a/keystore2/src/remote_provisioning.rs
+++ b/keystore2/src/remote_provisioning.rs
@@ -20,9 +20,8 @@
//! DB.
use android_hardware_security_keymint::aidl::android::hardware::security::keymint::{
- Algorithm::Algorithm, AttestationKey::AttestationKey, Certificate::Certificate,
- KeyParameter::KeyParameter, KeyParameterValue::KeyParameterValue, SecurityLevel::SecurityLevel,
- Tag::Tag,
+ Algorithm::Algorithm, AttestationKey::AttestationKey, KeyParameter::KeyParameter,
+ KeyParameterValue::KeyParameterValue, SecurityLevel::SecurityLevel, Tag::Tag,
};
use android_security_rkp_aidl::aidl::android::security::rkp::RemotelyProvisionedKey::RemotelyProvisionedKey;
use android_system_keystore2::aidl::android::system::keystore2::{
@@ -85,7 +84,7 @@
key: &KeyDescriptor,
caller_uid: u32,
params: &[KeyParameter],
- ) -> Result<Option<(AttestationKey, Certificate)>> {
+ ) -> Result<Option<(AttestationKey, Vec<u8>)>> {
if !self.is_asymmetric_key(params) || key.domain != Domain::APP {
Ok(None)
} else {
@@ -106,13 +105,14 @@
AttestationKey {
keyBlob: rkpd_key.keyBlob,
attestKeyParams: vec![],
- // Batch certificate is at the beginning of the certificate chain.
+ // Batch certificate is at the beginning of the concatenated certificate
+ // chain, and the helper function only looks at the first cert.
issuerSubjectName: parse_subject_from_certificate(
&rkpd_key.encodedCertChain,
)
.context(ks_err!("Failed to parse subject."))?,
},
- Certificate { encodedCertificate: rkpd_key.encodedCertChain },
+ rkpd_key.encodedCertChain,
))),
}
}
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(¶ms)
))
.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
})
}