Implement shadow fail-safe data in FabricTable (#19819)

* Implement shadow fail-safe data in FabricTable - FabricTable management during commissioning did not properly handle the fact that committing needs to only be done on CommissioningComplete, which prevented the AddTrustedRootCertificate, UpdateNOC and AddNOC command semantics to be implemented properly and prevented proper state observation of operational credential clusters server Fixes #7695 Issue #8905 Fixes #18633 Issue #17208 Fixes #15585 This PR: - Removes direct access to FabricInfo from everywhere, which caused possibly stale FabricInfo references during commissioning. - Remove immediate committing of fabric table on UpdateNOC. - Make Fabrics, NOCs and TrustedRootCertificates attributes reflect proper partial state during fail-safe, by using the shadow data capabilities of OperationalCertificateStore and by updates to FabricInfo - Make it possible to unit test fabric table by providing the necessary lifecycle public APIs to test every modality of the commissioning flow - Make Server and DeviceController use OperationalCertificateStore to allow proper external lifecycle management of the operational cert chain. - Update all examples/controller code to new API - Remove dangerous internal APIs from FabricTable and replace with direct accessors where needed - Add more of the necessary spec validations to the UpdateNOC and AddNOC flows Testing done: - Updated all unit tests, all pass - Cert tests still pass as before - Working on further integration tests and unit tests as a follow-up noting that current state has not regressed on existing test coverage, and that new usage of OperationalCertificateStore class in FabricTable gains a large amount of additional coverage transitively via some of the existing tests making use of FabricTable. * Restyled by clang-format * Fixes after merging master * Rename callback of FabricTable::Delegate - Remove needless ones - Make the callbacks do nothing by default to avoid more "Intentionally left blank" - Renamed to actually reflect what is happening * Rekick restyle * restyle * Align last known good time commit / revert to shadow fail-safe approach Fabric commit / revert is being refactored to only persist fabric data when we actually commit on receipt of CommissioningComplete. This reworks Last known Good Time to use the same strategy. Now, instead of persisting both last known good time and a fail-safe backup at the time of certificate installation, a single, updated last known good time is stored in RAM at the time of certificate installation. Then, on commit, this is persisted, but never before. * Fix CI * Fix TV app * Reduce flash usage on K32W0 by disabling detail (verbose) logs * Disable progress logging in TI platform lock example to save flash * Disable progress logging in TI platform shell example to save flash * Reduce stack usage of TestEventLogging * Apply review comments from @msandstedt * Save stack for Nordic build * Save stack for Nordic build, some more * Added unit tests for all basic operations * Fix merge conflict * Restyle * Change nrf connect stack limit up by 512 bytes for fake unit test comparisons * Revert "Save stack for Nordic build" This reverts commit 52699c4. * Revert "Save stack for Nordic build, some more" This reverts commit e62391e. * Fix UpdateLabel * Apply review comments, add ACL fabric removal * Apply review comments * per bzbarsky, clear in-memory Last Known Good Time if revert fails * Apply fixes from @bzbarsky-apple * Apply more review comments * Restyled * Fix tests * Add unit test for iterator * Iterator now works * Fix semantic merge conflict * Restyled * Fix predicates for storage presence temporarily issue #16958 * Fix Darwin build * Add missing docs, move methods to private that shouldn't be public * Change stack warning temporarily to pass on nRFConnect * Make MatterControllerFactory use const FabricInfo * Restyle * Fix semantic conflict on SessionManager * Fix an init ordering issue in TestSessionManager.cpp * Fix SessionManager shutdown * Restyled Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Michael Sandstedt <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Jun 24, 2022 · e5e09f5 · e5e09f5
1 parent eaafe61
commit e5e09f5
Show file tree

Hide file tree

Showing 66 changed files with 3,823 additions and 2,483 deletions.
diff --git a/build/config/compiler/BUILD.gn b/build/config/compiler/BUILD.gn
@@ -228,7 +228,11 @@ config("warnings_common") {
 
   if (current_os != "mac" && current_os != "ios" && current_os != "linux" &&
       current_os != "win" && current_os != "tizen" && current_os != "webos") {
-    cflags += [ "-Wstack-usage=8192" ]
+    # TODO(#19951): Make this go back to 8192 once unit test contexts are no
+    #               longer on the stack.
+    # cflags += [ "-Wstack-usage=8192" ]
+
+    cflags += [ "-Wstack-usage=8704" ]  # 8192 + 512
   }
 }
 

diff --git a/examples/chip-tool/commands/clusters/ModelCommand.cpp b/examples/chip-tool/commands/clusters/ModelCommand.cpp
@@ -28,8 +28,7 @@ CHIP_ERROR ModelCommand::RunCommand()
 
     if (IsGroupId(mDestinationId))
     {
-        FabricIndex fabricIndex;
-        ReturnErrorOnFailure(CurrentCommissioner().GetFabricIndex(&fabricIndex));
+        FabricIndex fabricIndex = CurrentCommissioner().GetFabricIndex();
         ChipLogProgress(chipTool, "Sending command to group 0x%x", GroupIdFromNodeId(mDestinationId));
 
         return SendGroupCommand(GroupIdFromNodeId(mDestinationId), fabricIndex);

diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp
@@ -72,11 +72,13 @@ CHIP_ERROR CHIPCommand::MaybeSetUpStack()
 
     ReturnLogErrorOnFailure(mDefaultStorage.Init());
     ReturnLogErrorOnFailure(mOperationalKeystore.Init(&mDefaultStorage));
+    ReturnLogErrorOnFailure(mOpCertStore.Init(&mDefaultStorage));
 
     chip::Controller::FactoryInitParams factoryInitParams;
 
     factoryInitParams.fabricIndependentStorage = &mDefaultStorage;
     factoryInitParams.operationalKeystore      = &mOperationalKeystore;
+    factoryInitParams.opCertStore              = &mOpCertStore;
 
     // Init group data provider that will be used for all group keys and IPKs for the
     // chip-tool-configured fabrics. This is OK to do once since the fabric tables
@@ -126,24 +128,26 @@ CHIP_ERROR CHIPCommand::MaybeSetUpStack()
     // Initialize Group Data, including IPK
     for (auto it = mCommissioners.begin(); it != mCommissioners.end(); it++)
     {
-        chip::FabricInfo * fabric = it->second->GetFabricInfo();
-        if ((nullptr != fabric) && (0 != it->first.compare(kIdentityNull)))
+        if (0 == it->first.compare(kIdentityNull))
         {
-            uint8_t compressed_fabric_id[sizeof(uint64_t)];
-            chip::MutableByteSpan compressed_fabric_id_span(compressed_fabric_id);
-            ReturnLogErrorOnFailure(fabric->GetCompressedId(compressed_fabric_id_span));
-
-            ReturnLogErrorOnFailure(
-                chip::GroupTesting::InitData(&mGroupDataProvider, fabric->GetFabricIndex(), compressed_fabric_id_span));
-
-            // Configure the default IPK for all fabrics used by CHIP-tool. The epoch
-            // key is the same, but the derived keys will be different for each fabric.
-            // This has to be done here after we know the Compressed Fabric ID of all
-            // chip-tool-managed fabrics
-            chip::ByteSpan defaultIpk = chip::GroupTesting::DefaultIpkValue::GetDefaultIpk();
-            ReturnLogErrorOnFailure(chip::Credentials::SetSingleIpkEpochKey(&mGroupDataProvider, fabric->GetFabricIndex(),
-                                                                            defaultIpk, compressed_fabric_id_span));
+            continue;
         }
+        const chip::Controller::DeviceCommissioner * controller = it->second.get();
+
+        chip::FabricIndex fabricIndex = controller->GetFabricIndex();
+        uint8_t compressed_fabric_id[sizeof(uint64_t)];
+        chip::MutableByteSpan compressed_fabric_id_span(compressed_fabric_id);
+        ReturnLogErrorOnFailure(controller->GetCompressedFabricIdBytes(compressed_fabric_id_span));
+
+        ReturnLogErrorOnFailure(chip::GroupTesting::InitData(&mGroupDataProvider, fabricIndex, compressed_fabric_id_span));
+
+        // Configure the default IPK for all fabrics used by CHIP-tool. The epoch
+        // key is the same, but the derived keys will be different for each fabric.
+        // This has to be done here after we know the Compressed Fabric ID of all
+        // chip-tool-managed fabrics
+        chip::ByteSpan defaultIpk = chip::GroupTesting::DefaultIpkValue::GetDefaultIpk();
+        ReturnLogErrorOnFailure(
+            chip::Credentials::SetSingleIpkEpochKey(&mGroupDataProvider, fabricIndex, defaultIpk, compressed_fabric_id_span));
     }
 
     return CHIP_NO_ERROR;

diff --git a/examples/chip-tool/commands/common/CHIPCommand.h b/examples/chip-tool/commands/common/CHIPCommand.h
@@ -27,6 +27,7 @@
 #include <commands/common/CredentialIssuerCommands.h>
 #include <commands/example/ExampleCredentialIssuerCommands.h>
 #include <credentials/GroupDataProviderImpl.h>
+#include <credentials/PersistentStorageOpCertStore.h>
 #include <crypto/PersistentStorageOperationalKeystore.h>
 
 #pragma once
@@ -121,6 +122,7 @@ class CHIPCommand : public Command
     PersistentStorage mCommissionerStorage;
 #endif // CONFIG_USE_LOCAL_STORAGE
     chip::PersistentStorageOperationalKeystore mOperationalKeystore;
+    chip::Credentials::PersistentStorageOpCertStore mOpCertStore;
 
     chip::Credentials::GroupDataProviderImpl mGroupDataProvider{ kMaxGroupsPerFabric, kMaxGroupKeysPerFabric };
     CredentialIssuerCommands * mCredIssuerCmds;

diff --git a/examples/chip-tool/commands/group/Commands.h b/examples/chip-tool/commands/group/Commands.h
@@ -54,8 +54,7 @@ class ShowControllerGroups : public CHIPCommand
         fprintf(stderr, "  | Available Groups :                                                                  |\n");
         fprintf(stderr, "  +-------------------------------------------------------------------------------------+\n");
         fprintf(stderr, "  | Group Id   |  KeySet Id     |   Group Name                                          |\n");
-        chip::FabricIndex fabricIndex;
-        CurrentCommissioner().GetFabricIndex(&fabricIndex);
+        chip::FabricIndex fabricIndex                            = CurrentCommissioner().GetFabricIndex();
         chip::Credentials::GroupDataProvider * groupDataProvider = chip::Credentials::GetGroupDataProvider();
         auto it                                                  = groupDataProvider->IterateGroupInfo(fabricIndex);
         chip::Credentials::GroupDataProvider::GroupInfo group;
@@ -98,8 +97,7 @@ class AddGroup : public CHIPCommand
             return CHIP_ERROR_INVALID_ARGUMENT;
         }
 
-        chip::FabricIndex fabricIndex;
-        CurrentCommissioner().GetFabricIndex(&fabricIndex);
+        chip::FabricIndex fabricIndex                            = CurrentCommissioner().GetFabricIndex();
         chip::Credentials::GroupDataProvider * groupDataProvider = chip::Credentials::GetGroupDataProvider();
         chip::Credentials::GroupDataProvider::GroupInfo group;
 
@@ -133,8 +131,7 @@ class RemoveGroup : public CHIPCommand
             return CHIP_ERROR_INVALID_ARGUMENT;
         }
 
-        chip::FabricIndex fabricIndex;
-        CurrentCommissioner().GetFabricIndex(&fabricIndex);
+        chip::FabricIndex fabricIndex                            = CurrentCommissioner().GetFabricIndex();
         chip::Credentials::GroupDataProvider * groupDataProvider = chip::Credentials::GetGroupDataProvider();
         ReturnErrorOnFailure(groupDataProvider->RemoveGroupInfo(fabricIndex, groupId));
         SetCommandExitStatus(CHIP_NO_ERROR);
@@ -153,8 +150,7 @@ class ShowKeySets : public CHIPCommand
 
     CHIP_ERROR RunCommand() override
     {
-        chip::FabricIndex fabricIndex;
-        CurrentCommissioner().GetFabricIndex(&fabricIndex);
+        chip::FabricIndex fabricIndex                            = CurrentCommissioner().GetFabricIndex();
         chip::Credentials::GroupDataProvider * groupDataProvider = chip::Credentials::GetGroupDataProvider();
         chip::Credentials::GroupDataProvider::KeySet keySet;
 
@@ -193,9 +189,8 @@ class BindKeySet : public CHIPCommand
 
     CHIP_ERROR RunCommand() override
     {
-        size_t current_count = 0;
-        chip::FabricIndex fabricIndex;
-        CurrentCommissioner().GetFabricIndex(&fabricIndex);
+        size_t current_count                                     = 0;
+        chip::FabricIndex fabricIndex                            = CurrentCommissioner().GetFabricIndex();
         chip::Credentials::GroupDataProvider * groupDataProvider = chip::Credentials::GetGroupDataProvider();
 
         auto iter     = groupDataProvider->IterateGroupKeys(fabricIndex);
@@ -226,9 +221,8 @@ class UnbindKeySet : public CHIPCommand
 
     CHIP_ERROR RunCommand() override
     {
-        size_t index = 0;
-        chip::FabricIndex fabricIndex;
-        CurrentCommissioner().GetFabricIndex(&fabricIndex);
+        size_t index                                             = 0;
+        chip::FabricIndex fabricIndex                            = CurrentCommissioner().GetFabricIndex();
         chip::Credentials::GroupDataProvider * groupDataProvider = chip::Credentials::GetGroupDataProvider();
         auto iter                                                = groupDataProvider->IterateGroupKeys(fabricIndex);
         size_t maxCount                                          = iter->Count();
@@ -272,12 +266,11 @@ class AddKeySet : public CHIPCommand
 
     CHIP_ERROR RunCommand() override
     {
-        chip::FabricIndex fabricIndex;
-        CurrentCommissioner().GetFabricIndex(&fabricIndex);
+        chip::FabricIndex fabricIndex                            = CurrentCommissioner().GetFabricIndex();
         chip::Credentials::GroupDataProvider * groupDataProvider = chip::Credentials::GetGroupDataProvider();
         uint8_t compressed_fabric_id[sizeof(uint64_t)];
         chip::MutableByteSpan compressed_fabric_id_span(compressed_fabric_id);
-        ReturnLogErrorOnFailure(CurrentCommissioner().GetFabricInfo()->GetCompressedId(compressed_fabric_id_span));
+        ReturnLogErrorOnFailure(CurrentCommissioner().GetCompressedFabricIdBytes(compressed_fabric_id_span));
 
         if ((keyPolicy != chip::Credentials::GroupDataProvider::SecurityPolicy::kCacheAndSync &&
              keyPolicy != chip::Credentials::GroupDataProvider::SecurityPolicy::kTrustFirst) ||
@@ -316,9 +309,8 @@ class RemoveKeySet : public CHIPCommand
 
     CHIP_ERROR RunCommand() override
     {
-        CHIP_ERROR err = CHIP_NO_ERROR;
-        chip::FabricIndex fabricIndex;
-        CurrentCommissioner().GetFabricIndex(&fabricIndex);
+        CHIP_ERROR err                                           = CHIP_NO_ERROR;
+        chip::FabricIndex fabricIndex                            = CurrentCommissioner().GetFabricIndex();
         chip::Credentials::GroupDataProvider * groupDataProvider = chip::Credentials::GetGroupDataProvider();
 
         // Unbind all group

diff --git a/examples/chip-tool/commands/tests/TestCommand.cpp b/examples/chip-tool/commands/tests/TestCommand.cpp
@@ -33,9 +33,8 @@ CHIP_ERROR TestCommand::RunCommand()
 CHIP_ERROR TestCommand::WaitForCommissionee(const char * identity,
                                             const chip::app::Clusters::DelayCommands::Commands::WaitForCommissionee::Type & value)
 {
-    chip::FabricIndex fabricIndex;
-
-    ReturnErrorOnFailure(GetCommissioner(identity).GetFabricIndex(&fabricIndex));
+    chip::FabricIndex fabricIndex = GetCommissioner(identity).GetFabricIndex();
+    ReturnErrorCodeIf(fabricIndex == chip::kUndefinedFabricIndex, CHIP_ERROR_INCORRECT_STATE);
 
     //
     // There's a chance the commissionee may have rebooted before this call here as part of a test flow

diff --git a/examples/platform/linux/CommissionerMain.cpp b/examples/platform/linux/CommissionerMain.cpp
@@ -171,30 +171,30 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort)
     ReturnErrorOnFailure(factory.Init(factoryParams));
     ReturnErrorOnFailure(factory.SetupCommissioner(params, gCommissioner));
 
-    chip::FabricInfo * fabricInfo = gCommissioner.GetFabricInfo();
-    VerifyOrReturnError(fabricInfo != nullptr, CHIP_ERROR_INTERNAL);
+    FabricIndex fabricIndex = gCommissioner.GetFabricIndex();
+    VerifyOrReturnError(fabricIndex != kUndefinedFabricIndex, CHIP_ERROR_INTERNAL);
 
     uint8_t compressedFabricId[sizeof(uint64_t)] = { 0 };
     MutableByteSpan compressedFabricIdSpan(compressedFabricId);
-    ReturnErrorOnFailure(fabricInfo->GetCompressedId(compressedFabricIdSpan));
-    ChipLogProgress(Support, "Setting up group data for Fabric Index %u with Compressed Fabric ID:",
-                    static_cast<unsigned>(fabricInfo->GetFabricIndex()));
+    ReturnErrorOnFailure(gCommissioner.GetCompressedFabricIdBytes(compressedFabricIdSpan));
+    ChipLogProgress(Support,
+                    "Setting up group data for Fabric Index %u with Compressed Fabric ID:", static_cast<unsigned>(fabricIndex));
     ChipLogByteSpan(Support, compressedFabricIdSpan);
 
     // TODO: Once ExampleOperationalCredentialsIssuer has support, set default IPK on it as well so
     // that commissioned devices get the IPK set from real values rather than "test-only" internal hookups.
     ByteSpan defaultIpk = chip::GroupTesting::DefaultIpkValue::GetDefaultIpk();
-    ReturnLogErrorOnFailure(chip::Credentials::SetSingleIpkEpochKey(&gGroupDataProvider, fabricInfo->GetFabricIndex(), defaultIpk,
-                                                                    compressedFabricIdSpan));
+    ReturnLogErrorOnFailure(
+        chip::Credentials::SetSingleIpkEpochKey(&gGroupDataProvider, fabricIndex, defaultIpk, compressedFabricIdSpan));
 
     gCommissionerDiscoveryController.SetUserDirectedCommissioningServer(gCommissioner.GetUserDirectedCommissioningServer());
     gCommissionerDiscoveryController.SetCommissionerCallback(&gCommissionerCallback);
 
     // advertise operational since we are an admin
     app::DnssdServer::Instance().AdvertiseOperational();
 
-    ChipLogProgress(Support, "InitCommissioner nodeId=0x" ChipLogFormatX64 " fabricIndex=%d",
-                    ChipLogValueX64(gCommissioner.GetNodeId()), fabricInfo->GetFabricIndex());
+    ChipLogProgress(Support, "InitCommissioner nodeId=0x" ChipLogFormatX64 " fabricIndex=0x%x",
+                    ChipLogValueX64(gCommissioner.GetNodeId()), static_cast<unsigned>(fabricIndex));
 
     return CHIP_NO_ERROR;
 }

diff --git a/examples/shell/cc13x2x7_26x2x7/args.gni b/examples/shell/cc13x2x7_26x2x7/args.gni
@@ -31,7 +31,7 @@ chip_enable_ota_requestor = false
 chip_openthread_ftd = false
 
 # Disable CHIP Logging
-#chip_progress_logging = false
+chip_progress_logging = false
 chip_detail_logging = false
 chip_automation_logging = false