From 40250247d4f5ad5e52675ffe72a5d2c33474f6b7 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 9 Feb 2022 14:19:08 -0500 Subject: [PATCH] Make FabricIndex be a common thing across all sessions. (#14904) * Move fabric index into session.h instead of SecureSession * Remove the cast to securesession when getting the fabric index * Make group sessions use the common per-session fabric index * Restyle * Address code review comments * Use undefined fabric index as an invalid marker --- .../chip-tool/commands/common/CommandInvoker.h | 3 +-- src/app/CommandHandler.cpp | 12 +----------- src/app/InteractionModelEngine.cpp | 2 +- src/app/ReadClient.cpp | 4 ++-- src/app/ReadHandler.cpp | 2 +- src/app/WriteHandler.cpp | 12 +----------- .../general-commissioning-server.cpp | 2 +- .../operational-credentials-server.cpp | 2 +- src/controller/CHIPCluster.cpp | 2 +- src/credentials/FabricTable.h | 2 ++ src/transport/GroupSession.h | 4 +--- src/transport/SecureSession.cpp | 4 ++-- src/transport/SecureSession.h | 15 ++++++--------- src/transport/Session.h | 6 ++++++ 14 files changed, 27 insertions(+), 45 deletions(-) diff --git a/examples/chip-tool/commands/common/CommandInvoker.h b/examples/chip-tool/commands/common/CommandInvoker.h index 29088db1c09a32..11c0c44eb71063 100644 --- a/examples/chip-tool/commands/common/CommandInvoker.h +++ b/examples/chip-tool/commands/common/CommandInvoker.h @@ -235,8 +235,7 @@ CHIP_ERROR InvokeGroupCommand(DeviceProxy * aDevice, void * aContext, // // We assume the aDevice already has a Case session which is way we can use he established Secure Session ReturnErrorOnFailure(invoker->InvokeGroupCommand(aDevice->GetExchangeManager(), - aDevice->GetSecureSession().Value()->AsSecureSession()->GetFabricIndex(), - groupId, aRequestData)); + aDevice->GetSecureSession().Value()->GetFabricIndex(), groupId, aRequestData)); // invoker is already deleted and is not to be used invoker.release(); diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index e83b45a09dda1c..9d2cc5d467e6df 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -566,17 +566,7 @@ TLV::TLVWriter * CommandHandler::GetCommandDataIBTLVWriter() FabricIndex CommandHandler::GetAccessingFabricIndex() const { - FabricIndex fabric = kUndefinedFabricIndex; - if (mpExchangeCtx->GetSessionHandle()->IsGroupSession()) - { - fabric = mpExchangeCtx->GetSessionHandle()->AsGroupSession()->GetFabricIndex(); - } - else - { - fabric = mpExchangeCtx->GetSessionHandle()->AsSecureSession()->GetFabricIndex(); - } - - return fabric; + return mpExchangeCtx->GetSessionHandle()->GetFabricIndex(); } CommandHandler * CommandHandler::Handle::Get() diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 071591ac46192c..81956a730c4a2c 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -276,7 +276,7 @@ CHIP_ERROR InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeConte ChipLogProgress(InteractionModel, "Deleting previous subscription from NodeId: " ChipLogFormatX64 ", FabricIndex: %" PRIu8, ChipLogValueX64(apExchangeContext->GetSessionHandle()->AsSecureSession()->GetPeerNodeId()), - apExchangeContext->GetSessionHandle()->AsSecureSession()->GetFabricIndex()); + apExchangeContext->GetSessionHandle()->GetFabricIndex()); mReadHandlers.ReleaseObject(handler); } diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 126910fde78da5..c46cba6ee232c8 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -258,7 +258,7 @@ CHIP_ERROR ReadClient::SendReadRequest(ReadPrepareParams & aReadPrepareParams) Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse))); mPeerNodeId = aReadPrepareParams.mSessionHolder->AsSecureSession()->GetPeerNodeId(); - mFabricIndex = aReadPrepareParams.mSessionHolder->AsSecureSession()->GetFabricIndex(); + mFabricIndex = aReadPrepareParams.mSessionHolder->GetFabricIndex(); MoveToState(ClientState::AwaitingInitialReport); @@ -801,7 +801,7 @@ CHIP_ERROR ReadClient::SendSubscribeRequest(ReadPrepareParams & aReadPreparePara Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse))); mPeerNodeId = aReadPrepareParams.mSessionHolder->AsSecureSession()->GetPeerNodeId(); - mFabricIndex = aReadPrepareParams.mSessionHolder->AsSecureSession()->GetFabricIndex(); + mFabricIndex = aReadPrepareParams.mSessionHolder->GetFabricIndex(); MoveToState(ClientState::AwaitingInitialReport); diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 4b08abe8e410ea..0f49aaa1b7cde8 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -261,7 +261,7 @@ bool ReadHandler::IsFromSubscriber(Messaging::ExchangeContext & apExchangeContex { return (IsType(InteractionType::Subscribe) && GetInitiatorNodeId() == apExchangeContext.GetSessionHandle()->AsSecureSession()->GetPeerNodeId() && - GetAccessingFabricIndex() == apExchangeContext.GetSessionHandle()->AsSecureSession()->GetFabricIndex()); + GetAccessingFabricIndex() == apExchangeContext.GetSessionHandle()->GetFabricIndex()); } CHIP_ERROR ReadHandler::OnUnknownMsgType(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader, diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp index 7090f8d00a6091..82d9fb55d7f3ce 100644 --- a/src/app/WriteHandler.cpp +++ b/src/app/WriteHandler.cpp @@ -479,17 +479,7 @@ CHIP_ERROR WriteHandler::AddStatus(const ConcreteAttributePath & aPath, const St FabricIndex WriteHandler::GetAccessingFabricIndex() const { - FabricIndex fabric = kUndefinedFabricIndex; - if (mpExchangeCtx->GetSessionHandle()->IsGroupSession()) - { - fabric = mpExchangeCtx->GetSessionHandle()->AsGroupSession()->GetFabricIndex(); - } - else - { - fabric = mpExchangeCtx->GetSessionHandle()->AsSecureSession()->GetFabricIndex(); - } - - return fabric; + return mpExchangeCtx->GetSessionHandle()->GetFabricIndex(); } const char * WriteHandler::GetStateStr() const diff --git a/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp b/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp index 4bc13525cb1128..e1e3d1c735f986 100644 --- a/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp +++ b/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp @@ -150,7 +150,7 @@ bool emberAfGeneralCommissioningClusterCommissioningCompleteCallback( * Once bindings are implemented, this may no longer be needed. */ SessionHandle handle = commandObj->GetExchangeContext()->GetSessionHandle(); - server->SetFabricIndex(handle->AsSecureSession()->GetFabricIndex()); + server->SetFabricIndex(handle->GetFabricIndex()); server->SetPeerNodeId(handle->AsSecureSession()->GetPeerNodeId()); CheckSuccess(server->CommissioningComplete(), Failure); diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 8f3d5b6df4c137..6d4655bddf00ca 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -351,7 +351,7 @@ class FabricCleanupExchangeDelegate : public chip::Messaging::ExchangeDelegate void OnResponseTimeout(chip::Messaging::ExchangeContext * ec) override {} void OnExchangeClosing(chip::Messaging::ExchangeContext * ec) override { - FabricIndex currentFabricIndex = ec->GetSessionHandle()->AsSecureSession()->GetFabricIndex(); + FabricIndex currentFabricIndex = ec->GetSessionHandle()->GetFabricIndex(); ec->GetExchangeMgr()->GetSessionManager()->ExpireAllPairingsForFabric(currentFabricIndex); } }; diff --git a/src/controller/CHIPCluster.cpp b/src/controller/CHIPCluster.cpp index ef94a7c2c823b2..87c876329655c5 100644 --- a/src/controller/CHIPCluster.cpp +++ b/src/controller/CHIPCluster.cpp @@ -53,7 +53,7 @@ CHIP_ERROR ClusterBase::AssociateWithGroup(DeviceProxy * device, GroupId groupId { // Local copy to preserve original SessionHandle for future Unicast communication. Optional session = mDevice->GetExchangeManager()->GetSessionManager()->CreateGroupSession( - groupId, mDevice->GetSecureSession().Value()->AsSecureSession()->GetFabricIndex()); + groupId, mDevice->GetSecureSession().Value()->GetFabricIndex()); // Sanity check if (!session.HasValue() || !session.Value()->IsGroupSession()) { diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h index 90086c26d22471..631f3cf5dc2ffc 100644 --- a/src/credentials/FabricTable.h +++ b/src/credentials/FabricTable.h @@ -46,6 +46,8 @@ static constexpr FabricIndex kMinValidFabricIndex = 1; static constexpr FabricIndex kMaxValidFabricIndex = std::min(UINT8_MAX - 1, CHIP_CONFIG_MAX_FABRICS); static constexpr uint8_t kFabricLabelMaxLengthInBytes = 32; +static_assert(kUndefinedFabricIndex < chip::kMinValidFabricIndex, "Undefined fabric index should not be valid"); + // KVS store is sensitive to length of key strings, based on the underlying // platform. Keeping them short. constexpr char kFabricTableKeyPrefix[] = "Fabric"; diff --git a/src/transport/GroupSession.h b/src/transport/GroupSession.h index 6b5b25ff56d025..1b6a4bd51ed9d4 100644 --- a/src/transport/GroupSession.h +++ b/src/transport/GroupSession.h @@ -27,7 +27,7 @@ namespace Transport { class GroupSession : public Session { public: - GroupSession(GroupId group, FabricIndex fabricIndex) : mGroupId(group), mFabricIndex(fabricIndex) {} + GroupSession(GroupId group, FabricIndex fabricIndex) : mGroupId(group) { SetFabricIndex(fabricIndex); } ~GroupSession() { NotifySessionReleased(); } Session::SessionType GetSessionType() const override { return Session::SessionType::kGroup; } @@ -59,11 +59,9 @@ class GroupSession : public Session } GroupId GetGroupId() const { return mGroupId; } - FabricIndex GetFabricIndex() const { return mFabricIndex; } private: const GroupId mGroupId; - const FabricIndex mFabricIndex; }; /* diff --git a/src/transport/SecureSession.cpp b/src/transport/SecureSession.cpp index 67e4378ba4e0e0..e2688869bc509a 100644 --- a/src/transport/SecureSession.cpp +++ b/src/transport/SecureSession.cpp @@ -28,13 +28,13 @@ Access::SubjectDescriptor SecureSession::GetSubjectDescriptor() const subjectDescriptor.authMode = Access::AuthMode::kCase; subjectDescriptor.subject = mPeerNodeId; subjectDescriptor.cats = mPeerCATs; - subjectDescriptor.fabricIndex = mFabric; + subjectDescriptor.fabricIndex = GetFabricIndex(); } else if (IsPAKEKeyId(mPeerNodeId)) { subjectDescriptor.authMode = Access::AuthMode::kPase; subjectDescriptor.subject = mPeerNodeId; - subjectDescriptor.fabricIndex = mFabric; + subjectDescriptor.fabricIndex = GetFabricIndex(); } else { diff --git a/src/transport/SecureSession.h b/src/transport/SecureSession.h index d3aa6a9ed86b7b..04094961ae6c6e 100644 --- a/src/transport/SecureSession.h +++ b/src/transport/SecureSession.h @@ -67,8 +67,10 @@ class SecureSession : public Session FabricIndex fabric, const ReliableMessageProtocolConfig & config) : mSecureSessionType(secureSessionType), mPeerNodeId(peerNodeId), mPeerCATs(peerCATs), mLocalSessionId(localSessionId), mPeerSessionId(peerSessionId), - mFabric(fabric), mLastActivityTime(System::SystemClock().GetMonotonicTimestamp()), mMRPConfig(config) - {} + mLastActivityTime(System::SystemClock().GetMonotonicTimestamp()), mMRPConfig(config) + { + SetFabricIndex(fabric); + } ~SecureSession() { NotifySessionReleased(); } SecureSession(SecureSession &&) = delete; @@ -112,7 +114,6 @@ class SecureSession : public Session uint16_t GetLocalSessionId() const { return mLocalSessionId; } uint16_t GetPeerSessionId() const { return mPeerSessionId; } - FabricIndex GetFabricIndex() const { return mFabric; } // Should only be called for PASE sessions, which start with undefined fabric, // to migrate to a newly commissioned fabric after successful @@ -123,10 +124,10 @@ class SecureSession : public Session // TODO(#13711): this check won't work until the issue is addressed if (mSecureSessionType == Type::kPASE) { - mFabric = fabricIndex; + SetFabricIndex(fabricIndex); } #else - mFabric = fabricIndex; + SetFabricIndex(fabricIndex); #endif return CHIP_NO_ERROR; } @@ -145,10 +146,6 @@ class SecureSession : public Session const uint16_t mLocalSessionId; const uint16_t mPeerSessionId; - // PASE sessions start with undefined fabric, but are migrated to a newly - // commissioned fabric after successful OperationalCredentialsCluster::AddNOC - FabricIndex mFabric; - PeerAddress mPeerAddress; System::Clock::Timestamp mLastActivityTime; ReliableMessageProtocolConfig mMRPConfig; diff --git a/src/transport/Session.h b/src/transport/Session.h index 4e343cbef26034..99be7b2315a65b 100644 --- a/src/transport/Session.h +++ b/src/transport/Session.h @@ -16,6 +16,7 @@ #pragma once +#include #include #include #include @@ -67,6 +68,8 @@ class Session virtual const ReliableMessageProtocolConfig & GetMRPConfig() const = 0; virtual System::Clock::Milliseconds32 GetAckTimeout() const = 0; + FabricIndex GetFabricIndex() const { return mFabricIndex; } + SecureSession * AsSecureSession(); UnauthenticatedSession * AsUnauthenticatedSession(); GroupSession * AsGroupSession(); @@ -85,8 +88,11 @@ class Session } } + void SetFabricIndex(FabricIndex index) { mFabricIndex = index; } + private: IntrusiveList mHolders; + FabricIndex mFabricIndex = kUndefinedFabricIndex; }; } // namespace Transport