[1/3] Reserve SecureSession for CASE establishment on the responder…

… at init time (#19261)

* Initial commit.

* Review feedback

* Review feedback

* Minor integer promotion issue in a test

* Linux build fix

src/app/server/Server.cpp

@@ -348,6 +348,7 @@ void Server::ScheduleFactoryReset()
 
 void Server::Shutdown()
 {
+ mCASEServer.Shutdown();
  mCASESessionManager.Shutdown();
  app::DnssdServer::Instance().SetCommissioningModeProvider(nullptr);
  chip::Dnssd::ServiceAdvertiser::Instance().Shutdown();

src/controller/CHIPDeviceControllerFactory.cpp

@@ -339,6 +339,7 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown()
 
  if (mCASEServer != nullptr)
  {
+ mCASEServer->Shutdown();
  chip::Platform::Delete(mCASEServer);
  mCASEServer = nullptr;
  }

src/lib/core/CHIPConfig.h

@@ -644,15 +644,21 @@
 #endif // CHIP_CONFIG_UNAUTHENTICATED_CONNECTION_POOL_SIZE
 
 /**
- * @def CHIP_CONFIG_PEER_CONNECTION_POOL_SIZE
+ * @def CHIP_CONFIG_SECURE_SESSION_POOL_SIZE
+ *
+ * @brief Defines the size of the pool used for tracking the state of
+ * secure sessions. This controls the maximum number of concurrent
+ * established secure sessions across all supported transports.
+ *
+ * This is sized to cover the sum of the following:
+ * - At least 3 CASE sessions / fabric (Spec Ref: 4.13.2.8)
+ * - 1 reserved slot for CASEServer as a responder.
+ * - 1 reserved slot for PASE.
  *
- * @brief Define the size of the pool used for tracking CHIP
- * Peer connections. This defines maximum number of concurrent
- * device connections across all supported transports.
  */
-#ifndef CHIP_CONFIG_PEER_CONNECTION_POOL_SIZE
-#define CHIP_CONFIG_PEER_CONNECTION_POOL_SIZE 16
-#endif // CHIP_CONFIG_PEER_CONNECTION_POOL_SIZE
+#ifndef CHIP_CONFIG_SECURE_SESSION_POOL_SIZE
+#define CHIP_CONFIG_SECURE_SESSION_POOL_SIZE (CHIP_CONFIG_MAX_FABRICS * 3 + 2)
+#endif // CHIP_CONFIG_SECURE_SESSION_POOL_SIZE
 
 /**
  * @def CHIP_CONFIG_SECURE_SESSION_REFCOUNT_LOGGING

src/protocols/secure_channel/CASEServer.cpp

@@ -45,20 +45,18 @@ CHIP_ERROR CASEServer::ListenForSessionEstablishment(Messaging::ExchangeManager
  mExchangeManager = exchangeManager;
  mGroupDataProvider = responderGroupDataProvider;
 
- Cleanup();
+ // Set up the group state provider that persists across all handshakes.
+ GetSession().SetGroupDataProvider(mGroupDataProvider);
+
+ PrepareForSessionEstablishment();
+
  return CHIP_NO_ERROR;
 }
 
 CHIP_ERROR CASEServer::InitCASEHandshake(Messaging::ExchangeContext * ec)
 {
  ReturnErrorCodeIf(ec == nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
- // Setup CASE state machine using the credentials for the current fabric.
- GetSession().SetGroupDataProvider(mGroupDataProvider);
- ReturnErrorOnFailure(GetSession().ListenForSessionEstablishment(
- *mSessionManager, mFabrics, mSessionResumptionStorage, mCertificateValidityPolicy, this,
- Optional<ReliableMessageProtocolConfig>::Value(GetLocalMRPConfig())));
-
  // Hand over the exchange context to the CASE session.
  ec->SetDelegate(&GetSession());
 
@@ -90,31 +88,71 @@ CHIP_ERROR CASEServer::OnMessageReceived(Messaging::ExchangeContext * ec, const
 exit:
  if (err != CHIP_NO_ERROR)
  {
- Cleanup();
+ PrepareForSessionEstablishment();
  }
+
  return err;
 }
 
-void CASEServer::Cleanup()
+void CASEServer::PrepareForSessionEstablishment(const ScopedNodeId & previouslyEstablishedPeer)
 {
  // Let's re-register for CASE Sigma1 message, so that the next CASE session setup request can be processed.
  // https://github.com/project-chip/connectedhomeip/issues/8342
  ChipLogProgress(Inet, "CASE Server enabling CASE session setups");
  mExchangeManager->RegisterUnsolicitedMessageHandlerForType(Protocols::SecureChannel::MsgType::CASE_Sigma1, this);
 
  GetSession().Clear();
+
+ //
+ // Indicate to the underlying CASE session to prepare for session establishment requests coming its way. This will
+ // involve allocating a SecureSession that will be held until it's needed for the next CASE session handshake.
+ //
+ // Logically speaking, we're attempting to evict a session using details of the just-established session (to ensure
+ // we're evicting sessions from the right fabric if needed) and then transferring the just established session into that
+ // slot (and thereby free'ing up the slot for the next session attempt). However, this transfer isn't necessary - just
+ // evicting a session will ensure it is available for the next attempt.
+ //
+ // This call can fail if we have run out memory to allocate SecureSessions. Continuing without taking any action
+ // however will render this node deaf to future handshake requests, so it's better to die here to raise attention to the problem
+ // / facilitate recovery.
+ //
+ // TODO(#17568): Once session eviction is actually in place, this call should NEVER fail and if so, is a logic bug.
+ // Dying here on failure is even more appropriate then.
+ //
+ VerifyOrDie(GetSession().PrepareForSessionEstablishment(*mSessionManager, mFabrics, mSessionResumptionStorage,
+ mCertificateValidityPolicy, this, previouslyEstablishedPeer,
+ Optional<ReliableMessageProtocolConfig>::Value(GetLocalMRPConfig())) ==
+ CHIP_NO_ERROR);
+
+ //
+ // PairingSession::mSecureSessionHolder is a weak-reference. If MarkForRemoval is called on this session, the session is
+ // going to get de-allocated from underneath us. This session that has just been allocated should *never* get evicted, and
+ // remain available till the next hand-shake is received.
+ //
+ // TODO: Converting SessionHolder to a true weak-ref and making PairingSession hold a strong-ref (#18397) would avoid this
+ // headache...
+ //
+ // Let's create a SessionHandle strong-reference to it to keep it resident.
+ //
+ mPinnedSecureSession = GetSession().CopySecureSession();
+
+ //
+ // If we've gotten this far, it means we have successfully allocated a SecureSession to back our next attempt. If we haven't,
+ // there is a bug somewhere and we should raise attention to it by dying.
+ //
+ VerifyOrDie(mPinnedSecureSession.HasValue());
 }
 
 void CASEServer::OnSessionEstablishmentError(CHIP_ERROR err)
 {
  ChipLogError(Inet, "CASE Session establishment failed: %s", ErrorStr(err));
- Cleanup();
+ PrepareForSessionEstablishment();
 }
 
 void CASEServer::OnSessionEstablished(const SessionHandle & session)
 {
  ChipLogProgress(Inet, "CASE Session established to peer: " ChipLogFormatScopedNodeId,
  ChipLogValueScopedNodeId(session->GetPeer()));
- Cleanup();
+ PrepareForSessionEstablishment(session->GetPeer());
 }
 } // namespace chip

src/protocols/secure_channel/CASEServer.h

@@ -31,12 +31,23 @@ class CASEServer : public SessionEstablishmentDelegate,
 {
 public:
  CASEServer() {}
- ~CASEServer() override
+ ~CASEServer() override { Shutdown(); }
+
+ /*
+ * This method will shutdown this object, releasing the strong reference to the pinned SecureSession object.
+ * It will also unregister the unsolicited handler and clear out the session object (which will release the weak
+ * reference through the underlying SessionHolder).
+ *
+ */
+ void Shutdown()
  {
  if (mExchangeManager != nullptr)
  {
  mExchangeManager->UnregisterUnsolicitedMessageHandlerForType(Protocols::SecureChannel::MsgType::CASE_Sigma1);
  }
+
+ GetSession().Clear();
+ mPinnedSecureSession.ClearValue();
  }
 
  CHIP_ERROR ListenForSessionEstablishment(Messaging::ExchangeManager * exchangeManager, SessionManager * sessionManager,
@@ -64,6 +75,19 @@ class CASEServer : public SessionEstablishmentDelegate,
  SessionResumptionStorage * mSessionResumptionStorage = nullptr;
  Credentials::CertificateValidityPolicy * mCertificateValidityPolicy = nullptr;
 
+ //
+ // When we're in the process of establishing a session, this is used
+ // to maintain an additional, strong reference to the underlying SecureSession.
+ // This is because the existing reference in PairingSession is a weak one
+ // (i.e a SessionHolder) and can lose its reference if the session is evicted
+ // for any reason.
+ //
+ // This initially points to a session that is not yet active. Upon activation, it
+ // transfers ownership of the session to the SecureSessionManager and this reference
+ // is released before simultaneously acquiring ownership of a new SecureSession.
+ //
+ Optional<SessionHandle> mPinnedSecureSession;
+
  CASESession mPairingSession;
  SessionManager * mSessionManager = nullptr;
 
@@ -72,7 +96,16 @@ class CASEServer : public SessionEstablishmentDelegate,
 
  CHIP_ERROR InitCASEHandshake(Messaging::ExchangeContext * ec);
 
- void Cleanup();
+ /*
+ * This will clean up any state from a previous session establishment
+ * attempt (if any) and setup the machinery to listen for and handle
+ * any session handshakes there-after.
+ *
+ * If a session had previously been established successfully, previouslyEstablishedPeer
+ * should be set to the scoped node-id of the peer associated with that session.
+ *
+ */
+ void PrepareForSessionEstablishment(const ScopedNodeId & previouslyEstablishedPeer = ScopedNodeId());
 };
 
 } // namespace chip

src/protocols/secure_channel/CASESession.cpp

@@ -151,7 +151,7 @@ void CASESession::Clear()
 }
 
 CHIP_ERROR CASESession::Init(SessionManager & sessionManager, Credentials::CertificateValidityPolicy * policy,
- SessionEstablishmentDelegate * delegate)
+ SessionEstablishmentDelegate * delegate, const ScopedNodeId & sessionEvictionHint)
 {
  VerifyOrReturnError(delegate != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
  VerifyOrReturnError(mGroupDataProvider != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
@@ -161,7 +161,7 @@ CHIP_ERROR CASESession::Init(SessionManager & sessionManager, Credentials::Certi
  ReturnErrorOnFailure(mCommissioningHash.Begin());
 
  mDelegate = delegate;
- ReturnErrorOnFailure(AllocateSecureSession(sessionManager));
+ ReturnErrorOnFailure(AllocateSecureSession(sessionManager, sessionEvictionHint));
 
  mValidContext.Reset();
  mValidContext.mRequiredKeyUsages.Set(KeyUsageFlags::kDigitalSignature);
@@ -172,13 +172,14 @@ CHIP_ERROR CASESession::Init(SessionManager & sessionManager, Credentials::Certi
 }
 
 CHIP_ERROR
-CASESession::ListenForSessionEstablishment(SessionManager & sessionManager, FabricTable * fabrics,
- SessionResumptionStorage * sessionResumptionStorage,
- Credentials::CertificateValidityPolicy * policy, SessionEstablishmentDelegate * delegate,
- Optional<ReliableMessageProtocolConfig> mrpConfig)
+CASESession::PrepareForSessionEstablishment(SessionManager & sessionManager, FabricTable * fabrics,
+ SessionResumptionStorage * sessionResumptionStorage,
+ Credentials::CertificateValidityPolicy * policy,
+ SessionEstablishmentDelegate * delegate, ScopedNodeId previouslyEstablishedPeer,
+ Optional<ReliableMessageProtocolConfig> mrpConfig)
 {
  VerifyOrReturnError(fabrics != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
- ReturnErrorOnFailure(Init(sessionManager, policy, delegate));
+ ReturnErrorOnFailure(Init(sessionManager, policy, delegate, previouslyEstablishedPeer));
 
  mRole = CryptoContext::SessionRole::kResponder;
  mFabricsTable = fabrics;
@@ -207,7 +208,7 @@ CHIP_ERROR CASESession::EstablishSession(SessionManager & sessionManager, Fabric
  ReturnErrorCodeIf(fabricIndex == kUndefinedFabricIndex, CHIP_ERROR_INVALID_ARGUMENT);
  VerifyOrReturnError((fabricInfo = fabricTable->FindFabricWithIndex(fabricIndex)) != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
- err = Init(sessionManager, policy, delegate);
+ err = Init(sessionManager, policy, delegate, ScopedNodeId(peerNodeId, fabricIndex));
 
  mRole = CryptoContext::SessionRole::kInitiator;
 

src/protocols/secure_channel/CASESession.h

@@ -75,12 +75,17 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
  * @param fabrics 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
+ * be set to its scoped node-id. Else, this should be initialized to a
+ * default-constructed ScopedNodeId().
+ * @param mrpConfig MRP configuration to encode into Sigma2. If not provided, it won't be encoded.
  *
  * @return CHIP_ERROR The result of initialization
  */
- CHIP_ERROR ListenForSessionEstablishment(
+ CHIP_ERROR PrepareForSessionEstablishment(
  SessionManager & sessionManager, FabricTable * fabrics, SessionResumptionStorage * sessionResumptionStorage,
  Credentials::CertificateValidityPolicy * policy, SessionEstablishmentDelegate * delegate,
+ ScopedNodeId previouslyEstablishedPeer,
  Optional<ReliableMessageProtocolConfig> mrpConfig = Optional<ReliableMessageProtocolConfig>::Missing());
 
  /**
@@ -181,8 +186,17 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
  kFinishedViaResume = 7,
  };
 
+ /*
+ * Initialize the object given a reference to the SessionManager, certificate validity policy and a delegate which will be
+ * notified of any further progress on this session.
+ *
+ * If we're either establishing or finished establishing a session to a peer in either initiator or responder
+ * roles, the node id of that peer should be provided in sessionEvictionHint. Else, it should be initialized
+ * to a default-constructed ScopedNodeId().
+ *
+ */
  CHIP_ERROR Init(SessionManager & sessionManager, Credentials::CertificateValidityPolicy * policy,
- SessionEstablishmentDelegate * delegate);
+ SessionEstablishmentDelegate * delegate, const ScopedNodeId & sessionEvictionHint);
 
  // On success, sets mIpk to the correct value for outgoing Sigma1 based on internal state
  CHIP_ERROR RecoverInitiatorIpk();

src/protocols/secure_channel/PairingSession.cpp

@@ -23,9 +23,9 @@
 
 namespace chip {
 
-CHIP_ERROR PairingSession::AllocateSecureSession(SessionManager & sessionManager)
+CHIP_ERROR PairingSession::AllocateSecureSession(SessionManager & sessionManager, const ScopedNodeId & sessionEvictionHint)
 {
- auto handle = sessionManager.AllocateSession(GetSecureSessionType());
+ auto handle = sessionManager.AllocateSession(GetSecureSessionType(), sessionEvictionHint);
  VerifyOrReturnError(handle.HasValue(), CHIP_ERROR_NO_MEMORY);
  VerifyOrReturnError(mSecureSessionHolder.GrabPairingSession(handle.Value()), CHIP_ERROR_INTERNAL);
  mSessionManager = &sessionManager;

src/protocols/secure_channel/PairingSession.h

@@ -63,11 +63,27 @@ class DLL_EXPORT PairingSession : public SessionDelegate
  return localSessionId;
  }
 
+ /**
+ * Copy the underlying session (if present) into a SessionHandle that a caller can use to
+ * obtain a reference to the session.
+ */
+ Optional<SessionHandle> CopySecureSession()
+ {
+ if (mSecureSessionHolder)
+ {
+ VerifyOrDie(mSecureSessionHolder->GetSessionType() == Transport::Session::SessionType::kSecure);
+ return MakeOptional<SessionHandle>(*mSecureSessionHolder->AsSecureSession());
+ }
+
+ return Optional<SessionHandle>::Missing();
+ }
+
  uint16_t GetPeerSessionId() const
  {
  VerifyOrDie(mPeerSessionId.HasValue());
  return mPeerSessionId.Value();
  }
+
  bool IsValidPeerSessionId() const { return mPeerSessionId.HasValue(); }
 
  /**
@@ -93,10 +109,14 @@ class DLL_EXPORT PairingSession : public SessionDelegate
  * Allocate a secure session object from the passed session manager for the
  * pending session establishment operation.
  *
- * @param sessionManager session manager from which to allocate a secure session object
+ * @param sessionManager Session manager from which to allocate a secure session object
+ * @param sessionEvictionHint If we're either establishing or just finished establishing a session to a peer in either
+ * initiator or responder roles, the node id of that peer should be provided in this argument. Else, it should be initialized to
+ * a default-constructed ScopedNodeId().
+ *
  * @return CHIP_ERROR The outcome of the allocation attempt
  */
- CHIP_ERROR AllocateSecureSession(SessionManager & sessionManager);
+ CHIP_ERROR AllocateSecureSession(SessionManager & sessionManager, const ScopedNodeId & sessionEvictionHint = ScopedNodeId());
 
  CHIP_ERROR ActivateSecureSession(const Transport::PeerAddress & peerAddress);
 

src/protocols/secure_channel/tests/TestCASESession.cpp

@@ -198,13 +198,14 @@ void CASE_SecurePairingWaitTest(nlTestSuite * inSuite, void * inContext)
 
  pairing.SetGroupDataProvider(&gDeviceGroupDataProvider);
  NL_TEST_ASSERT(inSuite,
- pairing.ListenForSessionEstablishment(sessionManager, nullptr, nullptr, nullptr, nullptr) ==
+ pairing.PrepareForSessionEstablishment(sessionManager, nullptr, nullptr, nullptr, nullptr, ScopedNodeId()) ==
  CHIP_ERROR_INVALID_ARGUMENT);
  NL_TEST_ASSERT(inSuite,
- pairing.ListenForSessionEstablishment(sessionManager, nullptr, nullptr, nullptr, &delegate) ==
+ pairing.PrepareForSessionEstablishment(sessionManager, nullptr, nullptr, nullptr, &delegate, ScopedNodeId()) ==
  CHIP_ERROR_INVALID_ARGUMENT);
  NL_TEST_ASSERT(inSuite,
- pairing.ListenForSessionEstablishment(sessionManager, &fabrics, nullptr, nullptr, &delegate) == CHIP_NO_ERROR);
+ pairing.PrepareForSessionEstablishment(sessionManager, &fabrics, nullptr, nullptr, &delegate, ScopedNodeId()) ==
+ CHIP_NO_ERROR);
 }
 
 void CASE_SecurePairingStartTest(nlTestSuite * inSuite, void * inContext)
@@ -283,9 +284,9 @@ void CASE_SecurePairingHandshakeTestCommon(nlTestSuite * inSuite, void * inConte
 
  pairingAccessory.SetGroupDataProvider(&gDeviceGroupDataProvider);
  NL_TEST_ASSERT(inSuite,
- pairingAccessory.ListenForSessionEstablishment(sessionManager, &gDeviceFabrics, nullptr, nullptr,
- &delegateAccessory,
- MakeOptional(verySleepyAccessoryRmpConfig)) == CHIP_NO_ERROR);
+ pairingAccessory.PrepareForSessionEstablishment(sessionManager, &gDeviceFabrics, nullptr, nullptr,
+  &delegateAccessory, ScopedNodeId(),
+  MakeOptional(verySleepyAccessoryRmpConfig)) == CHIP_NO_ERROR);
  NL_TEST_ASSERT(inSuite,
  pairingCommissioner.EstablishSession(sessionManager, &gCommissionerFabrics, gCommissionerFabricIndex, Node01_01,
  contextCommissioner, nullptr, nullptr, &delegateCommissioner,
@@ -847,6 +848,7 @@ int CASE_TestSecurePairing_Setup(void * inContext)
  */
 int CASE_TestSecurePairing_Teardown(void * inContext)
 {
+ gPairingServer.Shutdown();
  gCommissionerStorageDelegate.ClearStorage();
  gDeviceStorageDelegate.ClearStorage();
  gCommissionerFabrics.DeleteAllFabrics();

src/transport/SecureSessionTable.h

@@ -214,7 +214,7 @@ class SecureSessionTable
  return NullOptional;
  }
 
- BitMapObjectPool<SecureSession, CHIP_CONFIG_PEER_CONNECTION_POOL_SIZE> mEntries;
+ BitMapObjectPool<SecureSession, CHIP_CONFIG_SECURE_SESSION_POOL_SIZE> mEntries;
  uint16_t mNextSessionId = 0;
 };