From b0b8acc849307e0e9531635a6c70289674e2f99b Mon Sep 17 00:00:00 2001 From: Tri Vo Date: Wed, 30 Nov 2022 16:22:23 -0800 Subject: [PATCH] Make IRPC v3 optionally backwards compatible Specifically, we want IRPC v3 to be able to serve old v2 clients. This way we can ship parts IRPC v3 stack incrementally. To that end, allow IRPC v3 to implement v2 behavior of generateCertificateRequest and testMode. Bug: 260920864 Test: atest VtsHalRemotelyProvisionedComponentTargetTest Change-Id: I9e47697bd948c8fd6b82147165d0c67bdef9fbd3 --- .../VtsRemotelyProvisionedComponentTests.cpp | 50 +++++++------------ security/rkp/CHANGELOG.md | 3 +- .../IRemotelyProvisionedComponent.aidl | 14 +++--- 3 files changed, 26 insertions(+), 41 deletions(-) diff --git a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp index 97fe08a110..6d9c8c9aee 100644 --- a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp +++ b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp @@ -181,15 +181,6 @@ class VtsRemotelyProvisionedComponentTests : public testing::TestWithParam* payload_value) { - if (rpcHardwareInfo.versionNumber >= VERSION_WITHOUT_TEST_MODE) { - check_maced_pubkey(macedPubKey, false, payload_value); - } else { - check_maced_pubkey(macedPubKey, testMode, payload_value); - } - } - protected: std::shared_ptr provisionable_; RpcHardwareInfo rpcHardwareInfo; @@ -279,7 +270,7 @@ TEST_P(GenerateKeyTests, generateEcdsaP256Key_prodMode) { auto status = provisionable_->generateEcdsaP256KeyPair(testMode, &macedPubKey, &privateKeyBlob); ASSERT_TRUE(status.isOk()); vector coseKeyData; - checkMacedPubkeyVersioned(macedPubKey, testMode, &coseKeyData); + check_maced_pubkey(macedPubKey, testMode, &coseKeyData); } /** @@ -302,7 +293,7 @@ TEST_P(GenerateKeyTests, generateAndUseEcdsaP256Key_prodMode) { auto status = provisionable_->generateEcdsaP256KeyPair(testMode, &macedPubKey, &privateKeyBlob); ASSERT_TRUE(status.isOk()); vector coseKeyData; - checkMacedPubkeyVersioned(macedPubKey, testMode, &coseKeyData); + check_maced_pubkey(macedPubKey, testMode, &coseKeyData); AttestationKey attestKey; attestKey.keyBlob = std::move(privateKeyBlob); @@ -357,7 +348,7 @@ TEST_P(GenerateKeyTests, generateEcdsaP256Key_testMode) { bool testMode = true; auto status = provisionable_->generateEcdsaP256KeyPair(testMode, &macedPubKey, &privateKeyBlob); ASSERT_TRUE(status.isOk()); - checkMacedPubkeyVersioned(macedPubKey, testMode, nullptr); + check_maced_pubkey(macedPubKey, testMode, nullptr); } class CertificateRequestTestBase : public VtsRemotelyProvisionedComponentTests { @@ -382,7 +373,7 @@ class CertificateRequestTestBase : public VtsRemotelyProvisionedComponentTests { ASSERT_TRUE(status.isOk()) << status.getMessage(); vector payload_value; - checkMacedPubkeyVersioned(key, testMode, &payload_value); + check_maced_pubkey(key, testMode, &payload_value); cborKeysToSign_.add(cppbor::EncodedItem(payload_value)); } } @@ -401,8 +392,16 @@ class CertificateRequestTest : public CertificateRequestTestBase { CertificateRequestTestBase::SetUp(); if (rpcHardwareInfo.versionNumber >= VERSION_WITHOUT_TEST_MODE) { - GTEST_SKIP() << "This test case only applies to RKP v1 and v2. " - << "RKP version discovered: " << rpcHardwareInfo.versionNumber; + bytevec keysToSignMac; + DeviceInfo deviceInfo; + ProtectedData protectedData; + auto status = provisionable_->generateCertificateRequest( + false, {}, {}, {}, &deviceInfo, &protectedData, &keysToSignMac); + if (!status.isOk() && (status.getServiceSpecificError() == + BnRemotelyProvisionedComponent::STATUS_REMOVED)) { + GTEST_SKIP() << "This test case applies to RKP v3+ only if " + << "generateCertificateRequest() is implemented."; + } } } }; @@ -769,30 +768,17 @@ TEST_P(CertificateRequestV2Test, NonEmptyRequestCorruptMac) { } /** - * Generate a non-empty certificate request in prod mode, with test keys. Test mode must be - * ignored, i.e. test must pass. + * Generate a non-empty certificate request in prod mode, with test keys. Must fail with + * STATUS_TEST_KEY_IN_PRODUCTION_REQUEST. */ TEST_P(CertificateRequestV2Test, NonEmptyRequest_testKeyInProdCert) { generateKeys(true /* testMode */, 1 /* numKeys */); bytevec csr; auto status = provisionable_->generateCertificateRequestV2(keysToSign_, challenge_, &csr); - ASSERT_TRUE(status.isOk()) << status.getMessage(); -} - -/** - * Call generateCertificateRequest(). Make sure it's removed. - */ -TEST_P(CertificateRequestV2Test, CertificateRequestV1Removed) { - generateTestEekChain(2); - bytevec keysToSignMac; - DeviceInfo deviceInfo; - ProtectedData protectedData; - auto status = provisionable_->generateCertificateRequest( - true /* testMode */, {} /* keysToSign */, testEekChain_.chain, challenge_, &deviceInfo, - &protectedData, &keysToSignMac); ASSERT_FALSE(status.isOk()) << status.getMessage(); - EXPECT_EQ(status.getServiceSpecificError(), BnRemotelyProvisionedComponent::STATUS_REMOVED); + ASSERT_EQ(status.getServiceSpecificError(), + BnRemotelyProvisionedComponent::STATUS_TEST_KEY_IN_PRODUCTION_REQUEST); } INSTANTIATE_REM_PROV_AIDL_TEST(CertificateRequestV2Test); diff --git a/security/rkp/CHANGELOG.md b/security/rkp/CHANGELOG.md index c3e36091d7..715cf28e4e 100644 --- a/security/rkp/CHANGELOG.md +++ b/security/rkp/CHANGELOG.md @@ -30,7 +30,8 @@ This document provides an exact description of which changes have occurred in th * `version` has moved to a top-level field within the CSR generated by the HAL. * IRemotelyProvisionedComponent * The need for an EEK has been removed. There is no longer an encrypted portion of the CSR. - * Test mode has been removed. + * Keys for new CSR format must be generated with test mode set to false, effectively removing test + mode in the new CSR flow. Old behavior is kept unchanged for backwards compatibility. * The schema for the CSR itself has been significantly simplified, please see IRemotelyProvisionedComponent.aidl for more details. Notably, * the chain of signing, MACing, and encryption operations has been replaced with a single diff --git a/security/rkp/aidl/android/hardware/security/keymint/IRemotelyProvisionedComponent.aidl b/security/rkp/aidl/android/hardware/security/keymint/IRemotelyProvisionedComponent.aidl index 2fc780c38e..5485db3e9b 100644 --- a/security/rkp/aidl/android/hardware/security/keymint/IRemotelyProvisionedComponent.aidl +++ b/security/rkp/aidl/android/hardware/security/keymint/IRemotelyProvisionedComponent.aidl @@ -132,11 +132,7 @@ interface IRemotelyProvisionedComponent { * generateKeyPair generates a new ECDSA P-256 key pair that can be attested by the remote * server. * - * @param in boolean testMode this field is now deprecated. It is ignored by the implementation - * in v3, but retained to simplify backwards compatibility support. V1 and V2 - * implementations must still respect the testMode flag. - * - * testMode indicates whether the generated key is for testing only. Test keys + * @param in boolean testMode indicates whether the generated key is for testing only. Test keys * are marked (see the definition of PublicKey in the MacedPublicKey structure) to * prevent them from being confused with production keys. * @@ -150,8 +146,8 @@ interface IRemotelyProvisionedComponent { byte[] generateEcdsaP256KeyPair(in boolean testMode, out MacedPublicKey macedPublicKey); /** - * This method has been removed in version 3 of the HAL. The header is kept around for - * backwards compatibility purposes. From v3, this method should raise a + * This method can be removed in version 3 of the HAL. The header is kept around for + * backwards compatibility purposes. From v3, this method is allowed to raise a * ServiceSpecificException with an error code of STATUS_REMOVED. * * For v1 and v2 implementations: @@ -306,7 +302,9 @@ interface IRemotelyProvisionedComponent { * * @param in MacedPublicKey[] keysToSign contains the set of keys to certify. The * IRemotelyProvisionedComponent must validate the MACs on each key. If any entry in the - * array lacks a valid MAC, the method must return STATUS_INVALID_MAC. + * array lacks a valid MAC, the method must return STATUS_INVALID_MAC. This method must + * not accept test keys. If any entry in the array is a test key, the method must return + * STATUS_TEST_KEY_IN_PRODUCTION_REQUEST. * * @param in challenge contains a byte string from the provisioning server which will be * included in the signed data of the CSR structure. Different provisioned backends may