From 513235823b2a1f6b5a299f71feef9e28c95dc1cf Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 20 Jul 2022 19:30:37 -0400 Subject: [PATCH] Include non-fabric-associated pending roots in the RootCertificates attribute. (#21016) * Include non-fabric-associated pending roots in the RootCertificates attribute. Fixes https://github.com/project-chip/connectedhomeip/issues/20791 Also moves a function that was supposed to be private from public to private. * Add unit test. --- .../operational-credentials-server.cpp | 18 +++++ src/credentials/FabricTable.cpp | 18 +++++ src/credentials/FabricTable.h | 42 ++++++++---- src/credentials/tests/TestFabricTable.cpp | 65 +++++++++++++++++++ 4 files changed, 130 insertions(+), 13 deletions(-) diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 0894122eab8fb7..ce29aaea9c5fb1 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -206,6 +206,24 @@ CHIP_ERROR OperationalCredentialsAttrAccess::ReadRootCertificates(EndpointId end ReturnErrorOnFailure(encoder.Encode(ByteSpan{ cert })); } + { + uint8_t certBuf[kMaxCHIPCertLength]; + MutableByteSpan cert{ certBuf }; + CHIP_ERROR err = fabricTable.FetchPendingNonFabricAssociatedRootCert(cert); + if (err == CHIP_ERROR_NOT_FOUND) + { + // No pending root cert, do nothing + } + else if (err != CHIP_NO_ERROR) + { + return err; + } + else + { + ReturnErrorOnFailure(encoder.Encode(ByteSpan{ cert })); + } + } + return CHIP_NO_ERROR; }); } diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index bf380af1a74136..1829541e635e8a 100644 --- a/src/credentials/FabricTable.cpp +++ b/src/credentials/FabricTable.cpp @@ -554,6 +554,24 @@ CHIP_ERROR FabricTable::FetchRootCert(FabricIndex fabricIndex, MutableByteSpan & return mOpCertStore->GetCertificate(fabricIndex, CertChainElement::kRcac, outCert); } +CHIP_ERROR FabricTable::FetchPendingNonFabricAssociatedRootCert(MutableByteSpan & outCert) const +{ + VerifyOrReturnError(mOpCertStore != nullptr, CHIP_ERROR_INCORRECT_STATE); + if (!mStateFlags.Has(StateFlags::kIsTrustedRootPending)) + { + return CHIP_ERROR_NOT_FOUND; + } + + if (mStateFlags.Has(StateFlags::kIsAddPending)) + { + // The root certificate is already associated with a pending fabric, so + // does not exist for purposes of this API. + return CHIP_ERROR_NOT_FOUND; + } + + return FetchRootCert(mFabricIndexWithPendingState, outCert); +} + CHIP_ERROR FabricTable::FetchICACert(FabricIndex fabricIndex, MutableByteSpan & outCert) const { VerifyOrReturnError(mOpCertStore != nullptr, CHIP_ERROR_INCORRECT_STATE); diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h index 68f9f9c8e132a1..4c0fbb4a3bf61b 100644 --- a/src/credentials/FabricTable.h +++ b/src/credentials/FabricTable.h @@ -514,19 +514,6 @@ class DLL_EXPORT FabricTable ConstFabricIterator begin() const { return cbegin(); } ConstFabricIterator end() const { return cend(); } - /** - * @brief Get a mutable FabricInfo entry from the table by FabricIndex. - * - * NOTE: This is private for use within the FabricTable itself. All mutations have to go through the - * FabricTable public methods that take a FabricIndex so that there are no mutations about which - * the FabricTable is unaware, since this would break expectations regarding shadow/pending - * entries used during fail-safe. - * - * @param fabricIndex - fabric index for which to get a mutable FabricInfo entry - * @return the FabricInfo entry for the fabricIndex if found, or nullptr if not found - */ - FabricInfo * GetMutableFabricByIndex(FabricIndex fabricIndex); - /** * @brief Get the RCAC (operational root certificate) associated with a fabric. * @@ -541,6 +528,22 @@ class DLL_EXPORT FabricTable */ CHIP_ERROR FetchRootCert(FabricIndex fabricIndex, MutableByteSpan & outCert) const; + /** + * @brief Get the pending root certificate which is not associated with a fabric, if there is one. + * + * If a root is pending from `AddNewPendingTrustedRootCert`, and there is no + * fabric associated with the corresponding fabric index yet + * (i.e. `AddNewPendingFabric*` has not been called yet) it is returned. + * + * @param outCert - MutableByteSpan to receive the certificate. Resized to actual size. + * @retval CHIP_NO_ERROR on success + * @retval CHIP_ERROR_BUFFER_TOO_SMALL if `outCert` is too small. + * @retval CHIP_ERROR_NOT_FOUND if there is no pending root certificate + * that's not yet associated with a fabric. + * @retval other CHIP_ERROR values on invalid arguments or internal errors. + */ + CHIP_ERROR FetchPendingNonFabricAssociatedRootCert(MutableByteSpan & outCert) const; + /** * @brief Get the ICAC (operational intermediate certificate) associated with a fabric. * @@ -948,6 +951,19 @@ class DLL_EXPORT FabricTable bool isAddition = false; }; + /** + * @brief Get a mutable FabricInfo entry from the table by FabricIndex. + * + * NOTE: This is private for use within the FabricTable itself. All mutations have to go through the + * FabricTable public methods that take a FabricIndex so that there are no mutations about which + * the FabricTable is unaware, since this would break expectations regarding shadow/pending + * entries used during fail-safe. + * + * @param fabricIndex - fabric index for which to get a mutable FabricInfo entry + * @return the FabricInfo entry for the fabricIndex if found, or nullptr if not found + */ + FabricInfo * GetMutableFabricByIndex(FabricIndex fabricIndex); + // Load a FabricInfo metatada item from storage for a given new fabric index. Returns internal error on failure. CHIP_ERROR LoadFromStorage(FabricInfo * fabric, FabricIndex newFabricIndex); diff --git a/src/credentials/tests/TestFabricTable.cpp b/src/credentials/tests/TestFabricTable.cpp index 2b8debafffa37d..c0923279a0d1f9 100644 --- a/src/credentials/tests/TestFabricTable.cpp +++ b/src/credentials/tests/TestFabricTable.cpp @@ -478,7 +478,21 @@ void TestBasicAddNocUpdateNocFlow(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, saw1 == false); } + uint8_t rcacBuf[Credentials::kMaxCHIPCertLength]; + { + // No pending root cert yet. + MutableByteSpan fetchedSpan{ rcacBuf }; + NL_TEST_ASSERT(inSuite, fabricTable.FetchPendingNonFabricAssociatedRootCert(fetchedSpan) == CHIP_ERROR_NOT_FOUND); + } + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AddNewPendingTrustedRootCert(rcac)); + { + // Now have a pending root cert. + MutableByteSpan fetchedSpan{ rcacBuf }; + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchPendingNonFabricAssociatedRootCert(fetchedSpan)); + NL_TEST_ASSERT(inSuite, fetchedSpan.data_equal(rcac)); + } + FabricIndex newFabricIndex = kUndefinedFabricIndex; bool keyIsExternallyOwned = true; @@ -488,6 +502,11 @@ void TestBasicAddNocUpdateNocFlow(nlTestSuite * inSuite, void * inContext) keyIsExternallyOwned, &newFabricIndex)); NL_TEST_ASSERT(inSuite, newFabricIndex == 1); NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 1); + { + // No more pending root cert; it's associated with a fabric now. + MutableByteSpan fetchedSpan{ rcacBuf }; + NL_TEST_ASSERT(inSuite, fabricTable.FetchPendingNonFabricAssociatedRootCert(fetchedSpan) == CHIP_ERROR_NOT_FOUND); + } // No storage yet NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == numStorageKeysAtStart); @@ -1750,6 +1769,51 @@ void TestUpdateNocFailSafe(nlTestSuite * inSuite, void * inContext) } } +void TestAddRootCertFailSafe(nlTestSuite * inSuite, void * inContext) +{ + Credentials::TestOnlyLocalCertificateAuthority fabric11CertAuthority; + + chip::TestPersistentStorageDelegate storage; + + NL_TEST_ASSERT(inSuite, fabric11CertAuthority.Init().IsSuccess()); + + // 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); + + // Add a root cert, see that pending works, and that revert works + { + ByteSpan rcac = fabric11CertAuthority.GetRcac(); + + uint8_t rcacBuf[Credentials::kMaxCHIPCertLength]; + { + // No pending root cert yet. + MutableByteSpan fetchedSpan{ rcacBuf }; + NL_TEST_ASSERT(inSuite, fabricTable.FetchPendingNonFabricAssociatedRootCert(fetchedSpan) == CHIP_ERROR_NOT_FOUND); + } + + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AddNewPendingTrustedRootCert(rcac)); + { + // Now have a pending root cert. + MutableByteSpan fetchedSpan{ rcacBuf }; + NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.FetchPendingNonFabricAssociatedRootCert(fetchedSpan)); + NL_TEST_ASSERT(inSuite, fetchedSpan.data_equal(rcac)); + } + + // Revert + fabricTable.RevertPendingFabricData(); + + { + // No pending root cert anymore. + MutableByteSpan fetchedSpan{ rcacBuf }; + NL_TEST_ASSERT(inSuite, fabricTable.FetchPendingNonFabricAssociatedRootCert(fetchedSpan) == CHIP_ERROR_NOT_FOUND); + } + } +} + void TestSequenceErrors(nlTestSuite * inSuite, void * inContext) { // TODO: Write test @@ -2436,6 +2500,7 @@ static const nlTest sTests[] = NL_TEST_DEF("Validate fabrics are loaded from persistence at FabricTable::init", TestPersistence), NL_TEST_DEF("Test fail-safe handling during AddNOC", TestAddNocFailSafe), NL_TEST_DEF("Test fail-safe handling during UpdateNoc", TestUpdateNocFailSafe), + NL_TEST_DEF("Test fail-safe handling for root cert", TestAddRootCertFailSafe), NL_TEST_DEF("Test interlock sequencing errors", TestSequenceErrors), NL_TEST_DEF("Test fabric label changes", TestFabricLabelChange), NL_TEST_DEF("Test compressed fabric ID is properly generated", TestCompressedFabricId),