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 <thampson@google.com>

* Address review comments

* Fix CI

* More clang-tidy fixes

Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Co-authored-by: Justin Wood <woody@apple.com>
Co-authored-by: tehampson <thampson@google.com>

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

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;
 

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;

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; }
 

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;

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();
  }