From 5e971c1138214a000517e9932f369034899fa0ec Mon Sep 17 00:00:00 2001 From: Zang MingJie Date: Thu, 5 May 2022 22:02:55 +0800 Subject: [PATCH] Move PairingSession from transport/ to protocols/secure_channel/ (#17889) * Move PairingSession from transport/ to protocols/secure_channel/ * Resolve comments --- src/protocols/secure_channel/BUILD.gn | 2 + src/protocols/secure_channel/CASESession.cpp | 48 +------------------ src/protocols/secure_channel/CASESession.h | 17 +------ src/protocols/secure_channel/PASESession.cpp | 39 +-------------- src/protocols/secure_channel/PASESession.h | 16 +------ .../secure_channel}/PairingSession.cpp | 46 +++++++++++++++++- .../secure_channel}/PairingSession.h | 9 +++- src/protocols/secure_channel/tests/BUILD.gn | 1 + .../tests/TestPairingSession.cpp | 6 +-- src/transport/BUILD.gn | 1 - src/transport/SessionManager.cpp | 1 - src/transport/SessionManager.h | 2 - src/transport/tests/BUILD.gn | 1 - 13 files changed, 62 insertions(+), 127 deletions(-) rename src/{transport => protocols/secure_channel}/PairingSession.cpp (77%) rename src/{transport => protocols/secure_channel}/PairingSession.h (94%) rename src/{transport => protocols/secure_channel}/tests/TestPairingSession.cpp (99%) diff --git a/src/protocols/secure_channel/BUILD.gn b/src/protocols/secure_channel/BUILD.gn index 25c6bd98011568..24ca97fd5bc964 100644 --- a/src/protocols/secure_channel/BUILD.gn +++ b/src/protocols/secure_channel/BUILD.gn @@ -14,6 +14,8 @@ static_library("secure_channel") { "DefaultSessionResumptionStorage.h", "PASESession.cpp", "PASESession.h", + "PairingSession.cpp", + "PairingSession.h", "RendezvousParameters.h", "SessionEstablishmentDelegate.h", "SessionEstablishmentExchangeDispatch.cpp", diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 77653953a3c947..50afc4383ff9c1 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -37,11 +37,11 @@ #include #include #include +#include #include #include #include #include -#include #include #if CHIP_CRYPTO_HSM #include @@ -127,24 +127,6 @@ CASESession::~CASESession() Clear(); } -void CASESession::Finish() -{ - Transport::PeerAddress address = mExchangeCtxt->GetSessionHandle()->AsUnauthenticatedSession()->GetPeerAddress(); - - // Discard the exchange so that Clear() doesn't try closing it. The exchange will handle that. - DiscardExchange(); - - CHIP_ERROR err = ActivateSecureSession(address); - if (err == CHIP_NO_ERROR) - { - mDelegate->OnSessionEstablished(mSecureSessionHolder.Get()); - } - else - { - mDelegate->OnSessionEstablishmentError(err); - } -} - void CASESession::Clear() { // This function zeroes out and resets the memory used by the object. @@ -155,39 +137,11 @@ void CASESession::Clear() mState = State::kInitialized; Crypto::ClearSecretData(mIPK); - AbortExchange(); - mLocalNodeId = kUndefinedNodeId; mPeerNodeId = kUndefinedNodeId; mFabricInfo = nullptr; } -void CASESession::AbortExchange() -{ - if (mExchangeCtxt != nullptr) - { - // The only time we reach this is if we are getting destroyed in the - // middle of our handshake. In that case, there is no point trying to - // do MRP resends of the last message we sent, so abort the exchange - // instead of just closing it. - mExchangeCtxt->Abort(); - mExchangeCtxt = nullptr; - } -} - -void CASESession::DiscardExchange() -{ - if (mExchangeCtxt != nullptr) - { - // Make sure the exchange doesn't try to notify us when it closes, - // since we might be dead by then. - mExchangeCtxt->SetDelegate(nullptr); - // Null out mExchangeCtxt so that Clear() doesn't try closing it. The - // exchange will handle that. - mExchangeCtxt = nullptr; - } -} - CHIP_ERROR CASESession::Init(SessionManager & sessionManager, SessionEstablishmentDelegate * delegate) { VerifyOrReturnError(delegate != nullptr, CHIP_ERROR_INVALID_ARGUMENT); diff --git a/src/protocols/secure_channel/CASESession.h b/src/protocols/secure_channel/CASESession.h index 3c40a0e1a0f77a..c89a115d28e913 100644 --- a/src/protocols/secure_channel/CASESession.h +++ b/src/protocols/secure_channel/CASESession.h @@ -38,12 +38,11 @@ #include #include #include -#include +#include #include #include #include #include -#include #include #include @@ -216,17 +215,6 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, void OnSuccessStatusReport() override; CHIP_ERROR OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode) override; - // TODO: pull up Finish to PairingSession class - void Finish(); - - void AbortExchange(); - - /** - * Clear our reference to our exchange context pointer so that it can close - * itself at some later time. - */ - void DiscardExchange(); - CHIP_ERROR GetHardcodedTime(); CHIP_ERROR SetEffectiveTime(); @@ -234,8 +222,6 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, CHIP_ERROR ValidateReceivedMessage(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, const System::PacketBufferHandle & msg); - SessionEstablishmentDelegate * mDelegate = nullptr; - Crypto::Hash_SHA256_stream mCommissioningHash; Crypto::P256PublicKey mRemotePubKey; #ifdef ENABLE_HSM_CASE_EPHEMERAL_KEY @@ -250,7 +236,6 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, uint8_t mMessageDigest[Crypto::kSHA256_Hash_Length]; uint8_t mIPK[kIPKSize]; - Messaging::ExchangeContext * mExchangeCtxt = nullptr; SessionResumptionStorage * mSessionResumptionStorage = nullptr; FabricTable * mFabricsTable = nullptr; diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index cbe67852f698d1..790fd5c530f83c 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -72,21 +72,7 @@ PASESession::~PASESession() void PASESession::Finish() { mPairingComplete = true; - - Transport::PeerAddress address = mExchangeCtxt->GetSessionHandle()->AsUnauthenticatedSession()->GetPeerAddress(); - - // Discard the exchange so that Clear() doesn't try closing it. The exchange will handle that. - DiscardExchange(); - - CHIP_ERROR err = ActivateSecureSession(address); - if (err == CHIP_NO_ERROR) - { - mDelegate->OnSessionEstablished(mSecureSessionHolder.Get()); - } - else - { - mDelegate->OnSessionEstablishmentError(err); - } + PairingSession::Finish(); } void PASESession::Clear() @@ -110,29 +96,6 @@ void PASESession::Clear() mKeLen = sizeof(mKe); mPairingComplete = false; PairingSession::Clear(); - CloseExchange(); -} - -void PASESession::CloseExchange() -{ - if (mExchangeCtxt != nullptr) - { - mExchangeCtxt->Close(); - mExchangeCtxt = nullptr; - } -} - -void PASESession::DiscardExchange() -{ - if (mExchangeCtxt != nullptr) - { - // Make sure the exchange doesn't try to notify us when it closes, - // since we might be dead by then. - mExchangeCtxt->SetDelegate(nullptr); - // Null out mExchangeCtxt so that Clear() doesn't try closing it. The - // exchange will handle that. - mExchangeCtxt = nullptr; - } } CHIP_ERROR PASESession::Init(SessionManager & sessionManager, uint32_t setupCode, SessionEstablishmentDelegate * delegate) diff --git a/src/protocols/secure_channel/PASESession.h b/src/protocols/secure_channel/PASESession.h index 1168be478b4519..276be55d5d1568 100644 --- a/src/protocols/secure_channel/PASESession.h +++ b/src/protocols/secure_channel/PASESession.h @@ -35,11 +35,10 @@ #include #include #include -#include +#include #include #include #include -#include #include #include @@ -208,19 +207,8 @@ class DLL_EXPORT PASESession : public Messaging::UnsolicitedMessageHandler, void OnSuccessStatusReport() override; CHIP_ERROR OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode) override; - // TODO: pull up Finish to PairingSession class void Finish(); - void CloseExchange(); - - /** - * Clear our reference to our exchange context pointer so that it can close - * itself at some later time. - */ - void DiscardExchange(); - - SessionEstablishmentDelegate * mDelegate = nullptr; - Protocols::SecureChannel::MsgType mNextExpectedMsg = Protocols::SecureChannel::MsgType::PASE_PakeError; #ifdef ENABLE_HSM_SPAKE @@ -242,8 +230,6 @@ class DLL_EXPORT PASESession : public Messaging::UnsolicitedMessageHandler, uint16_t mSaltLength = 0; uint8_t * mSalt = nullptr; - Messaging::ExchangeContext * mExchangeCtxt = nullptr; - struct Spake2pErrorMsg { Spake2pErrorType error; diff --git a/src/transport/PairingSession.cpp b/src/protocols/secure_channel/PairingSession.cpp similarity index 77% rename from src/transport/PairingSession.cpp rename to src/protocols/secure_channel/PairingSession.cpp index ac98f4dc7a7421..f93b79e81989b1 100644 --- a/src/transport/PairingSession.cpp +++ b/src/protocols/secure_channel/PairingSession.cpp @@ -16,7 +16,7 @@ * limitations under the License. */ -#include +#include #include #include @@ -57,6 +57,37 @@ CHIP_ERROR PairingSession::ActivateSecureSession(const Transport::PeerAddress & return CHIP_NO_ERROR; } +void PairingSession::Finish() +{ + Transport::PeerAddress address = mExchangeCtxt->GetSessionHandle()->AsUnauthenticatedSession()->GetPeerAddress(); + + // Discard the exchange so that Clear() doesn't try closing it. The exchange will handle that. + DiscardExchange(); + + CHIP_ERROR err = ActivateSecureSession(address); + if (err == CHIP_NO_ERROR) + { + mDelegate->OnSessionEstablished(mSecureSessionHolder.Get()); + } + else + { + mDelegate->OnSessionEstablishmentError(err); + } +} + +void PairingSession::DiscardExchange() +{ + if (mExchangeCtxt != nullptr) + { + // Make sure the exchange doesn't try to notify us when it closes, + // since we might be dead by then. + mExchangeCtxt->SetDelegate(nullptr); + // Null out mExchangeCtxt so that Clear() doesn't try closing it. The + // exchange will handle that. + mExchangeCtxt = nullptr; + } +} + CHIP_ERROR PairingSession::EncodeMRPParameters(TLV::Tag tag, const ReliableMessageProtocolConfig & mrpConfig, TLV::TLVWriter & tlvWriter) { @@ -109,6 +140,19 @@ CHIP_ERROR PairingSession::DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TL void PairingSession::Clear() { + // Clear acts like the destructor if PairingSession, if it is call during + // middle of a pairing, means we should terminate the exchange. For normal + // path, the exchange should already be discarded before calling Clear. + if (mExchangeCtxt != nullptr) + { + // The only time we reach this is if we are getting destroyed in the + // middle of our handshake. In that case, there is no point trying to + // do MRP resends of the last message we sent, so abort the exchange + // instead of just closing it. + mExchangeCtxt->Abort(); + mExchangeCtxt = nullptr; + } + if (mSessionManager != nullptr) { if (mSecureSessionHolder && !mSecureSessionHolder->AsSecureSession()->IsActiveSession()) diff --git a/src/transport/PairingSession.h b/src/protocols/secure_channel/PairingSession.h similarity index 94% rename from src/transport/PairingSession.h rename to src/protocols/secure_channel/PairingSession.h index 72318ecd0e79b1..de75684384f71d 100644 --- a/src/transport/PairingSession.h +++ b/src/protocols/secure_channel/PairingSession.h @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -95,6 +96,10 @@ class DLL_EXPORT PairingSession CHIP_ERROR ActivateSecureSession(const Transport::PeerAddress & peerAddress); + void Finish(); + + void DiscardExchange(); // Clear our reference to our exchange context pointer so that it can close itself at some later time. + void SetPeerSessionId(uint16_t id) { mPeerSessionId.SetValue(id); } virtual void OnSuccessStatusReport() {} virtual CHIP_ERROR OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode) @@ -168,7 +173,9 @@ class DLL_EXPORT PairingSession SessionHolder mSecureSessionHolder; // mSessionManager is set if we actually allocate a secure session, so we // can clean it up later as needed. - SessionManager * mSessionManager = nullptr; + SessionManager * mSessionManager = nullptr; + Messaging::ExchangeContext * mExchangeCtxt = nullptr; + SessionEstablishmentDelegate * mDelegate = nullptr; // mLocalMRPConfig is our config which is sent to the other end and used by the peer session. // mRemoteMRPConfig is received from other end and set to our session. diff --git a/src/protocols/secure_channel/tests/BUILD.gn b/src/protocols/secure_channel/tests/BUILD.gn index 23039e649b9286..11983ba685b76f 100644 --- a/src/protocols/secure_channel/tests/BUILD.gn +++ b/src/protocols/secure_channel/tests/BUILD.gn @@ -15,6 +15,7 @@ chip_test_suite("tests") { # "TestMessageCounterManager.cpp", "TestDefaultSessionResumptionStorage.cpp", "TestPASESession.cpp", + "TestPairingSession.cpp", "TestSimpleSessionResumptionStorage.cpp", "TestStatusReport.cpp", ] diff --git a/src/transport/tests/TestPairingSession.cpp b/src/protocols/secure_channel/tests/TestPairingSession.cpp similarity index 99% rename from src/transport/tests/TestPairingSession.cpp rename to src/protocols/secure_channel/tests/TestPairingSession.cpp index b40c044d4d893a..e34094ff7cb34a 100644 --- a/src/transport/tests/TestPairingSession.cpp +++ b/src/protocols/secure_channel/tests/TestPairingSession.cpp @@ -26,11 +26,9 @@ #include #include - -#include -#include - #include +#include +#include #include #include #include diff --git a/src/transport/BUILD.gn b/src/transport/BUILD.gn index 42ea4dcd73b839..defde06d387fdc 100644 --- a/src/transport/BUILD.gn +++ b/src/transport/BUILD.gn @@ -30,7 +30,6 @@ static_library("transport") { "MessageCounter.cpp", "MessageCounter.h", "MessageCounterManagerInterface.h", - "PairingSession.cpp", "PeerMessageCounter.h", "SecureMessageCodec.cpp", "SecureMessageCodec.h", diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp index 2dbc71e1a6d569..fb23bb46116b1f 100644 --- a/src/transport/SessionManager.cpp +++ b/src/transport/SessionManager.cpp @@ -40,7 +40,6 @@ #include #include #include -#include #include #include diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index 260db7866a395b..7cd5bf2925f98a 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -53,8 +53,6 @@ namespace chip { -class PairingSession; - /** * @brief * Tracks ownership of a encrypted packet buffer. diff --git a/src/transport/tests/BUILD.gn b/src/transport/tests/BUILD.gn index ad14819c67145d..4fea0a0a66a1c3 100644 --- a/src/transport/tests/BUILD.gn +++ b/src/transport/tests/BUILD.gn @@ -34,7 +34,6 @@ chip_test_suite("tests") { test_sources = [ "TestGroupMessageCounter.cpp", - "TestPairingSession.cpp", "TestPeerConnections.cpp", "TestPeerMessageCounter.cpp", "TestSecureSession.cpp",