From 464d4c9259463f3e49d069d7b526e821ebbd2945 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Sun, 10 Jul 2022 22:05:01 -0400 Subject: [PATCH 1/4] Add some additional FabricTable unit tests - Some operations have been manually and integration-tested, but there are still FabricTable tests with TODO - This PR implements the following FabricTable unit test - TestFabricLabelChange - TestAddNocRootCollision - During development of unit tests, the following fixes were done: - Add a `GetFabricLabel(FabricIndex, CharSpan)` method to be symmetrical with `SetFabricLabel(FabricIndex, CharSpan)` - Fix the error code returned by CommitPendingFabric when a root is added, but neither Add was never done - Better document the existing methods Testing done: - Added unit tests - All unit tests pass - Integration tests still pass --- src/credentials/FabricTable.cpp | 34 ++- src/credentials/FabricTable.h | 49 +++- src/credentials/tests/TestFabricTable.cpp | 305 +++++++++++++++++++++- 3 files changed, 365 insertions(+), 23 deletions(-) diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index f854956f067a64..745764767111d4 100644 --- a/src/credentials/FabricTable.cpp +++ b/src/credentials/FabricTable.cpp @@ -1788,15 +1788,18 @@ CHIP_ERROR FabricTable::CommitPendingFabricData() CHIP_ERROR err = CHIP_NO_ERROR; if (hasInvalidInternalState) { - ChipLogError(FabricProvisioning, "Failed to commit: internally inconsistent state!"); - err = CHIP_ERROR_INTERNAL; - } - else if (haveNewTrustedRoot) - { - ChipLogError(FabricProvisioning, - "Failed to commit: tried to commit with only a new trusted root cert. No data committed."); - hasInvalidInternalState = true; - err = CHIP_ERROR_INCORRECT_STATE; + if (haveNewTrustedRoot) + { + ChipLogError(FabricProvisioning, + "Failed to commit: tried to commit with only a new trusted root cert. No data committed."); + hasInvalidInternalState = true; + err = CHIP_ERROR_INCORRECT_STATE; + } + else + { + ChipLogError(FabricProvisioning, "Failed to commit: internally inconsistent state!"); + err = CHIP_ERROR_INTERNAL; + } } else { @@ -2005,13 +2008,13 @@ void FabricTable::RevertPendingOpCertsExceptRoot() CHIP_ERROR FabricTable::SetFabricLabel(FabricIndex fabricIndex, const CharSpan & fabricLabel) { VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(IsValidFabricIndex(fabricIndex), CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(IsValidFabricIndex(fabricIndex), CHIP_ERROR_INVALID_FABRIC_INDEX); ReturnErrorCodeIf(fabricLabel.size() > kFabricLabelMaxLengthInBytes, CHIP_ERROR_INVALID_ARGUMENT); FabricInfo * fabricInfo = GetMutableFabricByIndex(fabricIndex); bool fabricIsInitialized = (fabricInfo != nullptr) && fabricInfo->IsInitialized(); - VerifyOrReturnError(fabricIsInitialized, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(fabricIsInitialized, CHIP_ERROR_INVALID_FABRIC_INDEX); // Update fabric table current in-memory entry, whether pending or not ReturnErrorOnFailure(fabricInfo->SetFabricLabel(fabricLabel)); @@ -2025,4 +2028,13 @@ CHIP_ERROR FabricTable::SetFabricLabel(FabricIndex fabricIndex, const CharSpan & return CHIP_NO_ERROR; } +CHIP_ERROR FabricTable::GetFabricLabel(FabricIndex fabricIndex, CharSpan & outFabricLabel) +{ + const FabricInfo * fabricInfo = FindFabricWithIndex(fabricIndex); + VerifyOrReturnError(fabricInfo != nullptr, CHIP_ERROR_INVALID_FABRIC_INDEX); + + outFabricLabel = fabricInfo->GetFabricLabel(); + return CHIP_NO_ERROR; +} + } // namespace chip diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h index 0336dcc1a88d19..110c2fdafe63ac 100644 --- a/src/credentials/FabricTable.h +++ b/src/credentials/FabricTable.h @@ -435,12 +435,39 @@ class DLL_EXPORT FabricTable CHIP_ERROR AddFabricDelegate(FabricTable::Delegate * delegate); void RemoveFabricDelegate(FabricTable::Delegate * delegate); - // Set the Fabric Label for the given fabricIndex. If a fabric add/update is pending, - // only the pending version will be updated, so that on fail-safe expiry, you would - // actually see the only fabric label if Update fails. If the fabric label is - // set before UpdateNOC, then the change is immediate. + // Set the Fabric Label for the given fabricIndex. + + /** + * @brief Set the Fabric Label for the fabric referred by `fabricIndex`. + * + * If a fabric add/update is pending, only the pending version will be updated, + * so that on fail-safe expiry, you would actually see the only fabric label if + * Update fails. If the fabric label is set before UpdateNOC, then the change is immediate. + * + * @param fabricIndex - Fabric Index for which to set the label + * @param fabricLabel - Label to set on the fabric + * @retval CHIP_NO_ERROR on success + * @retval CHIP_ERROR_INVALID_FABRIC_INDEX if fabricIndex does not refer to an fabric in the table + * @retval CHIP_ERROR_INVALID_ARGUMENT on fabric label error (e.g. too large) + * @retval other CHIP_ERROR on internal errors + */ CHIP_ERROR SetFabricLabel(FabricIndex fabricIndex, const CharSpan & fabricLabel); + /** + * @brief Get the Fabric Label for a given fabric + * + * NOTE: The outFabricLabel argument points to internal memory of the fabric info. + * It may become invalid on the next FabricTable API call due to shadow + * storage of data. + * + * @param fabricIndex - Fabric index for which to get the label + * @param outFabricLabel - char span that will be set to the label value + * @retval CHIP_NO_ERROR on success + * @retval CHIP_ERROR_INVALID_FABRIC_INDEX on error + * @retval other CHIP_ERROR on internal errors + */ + CHIP_ERROR GetFabricLabel(FabricIndex fabricIndex, CharSpan & outFabricLabel); + /** * Get the current Last Known Good Time. * @@ -692,10 +719,11 @@ class DLL_EXPORT FabricTable * @param vendorId - VendorID to use for the new fabric * @param outNewFabricIndex - Pointer where the new fabric index for the fabric just added will be set. Cannot be nullptr. * - * @retval CHIP_NO_ERROR on success - * @retval CHIP_ERROR_INCORRECT_STATE if this is called in an inconsistent order - * @retval CHIP_ERROR_NO_MEMORY if there is insufficient memory to make the fabric pending + * @retval CHIP_NO_ERROR on success. + * @retval CHIP_ERROR_INCORRECT_STATE if this is called in an inconsistent order. + * @retval CHIP_ERROR_NO_MEMORY if there is insufficient memory to make the fabric pending. * @retval CHIP_ERROR_INVALID_ARGUMENT if any of the arguments are invalid such as too large or out of bounds. + * @retval CHIP_ERROR_FABRIC_EXISTS if operational identity collides with one already present. * @retval other CHIP_ERROR_* on internal errors or certificate validation errors. */ CHIP_ERROR AddNewPendingFabricWithOperationalKeystore(const ByteSpan & noc, const ByteSpan & icac, uint16_t vendorId, @@ -725,10 +753,11 @@ class DLL_EXPORT FabricTable * copied using P256Keypair::Serialize/Deserialize and owned in heap of a FabricInfo. * @param outNewFabricIndex - Pointer where the new fabric index for the fabric just added will be set. Cannot be nullptr. * - * @retval CHIP_NO_ERROR on success - * @retval CHIP_ERROR_INCORRECT_STATE if this is called in an inconsistent order - * @retval CHIP_ERROR_NO_MEMORY if there is insufficient memory to make the fabric pending + * @retval CHIP_NO_ERROR on success. + * @retval CHIP_ERROR_INCORRECT_STATE if this is called in an inconsistent order. + * @retval CHIP_ERROR_NO_MEMORY if there is insufficient memory to make the fabric pending. * @retval CHIP_ERROR_INVALID_ARGUMENT if any of the arguments are invalid such as too large or out of bounds. + * @retval CHIP_ERROR_FABRIC_EXISTS if operational identity collides with one already present. * @retval other CHIP_ERROR_* on internal errors or certificate validation errors. */ CHIP_ERROR AddNewPendingFabricWithProvidedOpKey(const ByteSpan & noc, const ByteSpan & icac, uint16_t vendorId, diff --git a/src/credentials/tests/TestFabricTable.cpp b/src/credentials/tests/TestFabricTable.cpp index 400c5820e5bfba..2b8debafffa37d 100644 --- a/src/credentials/tests/TestFabricTable.cpp +++ b/src/credentials/tests/TestFabricTable.cpp @@ -1757,7 +1757,140 @@ void TestSequenceErrors(nlTestSuite * inSuite, void * inContext) void TestFabricLabelChange(nlTestSuite * inSuite, void * inContext) { - // TODO: Write test + Credentials::TestOnlyLocalCertificateAuthority fabricCertAuthority; + + chip::TestPersistentStorageDelegate storage; + NL_TEST_ASSERT(inSuite, fabricCertAuthority.Init().IsSuccess()); + + constexpr uint16_t kVendorId = 0xFFF1u; + + // Initialize a fabric table. + ScopedFabricTable fabricTableHolder; + NL_TEST_ASSERT(inSuite, fabricTableHolder.Init(&storage) == CHIP_NO_ERROR); + FabricTable & fabricTable = fabricTableHolder.GetFabricTable(); + + NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0); + + // First scope: add FabricID 1111, node ID 55 + { + FabricId fabricId = 1111; + NodeId nodeId = 55; + + uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + MutableByteSpan csrSpan{ csrBuf }; + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); + + NL_TEST_ASSERT_SUCCESS(inSuite, + fabricCertAuthority.SetIncludeIcac(true).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus()); + ByteSpan rcac = fabricCertAuthority.GetRcac(); + ByteSpan icac = fabricCertAuthority.GetIcac(); + ByteSpan noc = fabricCertAuthority.GetNoc(); + + NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0); + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AddNewPendingTrustedRootCert(rcac)); + FabricIndex newFabricIndex = kUndefinedFabricIndex; + NL_TEST_ASSERT_SUCCESS(inSuite, + fabricTable.AddNewPendingFabricWithOperationalKeystore(noc, icac, kVendorId, &newFabricIndex)); + NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1); + NL_TEST_ASSERT(inSuite, newFabricIndex == 1); + + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.CommitPendingFabricData()); + + // Validate contents + const auto * fabricInfo = fabricTable.FindFabricWithIndex(1); + NL_TEST_ASSERT(inSuite, fabricInfo != nullptr); + if (fabricInfo != nullptr) + { + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == 1); + NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == 55); + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == 1111); + NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId); + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricLabel().size() == 0); + } + } + size_t numStorageKeysAfterFirstAdd = storage.GetNumKeys(); + NL_TEST_ASSERT(inSuite, numStorageKeysAfterFirstAdd == 7); // Metadata, index, 3 certs, 1 opkey, last known good time + + // Second scope: set FabricLabel to "acme fabric", make sure it cannot be reverted + { + // Fabric label starts unset from prior scope + CharSpan fabricLabel = CharSpan::fromCharString("placeholder"); + + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.GetFabricLabel(1, fabricLabel)); + NL_TEST_ASSERT(inSuite, fabricLabel.size() == 0); + + // Set a valid name + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SetFabricLabel(1, CharSpan::fromCharString("acme fabric"))); + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.GetFabricLabel(1, fabricLabel)); + NL_TEST_ASSERT(inSuite, fabricLabel.data_equal(CharSpan::fromCharString("acme fabric")) == true); + + // Revert pending fabric data. Should not revert name since nothing pending. + fabricTable.RevertPendingFabricData(); + + fabricLabel = CharSpan::fromCharString("placeholder"); + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.GetFabricLabel(1, fabricLabel)); + NL_TEST_ASSERT(inSuite, fabricLabel.data_equal(CharSpan::fromCharString("acme fabric")) == true); + + // Verify we fail to set too large a label (> kFabricLabelMaxLengthInBytes) + CharSpan fabricLabelTooBig = CharSpan::fromCharString("012345678901234567890123456789123456"); + NL_TEST_ASSERT(inSuite, fabricLabelTooBig.size() > chip::kFabricLabelMaxLengthInBytes); + + NL_TEST_ASSERT(inSuite, fabricTable.SetFabricLabel(1, fabricLabelTooBig) == CHIP_ERROR_INVALID_ARGUMENT); + + fabricLabel = CharSpan::fromCharString("placeholder"); + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.GetFabricLabel(1, fabricLabel)); + NL_TEST_ASSERT(inSuite, fabricLabel.data_equal(CharSpan::fromCharString("acme fabric")) == true); + } + + // Third scope: set fabric label after an update, it sticks, but then goes back after revert + { + FabricId fabricId = 1111; + NodeId nodeId = 66; // Update node ID from 55 to 66 + + uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + MutableByteSpan csrSpan{ csrBuf }; + NL_TEST_ASSERT_SUCCESS(inSuite, + fabricTable.AllocatePendingOperationalKey(chip::MakeOptional(static_cast(1)), csrSpan)); + + NL_TEST_ASSERT_SUCCESS(inSuite, + fabricCertAuthority.SetIncludeIcac(true).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus()); + ByteSpan icac = fabricCertAuthority.GetIcac(); + ByteSpan noc = fabricCertAuthority.GetNoc(); + + NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1); + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.UpdatePendingFabricWithOperationalKeystore(1, noc, icac)); + NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1); + + // Validate contents prior to change/revert + const auto * fabricInfo = fabricTable.FindFabricWithIndex(1); + NL_TEST_ASSERT(inSuite, fabricInfo != nullptr); + + if (fabricInfo != nullptr) + { + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == 1); + NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == 66); + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == 1111); + NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId); + + CharSpan fabricLabel = fabricInfo->GetFabricLabel(); + NL_TEST_ASSERT(inSuite, fabricLabel.data_equal(CharSpan::fromCharString("acme fabric")) == true); + } + + // Update fabric label + CharSpan fabricLabel = CharSpan::fromCharString("placeholder"); + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SetFabricLabel(1, CharSpan::fromCharString("roboto fabric"))); + + fabricLabel = CharSpan::fromCharString("placeholder"); + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.GetFabricLabel(1, fabricLabel)); + NL_TEST_ASSERT(inSuite, fabricLabel.data_equal(CharSpan::fromCharString("roboto fabric")) == true); + + // Revert pending fabric data. Should revert name to "acme fabric" + fabricTable.RevertPendingFabricData(); + + fabricLabel = CharSpan::fromCharString("placeholder"); + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.GetFabricLabel(1, fabricLabel)); + NL_TEST_ASSERT(inSuite, fabricLabel.data_equal(CharSpan::fromCharString("acme fabric")) == true); + } } void TestCompressedFabricId(nlTestSuite * inSuite, void * inContext) @@ -1841,9 +1974,177 @@ void TestFetchCATs(nlTestSuite * inSuite, void * inContext) // TODO(#20335): Add test cases for NOCs that actually embed CATs } +// Validate that adding the same fabric twice fails (same root, same FabricId) void TestAddNocRootCollision(nlTestSuite * inSuite, void * inContext) { - // TODO: Write test + Credentials::TestOnlyLocalCertificateAuthority fabricCertAuthority; + + chip::TestPersistentStorageDelegate storage; + NL_TEST_ASSERT(inSuite, fabricCertAuthority.Init().IsSuccess()); + + constexpr uint16_t kVendorId = 0xFFF1u; + + // Initialize a fabric table. + ScopedFabricTable fabricTableHolder; + NL_TEST_ASSERT(inSuite, fabricTableHolder.Init(&storage) == CHIP_NO_ERROR); + FabricTable & fabricTable = fabricTableHolder.GetFabricTable(); + + NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0); + + // First scope: add FabricID 1111, node ID 55 + { + FabricId fabricId = 1111; + NodeId nodeId = 55; + + uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + MutableByteSpan csrSpan{ csrBuf }; + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); + + NL_TEST_ASSERT_SUCCESS(inSuite, + fabricCertAuthority.SetIncludeIcac(true).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus()); + ByteSpan rcac = fabricCertAuthority.GetRcac(); + ByteSpan icac = fabricCertAuthority.GetIcac(); + ByteSpan noc = fabricCertAuthority.GetNoc(); + + NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 0); + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AddNewPendingTrustedRootCert(rcac)); + FabricIndex newFabricIndex = kUndefinedFabricIndex; + NL_TEST_ASSERT_SUCCESS(inSuite, + fabricTable.AddNewPendingFabricWithOperationalKeystore(noc, icac, kVendorId, &newFabricIndex)); + NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1); + NL_TEST_ASSERT(inSuite, newFabricIndex == 1); + + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.CommitPendingFabricData()); + + // Validate contents + const auto * fabricInfo = fabricTable.FindFabricWithIndex(1); + NL_TEST_ASSERT(inSuite, fabricInfo != nullptr); + if (fabricInfo != nullptr) + { + Credentials::ChipCertificateSet certificates; + NL_TEST_ASSERT_SUCCESS(inSuite, certificates.Init(1)); + NL_TEST_ASSERT_SUCCESS(inSuite, + certificates.LoadCert(rcac, BitFlags(CertDecodeFlags::kIsTrustAnchor))); + Crypto::P256PublicKey rcacPublicKey(certificates.GetCertSet()[0].mPublicKey); + + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == 1); + NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == 55); + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == 1111); + NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId); + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricLabel().size() == 0); + + Crypto::P256PublicKey rootPublicKeyOfFabric; + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootPubkey(newFabricIndex, rootPublicKeyOfFabric)); + NL_TEST_ASSERT(inSuite, rootPublicKeyOfFabric.Matches(rcacPublicKey)); + } + } + size_t numStorageKeysAfterFirstAdd = storage.GetNumKeys(); + NL_TEST_ASSERT(inSuite, numStorageKeysAfterFirstAdd == 7); // Metadata, index, 3 certs, 1 opkey, last known good time + + // Second scope: add FabricID 1111, node ID 55 *again* --> Collision of Root/FabricID with existing + { + FabricId fabricId = 1111; + NodeId nodeId = 55; + + uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + MutableByteSpan csrSpan{ csrBuf }; + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); + + NL_TEST_ASSERT_SUCCESS(inSuite, + fabricCertAuthority.SetIncludeIcac(true).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus()); + ByteSpan rcac = fabricCertAuthority.GetRcac(); + ByteSpan icac = fabricCertAuthority.GetIcac(); + ByteSpan noc = fabricCertAuthority.GetNoc(); + + NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1); + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AddNewPendingTrustedRootCert(rcac)); + FabricIndex newFabricIndex = kUndefinedFabricIndex; + NL_TEST_ASSERT(inSuite, + fabricTable.AddNewPendingFabricWithOperationalKeystore(noc, icac, kVendorId, &newFabricIndex) == + CHIP_ERROR_FABRIC_EXISTS); + NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1); + + CHIP_ERROR err = fabricTable.CommitPendingFabricData(); + printf("err = %" CHIP_ERROR_FORMAT "\n", err.Format()); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INCORRECT_STATE); + + // Validate contents of Fabric index 1 still valid + const auto * fabricInfo = fabricTable.FindFabricWithIndex(1); + NL_TEST_ASSERT(inSuite, fabricInfo != nullptr); + if (fabricInfo != nullptr) + { + Credentials::ChipCertificateSet certificates; + NL_TEST_ASSERT_SUCCESS(inSuite, certificates.Init(1)); + NL_TEST_ASSERT_SUCCESS(inSuite, + certificates.LoadCert(rcac, BitFlags(CertDecodeFlags::kIsTrustAnchor))); + Crypto::P256PublicKey rcacPublicKey(certificates.GetCertSet()[0].mPublicKey); + + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == 1); + NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == 55); + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == 1111); + NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId); + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricLabel().size() == 0); + + Crypto::P256PublicKey rootPublicKeyOfFabric; + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootPubkey(1, rootPublicKeyOfFabric)); + NL_TEST_ASSERT(inSuite, rootPublicKeyOfFabric.Matches(rcacPublicKey)); + } + } + + // Ensure no new persisted keys after failed colliding add + NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == numStorageKeysAfterFirstAdd); + + // Third scope: add FabricID 2222, node ID 55 --> Not colliding, should work. The failing commit above] + // should have been enough of a revert that this scope succeeds without any additional revert. + { + FabricId fabricId = 2222; + NodeId nodeId = 55; + + uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + MutableByteSpan csrSpan{ csrBuf }; + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); + + NL_TEST_ASSERT_SUCCESS(inSuite, + fabricCertAuthority.SetIncludeIcac(true).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus()); + ByteSpan rcac = fabricCertAuthority.GetRcac(); + ByteSpan icac = fabricCertAuthority.GetIcac(); + ByteSpan noc = fabricCertAuthority.GetNoc(); + + NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1); + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AddNewPendingTrustedRootCert(rcac)); + FabricIndex newFabricIndex = kUndefinedFabricIndex; + NL_TEST_ASSERT_SUCCESS(inSuite, + fabricTable.AddNewPendingFabricWithOperationalKeystore(noc, icac, kVendorId, &newFabricIndex)); + NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 2); + NL_TEST_ASSERT(inSuite, newFabricIndex == 2); + + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.CommitPendingFabricData()); + + // Validate contents + const auto * fabricInfo = fabricTable.FindFabricWithIndex(2); + NL_TEST_ASSERT(inSuite, fabricInfo != nullptr); + if (fabricInfo != nullptr) + { + Credentials::ChipCertificateSet certificates; + NL_TEST_ASSERT_SUCCESS(inSuite, certificates.Init(1)); + NL_TEST_ASSERT_SUCCESS(inSuite, + certificates.LoadCert(rcac, BitFlags(CertDecodeFlags::kIsTrustAnchor))); + Crypto::P256PublicKey rcacPublicKey(certificates.GetCertSet()[0].mPublicKey); + + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == 2); + NL_TEST_ASSERT(inSuite, fabricInfo->GetNodeId() == 55); + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricId() == 2222); + NL_TEST_ASSERT(inSuite, fabricInfo->GetVendorId() == kVendorId); + NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricLabel().size() == 0); + + Crypto::P256PublicKey rootPublicKeyOfFabric; + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchRootPubkey(newFabricIndex, rootPublicKeyOfFabric)); + NL_TEST_ASSERT(inSuite, rootPublicKeyOfFabric.Matches(rcacPublicKey)); + } + } + size_t numStorageKeysAfterSecondAdd = storage.GetNumKeys(); + + NL_TEST_ASSERT(inSuite, numStorageKeysAfterSecondAdd == (numStorageKeysAfterFirstAdd + 5)); // Metadata, 3 certs, 1 opkey } void TestInvalidChaining(nlTestSuite * inSuite, void * inContext) From 7ef18a82ce7602ba9e8d3c4b11c56043f7369af1 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Sun, 10 Jul 2022 22:12:02 -0400 Subject: [PATCH 2/4] Remove needless comment line --- src/credentials/FabricTable.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h index 110c2fdafe63ac..68f9f9c8e132a1 100644 --- a/src/credentials/FabricTable.h +++ b/src/credentials/FabricTable.h @@ -435,8 +435,6 @@ class DLL_EXPORT FabricTable CHIP_ERROR AddFabricDelegate(FabricTable::Delegate * delegate); void RemoveFabricDelegate(FabricTable::Delegate * delegate); - // Set the Fabric Label for the given fabricIndex. - /** * @brief Set the Fabric Label for the fabric referred by `fabricIndex`. * From 165a75643697ca6d438a29eb5dceddd683a25498 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Mon, 11 Jul 2022 09:49:03 -0400 Subject: [PATCH 3/4] Address review comment --- src/credentials/FabricTable.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index 745764767111d4..084b2a5cf87c9a 100644 --- a/src/credentials/FabricTable.cpp +++ b/src/credentials/FabricTable.cpp @@ -1792,8 +1792,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData() { ChipLogError(FabricProvisioning, "Failed to commit: tried to commit with only a new trusted root cert. No data committed."); - hasInvalidInternalState = true; - err = CHIP_ERROR_INCORRECT_STATE; + err = CHIP_ERROR_INCORRECT_STATE; } else { From d608b598e3c6157b94f463d0fb10446d60d28c2d Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Mon, 11 Jul 2022 11:20:15 -0400 Subject: [PATCH 4/4] Improve trusted-root-only case and metadata deletion logging --- src/credentials/FabricTable.cpp | 37 ++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index 084b2a5cf87c9a..bf380af1a74136 100644 --- a/src/credentials/FabricTable.cpp +++ b/src/credentials/FabricTable.cpp @@ -226,8 +226,18 @@ CHIP_ERROR FabricTable::DeleteMetadataFromStorage(FabricIndex fabricIndex) if (deleteErr != CHIP_NO_ERROR) { - ChipLogError(FabricProvisioning, "Error deleting part of fabric %d: %" CHIP_ERROR_FORMAT, fabricIndex, deleteErr.Format()); + if (deleteErr == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND) + { + ChipLogError(FabricProvisioning, "Warning: metadata not found during delete of fabric 0x%x", + static_cast(fabricIndex)); + } + else + { + ChipLogError(FabricProvisioning, "Error deleting metadata for fabric fabric 0x%x: %" CHIP_ERROR_FORMAT, + static_cast(fabricIndex), deleteErr.Format()); + } } + return deleteErr; } @@ -1733,6 +1743,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData() bool isAdding = mStateFlags.Has(StateFlags::kIsAddPending); bool isUpdating = mStateFlags.Has(StateFlags::kIsUpdatePending); bool hasPending = mStateFlags.Has(StateFlags::kIsPendingFabricDataPresent); + bool onlyHaveNewTrustedRoot = hasPending && haveNewTrustedRoot && !(isAdding || isUpdating); bool hasInvalidInternalState = hasPending && (!IsValidFabricIndex(mFabricIndexWithPendingState) || !(isAdding || isUpdating)); FabricIndex fabricIndexBeingCommitted = mFabricIndexWithPendingState; @@ -1783,22 +1794,20 @@ CHIP_ERROR FabricTable::CommitPendingFabricData() // If there was nothing pending, we are either in a completely OK state, or weird internally inconsistent // state. In either case, let's clear all pending state anyway, in case it was partially stale! - if (!hasPending || hasInvalidInternalState) + if (!hasPending || onlyHaveNewTrustedRoot || hasInvalidInternalState) { CHIP_ERROR err = CHIP_NO_ERROR; - if (hasInvalidInternalState) + + if (onlyHaveNewTrustedRoot) { - if (haveNewTrustedRoot) - { - ChipLogError(FabricProvisioning, - "Failed to commit: tried to commit with only a new trusted root cert. No data committed."); - err = CHIP_ERROR_INCORRECT_STATE; - } - else - { - ChipLogError(FabricProvisioning, "Failed to commit: internally inconsistent state!"); - err = CHIP_ERROR_INTERNAL; - } + ChipLogError(FabricProvisioning, + "Failed to commit: tried to commit with only a new trusted root cert. No data committed."); + err = CHIP_ERROR_INCORRECT_STATE; + } + else if (hasInvalidInternalState) + { + ChipLogError(FabricProvisioning, "Failed to commit: internally inconsistent state!"); + err = CHIP_ERROR_INTERNAL; } else {