Separate FabricTable storage from Persistent Storage Delegate (#10747)

* Separate FabricTable storage from Persistent Storage Delegate * Fix Android compile * Fix format * Attempt to fix compile again * Move the persistent storage delegate into the Controller/Commissioner SetupParams * Stop checking for invalid uncompressed fabric Id * Update naming inside FabricTable * Update SimpleFabricStorage to preserve FabricIndex * Remove usage of std::string inside SimpleFabricStorage * Pass the mutable char spans by value instead
project-chip · Jul 20, 2023 · 1248019 · 1248019
1 parent 90fa6e0
commit 1248019
Show file tree

Hide file tree

Showing 14 changed files with 239 additions and 45 deletions.
diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp
@@ -37,6 +37,7 @@ CHIP_ERROR CHIPCommand::Run()
 
     ReturnLogErrorOnFailure(mStorage.Init());
     ReturnLogErrorOnFailure(mOpCredsIssuer.Initialize(mStorage));
+    ReturnLogErrorOnFailure(mFabricStorage.Initialize(&mStorage));
 
     chip::Platform::ScopedMemoryBuffer<uint8_t> noc;
     chip::Platform::ScopedMemoryBuffer<uint8_t> icac;
@@ -63,10 +64,11 @@ CHIP_ERROR CHIPCommand::Run()
                                                                            rcacSpan, icacSpan, nocSpan));
 
     chip::Controller::FactoryInitParams factoryInitParams;
-    factoryInitParams.storageDelegate = &mStorage;
-    factoryInitParams.listenPort      = mStorage.GetListenPort();
+    factoryInitParams.fabricStorage = &mFabricStorage;
+    factoryInitParams.listenPort    = mStorage.GetListenPort();
 
     chip::Controller::SetupParams commissionerParams;
+    commissionerParams.storageDelegate                = &mStorage;
     commissionerParams.operationalCredentialsDelegate = &mOpCredsIssuer;
     commissionerParams.ephemeralKeypair               = &ephemeralKey;
     commissionerParams.controllerRCAC                 = rcacSpan;

diff --git a/examples/chip-tool/commands/common/CHIPCommand.h b/examples/chip-tool/commands/common/CHIPCommand.h
@@ -65,6 +65,7 @@ class CHIPCommand : public Command
 
     ChipDeviceCommissioner mController;
     PersistentStorage mStorage;
+    chip::SimpleFabricStorage mFabricStorage;
 
 private:
     static void RunQueuedCommand(intptr_t commandArg);

diff --git a/examples/platform/linux/AppMain.cpp b/examples/platform/linux/AppMain.cpp
@@ -209,6 +209,7 @@ class MyServerStorageDelegate : public PersistentStorageDelegate
 
 DeviceCommissioner gCommissioner;
 MyServerStorageDelegate gServerStorage;
+chip::SimpleFabricStorage gFabricStorage;
 ExampleOperationalCredentialsIssuer gOpCredsIssuer;
 
 CHIP_ERROR InitCommissioner()
@@ -218,9 +219,12 @@ CHIP_ERROR InitCommissioner()
     chip::Controller::FactoryInitParams factoryParams;
     chip::Controller::SetupParams params;
 
-    factoryParams.storageDelegate = &gServerStorage;
+    ReturnErrorOnFailure(gFabricStorage.Initialize(&gServerStorage));
+
+    factoryParams.fabricStorage = &gFabricStorage;
     // use a different listen port for the commissioner.
     factoryParams.listenPort              = LinuxDeviceOptions::GetInstance().securedCommissionerPort;
+    params.storageDelegate                = &gServerStorage;
     params.deviceAddressUpdateDelegate    = nullptr;
     params.operationalCredentialsDelegate = &gOpCredsIssuer;
 

diff --git a/src/app/server/Server.h b/src/app/server/Server.h
@@ -86,7 +86,7 @@ class Server : public Messaging::ExchangeDelegate
 
     static Server sServer;
 
-    class ServerStorageDelegate : public PersistentStorageDelegate
+    class ServerStorageDelegate : public PersistentStorageDelegate, public FabricStorage
     {
         CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override
         {
@@ -108,6 +108,18 @@ class Server : public Messaging::ExchangeDelegate
             ChipLogProgress(AppServer, "Deleted from server storage: %s", key);
             return CHIP_NO_ERROR;
         }
+
+        CHIP_ERROR SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override
+        {
+            return SyncSetKeyValue(key, buffer, size);
+        };
+
+        CHIP_ERROR SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override
+        {
+            return SyncGetKeyValue(key, buffer, size);
+        };
+
+        CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) override { return SyncDeleteKeyValue(key); };
     };
 
     // Messaging::ExchangeDelegate

diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp
@@ -49,8 +49,8 @@ CHIP_ERROR DeviceControllerFactory::Init(FactoryInitParams params)
         return CHIP_NO_ERROR;
     }
 
-    mListenPort      = params.listenPort;
-    mStorageDelegate = params.storageDelegate;
+    mListenPort    = params.listenPort;
+    mFabricStorage = params.fabricStorage;
 
     CHIP_ERROR err = InitSystemState(params);
 
@@ -134,7 +134,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
     stateParams.exchangeMgr           = chip::Platform::New<Messaging::ExchangeManager>();
     stateParams.messageCounterManager = chip::Platform::New<secure_channel::MessageCounterManager>();
 
-    ReturnErrorOnFailure(stateParams.fabricTable->Init(mStorageDelegate));
+    ReturnErrorOnFailure(stateParams.fabricTable->Init(mFabricStorage));
     ReturnErrorOnFailure(
         stateParams.sessionMgr->Init(stateParams.systemLayer, stateParams.transportMgr, stateParams.messageCounterManager));
     ReturnErrorOnFailure(stateParams.exchangeMgr->Init(stateParams.sessionMgr));
@@ -160,9 +160,9 @@ void DeviceControllerFactory::PopulateInitParams(ControllerInitParams & controll
     controllerParams.controllerICAC                 = params.controllerICAC;
     controllerParams.controllerRCAC                 = params.controllerRCAC;
     controllerParams.fabricId                       = params.fabricId;
+    controllerParams.storageDelegate                = params.storageDelegate;
 
     controllerParams.systemState        = mSystemState;
-    controllerParams.storageDelegate    = mStorageDelegate;
     controllerParams.controllerVendorId = params.controllerVendorId;
 }
 
@@ -210,7 +210,7 @@ DeviceControllerFactory::~DeviceControllerFactory()
         chip::Platform::Delete(mSystemState);
         mSystemState = nullptr;
     }
-    mStorageDelegate = nullptr;
+    mFabricStorage = nullptr;
 }
 
 CHIP_ERROR DeviceControllerSystemState::Shutdown()

diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h
@@ -43,6 +43,8 @@ struct SetupParams
 #endif
     OperationalCredentialsDelegate * operationalCredentialsDelegate = nullptr;
 
+    PersistentStorageDelegate * storageDelegate = nullptr;
+
     /* The following keypair must correspond to the public key used for generating
     controllerNOC. It's used by controller to establish CASE sessions with devices */
     Crypto::P256Keypair * ephemeralKeypair = nullptr;
@@ -60,11 +62,11 @@ struct SetupParams
     DevicePairingDelegate * pairingDelegate = nullptr;
 };
 
-// TODO everything other than the storage delegate here should be removed.
+// TODO everything other than the fabric storage here should be removed.
 // We're blocked because of the need to support !CHIP_DEVICE_LAYER
 struct FactoryInitParams
 {
-    PersistentStorageDelegate * storageDelegate           = nullptr;
+    FabricStorage * fabricStorage                         = nullptr;
     System::Layer * systemLayer                           = nullptr;
     Inet::InetLayer * inetLayer                           = nullptr;
     DeviceControllerInteractionModelDelegate * imDelegate = nullptr;
@@ -109,8 +111,8 @@ class DeviceControllerFactory
     CHIP_ERROR InitSystemState();
 
     uint16_t mListenPort;
-    PersistentStorageDelegate * mStorageDelegate = nullptr;
-    DeviceControllerSystemState * mSystemState   = nullptr;
+    FabricStorage * mFabricStorage             = nullptr;
+    DeviceControllerSystemState * mSystemState = nullptr;
 };
 
 } // namespace Controller

diff --git a/src/controller/java/AndroidDeviceControllerWrapper.cpp b/src/controller/java/AndroidDeviceControllerWrapper.cpp
@@ -209,12 +209,13 @@ AndroidDeviceControllerWrapper * AndroidDeviceControllerWrapper::AllocateNew(Jav
     chip::Controller::FactoryInitParams initParams;
     chip::Controller::SetupParams setupParams;
 
-    initParams.storageDelegate = wrapper.get();
-    initParams.systemLayer     = systemLayer;
-    initParams.inetLayer       = inetLayer;
+    initParams.systemLayer   = systemLayer;
+    initParams.inetLayer     = inetLayer;
+    initParams.fabricStorage = wrapper.get();
     // move bleLayer into platform/android to share with app server
     initParams.bleLayer                        = DeviceLayer::ConnectivityMgr().GetBleLayer();
     initParams.listenPort                      = CHIP_PORT + 1;
+    setupParams.storageDelegate                = wrapper.get();
     setupParams.pairingDelegate                = wrapper.get();
     setupParams.operationalCredentialsDelegate = wrapper.get();
 
@@ -352,3 +353,19 @@ CHIP_ERROR AndroidDeviceControllerWrapper::SyncDeleteKeyValue(const char * key)
     ChipLogProgress(chipTool, "KVS: Deleting key %s", key);
     return chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Delete(key);
 }
+
+CHIP_ERROR AndroidDeviceControllerWrapper::SyncStore(chip::FabricIndex fabricIndex, const char * key, const void * buffer,
+                                                     uint16_t size)
+{
+    return SyncSetKeyValue(key, buffer, size);
+};
+
+CHIP_ERROR AndroidDeviceControllerWrapper::SyncLoad(chip::FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size)
+{
+    return SyncGetKeyValue(key, buffer, size);
+};
+
+CHIP_ERROR AndroidDeviceControllerWrapper::SyncDelete(chip::FabricIndex fabricIndex, const char * key)
+{
+    return SyncDeleteKeyValue(key);
+};
diff --git a/src/controller/java/AndroidDeviceControllerWrapper.h b/src/controller/java/AndroidDeviceControllerWrapper.h
@@ -37,7 +37,8 @@
 class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDelegate,
                                        public chip::Controller::DeviceStatusDelegate,
                                        public chip::Controller::OperationalCredentialsDelegate,
-                                       public chip::PersistentStorageDelegate
+                                       public chip::PersistentStorageDelegate,
+                                       public chip::FabricStorage
 {
 public:
     ~AndroidDeviceControllerWrapper();
@@ -78,6 +79,11 @@ class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDel
     CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override;
     CHIP_ERROR SyncDeleteKeyValue(const char * key) override;
 
+    // FabricStorage implementation
+    CHIP_ERROR SyncStore(chip::FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override;
+    CHIP_ERROR SyncLoad(chip::FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override;
+    CHIP_ERROR SyncDelete(chip::FabricIndex fabricIndex, const char * key) override;
+
     static AndroidDeviceControllerWrapper * FromJNIHandle(jlong handle)
     {
         return reinterpret_cast<AndroidDeviceControllerWrapper *>(handle);

diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp
@@ -86,6 +86,7 @@ chip::Controller::PythonPersistentStorageDelegate sStorageDelegate;
 chip::Controller::ScriptDevicePairingDelegate sPairingDelegate;
 chip::Controller::ScriptDeviceAddressUpdateDelegate sDeviceAddressUpdateDelegate;
 chip::Controller::ExampleOperationalCredentialsIssuer sOperationalCredentialsIssuer;
+chip::SimpleFabricStorage sFabricStorage;
 } // namespace
 
 // NOTE: Remote device ID is in sync with the echo server device id
@@ -186,6 +187,9 @@ ChipError::StorageType pychip_DeviceController_NewDeviceController(chip::Control
     CHIP_ERROR err = sOperationalCredentialsIssuer.Initialize(sStorageDelegate);
     VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger());
 
+    err = sFabricStorage.Initialize(&sStorageDelegate);
+    VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger());
+
     chip::Crypto::P256Keypair ephemeralKey;
     err = ephemeralKey.Initialize();
     VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger());
@@ -207,10 +211,11 @@ ChipError::StorageType pychip_DeviceController_NewDeviceController(chip::Control
     VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger());
 
     FactoryInitParams factoryParams;
-    factoryParams.storageDelegate = &sStorageDelegate;
-    factoryParams.imDelegate      = &PythonInteractionModelDelegate::Instance();
+    factoryParams.fabricStorage = &sFabricStorage;
+    factoryParams.imDelegate    = &PythonInteractionModelDelegate::Instance();
 
     SetupParams initParams;
+    initParams.storageDelegate                = &sStorageDelegate;
     initParams.deviceAddressUpdateDelegate    = &sDeviceAddressUpdateDelegate;
     initParams.pairingDelegate                = &sPairingDelegate;
     initParams.operationalCredentialsDelegate = &sOperationalCredentialsIssuer;

diff --git a/src/controller/python/chip/internal/CommissionerImpl.cpp b/src/controller/python/chip/internal/CommissionerImpl.cpp
@@ -79,6 +79,7 @@ class ScriptDevicePairingDelegate final : public chip::Controller::DevicePairing
 };
 
 ServerStorageDelegate gServerStorage;
+chip::SimpleFabricStorage gFabricStorage;
 ScriptDevicePairingDelegate gPairingDelegate;
 chip::Controller::ExampleOperationalCredentialsIssuer gOperationalCredentialsIssuer;
 
@@ -102,18 +103,22 @@ extern "C" chip::Controller::DeviceCommissioner * pychip_internal_Commissioner_N
         // already assumed initialized
         chip::Controller::SetupParams commissionerParams;
         chip::Controller::FactoryInitParams factoryParams;
-
-        commissionerParams.pairingDelegate = &gPairingDelegate;
-        factoryParams.storageDelegate      = &gServerStorage;
-
         chip::Platform::ScopedMemoryBuffer<uint8_t> noc;
         chip::Platform::ScopedMemoryBuffer<uint8_t> icac;
         chip::Platform::ScopedMemoryBuffer<uint8_t> rcac;
+        chip::Crypto::P256Keypair ephemeralKey;
+
+        err = gFabricStorage.Initialize(&gServerStorage);
+        SuccessOrExit(err);
+
+        factoryParams.fabricStorage = &gFabricStorage;
+
+        commissionerParams.pairingDelegate = &gPairingDelegate;
+        commissionerParams.storageDelegate = &gServerStorage;
 
         // Initialize device attestation verifier
         chip::Credentials::SetDeviceAttestationVerifier(chip::Credentials::Examples::GetExampleDACVerifier());
 
-        chip::Crypto::P256Keypair ephemeralKey;
         err = ephemeralKey.Initialize();
         SuccessOrExit(err);
 

diff --git a/src/darwin/Framework/CHIP/CHIPDeviceController.mm b/src/darwin/Framework/CHIP/CHIPDeviceController.mm
@@ -60,6 +60,7 @@ @interface CHIPDeviceController ()
 @property (readonly) chip::Controller::DeviceCommissioner * cppCommissioner;
 @property (readonly) CHIPDevicePairingDelegateBridge * pairingDelegateBridge;
 @property (readonly) CHIPPersistentStorageDelegateBridge * persistentStorageDelegateBridge;
+@property (readonly) chip::FabricStorage * fabricStorage;
 @property (readonly) CHIPOperationalCredentialsDelegate * operationalCredentialsDelegate;
 @property (readonly) CHIPP256KeypairBridge keypairBridge;
 @property (readonly) chip::NodeId localDeviceId;
@@ -153,7 +154,11 @@ - (BOOL)startup:(_Nullable id<CHIPPersistentStorageDelegate>)storageDelegate
         CHIP_ERROR errorCode = CHIP_ERROR_INCORRECT_STATE;
 
         _persistentStorageDelegateBridge->setFrameworkDelegate(storageDelegate);
-
+        // TODO Expose FabricStorage to CHIPFramework consumers.
+        _fabricStorage = new chip::SimpleFabricStorage(_persistentStorageDelegateBridge);
+        if ([self checkForStartError:(_fabricStorage != nullptr) logMsg:kErrorMemoryInit]) {
+            return;
+        }
         // create a CHIPP256KeypairBridge here and pass it to the operationalCredentialsDelegate
         std::unique_ptr<chip::Crypto::CHIPP256KeypairNativeBridge> nativeBridge;
         if (nocSigner != nil) {
@@ -183,7 +188,8 @@ - (BOOL)startup:(_Nullable id<CHIPPersistentStorageDelegate>)storageDelegate
         // Initialize device attestation verifier
         chip::Credentials::SetDeviceAttestationVerifier(chip::Credentials::Examples::GetExampleDACVerifier());
 
-        params.storageDelegate = _persistentStorageDelegateBridge;
+        params.fabricStorage = _fabricStorage;
+        commissionerParams.storageDelegate = _persistentStorageDelegateBridge;
         commissionerParams.deviceAddressUpdateDelegate = _pairingDelegateBridge;
         commissionerParams.pairingDelegate = _pairingDelegateBridge;
 
@@ -490,6 +496,11 @@ - (BOOL)checkForStartError:(BOOL)condition logMsg:(NSString *)logMsg
         _cppCommissioner = NULL;
     }
 
+    if (_fabricStorage) {
+        delete _fabricStorage;
+        _fabricStorage = nullptr;
+    }
+
     return YES;
 }
 
@@ -507,4 +518,12 @@ - (BOOL)checkForError:(CHIP_ERROR)errorCode logMsg:(NSString *)logMsg error:(NSE
     return YES;
 }
 
+- (void)dealloc
+{
+    if (_fabricStorage) {
+        delete _fabricStorage;
+        _fabricStorage = nullptr;
+    }
+}
+
 @end
diff --git a/src/protocols/secure_channel/tests/TestCASESession.cpp b/src/protocols/secure_channel/tests/TestCASESession.cpp
@@ -251,7 +251,7 @@ void CASE_SecurePairingHandshakeTest(nlTestSuite * inSuite, void * inContext)
     CASE_SecurePairingHandshakeTestCommon(inSuite, inContext, pairingCommissioner, delegateCommissioner);
 }
 
-class TestPersistentStorageDelegate : public PersistentStorageDelegate
+class TestPersistentStorageDelegate : public PersistentStorageDelegate, public FabricStorage
 {
 public:
     TestPersistentStorageDelegate()
@@ -319,6 +319,18 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
 
     CHIP_ERROR SyncDeleteKeyValue(const char * key) override { return CHIP_NO_ERROR; }
 
+    CHIP_ERROR SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override
+    {
+        return SyncSetKeyValue(key, buffer, size);
+    };
+
+    CHIP_ERROR SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override
+    {
+        return SyncGetKeyValue(key, buffer, size);
+    };
+
+    CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) override { return SyncDeleteKeyValue(key); };
+
 private:
     char * keys[16];
     void * values[16];