Move NewPairing from SessionEstablishDelegate to CASE/PASE session (#…

…17422)

* Move NewPairing from establish delegate to CASE/PASE session

* Remove role argument from DeriveSecureSession

examples/chip-tool/commands/tests/TestCommand.cpp

@@ -41,7 +41,7 @@ CHIP_ERROR TestCommand::WaitForCommissionee(chip::NodeId nodeId)
     // or is just starting out fresh outright. Let's make sure we're not re-using any cached CASE sessions
     // that will now be stale and mismatched with the peer, causing subsequent interactions to fail.
     //
-    CurrentCommissioner().SessionMgr()->ExpireAllPairings(nodeId, fabricIndex);
+    CurrentCommissioner().SessionMgr()->ExpireAllPairings(chip::ScopedNodeId(nodeId, fabricIndex));
 
     return CurrentCommissioner().GetConnectedDevice(nodeId, &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback);
 }

src/app/CASEClient.cpp

@@ -23,12 +23,11 @@ CASEClient::CASEClient(const CASEClientInitParams & params) : mInitParams(params
 
 void CASEClient::SetMRPIntervals(const ReliableMessageProtocolConfig & mrpConfig)
 {
-    mCASESession.SetMRPConfig(mrpConfig);
+    mCASESession.SetRemoteMRPConfig(mrpConfig);
 }
 
 CHIP_ERROR CASEClient::EstablishSession(PeerId peer, const Transport::PeerAddress & peerAddress,
-                                        const ReliableMessageProtocolConfig & mrpConfig, OnCASEConnected onConnection,
-                                        OnCASEConnectionFailure onFailure, void * context)
+                                        const ReliableMessageProtocolConfig & mrpConfig, SessionEstablishmentDelegate * delegate)
 {
     // Create a UnauthenticatedSession for CASE pairing.
     // Don't use mSecureSession here, because mSecureSession is for encrypted communication.
@@ -45,45 +44,9 @@ CHIP_ERROR CASEClient::EstablishSession(PeerId peer, const Transport::PeerAddres
     VerifyOrReturnError(exchange != nullptr, CHIP_ERROR_INTERNAL);
 
     mCASESession.SetGroupDataProvider(mInitParams.groupDataProvider);
-    ReturnErrorOnFailure(mCASESession.EstablishSession(*mInitParams.sessionManager, peerAddress, mInitParams.fabricInfo,
-                                                       peer.GetNodeId(), exchange, mInitParams.sessionResumptionStorage, this,
+    ReturnErrorOnFailure(mCASESession.EstablishSession(*mInitParams.sessionManager, mInitParams.fabricInfo, peer.GetNodeId(),
+                                                       exchange, mInitParams.sessionResumptionStorage, delegate,
                                                        mInitParams.mrpLocalConfig));
-    mConnectionSuccessCallback = onConnection;
-    mConnectionFailureCallback = onFailure;
-    mConectionContext          = context;
-    mPeerId                    = peer;
-    mPeerAddress               = peerAddress;
-
-    return CHIP_NO_ERROR;
-}
-
-void CASEClient::OnSessionEstablishmentError(CHIP_ERROR error)
-{
-    if (mConnectionFailureCallback)
-    {
-        mConnectionFailureCallback(mConectionContext, this, error);
-    }
-}
-
-void CASEClient::OnSessionEstablished()
-{
-    // On successful CASE connection, the local session ID will be used for the derived secure session.
-    if (mConnectionSuccessCallback)
-    {
-        mConnectionSuccessCallback(mConectionContext, this);
-    }
-}
-
-CHIP_ERROR CASEClient::DeriveSecureSessionHandle(SessionHolder & handle)
-{
-    CHIP_ERROR err = mInitParams.sessionManager->NewPairing(
-        handle, Optional<Transport::PeerAddress>::Value(mPeerAddress), mPeerId.GetNodeId(), &mCASESession,
-        CryptoContext::SessionRole::kInitiator, mInitParams.fabricInfo->GetFabricIndex());
-    if (err != CHIP_NO_ERROR)
-    {
-        ChipLogError(Controller, "Failed in setting up CASE secure channel: err %s", ErrorStr(err));
-        return err;
-    }
 
     return CHIP_NO_ERROR;
 }

src/app/CASEClient.h

@@ -40,34 +40,20 @@ struct CASEClientInitParams
     Optional<ReliableMessageProtocolConfig> mrpLocalConfig = Optional<ReliableMessageProtocolConfig>::Missing();
 };
 
-class DLL_EXPORT CASEClient : public SessionEstablishmentDelegate
+class DLL_EXPORT CASEClient
 {
 public:
     CASEClient(const CASEClientInitParams & params);
 
     void SetMRPIntervals(const ReliableMessageProtocolConfig & mrpConfig);
 
     CHIP_ERROR EstablishSession(PeerId peer, const Transport::PeerAddress & peerAddress,
-                                const ReliableMessageProtocolConfig & mrpConfig, OnCASEConnected onConnection,
-                                OnCASEConnectionFailure onFailure, void * context);
-
-    // Implementation of SessionEstablishmentDelegate
-    void OnSessionEstablishmentError(CHIP_ERROR error) override;
-
-    void OnSessionEstablished() override;
-
-    CHIP_ERROR DeriveSecureSessionHandle(SessionHolder & handle);
+                                const ReliableMessageProtocolConfig & mrpConfig, SessionEstablishmentDelegate * delegate);
 
 private:
     CASEClientInitParams mInitParams;
 
     CASESession mCASESession;
-    PeerId mPeerId;
-    Transport::PeerAddress mPeerAddress;
-
-    OnCASEConnected mConnectionSuccessCallback         = nullptr;
-    OnCASEConnectionFailure mConnectionFailureCallback = nullptr;
-    void * mConectionContext                           = nullptr;
 };
 
 } // namespace chip

src/app/OperationalDeviceProxy.cpp

@@ -214,8 +214,8 @@ CHIP_ERROR OperationalDeviceProxy::EstablishConnection()
         CASEClientInitParams{ mInitParams.sessionManager, mInitParams.sessionResumptionStorage, mInitParams.exchangeMgr,
                               mFabricInfo, mInitParams.groupDataProvider, mInitParams.mrpLocalConfig });
     ReturnErrorCodeIf(mCASEClient == nullptr, CHIP_ERROR_NO_MEMORY);
-    CHIP_ERROR err =
-        mCASEClient->EstablishSession(mPeerId, mDeviceAddress, mMRPConfig, HandleCASEConnected, HandleCASEConnectionFailure, this);
+
+    CHIP_ERROR err = mCASEClient->EstablishSession(mPeerId, mDeviceAddress, mMRPConfig, this);
     if (err != CHIP_NO_ERROR)
     {
         CleanupCASEClient();
@@ -282,50 +282,33 @@ void OperationalDeviceProxy::DequeueConnectionCallbacks(CHIP_ERROR error)
     }
 }
 
-void OperationalDeviceProxy::HandleCASEConnectionFailure(void * context, CASEClient * client, CHIP_ERROR error)
+void OperationalDeviceProxy::OnSessionEstablishmentError(CHIP_ERROR error)
 {
-    OperationalDeviceProxy * device = static_cast<OperationalDeviceProxy *>(context);
-    VerifyOrReturn(device->mState != State::Uninitialized && device->mState != State::NeedsAddress,
+    VerifyOrReturn(mState != State::Uninitialized && mState != State::NeedsAddress,
                    ChipLogError(Controller, "HandleCASEConnectionFailure was called while the device was not initialized"));
-    VerifyOrReturn(client == device->mCASEClient, ChipLogError(Controller, "HandleCASEConnectionFailure for unknown CASEClient"));
 
     //
     // We don't need to reset the state all the way back to NeedsAddress since all that transpired
     // was just CASE connection failure. So let's re-use the cached address to re-do CASE again
     // if need-be.
     //
-    device->MoveToState(State::Initialized);
+    MoveToState(State::Initialized);
 
-    device->DequeueConnectionCallbacks(error);
+    DequeueConnectionCallbacks(error);
 
-    //
-    // Do not touch device instance anymore; it might have been destroyed by a failure
-    // callback.
-    //
+    // Do not touch device instance anymore; it might have been destroyed by a failure callback.
 }
 
-void OperationalDeviceProxy::HandleCASEConnected(void * context, CASEClient * client)
+void OperationalDeviceProxy::OnSessionEstablished(const SessionHandle & session)
 {
-    OperationalDeviceProxy * device = static_cast<OperationalDeviceProxy *>(context);
-    VerifyOrReturn(device->mState != State::Uninitialized,
+    VerifyOrReturn(mState != State::Uninitialized,
                    ChipLogError(Controller, "HandleCASEConnected was called while the device was not initialized"));
-    VerifyOrReturn(client == device->mCASEClient, ChipLogError(Controller, "HandleCASEConnected for unknown CASEClient"));
 
-    CHIP_ERROR err = client->DeriveSecureSessionHandle(device->mSecureSession);
-    if (err != CHIP_NO_ERROR)
-    {
-        device->HandleCASEConnectionFailure(context, client, err);
-    }
-    else
-    {
-        device->MoveToState(State::SecureConnected);
-        device->DequeueConnectionCallbacks(CHIP_NO_ERROR);
-    }
+    mSecureSession.Grab(session);
+    MoveToState(State::SecureConnected);
+    DequeueConnectionCallbacks(CHIP_NO_ERROR);
 
-    //
-    // Do not touch this instance anymore; it might have been destroyed by a
-    // callback.
-    //
+    // Do not touch this instance anymore; it might have been destroyed by a callback.
 }
 
 CHIP_ERROR OperationalDeviceProxy::Disconnect()

src/app/OperationalDeviceProxy.h

@@ -85,7 +85,7 @@ typedef void (*OnDeviceConnectionFailure)(void * context, PeerId peerId, CHIP_ER
  *    - Expose to consumers the secure session for talking to the device.
  */
 class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
-                                          SessionReleaseDelegate,
+                                          public SessionReleaseDelegate,
                                           public SessionEstablishmentDelegate,
                                           public AddressResolve::NodeListener
 {
@@ -140,6 +140,10 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
 
     bool IsConnecting() const { return mState == State::Connecting; }
 
+    //////////// SessionEstablishmentDelegate Implementation ///////////////
+    void OnSessionEstablished(const SessionHandle & session) override;
+    void OnSessionEstablishmentError(CHIP_ERROR error) override;
+
     /**
      *   Called when a connection is closing.
      *   The object releases all resources associated with the connection.
@@ -276,9 +280,6 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
 
     bool IsSecureConnected() const override { return mState == State::SecureConnected; }
 
-    static void HandleCASEConnected(void * context, CASEClient * client);
-    static void HandleCASEConnectionFailure(void * context, CASEClient * client, CHIP_ERROR error);
-
     void CleanupCASEClient();
 
     void EnqueueConnectionCallbacks(Callback::Callback<OnDeviceConnected> * onConnection,

src/app/server/CommissioningWindowManager.cpp

@@ -144,19 +144,9 @@ void CommissioningWindowManager::OnSessionEstablishmentStarted()
     DeviceLayer::SystemLayer().StartTimer(kPASESessionEstablishmentTimeout, HandleSessionEstablishmentTimeout, this);
 }
 
-void CommissioningWindowManager::OnSessionEstablished()
+void CommissioningWindowManager::OnSessionEstablished(const SessionHandle & session)
 {
     DeviceLayer::SystemLayer().CancelTimer(HandleSessionEstablishmentTimeout, this);
-    SessionHolder sessionHolder;
-    CHIP_ERROR err = mServer->GetSecureSessionManager().NewPairing(
-        sessionHolder, Optional<Transport::PeerAddress>::Value(mPairingSession.GetPeerAddress()), mPairingSession.GetPeerNodeId(),
-        &mPairingSession, CryptoContext::SessionRole::kResponder, 0);
-    if (err != CHIP_NO_ERROR)
-    {
-        ChipLogError(AppServer, "Commissioning failed while setting up secure channel: err %s", ErrorStr(err));
-        OnSessionEstablishmentError(err);
-        return;
-    }
 
     ChipLogProgress(AppServer, "Commissioning completed session establishment step");
     if (mAppDelegate != nullptr)
@@ -177,7 +167,7 @@ void CommissioningWindowManager::OnSessionEstablished()
     }
     else
     {
-        err = failSafeContext.ArmFailSafe(kUndefinedFabricId, System::Clock::Seconds16(60));
+        CHIP_ERROR err = failSafeContext.ArmFailSafe(kUndefinedFabricId, System::Clock::Seconds16(60));
         if (err != CHIP_NO_ERROR)
         {
             ChipLogError(AppServer, "Error arming failsafe on PASE session establishment completion");

src/app/server/CommissioningWindowManager.h

@@ -85,7 +85,7 @@ class CommissioningWindowManager : public SessionEstablishmentDelegate, public a
     //////////// SessionEstablishmentDelegate Implementation ///////////////
     void OnSessionEstablishmentError(CHIP_ERROR error) override;
     void OnSessionEstablishmentStarted() override;
-    void OnSessionEstablished() override;
+    void OnSessionEstablished(const SessionHandle & session) override;
 
     void Shutdown();
     void Cleanup();

src/controller/CHIPDeviceController.cpp

@@ -646,7 +646,7 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
     exchangeCtxt = mSystemState->ExchangeMgr()->NewContext(session.Value(), &device->GetPairing());
     VerifyOrExit(exchangeCtxt != nullptr, err = CHIP_ERROR_INTERNAL);
 
-    err = device->GetPairing().Pair(*mSystemState->SessionMgr(), params.GetPeerAddress(), params.GetSetupPINCode(),
+    err = device->GetPairing().Pair(*mSystemState->SessionMgr(), params.GetSetupPINCode(),
                                     Optional<ReliableMessageProtocolConfig>::Value(GetLocalMRPConfig()), exchangeCtxt, this);
     SuccessOrExit(err);
 
@@ -827,7 +827,7 @@ void DeviceCommissioner::OnSessionEstablishmentError(CHIP_ERROR err)
     RendezvousCleanup(err);
 }
 
-void DeviceCommissioner::OnSessionEstablished()
+void DeviceCommissioner::OnSessionEstablished(const SessionHandle & session)
 {
     // PASE session established.
     CommissioneeDeviceProxy * device = mDeviceInPASEEstablishment;
@@ -837,12 +837,7 @@ void DeviceCommissioner::OnSessionEstablished()
 
     VerifyOrReturn(device != nullptr, OnSessionEstablishmentError(CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR));
 
-    PASESession * pairing = &device->GetPairing();
-
-    // TODO: the session should know which peer we are trying to connect to when started
-    pairing->SetPeerNodeId(device->GetDeviceId());
-
-    CHIP_ERROR err = device->SetConnected();
+    CHIP_ERROR err = device->SetConnected(session);
     if (err != CHIP_NO_ERROR)
     {
         ChipLogError(Controller, "Failed in setting up secure channel: err %s", ErrorStr(err));

src/controller/CHIPDeviceController.h

@@ -459,7 +459,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
 
     //////////// SessionEstablishmentDelegate Implementation ///////////////
     void OnSessionEstablishmentError(CHIP_ERROR error) override;
-    void OnSessionEstablished() override;
+    void OnSessionEstablished(const SessionHandle & session) override;
 
     void RendezvousCleanup(CHIP_ERROR status);
 

src/controller/CommissioneeDeviceProxy.cpp

@@ -76,7 +76,7 @@ CHIP_ERROR CommissioneeDeviceProxy::UpdateDeviceData(const Transport::PeerAddres
 
     // Initialize PASE session state with any MRP parameters that DNS-SD has provided.
     // It can be overridden by PASE session protocol messages that include MRP parameters.
-    mPairing.SetMRPConfig(mMRPConfig);
+    mPairing.SetRemoteMRPConfig(mMRPConfig);
 
     if (!mSecureSession)
     {
@@ -93,26 +93,12 @@ CHIP_ERROR CommissioneeDeviceProxy::UpdateDeviceData(const Transport::PeerAddres
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR CommissioneeDeviceProxy::SetConnected()
+CHIP_ERROR CommissioneeDeviceProxy::SetConnected(const SessionHandle & session)
 {
-    if (mState != ConnectionState::Connecting)
-    {
-        return CHIP_ERROR_INCORRECT_STATE;
-    }
-
-    CHIP_ERROR err = mSessionManager->NewPairing(mSecureSession, Optional<Transport::PeerAddress>::Value(mDeviceAddress),
-                                                 GetDeviceId(), &mPairing, CryptoContext::SessionRole::kInitiator, mFabricIndex);
-
-    if (err == CHIP_NO_ERROR)
-    {
-        mState = ConnectionState::SecureConnected;
-    }
-    else
-    {
-        ChipLogError(Controller, "NewPairing returning error %" CHIP_ERROR_FORMAT, err.Format());
-        mState = ConnectionState::NotConnected;
-    }
-    return err;
+    VerifyOrReturnError(mState == ConnectionState::Connecting, CHIP_ERROR_INCORRECT_STATE);
+    mState = ConnectionState::SecureConnected;
+    mSecureSession.Grab(session);
+    return CHIP_NO_ERROR;
 }
 
 void CommissioneeDeviceProxy::Reset()

src/controller/CommissioneeDeviceProxy.h

@@ -189,7 +189,7 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat
      *
      * This stores the session details in the session manager.
      */
-    CHIP_ERROR SetConnected();
+    CHIP_ERROR SetConnected(const SessionHandle & session);
 
     bool IsSecureConnected() const override { return IsActive() && mState == ConnectionState::SecureConnected; }
 

src/lib/support/logging/CHIPLogging.h

@@ -407,5 +407,9 @@ bool IsCategoryEnabled(uint8_t category);
  */
 #define ChipLogFormatMessageType "0x%x"
 
+/** Logging helpers for scoped node ids, which is a tuple of <NodeId, FabricIndex> */
+#define ChipLogFormatScopedNodeId "<" ChipLogFormatX64 ", %d>"
+#define ChipLogValueScopedNodeId(id) ChipLogValueX64((id).GetNodeId()), (id).GetFabricIndex()
+
 } // namespace Logging
 } // namespace chip

src/messaging/tests/MessagingContext.h

@@ -126,6 +126,9 @@ class MessagingContext : public PlatformMemoryUser
     SessionHandle GetSessionAliceToBob();
     SessionHandle GetSessionBobToFriends();
 
+    const Transport::PeerAddress & GetAliceAddress() { return mAliceAddress; }
+    const Transport::PeerAddress & GetBobAddress() { return mBobAddress; }
+
     Messaging::ExchangeContext * NewUnauthenticatedExchangeToAlice(Messaging::ExchangeDelegate * delegate);
     Messaging::ExchangeContext * NewUnauthenticatedExchangeToBob(Messaging::ExchangeDelegate * delegate);
 

src/protocols/secure_channel/CASEServer.cpp

@@ -127,23 +127,10 @@ void CASEServer::OnSessionEstablishmentError(CHIP_ERROR err)
     Cleanup();
 }
 
-void CASEServer::OnSessionEstablished()
+void CASEServer::OnSessionEstablished(const SessionHandle & session)
 {
-    ChipLogProgress(Inet, "CASE Session established. Setting up the secure channel.");
-    mSessionManager->ExpireAllPairings(GetSession().GetPeerNodeId(), GetSession().GetFabricIndex());
-
-    SessionHolder sessionHolder;
-    CHIP_ERROR err = mSessionManager->NewPairing(
-        sessionHolder, Optional<Transport::PeerAddress>::Value(GetSession().GetPeerAddress()), GetSession().GetPeerNodeId(),
-        &GetSession(), CryptoContext::SessionRole::kResponder, GetSession().GetFabricIndex());
-    if (err != CHIP_NO_ERROR)
-    {
-        ChipLogError(Inet, "Failed in setting up secure channel: err %s", ErrorStr(err));
-        OnSessionEstablishmentError(err);
-        return;
-    }
-
-    ChipLogProgress(Inet, "CASE secure channel is available now.");
+    ChipLogProgress(Inet, "CASE Session established to peer: " ChipLogFormatScopedNodeId,
+                    ChipLogValueScopedNodeId(session->GetPeer()));
     Cleanup();
 }
 } // namespace chip

src/protocols/secure_channel/CASEServer.h

@@ -51,7 +51,7 @@ class CASEServer : public SessionEstablishmentDelegate,
 
     //////////// SessionEstablishmentDelegate Implementation ///////////////
     void OnSessionEstablishmentError(CHIP_ERROR error) override;
-    void OnSessionEstablished() override;
+    void OnSessionEstablished(const SessionHandle & session) override;
 
     //// UnsolicitedMessageHandler Implementation ////
     CHIP_ERROR OnUnsolicitedMessageReceived(const PayloadHeader & payloadHeader, ExchangeDelegate *& newDelegate) override;