address review comments

examples/chip-tool/commands/clusters/ModelCommand.cpp

@@ -39,11 +39,11 @@ void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aC
 
 CHIP_ERROR WaitForSessionSetup(chip::Controller::Device * device)
 {
-    constexpr time_t kWaitPerIteration = 1;
-    constexpr uint16_t kIterationCount = 5;
+    constexpr time_t kWaitPerIterationSec = 1;
+    constexpr uint16_t kIterationCount    = 5;
 
     struct timespec sleep_time;
-    sleep_time.tv_sec  = kWaitPerIteration;
+    sleep_time.tv_sec  = kWaitPerIterationSec;
     sleep_time.tv_nsec = 0;
 
     for (uint32_t i = 0; i < kIterationCount && device->IsSessionSetupInProgress(); i++)
@@ -73,12 +73,12 @@ CHIP_ERROR ModelCommand::Run(NodeId localId, NodeId remoteId)
         err = GetExecContext()->commissioner->GetDevice(remoteId, &mDevice);
         VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Init failure! No pairing for device: %" PRIu64, localId));
 
-        if (mDevice->IsSessionSetupInProgress())
-        {
-            err = WaitForSessionSetup(mDevice);
-            VerifyOrExit(err == CHIP_NO_ERROR,
-                         ChipLogError(chipTool, "Timed out while waiting for session setup for device: %" PRIu64, localId));
-        }
+        // TODO - Implement notification from device object when the secure session is available.
+        // Current code is polling the device object to check for secure session availability. This should
+        // be updated to a notification/callback mechanism.
+        err = WaitForSessionSetup(mDevice);
+        VerifyOrExit(err == CHIP_NO_ERROR,
+                     ChipLogError(chipTool, "Timed out while waiting for session setup for device: %" PRIu64, localId));
 
         err = SendCommand(mDevice, mEndPointId);
         VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Failed to send message: %s", ErrorStr(err)));

src/app/server/RendezvousServer.cpp

@@ -184,9 +184,12 @@ void RendezvousServer::OnSessionEstablished()
     VerifyOrReturn(connection.StoreIntoKVS(*mStorage) == CHIP_NO_ERROR,
                    ChipLogError(AppServer, "Failed to store the connection state"));
 
-    uint16_t keyID = 0;
-    mIDAllocator->Peek(keyID);
-
+    // The Peek() is used to find the highest key ID that's been assigned to any session.
+    // This value is persisted, and on reboot, it is used to revive any previously
+    // active secure sessions.
+    // We support one active PASE session at any time. So the key ID should not be updated
+    // in another thread, while we retrieve it here.
+    uint16_t keyID = mIDAllocator->Peek();
     mStorage->SyncSetKeyValue(kStorablePeerConnectionCountKey, &keyID, sizeof(keyID));
 }
 } // namespace chip

src/app/server/RendezvousServer.h

@@ -35,9 +35,14 @@ class RendezvousServer : public SessionEstablishmentDelegate
     CHIP_ERROR Init(AppDelegate * delegate, PersistentStorageDelegate * storage, SessionIDAllocator * idAllocator)
     {
         VerifyOrReturnError(storage != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
-        mDelegate    = delegate;
-        mStorage     = storage;
+        mStorage = storage;
+
+        VerifyOrReturnError(idAllocator != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
         mIDAllocator = idAllocator;
+
+        // The caller may chose to not provide a delegate object. The RendezvousServer checks for null delegate before calling
+        // it's methods.
+        mDelegate = delegate;
         return CHIP_NO_ERROR;
     }
 

src/app/server/Server.cpp

@@ -171,10 +171,16 @@ static CHIP_ERROR RestoreAllSessionsFromKVS(SecureSessionMgr & sessionMgr)
 
             ChipLogProgress(AppServer, "Fetched the session information: from 0x" ChipLogFormatX64,
                             ChipLogValueX64(session->PeerConnection().GetPeerNodeId()));
-            sessionMgr.NewPairing(Optional<Transport::PeerAddress>::Value(session->PeerConnection().GetPeerAddress()),
-                                  session->PeerConnection().GetPeerNodeId(), session, SecureSession::SessionRole::kResponder,
-                                  connection.GetAdminId(), nullptr);
-            gSessionIDAllocator.Reserve(keyId);
+            if (gSessionIDAllocator.Reserve(keyId) == CHIP_NO_ERROR)
+            {
+                sessionMgr.NewPairing(Optional<Transport::PeerAddress>::Value(session->PeerConnection().GetPeerAddress()),
+                                      session->PeerConnection().GetPeerNodeId(), session, SecureSession::SessionRole::kResponder,
+                                      connection.GetAdminId(), nullptr);
+            }
+            else
+            {
+                ChipLogProgress(AppServer, "Session Key ID %d cannot be used. Skipping over this session", keyId);
+            }
             session->Clear();
         }
     }
@@ -429,10 +435,8 @@ CHIP_ERROR OpenDefaultPairingWindow(ResetAdmins resetAdmins, chip::PairingWindow
 
     if (resetAdmins == ResetAdmins::kYes)
     {
-        uint16_t nextKeyId;
-        ReturnErrorOnFailure(gSessionIDAllocator.Peek(nextKeyId));
         EraseAllAdminPairingsUpTo(gNextAvailableAdminId);
-        EraseAllSessionsUpTo(nextKeyId);
+        EraseAllSessionsUpTo(gSessionIDAllocator.Peek());
         // Only resetting gNextAvailableAdminId at reboot otherwise previously paired device with adminID 0
         // can continue sending messages to accessory as next available admin will also be 0.
         // This logic is not up to spec, will be implemented up to spec once AddOptCert is implemented.

src/controller/CHIPDevice.cpp

@@ -170,11 +170,20 @@ CHIP_ERROR Device::Serialize(SerializedDevice & output)
     serializable.mAdminId    = Encoding::LittleEndian::HostSwap16(mAdminId);
 
     Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession);
+    // The connection state could be null if the device is moving from PASE connection to CASE connection.
+    // The device parameters (e.g. mDeviceOperationalCertProvisioned) are updated during this transition.
+    // The state during this transistion is being persisted so that the next access of the device will
+    // trigger the CASE based secure session.
     if (connectionState != nullptr)
     {
         localMessageCounter = connectionState->GetSessionMessageCounter().GetLocalMessageCounter().Value();
         peerMessageCounter  = connectionState->GetSessionMessageCounter().GetPeerMessageCounter().GetCounter();
     }
+    else
+    {
+        localMessageCounter = 0;
+        peerMessageCounter  = 0;
+    }
 
     serializable.mLocalMessageCounter = Encoding::LittleEndian::HostSwap32(localMessageCounter);
     serializable.mPeerMessageCounter  = Encoding::LittleEndian::HostSwap32(peerMessageCounter);

src/controller/CHIPDeviceController.cpp

@@ -256,11 +256,14 @@ CHIP_ERROR DeviceController::GenerateOperationalCertificates(const ByteSpan & CS
         // This implies that the commissioner application uses root CA to sign the operational
         // certificates, and an intermediate CA is not needed. It's not an error condition, so
         // let's just send operational certificate and root CA certificate to the device.
-        err    = CHIP_NO_ERROR;
         icaLen = 0;
         ChipLogProgress(Controller, "Intermediate CA is not needed");
     }
-    ReturnErrorOnFailure(err);
+    else if (err != CHIP_NO_ERROR)
+    {
+        return err;
+    }
+
     ReturnErrorOnFailure(ConvertX509CertsToChipCertArray(ByteSpan(noc.Get(), nocLen), ByteSpan(ica.Get(), icaLen), certBuf,
                                                          certBufSize, outCertLen));
 
@@ -752,8 +755,7 @@ void DeviceController::PersistNextKeyId()
 {
     if (mStorageDelegate != nullptr && mState == State::Initialized)
     {
-        uint16_t nextKeyID = 0;
-        mIDAllocator.Peek(nextKeyID);
+        uint16_t nextKeyID = mIDAllocator.Peek();
         mStorageDelegate->SyncSetKeyValue(kNextAvailableKeyID, &nextKeyID, sizeof(nextKeyID));
     }
 }
@@ -857,7 +859,7 @@ CHIP_ERROR DeviceCommissioner::Init(NodeId localDeviceId, CommissionerInitParams
     }
     for (uint16_t i = 0; i < nextKeyID; i++)
     {
-        mIDAllocator.Reserve(i);
+        ReturnErrorOnFailure(mIDAllocator.Reserve(i));
     }
     mPairingDelegate = params.pairingDelegate;
 

src/protocols/secure_channel/SessionIDAllocator.cpp

@@ -32,13 +32,12 @@ CHIP_ERROR SessionIDAllocator::Allocate(uint16_t & id)
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR SessionIDAllocator::Free(uint16_t id)
+void SessionIDAllocator::Free(uint16_t id)
 {
     if (mNextAvailable == id && mNextAvailable > 0)
     {
         mNextAvailable--;
     }
-    return CHIP_NO_ERROR;
 }
 
 CHIP_ERROR SessionIDAllocator::Reserve(uint16_t id)
@@ -53,12 +52,9 @@ CHIP_ERROR SessionIDAllocator::Reserve(uint16_t id)
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR SessionIDAllocator::Peek(uint16_t & id)
+uint16_t SessionIDAllocator::Peek()
 {
-    VerifyOrReturnError(mNextAvailable < kMaxSessionID, CHIP_ERROR_NO_MEMORY);
-    id = mNextAvailable;
-
-    return CHIP_NO_ERROR;
+    return mNextAvailable;
 }
 
 } // namespace chip

src/protocols/secure_channel/SessionIDAllocator.h

@@ -28,9 +28,9 @@ class SessionIDAllocator
     ~SessionIDAllocator() {}
 
     CHIP_ERROR Allocate(uint16_t & id);
-    CHIP_ERROR Free(uint16_t id);
+    void Free(uint16_t id);
     CHIP_ERROR Reserve(uint16_t id);
-    CHIP_ERROR Peek(uint16_t & id);
+    uint16_t Peek();
 
 private:
     // Session ID is a 15 bit value (16th bit indicates unicast/group key)