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