From ca3c297a74963ac5c269b9a2897d89633cd96b73 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 1 Aug 2023 12:15:30 -0400 Subject: [PATCH] Make SessionResumptionStorage injectable in controller. (#28422) * Make SessionResumptionStorage injectable in controller. Fixes https://github.com/project-chip/connectedhomeip/issues/28351 * Address review comment. --- .../CHIPDeviceControllerFactory.cpp | 27 ++++++++++++++----- src/controller/CHIPDeviceControllerFactory.h | 7 +++-- .../CHIPDeviceControllerSystemState.h | 27 ++++++++++++++++--- 3 files changed, 49 insertions(+), 12 deletions(-) diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index f39a3c7a65ed0e..c57cb9a57dd28c 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -65,6 +65,7 @@ CHIP_ERROR DeviceControllerFactory::Init(FactoryInitParams params) mOperationalKeystore = params.operationalKeystore; mOpCertStore = params.opCertStore; mCertificateValidityPolicy = params.certificateValidityPolicy; + mSessionResumptionStorage = params.sessionResumptionStorage; mEnableServerInteractions = params.enableServerInteractions; CHIP_ERROR err = InitSystemState(params); @@ -94,6 +95,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState() params.operationalKeystore = mOperationalKeystore; params.opCertStore = mOpCertStore; params.certificateValidityPolicy = mCertificateValidityPolicy; + params.sessionResumptionStorage = mSessionResumptionStorage; } return InitSystemState(params); @@ -195,12 +197,24 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) tempFabricTable = stateParams.fabricTable; } - auto sessionResumptionStorage = chip::Platform::MakeUnique(); - ReturnErrorOnFailure(sessionResumptionStorage->Init(params.fabricIndependentStorage)); - stateParams.sessionResumptionStorage = std::move(sessionResumptionStorage); + SessionResumptionStorage * sessionResumptionStorage; + if (params.sessionResumptionStorage == nullptr) + { + auto ownedSessionResumptionStorage = chip::Platform::MakeUnique(); + ReturnErrorOnFailure(ownedSessionResumptionStorage->Init(params.fabricIndependentStorage)); + stateParams.ownedSessionResumptionStorage = std::move(ownedSessionResumptionStorage); + stateParams.externalSessionResumptionStorage = nullptr; + sessionResumptionStorage = stateParams.ownedSessionResumptionStorage.get(); + } + else + { + stateParams.ownedSessionResumptionStorage = nullptr; + stateParams.externalSessionResumptionStorage = params.sessionResumptionStorage; + sessionResumptionStorage = stateParams.externalSessionResumptionStorage; + } auto delegate = chip::Platform::MakeUnique(); - ReturnErrorOnFailure(delegate->Init(stateParams.sessionResumptionStorage.get(), stateParams.groupDataProvider)); + ReturnErrorOnFailure(delegate->Init(sessionResumptionStorage, stateParams.groupDataProvider)); stateParams.fabricTableDelegate = delegate.get(); ReturnErrorOnFailure(stateParams.fabricTable->AddFabricDelegate(stateParams.fabricTableDelegate)); delegate.release(); @@ -222,7 +236,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) // Enable listening for session establishment messages. ReturnErrorOnFailure(stateParams.caseServer->ListenForSessionEstablishment( - stateParams.exchangeMgr, stateParams.sessionMgr, stateParams.fabricTable, stateParams.sessionResumptionStorage.get(), + stateParams.exchangeMgr, stateParams.sessionMgr, stateParams.fabricTable, sessionResumptionStorage, stateParams.certificateValidityPolicy, stateParams.groupDataProvider)); // @@ -256,7 +270,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) CASEClientInitParams sessionInitParams = { .sessionManager = stateParams.sessionMgr, - .sessionResumptionStorage = stateParams.sessionResumptionStorage.get(), + .sessionResumptionStorage = sessionResumptionStorage, .certificateValidityPolicy = stateParams.certificateValidityPolicy, .exchangeMgr = stateParams.exchangeMgr, .fabricTable = stateParams.fabricTable, @@ -373,6 +387,7 @@ void DeviceControllerFactory::Shutdown() mOperationalKeystore = nullptr; mOpCertStore = nullptr; mCertificateValidityPolicy = nullptr; + mSessionResumptionStorage = nullptr; } void DeviceControllerSystemState::Shutdown() diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index f807f39e1fd570..04855d62cfc3d2 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -34,6 +34,7 @@ #include #include #include +#include namespace chip { @@ -106,8 +107,8 @@ struct SetupParams }; // TODO everything other than the fabric storage, group data provider, OperationalKeystore, -// OperationalCertificateStore and SessionKeystore here should be removed. We're blocked -// because of the need to support !CHIP_DEVICE_LAYER +// OperationalCertificateStore, SessionKeystore, and SessionResumptionStorage here should +// be removed. We're blocked because of the need to support !CHIP_DEVICE_LAYER struct FactoryInitParams { System::Layer * systemLayer = nullptr; @@ -121,6 +122,7 @@ struct FactoryInitParams FabricTable * fabricTable = nullptr; OperationalKeystore * operationalKeystore = nullptr; Credentials::OperationalCertificateStore * opCertStore = nullptr; + SessionResumptionStorage * sessionResumptionStorage = nullptr; #if CONFIG_NETWORK_LAYER_BLE Ble::BleLayer * bleLayer = nullptr; #endif @@ -257,6 +259,7 @@ class DeviceControllerFactory Crypto::OperationalKeystore * mOperationalKeystore = nullptr; Credentials::OperationalCertificateStore * mOpCertStore = nullptr; Credentials::CertificateValidityPolicy * mCertificateValidityPolicy = nullptr; + SessionResumptionStorage * mSessionResumptionStorage = nullptr; bool mEnableServerInteractions = false; }; diff --git a/src/controller/CHIPDeviceControllerSystemState.h b/src/controller/CHIPDeviceControllerSystemState.h index 37f85b1cd90784..6692fc6ecbc308 100644 --- a/src/controller/CHIPDeviceControllerSystemState.h +++ b/src/controller/CHIPDeviceControllerSystemState.h @@ -85,10 +85,18 @@ struct DeviceControllerSystemStateParams Credentials::GroupDataProvider * groupDataProvider = nullptr; Crypto::SessionKeystore * sessionKeystore = nullptr; + // NOTE: Exactly one of externalSessionResumptionStorage (externally provided, + // externally owned) or ownedSessionResumptionStorage (managed by the system + // state) must be non-null. + SessionResumptionStorage * externalSessionResumptionStorage = nullptr; + // Params that will be deallocated via Platform::Delete in // DeviceControllerSystemState::Shutdown. DeviceTransportMgr * transportMgr = nullptr; - Platform::UniquePtr sessionResumptionStorage; + // NOTE: Exactly one of externalSessionResumptionStorage (externally provided, + // externally owned) or ownedSessionResumptionStorage (managed by the system + // state) must be non-null. + Platform::UniquePtr ownedSessionResumptionStorage; Credentials::CertificateValidityPolicy * certificateValidityPolicy = nullptr; SessionManager * sessionMgr = nullptr; Protocols::SecureChannel::UnsolicitedStatusHandler * unsolicitedStatusHandler = nullptr; @@ -132,8 +140,18 @@ class DeviceControllerSystemState mCASESessionManager(params.caseSessionManager), mSessionSetupPool(params.sessionSetupPool), mCASEClientPool(params.caseClientPool), mGroupDataProvider(params.groupDataProvider), mTimerDelegate(params.timerDelegate), mReportScheduler(params.reportScheduler), mSessionKeystore(params.sessionKeystore), - mFabricTableDelegate(params.fabricTableDelegate), mSessionResumptionStorage(std::move(params.sessionResumptionStorage)) + mFabricTableDelegate(params.fabricTableDelegate), + mOwnedSessionResumptionStorage(std::move(params.ownedSessionResumptionStorage)) { + if (mOwnedSessionResumptionStorage) + { + mSessionResumptionStorage = mOwnedSessionResumptionStorage.get(); + } + else + { + mSessionResumptionStorage = params.externalSessionResumptionStorage; + } + #if CONFIG_NETWORK_LAYER_BLE mBleLayer = params.bleLayer; #endif @@ -172,7 +190,7 @@ class DeviceControllerSystemState mUnsolicitedStatusHandler != nullptr && mExchangeMgr != nullptr && mMessageCounterManager != nullptr && mFabrics != nullptr && mCASESessionManager != nullptr && mSessionSetupPool != nullptr && mCASEClientPool != nullptr && mGroupDataProvider != nullptr && mReportScheduler != nullptr && mTimerDelegate != nullptr && - mSessionKeystore != nullptr; + mSessionKeystore != nullptr && mSessionResumptionStorage != nullptr; }; System::Layer * SystemLayer() const { return mSystemLayer; }; @@ -221,7 +239,8 @@ class DeviceControllerSystemState app::reporting::ReportScheduler * mReportScheduler = nullptr; Crypto::SessionKeystore * mSessionKeystore = nullptr; FabricTable::Delegate * mFabricTableDelegate = nullptr; - Platform::UniquePtr mSessionResumptionStorage; + SessionResumptionStorage * mSessionResumptionStorage = nullptr; + Platform::UniquePtr mOwnedSessionResumptionStorage; // If mTempFabricTable is not null, it was created during // DeviceControllerFactory::InitSystemState and needs to be