From b949ac5c92f4804b6c05b4336aa7a878dd435ed7 Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Mon, 8 Feb 2021 12:29:48 -0800 Subject: [PATCH] Address review comments. --- src/messaging/ExchangeContext.h | 6 +- src/messaging/ExchangeMgr.h | 4 +- src/messaging/SecureChannelMgr.cpp | 64 ++++++++++---------- src/messaging/SecureChannelMgr.h | 18 +++--- src/messaging/tests/TestSecureChannelMgr.cpp | 4 +- 5 files changed, 48 insertions(+), 48 deletions(-) diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index f6fb57e243af48..143db841b038ac 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -152,9 +152,9 @@ class DLL_EXPORT ExchangeContext : public ReferenceCounted mFlags; // Internal state flags diff --git a/src/messaging/ExchangeMgr.h b/src/messaging/ExchangeMgr.h index e375a8a34532ad..b62427e2f4537c 100644 --- a/src/messaging/ExchangeMgr.h +++ b/src/messaging/ExchangeMgr.h @@ -178,7 +178,7 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate ReliableMessageMgr * GetReliableMessageMgr() { return &mReliableMessageMgr; }; - SecureChannel::SecureChannelMgr * GetSecureChannelMgr() { return &mSecureChannelMgr; }; + Protocols::SecureChannel::SecureChannelMgr * GetSecureChannelMgr() { return &mSecureChannelMgr; }; size_t GetContextsInUse() const { return mContextsInUse; } @@ -200,7 +200,7 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate State mState; SecureSessionMgr * mSessionMgr; ReliableMessageMgr mReliableMessageMgr; - SecureChannel::SecureChannelMgr mSecureChannelMgr; + Protocols::SecureChannel::SecureChannelMgr mSecureChannelMgr; std::array mContextPool; size_t mContextsInUse; diff --git a/src/messaging/SecureChannelMgr.cpp b/src/messaging/SecureChannelMgr.cpp index 28a3cf0452eb94..e2be22ae970657 100644 --- a/src/messaging/SecureChannelMgr.cpp +++ b/src/messaging/SecureChannelMgr.cpp @@ -34,13 +34,9 @@ #include namespace chip { +namespace Protocols { namespace SecureChannel { -SecureChannelMgr::SecureChannelMgr() -{ - mExchangeMgr = nullptr; -} - CHIP_ERROR SecureChannelMgr::Init(Messaging::ExchangeManager * exchangeMgr) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -66,28 +62,28 @@ void SecureChannelMgr::Shutdown() } } -void SecureChannelMgr::OnMessageReceived(Messaging::ExchangeContext * ec, const PacketHeader & packetHeader, +void SecureChannelMgr::OnMessageReceived(Messaging::ExchangeContext * exchangeContext, const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, System::PacketBufferHandle msgBuf) { if (payloadHeader.HasMessageType(Protocols::SecureChannel::MsgType::MsgCounterSyncReq)) { - HandleMsgCounterSyncReq(ec, packetHeader, std::move(msgBuf)); + HandleMsgCounterSyncReq(exchangeContext, packetHeader, std::move(msgBuf)); } else if (payloadHeader.HasMessageType(Protocols::SecureChannel::MsgType::MsgCounterSyncRsp)) { - HandleMsgCounterSyncResp(ec, packetHeader, std::move(msgBuf)); + HandleMsgCounterSyncResp(exchangeContext, packetHeader, std::move(msgBuf)); } } -void SecureChannelMgr::OnResponseTimeout(Messaging::ExchangeContext * ec) +void SecureChannelMgr::OnResponseTimeout(Messaging::ExchangeContext * exchangeContext) { // Close the exchange if MsgCounterSyncRsp is not received before kMsgCounterSyncTimeout. - if (ec != nullptr) - ec->Close(); + if (exchangeContext != nullptr) + exchangeContext->Close(); } // Create and initialize new exchange for the message counter synchronization request/response messages. -CHIP_ERROR SecureChannelMgr::NewMsgCounterSyncExchange(SecureSessionHandle session, Messaging::ExchangeContext *& ec) +CHIP_ERROR SecureChannelMgr::NewMsgCounterSyncExchange(SecureSessionHandle session, Messaging::ExchangeContext *& exchangeContext) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -95,8 +91,8 @@ CHIP_ERROR SecureChannelMgr::NewMsgCounterSyncExchange(SecureSessionHandle sessi VerifyOrExit(ChipKeyId::IsAppGroupKey(session.GetPeerKeyId()), err = CHIP_ERROR_INVALID_ARGUMENT); // Create new exchange context. - ec = mExchangeMgr->NewContext(session, this); - VerifyOrExit(ec != nullptr, err = CHIP_ERROR_NO_MEMORY); + exchangeContext = mExchangeMgr->NewContext(session, this); + VerifyOrExit(exchangeContext != nullptr, err = CHIP_ERROR_NO_MEMORY); exit: return err; @@ -104,14 +100,14 @@ CHIP_ERROR SecureChannelMgr::NewMsgCounterSyncExchange(SecureSessionHandle sessi CHIP_ERROR SecureChannelMgr::SendMsgCounterSyncReq(SecureSessionHandle session) { - CHIP_ERROR err = CHIP_NO_ERROR; - Messaging::ExchangeContext * ec = nullptr; + CHIP_ERROR err = CHIP_NO_ERROR; + Messaging::ExchangeContext * exchangeContext = nullptr; System::PacketBufferHandle msgBuf; Messaging::SendFlags sendFlags; uint8_t challenge[kMsgCounterChallengeSize]; // Create and initialize new exchange. - err = NewMsgCounterSyncExchange(session, ec); + err = NewMsgCounterSyncExchange(session, exchangeContext); SuccessOrExit(err); // Allocate a buffer for the null message. @@ -123,7 +119,7 @@ CHIP_ERROR SecureChannelMgr::SendMsgCounterSyncReq(SecureSessionHandle session) SuccessOrExit(err); // Store generated Challenge value to ExchangeContext to resolve synchronization response. - ec->SetChallenge(challenge); + exchangeContext->SetChallenge(challenge); memcpy(msgBuf->Start(), challenge, kMsgCounterChallengeSize); msgBuf->SetDataLength(kMsgCounterChallengeSize); @@ -132,10 +128,10 @@ CHIP_ERROR SecureChannelMgr::SendMsgCounterSyncReq(SecureSessionHandle session) sendFlags.Set(Messaging::SendMessageFlags::kNoAutoRequestAck, true).Set(Messaging::SendMessageFlags::kExpectResponse, true); // Arm a timer to enforce that a MsgCounterSyncRsp is received before kMsgCounterSyncTimeout. - ec->SetResponseTimeout(kMsgCounterSyncTimeout); + exchangeContext->SetResponseTimeout(kMsgCounterSyncTimeout); // Send the message counter synchronization request in a Secure Channel Protocol::MsgCounterSyncReq message. - err = ec->SendMessage(Protocols::SecureChannel::MsgType::MsgCounterSyncReq, std::move(msgBuf), sendFlags); + err = exchangeContext->SendMessage(Protocols::SecureChannel::MsgType::MsgCounterSyncReq, std::move(msgBuf), sendFlags); SuccessOrExit(err); exit: @@ -147,7 +143,7 @@ CHIP_ERROR SecureChannelMgr::SendMsgCounterSyncReq(SecureSessionHandle session) return err; } -CHIP_ERROR SecureChannelMgr::SendMsgCounterSyncResp(Messaging::ExchangeContext * ec, SecureSessionHandle session) +CHIP_ERROR SecureChannelMgr::SendMsgCounterSyncResp(Messaging::ExchangeContext * exchangeContext, SecureSessionHandle session) { CHIP_ERROR err = CHIP_NO_ERROR; Transport::PeerConnectionState * state = nullptr; @@ -171,7 +167,7 @@ CHIP_ERROR SecureChannelMgr::SendMsgCounterSyncResp(Messaging::ExchangeContext * bbuf.Put32(state->GetSendMessageIndex()); // Fill in the random value - bbuf.Put(ec->GetChallenge(), kMsgCounterChallengeSize); + bbuf.Put(exchangeContext->GetChallenge(), kMsgCounterChallengeSize); VerifyOrExit(bbuf.Fit(), err = CHIP_ERROR_NO_MEMORY); } @@ -180,8 +176,8 @@ CHIP_ERROR SecureChannelMgr::SendMsgCounterSyncResp(Messaging::ExchangeContext * msgBuf->SetDataLength(kMsgCounterSyncRespMsgSize); // Send message counter synchronization response message. - err = ec->SendMessage(Protocols::SecureChannel::MsgType::MsgCounterSyncRsp, std::move(msgBuf), - Messaging::SendFlags(Messaging::SendMessageFlags::kNoAutoRequestAck)); + err = exchangeContext->SendMessage(Protocols::SecureChannel::MsgType::MsgCounterSyncRsp, std::move(msgBuf), + Messaging::SendFlags(Messaging::SendMessageFlags::kNoAutoRequestAck)); exit: if (err != CHIP_NO_ERROR) @@ -192,7 +188,7 @@ CHIP_ERROR SecureChannelMgr::SendMsgCounterSyncResp(Messaging::ExchangeContext * return err; } -void SecureChannelMgr::HandleMsgCounterSyncReq(Messaging::ExchangeContext * ec, const PacketHeader & packetHeader, +void SecureChannelMgr::HandleMsgCounterSyncReq(Messaging::ExchangeContext * exchangeContext, const PacketHeader & packetHeader, System::PacketBufferHandle msgBuf) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -208,10 +204,10 @@ void SecureChannelMgr::HandleMsgCounterSyncReq(Messaging::ExchangeContext * ec, VerifyOrExit(reqlen == kMsgCounterChallengeSize, err = CHIP_ERROR_INVALID_MESSAGE_LENGTH); // Store the 64-bit value sent in the Challenge filed of the MsgCounterSyncReq. - ec->SetChallenge(req); + exchangeContext->SetChallenge(req); // Respond with MsgCounterSyncResp - err = SendMsgCounterSyncResp(ec, { packetHeader.GetSourceNodeId().Value(), packetHeader.GetEncryptionKeyID() }); + err = SendMsgCounterSyncResp(exchangeContext, { packetHeader.GetSourceNodeId().Value(), packetHeader.GetEncryptionKeyID() }); exit: if (err != CHIP_NO_ERROR) @@ -219,13 +215,13 @@ void SecureChannelMgr::HandleMsgCounterSyncReq(Messaging::ExchangeContext * ec, ChipLogError(ExchangeManager, "Failed to handle MsgCounterSyncReq message with error:%s", ErrorStr(err)); } - if (ec != nullptr) - ec->Close(); + if (exchangeContext != nullptr) + exchangeContext->Close(); return; } -void SecureChannelMgr::HandleMsgCounterSyncResp(Messaging::ExchangeContext * ec, const PacketHeader & packetHeader, +void SecureChannelMgr::HandleMsgCounterSyncResp(Messaging::ExchangeContext * exchangeContext, const PacketHeader & packetHeader, System::PacketBufferHandle msgBuf) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -249,7 +245,8 @@ void SecureChannelMgr::HandleMsgCounterSyncResp(Messaging::ExchangeContext * ec, memcpy(challenge, resp, kMsgCounterChallengeSize); // Verify that the response field matches the expected Challenge field for the exchange. - VerifyOrExit(memcmp(ec->GetChallenge(), challenge, kMsgCounterChallengeSize) == 0, err = CHIP_ERROR_INVALID_SIGNATURE); + VerifyOrExit(memcmp(exchangeContext->GetChallenge(), challenge, kMsgCounterChallengeSize) == 0, + err = CHIP_ERROR_INVALID_SIGNATURE); // ToDo:(#4628)Initialize/synchronize peer's message counter to FabricState. VerifyOrExit(syncCounter != 0, err = CHIP_ERROR_READ_FAILED); @@ -260,11 +257,12 @@ void SecureChannelMgr::HandleMsgCounterSyncResp(Messaging::ExchangeContext * ec, ChipLogError(ExchangeManager, "Failed to handle MsgCounterSyncResp message with error:%s", ErrorStr(err)); } - if (ec != nullptr) - ec->Close(); + if (exchangeContext != nullptr) + exchangeContext->Close(); return; } } // namespace SecureChannel +} // namespace Protocols } // namespace chip diff --git a/src/messaging/SecureChannelMgr.h b/src/messaging/SecureChannelMgr.h index 7d7b6640c2724e..194c332ae7b7af 100644 --- a/src/messaging/SecureChannelMgr.h +++ b/src/messaging/SecureChannelMgr.h @@ -25,6 +25,7 @@ #include namespace chip { +namespace Protocols { namespace SecureChannel { constexpr uint16_t kMsgCounterSyncRespMsgSize = 12; // The size of the message counter synchronization response message. @@ -36,7 +37,7 @@ class ExchangeManager; class SecureChannelMgr : public Messaging::ExchangeDelegate { public: - SecureChannelMgr(); + SecureChannelMgr() : mExchangeMgr(nullptr) {} CHIP_ERROR Init(Messaging::ExchangeManager * exchangeMgr); void Shutdown(); @@ -58,21 +59,22 @@ class SecureChannelMgr : public Messaging::ExchangeDelegate private: Messaging::ExchangeManager * mExchangeMgr; // [READ ONLY] Associated Exchange Manager object. - CHIP_ERROR NewMsgCounterSyncExchange(SecureSessionHandle session, Messaging::ExchangeContext *& ec); + CHIP_ERROR NewMsgCounterSyncExchange(SecureSessionHandle session, Messaging::ExchangeContext *& exchangeContext); - CHIP_ERROR SendMsgCounterSyncResp(Messaging::ExchangeContext * ec, SecureSessionHandle session); + CHIP_ERROR SendMsgCounterSyncResp(Messaging::ExchangeContext * exchangeContext, SecureSessionHandle session); - void HandleMsgCounterSyncReq(Messaging::ExchangeContext * ec, const PacketHeader & packetHeader, + void HandleMsgCounterSyncReq(Messaging::ExchangeContext * exchangeContext, const PacketHeader & packetHeader, System::PacketBufferHandle msgBuf); - void HandleMsgCounterSyncResp(Messaging::ExchangeContext * ec, const PacketHeader & packetHeader, + void HandleMsgCounterSyncResp(Messaging::ExchangeContext * exchangeContext, const PacketHeader & packetHeader, System::PacketBufferHandle msgBuf); - void OnMessageReceived(Messaging::ExchangeContext * ec, const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, - System::PacketBufferHandle payload) override; + void OnMessageReceived(Messaging::ExchangeContext * exchangeContext, const PacketHeader & packetHeader, + const PayloadHeader & payloadHeader, System::PacketBufferHandle payload) override; - void OnResponseTimeout(Messaging::ExchangeContext * ec) override; + void OnResponseTimeout(Messaging::ExchangeContext * exchangeContext) override; }; } // namespace SecureChannel +} // namespace Protocols } // namespace chip diff --git a/src/messaging/tests/TestSecureChannelMgr.cpp b/src/messaging/tests/TestSecureChannelMgr.cpp index 641cbc491853dc..ce9f237e98bff7 100644 --- a/src/messaging/tests/TestSecureChannelMgr.cpp +++ b/src/messaging/tests/TestSecureChannelMgr.cpp @@ -139,7 +139,7 @@ void CheckSendMsgCounterSyncReq(nlTestSuite * inSuite, void * inContext) testExchangeMgr.mSuite = inSuite; ctx.GetSecureSessionManager().SetDelegate(&testExchangeMgr); - chip::SecureChannel::SecureChannelMgr * sm = ctx.GetExchangeManager().GetSecureChannelMgr(); + chip::Protocols::SecureChannel::SecureChannelMgr * sm = ctx.GetExchangeManager().GetSecureChannelMgr(); NL_TEST_ASSERT(inSuite, sm != nullptr); Optional peer(Transport::PeerAddress::UDP(addr, CHIP_PORT)); @@ -180,7 +180,7 @@ void CheckReceiveMsgCounterSyncReq(nlTestSuite * inSuite, void * inContext) mockAppDelegate.mSuite = inSuite; - chip::SecureChannel::SecureChannelMgr * sm = ctx.GetExchangeManager().GetSecureChannelMgr(); + chip::Protocols::SecureChannel::SecureChannelMgr * sm = ctx.GetExchangeManager().GetSecureChannelMgr(); NL_TEST_ASSERT(inSuite, sm != nullptr); // Register to receive unsolicited Secure Channel Request messages from the exchange manager.