diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index af21b52d4e194d..2a84ec920838e6 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -146,7 +146,7 @@ void CASESession::Clear() if (mFabricsTable) { - mFabricsTable->RemoveFabricDelegate(&mFabricDelegate); + mFabricsTable->RemoveFabricDelegate(this); } mLocalNodeId = kUndefinedNodeId; @@ -196,11 +196,13 @@ CASESession::PrepareForSessionEstablishment(SessionManager & sessionManager, Fab SessionEstablishmentDelegate * delegate, ScopedNodeId previouslyEstablishedPeer, Optional mrpConfig) { + // 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)); CHIP_ERROR err = CHIP_NO_ERROR; - SuccessOrExit(err = fabricTable->AddFabricDelegate(&mFabricDelegate)); + SuccessOrExit(err = fabricTable->AddFabricDelegate(this)); mFabricsTable = fabricTable; mRole = CryptoContext::SessionRole::kResponder; @@ -248,7 +250,7 @@ CHIP_ERROR CASESession::EstablishSession(SessionManager & sessionManager, Fabric // been initialized SuccessOrExit(err); - SuccessOrExit(err = fabricTable->AddFabricDelegate(&mFabricDelegate)); + SuccessOrExit(err = fabricTable->AddFabricDelegate(this)); mFabricsTable = fabricTable; mFabricIndex = fabricInfo->GetFabricIndex(); @@ -279,7 +281,7 @@ 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); @@ -1734,7 +1736,7 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea // 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 theortically pass along msg to a callback that gets + // 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; @@ -1837,7 +1839,7 @@ 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(); AbortPendingEstablish(err); diff --git a/src/protocols/secure_channel/CASESession.h b/src/protocols/secure_channel/CASESession.h index 2214948e1bdaa6..01601ba8d2676c 100644 --- a/src/protocols/secure_channel/CASESession.h +++ b/src/protocols/secure_channel/CASESession.h @@ -58,22 +58,10 @@ namespace chip { // when implementing concurrent CASE session. class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, public Messaging::ExchangeDelegate, + public FabricTable::Delegate, public PairingSession { public: - // Public so it is accessible to unit test - enum class State : uint8_t - { - kInitialized = 0, - kSentSigma1 = 1, - kSentSigma2 = 2, - kSentSigma3 = 3, - kSentSigma1Resume = 4, - kSentSigma2Resume = 5, - kFinished = 6, - kFinishedViaResume = 7, - }; - ~CASESession() override; Transport::SecureSession::Type GetSecureSessionType() const override { return Transport::SecureSession::Type::kCASE; } @@ -179,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. @@ -186,44 +196,18 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, **/ void Clear(); - void InvalidateIfPendingEstablishmentOnFabric(FabricIndex fabricIndex); - -#if CONFIG_IM_BUILD_FOR_UNIT_TEST - void SetStopSigmaHandshakeAt(Optional state) { mStopHandshakeAtState = state; } -#endif // CONFIG_IM_BUILD_FOR_UNIT_TEST - private: - class CASESessionFabricDelegate final : public chip::FabricTable::Delegate + friend class CASESessionForTest; + enum class State : uint8_t { - public: - CASESessionFabricDelegate(CASESession * caseSession) : mCASESession(caseSession) {} - - void OnFabricDeletedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override - { - (void) fabricTable; - mCASESession->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; - mCASESession->InvalidateIfPendingEstablishmentOnFabric(fabricIndex); - } - - private: - CASESession * mCASESession; + kInitialized = 0, + kSentSigma1 = 1, + kSentSigma2 = 2, + kSentSigma3 = 3, + kSentSigma1Resume = 4, + kSentSigma2Resume = 5, + kFinished = 6, + kFinishedViaResume = 7, }; /* @@ -286,6 +270,12 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, 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) { mStopHandshakeAtState = state; } +#endif // CONFIG_IM_BUILD_FOR_UNIT_TEST + Crypto::Hash_SHA256_stream mCommissioningHash; Crypto::P256PublicKey mRemotePubKey; #ifdef ENABLE_HSM_CASE_EPHEMERAL_KEY @@ -313,7 +303,6 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, // Sigma1 initiator random, maintained to be reused post-Sigma1, such as when generating Sigma2 S2RK key uint8_t mInitiatorRandom[kSigmaParamRandomNumberSize]; - CASESessionFabricDelegate mFabricDelegate{ this }; State mState; #if CONFIG_IM_BUILD_FOR_UNIT_TEST diff --git a/src/protocols/secure_channel/tests/TestCASESession.cpp b/src/protocols/secure_channel/tests/TestCASESession.cpp index 731e782e8f3249..339fc7123fc811 100644 --- a/src/protocols/secure_channel/tests/TestCASESession.cpp +++ b/src/protocols/secure_channel/tests/TestCASESession.cpp @@ -843,8 +843,18 @@ static void CASE_SessionResumptionStorage(nlTestSuite * inSuite, void * inContex } } +// TODO, move all tests above into this class #if CONFIG_IM_BUILD_FOR_UNIT_TEST -static void CASE_SimulateUpdateNOCInvalidatePendingEstablishment(nlTestSuite * inSuite, void * inContext) +namespace chip { +// TODO rename CASESessionForTest to TestCASESession. Not doing that immediately since that requires +// removing a lot of the `using namesapce` above which is a larger cleanup. +class CASESessionForTest +{ +public: + static void CASE_SimulateUpdateNOCInvalidatePendingEstablishment(nlTestSuite * inSuite, void * inContext); +}; + +void CASESessionForTest::CASE_SimulateUpdateNOCInvalidatePendingEstablishment(nlTestSuite * inSuite, void * inContext) { SessionManager sessionManager; TestCASESecurePairingDelegate delegateCommissioner; @@ -863,6 +873,8 @@ static void CASE_SimulateUpdateNOCInvalidatePendingEstablishment(nlTestSuite * i ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Protocols::SecureChannel::MsgType::CASE_Sigma1, &pairingAccessory) == CHIP_NO_ERROR); + // In order for all the test iterations below, we need to stop the CASE sigma handshake in the middle such + // that the CASE session is in the process of being established. pairingCommissioner.SetStopSigmaHandshakeAt(MakeOptional(CASESession::State::kSentSigma1)); ExchangeContext * contextCommissioner = ctx.NewUnauthenticatedExchangeToBob(&pairingCommissioner); @@ -882,26 +894,32 @@ static void CASE_SimulateUpdateNOCInvalidatePendingEstablishment(nlTestSuite * i nullptr, nullptr, &delegateCommissioner) == CHIP_NO_ERROR); ctx.DrainAndServiceIO(); + // At this point the CASESession is in the process of establishing. Confirm that there are no errors and there are session + // has not been established. NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingComplete == 0); NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingComplete == 0); NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingErrors == 0); NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingErrors == 0); + // Simulating an update to the Fabric NOC for gCommissionerFabrics fabric table. + // Confirm that CASESession on commisioner side has reported an error. gCommissionerFabrics.SendUpdateFabricNotificationForTest(gCommissionerFabricIndex); ctx.DrainAndServiceIO(); - NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingErrors == 0); NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingErrors == 1); + // Simulating an update to the Fabric NOC for gDeviceFabrics fabric table. + // Confirm that CASESession on accessory side has reported an error. gDeviceFabrics.SendUpdateFabricNotificationForTest(gDeviceFabricIndex); ctx.DrainAndServiceIO(); - NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingErrors == 1); NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingErrors == 1); + // Sanity check that pairing did not complete. NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingComplete == 0); NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingComplete == 0); } +} // namespace chip #endif // CONFIG_IM_BUILD_FOR_UNIT_TEST // Test Suite @@ -922,7 +940,7 @@ static const nlTest sTests[] = #if CONFIG_IM_BUILD_FOR_UNIT_TEST // This is compiled for host tests which is enough test coverage to ensure updating NOC invalidates // CASESession that are in the process of establishing. - NL_TEST_DEF("InvalidatePendingSessionEstablishment", CASE_SimulateUpdateNOCInvalidatePendingEstablishment), + NL_TEST_DEF("InvalidatePendingSessionEstablishment", chip::CASESessionForTest::CASE_SimulateUpdateNOCInvalidatePendingEstablishment), #endif // CONFIG_IM_BUILD_FOR_UNIT_TEST NL_TEST_SENTINEL()