Merge "Make IRPC v3 optionally backwards compatible"
diff --git a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp
index 97fe08a..6d9c8c9 100644
--- a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp
+++ b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp
@@ -181,15 +181,6 @@
return params;
}
- void checkMacedPubkeyVersioned(const MacedPublicKey& macedPubKey, bool testMode,
- vector<uint8_t>* 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<IRemotelyProvisionedComponent> provisionable_;
RpcHardwareInfo rpcHardwareInfo;
@@ -279,7 +270,7 @@
auto status = provisionable_->generateEcdsaP256KeyPair(testMode, &macedPubKey, &privateKeyBlob);
ASSERT_TRUE(status.isOk());
vector<uint8_t> coseKeyData;
- checkMacedPubkeyVersioned(macedPubKey, testMode, &coseKeyData);
+ check_maced_pubkey(macedPubKey, testMode, &coseKeyData);
}
/**
@@ -302,7 +293,7 @@
auto status = provisionable_->generateEcdsaP256KeyPair(testMode, &macedPubKey, &privateKeyBlob);
ASSERT_TRUE(status.isOk());
vector<uint8_t> coseKeyData;
- checkMacedPubkeyVersioned(macedPubKey, testMode, &coseKeyData);
+ check_maced_pubkey(macedPubKey, testMode, &coseKeyData);
AttestationKey attestKey;
attestKey.keyBlob = std::move(privateKeyBlob);
@@ -357,7 +348,7 @@
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 @@
ASSERT_TRUE(status.isOk()) << status.getMessage();
vector<uint8_t> payload_value;
- checkMacedPubkeyVersioned(key, testMode, &payload_value);
+ check_maced_pubkey(key, testMode, &payload_value);
cborKeysToSign_.add(cppbor::EncodedItem(payload_value));
}
}
@@ -401,8 +392,16 @@
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 @@
}
/**
- * 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 c3e3609..715cf28 100644
--- a/security/rkp/CHANGELOG.md
+++ b/security/rkp/CHANGELOG.md
@@ -30,7 +30,8 @@
* `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 2fc780c..5485db3 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 @@
* 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 @@
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 @@
*
* @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
