Invalidate pending CASESession establishment when Fabric changes (#19346

)

* Add invalidate pending CASESession establishment when Fabric changes

* Address comment and clean up after merging master

* Fix runtime and compile time errors

* Clean up a couple of TODOs

* Breakout OnFabricNOCUpdated as a standalone callback

* restyle

* restyle and figuring out unit tests

* Add unit test and fix issue discovered by unit test

* Fix formating issue

* clean up TestCASESession

* Change simulating updating fabric NOC test to run in host tests only

* Address PR comments

* Address PR comments

src/app/clusters/bindings/BindingManager.cpp

@@ -48,6 +48,9 @@ class BindingFabricTableDelegate : public chip::FabricTable::Delegate
 
     // Intentionally left blank
     void OnFabricPersistedToStorage(chip::FabricTable & fabricTable, chip::FabricIndex fabricIndex) override {}
+
+    // Intentionally left blank
+    void OnFabricNOCUpdated(chip::FabricTable & fabricTable, chip::FabricIndex fabricIndex) override {}
 };
 
 BindingFabricTableDelegate gFabricTableDelegate;

src/app/clusters/door-lock-server/door-lock-server.cpp

@@ -73,6 +73,9 @@ class DoorLockClusterFabricDelegate : public chip::FabricTable::Delegate
 
     // Intentionally left blank
     void OnFabricPersistedToStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override {}
+
+    // Intentionally left blank
+    void OnFabricNOCUpdated(chip::FabricTable & fabricTable, chip::FabricIndex fabricIndex) override {}
 };
 static DoorLockClusterFabricDelegate gFabricDelegate;
 

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp

@@ -460,6 +460,9 @@ class OpCredsFabricTableDelegate : public chip::FabricTable::Delegate
                         ChipLogValueX64(fabric->GetNodeId()), fabric->GetVendorId());
         fabricListChanged();
     }
+
+    // This is triggered by operation credential server so there is nothing additional that we need to do.
+    void OnFabricNOCUpdated(chip::FabricTable & fabricTable, chip::FabricIndex fabricIndex) override {}
 };
 
 OpCredsFabricTableDelegate gFabricDelegate;

src/app/server/Server.h

@@ -437,6 +437,12 @@ class Server
             (void) fabricIndex;
         }
 
+        void OnFabricNOCUpdated(chip::FabricTable & fabricTable, chip::FabricIndex fabricIndex) override
+        {
+            (void) fabricTable;
+            (void) fabricIndex;
+        }
+
     private:
         Server * mServer = nullptr;
     };

src/controller/CHIPDeviceControllerFactory.h

@@ -200,6 +200,12 @@ class DeviceControllerFactory
             (void) fabricIndex;
         }
 
+        void OnFabricNOCUpdated(chip::FabricTable & fabricTable, chip::FabricIndex fabricIndex) override
+        {
+            (void) fabricTable;
+            (void) fabricIndex;
+        }
+
     private:
         SessionManager * mSessionManager                    = nullptr;
         Credentials::GroupDataProvider * mGroupDataProvider = nullptr;

src/credentials/FabricTable.cpp

@@ -754,6 +754,20 @@ class NotBeforeCollector : public Credentials::CertificateValidityPolicy
     System::Clock::Seconds32 mLatestNotBefore;
 };
 
+CHIP_ERROR FabricTable::NotifyNOCUpdatedOnFabric(FabricIndex fabricIndex)
+{
+    FabricTable::Delegate * delegate = mDelegateListRoot;
+    while (delegate)
+    {
+        // It is possible that delegate will remove itself from the list in OnFabricNOCUpdated,
+        // so we grab the next delegate in the list now.
+        FabricTable::Delegate * nextDelegate = delegate->next;
+        delegate->OnFabricNOCUpdated(*this, fabricIndex);
+        delegate = nextDelegate;
+    }
+    return CHIP_NO_ERROR;
+}
+
 CHIP_ERROR FabricTable::UpdateFabric(FabricIndex fabricIndex, FabricInfo & newFabricInfo)
 {
     FabricInfo * fabricInfo = FindFabricWithIndex(fabricIndex);
@@ -766,6 +780,9 @@ CHIP_ERROR FabricTable::UpdateFabric(FabricIndex fabricIndex, FabricInfo & newFa
     // for CASE and current time is also unknown, the certificate
     // validity policy will see this condition and can act appropriately.
     mLastKnownGoodTime.UpdateLastKnownGoodChipEpochTime(notBeforeCollector.mLatestNotBefore);
+
+    NotifyNOCUpdatedOnFabric(fabricIndex);
+
     return CHIP_NO_ERROR;
 }
 
@@ -892,8 +909,11 @@ CHIP_ERROR FabricTable::Delete(FabricIndex fabricIndex)
         FabricTable::Delegate * delegate = mDelegateListRoot;
         while (delegate)
         {
+            // It is possible that delegate will remove itself from the list in OnFabricDeletedFromStorage,
+            // so we grab the next delegate in the list now.
+            FabricTable::Delegate * nextDelegate = delegate->next;
             delegate->OnFabricDeletedFromStorage(*this, fabricIndex);
-            delegate = delegate->next;
+            delegate = nextDelegate;
         }
     }
     return CHIP_NO_ERROR;

src/credentials/FabricTable.h

@@ -364,6 +364,11 @@ class DLL_EXPORT FabricTable
          **/
         virtual void OnFabricPersistedToStorage(FabricTable & fabricTable, FabricIndex fabricIndex) = 0;
 
+        /**
+         * Gets called when operational credentials are changed.
+         **/
+        virtual void OnFabricNOCUpdated(FabricTable & fabricTable, FabricIndex fabricIndex) = 0;
+
         // Intrusive list pointer for FabricTable to manage the entries.
         Delegate * next = nullptr;
     };
@@ -404,6 +409,12 @@ class DLL_EXPORT FabricTable
      */
     CHIP_ERROR UpdateFabric(FabricIndex fabricIndex, FabricInfo & fabricInfo);
 
+    // TODO this #if CONFIG_IM_BUILD_FOR_UNIT_TEST is temporary. There is a change incoming soon
+    // that will allow triggering NOC update directly.
+#if CONFIG_IM_BUILD_FOR_UNIT_TEST
+    void SendUpdateFabricNotificationForTest(FabricIndex fabricIndex) { NotifyNOCUpdatedOnFabric(fabricIndex); }
+#endif // CONFIG_IM_BUILD_FOR_UNIT_TEST
+
     FabricInfo * FindFabric(const Crypto::P256PublicKey & rootPubKey, FabricId fabricId);
     FabricInfo * FindFabricWithIndex(FabricIndex fabricIndex);
     const FabricInfo * FindFabricWithIndex(FabricIndex fabricIndex) const;
@@ -594,6 +605,8 @@ class DLL_EXPORT FabricTable
 
     CHIP_ERROR AddNewFabricInner(FabricInfo & fabric, FabricIndex * assignedIndex);
 
+    CHIP_ERROR NotifyNOCUpdatedOnFabric(FabricIndex fabricIndex);
+
     FabricInfo mStates[CHIP_CONFIG_MAX_FABRICS];
     PersistentStorageDelegate * mStorage               = nullptr;
     Crypto::OperationalKeystore * mOperationalKeystore = nullptr;

src/protocols/secure_channel/CASESession.cpp

@@ -144,12 +144,30 @@ void CASESession::Clear()
     mState = State::kInitialized;
     Crypto::ClearSecretData(mIPK);
 
+    if (mFabricsTable)
+    {
+        mFabricsTable->RemoveFabricDelegate(this);
+    }
+
     mLocalNodeId  = kUndefinedNodeId;
     mPeerNodeId   = kUndefinedNodeId;
     mFabricsTable = nullptr;
     mFabricIndex  = kUndefinedFabricIndex;
 }
 
+void CASESession::InvalidateIfPendingEstablishmentOnFabric(FabricIndex fabricIndex)
+{
+    if (mFabricIndex != fabricIndex)
+    {
+        return;
+    }
+    if (!IsSessionEstablishmentInProgress())
+    {
+        return;
+    }
+    AbortPendingEstablish(CHIP_ERROR_CANCELLED);
+}
+
 CHIP_ERROR CASESession::Init(SessionManager & sessionManager, Credentials::CertificateValidityPolicy * policy,
                              SessionEstablishmentDelegate * delegate, const ScopedNodeId & sessionEvictionHint)
 {
@@ -172,24 +190,34 @@ CHIP_ERROR CASESession::Init(SessionManager & sessionManager, Credentials::Certi
 }
 
 CHIP_ERROR
-CASESession::PrepareForSessionEstablishment(SessionManager & sessionManager, FabricTable * fabrics,
+CASESession::PrepareForSessionEstablishment(SessionManager & sessionManager, FabricTable * fabricTable,
                                             SessionResumptionStorage * sessionResumptionStorage,
                                             Credentials::CertificateValidityPolicy * policy,
                                             SessionEstablishmentDelegate * delegate, ScopedNodeId previouslyEstablishedPeer,
                                             Optional<ReliableMessageProtocolConfig> mrpConfig)
 {
-    VerifyOrReturnError(fabrics != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+    // Below VerifyOrReturnError is not SuccessOrExit since we only want to goto `exit:` after
+    // Init has been successfully called.
+    VerifyOrReturnError(fabricTable != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
     ReturnErrorOnFailure(Init(sessionManager, policy, delegate, previouslyEstablishedPeer));
 
-    mFabricsTable             = fabrics;
+    CHIP_ERROR err = CHIP_NO_ERROR;
+    SuccessOrExit(err = fabricTable->AddFabricDelegate(this));
+
+    mFabricsTable             = fabricTable;
     mRole                     = CryptoContext::SessionRole::kResponder;
     mSessionResumptionStorage = sessionResumptionStorage;
     mLocalMRPConfig           = mrpConfig;
 
     ChipLogDetail(SecureChannel, "Allocated SecureSession (%p) - waiting for Sigma1 msg",
                   mSecureSessionHolder.Get().Value()->AsSecureSession());
 
-    return CHIP_NO_ERROR;
+exit:
+    if (err != CHIP_NO_ERROR)
+    {
+        Clear();
+    }
+    return err;
 }
 
 CHIP_ERROR CASESession::EstablishSession(SessionManager & sessionManager, FabricTable * fabricTable, ScopedNodeId peerScopedNodeId,
@@ -222,6 +250,8 @@ CHIP_ERROR CASESession::EstablishSession(SessionManager & sessionManager, Fabric
     // been initialized
     SuccessOrExit(err);
 
+    SuccessOrExit(err = fabricTable->AddFabricDelegate(this));
+
     mFabricsTable             = fabricTable;
     mFabricIndex              = fabricInfo->GetFabricIndex();
     mSessionResumptionStorage = sessionResumptionStorage;
@@ -251,12 +281,17 @@ void CASESession::OnResponseTimeout(ExchangeContext * ec)
     VerifyOrReturn(mExchangeCtxt == ec, ChipLogError(SecureChannel, "CASESession::OnResponseTimeout exchange doesn't match"));
     ChipLogError(SecureChannel, "CASESession timed out while waiting for a response from the peer. Current state was %u",
                  to_underlying(mState));
-    // Discard the exchange so that Clear() doesn't try closing it.  The
+    // Discard the exchange so that Clear() doesn't try aborting it.  The
     // exchange will handle that.
     DiscardExchange();
+    AbortPendingEstablish(CHIP_ERROR_TIMEOUT);
+}
+
+void CASESession::AbortPendingEstablish(CHIP_ERROR err)
+{
     Clear();
     // Do this last in case the delegate frees us.
-    mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT);
+    mDelegate->OnSessionEstablishmentError(err);
 }
 
 CHIP_ERROR CASESession::DeriveSecureSession(CryptoContext & session) const
@@ -1692,6 +1727,22 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea
     Protocols::SecureChannel::MsgType msgType = static_cast<Protocols::SecureChannel::MsgType>(payloadHeader.GetMessageType());
     SuccessOrExit(err);
 
+#if CONFIG_IM_BUILD_FOR_UNIT_TEST
+    if (mStopHandshakeAtState.HasValue() && mState == mStopHandshakeAtState.Value())
+    {
+        mStopHandshakeAtState = Optional<State>::Missing();
+        // For testing purposes we are trying to stop a successful CASESession from happening by dropping part of the
+        // handshake in the middle. We are trying to keep both sides of the CASESession establishment in an active
+        // pending state. In order to keep this side open we have to tell the exchange context that we will send an
+        // async message.
+        //
+        // Should you need to resume the CASESession, you could theoretically pass along the msg to a callback that gets
+        // registered when setting mStopHandshakeAtState.
+        mExchangeCtxt->WillSendMessage();
+        return CHIP_NO_ERROR;
+    }
+#endif // CONFIG_IM_BUILD_FOR_UNIT_TEST
+
 #if CHIP_CONFIG_SLOW_CRYPTO
     if (msgType == Protocols::SecureChannel::MsgType::CASE_Sigma1 || msgType == Protocols::SecureChannel::MsgType::CASE_Sigma2 ||
         msgType == Protocols::SecureChannel::MsgType::CASE_Sigma2Resume ||
@@ -1788,12 +1839,10 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea
     // Call delegate to indicate session establishment failure.
     if (err != CHIP_NO_ERROR)
     {
-        // Discard the exchange so that Clear() doesn't try closing it.  The
+        // Discard the exchange so that Clear() doesn't try aborting it.  The
         // exchange will handle that.
         DiscardExchange();
-        Clear();
-        // Do this last in case the delegate frees us.
-        mDelegate->OnSessionEstablishmentError(err);
+        AbortPendingEstablish(err);
     }
     return err;
 }

src/protocols/secure_channel/CASESession.h

@@ -58,6 +58,7 @@ namespace chip {
 // when implementing concurrent CASE session.
 class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
                                public Messaging::ExchangeDelegate,
+                               public FabricTable::Delegate,
                                public PairingSession
 {
 public:
@@ -73,7 +74,7 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
      *   Initialize using configured fabrics and wait for session establishment requests.
      *
      * @param sessionManager                session manager from which to allocate a secure session object
-     * @param fabrics                       Table of fabrics that are currently configured on the device
+     * @param fabricTable                   Table of fabrics that are currently configured on the device
      * @param policy                        Optional application-provided certificate validity policy
      * @param delegate                      Callback object
      * @param previouslyEstablishedPeer     If a session had previously been established successfully to a peer, this should
@@ -84,7 +85,7 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
      * @return CHIP_ERROR     The result of initialization
      */
     CHIP_ERROR PrepareForSessionEstablishment(
-        SessionManager & sessionManager, FabricTable * fabrics, SessionResumptionStorage * sessionResumptionStorage,
+        SessionManager & sessionManager, FabricTable * fabricTable, SessionResumptionStorage * sessionResumptionStorage,
         Credentials::CertificateValidityPolicy * policy, SessionEstablishmentDelegate * delegate,
         ScopedNodeId previouslyEstablishedPeer,
         Optional<ReliableMessageProtocolConfig> mrpConfig = Optional<ReliableMessageProtocolConfig>::Missing());
@@ -166,6 +167,28 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
     //// SessionDelegate ////
     void OnSessionReleased() override;
 
+    //// FabricTable::Delegate Implementation ////
+    void OnFabricDeletedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override
+    {
+        (void) fabricTable;
+        InvalidateIfPendingEstablishmentOnFabric(fabricIndex);
+    }
+    void OnFabricRetrievedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override
+    {
+        (void) fabricTable;
+        (void) fabricIndex;
+    }
+    void OnFabricPersistedToStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override
+    {
+        (void) fabricTable;
+        (void) fabricIndex;
+    }
+    void OnFabricNOCUpdated(chip::FabricTable & fabricTable, chip::FabricIndex fabricIndex) override
+    {
+        (void) fabricTable;
+        InvalidateIfPendingEstablishmentOnFabric(fabricIndex);
+    }
+
     FabricIndex GetFabricIndex() const { return mFabricIndex; }
 
     // TODO: remove Clear, we should create a new instance instead reset the old instance.
@@ -174,6 +197,7 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
     void Clear();
 
 private:
+    friend class CASESessionForTest;
     enum class State : uint8_t
     {
         kInitialized       = 0,
@@ -237,13 +261,21 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
     void OnSuccessStatusReport() override;
     CHIP_ERROR OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode) override;
 
+    void AbortPendingEstablish(CHIP_ERROR err);
+
     CHIP_ERROR GetHardcodedTime();
 
     CHIP_ERROR SetEffectiveTime();
 
     CHIP_ERROR ValidateReceivedMessage(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader,
                                        const System::PacketBufferHandle & msg);
 
+    void InvalidateIfPendingEstablishmentOnFabric(FabricIndex fabricIndex);
+
+#if CONFIG_IM_BUILD_FOR_UNIT_TEST
+    void SetStopSigmaHandshakeAt(Optional<State> state) { mStopHandshakeAtState = state; }
+#endif // CONFIG_IM_BUILD_FOR_UNIT_TEST
+
     Crypto::Hash_SHA256_stream mCommissioningHash;
     Crypto::P256PublicKey mRemotePubKey;
 #ifdef ENABLE_HSM_CASE_EPHEMERAL_KEY
@@ -272,6 +304,10 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
     uint8_t mInitiatorRandom[kSigmaParamRandomNumberSize];
 
     State mState;
+
+#if CONFIG_IM_BUILD_FOR_UNIT_TEST
+    Optional<State> mStopHandshakeAtState = Optional<State>::Missing();
+#endif // CONFIG_IM_BUILD_FOR_UNIT_TEST
 };
 
 } // namespace chip

src/protocols/secure_channel/PairingSession.cpp

@@ -138,6 +138,17 @@ CHIP_ERROR PairingSession::DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TL
     return tlvReader.ExitContainer(containerType);
 }
 
+bool PairingSession::IsSessionEstablishmentInProgress()
+{
+    if (!mSecureSessionHolder)
+    {
+        return false;
+    }
+
+    Transport::SecureSession * secureSession = mSecureSessionHolder->AsSecureSession();
+    return secureSession->IsPairing();
+}
+
 void PairingSession::Clear()
 {
     // Clear acts like the destructor if PairingSession, if it is call during

src/protocols/secure_channel/PairingSession.h

@@ -187,6 +187,8 @@ class DLL_EXPORT PairingSession : public SessionDelegate
      */
     CHIP_ERROR DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TLV::ContiguousBufferTLVReader & tlvReader);
 
+    bool IsSessionEstablishmentInProgress();
+
     // TODO: remove Clear, we should create a new instance instead reset the old instance.
     void Clear();