Properly manage operational key lifecycle for fail-safe (#19277)

* Properly manage operational key lifecycle for fail-safe - Fail-safe did not properly manage the roll-back of operational keys - Operational key storage being centralized by value in FabricTable prevented ability to back keys by hardware/OS and allow the rollback of keys on failsafe expiry - CASE code was using "raw" FabricInfo * which could go stale on UpdateNOC or after fail-safe expiry. This PR: - Adds an OperationalKeystore interface - Make the FabricTable use the OperationalKeystore for when a commissionable node (with Opcreds cluster) is being commissioned - Retain legacy controller behavior that allows injection of operational keys - Simplifies the fail-safe handling lifecycle - Add logging to fail-safe handling - Add logging to general commissioning cluster - Make CASE use ScopedNodeId everywhere - Implement IsForUpdateNOC in fail-safe and opcreds cluster Fixes #19072 Issue #18633 Fixes #16443 * Fix merge of upstream * Restyled by whitespace * Restyled by clang-format * Revert unintended testing changes * Add remove operation * Fix CI and add tests to support further tests * Fix more CI * Restyled by clang-format * Darwin changes to use the new setup * Added unit test and HasOpKeypairForFabric() * Restyled by clang-format * Restyled by gn * Apply review comments from @msandstedt * Add plumbing for init of controllers * Restyled by clang-format * Fix darwin tests * Fix CI and address review comments * Fix comment typos * Apply review comments from @bzbarsky-apple and @tehampson * Restyled by clang-format * Fix more comments * Restyled by clang-format * Fix CI * Fix cirque * Restyled by clang-format * Update src/crypto/tests/TestPersistentStorageOpKeyStore.cpp Co-authored-by: tehampson <[email protected]> * Address review comments * Fix CI * More clang-tidy fixes Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Justin Wood <[email protected]> Co-authored-by: tehampson <[email protected]>
project-chip · Jun 10, 2022 · 4e3930a · 4e3930a
1 parent 5dd202c
commit 4e3930a
Show file tree

Hide file tree

Showing 42 changed files with 1,862 additions and 497 deletions.
diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp
@@ -71,10 +71,12 @@ CHIP_ERROR CHIPCommand::MaybeSetUpStack()
 #endif
 
     ReturnLogErrorOnFailure(mDefaultStorage.Init());
+    ReturnLogErrorOnFailure(mOperationalKeystore.Init(&mDefaultStorage));
 
     chip::Controller::FactoryInitParams factoryInitParams;
 
     factoryInitParams.fabricIndependentStorage = &mDefaultStorage;
+    factoryInitParams.operationalKeystore      = &mOperationalKeystore;
 
     // 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

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

diff --git a/src/app/CASEClient.cpp b/src/app/CASEClient.cpp
@@ -30,6 +30,8 @@ CHIP_ERROR CASEClient::EstablishSession(PeerId peer, const Transport::PeerAddres
                                         const ReliableMessageProtocolConfig & remoteMRPConfig,
                                         SessionEstablishmentDelegate * delegate)
 {
+    VerifyOrReturnError(mInitParams.fabricTable != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+
     // Create a UnauthenticatedSession for CASE pairing.
     Optional<SessionHandle> session = mInitParams.sessionManager->CreateUnauthenticatedSession(peerAddress, remoteMRPConfig);
     VerifyOrReturnError(session.HasValue(), CHIP_ERROR_NO_MEMORY);
@@ -45,7 +47,7 @@ CHIP_ERROR CASEClient::EstablishSession(PeerId peer, const Transport::PeerAddres
 
     mCASESession.SetGroupDataProvider(mInitParams.groupDataProvider);
     ReturnErrorOnFailure(mCASESession.EstablishSession(
-        *mInitParams.sessionManager, mInitParams.fabricTable, mInitParams.fabricIndex, peer.GetNodeId(), exchange,
+        *mInitParams.sessionManager, mInitParams.fabricTable, ScopedNodeId{ peer.GetNodeId(), mInitParams.fabricIndex }, exchange,
         mInitParams.sessionResumptionStorage, mInitParams.certificateValidityPolicy, delegate, mInitParams.mrpLocalConfig));
 
     return CHIP_NO_ERROR;

diff --git a/src/app/OperationalDeviceProxy.h b/src/app/OperationalDeviceProxy.h
@@ -199,7 +199,7 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
     }
 
     /**
-     * @brief Get the raw Fabric ID assigned to the device.
+     * @brief Get the fabricIndex
      */
     FabricIndex GetFabricIndex() const { return mFabricIndex; }
 

diff --git a/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp b/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp
@@ -155,6 +155,9 @@ bool emberAfGeneralCommissioningClusterArmFailSafeCallback(app::CommandHandler *
     FailSafeContext & failSafeContext = DeviceLayer::DeviceControlServer::DeviceControlSvr().GetFailSafeContext();
     Commands::ArmFailSafeResponse::Type response;
 
+    ChipLogProgress(FailSafe, "GeneralCommissioning: Received ArmFailSafe (%us)",
+                    static_cast<unsigned>(commandData.expiryLengthSeconds));
+
     /*
      * If the fail-safe timer is not fully disarmed, don't allow arming a new fail-safe.
      * If the fail-safe timer was not currently armed, then the fail-safe timer SHALL be armed.
@@ -214,7 +217,10 @@ bool emberAfGeneralCommissioningClusterCommissioningCompleteCallback(
     MATTER_TRACE_EVENT_SCOPE("CommissioningComplete", "GeneralCommissioning");
 
     DeviceControlServer * server = &DeviceLayer::DeviceControlServer::DeviceControlSvr();
-    const auto & failSafe        = server->GetFailSafeContext();
+    auto & failSafe              = server->GetFailSafeContext();
+    auto & fabricTable           = Server::GetInstance().GetFabricTable();
+
+    ChipLogProgress(FailSafe, "GeneralCommissioning: Received CommissioningComplete");
 
     Commands::CommissioningCompleteResponse::Type response;
     if (!failSafe.IsFailSafeArmed())
@@ -231,16 +237,34 @@ bool emberAfGeneralCommissioningClusterCommissioningCompleteCallback(
             !failSafe.MatchesFabricIndex(commandObj->GetAccessingFabricIndex()))
         {
             response.errorCode = CommissioningError::kInvalidAuthentication;
+            ChipLogError(FailSafe, "GeneralCommissioning: Got commissioning complete in invalid security context");
         }
         else
         {
+            if (failSafe.NocCommandHasBeenInvoked())
+            {
+                CHIP_ERROR err = fabricTable.CommitPendingFabricData();
+                if (err != CHIP_NO_ERROR)
+                {
+                    ChipLogError(FailSafe, "GeneralCommissioning: Failed to commit pending fabric data: %" CHIP_ERROR_FORMAT,
+                                 err.Format());
+                }
+                else
+                {
+                    ChipLogProgress(FailSafe, "GeneralCommissioning: Successfully commited pending fabric data");
+                }
+                CheckSuccess(err, Failure);
+            }
+
             /*
              * Pass fabric of commissioner to DeviceControlSvr.
              * This allows device to send messages back to commissioner.
              * Once bindings are implemented, this may no longer be needed.
              */
-            CheckSuccess(server->CommissioningComplete(handle->AsSecureSession()->GetPeerNodeId(), handle->GetFabricIndex()),
-                         Failure);
+            failSafe.DisarmFailSafe();
+            CheckSuccess(
+                server->PostCommissioningCompleteEvent(handle->AsSecureSession()->GetPeerNodeId(), handle->GetFabricIndex()),
+                Failure);
 
             Breadcrumb::Set(commandPath.mEndpointId, 0);
             response.errorCode = CommissioningError::kOk;

diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp
@@ -486,7 +486,7 @@ void Instance::OnResult(Status commissioningError, CharSpan debugText, int32_t i
     }
     if (commissioningError == Status::kSuccess)
     {
-        DeviceLayer::DeviceControlServer::DeviceControlSvr().ConnectNetworkForOperational(
+        DeviceLayer::DeviceControlServer::DeviceControlSvr().PostConnectedToOperationalNetworkEvent(
             ByteSpan(mLastNetworkID, mLastNetworkIDLen));
         mLastConnectErrorValue.SetNull();
     }
-Original file line number
+Diff line change
@@ Expand Up @@
         }
         /**
-         * @brief Get the raw Fabric ID assigned to the device.
+         * @brief Get the fabricIndex
          */
         FabricIndex GetFabricIndex() const { return mFabricIndex; }
@@ Expand Down @@