From 2bd85790014b40712137ca4666d7894cb332b2b5 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Wed, 16 Jun 2021 13:45:00 -0700 Subject: [PATCH] address review comments --- .../commands/clusters/ModelCommand.cpp | 18 +++++++++--------- src/app/server/RendezvousServer.cpp | 9 ++++++--- src/app/server/RendezvousServer.h | 9 +++++++-- src/app/server/Server.cpp | 18 +++++++++++------- src/controller/CHIPDevice.cpp | 9 +++++++++ src/controller/CHIPDeviceController.cpp | 12 +++++++----- .../secure_channel/SessionIDAllocator.cpp | 10 +++------- .../secure_channel/SessionIDAllocator.h | 4 ++-- 8 files changed, 54 insertions(+), 35 deletions(-) diff --git a/examples/chip-tool/commands/clusters/ModelCommand.cpp b/examples/chip-tool/commands/clusters/ModelCommand.cpp index 0655b79192ef51..f04b7e275c9268 100644 --- a/examples/chip-tool/commands/clusters/ModelCommand.cpp +++ b/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))); diff --git a/src/app/server/RendezvousServer.cpp b/src/app/server/RendezvousServer.cpp index 344075ca039ec7..f595c10ab57a54 100644 --- a/src/app/server/RendezvousServer.cpp +++ b/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 diff --git a/src/app/server/RendezvousServer.h b/src/app/server/RendezvousServer.h index c9920976157e9e..e470534603b1a2 100644 --- a/src/app/server/RendezvousServer.h +++ b/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; } diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 2eddf3c633dcbe..335ce725db5891 100644 --- a/src/app/server/Server.cpp +++ b/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::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::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. diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index 5c21cf2df8b13c..6b2fd9dc272bc6 100644 --- a/src/controller/CHIPDevice.cpp +++ b/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); diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 4b6beccb3fb67d..651a41af7bdcd1 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/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; diff --git a/src/protocols/secure_channel/SessionIDAllocator.cpp b/src/protocols/secure_channel/SessionIDAllocator.cpp index d3d5a10969baa4..f2a81c5d6c3cfc 100644 --- a/src/protocols/secure_channel/SessionIDAllocator.cpp +++ b/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 diff --git a/src/protocols/secure_channel/SessionIDAllocator.h b/src/protocols/secure_channel/SessionIDAllocator.h index 9515282362920a..88c705c7af4a4b 100644 --- a/src/protocols/secure_channel/SessionIDAllocator.h +++ b/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)