From ffc22200a7716b69f3ab620310b5d976b93691ff Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 27 Jul 2023 17:47:31 -0400 Subject: [PATCH 01/11] Modify Matter.framework to allow per-controller storage. This also allows starting multiple controllers with different node IDs on a single fabric. Public API changes: * It's now possible to initialize MTRDeviceControllerFactoryParams without storage. When a factory is then started with those params, it will expect storage to be provided for every controller that is created. * Controllers to be created in the new setup use MTRDeviceControllerStartupParameters, not MTRDeviceControllerStartupParams. * When starting a controller, API clients provide a UUID for that controller (which is then exposed on the MTRDeviceController) and a storage delegate. * For now, the only supported controller startup mode is for the client to provide the full certificate chain, operational key and vendor ID, via MTRDeviceControllerExternalCertificateStartupParameters. For controllers that will commission devices, that means also providing an MTROperationalCertificateIssuer. * The new "create a controller" API is called createController. * The new MTRDeviceControllerStorageDelegate API provides some context for the key/value pairs in terms of whether they need to be stored in encrypted storage or not, and whether they can be shared across multiple devices and under what conditions. Implementation notes: * MTRDemuxingStorage handles directing storage requests to the right per-controller storage object. * MTRDeviceControllerDataStore wraps the raw storage delegate and provides a semantic API on top of its key/value storage for the storage operations we actually want to perform. * MTRSessionResumptionStorageBridge implements session resumption storage, acting as an adapter between the Matter session resumption storage API and MTRDeviceControllerDataStore. In particular, it happens locating the right controller(s) to talk to and so on. This avoids dealing with the default Matter implementation's use of non-fabric-index-scoped keys for storing session resumption information. Fixes https://github.com/project-chip/connectedhomeip/issues/27394 --- .../Framework/CHIP/MTRDemuxingStorage.h | 109 ++ .../Framework/CHIP/MTRDemuxingStorage.mm | 388 ++++++ .../Framework/CHIP/MTRDeviceController.h | 5 + .../Framework/CHIP/MTRDeviceController.mm | 21 +- .../CHIP/MTRDeviceControllerDataStore.h | 64 + .../CHIP/MTRDeviceControllerDataStore.mm | 243 ++++ .../CHIP/MTRDeviceControllerFactory.h | 40 +- .../CHIP/MTRDeviceControllerFactory.mm | 346 ++++- .../MTRDeviceControllerFactory_Internal.h | 9 + .../MTRDeviceControllerStartupParameters.h | 84 ++ .../CHIP/MTRDeviceControllerStartupParams.mm | 242 +++- ...TRDeviceControllerStartupParams_Internal.h | 59 +- .../CHIP/MTRDeviceControllerStorageDelegate.h | 117 ++ .../CHIP/MTRDeviceController_Internal.h | 14 +- .../CHIP/MTRSessionResumptionStorageBridge.h | 59 + .../CHIP/MTRSessionResumptionStorageBridge.mm | 149 +++ src/darwin/Framework/CHIP/Matter.h | 2 + .../Framework/CHIPTests/MTRControllerTests.m | 7 - .../CHIPTests/MTRPerControllerStorageTests.m | 1121 +++++++++++++++++ .../Matter.xcodeproj/project.pbxproj | 36 + 20 files changed, 3048 insertions(+), 67 deletions(-) create mode 100644 src/darwin/Framework/CHIP/MTRDemuxingStorage.h create mode 100644 src/darwin/Framework/CHIP/MTRDemuxingStorage.mm create mode 100644 src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h create mode 100644 src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm create mode 100644 src/darwin/Framework/CHIP/MTRDeviceControllerStartupParameters.h create mode 100644 src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h create mode 100644 src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.h create mode 100644 src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.mm create mode 100644 src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m diff --git a/src/darwin/Framework/CHIP/MTRDemuxingStorage.h b/src/darwin/Framework/CHIP/MTRDemuxingStorage.h new file mode 100644 index 00000000000000..bbdfa3bc5e0024 --- /dev/null +++ b/src/darwin/Framework/CHIP/MTRDemuxingStorage.h @@ -0,0 +1,109 @@ +/** + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import +#import + +#include +#include + +NS_ASSUME_NONNULL_BEGIN + +/** + * A PersistentStorageDelegate implementation that does the following: + * + * 1) Ensures that any "global" storage keys are stored in RAM as needed so that + * the Matter stack has access to them. + * 2) Hands off fabric-index-specific keys to the controller that corresponds to + * that fabric index, if any. + */ +class MTRDemuxingStorage : public chip::PersistentStorageDelegate { +public: + MTRDemuxingStorage(MTRDeviceControllerFactory * factory); + ~MTRDemuxingStorage() {} + + // PersistentStorageDelegate API. + CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override; + + CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) override; + + CHIP_ERROR SyncDeleteKeyValue(const char * key) override; + +private: + static bool IsGlobalKey(NSString * key); + + /** + * Checks for a key that is scoped to a specific fabric index. + */ + static bool IsIndexSpecificKey(NSString * key); + + /** + * Extracts the fabric index from an index-specific key. Fails if the key + * is not index-specific or if a numeric FabricIndex could not be extracted + * from it. + */ + static CHIP_ERROR ExtractIndexFromKey(NSString * key, chip::FabricIndex * index); + + /** + * Extracts the "index-specific" part of an index-specific key (i.e. the + * part after "f/index/"). + */ + static CHIP_ERROR ExtractIndexSpecificKey(NSString * key, NSString * _Nullable __autoreleasing * _Nonnull extractedKey); + + /** + * Methods for reading/writing/deleting things. The index-specific ones + * will have the "f/index/" bit already stripped of from the front of the key. + */ + NSData * _Nullable GetGlobalValue(NSString * key); + NSData * _Nullable GetIndexSpecificValue(chip::FabricIndex index, NSString * key); + + CHIP_ERROR SetGlobalValue(NSString * key, NSData * data); + CHIP_ERROR SetIndexSpecificValue(chip::FabricIndex index, NSString * key, NSData * data); + + CHIP_ERROR DeleteGlobalValue(NSString * key); + CHIP_ERROR DeleteIndexSpecificValue(chip::FabricIndex index, NSString * key); + + /** + * Method to test whether a global key should be stored in memory only, as + * opposed to being passed on to the actual storage related to controllers. + */ + static bool IsMemoryOnlyGlobalKey(NSString * key); + + /** + * Method to test whether an index-specific key should be stored in memory only, as + * opposed to being passed on to the actual storage related to controllers. + * The key string will ahve the "f/index/" bit already stripped off the + * front of the key. + */ + static bool IsMemoryOnlyIndexSpecificKey(NSString * key); + + /** + * Methods for modifying our in-memory store for fully qualified keys. + */ + NSData * _Nullable GetInMemoryValue(NSString * key); + CHIP_ERROR SetInMemoryValue(NSString * key, NSData * data); + CHIP_ERROR DeleteInMemoryValue(NSString * key); + + /** + * Method to convert an index-specific key into a fully qualified key. + */ + static NSString * FullyQualifiedKey(chip::FabricIndex index, NSString * key); + + MTRDeviceControllerFactory * mFactory; + NSMutableDictionary * mInMemoryStore; +}; + +NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm b/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm new file mode 100644 index 00000000000000..070391e83b5934 --- /dev/null +++ b/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm @@ -0,0 +1,388 @@ +/** + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import "MTRDemuxingStorage.h" + +#import "MTRDeviceControllerFactory_Internal.h" +#import "MTRDeviceController_Internal.h" +#import "MTRLogging_Internal.h" + +#include +#include +#include + +using namespace chip; + +MTRDemuxingStorage::MTRDemuxingStorage(MTRDeviceControllerFactory * factory) + : mFactory(factory) +{ + mInMemoryStore = [[NSMutableDictionary alloc] init]; +} + +CHIP_ERROR MTRDemuxingStorage::SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) +{ + assertChipStackLockedByCurrentThread(); + if (buffer == nullptr && size != 0) { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + NSString * keyString = [NSString stringWithUTF8String:key]; + +#if LOG_DEBUG_PERSISTENT_STORAGE_DELEGATE + MTR_LOG_DEBUG("MTRDemuxingStorage Sync Get Value for Key: %@", keyString); +#endif + + NSData * value; + if (IsGlobalKey(keyString)) { + value = GetGlobalValue(keyString); + } else if (IsIndexSpecificKey(keyString)) { + FabricIndex index; + ReturnErrorOnFailure(ExtractIndexFromKey(keyString, &index)); + ReturnErrorOnFailure(ExtractIndexSpecificKey(keyString, &keyString)); + value = GetIndexSpecificValue(index, keyString); + } else { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + if (value == nil) { + return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; + } + + if (value.length > UINT16_MAX) { + return CHIP_ERROR_PERSISTED_STORAGE_FAILED; + } + + uint16_t valueSize = static_cast(value.length); + if (valueSize > size) { + return CHIP_ERROR_BUFFER_TOO_SMALL; + } + + size = valueSize; + + if (size != 0) { + // buffer is known to be non-null here. + memcpy(buffer, value.bytes, size); + } + + return CHIP_NO_ERROR; +} + +CHIP_ERROR MTRDemuxingStorage::SyncSetKeyValue(const char * key, const void * value, uint16_t size) +{ + if (value == nullptr && size != 0) { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + NSString * keyString = [NSString stringWithUTF8String:key]; + NSData * valueData = (value == nullptr) ? [NSData data] : [NSData dataWithBytes:value length:size]; + +#if LOG_DEBUG_PERSISTENT_STORAGE_DELEGATE + MTR_LOG_DEBUG("MTRDemuxingStorage Set Key %@", keyString); +#endif + + if (IsGlobalKey(keyString)) { + return SetGlobalValue(keyString, valueData); + } + + if (IsIndexSpecificKey(keyString)) { + FabricIndex index; + ReturnErrorOnFailure(ExtractIndexFromKey(keyString, &index)); + ReturnErrorOnFailure(ExtractIndexSpecificKey(keyString, &keyString)); + return SetIndexSpecificValue(index, keyString, valueData); + } + + return CHIP_ERROR_INVALID_ARGUMENT; +} + +CHIP_ERROR MTRDemuxingStorage::SyncDeleteKeyValue(const char * key) +{ + NSString * keyString = [NSString stringWithUTF8String:key]; + +#if LOG_DEBUG_PERSISTENT_STORAGE_DELEGATE + MTR_LOG_DEBUG("MTRDemuxingStorage Delete Key: %@", keyString); +#endif + + if (IsGlobalKey(keyString)) { + return DeleteGlobalValue(keyString); + } + + if (IsIndexSpecificKey(keyString)) { + FabricIndex index; + ReturnErrorOnFailure(ExtractIndexFromKey(keyString, &index)); + ReturnErrorOnFailure(ExtractIndexSpecificKey(keyString, &keyString)); + return DeleteIndexSpecificValue(index, keyString); + } + + return CHIP_ERROR_INVALID_ARGUMENT; +} + +bool MTRDemuxingStorage::IsGlobalKey(NSString * key) { return [key hasPrefix:@"g/"]; } + +bool MTRDemuxingStorage::IsIndexSpecificKey(NSString * key) { return [key hasPrefix:@"f/"]; } + +CHIP_ERROR MTRDemuxingStorage::ExtractIndexFromKey(NSString * key, FabricIndex * index) +{ + if (!IsIndexSpecificKey(key)) { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + auto * components = [key componentsSeparatedByString:@"/"]; + if (components.count < 3) { + // Unexpected "f/something" without any actual data. + return CHIP_ERROR_INVALID_ARGUMENT; + } + + auto * indexString = components[1]; + auto * scanner = [NSScanner scannerWithString:indexString]; + + auto * charset = [NSCharacterSet characterSetWithCharactersInString:@"0123456789abcdefABCDEF"]; + charset = [charset invertedSet]; + + if ([scanner scanCharactersFromSet:charset intoString:nil] == YES) { + // Leading non-hex chars. + return CHIP_ERROR_INVALID_ARGUMENT; + } + + unsigned int value; + if ([scanner scanHexInt:&value] == NO) { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + if (scanner.atEnd == NO) { + // Trailing garbage chars. + return CHIP_ERROR_INVALID_ARGUMENT; + } + + if (!CanCastTo(value)) { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + *index = static_cast(value); + return CHIP_NO_ERROR; +} + +CHIP_ERROR MTRDemuxingStorage::ExtractIndexSpecificKey(NSString * key, NSString * __autoreleasing * extractedKey) +{ + if (!IsIndexSpecificKey(key)) { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + auto * components = [key componentsSeparatedByString:@"/"]; + if (components.count < 3) { + // Unexpected "f/something" without any actual data. + return CHIP_ERROR_INVALID_ARGUMENT; + } + + components = [components subarrayWithRange:NSMakeRange(2, components.count - 2)]; + *extractedKey = [components componentsJoinedByString:@"/"]; + return CHIP_NO_ERROR; +} + +NSData * _Nullable MTRDemuxingStorage::GetGlobalValue(NSString * key) +{ + if (IsMemoryOnlyGlobalKey(key)) { + return GetInMemoryValue(key); + } + + MTR_LOG_ERROR("MTRDemuxingStorage reading unknown global key: %@", key); + + return nil; +} + +NSData * _Nullable MTRDemuxingStorage::GetIndexSpecificValue(chip::FabricIndex index, NSString * key) +{ + if (IsMemoryOnlyIndexSpecificKey(key)) { + return GetInMemoryValue(FullyQualifiedKey(index, key)); + } + + return nil; +} + +CHIP_ERROR MTRDemuxingStorage::SetGlobalValue(NSString * key, NSData * data) +{ + if (IsMemoryOnlyGlobalKey(key)) { + // Fabric index list only needs to be stored in-memory, not persisted, + // because we do not tie controllers to specific fabric indices. + return SetInMemoryValue(key, data); + } + + MTR_LOG_ERROR("MTRDemuxingStorage setting unknown global key: %@", key); + + return CHIP_ERROR_PERSISTED_STORAGE_FAILED; +} + +CHIP_ERROR MTRDemuxingStorage::SetIndexSpecificValue(chip::FabricIndex index, NSString * key, NSData * data) +{ + if ([key isEqualToString:@"n"]) { + auto * controller = [mFactory runningControllerForFabricIndex:index]; + if (controller == nil) { + return CHIP_ERROR_PERSISTED_STORAGE_FAILED; + } + + ReturnErrorOnFailure([controller.controllerDataStore storeLastUsedNOC:data]); + } + + if (IsMemoryOnlyIndexSpecificKey(key)) { + return SetInMemoryValue(FullyQualifiedKey(index, key), data); + } + + return CHIP_ERROR_PERSISTED_STORAGE_FAILED; +} + +CHIP_ERROR MTRDemuxingStorage::DeleteGlobalValue(NSString * key) +{ + if (IsMemoryOnlyGlobalKey(key)) { + return DeleteInMemoryValue(key); + } + + MTR_LOG_ERROR("MTRDemuxingStorage deleting unknown global key: %@", key); + + return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; +} + +CHIP_ERROR MTRDemuxingStorage::DeleteIndexSpecificValue(chip::FabricIndex index, NSString * key) +{ + if (IsMemoryOnlyIndexSpecificKey(key)) { + return DeleteInMemoryValue(FullyQualifiedKey(index, key)); + } + + return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; +} + +bool MTRDemuxingStorage::IsMemoryOnlyGlobalKey(NSString * key) +{ + if ([key isEqualToString:@"g/fidx"]) { + // Fabric index list only needs to be stored in-memory, not persisted, + // because we do not tie controllers to specific fabric indices. + return true; + } + + if ([key isEqualToString:@"g/fs/c"] || [key isEqualToString:@"g/fs/n"]) { + // Just store the fail-safe markers in memory as well. We could + // plausibly not store them at all, since we never actually need to + // clean up anything by fabric index, but this is safer in case + // consumers try to read back right after writing. + return true; + } + + if ([key isEqualToString:@"g/lkgt"]) { + // Store Last Known Good Time in memory only. We never need this in + // general, because we can always provide wall-clock time. + return true; + } + + // We do not expect to see the "g/sri" or "g/s/*" keys for session + // resumption, because we implement SessionResumptionStorage ourselves. + + // For now, put group global counters in memory. + // TODO: we should inject a group counter manager that makes these counters + // per-controller, not globally handled via storage. + // See https://github.com/project-chip/connectedhomeip/issues/28510 + if ([key isEqualToString:@"g/gdc"] || [key isEqualToString:@"g/gcc"]) { + return true; + } + + // We do not expect to see "g/userlbl/*" User Label keys. + + // We do not expect to see the "g/gfl" key for endpoint-to-group + // associations. + + // We do not expect to see the "g/a/*" keys for attribute values. + + // We do not expect to see the "g/bt" and "g/bt/*" keys for the binding + // table. + + // We do not expect to see the "g/o/*" OTA Requestor keys. + + // We do not expect to see the "g/im/ec" event number counter key. + + // We do not expect to see the "g/su/*" and "g/sum" keys for server-side + // subscription resumption storage. + + // We do not expect to see the "g/scc/*" scenes keys. + + // We do not expect to see the "g/ts/tts", "g/ts/dntp", "g/ts/tz", + // "g/ts/dsto" Time Synchronization keys. + + return false; +} + +bool MTRDemuxingStorage::IsMemoryOnlyIndexSpecificKey(NSString * key) +{ + // Store all the fabric table bits in memory only. This is done because the + // fabric table expects none of these things to be stored in the case of a + // "new fabric addition", which is what we always do when using + // per-controller storage. + // + // TODO: Figure out which, if any, of these things we should also store for + // later recall when starting a controller with storage we have used before. + // + // For future reference: + // + // n == NOC + // i == ICAC + // r == RCAC + // m == Fabric metadata (TLV containing the vendor ID) + // o == operational key, only written if internally generated. + if ([key isEqualToString:@"n"] || [key isEqualToString:@"i"] || [key isEqualToString:@"r"] || [key isEqualToString:@"m"] || + [key isEqualToString:@"o"]) { + return true; + } + + // We do not expect to see the "s/*" keys for session resumption, because we + // implement SessionResumptionStorage ourselves. + + // We do not expect to see the "ac/*" keys for ACL entries. + + // We do not expect to see the "g" or "g/*" keys for which endpoints are in which + // group. + + // For now, just store group keysets and group keys in memory. + // TODO: We want to start persisting these, per-controller, if we're going + // to support group keys. Or inject a GroupDataProvider of our own instead + // of using Credentials::GroupDataProviderImp and then + // not be tied to whatever storage format that uses. + // https://github.com/project-chip/connectedhomeip/issues/28511 + if ([key hasPrefix:@"gk/"] || [key hasPrefix:@"k/"]) { + return true; + } + + // We do not expect to see the "icd/*" keys for the ICD Management table. + + // We do not expect to see the "e/*" scenes keys. + + return false; +} + +NSData * _Nullable MTRDemuxingStorage::GetInMemoryValue(NSString * key) { return mInMemoryStore[key]; } + +CHIP_ERROR MTRDemuxingStorage::SetInMemoryValue(NSString * key, NSData * data) +{ + mInMemoryStore[key] = data; + return CHIP_NO_ERROR; +} + +CHIP_ERROR MTRDemuxingStorage::DeleteInMemoryValue(NSString * key) +{ + BOOL present = (mInMemoryStore[key] != nil); + [mInMemoryStore removeObjectForKey:key]; + return present ? CHIP_NO_ERROR : CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; +} + +NSString * MTRDemuxingStorage::FullyQualifiedKey(chip::FabricIndex index, NSString * key) +{ + return [NSString stringWithFormat:@"f/%x/%s", index, key.UTF8String]; +} diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.h b/src/darwin/Framework/CHIP/MTRDeviceController.h index fce1a88002aaf8..293405bc6699eb 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController.h @@ -47,6 +47,11 @@ typedef void (^MTRDeviceConnectionCallback)(MTRBaseDevice * _Nullable device, NS */ @property (readonly, nonatomic, getter=isRunning) BOOL running; +/** + * The ID assigned to this controller at creation time. + */ +@property (readonly, nonatomic) NSUUID * UUID MTR_NEWLY_AVAILABLE; + /** * Return the Node ID assigned to the controller. Will return nil if the * controller is not running (and hence does not know its node id). diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 21ec9e56f737e9..9fe49545233f3a 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -114,9 +114,22 @@ @interface MTRDeviceController () { @implementation MTRDeviceController -- (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory queue:(dispatch_queue_t)queue +- (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory + queue:(dispatch_queue_t)queue + storageDelegate:(id _Nullable)storageDelegate + UUID:(NSUUID *)UUID { if (self = [super init]) { + // Make sure our storage is all set up to work as early as possible, + // before we start doing anything else with the controller. + _UUID = UUID; + if (storageDelegate != nil) { + _controllerDataStore = [[MTRDeviceControllerDataStore alloc] initWithController:self storageDelegate:storageDelegate]; + if (_controllerDataStore == nil) { + return nil; + } + } + _chipWorkQueue = queue; _factory = factory; _deviceMapLock = OS_UNFAIR_LOCK_INIT; @@ -191,8 +204,11 @@ - (void)shutDownCppController // _cppCommissioner, so we're not in a state where we claim to be // running but are actually partially shut down. _cppCommissioner = nullptr; - _storedFabricIndex = chip::kUndefinedFabricIndex; commissionerToShutDown->Shutdown(); + // Don't clear out our fabric index association until controller + // shutdown completes, in case it wants to write to storage as it + // shuts down. + _storedFabricIndex = chip::kUndefinedFabricIndex; delete commissionerToShutDown; if (_operationalCredentialsDelegate != nil) { _operationalCredentialsDelegate->SetDeviceCommissioner(nullptr); @@ -365,6 +381,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams // bring-up. commissionerParams.removeFromFabricTableOnShutdown = false; commissionerParams.deviceAttestationVerifier = _factory.deviceAttestationVerifier; + commissionerParams.permitMultiControllerFabrics = startupParams.allowMultipleControllersPerFabric; auto & factory = chip::Controller::DeviceControllerFactory::GetInstance(); diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h new file mode 100644 index 00000000000000..c901476df20aeb --- /dev/null +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h @@ -0,0 +1,64 @@ +/** + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import +#import +#import +#import + +#include + +NS_ASSUME_NONNULL_BEGIN + +/** + * Interface that represents a single CASE session resumption entry. + */ +MTR_HIDDEN +@interface MTRCASESessionResumptionInfo : NSObject +@property (nonatomic) NSNumber * nodeID; +@property (nonatomic) NSData * resumptionID; +@property (nonatomic) NSData * sharedSecret; +@property (nonatomic) NSSet * caseAuthenticatedTags; +@end + +/** + * Interface that wraps a type-safe API around + * MTRDeviceControllerStorageDelegate. + */ +MTR_HIDDEN +@interface MTRDeviceControllerDataStore : NSObject + +- (instancetype)initWithController:(MTRDeviceController *)controller + storageDelegate:(id)storageDelegate; + +/** + * Resumption info APIs. + */ +- (nullable MTRCASESessionResumptionInfo *)findResumptionInfoByNodeID:(NSNumber *)nodeID; +- (nullable MTRCASESessionResumptionInfo *)findResumptionInfoByResumptionID:(NSData *)resumptionID; +- (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo; +- (void)clearAllResumptionInfo; + +/** + * APIs for storing/retrieving last-used fabric state. These are used at + * startup to make sure the new state and the old state are merged properly. + */ +- (CHIP_ERROR)storeLastUsedNOC:(MTRCertificateTLVBytes)noc; +- (MTRCertificateTLVBytes _Nullable)fetchLastUsedNOC; + +@end + +NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm new file mode 100644 index 00000000000000..30b12f3ca9742b --- /dev/null +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -0,0 +1,243 @@ +/** + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "MTRDeviceControllerDataStore.h" +#import "MTRLogging_Internal.h" + +// TODO: FIXME: Figure out whether these are good key strings. +static NSString * sResumptionNodeListKey = @"caseResumptionNodeList"; +static NSString * sLastUsedNOCKey = @"sLastUsedControllerNOC"; + +static NSString * ResumptionByNodeIDKey(NSNumber * nodeID) +{ + return [NSString stringWithFormat:@"caseResumptionByNodeID/%llx", nodeID.unsignedLongLongValue]; +} + +static NSString * ResumptionByResumptionIDKey(NSData * resumptionID) +{ + return + [NSString stringWithFormat:@"caseResumptionByResumptionID/%s", [resumptionID base64EncodedStringWithOptions:0].UTF8String]; +} + +static dispatch_queue_t sWorkQueue; + +@implementation MTRDeviceControllerDataStore { + id _delegate; + MTRDeviceController * _controller; + // Array of nodes with resumption info, oldest-stored first. + NSMutableArray * _nodesWithResumptionInfo; +} + ++ (void)initialize +{ + sWorkQueue = dispatch_queue_create( + "org.csa-iot.matter.framework.controller-storage.workqueue", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL); +} + +- (instancetype)initWithController:(MTRDeviceController *)controller + storageDelegate:(id)storageDelegate +{ + if (!(self = [super init])) { + return nil; + } + + _controller = controller; + _delegate = storageDelegate; + + __block id resumptionNodeList; + dispatch_sync(sWorkQueue, ^{ + resumptionNodeList = [_delegate controller:_controller + valueForKey:sResumptionNodeListKey + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + }); + if (resumptionNodeList != nil) { + if (![resumptionNodeList isKindOfClass:[NSMutableArray class]]) { + MTR_LOG_ERROR("List of CASE resumption node IDs is not an array"); + return nil; + } + _nodesWithResumptionInfo = resumptionNodeList; + } else { + _nodesWithResumptionInfo = [[NSMutableArray alloc] init]; + } + return self; +} + +- (nullable MTRCASESessionResumptionInfo *)findResumptionInfoByNodeID:(NSNumber *)nodeID +{ + return [self _findResumptionInfoWithKey:ResumptionByNodeIDKey(nodeID)]; +} + +- (nullable MTRCASESessionResumptionInfo *)findResumptionInfoByResumptionID:(NSData *)resumptionID +{ + return [self _findResumptionInfoWithKey:ResumptionByResumptionIDKey(resumptionID)]; +} + +- (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo +{ + auto * oldInfo = [self findResumptionInfoByNodeID:resumptionInfo.nodeID]; + dispatch_sync(sWorkQueue, ^{ + if (oldInfo != nil) { + // Remove old resumption id key. No need to do that for the + // node id, because we are about to overwrite it. + [_delegate controller:_controller + removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_nodesWithResumptionInfo removeObject:resumptionInfo.nodeID]; + } + + [_delegate controller:_controller + storeValue:resumptionInfo + forKey:ResumptionByNodeIDKey(resumptionInfo.nodeID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_delegate controller:_controller + storeValue:resumptionInfo + forKey:ResumptionByResumptionIDKey(resumptionInfo.resumptionID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + + // Update our resumption info node list. + [_nodesWithResumptionInfo addObject:resumptionInfo.nodeID]; + [_delegate controller:_controller + storeValue:_nodesWithResumptionInfo + forKey:sResumptionNodeListKey + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + }); +} + +- (void)clearAllResumptionInfo +{ + // Can we do less dispatch? We would need to have a version of + // _findResumptionInfoWithKey that assumes we are already on the right queue. + for (NSNumber * nodeID in _nodesWithResumptionInfo) { + auto * oldInfo = [self findResumptionInfoByNodeID:nodeID]; + if (oldInfo != nil) { + dispatch_sync(sWorkQueue, ^{ + [_delegate controller:_controller + removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_delegate controller:_controller + removeValueForKey:ResumptionByNodeIDKey(oldInfo.nodeID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + }); + } + } + + [_nodesWithResumptionInfo removeAllObjects]; +} + +- (CHIP_ERROR)storeLastUsedNOC:(MTRCertificateTLVBytes)noc +{ + __block BOOL ok; + dispatch_sync(sWorkQueue, ^{ + ok = [_delegate controller:_controller + storeValue:noc + forKey:sLastUsedNOCKey + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityRequired]; + }); + return ok ? CHIP_NO_ERROR : CHIP_ERROR_PERSISTED_STORAGE_FAILED; +} + +- (MTRCertificateTLVBytes _Nullable)fetchLastUsedNOC +{ + __block id data; + dispatch_sync(sWorkQueue, ^{ + data = [_delegate controller:_controller + valueForKey:sLastUsedNOCKey + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityRequired]; + }); + + if (data == nil) { + return nil; + } + + if (![data isKindOfClass:[NSData class]]) { + return nil; + } + + return data; +} + +- (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(NSString *)key +{ + __block id resumptionInfo; + dispatch_sync(sWorkQueue, ^{ + resumptionInfo = [_delegate controller:_controller + valueForKey:key + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + }); + + if (resumptionInfo == nil) { + return nil; + } + + if (![resumptionInfo isKindOfClass:[MTRCASESessionResumptionInfo class]]) { + return nil; + } + + return resumptionInfo; +} + +@end + +@implementation MTRCASESessionResumptionInfo + +#pragma mark - NSSecureCoding + +static NSString * const sNodeIDKey = @"nodeID"; +static NSString * const sResumptionIDKey = @"resumptionID"; +static NSString * const sSharedSecretKey = @"sharedSecret"; +static NSString * const sCATsKey = @"CATs"; + ++ (BOOL)supportsSecureCoding +{ + return YES; +} + +- (instancetype _Nullable)initWithCoder:(NSCoder *)decoder +{ + _nodeID = [decoder decodeObjectOfClass:[NSNumber class] forKey:sNodeIDKey]; + _resumptionID = [decoder decodeObjectOfClass:[NSData class] forKey:sResumptionIDKey]; + _sharedSecret = [decoder decodeObjectOfClass:[NSData class] forKey:sSharedSecretKey]; + _caseAuthenticatedTags = [decoder decodeObjectOfClass:[NSSet class] forKey:sCATsKey]; + return self; +} + +- (void)encodeWithCoder:(NSCoder *)coder +{ + [coder encodeObject:self.nodeID forKey:sNodeIDKey]; + [coder encodeObject:self.resumptionID forKey:sResumptionIDKey]; + [coder encodeObject:self.sharedSecret forKey:sSharedSecretKey]; + [coder encodeObject:self.caseAuthenticatedTags forKey:sCATsKey]; +} + +@end + +NSSet * MTRDeviceControllerStorageClasses() +{ + static NSSet * const sStorageClasses = [NSSet setWithArray:@[ + [NSNumber class], [NSData class], [NSSet class], [MTRCASESessionResumptionInfo class], [NSMutableArray class] + ]]; + return sStorageClasses; +} diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h index 0423aeae585c70..8866b4ae55d532 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h @@ -22,6 +22,7 @@ #import #import +#import NS_ASSUME_NONNULL_BEGIN @@ -37,9 +38,11 @@ NS_ASSUME_NONNULL_BEGIN API_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)) @interface MTRDeviceControllerFactoryParams : NSObject /* - * Storage delegate must be provided for correct functioning of Matter - * controllers. It is used to store persistent information for the fabrics the - * controllers ends up interacting with. + * Storage used to store persistent information for the fabrics the + * controllers ends up interacting with. This is only used if "initWithStorage" + * is used to initialize the MTRDeviceControllerFactoryParams. If init is used, + * called. If "init" is used, this property will contain a dummy storage that + * will not be used for anything. */ @property (nonatomic, strong, readonly) id storage; @@ -83,8 +86,19 @@ API_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)) */ @property (nonatomic, assign) BOOL shouldStartServer; -- (instancetype)init NS_UNAVAILABLE; +/* + * Initialize the device controller factory with storage. In this mode, the + * storage will be used to store various information needed by the Matter + * framework. + */ - (instancetype)initWithStorage:(id)storage; + +/* + * Initialize the device controller factory without storage. In this mode, + * device controllers will need to have per-controller storage provided to allow + * storing controller-specific information. + */ +- (instancetype)init MTR_NEWLY_AVAILABLE; @end API_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)) @@ -145,6 +159,9 @@ API_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)) * * The fabric is identified by the root public key and fabric id in * the startupParams. + * + * This method can only be used if the factory was initialized with storage. + * When using per-controller storage, use createController. */ - (MTRDeviceController * _Nullable)createControllerOnExistingFabric:(MTRDeviceControllerStartupParams *)startupParams error:(NSError * __autoreleasing *)error; @@ -156,10 +173,25 @@ API_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)) * * The fabric is identified by the root public key and fabric id in * the startupParams. + * + * This method can only be used if the factory was initialized with storage. + * When using per-controller storage, use createController. */ - (MTRDeviceController * _Nullable)createControllerOnNewFabric:(MTRDeviceControllerStartupParams *)startupParams error:(NSError * __autoreleasing *)error; +/** + * Create an MTRDeviceController. Returns nil on failure. + * + * This method will fail if there is already a controller running for the given + * node identity. + * + * This method will fail if the controller factory was not initialized in + * storage-per-controller mode. + */ +- (MTRDeviceController * _Nullable)createController:(MTRDeviceControllerStartupParameters *)startupParameters + error:(NSError * __autoreleasing *)error MTR_NEWLY_AVAILABLE; + @end MTR_DEPRECATED( diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index 64e4af1ba29fc9..fb9c4af822cf7e 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -20,6 +20,7 @@ #import "MTRAttestationTrustStoreBridge.h" #import "MTRCertificates.h" #import "MTRControllerAccessControl.h" +#import "MTRDemuxingStorage.h" #import "MTRDeviceController.h" #import "MTRDeviceControllerStartupParams.h" #import "MTRDeviceControllerStartupParams_Internal.h" @@ -32,6 +33,7 @@ #import "MTROperationalBrowser.h" #import "MTRP256KeypairBridge.h" #import "MTRPersistentStorageDelegateBridge.h" +#import "MTRSessionResumptionStorageBridge.h" #import "NSDataSpanConversion.h" #import @@ -55,6 +57,7 @@ using namespace chip::Controller; static NSString * const kErrorPersistentStorageInit = @"Init failure while creating a persistent storage delegate"; +static NSString * const kErrorSessionResumptionStorageInit = @"Init failure while creating a session resumption storage delegate"; static NSString * const kErrorAttestationTrustStoreInit = @"Init failure while creating the attestation trust store"; static NSString * const kErrorDACVerifierInit = @"Init failure while creating the device attestation verifier"; static NSString * const kErrorGroupProviderInit = @"Init failure while initializing group data provider"; @@ -90,21 +93,36 @@ @interface MTRDeviceControllerFactory () @property () chip::Credentials::DeviceAttestationVerifier * deviceAttestationVerifier; @property (readonly) BOOL advertiseOperational; @property (nonatomic, readonly) Credentials::IgnoreCertificateValidityPeriodPolicy * certificateValidityPolicy; -// Lock used to serialize access to the "controllers" array, since it needs to -// be touched from both whatever queue is starting controllers and from the -// Matter queue. The way this lock is used assumes that: +@property (readonly) MTRSessionResumptionStorageBridge * sessionResumptionStorage; +// Lock used to serialize access to the "controllers" array and the +// "_controllerBeingStarted" and "_controllerBeingShutDown" ivars, since those +// need to be touched from both whatever queue is starting controllers and from +// the Matter queue. The way this lock is used assumes that: +// +// 1) The only mutating accesses to the controllers array and the ivars happen +// when the current queue is not the Matter queue or when in a block that was +// sync-dispatched to the Matter queue. This is a good assumption, because +// the implementations of the functions that mutate these do sync dispatch to +// the Matter queue, which would deadlock if they were called when that queue +// was the current queue. // -// 1) The only mutating accesses to the controllers array happen when the -// current queue is not the Matter queue. This is a good assumption, because -// the implementation of the functions that mutate the array do sync dispatch -// to the Matter queue, which would deadlock if they were called when that -// queue was the current queue. // 2) It's our API consumer's responsibility to serialize access to us from // outside. // -// This means that we only take the lock around mutations of the array and -// accesses to the array that are from code running on the Matter queue. - +// These assumptions mean that if we are in a block that was sync-dispatched to +// the Matter queue, that block cannot race with either the MAtter queue nor the +// non-Matter queue. Similarly, if we are in a situation where the Matter queue +// has been shut down, any accesses to the variables cannot race anything else. +// +// This means that: +// +// A. In a sync-dispatched block, or if the Matter queue has been shut down, we +// do not need to lock and can do read or write access. +// B. Apart from item A, mutations of the array and ivars must happen outside the +// Matter queue and must lock. +// C. Apart from item A, accesses on the Matter queue must be reads only and +// must lock. +// D. Locking around reads not from the Matter queue is OK but not required. @property (nonatomic, readonly) os_unfair_lock controllersLock; - (BOOL)findMatchingFabric:(FabricTable &)fabricTable @@ -114,7 +132,34 @@ - (BOOL)findMatchingFabric:(FabricTable &)fabricTable - (MTRDeviceController * _Nullable)maybeInitializeOTAProvider:(MTRDeviceController * _Nonnull)controller; @end -@implementation MTRDeviceControllerFactory +@interface MTRDeviceControllerFactoryParams () + +// Flag to keep track of whether our .storage is real consumer-provided storage +// or just the fake thing we made up. +@property (nonatomic, assign) BOOL hasStorage; + +@end + +@implementation MTRDeviceControllerFactory { + // _usingPerControllerStorage is only written once, during controller + // factory start. After that it is only read, and can be read from + // arbitrary threads. + BOOL _usingPerControllerStorage; + + // See documentation for controllersLock above for the rules for accessing + // _controllerBeingStarted. + MTRDeviceController * _controllerBeingStarted; + + // See documentation for controllersLock above for the rules for access + // _controllerBeingShutDown. + MTRDeviceController * _controllerBeingShutDown; + + // Next available fabric index. Only valid when _controllerBeingStarted is + // non-nil, and then it corresponds to the controller being started. This + // is only accessed on the Matter queue or after the Matter queue has shut + // down. + FabricIndex _nextAvailableFabricIndex; +} + (void)initialize { @@ -273,12 +318,23 @@ - (void)cleanupStartupObjects _opCertStore = nullptr; } + if (_sessionResumptionStorage) { + delete _sessionResumptionStorage; + _sessionResumptionStorage = nullptr; + } + if (_persistentStorageDelegate) { delete _persistentStorageDelegate; _persistentStorageDelegate = nullptr; } } +- (CHIP_ERROR)_initFabricTable:(FabricTable &)fabricTable +{ + return fabricTable.Init( + { .storage = _persistentStorageDelegate, .operationalKeystore = _keystore, .opCertStore = _opCertStore }); +} + - (nullable NSArray *)knownFabrics { [self _assertCurrentQueueIsNotMatterQueue]; @@ -291,9 +347,7 @@ - (void)cleanupStartupObjects __block BOOL listFilled = NO; auto fillListBlock = ^{ FabricTable fabricTable; - CHIP_ERROR err = fabricTable.Init({ .storage = self->_persistentStorageDelegate, - .operationalKeystore = self->_keystore, - .opCertStore = self->_opCertStore }); + CHIP_ERROR err = [self _initFabricTable:fabricTable]; if (err != CHIP_NO_ERROR) { MTR_LOG_ERROR("Can't initialize fabric table when getting known fabrics: %s", err.AsString()); return; @@ -348,7 +402,22 @@ - (BOOL)startControllerFactory:(MTRDeviceControllerFactoryParams *)startupParams [MTRControllerAccessControl init]; - _persistentStorageDelegate = new MTRPersistentStorageDelegateBridge(startupParams.storage); + if (startupParams.hasStorage) { + _persistentStorageDelegate = new (std::nothrow) MTRPersistentStorageDelegateBridge(startupParams.storage); + _sessionResumptionStorage = nullptr; + _usingPerControllerStorage = NO; + } else { + _persistentStorageDelegate = new (std::nothrow) MTRDemuxingStorage(self); + _sessionResumptionStorage = new (std::nothrow) MTRSessionResumptionStorageBridge(self); + _usingPerControllerStorage = YES; + + if (_sessionResumptionStorage == nil) { + MTR_LOG_ERROR("Error: %@", kErrorSessionResumptionStorageInit); + errorCode = CHIP_ERROR_NO_MEMORY; + return; + } + } + if (_persistentStorageDelegate == nil) { MTR_LOG_ERROR("Error: %@", kErrorPersistentStorageInit); errorCode = CHIP_ERROR_NO_MEMORY; @@ -486,6 +555,7 @@ - (BOOL)startControllerFactory:(MTRDeviceControllerFactoryParams *)startupParams params.operationalKeystore = _keystore; params.opCertStore = _opCertStore; params.certificateValidityPolicy = _certificateValidityPolicy; + params.sessionResumptionStorage = _sessionResumptionStorage; errorCode = _controllerFactory->Init(params); if (errorCode != CHIP_NO_ERROR) { MTR_LOG_ERROR("Error: %@", kErrorControllerFactoryInit); @@ -559,7 +629,7 @@ - (void)stopControllerFactory * return nil if pre-startup fabric table checks fail, and set fabricError to * the right error value in that situation. */ -- (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceControllerStartupParams *)startupParams +- (MTRDeviceController * _Nullable)_startDeviceController:(id)startupParams fabricChecker:(MTRDeviceControllerStartupParamsInternal * (^)(FabricTable * fabricTable, MTRDeviceController * controller, CHIP_ERROR & fabricError))fabricChecker @@ -572,9 +642,37 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceControllerSt return nil; } + id storageDelegate; + NSUUID * uuid; + if ([startupParams isKindOfClass:[MTRDeviceControllerStartupParameters class]]) { + MTRDeviceControllerStartupParameters * params = startupParams; + storageDelegate = params.storageDelegate; + uuid = params.UUID; + } else { + MTRDeviceControllerStartupParams * params = startupParams; + storageDelegate = nil; + uuid = params.UUID; + } + + if (_usingPerControllerStorage && storageDelegate == nil) { + MTR_LOG_ERROR("Must have a controller storage delegate when we do not have storage for the controller factory"); + if (error != nil) { + *error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_INVALID_ARGUMENT]; + } + return nil; + } + + if (!_usingPerControllerStorage && storageDelegate != nil) { + MTR_LOG_ERROR("Must not have a controller storage delegate when we have storage for the controller factory"); + if (error != nil) { + *error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_INVALID_ARGUMENT]; + } + return nil; + } + // Create the controller, so we start the event loop, since we plan to do // our fabric table operations there. - auto * controller = [self createController]; + auto * controller = [self _createController:storageDelegate UUID:uuid]; if (controller == nil) { if (error != nil) { *error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY]; @@ -591,7 +689,39 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceControllerSt FabricTable * fabricTable = &fabricTableInstance; dispatch_sync(_chipWorkQueue, ^{ + fabricError = [self _initFabricTable:*fabricTable]; + if (fabricError != CHIP_NO_ERROR) { + MTR_LOG_ERROR("Can't initialize fabric table: %s", fabricError.AsString()); + return; + } + params = fabricChecker(fabricTable, controller, fabricError); + + if (params == nil) { + return; + } + + // Check that we are not trying to start a controller with a UUID that + // matches a running controller. + auto * controllersCopy = [self getRunningControllers]; + for (MTRDeviceController * existing in controllersCopy) { + if (existing != controller && [existing.UUID compare:params.UUID] == NSOrderedSame) { + MTR_LOG_ERROR("Already have running controller with UUID %@", existing.UUID); + fabricError = CHIP_ERROR_INVALID_ARGUMENT; + params = nil; + return; + } + } + + // Save off the next available fabric index, in case we are starting a + // controller with a new fabric index. This just needs to happen before + // we set _controllerBeingStarted below. + fabricError = fabricTable->PeekFabricIndexForNextAddition(self->_nextAvailableFabricIndex); + if (fabricError != CHIP_NO_ERROR) { + MTR_LOG_ERROR("Out of space in the fabric table"); + params = nil; + return; + } }); if (params == nil) { @@ -602,7 +732,16 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceControllerSt return nil; } + os_unfair_lock_lock(&_controllersLock); + _controllerBeingStarted = controller; + os_unfair_lock_unlock(&_controllersLock); + BOOL ok = [controller startup:params]; + + os_unfair_lock_lock(&_controllersLock); + _controllerBeingStarted = nil; + os_unfair_lock_unlock(&_controllersLock); + if (ok == NO) { // TODO: get error from controller's startup. if (error != nil) { @@ -625,6 +764,16 @@ - (MTRDeviceController * _Nullable)createControllerOnExistingFabric:(MTRDeviceCo { [self _assertCurrentQueueIsNotMatterQueue]; + if (_usingPerControllerStorage) { + // We can never have an "existing fabric" for a new controller to be + // created on, in the sense of createControllerOnExistingFabric. + MTR_LOG_ERROR("Can't createControllerOnExistingFabric when using per-controller data store"); + if (error != nil) { + *error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]; + } + return nil; + } + return [self _startDeviceController:startupParams fabricChecker:^MTRDeviceControllerStartupParamsInternal *( FabricTable * fabricTable, MTRDeviceController * controller, CHIP_ERROR & fabricError) { @@ -728,11 +877,34 @@ - (MTRDeviceController * _Nullable)createControllerOnNewFabric:(MTRDeviceControl error:error]; } -- (MTRDeviceController * _Nullable)createController +- (MTRDeviceController * _Nullable)createController:(MTRDeviceControllerStartupParameters *)startupParameters + error:(NSError * __autoreleasing *)error +{ + [self _assertCurrentQueueIsNotMatterQueue]; + + return [self _startDeviceController:startupParameters + fabricChecker:^MTRDeviceControllerStartupParamsInternal *( + FabricTable * fabricTable, MTRDeviceController * controller, CHIP_ERROR & fabricError) { + return + [[MTRDeviceControllerStartupParamsInternal alloc] initForNewController:controller + fabricTable:fabricTable + keystore:self->_keystore + advertiseOperational:self.advertiseOperational + params:startupParameters + error:fabricError]; + } + error:error]; +} + +- (MTRDeviceController * _Nullable)_createController:(id _Nullable)storageDelegate + UUID:(NSUUID *)UUID { [self _assertCurrentQueueIsNotMatterQueue]; - MTRDeviceController * controller = [[MTRDeviceController alloc] initWithFactory:self queue:_chipWorkQueue]; + MTRDeviceController * controller = [[MTRDeviceController alloc] initWithFactory:self + queue:_chipWorkQueue + storageDelegate:storageDelegate + UUID:UUID]; if (controller == nil) { MTR_LOG_ERROR("Failed to init controller"); return nil; @@ -761,7 +933,7 @@ - (MTRDeviceController * _Nullable)createController // Returns NO on failure, YES on success. If YES is returned, the // outparam will be written to, but possibly with a null value. // -// fabricTable should be an un-initialized fabric table. It needs to +// fabricTable should be an initialized fabric table. It needs to // outlive the consumer's use of the FabricInfo we return, which is // why it's provided by the caller. - (BOOL)findMatchingFabric:(FabricTable &)fabricTable @@ -770,16 +942,9 @@ - (BOOL)findMatchingFabric:(FabricTable &)fabricTable { assertChipStackLockedByCurrentThread(); - CHIP_ERROR err = fabricTable.Init( - { .storage = _persistentStorageDelegate, .operationalKeystore = _keystore, .opCertStore = _opCertStore }); - if (err != CHIP_NO_ERROR) { - MTR_LOG_ERROR("Can't initialize fabric table: %s", ErrorStr(err)); - return NO; - } - Crypto::P256PublicKey pubKey; if (params.rootCertificate != nil) { - err = ExtractPubkeyFromX509Cert(AsByteSpan(params.rootCertificate), pubKey); + CHIP_ERROR err = ExtractPubkeyFromX509Cert(AsByteSpan(params.rootCertificate), pubKey); if (err != CHIP_NO_ERROR) { MTR_LOG_ERROR("Can't extract public key from root certificate: %s", ErrorStr(err)); return NO; @@ -787,7 +952,7 @@ - (BOOL)findMatchingFabric:(FabricTable &)fabricTable } else { // No root certificate means the nocSigner is using the root keys, because // consumers must provide a root certificate whenever an ICA is used. - err = MTRP256KeypairBridge::MatterPubKeyFromSecKeyRef(params.nocSigner.publicKey, &pubKey); + CHIP_ERROR err = MTRP256KeypairBridge::MatterPubKeyFromSecKeyRef(params.nocSigner.publicKey, &pubKey); if (err != CHIP_NO_ERROR) { MTR_LOG_ERROR("Can't extract public key from MTRKeypair: %s", ErrorStr(err)); return NO; @@ -850,9 +1015,33 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller } os_unfair_lock_lock(&_controllersLock); + // Make sure to set _controllerBeingShutDown and do the remove in the same + // locked section, so there is never a time when the controller is gone from + // both places as viewed from the Matter thread, as long as it's locking + // around its reads. + _controllerBeingShutDown = controller; [_controllers removeObject:controller]; os_unfair_lock_unlock(&_controllersLock); + // Snapshot the controller's fabric index, if any, before it clears it + // out in shutDownCppController. + __block FabricIndex controllerFabricIndex = controller.fabricIndex; + + // This block runs either during sync dispatch to the Matter queue or after + // Matter queue shutdown, so it can touch any of our members without + // worrying about locking, since nothing else will race it. + auto sharedCleanupBlock = ^{ + assertChipStackLockedByCurrentThread(); + + [controller shutDownCppController]; + + self->_controllerBeingShutDown = nil; + if (self->_controllerBeingStarted == controller) { + controllerFabricIndex = self->_nextAvailableFabricIndex; + self->_controllerBeingStarted = nil; + } + }; + if ([_controllers count] == 0) { dispatch_sync(_chipWorkQueue, ^{ delete self->_operationalBrowser; @@ -867,7 +1056,23 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller _otaProviderDelegateBridge->Shutdown(); } - [controller shutDownCppController]; + sharedCleanupBlock(); + + // Now that our per-controller storage for the controller being shut + // down is guaranteed to be disconnected, go ahead and clean up the + // fabric table entry for the controller if we're in per-controller + // storage mode. + if (self->_usingPerControllerStorage) { + // We have to use a new fabric table to do this cleanup, because + // our system state is gone now. + FabricTable fabricTable; + CHIP_ERROR err = [self _initFabricTable:fabricTable]; + if (err != CHIP_NO_ERROR) { + MTR_LOG_ERROR("Failed to clean up fabric entries. Expect things to act oddly: %" CHIP_ERROR_FORMAT, err.Format()); + } else { + fabricTable.Delete(controllerFabricIndex); + } + } } else { // Do the controller shutdown on the Matter work queue. dispatch_sync(_chipWorkQueue, ^{ @@ -875,7 +1080,19 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller _otaProviderDelegateBridge->ControllerShuttingDown(controller); } - [controller shutDownCppController]; + sharedCleanupBlock(); + + // Now that our per-controller storage for the controller being shut + // down is guaranteed to be disconnected, go ahead and clean up the + // fabric table entry for the controller if we're in per-controller + // storage mode. + if (self->_usingPerControllerStorage) { + // Make sure to delete controllerFabricIndex from the system state's + // fabric table. We know there's a system state here, because we + // still have a running controller. + auto * systemState = _controllerFactory->GetSystemState(); + systemState->Fabrics()->Delete(controllerFabricIndex); + } }); } @@ -890,21 +1107,42 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller return controllersCopy; } -- (nullable MTRDeviceController *)runningControllerForFabricIndex:(chip::FabricIndex)fabricIndex +- (nullable MTRDeviceController *)runningControllerForFabricIndex:(FabricIndex)fabricIndex + includeControllerStartingUp:(BOOL)includeControllerStartingUp + includeControllerShuttingDown:(BOOL)includeControllerShuttingDown { assertChipStackLockedByCurrentThread(); auto * controllersCopy = [self getRunningControllers]; + os_unfair_lock_lock(&_controllersLock); + MTRDeviceController * controllerBeingStarted = _controllerBeingStarted; + MTRDeviceController * controllerBeingShutDown = _controllerBeingShutDown; + os_unfair_lock_unlock(&_controllersLock); + for (MTRDeviceController * existing in controllersCopy) { - if ([existing fabricIndex] == fabricIndex) { + if (existing.fabricIndex == fabricIndex) { return existing; } } + if (includeControllerStartingUp == YES && controllerBeingStarted != nil && fabricIndex == _nextAvailableFabricIndex) { + return controllerBeingStarted; + } + + if (includeControllerShuttingDown == YES && controllerBeingShutDown != nil + && controllerBeingShutDown.fabricIndex == fabricIndex) { + return controllerBeingShutDown; + } + return nil; } +- (nullable MTRDeviceController *)runningControllerForFabricIndex:(chip::FabricIndex)fabricIndex +{ + return [self runningControllerForFabricIndex:fabricIndex includeControllerStartingUp:YES includeControllerShuttingDown:YES]; +} + - (void)operationalInstanceAdded:(chip::PeerId &)operationalID { assertChipStackLockedByCurrentThread(); @@ -936,6 +1174,25 @@ - (PersistentStorageDelegate *)storageDelegate @end +MTR_HIDDEN +@interface MTRDummyStorage : NSObject +@end + +@implementation MTRDummyStorage +- (nullable NSData *)storageDataForKey:(NSString *)key +{ + return nil; +} +- (BOOL)setStorageData:(NSData *)value forKey:(NSString *)key +{ + return NO; +} +- (BOOL)removeStorageDataForKey:(NSString *)key +{ + return NO; +} +@end + @implementation MTRDeviceControllerFactoryParams - (instancetype)initWithStorage:(id)storage @@ -945,6 +1202,27 @@ - (instancetype)initWithStorage:(id)storage } _storage = storage; + _hasStorage = YES; + _otaProviderDelegate = nil; + _productAttestationAuthorityCertificates = nil; + _certificationDeclarationCertificates = nil; + _port = nil; + _shouldStartServer = NO; + + return self; +} + +- (instancetype)init +{ + if (!(self = [super init])) { + return nil; + } + + // We promise to have a non-null storage for purposes of our attribute, but + // now we're allowing initialization without storage. Make up a dummy + // storage just so we don't have nil there. + _storage = [[MTRDummyStorage alloc] init]; + _hasStorage = NO; _otaProviderDelegate = nil; _productAttestationAuthorityCertificates = nil; _certificationDeclarationCertificates = nil; diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h index dc0b968b427782..4ffcb3af545506 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h @@ -53,6 +53,15 @@ NS_ASSUME_NONNULL_BEGIN */ - (nullable MTRDeviceController *)runningControllerForFabricIndex:(chip::FabricIndex)fabricIndex; +/** + * Find a running controller, if any, for the given fabric index. Allows + * controlling whether to include a controller that is in the middle of startup + * or shutdown. + */ +- (nullable MTRDeviceController *)runningControllerForFabricIndex:(chip::FabricIndex)fabricIndex + includeControllerStartingUp:(BOOL)includeControllerStartingUp + includeControllerShuttingDown:(BOOL)includeControllerShuttingDown; + /** * Notify the controller factory that a new operational instance with the given * compressed fabric id and node id has been observed. diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParameters.h b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParameters.h new file mode 100644 index 00000000000000..307f819c44af07 --- /dev/null +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParameters.h @@ -0,0 +1,84 @@ +/** + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import + +#import +#import + +NS_ASSUME_NONNULL_BEGIN + +MTR_NEWLY_AVAILABLE +@interface MTRDeviceControllerStartupParameters : NSObject + +- (instancetype)init NS_UNAVAILABLE; ++ (instancetype)new NS_UNAVAILABLE; + +/** + * Set an MTROperationalCertificateIssuer to call (on the provided queue) when + * operational certificates need to be provided during commissioning. + */ +- (void)setOperationalCertificateIssuer:(id)operationalCertificateIssuer + queue:(dispatch_queue_t)queue; + +@end + +MTR_NEWLY_AVAILABLE +@interface MTRDeviceControllerExternalCertificateStartupParameters : MTRDeviceControllerStartupParameters + +- (instancetype)init NS_UNAVAILABLE; ++ (instancetype)new NS_UNAVAILABLE; + +/** + * Prepare to initialize a controller that is not able to sign operational + * certificates itself, and therefore needs to be provided with a complete + * operational certificate chain. + * + * A controller created from MTRDeviceControllerStartupParams initialized with + * this method will not be able to commission devices unless + * operationalCertificateIssuer and operationalCertificateIssuerQueue are set. + * + * The fabric id and node id to use for the controller will be derived from the provided + * operationalCertificate. + * + * @param storageDelegate The storage to use for the controller. This will be + * called into on some arbitrary thread, but calls will + * be guaranteed to be serialized not race with each other. + * + * @param UUID The unique id to assign to the controller. + * + * @param vendorID The vendor ID (allocated by the Connectivity Standards Alliance) for + * this controller. Must not be the "standard" vendor id (0). + * + * @param ipk The Identity Protection Key. Must be 16 bytes in length. + * + * @param intermediateCertificate Must be nil if operationalCertificate is + * directly signed by rootCertificate. Otherwise + * must be the certificate that signed + * operationalCertificate. + */ +- (instancetype)initWithStorageDelegate:(id)storageDelegate + UUID:(NSUUID *)UUID + ipk:(NSData *)ipk + vendorID:(NSNumber *)vendorID + operationalKeypair:(id)operationalKeypair + operationalCertificate:(MTRCertificateDERBytes)operationalCertificate + intermediateCertificate:(MTRCertificateDERBytes _Nullable)intermediateCertificate + rootCertificate:(MTRCertificateDERBytes)rootCertificate; + +@end + +NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm index ca92c63bf2171b..93ca55b1c43993 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm @@ -18,10 +18,13 @@ #import "MTRCertificates.h" #import "MTRConversion.h" #import "MTRDeviceControllerStartupParams_Internal.h" +#import "MTRDeviceController_Internal.h" #import "MTRLogging_Internal.h" #import "MTRP256KeypairBridge.h" #import "NSDataSpanConversion.h" +#import + #include #include #include @@ -29,6 +32,41 @@ using namespace chip; +static CHIP_ERROR ExtractNodeIDFabricIDFromNOC( + MTRCertificateDERBytes noc, NSNumber * __autoreleasing * nodeID, NSNumber * __autoreleasing * fabricID) +{ + // ExtractNodeIdFabricIdFromOpCert needs a TLV-encoded opcert, not a DER-encoded one. + auto * tlvNOC = [MTRCertificates convertX509Certificate:noc]; + if (tlvNOC == nil) { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + ByteSpan nocSpan = AsByteSpan(tlvNOC); + + FabricId certFabricID = kUndefinedFabricId; + NodeId certNodeID = kUndefinedNodeId; + CHIP_ERROR err = Credentials::ExtractNodeIdFabricIdFromOpCert(nocSpan, &certNodeID, &certFabricID); + if (err != CHIP_NO_ERROR) { + MTR_LOG_ERROR("Unable to extract node ID and fabric ID from operational certificate: %s", err.AsString()); + return err; + } + *nodeID = @(certNodeID); + *fabricID = @(certFabricID); + return CHIP_NO_ERROR; +} + +static CHIP_ERROR ExtractFabricIDFromNOC(MTRCertificateDERBytes noc, NSNumber * __autoreleasing * fabricID) +{ + NSNumber * ignored; + return ExtractNodeIDFabricIDFromNOC(noc, &ignored, fabricID); +} + +static CHIP_ERROR ExtractNodeIDFromNOC(MTRCertificateDERBytes noc, NSNumber * __autoreleasing * nodeID) +{ + NSNumber * ignored; + return ExtractNodeIDFabricIDFromNOC(noc, nodeID, &ignored); +} + @implementation MTRDeviceControllerStartupParams - (instancetype)initWithIPK:(NSData *)ipk fabricID:(NSNumber *)fabricID nocSigner:(id)nocSigner @@ -45,6 +83,7 @@ - (instancetype)initWithIPK:(NSData *)ipk fabricID:(NSNumber *)fabricID nocSigne _nocSigner = nocSigner; _fabricID = [fabricID copy]; _ipk = [ipk copy]; + _UUID = [NSUUID UUID]; return self; } @@ -59,24 +98,13 @@ - (instancetype)initWithIPK:(NSData *)ipk return nil; } - { // Scope for temporaries - // ExtractNodeIdFabricIdFromOpCert needs a TLV-encoded opcert, not a DER-encoded one. - uint8_t tlvOpCertBuf[Credentials::kMaxCHIPCertLength]; - MutableByteSpan tlvOpCert(tlvOpCertBuf); - CHIP_ERROR err = Credentials::ConvertX509CertToChipCert(AsByteSpan(operationalCertificate), tlvOpCert); + { // Scope for temporary + NSNumber * fabricID; + CHIP_ERROR err = ExtractFabricIDFromNOC(operationalCertificate, &fabricID); if (err != CHIP_NO_ERROR) { - MTR_LOG_ERROR("Unable to convert operational certificate to TLV: %s", ErrorStr(err)); return nil; } - - FabricId fabricId = kUndefinedFabricId; - NodeId unused = kUndefinedNodeId; - err = Credentials::ExtractNodeIdFabricIdFromOpCert(tlvOpCert, &unused, &fabricId); - if (err != CHIP_NO_ERROR) { - MTR_LOG_ERROR("Unable to extract fabric id from operational certificate: %s", ErrorStr(err)); - return nil; - } - _fabricID = @(fabricId); + _fabricID = fabricID; } _operationalKeypair = operationalKeypair; @@ -84,6 +112,7 @@ - (instancetype)initWithIPK:(NSData *)ipk _intermediateCertificate = [intermediateCertificate copy]; _rootCertificate = [rootCertificate copy]; _ipk = [ipk copy]; + _UUID = [NSUUID UUID]; return self; } @@ -106,6 +135,44 @@ - (instancetype)initWithParams:(MTRDeviceControllerStartupParams *)params _operationalKeypair = params.operationalKeypair; _operationalCertificateIssuer = params.operationalCertificateIssuer; _operationalCertificateIssuerQueue = params.operationalCertificateIssuerQueue; + _UUID = params.UUID; + + return self; +} + +- (instancetype)initWithParameters:(MTRDeviceControllerStartupParameters *)params error:(CHIP_ERROR &)error +{ + if (!(self = [super init])) { + error = CHIP_ERROR_INCORRECT_STATE; + return nil; + } + + if (![params isKindOfClass:[MTRDeviceControllerExternalCertificateStartupParameters class]]) { + MTR_LOG_ERROR("Unexpected subclass of MTRDeviceControllerStartupParameters"); + error = CHIP_ERROR_INVALID_ARGUMENT; + return nil; + } + + _nocSigner = nil; + + NSNumber * fabricID; + error = ExtractFabricIDFromNOC(params.operationalCertificate, &fabricID); + if (error != CHIP_NO_ERROR) { + return nil; + } + _fabricID = fabricID; + + _ipk = params.ipk; + _vendorID = params.vendorID; + _nodeID = nil; + _caseAuthenticatedTags = nil; + _rootCertificate = params.rootCertificate; + _intermediateCertificate = params.intermediateCertificate; + _operationalCertificate = params.operationalCertificate; + _operationalKeypair = params.operationalKeypair; + _operationalCertificateIssuer = params.operationalCertificateIssuer; + _operationalCertificateIssuerQueue = params.operationalCertificateIssuerQueue; + _UUID = params.UUID; return self; } @@ -120,7 +187,7 @@ - (instancetype)initWithParams:(MTRDeviceControllerStartupParams *)params MutableByteSpan derCert(buf); CHIP_ERROR err = Credentials::ConvertChipCertToX509Cert(cert, derCert); if (err != CHIP_NO_ERROR) { - MTR_LOG_ERROR("Failed do convert Matter certificate to X.509 DER: %s", ErrorStr(err)); + MTR_LOG_ERROR("Failed to convert Matter certificate to X.509 DER: %s", ErrorStr(err)); return nil; } @@ -174,6 +241,64 @@ - (instancetype)initWithOperationalKeypair:(id)operationalKeypair @end +@implementation MTRDeviceControllerStartupParameters +- (instancetype)initWithStorageDelegate:(id)storageDelegate + UUID:(NSUUID *)UUID + ipk:(NSData *)ipk + vendorID:(NSNumber *)vendorID + operationalKeypair:(id)operationalKeypair + operationalCertificate:(MTRCertificateDERBytes)operationalCertificate + intermediateCertificate:(MTRCertificateDERBytes _Nullable)intermediateCertificate + rootCertificate:(MTRCertificateDERBytes)rootCertificate +{ + if (!(self = [super init])) { + return nil; + } + + _ipk = ipk; + _vendorID = vendorID; + _rootCertificate = rootCertificate; + _intermediateCertificate = intermediateCertificate; + _operationalCertificate = operationalCertificate; + _operationalKeypair = operationalKeypair; + + _operationalCertificateIssuer = nil; + _operationalCertificateIssuerQueue = nil; + _storageDelegate = storageDelegate; + _UUID = UUID; + + return self; +} + +- (void)setOperationalCertificateIssuer:(id)operationalCertificateIssuer + queue:(dispatch_queue_t)queue +{ + _operationalCertificateIssuer = operationalCertificateIssuer; + _operationalCertificateIssuerQueue = queue; +} +@end + +@implementation MTRDeviceControllerExternalCertificateStartupParameters +- (instancetype)initWithStorageDelegate:(id)storageDelegate + UUID:(NSUUID *)UUID + ipk:(NSData *)ipk + vendorID:(NSNumber *)vendorID + operationalKeypair:(id)operationalKeypair + operationalCertificate:(MTRCertificateDERBytes)operationalCertificate + intermediateCertificate:(MTRCertificateDERBytes _Nullable)intermediateCertificate + rootCertificate:(MTRCertificateDERBytes)rootCertificate +{ + return [super initWithStorageDelegate:storageDelegate + UUID:UUID + ipk:ipk + vendorID:vendorID + operationalKeypair:operationalKeypair + operationalCertificate:operationalCertificate + intermediateCertificate:intermediateCertificate + rootCertificate:rootCertificate]; +} +@end + @implementation MTRDeviceControllerStartupParamsInternal - (instancetype)initWithParams:(MTRDeviceControllerStartupParams *)params @@ -182,6 +307,8 @@ - (instancetype)initWithParams:(MTRDeviceControllerStartupParams *)params return nil; } + _storageDelegate = nil; + if (self.nocSigner == nil && self.rootCertificate == nil) { MTR_LOG_ERROR("nocSigner and rootCertificate are both nil; no public key available to identify the fabric"); return nil; @@ -249,6 +376,7 @@ - (instancetype)initForNewFabric:(chip::FabricTable *)fabricTable _fabricTable = fabricTable; _keystore = keystore; _advertiseOperational = advertiseOperational; + _allowMultipleControllersPerFabric = NO; return self; } @@ -379,6 +507,88 @@ - (instancetype)initForExistingFabric:(FabricTable *)fabricTable _fabricIndex.Emplace(fabricIndex); _keystore = keystore; _advertiseOperational = advertiseOperational; + _allowMultipleControllersPerFabric = NO; + + return self; +} + +- (instancetype)initForNewController:(MTRDeviceController *)controller + fabricTable:(chip::FabricTable *)fabricTable + keystore:(chip::Crypto::OperationalKeystore *)keystore + advertiseOperational:(BOOL)advertiseOperational + params:(MTRDeviceControllerStartupParameters *)params + error:(CHIP_ERROR &)error +{ + if (!(self = [super initWithParameters:params error:error])) { + return nil; + } + + Crypto::P256PublicKey pubKey; + error = ExtractPubkeyFromX509Cert(AsByteSpan(self.rootCertificate), pubKey); + if (error != CHIP_NO_ERROR) { + MTR_LOG_ERROR("Can't extract public key from root certificate: %s", error.AsString()); + return nil; + } + + NSNumber * nodeID; + error = ExtractNodeIDFromNOC(self.operationalCertificate, &nodeID); + if (error != CHIP_NO_ERROR) { + // Already logged. + return nil; + } + + if (fabricTable->FindIdentity(pubKey, self.fabricID.unsignedLongLongValue, nodeID.unsignedLongLongValue)) { + MTR_LOG_ERROR("Trying to start a controller identity that is already running"); + error = CHIP_ERROR_INVALID_ARGUMENT; + return nil; + } + + auto * oldNOCTLV = [controller.controllerDataStore fetchLastUsedNOC]; + if (oldNOCTLV != nil) { + ByteSpan oldNOCSpan = AsByteSpan(oldNOCTLV); + + FabricId ignored = kUndefinedFabricId; + NodeId oldNodeID = kUndefinedNodeId; + CHIP_ERROR err = Credentials::ExtractNodeIdFabricIdFromOpCert(oldNOCSpan, &oldNodeID, &ignored); + if (err != CHIP_NO_ERROR) { + MTR_LOG_ERROR("Unable to extract node ID and fabric ID from old operational certificate: %s", err.AsString()); + return nil; + } + + CATValues oldCATs; + err = Credentials::ExtractCATsFromOpCert(oldNOCSpan, oldCATs); + if (err != CHIP_NO_ERROR) { + MTR_LOG_ERROR("Failed to extract CATs from old operational certificate: %s", err.AsString()); + return nil; + } + + auto * tlvNOC = [MTRCertificates convertX509Certificate:self.operationalCertificate]; + if (tlvNOC == nil) { + return nil; + } + + ByteSpan nocSpan = AsByteSpan(tlvNOC); + CATValues newCATs; + err = Credentials::ExtractCATsFromOpCert(nocSpan, newCATs); + if (err != CHIP_NO_ERROR) { + MTR_LOG_ERROR("Failed to extract CATs from new operational certificate: %s", err.AsString()); + return nil; + } + + if (nodeID.unsignedLongLongValue != oldNodeID || oldCATs != newCATs) { + // Our NOC has changed in a way that would affect ACL checks. Clear + // out our session resumption storage, because resuming those CASE + // sessions will end up doing ACL checks against our old NOC. + MTR_LOG_DEFAULT("Node ID or CATs changed. Clearing CASE resumption storage."); + [controller.controllerDataStore clearAllResumptionInfo]; + } + } + + _fabricTable = fabricTable; + _keystore = keystore; + _advertiseOperational = advertiseOperational; + _allowMultipleControllersPerFabric = YES; + _storageDelegate = params.storageDelegate; return self; } diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams_Internal.h index 24acc7a2dbb3a8..b68ef04a8c0d64 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams_Internal.h @@ -19,6 +19,8 @@ #import "MTRDeviceControllerStartupParams.h" #import #import +#import +#import #include #include @@ -39,18 +41,50 @@ NS_ASSUME_NONNULL_BEGIN // MTRDeviceControllerStartupParamsInternal. @property (nonatomic, copy, nullable) MTRCertificateDERBytes operationalCertificate; +// UUID, so that we always have one. +@property (nonatomic, strong, readonly) NSUUID * UUID; + // Init method that just copies the values of all our ivars. - (instancetype)initWithParams:(MTRDeviceControllerStartupParams *)params; @end +@interface MTRDeviceControllerStartupParameters () + +- (instancetype)initWithStorageDelegate:(id)storageDelegate + UUID:(NSUUID *)UUID + ipk:(NSData *)ipk + vendorID:(NSNumber *)vendorID + operationalKeypair:(id)operationalKeypair + operationalCertificate:(MTRCertificateDERBytes)operationalCertificate + intermediateCertificate:(MTRCertificateDERBytes _Nullable)intermediateCertificate + rootCertificate:(MTRCertificateDERBytes)rootCertificate; + +// When we have other subclasses of MTRDeviceControllerStartupParameters, we may +// need to make more things nullable here and/or add more fields. But for now +// we know exactly what information we have. +@property (nonatomic, copy, readonly) NSData * ipk; +@property (nonatomic, copy, readonly) NSNumber * vendorID; +@property (nonatomic, copy, readonly) MTRCertificateDERBytes rootCertificate; +@property (nonatomic, copy, readonly, nullable) MTRCertificateDERBytes intermediateCertificate; +@property (nonatomic, copy, readonly) MTRCertificateDERBytes operationalCertificate; +@property (nonatomic, strong, readonly) id operationalKeypair; + +@property (nonatomic, strong, nullable, readonly) id operationalCertificateIssuer; +@property (nonatomic, strong, nullable, readonly) dispatch_queue_t operationalCertificateIssuerQueue; + +@property (nonatomic, strong, readonly) id storageDelegate; +@property (nonatomic, strong, readonly) NSUUID * UUID; + +@end + MTR_HIDDEN @interface MTRDeviceControllerStartupParamsInternal : MTRDeviceControllerStartupParams // Fabric table we can use to do things like allocate operational keys. @property (nonatomic, assign, readonly) chip::FabricTable * fabricTable; -// Fabric index we're starting on. Only has a value when starting on an -// existing fabric. +// Fabric index we're starting on. Only has a value when starting against an +// existing fabric table entry. @property (nonatomic, assign, readonly) chip::Optional fabricIndex; // Key store we're using with our fabric table, for sanity checks. @@ -58,6 +92,15 @@ MTR_HIDDEN @property (nonatomic, assign, readonly) BOOL advertiseOperational; +@property (nonatomic, assign, readonly) BOOL allowMultipleControllersPerFabric; + +/** + * A storage delegate that can be provided when initializing the startup params. + * This must be provided if and only if the controller factory was initialized + * without storage. + */ +@property (nonatomic, strong, nullable, readonly) id storageDelegate; + /** * Helper method that checks that our keypairs match our certificates. * Specifically: @@ -90,7 +133,17 @@ MTR_HIDDEN params:(MTRDeviceControllerStartupParams *)params; /** - * Should use initForExistingFabric or initForNewFabric to initialize + * Initialize for controller bringup with per-controller storage. + */ +- (instancetype)initForNewController:(MTRDeviceController *)controller + fabricTable:(chip::FabricTable *)fabricTable + keystore:(chip::Crypto::OperationalKeystore *)keystore + advertiseOperational:(BOOL)advertiseOperational + params:(MTRDeviceControllerStartupParameters *)params + error:(CHIP_ERROR &)error; + +/** + * Should use initForExistingFabric or initForNewFabric or initForController to initialize * internally. */ - (instancetype)initWithIPK:(NSData *)ipk fabricID:(NSNumber *)fabricID nocSigner:(id)nocSigner NS_UNAVAILABLE; diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h b/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h new file mode 100644 index 00000000000000..86be0ecc7ee773 --- /dev/null +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h @@ -0,0 +1,117 @@ +/** + * Copyright (c) 2022-2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import +#import + +NS_ASSUME_NONNULL_BEGIN + +typedef NS_ENUM(NSUInteger, MTRStorageSecurityLevel) { + // Data must be stored in secure (encrypted) storage. + MTRStorageSecurityLevelSecure, + // Data may be stored in the clear. + MTRStorageSecurityLevelNotSecure, +} MTR_NEWLY_AVAILABLE; + +typedef NS_ENUM(NSUInteger, MTRStorageSharingType) { + // Data must not be shared at all (just store locally). + MTRStorageSharingTypeNotShared, + + // Data may be shared, but only between controllers that have the same node + // identity (same fabric, same node ID). Sharing not required (could be + // stored locally). + MTRStorageSharingTypeSameIdentityAllowed, + + // Data must be shared, but only between controllers that have the same node + // identity (same fabric, same node ID). + MTRStorageSharingTypeSameIdentityRequired, + + // Data may be shared, but only between controllers that have the same + // access to devices (e.g. controllers that all have the same CATs if ACLs + // are being done via CATs). + MTRStorageSharingTypeSameACLs, + + // Data may be, but does not have to be, shared across all controllers on a + // given fabric. + MTRStorageSharingTypeSameFabricAllowed, + + // Data must be shared across all controllers on a given fabric. + MTRStorageSharingTypeSameFabricRequired, +} MTR_NEWLY_AVAILABLE; + +/** + * Protocol for storing and retrieving controller-specific data. + * + * Implementations of this protocol MUST keep two things in mind: + * + * 1) The controller provided to the delegate methods may not be fully + * initialized when the callbacks are called. The only safe thing to do with + * it is to get its controllerID. + * + * 2) The delegate method calls will happen serially, not concurrently, on some arbitrary + * thread. During execution of the delegate methods, all Matter work will be + * blocked until the method completes. Attempting to call any Matter API + * from inside the delegate methods, apart from de-serializing and + * serializing the items being stored, is likely to lead to deadlocks. + */ +MTR_NEWLY_AVAILABLE +@protocol MTRDeviceControllerStorageDelegate +@required +/** + * Return the stored value for the given key, if any, for the provided + * controller. Returns nil if there is no stored value. + * + * securityLevel and dataType will always be the same for any given key value + * and are just present here to help locate the data if storage location is + * separated out by security level and data type. + * + * The set of classes that might be decoded by this function is available by + * calling MTRDeviceControllerStorageClasses(). + */ +- (nullable id)controller:(MTRDeviceController *)controller + valueForKey:(NSString *)key + securityLevel:(MTRStorageSecurityLevel)securityLevel + sharingType:(MTRStorageSharingType)sharingType; + +/** + * Store a value for the given key. Returns whether the store succeeded. + * + * securityLevel and dataType will always be the same for any given key value + * and are present here as a hint to how the value should be stored. + */ +- (BOOL)controller:(MTRDeviceController *)controller + storeValue:(id)value + forKey:(NSString *)key + securityLevel:(MTRStorageSecurityLevel)securityLevel + sharingType:(MTRStorageSharingType)sharingType; + +/** + * Remove the stored value for the given key. Returns whether the remove succeeded. + * + * securityLevel and dataType will always be the same for any given key value + * and are just present here to help locate the data if storage location is + * separated out by security level and data type. + */ +- (BOOL)controller:(MTRDeviceController *)controller + removeValueForKey:(NSString *)key + securityLevel:(MTRStorageSecurityLevel)securityLevel + sharingType:(MTRStorageSharingType)sharingType; +@end + +// TODO: FIXME: Is this a sane place to put this API? +MTR_EXTERN MTR_NEWLY_AVAILABLE NSSet * MTRDeviceControllerStorageClasses(void); + +NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h index 28beb9b8244a13..9bb249d0ee31d7 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h @@ -29,6 +29,10 @@ #import "MTRBaseDevice.h" #import "MTRDeviceController.h" +#import "MTRDeviceControllerDataStore.h" +#import "MTRDeviceControllerStorageDelegate.h" + +#import @class MTRDeviceControllerStartupParamsInternal; @class MTRDeviceControllerFactory; @@ -73,12 +77,20 @@ NS_ASSUME_NONNULL_BEGIN */ @property (readonly, nullable) NSNumber * compressedFabricID; +/** + * The per-controller data store this controller was initialized with, if any. + */ +@property (nonatomic, nullable) MTRDeviceControllerDataStore * controllerDataStore; + /** * Init a newly created controller. * * Only MTRDeviceControllerFactory should be calling this. */ -- (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory queue:(dispatch_queue_t)queue; +- (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory + queue:(dispatch_queue_t)queue + storageDelegate:(id _Nullable)storageDelegate + UUID:(NSUUID *)UUID; /** * Check whether this controller is running on the given fabric, as represented diff --git a/src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.h b/src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.h new file mode 100644 index 00000000000000..389fcc2a9eba10 --- /dev/null +++ b/src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.h @@ -0,0 +1,59 @@ +/** + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import +#import + +#import "MTRDeviceControllerDataStore.h" + +#include + +NS_ASSUME_NONNULL_BEGIN + +/** + * Implements SessionResumptionStorage and dispatches the relevant lookup + * requests to running controllers as needed. + */ +class MTRSessionResumptionStorageBridge : public chip::SessionResumptionStorage +{ +public: + MTRSessionResumptionStorageBridge(MTRDeviceControllerFactory * factory); + + ~MTRSessionResumptionStorageBridge() {} + + // SessionResumptionStorage API. + CHIP_ERROR FindByScopedNodeId(const chip::ScopedNodeId & node, ResumptionIdStorage & resumptionId, + chip::Crypto::P256ECDHDerivedSecret & sharedSecret, chip::CATValues & peerCATs) override; + + CHIP_ERROR FindByResumptionId(ConstResumptionIdView resumptionId, chip::ScopedNodeId & node, + chip::Crypto::P256ECDHDerivedSecret & sharedSecret, chip::CATValues & peerCATs) override; + CHIP_ERROR Save(const chip::ScopedNodeId & node, ConstResumptionIdView resumptionId, + const chip::Crypto::P256ECDHDerivedSecret & sharedSecret, const chip::CATValues & peerCATs) override; + CHIP_ERROR DeleteAll(chip::FabricIndex fabricIndex) override; + +private: + /** + * Helper method to convert a MTRCASESessionResumptionInfo into the pieces + * we need to return from our Find* methods. + */ + static CHIP_ERROR DeconstructResumptionInfo(MTRCASESessionResumptionInfo * resumptionInfo, chip::NodeId & nodeID, + ResumptionIdStorage & resumptionId, + chip::Crypto::P256ECDHDerivedSecret & sharedSecret, chip::CATValues & peerCATs); + + MTRDeviceControllerFactory * mFactory; +}; + +NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.mm b/src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.mm new file mode 100644 index 00000000000000..56fe7bc7beb6f8 --- /dev/null +++ b/src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.mm @@ -0,0 +1,149 @@ +/** + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import "MTRSessionResumptionStorageBridge.h" + +#import "MTRConversion.h" +#import "MTRDeviceControllerFactory_Internal.h" +#import "MTRDeviceController_Internal.h" +#import "MTRLogging_Internal.h" +#import "NSDataSpanConversion.h" + +#include + +using namespace chip; + +MTRSessionResumptionStorageBridge::MTRSessionResumptionStorageBridge(MTRDeviceControllerFactory * factory) + : mFactory(factory) +{ +} + +CHIP_ERROR MTRSessionResumptionStorageBridge::FindByScopedNodeId(const chip::ScopedNodeId & node, + ResumptionIdStorage & resumptionId, chip::Crypto::P256ECDHDerivedSecret & sharedSecret, chip::CATValues & peerCATs) +{ + assertChipStackLockedByCurrentThread(); + + auto * controller = [mFactory runningControllerForFabricIndex:node.GetFabricIndex()]; + if (controller == nil) { + return CHIP_ERROR_KEY_NOT_FOUND; + } + + auto * resumptionInfo = [controller.controllerDataStore findResumptionInfoByNodeID:@(node.GetNodeId())]; + if (resumptionInfo == nil) { + return CHIP_ERROR_KEY_NOT_FOUND; + } + + NodeId ignored; + return DeconstructResumptionInfo(resumptionInfo, ignored, resumptionId, sharedSecret, peerCATs); +} + +CHIP_ERROR MTRSessionResumptionStorageBridge::FindByResumptionId(ConstResumptionIdView resumptionId, chip::ScopedNodeId & node, + chip::Crypto::P256ECDHDerivedSecret & sharedSecret, chip::CATValues & peerCATs) +{ + assertChipStackLockedByCurrentThread(); + + auto * resumptionIDData = AsData(resumptionId); + + auto * controllerList = [mFactory getRunningControllers]; + for (MTRDeviceController * controller in controllerList) { + FabricIndex fabricIndex = controller.fabricIndex; + if (!IsValidFabricIndex(fabricIndex)) { + // This controller is not sufficiently "running"; it does not have a + // fabric index yet. Just skip it. + continue; + } + + auto * resumptionInfo = [controller.controllerDataStore findResumptionInfoByResumptionID:resumptionIDData]; + if (resumptionInfo != nil) { + NodeId nodeID; + ResumptionIdStorage ignored; + ReturnErrorOnFailure(DeconstructResumptionInfo(resumptionInfo, nodeID, ignored, sharedSecret, peerCATs)); + node = ScopedNodeId(nodeID, fabricIndex); + return CHIP_NO_ERROR; + } + } + + // None of the controllers matched. + return CHIP_ERROR_KEY_NOT_FOUND; +} + +CHIP_ERROR MTRSessionResumptionStorageBridge::Save(const chip::ScopedNodeId & node, ConstResumptionIdView resumptionId, + const chip::Crypto::P256ECDHDerivedSecret & sharedSecret, const chip::CATValues & peerCATs) +{ + assertChipStackLockedByCurrentThread(); + + auto * controller = [mFactory runningControllerForFabricIndex:node.GetFabricIndex()]; + if (controller == nil) { + return CHIP_ERROR_PERSISTED_STORAGE_FAILED; + } + + auto * resumptionInfo = [[MTRCASESessionResumptionInfo alloc] init]; + resumptionInfo.nodeID = @(node.GetNodeId()); + resumptionInfo.resumptionID = AsData(resumptionId); + resumptionInfo.sharedSecret = AsData(ByteSpan(sharedSecret.ConstBytes(), sharedSecret.Length())); + resumptionInfo.caseAuthenticatedTags = CATValuesToSet(peerCATs); + + [controller.controllerDataStore storeResumptionInfo:resumptionInfo]; + return CHIP_NO_ERROR; +} + +CHIP_ERROR MTRSessionResumptionStorageBridge::DeleteAll(chip::FabricIndex fabricIndex) +{ + assertChipStackLockedByCurrentThread(); + + // NOTE: During controller startup, the Matter SDK thinks that the cert for + // a fabric index is changing and hence that it must remove session + // resumption data for that fabric index. For us that does not matter, + // since we don't key that data on fabric index anyway. But we do want to + // avoid doing this delete-on-startup so we can actually store session + // resumption data persistently. + + // And that is the only use of DeleteAll for controllers in practice, in the + // situations where we are using MTRSessionResumptionStorageBridge at all. + // So just no-op this function, but verify that our assumptions hold. + auto * controller = [mFactory runningControllerForFabricIndex:fabricIndex + includeControllerStartingUp:NO + includeControllerShuttingDown:YES]; + VerifyOrDieWithMsg(controller == nil, Controller, "Deleting resumption storage for controller outside startup"); + return CHIP_NO_ERROR; +} + +CHIP_ERROR MTRSessionResumptionStorageBridge::DeconstructResumptionInfo(MTRCASESessionResumptionInfo * resumptionInfo, + chip::NodeId & nodeID, ResumptionIdStorage & resumptionId, chip::Crypto::P256ECDHDerivedSecret & sharedSecret, + chip::CATValues & peerCATs) +{ + if (resumptionInfo.resumptionID.length != resumptionId.size()) { + MTR_LOG_ERROR("Unable to return resumption ID: Stored size %llu does not match required size %llu", + static_cast(resumptionInfo.resumptionID.length), + static_cast(resumptionId.size())); + return CHIP_ERROR_PERSISTED_STORAGE_FAILED; + } + + if (resumptionInfo.sharedSecret.length > sharedSecret.Capacity()) { + MTR_LOG_ERROR("Unable to return resumption shared secret: Stored size %llu is larger than allowed size %llu", + static_cast(resumptionInfo.sharedSecret.length), + static_cast(sharedSecret.Capacity())); + return CHIP_ERROR_PERSISTED_STORAGE_FAILED; + } + + nodeID = resumptionInfo.nodeID.unsignedLongLongValue; + memcpy(resumptionId.data(), resumptionInfo.resumptionID.bytes, resumptionInfo.resumptionID.length); + sharedSecret.SetLength(resumptionInfo.sharedSecret.length); + memcpy(sharedSecret.Bytes(), resumptionInfo.sharedSecret.bytes, resumptionInfo.sharedSecret.length); + ReturnErrorOnFailure(SetToCATValues(resumptionInfo.caseAuthenticatedTags, peerCATs)); + + return CHIP_NO_ERROR; +} diff --git a/src/darwin/Framework/CHIP/Matter.h b/src/darwin/Framework/CHIP/Matter.h index 9a5ce73864f2c2..26c74cd80487d2 100644 --- a/src/darwin/Framework/CHIP/Matter.h +++ b/src/darwin/Framework/CHIP/Matter.h @@ -40,7 +40,9 @@ #import #import #import +#import #import +#import #import #import #import diff --git a/src/darwin/Framework/CHIPTests/MTRControllerTests.m b/src/darwin/Framework/CHIPTests/MTRControllerTests.m index e7a18bcb13804f..63a5baf32b64d5 100644 --- a/src/darwin/Framework/CHIPTests/MTRControllerTests.m +++ b/src/darwin/Framework/CHIPTests/MTRControllerTests.m @@ -1524,8 +1524,6 @@ - (void)testControllerCATs [controller shutdown]; XCTAssertFalse([controller isRunning]); - fprintf(stderr, "DOING TOO LONG TEST\n"); - // // Trying to bring up the same fabric with too-long CATs should fail, if we // are taking the provided CATs into account. @@ -1536,8 +1534,6 @@ - (void)testControllerCATs controller = [factory createControllerOnExistingFabric:params error:nil]; XCTAssertNil(controller); - fprintf(stderr, "DOING INVALID TEST\n"); - // // Trying to bring up the same fabric with invalid CATs should fail, if we // are taking the provided CATs into account. @@ -1545,12 +1541,9 @@ - (void)testControllerCATs params.nodeID = @(17); params.operationalKeypair = operationalKeys; params.caseAuthenticatedTags = invalidCATs; - fprintf(stderr, "BRINGING UP CONTROLLER\n"); controller = [factory createControllerOnExistingFabric:params error:nil]; - fprintf(stderr, "CONTROLLER SHOULD BE NIL\n"); XCTAssertNil(controller); - fprintf(stderr, "STOPPING FACTORY\n"); [factory stopControllerFactory]; XCTAssertFalse([factory isRunning]); } diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m new file mode 100644 index 00000000000000..82fa9fd07e8426 --- /dev/null +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -0,0 +1,1121 @@ +/* + * Copyright (c) 2022-2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import + +// system dependencies +#import + +#import "MTRErrorTestUtils.h" +#import "MTRFabricInfoChecker.h" +#import "MTRTestKeys.h" +#import "MTRTestResetCommissioneeHelper.h" +#import "MTRTestStorage.h" + +static const uint16_t kPairingTimeoutInSeconds = 10; +static const uint16_t kTimeoutInSeconds = 3; +static NSString * kOnboardingPayload = @"MT:-24J0AFN00KA0648G00"; +static const uint16_t kLocalPort = 5541; +static const uint16_t kTestVendorId = 0xFFF1u; + +@interface MTRPerControllerStorageTestsControllerDelegate : NSObject +@property (nonatomic, strong) XCTestExpectation * expectation; +@property (nonatomic, strong) NSNumber * deviceID; +@end + +@implementation MTRPerControllerStorageTestsControllerDelegate +- (id)initWithExpectation:(XCTestExpectation *)expectation newNodeID:(NSNumber *)newNodeID +{ + self = [super init]; + if (self) { + _expectation = expectation; + _deviceID = newNodeID; + } + return self; +} + +- (void)controller:(MTRDeviceController *)controller commissioningSessionEstablishmentDone:(NSError *)error +{ + XCTAssertEqual(error.code, 0); + + __auto_type * params = [[MTRCommissioningParameters alloc] init]; + + NSError * commissionError = nil; + [controller commissionNodeWithID:self.deviceID commissioningParams:params error:&commissionError]; + XCTAssertNil(commissionError); + + // Keep waiting for controller:commissioningComplete: +} + +- (void)controller:(MTRDeviceController *)controller commissioningComplete:(NSError *)error +{ + XCTAssertEqual(error.code, 0); + [_expectation fulfill]; + _expectation = nil; +} + +@end + +@interface MTRPerControllerStorageTestsStorageDelegate : NSObject +@property (nonatomic, readonly) NSMutableDictionary * storage; +@property (nonatomic, readonly) NSUUID * controllerID; +@end + +@implementation MTRPerControllerStorageTestsStorageDelegate + +- (instancetype)initWithControllerID:(NSUUID *)controllerID +{ + if (!(self = [super init])) { + return nil; + } + + _storage = [[NSMutableDictionary alloc] init]; + _controllerID = controllerID; + return self; +} + +- (nullable id)controller:(MTRDeviceController *)controller + valueForKey:(NSString *)key + securityLevel:(MTRStorageSecurityLevel)securityLevel + sharingType:(MTRStorageSharingType)sharingType +{ + XCTAssertEqualObjects(_controllerID, controller.UUID); + + __auto_type * data = self.storage[key]; + if (data == nil) { + return data; + } + + NSError * error; + id value = [NSKeyedUnarchiver unarchivedObjectOfClasses:MTRDeviceControllerStorageClasses() fromData:data error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(data); + + return value; +} + +- (BOOL)controller:(MTRDeviceController *)controller + storeValue:(id)value + forKey:(NSString *)key + securityLevel:(MTRStorageSecurityLevel)securityLevel + sharingType:(MTRStorageSharingType)sharingType +{ + XCTAssertEqualObjects(_controllerID, controller.UUID); + + __auto_type * archiver = [[NSKeyedArchiver alloc] initRequiringSecureCoding:YES]; + XCTAssertNotNil(archiver); + + NSError * error; + NSData * data = [NSKeyedArchiver archivedDataWithRootObject:value requiringSecureCoding:YES error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(data); + + self.storage[key] = data; + return YES; +} + +- (BOOL)controller:(MTRDeviceController *)controller + removeValueForKey:(NSString *)key + securityLevel:(MTRStorageSecurityLevel)securityLevel + sharingType:(MTRStorageSharingType)sharingType +{ + XCTAssertEqualObjects(_controllerID, controller.UUID); + self.storage[key] = nil; + return YES; +} + +@end + +@interface MTRPerControllerStorageTestsCertificateIssuer : NSObject +- (instancetype)initWithRootCertificate:(MTRCertificateDERBytes)rootCertificate + intermediateCertificate:(MTRCertificateDERBytes _Nullable)intermediateCertificate + signingKey:(id)signingKey + fabricID:(NSNumber *)fabricID; + +@property (nonatomic, readonly) MTRCertificateDERBytes rootCertificate; +@property (nonatomic, readonly, nullable) MTRCertificateDERBytes intermediateCertificate; +@property (nonatomic, readonly) id signingKey; +@property (nonatomic, readonly) NSNumber * fabricID; + +// The node ID to use for the next operational certificate we issue. This will +// be set to null after every certificate issuance. +@property (nonatomic, nullable) NSNumber * nextNodeID; + +@property (nonatomic, readonly) BOOL shouldSkipAttestationCertificateValidation; + +@end + +@implementation MTRPerControllerStorageTestsCertificateIssuer + +- (instancetype)initWithRootCertificate:(MTRCertificateDERBytes)rootCertificate + intermediateCertificate:(MTRCertificateDERBytes _Nullable)intermediateCertificate + signingKey:(id)signingKey + fabricID:(NSNumber *)fabricID +{ + if (!(self = [super init])) { + return nil; + } + + _rootCertificate = rootCertificate; + _intermediateCertificate = intermediateCertificate; + _signingKey = signingKey; + _fabricID = fabricID; + _nextNodeID = nil; + _shouldSkipAttestationCertificateValidation = NO; + + return self; +} + +- (void)issueOperationalCertificateForRequest:(MTROperationalCSRInfo *)csrInfo + attestationInfo:(MTRDeviceAttestationInfo *)attestationInfo + controller:(MTRDeviceController *)controller + completion:(void (^)(MTROperationalCertificateChain * _Nullable info, + NSError * _Nullable error))completion +{ + if (self.nextNodeID == nil) { + completion(nil, [NSError errorWithDomain:@"TestError" code:0 userInfo:@{ @"reason" : @"nextNodeID is nil" }]); + return; + } + + MTRCertificateDERBytes signingCertificate; + if (self.intermediateCertificate != nil) { + signingCertificate = self.intermediateCertificate; + } else { + signingCertificate = self.rootCertificate; + } + + __auto_type * csr = csrInfo.csr; + XCTAssertNotNil(csr); + + NSError * error; + __auto_type * rawPublicKey = [MTRCertificates publicKeyFromCSR:csr error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(rawPublicKey); + + if (error != nil) { + completion(nil, error); + return; + } + + NSDictionary * attributes = @{ + (__bridge NSString *) kSecAttrKeyType : (__bridge NSString *) kSecAttrKeyTypeECSECPrimeRandom, + (__bridge NSString *) kSecAttrKeyClass : (__bridge NSString *) kSecAttrKeyClassPublic + }; + CFErrorRef keyCreationError = NULL; + SecKeyRef publicKey + = SecKeyCreateWithData((__bridge CFDataRef) rawPublicKey, (__bridge CFDictionaryRef) attributes, &keyCreationError); + XCTAssertNil((__bridge id) keyCreationError); + XCTAssertNotNil((__bridge id) publicKey); + + __auto_type * operationalCert = [MTRCertificates createOperationalCertificate:self.signingKey + signingCertificate:signingCertificate + operationalPublicKey:publicKey + fabricID:self.fabricID + nodeID:self.nextNodeID + caseAuthenticatedTags:nil + error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(operationalCert); + + if (error != nil) { + completion(nil, error); + return; + } + + __auto_type * certChain = [[MTROperationalCertificateChain alloc] initWithOperationalCertificate:operationalCert + intermediateCertificate:self.intermediateCertificate + rootCertificate:self.rootCertificate + adminSubject:nil]; + completion(certChain, nil); +} + +@end + +@interface MTRPerControllerStorageTests : XCTestCase +@end + +@implementation MTRPerControllerStorageTests + ++ (void)tearDown +{ +} + +- (void)setUp +{ + // Per-test setup, runs before each test. + [super setUp]; + [self setContinueAfterFailure:NO]; + + [self startFactory]; +} + +- (void)tearDown +{ + // Per-test teardown, runs after each test. + [self stopFactory]; + [super tearDown]; +} + +- (void)startFactory +{ + __auto_type * factory = [MTRDeviceControllerFactory sharedInstance]; + XCTAssertNotNil(factory); + + __auto_type * factoryParams = [[MTRDeviceControllerFactoryParams alloc] init]; + factoryParams.port = @(kLocalPort); + + NSError * error; + BOOL ok = [factory startControllerFactory:factoryParams error:&error]; + XCTAssertNil(error); + XCTAssertTrue(ok); +} + +- (void)stopFactory +{ + __auto_type * factory = [MTRDeviceControllerFactory sharedInstance]; + [factory stopControllerFactory]; + XCTAssertFalse(factory.isRunning); +} + +// Test helpers + +- (void)commissionWithController:(MTRDeviceController *)controller newNodeID:(NSNumber *)newNodeID +{ + XCTestExpectation * expectation = [self expectationWithDescription:@"Pairing Complete"]; + + __auto_type * deviceControllerDelegate = [[MTRPerControllerStorageTestsControllerDelegate alloc] initWithExpectation:expectation + newNodeID:newNodeID]; + dispatch_queue_t callbackQueue = dispatch_queue_create("com.chip.device_controller_delegate", DISPATCH_QUEUE_SERIAL); + + [controller setDeviceControllerDelegate:deviceControllerDelegate queue:callbackQueue]; + + NSError * error; + __auto_type * payload = [MTRSetupPayload setupPayloadWithOnboardingPayload:kOnboardingPayload error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(payload); + + [controller setupCommissioningSessionWithPayload:payload newNodeID:newNodeID error:&error]; + XCTAssertNil(error); + + [self waitForExpectations:@[ expectation ] timeout:kPairingTimeoutInSeconds]; +} + +- (nullable MTRDeviceController *)startControllerWithRootKeys:(MTRTestKeys *)rootKeys + operationalKeys:(MTRTestKeys *)operationalKeys + fabricID:(NSNumber *)fabricID + nodeID:(NSNumber *)nodeID + storage:(MTRPerControllerStorageTestsStorageDelegate *)storage + caseAuthenticatedTags:(nullable NSSet *)caseAuthenticatedTags + error:(NSError * __autoreleasing *)error + certificateIssuer: + (MTRPerControllerStorageTestsCertificateIssuer * __autoreleasing *)certificateIssuer +{ + XCTAssertTrue(error != NULL); + + __auto_type * factory = [MTRDeviceControllerFactory sharedInstance]; + XCTAssertNotNil(factory); + + // Specify a fixed issuerID, so we get the same cert if we use the same keys. + __auto_type * root = [MTRCertificates createRootCertificate:rootKeys issuerID:@(1) fabricID:nil error:error]; + XCTAssertNil(*error); + XCTAssertNotNil(root); + + __auto_type * operational = [MTRCertificates createOperationalCertificate:rootKeys + signingCertificate:root + operationalPublicKey:operationalKeys.publicKey + fabricID:fabricID + nodeID:nodeID + caseAuthenticatedTags:caseAuthenticatedTags + error:error]; + XCTAssertNil(*error); + XCTAssertNotNil(operational); + + __auto_type * params = + [[MTRDeviceControllerExternalCertificateStartupParameters alloc] initWithStorageDelegate:storage + UUID:storage.controllerID + ipk:rootKeys.ipk + vendorID:@(kTestVendorId) + operationalKeypair:operationalKeys + operationalCertificate:operational + intermediateCertificate:nil + rootCertificate:root]; + XCTAssertNotNil(params); + + __auto_type * ourCertificateIssuer = [[MTRPerControllerStorageTestsCertificateIssuer alloc] initWithRootCertificate:root + intermediateCertificate:nil + signingKey:rootKeys + fabricID:fabricID]; + XCTAssertNotNil(ourCertificateIssuer); + + if (certificateIssuer) { + *certificateIssuer = ourCertificateIssuer; + } + + [params setOperationalCertificateIssuer:ourCertificateIssuer queue:dispatch_get_main_queue()]; + + return [factory createController:params error:error]; +} + +- (nullable MTRDeviceController *)startControllerWithRootKeys:(MTRTestKeys *)rootKeys + operationalKeys:(MTRTestKeys *)operationalKeys + fabricID:(NSNumber *)fabricID + nodeID:(NSNumber *)nodeID + storage:(MTRPerControllerStorageTestsStorageDelegate *)storage + error:(NSError * __autoreleasing *)error + certificateIssuer: + (MTRPerControllerStorageTestsCertificateIssuer * __autoreleasing *)certificateIssuer +{ + return [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID + storage:storage + caseAuthenticatedTags:nil + error:error + certificateIssuer:certificateIssuer]; +} + +- (nullable MTRDeviceController *)startControllerWithRootKeys:(MTRTestKeys *)rootKeys + operationalKeys:(MTRTestKeys *)operationalKeys + fabricID:(NSNumber *)fabricID + nodeID:(NSNumber *)nodeID + storage:(MTRPerControllerStorageTestsStorageDelegate *)storage + caseAuthenticatedTags:(nullable NSSet *)caseAuthenticatedTags + error:(NSError * __autoreleasing *)error +{ + return [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID + storage:storage + caseAuthenticatedTags:caseAuthenticatedTags + error:error + certificateIssuer:nil]; +} + +- (nullable MTRDeviceController *)startControllerWithRootKeys:(MTRTestKeys *)rootKeys + operationalKeys:(MTRTestKeys *)operationalKeys + fabricID:(NSNumber *)fabricID + nodeID:(NSNumber *)nodeID + storage:(MTRPerControllerStorageTestsStorageDelegate *)storage + error:(NSError * __autoreleasing *)error +{ + return [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID + storage:storage + error:error + certificateIssuer:nil]; +} + +- (void)test001_BasicControllerStartup +{ + __auto_type * factory = [MTRDeviceControllerFactory sharedInstance]; + XCTAssertNotNil(factory); + + __auto_type * rootKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(rootKeys); + + __auto_type * operationalKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(operationalKeys); + + __auto_type * storageDelegate = [[MTRPerControllerStorageTestsStorageDelegate alloc] initWithControllerID:[NSUUID UUID]]; + + NSNumber * nodeID = @(123); + NSNumber * fabricID = @(456); + + NSError * error; + MTRDeviceController * controller = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID + storage:storageDelegate + error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(controller); + XCTAssertTrue([controller isRunning]); + + XCTAssertEqualObjects(controller.controllerNodeID, nodeID); + + // This was the first controller we brought up, so it should have come up + // with fabric index 1. + __auto_type * fabricInfoList = factory.knownFabrics; + CheckFabricInfo(fabricInfoList, [NSMutableSet setWithArray:@[ + @{ + @"rootPublicKey" : rootKeys.publicKeyData, + @"vendorID" : @(kTestVendorId), + @"fabricID" : fabricID, + @"nodeID" : nodeID, + @"label" : @"", + @"hasIntermediateCertificate" : @(NO), + @"fabricIndex" : @(1), + }, + ]]); + + [controller shutdown]; + XCTAssertFalse([controller isRunning]); +} + +- (void)test002_TryStartingTwoControllersWithSameNodeID +{ + __auto_type * rootKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(rootKeys); + + __auto_type * operationalKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(operationalKeys); + + __auto_type * storageDelegate = [[MTRPerControllerStorageTestsStorageDelegate alloc] initWithControllerID:[NSUUID UUID]]; + + NSNumber * nodeID = @(123); + NSNumber * fabricID = @(456); + + NSError * error; + MTRDeviceController * controller = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID + storage:storageDelegate + error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(controller); + XCTAssertTrue([controller isRunning]); + + XCTAssertEqualObjects(controller.controllerNodeID, nodeID); + + // Try to bring up another controller with the same identity. This should + // fail, since our controller is still running + MTRDeviceController * otherController = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID + storage:storageDelegate + error:&error]; + XCTAssertNil(otherController); + XCTAssertNotNil(error); + + // Our controller should still be running. + XCTAssertTrue([controller isRunning]); + + [controller shutdown]; + XCTAssertFalse([controller isRunning]); +} + +- (void)test003_TestTwoControllersSameUUID +{ + __auto_type * rootKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(rootKeys); + + __auto_type * operationalKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(operationalKeys); + XCTAssertEqual(operationalKeys.signatureCount, 0); + + __auto_type * storageDelegate = [[MTRPerControllerStorageTestsStorageDelegate alloc] initWithControllerID:[NSUUID UUID]]; + + NSNumber * fabricID = @(456); + + NSNumber * nodeID1 = @(123); + + NSError * error; + MTRDeviceController * controller = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID1 + storage:storageDelegate + error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(controller); + XCTAssertTrue([controller isRunning]); + + XCTAssertEqualObjects(controller.controllerNodeID, nodeID1); + + // Try to bring up another controller with the same UUID (but a different + // node identity). This should fail, since our controller is still running. + NSNumber * nodeID2 = @(789); + MTRDeviceController * otherController = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID2 + storage:storageDelegate + error:&error]; + XCTAssertNil(otherController); + XCTAssertNotNil(error); + + // Our controller should still be running. + XCTAssertTrue([controller isRunning]); + + [controller shutdown]; + XCTAssertFalse([controller isRunning]); +} + +- (void)test004_TestBasicSessionResumption +{ + __auto_type * factory = [MTRDeviceControllerFactory sharedInstance]; + XCTAssertNotNil(factory); + + __auto_type queue = dispatch_get_main_queue(); + + __auto_type * rootKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(rootKeys); + + __auto_type * operationalKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(operationalKeys); + XCTAssertEqual(operationalKeys.signatureCount, 0); + + __auto_type * storageDelegate = [[MTRPerControllerStorageTestsStorageDelegate alloc] initWithControllerID:[NSUUID UUID]]; + + NSNumber * nodeID = @(123); + NSNumber * fabricID = @(456); + + NSError * error; + MTRPerControllerStorageTestsCertificateIssuer * certificateIssuer; + MTRDeviceController * controller = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID + storage:storageDelegate + error:&error + certificateIssuer:&certificateIssuer]; + XCTAssertNil(error); + XCTAssertNotNil(controller); + XCTAssertTrue([controller isRunning]); + + XCTAssertEqualObjects(controller.controllerNodeID, nodeID); + + // Now commission the device, to test that that works. + NSNumber * deviceID = @(17); + certificateIssuer.nextNodeID = deviceID; + [self commissionWithController:controller newNodeID:deviceID]; + + // We should have established CASE using our operational key. + XCTAssertEqual(operationalKeys.signatureCount, 1); + + [controller shutdown]; + XCTAssertFalse([controller isRunning]); + + // Start the controller again using the same identity. This should work, + // because we cleared out the fabric info when the controller shut down. + controller = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID + storage:storageDelegate + error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(controller); + XCTAssertTrue([controller isRunning]); + + XCTAssertEqualObjects(controller.controllerNodeID, nodeID); + + // Now we should have come up with fabric index 2. + __auto_type * fabricInfoList = factory.knownFabrics; + CheckFabricInfo(fabricInfoList, [NSMutableSet setWithArray:@[ + @{ + @"rootPublicKey" : rootKeys.publicKeyData, + @"vendorID" : @(kTestVendorId), + @"fabricID" : fabricID, + @"nodeID" : nodeID, + @"label" : @"", + @"hasIntermediateCertificate" : @(NO), + @"fabricIndex" : @(2), + }, + ]]); + + // Try sending an attribute read and make sure it works. + __auto_type * device = [MTRBaseDevice deviceWithNodeID:deviceID controller:controller]; + __auto_type * onOffCluster = [[MTRBaseClusterOnOff alloc] initWithDevice:device endpointID:@(1) queue:queue]; + __auto_type * readExpectation = [self expectationWithDescription:@"Read OnOff attribute"]; + [onOffCluster readAttributeOnOffWithCompletion:^(NSNumber * value, NSError * _Nullable error) { + XCTAssertNil(error); + // We expect the device to be off. + XCTAssertEqualObjects(value, @(0)); + [readExpectation fulfill]; + }]; + + [self waitForExpectations:@[ readExpectation ] timeout:kTimeoutInSeconds]; + + // We should have done CASE resumption, so not done any new signing using + // our keys. + XCTAssertEqual(operationalKeys.signatureCount, 1); + + // Reset our commissionee. + ResetCommissionee(device, queue, self, kTimeoutInSeconds); + + [controller shutdown]; + XCTAssertFalse([controller isRunning]); +} + +- (void)test005_TestSessionResumptionDataClearingNodeIDChanged +{ + __auto_type * factory = [MTRDeviceControllerFactory sharedInstance]; + XCTAssertNotNil(factory); + + __auto_type queue = dispatch_get_main_queue(); + + __auto_type * rootKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(rootKeys); + + __auto_type * operationalKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(operationalKeys); + XCTAssertEqual(operationalKeys.signatureCount, 0); + + __auto_type * storageDelegate = [[MTRPerControllerStorageTestsStorageDelegate alloc] initWithControllerID:[NSUUID UUID]]; + + NSNumber * nodeID1 = @(123); + NSNumber * nodeID2 = @(246); + NSNumber * fabricID = @(456); + + NSError * error; + MTRPerControllerStorageTestsCertificateIssuer * certificateIssuer; + MTRDeviceController * controller = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID1 + storage:storageDelegate + error:&error + certificateIssuer:&certificateIssuer]; + XCTAssertNil(error); + XCTAssertNotNil(controller); + XCTAssertTrue([controller isRunning]); + + XCTAssertEqualObjects(controller.controllerNodeID, nodeID1); + + NSNumber * deviceID = @(17); + certificateIssuer.nextNodeID = deviceID; + [self commissionWithController:controller newNodeID:deviceID]; + + // We should have established CASE using our operational key. + XCTAssertEqual(operationalKeys.signatureCount, 1); + + __auto_type * device1 = [MTRBaseDevice deviceWithNodeID:deviceID controller:controller]; + + // Now change ACLs so that nodeID2 has access and nodeID1 does not. + __auto_type * admin = [[MTRAccessControlClusterAccessControlEntryStruct alloc] init]; + admin.privilege = @(MTRAccessControlEntryPrivilegeAdminister); + admin.authMode = @(MTRAccessControlEntryAuthModeCASE); + admin.subjects = @[ nodeID2 ]; + + __auto_type * aclCluster = [[MTRBaseClusterAccessControl alloc] initWithDevice:device1 endpointID:@(0) queue:queue]; + + XCTestExpectation * aclWriteExpectation = [self expectationWithDescription:@"ACLs changed so new node ID can administer"]; + [aclCluster writeAttributeACLWithValue:@[ admin ] + completion:^(NSError * _Nullable err) { + XCTAssertNil(err); + [aclWriteExpectation fulfill]; + }]; + + [self waitForExpectations:@[ aclWriteExpectation ] timeout:kTimeoutInSeconds]; + + [controller shutdown]; + XCTAssertFalse([controller isRunning]); + + // There should have been no more CASE establishment going on. + XCTAssertEqual(operationalKeys.signatureCount, 1); + + // Bring up a controller with the same storage and keys and so on but nodeID2. + controller = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID2 + storage:storageDelegate + error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(controller); + XCTAssertTrue([controller isRunning]); + + XCTAssertEqualObjects(controller.controllerNodeID, nodeID2); + + // Try sending an attribute read and make sure it works. + __auto_type * device2 = [MTRBaseDevice deviceWithNodeID:deviceID controller:controller]; + __auto_type * onOffCluster = [[MTRBaseClusterOnOff alloc] initWithDevice:device2 endpointID:@(1) queue:queue]; + __auto_type * readExpectation = [self expectationWithDescription:@"Read OnOff attribute"]; + [onOffCluster readAttributeOnOffWithCompletion:^(NSNumber * value, NSError * _Nullable error) { + XCTAssertNil(error); + // We expect the device to be off. + XCTAssertEqualObjects(value, @(0)); + [readExpectation fulfill]; + }]; + + [self waitForExpectations:@[ readExpectation ] timeout:kTimeoutInSeconds]; + + // We should note have done CASE resumption, since our identity changed. + XCTAssertEqual(operationalKeys.signatureCount, 2); + + // Reset our commissionee. + ResetCommissionee(device2, queue, self, kTimeoutInSeconds); + + [controller shutdown]; + XCTAssertFalse([controller isRunning]); +} + +- (void)test006_TestSessionResumptionDataClearingCATsChanged +{ + __auto_type * factory = [MTRDeviceControllerFactory sharedInstance]; + XCTAssertNotNil(factory); + + __auto_type queue = dispatch_get_main_queue(); + + __auto_type * rootKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(rootKeys); + + __auto_type * operationalKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(operationalKeys); + XCTAssertEqual(operationalKeys.signatureCount, 0); + + __auto_type * storageDelegate = [[MTRPerControllerStorageTestsStorageDelegate alloc] initWithControllerID:[NSUUID UUID]]; + + NSNumber * nodeID = @(123); + NSNumber * fabricID = @(456); + + NSError * error; + MTRPerControllerStorageTestsCertificateIssuer * certificateIssuer; + MTRDeviceController * controller = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID + storage:storageDelegate + error:&error + certificateIssuer:&certificateIssuer]; + XCTAssertNil(error); + XCTAssertNotNil(controller); + XCTAssertTrue([controller isRunning]); + + XCTAssertEqualObjects(controller.controllerNodeID, nodeID); + + NSNumber * deviceID = @(17); + certificateIssuer.nextNodeID = deviceID; + [self commissionWithController:controller newNodeID:deviceID]; + + // We should have established CASE using our operational key. + XCTAssertEqual(operationalKeys.signatureCount, 1); + + __auto_type * device1 = [MTRBaseDevice deviceWithNodeID:deviceID controller:controller]; + + // Now change ACLs so that CAT 0x12340001 has access and nodeID does not. + uint32_t cat = 0x12340001; + NSNumber * catSubject = @(0xFFFFFFFD00000000 | cat); + __auto_type * admin = [[MTRAccessControlClusterAccessControlEntryStruct alloc] init]; + admin.privilege = @(MTRAccessControlEntryPrivilegeAdminister); + admin.authMode = @(MTRAccessControlEntryAuthModeCASE); + admin.subjects = @[ catSubject ]; + + __auto_type * aclCluster = [[MTRBaseClusterAccessControl alloc] initWithDevice:device1 endpointID:@(0) queue:queue]; + + XCTestExpectation * aclWriteExpectation = [self expectationWithDescription:@"ACLs changed so new node ID can administer"]; + [aclCluster writeAttributeACLWithValue:@[ admin ] + completion:^(NSError * _Nullable err) { + XCTAssertNil(err); + [aclWriteExpectation fulfill]; + }]; + + [self waitForExpectations:@[ aclWriteExpectation ] timeout:kTimeoutInSeconds]; + + [controller shutdown]; + XCTAssertFalse([controller isRunning]); + + // There should have been no more CASE establishment going on. + XCTAssertEqual(operationalKeys.signatureCount, 1); + + // Bring up a controller with the same storage and keys and so on but using + // our new CAT + controller = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID + storage:storageDelegate + caseAuthenticatedTags:[NSSet setWithArray:@[ @(cat) ]] + error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(controller); + XCTAssertTrue([controller isRunning]); + + XCTAssertEqualObjects(controller.controllerNodeID, nodeID); + + // Try sending an attribute read and make sure it works. + __auto_type * device2 = [MTRBaseDevice deviceWithNodeID:deviceID controller:controller]; + __auto_type * onOffCluster = [[MTRBaseClusterOnOff alloc] initWithDevice:device2 endpointID:@(1) queue:queue]; + __auto_type * readExpectation = [self expectationWithDescription:@"Read OnOff attribute"]; + [onOffCluster readAttributeOnOffWithCompletion:^(NSNumber * value, NSError * _Nullable error) { + XCTAssertNil(error); + // We expect the device to be off. + XCTAssertEqualObjects(value, @(0)); + [readExpectation fulfill]; + }]; + + [self waitForExpectations:@[ readExpectation ] timeout:kTimeoutInSeconds]; + + // We should note have done CASE resumption, since our CATs changed. + XCTAssertEqual(operationalKeys.signatureCount, 2); + + // Reset our commissionee. + ResetCommissionee(device2, queue, self, kTimeoutInSeconds); + + [controller shutdown]; + XCTAssertFalse([controller isRunning]); +} + +- (void)test007_TestMultipleControllers +{ + __auto_type * factory = [MTRDeviceControllerFactory sharedInstance]; + XCTAssertNotNil(factory); + + __auto_type * rootKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(rootKeys); + + __auto_type * operationalKeys = [[MTRTestKeys alloc] init]; + XCTAssertNotNil(operationalKeys); + XCTAssertEqual(operationalKeys.signatureCount, 0); + + NSNumber * nodeID1 = @(123); + NSNumber * nodeID2 = @(456); + NSNumber * fabricID1 = @(1); + NSNumber * fabricID2 = @(2); + + __auto_type * storageDelegate1 = [[MTRPerControllerStorageTestsStorageDelegate alloc] initWithControllerID:[NSUUID UUID]]; + + // Start several controllers that have distinct identities but share some + // node/fabric IDs. + NSError * error; + MTRPerControllerStorageTestsCertificateIssuer * certificateIssuer; + MTRDeviceController * controller1 = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID1 + nodeID:nodeID1 + storage:storageDelegate1 + error:&error + certificateIssuer:&certificateIssuer]; + XCTAssertNil(error); + XCTAssertNotNil(controller1); + XCTAssertTrue([controller1 isRunning]); + + __auto_type * storageDelegate2 = [[MTRPerControllerStorageTestsStorageDelegate alloc] initWithControllerID:[NSUUID UUID]]; + MTRDeviceController * controller2 = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID1 + nodeID:nodeID2 + storage:storageDelegate2 + error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(controller2); + XCTAssertTrue([controller2 isRunning]); + + __auto_type * storageDelegate3 = [[MTRPerControllerStorageTestsStorageDelegate alloc] initWithControllerID:[NSUUID UUID]]; + MTRDeviceController * controller3 = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID2 + nodeID:nodeID1 + storage:storageDelegate3 + error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(controller3); + XCTAssertTrue([controller3 isRunning]); + + // Now check our fabric table + __auto_type * fabricInfoList = factory.knownFabrics; + CheckFabricInfo(fabricInfoList, [NSMutableSet setWithArray:@[ + @{ + @"rootPublicKey" : rootKeys.publicKeyData, + @"vendorID" : @(kTestVendorId), + @"fabricID" : fabricID1, + @"nodeID" : nodeID1, + @"label" : @"", + @"hasIntermediateCertificate" : @(NO), + @"fabricIndex" : @(1), + }, + @{ + @"rootPublicKey" : rootKeys.publicKeyData, + @"vendorID" : @(kTestVendorId), + @"fabricID" : fabricID1, + @"nodeID" : nodeID2, + @"label" : @"", + @"hasIntermediateCertificate" : @(NO), + @"fabricIndex" : @(2), + }, + @{ + @"rootPublicKey" : rootKeys.publicKeyData, + @"vendorID" : @(kTestVendorId), + @"fabricID" : fabricID2, + @"nodeID" : nodeID1, + @"label" : @"", + @"hasIntermediateCertificate" : @(NO), + @"fabricIndex" : @(3), + }, + ]]); + + // Restart controller2 + [controller2 shutdown]; + XCTAssertFalse([controller2 isRunning]); + controller2 = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID1 + nodeID:nodeID2 + storage:storageDelegate2 + error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(controller2); + XCTAssertTrue([controller2 isRunning]); + + // Now check our fabric table again. + fabricInfoList = factory.knownFabrics; + CheckFabricInfo(fabricInfoList, [NSMutableSet setWithArray:@[ + @{ + @"rootPublicKey" : rootKeys.publicKeyData, + @"vendorID" : @(kTestVendorId), + @"fabricID" : fabricID1, + @"nodeID" : nodeID1, + @"label" : @"", + @"hasIntermediateCertificate" : @(NO), + @"fabricIndex" : @(1), + }, + @{ + @"rootPublicKey" : rootKeys.publicKeyData, + @"vendorID" : @(kTestVendorId), + @"fabricID" : fabricID1, + @"nodeID" : nodeID2, + @"label" : @"", + @"hasIntermediateCertificate" : @(NO), + @"fabricIndex" : @(4), + }, + @{ + @"rootPublicKey" : rootKeys.publicKeyData, + @"vendorID" : @(kTestVendorId), + @"fabricID" : fabricID2, + @"nodeID" : nodeID1, + @"label" : @"", + @"hasIntermediateCertificate" : @(NO), + @"fabricIndex" : @(3), + }, + ]]); + + // Now commission the device from controller1 + NSNumber * deviceID = @(17); + certificateIssuer.nextNodeID = deviceID; + [self commissionWithController:controller1 newNodeID:deviceID]; + + // We should have established CASE using our operational key. + XCTAssertEqual(operationalKeys.signatureCount, 1); + + // Ensure that controller2 does not have the same node ID as controller1. + XCTAssertNotEqualObjects(controller1.controllerNodeID, controller2.controllerNodeID); + + __auto_type * device1 = [MTRBaseDevice deviceWithNodeID:deviceID controller:controller1]; + __auto_type * device2 = [MTRBaseDevice deviceWithNodeID:deviceID controller:controller2]; + + dispatch_queue_t queue = dispatch_get_main_queue(); + __auto_type * onOff1 = [[MTRBaseClusterOnOff alloc] initWithDevice:device1 endpointID:@(1) queue:queue]; + __auto_type * onOff2 = [[MTRBaseClusterOnOff alloc] initWithDevice:device2 endpointID:@(1) queue:queue]; + + // Check that device1 can read the On/Off attribute + XCTestExpectation * canReadExpectation1 = [self expectationWithDescription:@"Initial commissioner can read on/off"]; + [onOff1 readAttributeOnOffWithCompletion:^(NSNumber * _Nullable value, NSError * _Nullable err) { + XCTAssertNil(err); + XCTAssertEqualObjects(value, @(0)); + [canReadExpectation1 fulfill]; + }]; + + [self waitForExpectations:@[ canReadExpectation1 ] timeout:kTimeoutInSeconds]; + + // Check that device2 cannot read the On/Off attribute due to missing ACLs. + XCTestExpectation * cantReadExpectation1 = [self expectationWithDescription:@"New node can't read on/off yet"]; + [onOff2 readAttributeOnOffWithCompletion:^(NSNumber * _Nullable value, NSError * _Nullable err) { + XCTAssertNil(value); + XCTAssertNotNil(err); + XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:err], MTRInteractionErrorCodeUnsupportedAccess); + [cantReadExpectation1 fulfill]; + }]; + + [self waitForExpectations:@[ cantReadExpectation1 ] timeout:kTimeoutInSeconds]; + + // Now change ACLs so that device2 can read. + __auto_type * admin1 = [[MTRAccessControlClusterAccessControlEntryStruct alloc] init]; + admin1.privilege = @(MTRAccessControlEntryPrivilegeAdminister); + admin1.authMode = @(MTRAccessControlEntryAuthModeCASE); + admin1.subjects = @[ controller1.controllerNodeID ]; + + __auto_type * admin2 = [[MTRAccessControlClusterAccessControlEntryStruct alloc] init]; + admin2.privilege = @(MTRAccessControlEntryPrivilegeAdminister); + admin2.authMode = @(MTRAccessControlEntryAuthModeCASE); + admin2.subjects = @[ controller2.controllerNodeID ]; + + __auto_type * acl1 = [[MTRBaseClusterAccessControl alloc] initWithDevice:device1 endpointID:@(0) queue:queue]; + + XCTestExpectation * let2ReadExpectation = [self expectationWithDescription:@"ACLs changed so new node can read"]; + [acl1 writeAttributeACLWithValue:@[ admin1, admin2 ] + completion:^(NSError * _Nullable err) { + XCTAssertNil(err); + [let2ReadExpectation fulfill]; + }]; + + [self waitForExpectations:@[ let2ReadExpectation ] timeout:kTimeoutInSeconds]; + + // Check that device2 can read the On/Off attribute + XCTestExpectation * canReadExpectation2 = [self expectationWithDescription:@"New node can read on/off"]; + [onOff2 readAttributeOnOffWithCompletion:^(NSNumber * _Nullable value, NSError * _Nullable err) { + XCTAssertNil(err); + XCTAssertEqualObjects(value, @(0)); + [canReadExpectation2 fulfill]; + }]; + + [self waitForExpectations:@[ canReadExpectation2 ] timeout:kTimeoutInSeconds]; + + // Check that device1 can still read the On/Off attribute + XCTestExpectation * canReadExpectation3 = [self expectationWithDescription:@"Initial commissioner can still read on/off"]; + [onOff1 readAttributeOnOffWithCompletion:^(NSNumber * _Nullable value, NSError * _Nullable err) { + XCTAssertNil(err); + XCTAssertEqualObjects(value, @(0)); + [canReadExpectation3 fulfill]; + }]; + + [self waitForExpectations:@[ canReadExpectation3 ] timeout:kTimeoutInSeconds]; + + // Check that the two devices are running on the same fabric. + __auto_type * opCreds1 = [[MTRBaseClusterOperationalCredentials alloc] initWithDevice:device1 endpoint:0 queue:queue]; + __auto_type * opCreds2 = [[MTRBaseClusterOperationalCredentials alloc] initWithDevice:device2 endpoint:0 queue:queue]; + + __block NSNumber * fabricIndex; + XCTestExpectation * readFabricIndexExpectation1 = + [self expectationWithDescription:@"Fabric index read by initial commissioner"]; + [opCreds1 readAttributeCurrentFabricIndexWithCompletion:^(NSNumber * _Nullable value, NSError * _Nullable readError) { + XCTAssertNil(readError); + XCTAssertNotNil(value); + fabricIndex = value; + [readFabricIndexExpectation1 fulfill]; + }]; + + [self waitForExpectations:@[ readFabricIndexExpectation1 ] timeout:kTimeoutInSeconds]; + + XCTestExpectation * readFabricIndexExpectation2 = [self expectationWithDescription:@"Fabric index read by new node"]; + [opCreds2 readAttributeCurrentFabricIndexWithCompletion:^(NSNumber * _Nullable value, NSError * _Nullable readError) { + XCTAssertNil(readError); + XCTAssertNotNil(value); + XCTAssertEqualObjects(value, fabricIndex); + [readFabricIndexExpectation2 fulfill]; + }]; + + [self waitForExpectations:@[ readFabricIndexExpectation2 ] timeout:kTimeoutInSeconds]; + + // Reset our commissionee. + ResetCommissionee(device1, queue, self, kTimeoutInSeconds); + + [controller1 shutdown]; + XCTAssertFalse([controller1 isRunning]); + [controller2 shutdown]; + XCTAssertFalse([controller2 isRunning]); + [controller3 shutdown]; + XCTAssertFalse([controller3 isRunning]); +} + +@end diff --git a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj b/src/darwin/Framework/Matter.xcodeproj/project.pbxproj index 5481acdf5ea66a..1520c2a9d12082 100644 --- a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj +++ b/src/darwin/Framework/Matter.xcodeproj/project.pbxproj @@ -153,7 +153,13 @@ 51431AF927D2973E008A7943 /* MTRIMDispatch.mm in Sources */ = {isa = PBXBuildFile; fileRef = 51431AF827D2973E008A7943 /* MTRIMDispatch.mm */; }; 51431AFB27D29CA4008A7943 /* ota-provider.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 51431AFA27D29CA4008A7943 /* ota-provider.cpp */; }; 5143851E2A65885500EDC8E6 /* MTRSwiftPairingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5143851D2A65885500EDC8E6 /* MTRSwiftPairingTests.swift */; }; + 514654492A72F9DF00904E61 /* MTRDemuxingStorage.mm in Sources */ = {isa = PBXBuildFile; fileRef = 514654482A72F9DF00904E61 /* MTRDemuxingStorage.mm */; }; + 5146544B2A72F9F500904E61 /* MTRDemuxingStorage.h in Headers */ = {isa = PBXBuildFile; fileRef = 5146544A2A72F9F500904E61 /* MTRDemuxingStorage.h */; }; 51565CAE2A79D42100469F18 /* MTRConversion.mm in Sources */ = {isa = PBXBuildFile; fileRef = 51565CAD2A79D42100469F18 /* MTRConversion.mm */; }; + 51565CB12A7AD77600469F18 /* MTRDeviceControllerDataStore.h in Headers */ = {isa = PBXBuildFile; fileRef = 51565CAF2A7AD77600469F18 /* MTRDeviceControllerDataStore.h */; }; + 51565CB22A7AD77600469F18 /* MTRDeviceControllerDataStore.mm in Sources */ = {isa = PBXBuildFile; fileRef = 51565CB02A7AD77600469F18 /* MTRDeviceControllerDataStore.mm */; }; + 51565CB42A7AD78D00469F18 /* MTRDeviceControllerStorageDelegate.h in Headers */ = {isa = PBXBuildFile; fileRef = 51565CB32A7AD78D00469F18 /* MTRDeviceControllerStorageDelegate.h */; settings = {ATTRIBUTES = (Public, ); }; }; + 51565CB62A7B0D6600469F18 /* MTRDeviceControllerStartupParameters.h in Headers */ = {isa = PBXBuildFile; fileRef = 51565CB52A7B0D6600469F18 /* MTRDeviceControllerStartupParameters.h */; settings = {ATTRIBUTES = (Public, ); }; }; 515C1C6F284F9FFB00A48F0C /* MTRFramework.mm in Sources */ = {isa = PBXBuildFile; fileRef = 515C1C6D284F9FFB00A48F0C /* MTRFramework.mm */; }; 515C1C70284F9FFB00A48F0C /* MTRFramework.h in Headers */ = {isa = PBXBuildFile; fileRef = 515C1C6E284F9FFB00A48F0C /* MTRFramework.h */; }; 51669AF02913204400F4AA36 /* MTRBackwardsCompatTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 51669AEF2913204400F4AA36 /* MTRBackwardsCompatTests.m */; }; @@ -182,6 +188,9 @@ 51E51FBF282AD37A00FC978D /* MTRDeviceControllerStartupParams.h in Headers */ = {isa = PBXBuildFile; fileRef = 51E51FBC282AD37A00FC978D /* MTRDeviceControllerStartupParams.h */; settings = {ATTRIBUTES = (Public, ); }; }; 51E51FC0282AD37A00FC978D /* MTRDeviceControllerStartupParams_Internal.h in Headers */ = {isa = PBXBuildFile; fileRef = 51E51FBD282AD37A00FC978D /* MTRDeviceControllerStartupParams_Internal.h */; }; 51E51FC1282AD37A00FC978D /* MTRDeviceControllerStartupParams.mm in Sources */ = {isa = PBXBuildFile; fileRef = 51E51FBE282AD37A00FC978D /* MTRDeviceControllerStartupParams.mm */; }; + 51E95DF82A78110900A434F0 /* MTRPerControllerStorageTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 51E95DF72A78110900A434F0 /* MTRPerControllerStorageTests.m */; }; + 51E95DFB2A78443C00A434F0 /* MTRSessionResumptionStorageBridge.h in Headers */ = {isa = PBXBuildFile; fileRef = 51E95DF92A78443C00A434F0 /* MTRSessionResumptionStorageBridge.h */; }; + 51E95DFC2A78443C00A434F0 /* MTRSessionResumptionStorageBridge.mm in Sources */ = {isa = PBXBuildFile; fileRef = 51E95DFA2A78443C00A434F0 /* MTRSessionResumptionStorageBridge.mm */; }; 51EF279F2A2A3EB100E33F75 /* MTRBackwardsCompatShims.h in Headers */ = {isa = PBXBuildFile; fileRef = 51EF279E2A2A3EB100E33F75 /* MTRBackwardsCompatShims.h */; settings = {ATTRIBUTES = (Public, ); }; }; 5A60370827EA1FF60020DB79 /* MTRClusterStateCacheContainer+XPC.h in Headers */ = {isa = PBXBuildFile; fileRef = 5A60370727EA1FF60020DB79 /* MTRClusterStateCacheContainer+XPC.h */; }; 5A6FEC9027B563D900F25F42 /* MTRDeviceControllerOverXPC.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5A6FEC8F27B563D900F25F42 /* MTRDeviceControllerOverXPC.mm */; }; @@ -457,7 +466,13 @@ 51431AFA27D29CA4008A7943 /* ota-provider.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = "ota-provider.cpp"; path = "clusters/ota-provider/ota-provider.cpp"; sourceTree = ""; }; 5143851C2A65885400EDC8E6 /* MatterTests-Bridging-Header.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "MatterTests-Bridging-Header.h"; sourceTree = ""; }; 5143851D2A65885500EDC8E6 /* MTRSwiftPairingTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = MTRSwiftPairingTests.swift; sourceTree = ""; }; + 514654482A72F9DF00904E61 /* MTRDemuxingStorage.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRDemuxingStorage.mm; sourceTree = ""; }; + 5146544A2A72F9F500904E61 /* MTRDemuxingStorage.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDemuxingStorage.h; sourceTree = ""; }; 51565CAD2A79D42100469F18 /* MTRConversion.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRConversion.mm; sourceTree = ""; }; + 51565CAF2A7AD77600469F18 /* MTRDeviceControllerDataStore.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceControllerDataStore.h; sourceTree = ""; }; + 51565CB02A7AD77600469F18 /* MTRDeviceControllerDataStore.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRDeviceControllerDataStore.mm; sourceTree = ""; }; + 51565CB32A7AD78D00469F18 /* MTRDeviceControllerStorageDelegate.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceControllerStorageDelegate.h; sourceTree = ""; }; + 51565CB52A7B0D6600469F18 /* MTRDeviceControllerStartupParameters.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceControllerStartupParameters.h; sourceTree = ""; }; 515C1C6D284F9FFB00A48F0C /* MTRFramework.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRFramework.mm; sourceTree = ""; }; 515C1C6E284F9FFB00A48F0C /* MTRFramework.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRFramework.h; sourceTree = ""; }; 51669AEF2913204400F4AA36 /* MTRBackwardsCompatTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MTRBackwardsCompatTests.m; sourceTree = ""; }; @@ -488,6 +503,9 @@ 51E51FBC282AD37A00FC978D /* MTRDeviceControllerStartupParams.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceControllerStartupParams.h; sourceTree = ""; }; 51E51FBD282AD37A00FC978D /* MTRDeviceControllerStartupParams_Internal.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceControllerStartupParams_Internal.h; sourceTree = ""; }; 51E51FBE282AD37A00FC978D /* MTRDeviceControllerStartupParams.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRDeviceControllerStartupParams.mm; sourceTree = ""; }; + 51E95DF72A78110900A434F0 /* MTRPerControllerStorageTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MTRPerControllerStorageTests.m; sourceTree = ""; }; + 51E95DF92A78443C00A434F0 /* MTRSessionResumptionStorageBridge.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRSessionResumptionStorageBridge.h; sourceTree = ""; }; + 51E95DFA2A78443C00A434F0 /* MTRSessionResumptionStorageBridge.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRSessionResumptionStorageBridge.mm; sourceTree = ""; }; 51EF279E2A2A3EB100E33F75 /* MTRBackwardsCompatShims.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRBackwardsCompatShims.h; sourceTree = ""; }; 5A60370727EA1FF60020DB79 /* MTRClusterStateCacheContainer+XPC.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "MTRClusterStateCacheContainer+XPC.h"; sourceTree = ""; }; 5A6FEC8B27B5609C00F25F42 /* MTRDeviceOverXPC.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = MTRDeviceOverXPC.h; sourceTree = ""; }; @@ -1045,6 +1063,8 @@ 5A7947E227C0101200434CF2 /* MTRDeviceController+XPC.h */, 5A7947E327C0129500434CF2 /* MTRDeviceController+XPC.mm */, 2CB7163E252F731E0026E2BB /* MTRDeviceControllerDelegate.h */, + 51565CAF2A7AD77600469F18 /* MTRDeviceControllerDataStore.h */, + 51565CB02A7AD77600469F18 /* MTRDeviceControllerDataStore.mm */, 2CB71638252E8A7B0026E2BB /* MTRDeviceControllerDelegateBridge.h */, 2CB71639252E8A7B0026E2BB /* MTRDeviceControllerDelegateBridge.mm */, 5136661228067D550025EDAE /* MTRDeviceControllerFactory.h */, @@ -1056,6 +1076,8 @@ 51E51FBC282AD37A00FC978D /* MTRDeviceControllerStartupParams.h */, 51E51FBD282AD37A00FC978D /* MTRDeviceControllerStartupParams_Internal.h */, 51E51FBE282AD37A00FC978D /* MTRDeviceControllerStartupParams.mm */, + 51565CB52A7B0D6600469F18 /* MTRDeviceControllerStartupParameters.h */, + 51565CB32A7AD78D00469F18 /* MTRDeviceControllerStorageDelegate.h */, 5A6FEC9427B5976200F25F42 /* MTRDeviceControllerXPCConnection.h */, 5A6FEC9527B5983000F25F42 /* MTRDeviceControllerXPCConnection.mm */, 5A6FEC8B27B5609C00F25F42 /* MTRDeviceOverXPC.h */, @@ -1088,6 +1110,10 @@ 998F287026D56940001846C6 /* MTRP256KeypairBridge.mm */, 2C8C8FBD253E0C2100797F05 /* MTRPersistentStorageDelegateBridge.h */, 2C8C8FBF253E0C2100797F05 /* MTRPersistentStorageDelegateBridge.mm */, + 514654482A72F9DF00904E61 /* MTRDemuxingStorage.mm */, + 5146544A2A72F9F500904E61 /* MTRDemuxingStorage.h */, + 51E95DF92A78443C00A434F0 /* MTRSessionResumptionStorageBridge.h */, + 51E95DFA2A78443C00A434F0 /* MTRSessionResumptionStorageBridge.mm */, B2E0D7AC245B0B5C003C5B48 /* MTRQRCodeSetupPayloadParser.h */, B2E0D7AE245B0B5C003C5B48 /* MTRQRCodeSetupPayloadParser.mm */, B2E0D7AF245B0B5C003C5B48 /* MTRSetupPayload.h */, @@ -1132,6 +1158,7 @@ 51339B1E2A0DA64D00C798C1 /* MTRCertificateValidityTests.m */, 519498312A25581C00B3BABE /* MTRSetupPayloadSerializerTests.m */, 5143851D2A65885500EDC8E6 /* MTRSwiftPairingTests.swift */, + 51E95DF72A78110900A434F0 /* MTRPerControllerStorageTests.m */, B202529D2459E34F00F97062 /* Info.plist */, 5143851C2A65885400EDC8E6 /* MatterTests-Bridging-Header.h */, ); @@ -1198,6 +1225,8 @@ isa = PBXHeadersBuildPhase; buildActionMask = 2147483647; files = ( + 51565CB62A7B0D6600469F18 /* MTRDeviceControllerStartupParameters.h in Headers */, + 51565CB42A7AD78D00469F18 /* MTRDeviceControllerStorageDelegate.h in Headers */, 510A07492A685D3900A9241C /* Matter.apinotes in Headers */, 51EF279F2A2A3EB100E33F75 /* MTRBackwardsCompatShims.h in Headers */, 5173A47729C0E2ED00F67F48 /* MTRFabricInfo.h in Headers */, @@ -1273,12 +1302,15 @@ 2C222ADF255C811800E446B9 /* MTRBaseDevice_Internal.h in Headers */, 511913FC28C100EF009235E9 /* MTRBaseSubscriptionCallback.h in Headers */, 51E0310027EA20D20083DC9C /* MTRControllerAccessControl.h in Headers */, + 51565CB12A7AD77600469F18 /* MTRDeviceControllerDataStore.h in Headers */, 3D843713294977000070D20A /* NSDataSpanConversion.h in Headers */, 991DC08B247704DC00C13860 /* MTRLogging_Internal.h in Headers */, 1E4D655029C208DD00BC3478 /* MTRCommissionableBrowserDelegate.h in Headers */, 7596A84828762783004DAE0E /* MTRAsyncCallbackWorkQueue.h in Headers */, 5A7947E527C0129F00434CF2 /* MTRDeviceController+XPC.h in Headers */, + 51E95DFB2A78443C00A434F0 /* MTRSessionResumptionStorageBridge.h in Headers */, B2E0D7B4245B0B5C003C5B48 /* MTRError_Internal.h in Headers */, + 5146544B2A72F9F500904E61 /* MTRDemuxingStorage.h in Headers */, 1EDCE545289049A100E41EC9 /* MTROTAHeader.h in Headers */, ); runOnlyForDeploymentPostprocessing = 0; @@ -1485,6 +1517,7 @@ 75B765C32A1D82D30014719B /* MTRAttributeSpecifiedCheck.mm in Sources */, AF5F90FF2878D351005503FA /* MTROTAProviderDelegateBridge.mm in Sources */, 3D84374B29498BAE0070D20A /* privilege-storage.cpp in Sources */, + 51E95DFC2A78443C00A434F0 /* MTRSessionResumptionStorageBridge.mm in Sources */, 7534F12828BFF20300390851 /* MTRDeviceAttestationDelegate.mm in Sources */, 2C5EEEF7268A85C400CAE3D3 /* MTRDeviceConnectionBridge.mm in Sources */, 51B22C262740CB32008D5055 /* MTRStructsObjc.mm in Sources */, @@ -1496,6 +1529,7 @@ 514304202914CED9004DC7FE /* generic-callback-stubs.cpp in Sources */, 1EDCE546289049A100E41EC9 /* MTROTAHeader.mm in Sources */, 1EC4CE5D25CC26E900D7304F /* MTRBaseClusters.mm in Sources */, + 51565CB22A7AD77600469F18 /* MTRDeviceControllerDataStore.mm in Sources */, 51E0310127EA20D20083DC9C /* MTRControllerAccessControl.mm in Sources */, 1ED276E226C5812A00547A89 /* MTRCluster.mm in Sources */, B2E0D7B3245B0B5C003C5B48 /* MTRError.mm in Sources */, @@ -1523,6 +1557,7 @@ 7596A85528788557004DAE0E /* MTRClusters.mm in Sources */, 88EBF8CF27FABDD500686BC1 /* MTRDeviceAttestationDelegateBridge.mm in Sources */, 5A6FEC9827B5C6AF00F25F42 /* MTRDeviceOverXPC.mm in Sources */, + 514654492A72F9DF00904E61 /* MTRDemuxingStorage.mm in Sources */, 51431AF927D2973E008A7943 /* MTRIMDispatch.mm in Sources */, 1E4D655229C30A8700BC3478 /* MTRCommissionableBrowser.mm in Sources */, 51431AFB27D29CA4008A7943 /* ota-provider.cpp in Sources */, @@ -1561,6 +1596,7 @@ 51E24E73274E0DAC007CCF6E /* MTRErrorTestUtils.mm in Sources */, 519498322A25581C00B3BABE /* MTRSetupPayloadSerializerTests.m in Sources */, 51A2F1322A00402A00F03298 /* MTRDataValueParserTests.m in Sources */, + 51E95DF82A78110900A434F0 /* MTRPerControllerStorageTests.m in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; From d3efb29e2e7994e0a200e791bff4167c57ecefb0 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 11 Aug 2023 23:27:46 -0400 Subject: [PATCH 02/11] Require consumers to provide a dispatch queue for the storage delegate. --- .../Framework/CHIP/MTRDeviceController.mm | 10 +++++++- .../CHIP/MTRDeviceControllerDataStore.h | 3 ++- .../CHIP/MTRDeviceControllerDataStore.mm | 23 ++++++++----------- .../CHIP/MTRDeviceControllerFactory.mm | 7 +++++- .../MTRDeviceControllerStartupParameters.h | 9 ++++++-- .../CHIP/MTRDeviceControllerStartupParams.mm | 6 +++++ ...TRDeviceControllerStartupParams_Internal.h | 8 +++++++ .../CHIP/MTRDeviceControllerStorageDelegate.h | 12 ++++++---- .../CHIP/MTRDeviceController_Internal.h | 1 + .../CHIPTests/MTRPerControllerStorageTests.m | 8 ++++++- 10 files changed, 62 insertions(+), 25 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 9fe49545233f3a..af72411224b580 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -117,6 +117,7 @@ @implementation MTRDeviceController - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory queue:(dispatch_queue_t)queue storageDelegate:(id _Nullable)storageDelegate + storageDelegateQueue:(dispatch_queue_t _Nullable)storageDelegateQueue UUID:(NSUUID *)UUID { if (self = [super init]) { @@ -124,7 +125,14 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory // before we start doing anything else with the controller. _UUID = UUID; if (storageDelegate != nil) { - _controllerDataStore = [[MTRDeviceControllerDataStore alloc] initWithController:self storageDelegate:storageDelegate]; + if (storageDelegateQueue == nil) { + MTR_LOG_ERROR("storageDelegate provided without storageDelegateQueue"); + return nil; + } + + _controllerDataStore = [[MTRDeviceControllerDataStore alloc] initWithController:self + storageDelegate:storageDelegate + storageDelegateQueue:storageDelegateQueue]; if (_controllerDataStore == nil) { return nil; } diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h index c901476df20aeb..dedc3e2e0e95bb 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h @@ -42,7 +42,8 @@ MTR_HIDDEN @interface MTRDeviceControllerDataStore : NSObject - (instancetype)initWithController:(MTRDeviceController *)controller - storageDelegate:(id)storageDelegate; + storageDelegate:(id)storageDelegate + storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue; /** * Resumption info APIs. diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm index 30b12f3ca9742b..19480d9f6dac78 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -32,23 +32,17 @@ [NSString stringWithFormat:@"caseResumptionByResumptionID/%s", [resumptionID base64EncodedStringWithOptions:0].UTF8String]; } -static dispatch_queue_t sWorkQueue; - @implementation MTRDeviceControllerDataStore { id _delegate; + dispatch_queue_t _delegateQueue; MTRDeviceController * _controller; // Array of nodes with resumption info, oldest-stored first. NSMutableArray * _nodesWithResumptionInfo; } -+ (void)initialize -{ - sWorkQueue = dispatch_queue_create( - "org.csa-iot.matter.framework.controller-storage.workqueue", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL); -} - - (instancetype)initWithController:(MTRDeviceController *)controller storageDelegate:(id)storageDelegate + storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue { if (!(self = [super init])) { return nil; @@ -56,9 +50,10 @@ - (instancetype)initWithController:(MTRDeviceController *)controller _controller = controller; _delegate = storageDelegate; + _delegateQueue = storageDelegateQueue; __block id resumptionNodeList; - dispatch_sync(sWorkQueue, ^{ + dispatch_sync(_delegateQueue, ^{ resumptionNodeList = [_delegate controller:_controller valueForKey:sResumptionNodeListKey securityLevel:MTRStorageSecurityLevelSecure @@ -89,7 +84,7 @@ - (nullable MTRCASESessionResumptionInfo *)findResumptionInfoByResumptionID:(NSD - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo { auto * oldInfo = [self findResumptionInfoByNodeID:resumptionInfo.nodeID]; - dispatch_sync(sWorkQueue, ^{ + dispatch_sync(_delegateQueue, ^{ if (oldInfo != nil) { // Remove old resumption id key. No need to do that for the // node id, because we are about to overwrite it. @@ -128,7 +123,7 @@ - (void)clearAllResumptionInfo for (NSNumber * nodeID in _nodesWithResumptionInfo) { auto * oldInfo = [self findResumptionInfoByNodeID:nodeID]; if (oldInfo != nil) { - dispatch_sync(sWorkQueue, ^{ + dispatch_sync(_delegateQueue, ^{ [_delegate controller:_controller removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) securityLevel:MTRStorageSecurityLevelSecure @@ -147,7 +142,7 @@ - (void)clearAllResumptionInfo - (CHIP_ERROR)storeLastUsedNOC:(MTRCertificateTLVBytes)noc { __block BOOL ok; - dispatch_sync(sWorkQueue, ^{ + dispatch_sync(_delegateQueue, ^{ ok = [_delegate controller:_controller storeValue:noc forKey:sLastUsedNOCKey @@ -160,7 +155,7 @@ - (CHIP_ERROR)storeLastUsedNOC:(MTRCertificateTLVBytes)noc - (MTRCertificateTLVBytes _Nullable)fetchLastUsedNOC { __block id data; - dispatch_sync(sWorkQueue, ^{ + dispatch_sync(_delegateQueue, ^{ data = [_delegate controller:_controller valueForKey:sLastUsedNOCKey securityLevel:MTRStorageSecurityLevelSecure @@ -181,7 +176,7 @@ - (MTRCertificateTLVBytes _Nullable)fetchLastUsedNOC - (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(NSString *)key { __block id resumptionInfo; - dispatch_sync(sWorkQueue, ^{ + dispatch_sync(_delegateQueue, ^{ resumptionInfo = [_delegate controller:_controller valueForKey:key securityLevel:MTRStorageSecurityLevelSecure diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index fb9c4af822cf7e..7dc9a9a08443e8 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -643,14 +643,17 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(id)startupParams } id storageDelegate; + dispatch_queue_t storageDelegateQueue; NSUUID * uuid; if ([startupParams isKindOfClass:[MTRDeviceControllerStartupParameters class]]) { MTRDeviceControllerStartupParameters * params = startupParams; storageDelegate = params.storageDelegate; + storageDelegateQueue = params.storageDelegateQueue; uuid = params.UUID; } else { MTRDeviceControllerStartupParams * params = startupParams; storageDelegate = nil; + storageDelegateQueue = nil; uuid = params.UUID; } @@ -672,7 +675,7 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(id)startupParams // Create the controller, so we start the event loop, since we plan to do // our fabric table operations there. - auto * controller = [self _createController:storageDelegate UUID:uuid]; + auto * controller = [self _createController:storageDelegate storageDelegateQueue:storageDelegateQueue UUID:uuid]; if (controller == nil) { if (error != nil) { *error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY]; @@ -897,6 +900,7 @@ - (MTRDeviceController * _Nullable)createController:(MTRDeviceControllerStartupP } - (MTRDeviceController * _Nullable)_createController:(id _Nullable)storageDelegate + storageDelegateQueue:(dispatch_queue_t _Nullable)storageDelegateQueue UUID:(NSUUID *)UUID { [self _assertCurrentQueueIsNotMatterQueue]; @@ -904,6 +908,7 @@ - (MTRDeviceController * _Nullable)_createController:(id)storageDelegate + storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue UUID:(NSUUID *)UUID ipk:(NSData *)ipk vendorID:(NSNumber *)vendorID diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm index 93ca55b1c43993..8e16140d8e21db 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm @@ -243,6 +243,7 @@ - (instancetype)initWithOperationalKeypair:(id)operationalKeypair @implementation MTRDeviceControllerStartupParameters - (instancetype)initWithStorageDelegate:(id)storageDelegate + storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue UUID:(NSUUID *)UUID ipk:(NSData *)ipk vendorID:(NSNumber *)vendorID @@ -265,6 +266,7 @@ - (instancetype)initWithStorageDelegate:(id) _operationalCertificateIssuer = nil; _operationalCertificateIssuerQueue = nil; _storageDelegate = storageDelegate; + _storageDelegateQueue = storageDelegateQueue; _UUID = UUID; return self; @@ -280,6 +282,7 @@ - (void)setOperationalCertificateIssuer:(id)ope @implementation MTRDeviceControllerExternalCertificateStartupParameters - (instancetype)initWithStorageDelegate:(id)storageDelegate + storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue UUID:(NSUUID *)UUID ipk:(NSData *)ipk vendorID:(NSNumber *)vendorID @@ -289,6 +292,7 @@ - (instancetype)initWithStorageDelegate:(id) rootCertificate:(MTRCertificateDERBytes)rootCertificate { return [super initWithStorageDelegate:storageDelegate + storageDelegateQueue:storageDelegateQueue UUID:UUID ipk:ipk vendorID:vendorID @@ -308,6 +312,7 @@ - (instancetype)initWithParams:(MTRDeviceControllerStartupParams *)params } _storageDelegate = nil; + _storageDelegateQueue = nil; if (self.nocSigner == nil && self.rootCertificate == nil) { MTR_LOG_ERROR("nocSigner and rootCertificate are both nil; no public key available to identify the fabric"); @@ -589,6 +594,7 @@ - (instancetype)initForNewController:(MTRDeviceController *)controller _advertiseOperational = advertiseOperational; _allowMultipleControllersPerFabric = YES; _storageDelegate = params.storageDelegate; + _storageDelegateQueue = params.storageDelegateQueue; return self; } diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams_Internal.h index b68ef04a8c0d64..cb14af68b260be 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams_Internal.h @@ -51,6 +51,7 @@ NS_ASSUME_NONNULL_BEGIN @interface MTRDeviceControllerStartupParameters () - (instancetype)initWithStorageDelegate:(id)storageDelegate + storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue UUID:(NSUUID *)UUID ipk:(NSData *)ipk vendorID:(NSNumber *)vendorID @@ -73,6 +74,7 @@ NS_ASSUME_NONNULL_BEGIN @property (nonatomic, strong, nullable, readonly) dispatch_queue_t operationalCertificateIssuerQueue; @property (nonatomic, strong, readonly) id storageDelegate; +@property (nonatomic, strong, readonly) dispatch_queue_t storageDelegateQueue; @property (nonatomic, strong, readonly) NSUUID * UUID; @end @@ -101,6 +103,12 @@ MTR_HIDDEN */ @property (nonatomic, strong, nullable, readonly) id storageDelegate; +/** + * The queue to use for storageDelegate. This will be nil if and only if + * storageDelegate is nil. + */ +@property (nonatomic, strong, nullable, readonly) dispatch_queue_t storageDelegateQueue; + /** * Helper method that checks that our keypairs match our certificates. * Specifically: diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h b/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h index 86be0ecc7ee773..570ff2ff58aa43 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h @@ -61,11 +61,13 @@ typedef NS_ENUM(NSUInteger, MTRStorageSharingType) { * initialized when the callbacks are called. The only safe thing to do with * it is to get its controllerID. * - * 2) The delegate method calls will happen serially, not concurrently, on some arbitrary - * thread. During execution of the delegate methods, all Matter work will be - * blocked until the method completes. Attempting to call any Matter API - * from inside the delegate methods, apart from de-serializing and - * serializing the items being stored, is likely to lead to deadlocks. + * 2) The delegate method calls will happen on the queue that was provided along + * with the gelegate. All Matter work will be blocked until the method + * completes, and these calls may themselves block other Matter API calls + * from completing. Attempting to call any Matter API on the queue used for + * this delegate, apart from de-serializing and serializing the items being + * stored and calling MTRDeviceControllerStorageClasses(), is likely to lead + * to deadlocks. */ MTR_NEWLY_AVAILABLE @protocol MTRDeviceControllerStorageDelegate diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h index 9bb249d0ee31d7..031ec1fb909825 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h @@ -90,6 +90,7 @@ NS_ASSUME_NONNULL_BEGIN - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory queue:(dispatch_queue_t)queue storageDelegate:(id _Nullable)storageDelegate + storageDelegateQueue:(dispatch_queue_t _Nullable)storageDelegateQueue UUID:(NSUUID *)UUID; /** diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m index 82fa9fd07e8426..5393d7e006c640 100644 --- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -247,7 +247,9 @@ - (void)issueOperationalCertificateForRequest:(MTROperationalCSRInfo *)csrInfo @interface MTRPerControllerStorageTests : XCTestCase @end -@implementation MTRPerControllerStorageTests +@implementation MTRPerControllerStorageTests { + dispatch_queue_t _storageQueue; +} + (void)tearDown { @@ -259,6 +261,8 @@ - (void)setUp [super setUp]; [self setContinueAfterFailure:NO]; + _storageQueue = dispatch_queue_create("test.storage.queue", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL); + [self startFactory]; } @@ -266,6 +270,7 @@ - (void)tearDown { // Per-test teardown, runs after each test. [self stopFactory]; + _storageQueue = nil; [super tearDown]; } @@ -345,6 +350,7 @@ - (nullable MTRDeviceController *)startControllerWithRootKeys:(MTRTestKeys *)roo __auto_type * params = [[MTRDeviceControllerExternalCertificateStartupParameters alloc] initWithStorageDelegate:storage + storageDelegateQueue:_storageQueue UUID:storage.controllerID ipk:rootKeys.ipk vendorID:@(kTestVendorId) From 993be068ebc88975b8191362fdf1a7f6f7a49078 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 28 Aug 2023 12:51:49 -0400 Subject: [PATCH 03/11] Address review comments. --- .../Framework/CHIP/MTRDemuxingStorage.h | 39 -- .../Framework/CHIP/MTRDemuxingStorage.mm | 375 ++++++++++-------- .../Framework/CHIP/MTRDeviceController.h | 2 +- .../Framework/CHIP/MTRDeviceController.mm | 4 +- .../CHIP/MTRDeviceControllerFactory.mm | 20 +- .../MTRDeviceControllerStartupParameters.h | 4 +- .../CHIP/MTRDeviceControllerStartupParams.mm | 16 +- ...TRDeviceControllerStartupParams_Internal.h | 8 +- .../CHIP/MTRDeviceController_Internal.h | 2 +- .../CHIPTests/MTRPerControllerStorageTests.m | 10 +- 10 files changed, 234 insertions(+), 246 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDemuxingStorage.h b/src/darwin/Framework/CHIP/MTRDemuxingStorage.h index bbdfa3bc5e0024..bfff6369800bc7 100644 --- a/src/darwin/Framework/CHIP/MTRDemuxingStorage.h +++ b/src/darwin/Framework/CHIP/MTRDemuxingStorage.h @@ -43,26 +43,6 @@ class MTRDemuxingStorage : public chip::PersistentStorageDelegate { CHIP_ERROR SyncDeleteKeyValue(const char * key) override; private: - static bool IsGlobalKey(NSString * key); - - /** - * Checks for a key that is scoped to a specific fabric index. - */ - static bool IsIndexSpecificKey(NSString * key); - - /** - * Extracts the fabric index from an index-specific key. Fails if the key - * is not index-specific or if a numeric FabricIndex could not be extracted - * from it. - */ - static CHIP_ERROR ExtractIndexFromKey(NSString * key, chip::FabricIndex * index); - - /** - * Extracts the "index-specific" part of an index-specific key (i.e. the - * part after "f/index/"). - */ - static CHIP_ERROR ExtractIndexSpecificKey(NSString * key, NSString * _Nullable __autoreleasing * _Nonnull extractedKey); - /** * Methods for reading/writing/deleting things. The index-specific ones * will have the "f/index/" bit already stripped of from the front of the key. @@ -76,20 +56,6 @@ class MTRDemuxingStorage : public chip::PersistentStorageDelegate { CHIP_ERROR DeleteGlobalValue(NSString * key); CHIP_ERROR DeleteIndexSpecificValue(chip::FabricIndex index, NSString * key); - /** - * Method to test whether a global key should be stored in memory only, as - * opposed to being passed on to the actual storage related to controllers. - */ - static bool IsMemoryOnlyGlobalKey(NSString * key); - - /** - * Method to test whether an index-specific key should be stored in memory only, as - * opposed to being passed on to the actual storage related to controllers. - * The key string will ahve the "f/index/" bit already stripped off the - * front of the key. - */ - static bool IsMemoryOnlyIndexSpecificKey(NSString * key); - /** * Methods for modifying our in-memory store for fully qualified keys. */ @@ -97,11 +63,6 @@ class MTRDemuxingStorage : public chip::PersistentStorageDelegate { CHIP_ERROR SetInMemoryValue(NSString * key, NSData * data); CHIP_ERROR DeleteInMemoryValue(NSString * key); - /** - * Method to convert an index-specific key into a fully qualified key. - */ - static NSString * FullyQualifiedKey(chip::FabricIndex index, NSString * key); - MTRDeviceControllerFactory * mFactory; NSMutableDictionary * mInMemoryStore; }; diff --git a/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm b/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm index 070391e83b5934..49aa58dae187e6 100644 --- a/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm +++ b/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm @@ -26,6 +26,203 @@ using namespace chip; +static bool IsGlobalKey(NSString * key) { return [key hasPrefix:@"g/"]; } + +/** + * Checks for a key that is scoped to a specific fabric index. + */ +static bool IsIndexSpecificKey(NSString * key) { return [key hasPrefix:@"f/"]; } + +/** + * Extracts the fabric index from an index-specific key. Fails if the key + * is not index-specific or if a numeric FabricIndex could not be extracted + * from it. + */ +static CHIP_ERROR ExtractIndexFromKey(NSString * key, FabricIndex * index) +{ + if (!IsIndexSpecificKey(key)) { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + auto * components = [key componentsSeparatedByString:@"/"]; + if (components.count < 3) { + // Unexpected "f/something" without any actual data. + return CHIP_ERROR_INVALID_ARGUMENT; + } + + auto * indexString = components[1]; + auto * scanner = [NSScanner scannerWithString:indexString]; + + auto * charset = [NSCharacterSet characterSetWithCharactersInString:@"0123456789abcdefABCDEF"]; + charset = [charset invertedSet]; + + if ([scanner scanCharactersFromSet:charset intoString:nil] == YES) { + // Leading non-hex chars. + return CHIP_ERROR_INVALID_ARGUMENT; + } + + unsigned int value; + if ([scanner scanHexInt:&value] == NO) { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + if (scanner.atEnd == NO) { + // Trailing garbage chars. + return CHIP_ERROR_INVALID_ARGUMENT; + } + + if (!CanCastTo(value)) { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + *index = static_cast(value); + return CHIP_NO_ERROR; +} + +/** + * Extracts the "index-specific" part of an index-specific key (i.e. the + * part after "f/index/"). + */ +static CHIP_ERROR ExtractIndexSpecificKey(NSString * key, NSString * __autoreleasing * extractedKey) +{ + if (!IsIndexSpecificKey(key)) { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + auto * components = [key componentsSeparatedByString:@"/"]; + if (components.count < 3) { + // Unexpected "f/something" without any actual data. + return CHIP_ERROR_INVALID_ARGUMENT; + } + + components = [components subarrayWithRange:NSMakeRange(2, components.count - 2)]; + *extractedKey = [components componentsJoinedByString:@"/"]; + return CHIP_NO_ERROR; +} + +/** + * Method to test whether a global key should be stored in memory only, as + * opposed to being passed on to the actual storage related to controllers. + */ +static bool IsMemoryOnlyGlobalKey(NSString * key) +{ + if ([key isEqualToString:@"g/fidx"]) { + // Fabric index list only needs to be stored in-memory, not persisted, + // because we do not tie controllers to specific fabric indices. + return true; + } + + if ([key isEqualToString:@"g/fs/c"] || [key isEqualToString:@"g/fs/n"]) { + // Just store the fail-safe markers in memory as well. We could + // plausibly not store them at all, since we never actually need to + // clean up anything by fabric index, but this is safer in case + // consumers try to read back right after writing. + return true; + } + + if ([key isEqualToString:@"g/lkgt"]) { + // Store Last Known Good Time in memory only. We never need this in + // general, because we can always provide wall-clock time. + return true; + } + + // We do not expect to see the "g/sri" or "g/s/*" keys for session + // resumption, because we implement SessionResumptionStorage ourselves. + + // For now, put group global counters in memory. + // TODO: we should inject a group counter manager that makes these counters + // per-controller, not globally handled via storage. + // See https://github.com/project-chip/connectedhomeip/issues/28510 + if ([key isEqualToString:@"g/gdc"] || [key isEqualToString:@"g/gcc"]) { + return true; + } + + // We do not expect to see "g/userlbl/*" User Label keys. + + // We do not expect to see the "g/gfl" key for endpoint-to-group + // associations. + + // We do not expect to see the "g/a/*" keys for attribute values. + + // We do not expect to see the "g/bt" and "g/bt/*" keys for the binding + // table. + + // We do not expect to see the "g/o/*" OTA Requestor keys. + + // We do not expect to see the "g/im/ec" event number counter key. + + // We do not expect to see the "g/su/*" and "g/sum" keys for server-side + // subscription resumption storage. + + // We do not expect to see the "g/scc/*" scenes keys. + + // We do not expect to see the "g/ts/tts", "g/ts/dntp", "g/ts/tz", + // "g/ts/dsto" Time Synchronization keys. + + return false; +} + +/** + * Method to test whether an index-specific key should be stored in memory only, as + * opposed to being passed on to the actual storage related to controllers. + * The key string will ahve the "f/index/" bit already stripped off the + * front of the key. + */ +static bool IsMemoryOnlyIndexSpecificKey(NSString * key) +{ + // Store all the fabric table bits in memory only. This is done because the + // fabric table expects none of these things to be stored in the case of a + // "new fabric addition", which is what we always do when using + // per-controller storage. + // + // TODO: Figure out which, if any, of these things we should also store for + // later recall when starting a controller with storage we have used before. + // + // For future reference: + // + // n == NOC + // i == ICAC + // r == RCAC + // m == Fabric metadata (TLV containing the vendor ID) + // o == operational key, only written if internally generated. + if ([key isEqualToString:@"n"] || [key isEqualToString:@"i"] || [key isEqualToString:@"r"] || [key isEqualToString:@"m"] || + [key isEqualToString:@"o"]) { + return true; + } + + // We do not expect to see the "s/*" keys for session resumption, because we + // implement SessionResumptionStorage ourselves. + + // We do not expect to see the "ac/*" keys for ACL entries. + + // We do not expect to see the "g" or "g/*" keys for which endpoints are in which + // group. + + // For now, just store group keysets and group keys in memory. + // TODO: We want to start persisting these, per-controller, if we're going + // to support group keys. Or inject a GroupDataProvider of our own instead + // of using Credentials::GroupDataProviderImp and then + // not be tied to whatever storage format that uses. + // https://github.com/project-chip/connectedhomeip/issues/28511 + if ([key hasPrefix:@"gk/"] || [key hasPrefix:@"k/"]) { + return true; + } + + // We do not expect to see the "icd/*" keys for the ICD Management table. + + // We do not expect to see the "e/*" scenes keys. + + return false; +} + +/** + * Method to convert an index-specific key into a fully qualified key. + */ +static NSString * FullyQualifiedKey(FabricIndex index, NSString * key) +{ + return [NSString stringWithFormat:@"f/%x/%s", index, key.UTF8String]; +} + MTRDemuxingStorage::MTRDemuxingStorage(MTRDeviceControllerFactory * factory) : mFactory(factory) { @@ -129,68 +326,6 @@ return CHIP_ERROR_INVALID_ARGUMENT; } -bool MTRDemuxingStorage::IsGlobalKey(NSString * key) { return [key hasPrefix:@"g/"]; } - -bool MTRDemuxingStorage::IsIndexSpecificKey(NSString * key) { return [key hasPrefix:@"f/"]; } - -CHIP_ERROR MTRDemuxingStorage::ExtractIndexFromKey(NSString * key, FabricIndex * index) -{ - if (!IsIndexSpecificKey(key)) { - return CHIP_ERROR_INVALID_ARGUMENT; - } - - auto * components = [key componentsSeparatedByString:@"/"]; - if (components.count < 3) { - // Unexpected "f/something" without any actual data. - return CHIP_ERROR_INVALID_ARGUMENT; - } - - auto * indexString = components[1]; - auto * scanner = [NSScanner scannerWithString:indexString]; - - auto * charset = [NSCharacterSet characterSetWithCharactersInString:@"0123456789abcdefABCDEF"]; - charset = [charset invertedSet]; - - if ([scanner scanCharactersFromSet:charset intoString:nil] == YES) { - // Leading non-hex chars. - return CHIP_ERROR_INVALID_ARGUMENT; - } - - unsigned int value; - if ([scanner scanHexInt:&value] == NO) { - return CHIP_ERROR_INVALID_ARGUMENT; - } - - if (scanner.atEnd == NO) { - // Trailing garbage chars. - return CHIP_ERROR_INVALID_ARGUMENT; - } - - if (!CanCastTo(value)) { - return CHIP_ERROR_INVALID_ARGUMENT; - } - - *index = static_cast(value); - return CHIP_NO_ERROR; -} - -CHIP_ERROR MTRDemuxingStorage::ExtractIndexSpecificKey(NSString * key, NSString * __autoreleasing * extractedKey) -{ - if (!IsIndexSpecificKey(key)) { - return CHIP_ERROR_INVALID_ARGUMENT; - } - - auto * components = [key componentsSeparatedByString:@"/"]; - if (components.count < 3) { - // Unexpected "f/something" without any actual data. - return CHIP_ERROR_INVALID_ARGUMENT; - } - - components = [components subarrayWithRange:NSMakeRange(2, components.count - 2)]; - *extractedKey = [components componentsJoinedByString:@"/"]; - return CHIP_NO_ERROR; -} - NSData * _Nullable MTRDemuxingStorage::GetGlobalValue(NSString * key) { if (IsMemoryOnlyGlobalKey(key)) { @@ -202,7 +337,7 @@ return nil; } -NSData * _Nullable MTRDemuxingStorage::GetIndexSpecificValue(chip::FabricIndex index, NSString * key) +NSData * _Nullable MTRDemuxingStorage::GetIndexSpecificValue(FabricIndex index, NSString * key) { if (IsMemoryOnlyIndexSpecificKey(key)) { return GetInMemoryValue(FullyQualifiedKey(index, key)); @@ -224,7 +359,7 @@ return CHIP_ERROR_PERSISTED_STORAGE_FAILED; } -CHIP_ERROR MTRDemuxingStorage::SetIndexSpecificValue(chip::FabricIndex index, NSString * key, NSData * data) +CHIP_ERROR MTRDemuxingStorage::SetIndexSpecificValue(FabricIndex index, NSString * key, NSData * data) { if ([key isEqualToString:@"n"]) { auto * controller = [mFactory runningControllerForFabricIndex:index]; @@ -253,7 +388,7 @@ return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; } -CHIP_ERROR MTRDemuxingStorage::DeleteIndexSpecificValue(chip::FabricIndex index, NSString * key) +CHIP_ERROR MTRDemuxingStorage::DeleteIndexSpecificValue(FabricIndex index, NSString * key) { if (IsMemoryOnlyIndexSpecificKey(key)) { return DeleteInMemoryValue(FullyQualifiedKey(index, key)); @@ -262,111 +397,6 @@ return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; } -bool MTRDemuxingStorage::IsMemoryOnlyGlobalKey(NSString * key) -{ - if ([key isEqualToString:@"g/fidx"]) { - // Fabric index list only needs to be stored in-memory, not persisted, - // because we do not tie controllers to specific fabric indices. - return true; - } - - if ([key isEqualToString:@"g/fs/c"] || [key isEqualToString:@"g/fs/n"]) { - // Just store the fail-safe markers in memory as well. We could - // plausibly not store them at all, since we never actually need to - // clean up anything by fabric index, but this is safer in case - // consumers try to read back right after writing. - return true; - } - - if ([key isEqualToString:@"g/lkgt"]) { - // Store Last Known Good Time in memory only. We never need this in - // general, because we can always provide wall-clock time. - return true; - } - - // We do not expect to see the "g/sri" or "g/s/*" keys for session - // resumption, because we implement SessionResumptionStorage ourselves. - - // For now, put group global counters in memory. - // TODO: we should inject a group counter manager that makes these counters - // per-controller, not globally handled via storage. - // See https://github.com/project-chip/connectedhomeip/issues/28510 - if ([key isEqualToString:@"g/gdc"] || [key isEqualToString:@"g/gcc"]) { - return true; - } - - // We do not expect to see "g/userlbl/*" User Label keys. - - // We do not expect to see the "g/gfl" key for endpoint-to-group - // associations. - - // We do not expect to see the "g/a/*" keys for attribute values. - - // We do not expect to see the "g/bt" and "g/bt/*" keys for the binding - // table. - - // We do not expect to see the "g/o/*" OTA Requestor keys. - - // We do not expect to see the "g/im/ec" event number counter key. - - // We do not expect to see the "g/su/*" and "g/sum" keys for server-side - // subscription resumption storage. - - // We do not expect to see the "g/scc/*" scenes keys. - - // We do not expect to see the "g/ts/tts", "g/ts/dntp", "g/ts/tz", - // "g/ts/dsto" Time Synchronization keys. - - return false; -} - -bool MTRDemuxingStorage::IsMemoryOnlyIndexSpecificKey(NSString * key) -{ - // Store all the fabric table bits in memory only. This is done because the - // fabric table expects none of these things to be stored in the case of a - // "new fabric addition", which is what we always do when using - // per-controller storage. - // - // TODO: Figure out which, if any, of these things we should also store for - // later recall when starting a controller with storage we have used before. - // - // For future reference: - // - // n == NOC - // i == ICAC - // r == RCAC - // m == Fabric metadata (TLV containing the vendor ID) - // o == operational key, only written if internally generated. - if ([key isEqualToString:@"n"] || [key isEqualToString:@"i"] || [key isEqualToString:@"r"] || [key isEqualToString:@"m"] || - [key isEqualToString:@"o"]) { - return true; - } - - // We do not expect to see the "s/*" keys for session resumption, because we - // implement SessionResumptionStorage ourselves. - - // We do not expect to see the "ac/*" keys for ACL entries. - - // We do not expect to see the "g" or "g/*" keys for which endpoints are in which - // group. - - // For now, just store group keysets and group keys in memory. - // TODO: We want to start persisting these, per-controller, if we're going - // to support group keys. Or inject a GroupDataProvider of our own instead - // of using Credentials::GroupDataProviderImp and then - // not be tied to whatever storage format that uses. - // https://github.com/project-chip/connectedhomeip/issues/28511 - if ([key hasPrefix:@"gk/"] || [key hasPrefix:@"k/"]) { - return true; - } - - // We do not expect to see the "icd/*" keys for the ICD Management table. - - // We do not expect to see the "e/*" scenes keys. - - return false; -} - NSData * _Nullable MTRDemuxingStorage::GetInMemoryValue(NSString * key) { return mInMemoryStore[key]; } CHIP_ERROR MTRDemuxingStorage::SetInMemoryValue(NSString * key, NSData * data) @@ -381,8 +411,3 @@ [mInMemoryStore removeObjectForKey:key]; return present ? CHIP_NO_ERROR : CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; } - -NSString * MTRDemuxingStorage::FullyQualifiedKey(chip::FabricIndex index, NSString * key) -{ - return [NSString stringWithFormat:@"f/%x/%s", index, key.UTF8String]; -} diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.h b/src/darwin/Framework/CHIP/MTRDeviceController.h index 293405bc6699eb..670e12bbd0caaf 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController.h @@ -50,7 +50,7 @@ typedef void (^MTRDeviceConnectionCallback)(MTRBaseDevice * _Nullable device, NS /** * The ID assigned to this controller at creation time. */ -@property (readonly, nonatomic) NSUUID * UUID MTR_NEWLY_AVAILABLE; +@property (readonly, nonatomic) NSUUID * uniqueIdentifier MTR_NEWLY_AVAILABLE; /** * Return the Node ID assigned to the controller. Will return nil if the diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index af72411224b580..7fe0b5e2bd256e 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -118,12 +118,12 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory queue:(dispatch_queue_t)queue storageDelegate:(id _Nullable)storageDelegate storageDelegateQueue:(dispatch_queue_t _Nullable)storageDelegateQueue - UUID:(NSUUID *)UUID + uniqueIdentifier:(NSUUID *)uniqueIdentifier { if (self = [super init]) { // Make sure our storage is all set up to work as early as possible, // before we start doing anything else with the controller. - _UUID = UUID; + _uniqueIdentifier = uniqueIdentifier; if (storageDelegate != nil) { if (storageDelegateQueue == nil) { MTR_LOG_ERROR("storageDelegate provided without storageDelegateQueue"); diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index 7dc9a9a08443e8..cdd40bfb3da810 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -644,17 +644,17 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(id)startupParams id storageDelegate; dispatch_queue_t storageDelegateQueue; - NSUUID * uuid; + NSUUID * uniqueIdentifier; if ([startupParams isKindOfClass:[MTRDeviceControllerStartupParameters class]]) { MTRDeviceControllerStartupParameters * params = startupParams; storageDelegate = params.storageDelegate; storageDelegateQueue = params.storageDelegateQueue; - uuid = params.UUID; + uniqueIdentifier = params.uniqueIdentifier; } else { MTRDeviceControllerStartupParams * params = startupParams; storageDelegate = nil; storageDelegateQueue = nil; - uuid = params.UUID; + uniqueIdentifier = params.uniqueIdentifier; } if (_usingPerControllerStorage && storageDelegate == nil) { @@ -675,7 +675,9 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(id)startupParams // Create the controller, so we start the event loop, since we plan to do // our fabric table operations there. - auto * controller = [self _createController:storageDelegate storageDelegateQueue:storageDelegateQueue UUID:uuid]; + auto * controller = [self _createController:storageDelegate + storageDelegateQueue:storageDelegateQueue + uniqueIdentifier:uniqueIdentifier]; if (controller == nil) { if (error != nil) { *error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY]; @@ -704,12 +706,12 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(id)startupParams return; } - // Check that we are not trying to start a controller with a UUID that + // Check that we are not trying to start a controller with a uniqueIdentifier that // matches a running controller. auto * controllersCopy = [self getRunningControllers]; for (MTRDeviceController * existing in controllersCopy) { - if (existing != controller && [existing.UUID compare:params.UUID] == NSOrderedSame) { - MTR_LOG_ERROR("Already have running controller with UUID %@", existing.UUID); + if (existing != controller && [existing.uniqueIdentifier compare:params.uniqueIdentifier] == NSOrderedSame) { + MTR_LOG_ERROR("Already have running controller with uniqueIdentifier %@", existing.uniqueIdentifier); fabricError = CHIP_ERROR_INVALID_ARGUMENT; params = nil; return; @@ -901,7 +903,7 @@ - (MTRDeviceController * _Nullable)createController:(MTRDeviceControllerStartupP - (MTRDeviceController * _Nullable)_createController:(id _Nullable)storageDelegate storageDelegateQueue:(dispatch_queue_t _Nullable)storageDelegateQueue - UUID:(NSUUID *)UUID + uniqueIdentifier:(NSUUID *)uniqueIdentifier { [self _assertCurrentQueueIsNotMatterQueue]; @@ -909,7 +911,7 @@ - (MTRDeviceController * _Nullable)_createController:(id)storageDelegate storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue - UUID:(NSUUID *)UUID + uniqueIdentifier:(NSUUID *)uniqueIdentifier ipk:(NSData *)ipk vendorID:(NSNumber *)vendorID operationalKeypair:(id)operationalKeypair diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm index 8e16140d8e21db..232750d6ab2317 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm @@ -83,7 +83,7 @@ - (instancetype)initWithIPK:(NSData *)ipk fabricID:(NSNumber *)fabricID nocSigne _nocSigner = nocSigner; _fabricID = [fabricID copy]; _ipk = [ipk copy]; - _UUID = [NSUUID UUID]; + _uniqueIdentifier = [NSUUID UUID]; return self; } @@ -112,7 +112,7 @@ - (instancetype)initWithIPK:(NSData *)ipk _intermediateCertificate = [intermediateCertificate copy]; _rootCertificate = [rootCertificate copy]; _ipk = [ipk copy]; - _UUID = [NSUUID UUID]; + _uniqueIdentifier = [NSUUID UUID]; return self; } @@ -135,7 +135,7 @@ - (instancetype)initWithParams:(MTRDeviceControllerStartupParams *)params _operationalKeypair = params.operationalKeypair; _operationalCertificateIssuer = params.operationalCertificateIssuer; _operationalCertificateIssuerQueue = params.operationalCertificateIssuerQueue; - _UUID = params.UUID; + _uniqueIdentifier = params.uniqueIdentifier; return self; } @@ -172,7 +172,7 @@ - (instancetype)initWithParameters:(MTRDeviceControllerStartupParameters *)param _operationalKeypair = params.operationalKeypair; _operationalCertificateIssuer = params.operationalCertificateIssuer; _operationalCertificateIssuerQueue = params.operationalCertificateIssuerQueue; - _UUID = params.UUID; + _uniqueIdentifier = params.uniqueIdentifier; return self; } @@ -244,7 +244,7 @@ - (instancetype)initWithOperationalKeypair:(id)operationalKeypair @implementation MTRDeviceControllerStartupParameters - (instancetype)initWithStorageDelegate:(id)storageDelegate storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue - UUID:(NSUUID *)UUID + uniqueIdentifier:(NSUUID *)uniqueIdentifier ipk:(NSData *)ipk vendorID:(NSNumber *)vendorID operationalKeypair:(id)operationalKeypair @@ -267,7 +267,7 @@ - (instancetype)initWithStorageDelegate:(id) _operationalCertificateIssuerQueue = nil; _storageDelegate = storageDelegate; _storageDelegateQueue = storageDelegateQueue; - _UUID = UUID; + _uniqueIdentifier = uniqueIdentifier; return self; } @@ -283,7 +283,7 @@ - (void)setOperationalCertificateIssuer:(id)ope @implementation MTRDeviceControllerExternalCertificateStartupParameters - (instancetype)initWithStorageDelegate:(id)storageDelegate storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue - UUID:(NSUUID *)UUID + uniqueIdentifier:(NSUUID *)uniqueIdentifier ipk:(NSData *)ipk vendorID:(NSNumber *)vendorID operationalKeypair:(id)operationalKeypair @@ -293,7 +293,7 @@ - (instancetype)initWithStorageDelegate:(id) { return [super initWithStorageDelegate:storageDelegate storageDelegateQueue:storageDelegateQueue - UUID:UUID + uniqueIdentifier:uniqueIdentifier ipk:ipk vendorID:vendorID operationalKeypair:operationalKeypair diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams_Internal.h index cb14af68b260be..53cec273ebdc52 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams_Internal.h @@ -41,8 +41,8 @@ NS_ASSUME_NONNULL_BEGIN // MTRDeviceControllerStartupParamsInternal. @property (nonatomic, copy, nullable) MTRCertificateDERBytes operationalCertificate; -// UUID, so that we always have one. -@property (nonatomic, strong, readonly) NSUUID * UUID; +// uniqueIdentifier, so that we always have one. +@property (nonatomic, strong, readonly) NSUUID * uniqueIdentifier; // Init method that just copies the values of all our ivars. - (instancetype)initWithParams:(MTRDeviceControllerStartupParams *)params; @@ -52,7 +52,7 @@ NS_ASSUME_NONNULL_BEGIN - (instancetype)initWithStorageDelegate:(id)storageDelegate storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue - UUID:(NSUUID *)UUID + uniqueIdentifier:(NSUUID *)uniqueIdentifier ipk:(NSData *)ipk vendorID:(NSNumber *)vendorID operationalKeypair:(id)operationalKeypair @@ -75,7 +75,7 @@ NS_ASSUME_NONNULL_BEGIN @property (nonatomic, strong, readonly) id storageDelegate; @property (nonatomic, strong, readonly) dispatch_queue_t storageDelegateQueue; -@property (nonatomic, strong, readonly) NSUUID * UUID; +@property (nonatomic, strong, readonly) NSUUID * uniqueIdentifier; @end diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h index 031ec1fb909825..89db3a9f951d3e 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h @@ -91,7 +91,7 @@ NS_ASSUME_NONNULL_BEGIN queue:(dispatch_queue_t)queue storageDelegate:(id _Nullable)storageDelegate storageDelegateQueue:(dispatch_queue_t _Nullable)storageDelegateQueue - UUID:(NSUUID *)UUID; + uniqueIdentifier:(NSUUID *)uniqueIdentifier; /** * Check whether this controller is running on the given fabric, as represented diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m index 5393d7e006c640..68ee35b54aa4dc 100644 --- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -92,7 +92,7 @@ - (instancetype)initWithControllerID:(NSUUID *)controllerID securityLevel:(MTRStorageSecurityLevel)securityLevel sharingType:(MTRStorageSharingType)sharingType { - XCTAssertEqualObjects(_controllerID, controller.UUID); + XCTAssertEqualObjects(_controllerID, controller.uniqueIdentifier); __auto_type * data = self.storage[key]; if (data == nil) { @@ -113,7 +113,7 @@ - (BOOL)controller:(MTRDeviceController *)controller securityLevel:(MTRStorageSecurityLevel)securityLevel sharingType:(MTRStorageSharingType)sharingType { - XCTAssertEqualObjects(_controllerID, controller.UUID); + XCTAssertEqualObjects(_controllerID, controller.uniqueIdentifier); __auto_type * archiver = [[NSKeyedArchiver alloc] initRequiringSecureCoding:YES]; XCTAssertNotNil(archiver); @@ -132,7 +132,7 @@ - (BOOL)controller:(MTRDeviceController *)controller securityLevel:(MTRStorageSecurityLevel)securityLevel sharingType:(MTRStorageSharingType)sharingType { - XCTAssertEqualObjects(_controllerID, controller.UUID); + XCTAssertEqualObjects(_controllerID, controller.uniqueIdentifier); self.storage[key] = nil; return YES; } @@ -351,7 +351,7 @@ - (nullable MTRDeviceController *)startControllerWithRootKeys:(MTRTestKeys *)roo __auto_type * params = [[MTRDeviceControllerExternalCertificateStartupParameters alloc] initWithStorageDelegate:storage storageDelegateQueue:_storageQueue - UUID:storage.controllerID + uniqueIdentifier:storage.controllerID ipk:rootKeys.ipk vendorID:@(kTestVendorId) operationalKeypair:operationalKeys @@ -548,7 +548,7 @@ - (void)test003_TestTwoControllersSameUUID XCTAssertEqualObjects(controller.controllerNodeID, nodeID1); - // Try to bring up another controller with the same UUID (but a different + // Try to bring up another controller with the same uniqueIdentifier (but a different // node identity). This should fail, since our controller is still running. NSNumber * nodeID2 = @(789); MTRDeviceController * otherController = [self startControllerWithRootKeys:rootKeys From f7f4ffbcced2e2b1547a9187d53aebf4b03aec3e Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 29 Aug 2023 12:59:14 -0400 Subject: [PATCH 04/11] Apply spelling/grammar suggestions from code review Co-authored-by: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com> --- src/darwin/Framework/CHIP/MTRDemuxingStorage.mm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm b/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm index 49aa58dae187e6..87d0de07116056 100644 --- a/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm +++ b/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm @@ -122,7 +122,7 @@ static bool IsMemoryOnlyGlobalKey(NSString * key) if ([key isEqualToString:@"g/lkgt"]) { // Store Last Known Good Time in memory only. We never need this in - // general, because we can always provide wall-clock time. + // general, because we can always provide the wall-clock time. return true; } @@ -165,7 +165,7 @@ static bool IsMemoryOnlyGlobalKey(NSString * key) /** * Method to test whether an index-specific key should be stored in memory only, as * opposed to being passed on to the actual storage related to controllers. - * The key string will ahve the "f/index/" bit already stripped off the + * The key string will have the "f/index/" bit already stripped off the * front of the key. */ static bool IsMemoryOnlyIndexSpecificKey(NSString * key) From 73d668ff1f5909fbffcc5564f9f6ff2cb7dedd78 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 29 Aug 2023 14:05:00 -0400 Subject: [PATCH 05/11] Address review comments. --- .../Framework/CHIP/MTRDemuxingStorage.mm | 5 +- .../CHIP/MTRDeviceControllerDataStore.mm | 110 +++++++++--------- .../CHIP/MTRDeviceControllerFactory.h | 6 +- .../CHIP/MTRDeviceControllerFactory.mm | 4 +- 4 files changed, 64 insertions(+), 61 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm b/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm index 87d0de07116056..3112ba952a4e81 100644 --- a/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm +++ b/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm @@ -362,6 +362,7 @@ static bool IsMemoryOnlyIndexSpecificKey(NSString * key) CHIP_ERROR MTRDemuxingStorage::SetIndexSpecificValue(FabricIndex index, NSString * key, NSData * data) { if ([key isEqualToString:@"n"]) { + // Index-scoped "n" is NOC. auto * controller = [mFactory runningControllerForFabricIndex:index]; if (controller == nil) { return CHIP_ERROR_PERSISTED_STORAGE_FAILED; @@ -408,6 +409,8 @@ static bool IsMemoryOnlyIndexSpecificKey(NSString * key) CHIP_ERROR MTRDemuxingStorage::DeleteInMemoryValue(NSString * key) { BOOL present = (mInMemoryStore[key] != nil); - [mInMemoryStore removeObjectForKey:key]; + if (present) { + [mInMemoryStore removeObjectForKey:key]; + } return present ? CHIP_NO_ERROR : CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; } diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm index 19480d9f6dac78..f83e50daff72b4 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -19,7 +19,7 @@ // TODO: FIXME: Figure out whether these are good key strings. static NSString * sResumptionNodeListKey = @"caseResumptionNodeList"; -static NSString * sLastUsedNOCKey = @"sLastUsedControllerNOC"; +static NSString * sLastUsedNOCKey = @"lastUsedControllerNOC"; static NSString * ResumptionByNodeIDKey(NSNumber * nodeID) { @@ -33,8 +33,8 @@ } @implementation MTRDeviceControllerDataStore { - id _delegate; - dispatch_queue_t _delegateQueue; + id _storageDelegate; + dispatch_queue_t _storageDelegateQueue; MTRDeviceController * _controller; // Array of nodes with resumption info, oldest-stored first. NSMutableArray * _nodesWithResumptionInfo; @@ -49,15 +49,15 @@ - (instancetype)initWithController:(MTRDeviceController *)controller } _controller = controller; - _delegate = storageDelegate; - _delegateQueue = storageDelegateQueue; + _storageDelegate = storageDelegate; + _storageDelegateQueue = storageDelegateQueue; __block id resumptionNodeList; - dispatch_sync(_delegateQueue, ^{ - resumptionNodeList = [_delegate controller:_controller - valueForKey:sResumptionNodeListKey - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + dispatch_sync(_storageDelegateQueue, ^{ + resumptionNodeList = [_storageDelegate controller:_controller + valueForKey:sResumptionNodeListKey + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; }); if (resumptionNodeList != nil) { if (![resumptionNodeList isKindOfClass:[NSMutableArray class]]) { @@ -84,35 +84,35 @@ - (nullable MTRCASESessionResumptionInfo *)findResumptionInfoByResumptionID:(NSD - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo { auto * oldInfo = [self findResumptionInfoByNodeID:resumptionInfo.nodeID]; - dispatch_sync(_delegateQueue, ^{ + dispatch_sync(_storageDelegateQueue, ^{ if (oldInfo != nil) { // Remove old resumption id key. No need to do that for the // node id, because we are about to overwrite it. - [_delegate controller:_controller - removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_storageDelegate controller:_controller + removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; [_nodesWithResumptionInfo removeObject:resumptionInfo.nodeID]; } - [_delegate controller:_controller - storeValue:resumptionInfo - forKey:ResumptionByNodeIDKey(resumptionInfo.nodeID) - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; - [_delegate controller:_controller - storeValue:resumptionInfo - forKey:ResumptionByResumptionIDKey(resumptionInfo.resumptionID) - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_storageDelegate controller:_controller + storeValue:resumptionInfo + forKey:ResumptionByNodeIDKey(resumptionInfo.nodeID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_storageDelegate controller:_controller + storeValue:resumptionInfo + forKey:ResumptionByResumptionIDKey(resumptionInfo.resumptionID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; // Update our resumption info node list. [_nodesWithResumptionInfo addObject:resumptionInfo.nodeID]; - [_delegate controller:_controller - storeValue:_nodesWithResumptionInfo - forKey:sResumptionNodeListKey - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_storageDelegate controller:_controller + storeValue:_nodesWithResumptionInfo + forKey:sResumptionNodeListKey + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; }); } @@ -123,15 +123,15 @@ - (void)clearAllResumptionInfo for (NSNumber * nodeID in _nodesWithResumptionInfo) { auto * oldInfo = [self findResumptionInfoByNodeID:nodeID]; if (oldInfo != nil) { - dispatch_sync(_delegateQueue, ^{ - [_delegate controller:_controller - removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; - [_delegate controller:_controller - removeValueForKey:ResumptionByNodeIDKey(oldInfo.nodeID) - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + dispatch_sync(_storageDelegateQueue, ^{ + [_storageDelegate controller:_controller + removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_storageDelegate controller:_controller + removeValueForKey:ResumptionByNodeIDKey(oldInfo.nodeID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; }); } } @@ -142,12 +142,12 @@ - (void)clearAllResumptionInfo - (CHIP_ERROR)storeLastUsedNOC:(MTRCertificateTLVBytes)noc { __block BOOL ok; - dispatch_sync(_delegateQueue, ^{ - ok = [_delegate controller:_controller - storeValue:noc - forKey:sLastUsedNOCKey - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityRequired]; + dispatch_sync(_storageDelegateQueue, ^{ + ok = [_storageDelegate controller:_controller + storeValue:noc + forKey:sLastUsedNOCKey + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityRequired]; }); return ok ? CHIP_NO_ERROR : CHIP_ERROR_PERSISTED_STORAGE_FAILED; } @@ -155,11 +155,11 @@ - (CHIP_ERROR)storeLastUsedNOC:(MTRCertificateTLVBytes)noc - (MTRCertificateTLVBytes _Nullable)fetchLastUsedNOC { __block id data; - dispatch_sync(_delegateQueue, ^{ - data = [_delegate controller:_controller - valueForKey:sLastUsedNOCKey - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityRequired]; + dispatch_sync(_storageDelegateQueue, ^{ + data = [_storageDelegate controller:_controller + valueForKey:sLastUsedNOCKey + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityRequired]; }); if (data == nil) { @@ -176,11 +176,11 @@ - (MTRCertificateTLVBytes _Nullable)fetchLastUsedNOC - (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(NSString *)key { __block id resumptionInfo; - dispatch_sync(_delegateQueue, ^{ - resumptionInfo = [_delegate controller:_controller - valueForKey:key - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + dispatch_sync(_storageDelegateQueue, ^{ + resumptionInfo = [_storageDelegate controller:_controller + valueForKey:key + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; }); if (resumptionInfo == nil) { diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h index 8866b4ae55d532..6471db8b005ccb 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h @@ -40,9 +40,9 @@ API_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)) /* * Storage used to store persistent information for the fabrics the * controllers ends up interacting with. This is only used if "initWithStorage" - * is used to initialize the MTRDeviceControllerFactoryParams. If init is used, - * called. If "init" is used, this property will contain a dummy storage that - * will not be used for anything. + * is used to initialize the MTRDeviceControllerFactoryParams. If "init" is + * used, this property will contain a dummy storage that will not be used for + * anything. */ @property (nonatomic, strong, readonly) id storage; diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index cdd40bfb3da810..6e5f581ffe257b 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -100,7 +100,7 @@ @interface MTRDeviceControllerFactory () // the Matter queue. The way this lock is used assumes that: // // 1) The only mutating accesses to the controllers array and the ivars happen -// when the current queue is not the Matter queue or when in a block that was +// when the current queue is not the Matter queue or in a block that was // sync-dispatched to the Matter queue. This is a good assumption, because // the implementations of the functions that mutate these do sync dispatch to // the Matter queue, which would deadlock if they were called when that queue @@ -110,7 +110,7 @@ @interface MTRDeviceControllerFactory () // outside. // // These assumptions mean that if we are in a block that was sync-dispatched to -// the Matter queue, that block cannot race with either the MAtter queue nor the +// the Matter queue, that block cannot race with either the Matter queue nor the // non-Matter queue. Similarly, if we are in a situation where the Matter queue // has been shut down, any accesses to the variables cannot race anything else. // From e6278e23c9ed45afa2ea65354067ac56155eaec0 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 29 Aug 2023 15:50:53 -0400 Subject: [PATCH 06/11] Address more review comments. --- src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm | 5 ++++- .../Framework/CHIP/MTRDeviceControllerStartupParams.mm | 6 ++++++ .../Framework/CHIP/MTRDeviceControllerStorageDelegate.h | 4 ++-- .../Framework/CHIPTests/MTRPerControllerStorageTests.m | 2 +- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index 6e5f581ffe257b..95282450952ef2 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -650,11 +650,14 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(id)startupParams storageDelegate = params.storageDelegate; storageDelegateQueue = params.storageDelegateQueue; uniqueIdentifier = params.uniqueIdentifier; - } else { + } else if ([startupParams isKindOfClass:[MTRDeviceControllerStartupParams class]]) { MTRDeviceControllerStartupParams * params = startupParams; storageDelegate = nil; storageDelegateQueue = nil; uniqueIdentifier = params.uniqueIdentifier; + } else { + MTR_LOG_ERROR("Unknown kind of startup params: %@", startupParams); + return nil; } if (_usingPerControllerStorage && storageDelegate == nil) { diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm index 232750d6ab2317..4584bbf8e4af9b 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm @@ -164,6 +164,12 @@ - (instancetype)initWithParameters:(MTRDeviceControllerStartupParameters *)param _ipk = params.ipk; _vendorID = params.vendorID; + // Note: Since we have an operationalCertificate, we do not need a nodeID as + // part of our params; it will not be used. Don't even initialize it, to + // avoid confusion about that. + // + // We don't really use the fabricID for anything either, but we promise to + // have a non-nil one, which is why we set it above. _nodeID = nil; _caseAuthenticatedTags = nil; _rootCertificate = params.rootCertificate; diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h b/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h index 570ff2ff58aa43..4f19ac81fd33d4 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h @@ -1,5 +1,5 @@ /** - * Copyright (c) 2022-2023 Project CHIP Authors + * Copyright (c) 2023 Project CHIP Authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -62,7 +62,7 @@ typedef NS_ENUM(NSUInteger, MTRStorageSharingType) { * it is to get its controllerID. * * 2) The delegate method calls will happen on the queue that was provided along - * with the gelegate. All Matter work will be blocked until the method + * with the delegate. All Matter work will be blocked until the method * completes, and these calls may themselves block other Matter API calls * from completing. Attempting to call any Matter API on the queue used for * this delegate, apart from de-serializing and serializing the items being diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m index 68ee35b54aa4dc..d5cd419e477063 100644 --- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022-2023 Project CHIP Authors + * Copyright (c) 2023 Project CHIP Authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. From acfabfa861ae463e345800b61a560c53ea6f2ca4 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 31 Aug 2023 12:21:32 -0400 Subject: [PATCH 07/11] Address more review comments. --- .../Framework/CHIP/MTRDemuxingStorage.mm | 2 +- .../CHIP/MTRDeviceControllerDataStore.h | 15 ++--- .../CHIP/MTRDeviceControllerDataStore.mm | 60 ++++++++++++------- .../CHIP/MTRDeviceControllerStartupParams.mm | 2 +- .../CHIP/MTRDeviceControllerStorageDelegate.h | 17 ++---- .../CHIPTests/MTRPerControllerStorageTests.m | 3 - 6 files changed, 51 insertions(+), 48 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm b/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm index 3112ba952a4e81..5d4162ef321c55 100644 --- a/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm +++ b/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm @@ -368,7 +368,7 @@ static bool IsMemoryOnlyIndexSpecificKey(NSString * key) return CHIP_ERROR_PERSISTED_STORAGE_FAILED; } - ReturnErrorOnFailure([controller.controllerDataStore storeLastUsedNOC:data]); + ReturnErrorOnFailure([controller.controllerDataStore storeLastLocallyUsedNOC:data]); } if (IsMemoryOnlyIndexSpecificKey(key)) { diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h index dedc3e2e0e95bb..8d10c7af3e810b 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h @@ -41,9 +41,9 @@ MTR_HIDDEN MTR_HIDDEN @interface MTRDeviceControllerDataStore : NSObject -- (instancetype)initWithController:(MTRDeviceController *)controller - storageDelegate:(id)storageDelegate - storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue; +- (nullable instancetype)initWithController:(MTRDeviceController *)controller + storageDelegate:(id)storageDelegate + storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue; /** * Resumption info APIs. @@ -54,11 +54,12 @@ MTR_HIDDEN - (void)clearAllResumptionInfo; /** - * APIs for storing/retrieving last-used fabric state. These are used at - * startup to make sure the new state and the old state are merged properly. + * Storage of the last NOC we used on this device. This is local-only storage, + * because it's used to invalidate (or not) the local-only session resumption + * storage. */ -- (CHIP_ERROR)storeLastUsedNOC:(MTRCertificateTLVBytes)noc; -- (MTRCertificateTLVBytes _Nullable)fetchLastUsedNOC; +- (CHIP_ERROR)storeLastLocallyUsedNOC:(MTRCertificateTLVBytes)noc; +- (MTRCertificateTLVBytes _Nullable)fetchLastLocallyUsedNOC; @end diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm index f83e50daff72b4..37778a185f118b 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -17,9 +17,9 @@ #include "MTRDeviceControllerDataStore.h" #import "MTRLogging_Internal.h" -// TODO: FIXME: Figure out whether these are good key strings. +// FIXME: Are these good key strings? https://github.com/project-chip/connectedhomeip/issues/28973 static NSString * sResumptionNodeListKey = @"caseResumptionNodeList"; -static NSString * sLastUsedNOCKey = @"lastUsedControllerNOC"; +static NSString * sLastLocallyUsedNOCKey = @"lastLocallyUsedControllerNOC"; static NSString * ResumptionByNodeIDKey(NSNumber * nodeID) { @@ -40,9 +40,9 @@ @implementation MTRDeviceControllerDataStore { NSMutableArray * _nodesWithResumptionInfo; } -- (instancetype)initWithController:(MTRDeviceController *)controller - storageDelegate:(id)storageDelegate - storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue +- (nullable instancetype)initWithController:(MTRDeviceController *)controller + storageDelegate:(id)storageDelegate + storageDelegateQueue:(dispatch_queue_t)storageDelegateQueue { if (!(self = [super init])) { return nil; @@ -57,7 +57,7 @@ - (instancetype)initWithController:(MTRDeviceController *)controller resumptionNodeList = [_storageDelegate controller:_controller valueForKey:sResumptionNodeListKey securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + sharingType:MTRStorageSharingTypeNotShared]; }); if (resumptionNodeList != nil) { if (![resumptionNodeList isKindOfClass:[NSMutableArray class]]) { @@ -83,6 +83,7 @@ - (nullable MTRCASESessionResumptionInfo *)findResumptionInfoByResumptionID:(NSD - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo { + // TODO: THis should be using the "same acls" scoping, not "same identity". auto * oldInfo = [self findResumptionInfoByNodeID:resumptionInfo.nodeID]; dispatch_sync(_storageDelegateQueue, ^{ if (oldInfo != nil) { @@ -91,7 +92,7 @@ - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo [_storageDelegate controller:_controller removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + sharingType:MTRStorageSharingTypeNotShared]; [_nodesWithResumptionInfo removeObject:resumptionInfo.nodeID]; } @@ -99,12 +100,12 @@ - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo storeValue:resumptionInfo forKey:ResumptionByNodeIDKey(resumptionInfo.nodeID) securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + sharingType:MTRStorageSharingTypeNotShared]; [_storageDelegate controller:_controller storeValue:resumptionInfo forKey:ResumptionByResumptionIDKey(resumptionInfo.resumptionID) securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + sharingType:MTRStorageSharingTypeNotShared]; // Update our resumption info node list. [_nodesWithResumptionInfo addObject:resumptionInfo.nodeID]; @@ -112,7 +113,7 @@ - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo storeValue:_nodesWithResumptionInfo forKey:sResumptionNodeListKey securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + sharingType:MTRStorageSharingTypeNotShared]; }); } @@ -127,11 +128,11 @@ - (void)clearAllResumptionInfo [_storageDelegate controller:_controller removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + sharingType:MTRStorageSharingTypeNotShared]; [_storageDelegate controller:_controller removeValueForKey:ResumptionByNodeIDKey(oldInfo.nodeID) securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + sharingType:MTRStorageSharingTypeNotShared]; }); } } @@ -139,27 +140,27 @@ - (void)clearAllResumptionInfo [_nodesWithResumptionInfo removeAllObjects]; } -- (CHIP_ERROR)storeLastUsedNOC:(MTRCertificateTLVBytes)noc +- (CHIP_ERROR)storeLastLocallyUsedNOC:(MTRCertificateTLVBytes)noc { __block BOOL ok; dispatch_sync(_storageDelegateQueue, ^{ ok = [_storageDelegate controller:_controller storeValue:noc - forKey:sLastUsedNOCKey + forKey:sLastLocallyUsedNOCKey securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityRequired]; + sharingType:MTRStorageSharingTypeNotShared]; }); return ok ? CHIP_NO_ERROR : CHIP_ERROR_PERSISTED_STORAGE_FAILED; } -- (MTRCertificateTLVBytes _Nullable)fetchLastUsedNOC +- (MTRCertificateTLVBytes _Nullable)fetchLastLocallyUsedNOC { __block id data; dispatch_sync(_storageDelegateQueue, ^{ data = [_storageDelegate controller:_controller - valueForKey:sLastUsedNOCKey + valueForKey:sLastLocallyUsedNOCKey securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityRequired]; + sharingType:MTRStorageSharingTypeNotShared]; }); if (data == nil) { @@ -173,14 +174,19 @@ - (MTRCertificateTLVBytes _Nullable)fetchLastUsedNOC return data; } -- (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(NSString *)key +- (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(nullable NSString *)key { + // key could be nil if [NSString stringWithFormat] returns nil for some reason. + if (key == nil) { + return nil; + } + __block id resumptionInfo; dispatch_sync(_storageDelegateQueue, ^{ resumptionInfo = [_storageDelegate controller:_controller valueForKey:key securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + sharingType:MTRStorageSharingTypeNotShared]; }); if (resumptionInfo == nil) { @@ -210,12 +216,18 @@ + (BOOL)supportsSecureCoding return YES; } -- (instancetype _Nullable)initWithCoder:(NSCoder *)decoder +- (instancetype)initWithCoder:(NSCoder *)decoder { + self = [super init]; + if (self == nil) { + return nil; + } + _nodeID = [decoder decodeObjectOfClass:[NSNumber class] forKey:sNodeIDKey]; _resumptionID = [decoder decodeObjectOfClass:[NSData class] forKey:sResumptionIDKey]; _sharedSecret = [decoder decodeObjectOfClass:[NSData class] forKey:sSharedSecretKey]; - _caseAuthenticatedTags = [decoder decodeObjectOfClass:[NSSet class] forKey:sCATsKey]; + auto caseAuthenticatedTagArray = [decoder decodeArrayOfObjectsOfClass:[NSNumber class] forKey:sCATsKey]; + _caseAuthenticatedTags = [NSSet setWithArray:caseAuthenticatedTagArray]; return self; } @@ -224,7 +236,9 @@ - (void)encodeWithCoder:(NSCoder *)coder [coder encodeObject:self.nodeID forKey:sNodeIDKey]; [coder encodeObject:self.resumptionID forKey:sResumptionIDKey]; [coder encodeObject:self.sharedSecret forKey:sSharedSecretKey]; - [coder encodeObject:self.caseAuthenticatedTags forKey:sCATsKey]; + // Encode the CATs as an array, so that we can decodeArrayOfObjectsOfClass + // to get type-safe decoding for them. + [coder encodeObject:[self.caseAuthenticatedTags allObjects] forKey:sCATsKey]; } @end diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm index 4584bbf8e4af9b..9fb71429a07720 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStartupParams.mm @@ -554,7 +554,7 @@ - (instancetype)initForNewController:(MTRDeviceController *)controller return nil; } - auto * oldNOCTLV = [controller.controllerDataStore fetchLastUsedNOC]; + auto * oldNOCTLV = [controller.controllerDataStore fetchLastLocallyUsedNOC]; if (oldNOCTLV != nil) { ByteSpan oldNOCSpan = AsByteSpan(oldNOCTLV); diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h b/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h index 4f19ac81fd33d4..1fee9c65ae460c 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerStorageDelegate.h @@ -30,26 +30,17 @@ typedef NS_ENUM(NSUInteger, MTRStorageSharingType) { // Data must not be shared at all (just store locally). MTRStorageSharingTypeNotShared, - // Data may be shared, but only between controllers that have the same node - // identity (same fabric, same node ID). Sharing not required (could be - // stored locally). - MTRStorageSharingTypeSameIdentityAllowed, - // Data must be shared, but only between controllers that have the same node - // identity (same fabric, same node ID). - MTRStorageSharingTypeSameIdentityRequired, + // identity (same fabric, same node ID, same CATs). + MTRStorageSharingTypeSameIdentity, - // Data may be shared, but only between controllers that have the same + // Data must be shared, but only between controllers that have the same // access to devices (e.g. controllers that all have the same CATs if ACLs // are being done via CATs). MTRStorageSharingTypeSameACLs, - // Data may be, but does not have to be, shared across all controllers on a - // given fabric. - MTRStorageSharingTypeSameFabricAllowed, - // Data must be shared across all controllers on a given fabric. - MTRStorageSharingTypeSameFabricRequired, + MTRStorageSharingTypeSameFabric, } MTR_NEWLY_AVAILABLE; /** diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m index d5cd419e477063..fdf03eab12ba36 100644 --- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -115,9 +115,6 @@ - (BOOL)controller:(MTRDeviceController *)controller { XCTAssertEqualObjects(_controllerID, controller.uniqueIdentifier); - __auto_type * archiver = [[NSKeyedArchiver alloc] initRequiringSecureCoding:YES]; - XCTAssertNotNil(archiver); - NSError * error; NSData * data = [NSKeyedArchiver archivedDataWithRootObject:value requiringSecureCoding:YES error:&error]; XCTAssertNil(error); From acde7a7586f663de5d7b396c38bcdde7cc933807 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 31 Aug 2023 12:38:36 -0400 Subject: [PATCH 08/11] Address more review comments. --- .../Framework/CHIP/MTRDeviceControllerDataStore.mm | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm index 37778a185f118b..addc3109c397bd 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -60,11 +60,18 @@ - (nullable instancetype)initWithController:(MTRDeviceController *)controller sharingType:MTRStorageSharingTypeNotShared]; }); if (resumptionNodeList != nil) { - if (![resumptionNodeList isKindOfClass:[NSMutableArray class]]) { + if (![resumptionNodeList isKindOfClass:[NSArray class]]) { MTR_LOG_ERROR("List of CASE resumption node IDs is not an array"); return nil; } - _nodesWithResumptionInfo = resumptionNodeList; + for (id value in resumptionNodeList) { + if (![value isKindOfClass:[NSNumber class]]) { + MTR_LOG_ERROR("List of CASE resumption node IDs contains a non-number"); + return nil; + } + } + _nodesWithResumptionInfo = [NSMutableArray arrayWithCapacity:[resumptionNodeList count]]; + [_nodesWithResumptionInfo addObjectsFromArray:resumptionNodeList]; } else { _nodesWithResumptionInfo = [[NSMutableArray alloc] init]; } @@ -83,7 +90,6 @@ - (nullable MTRCASESessionResumptionInfo *)findResumptionInfoByResumptionID:(NSD - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo { - // TODO: THis should be using the "same acls" scoping, not "same identity". auto * oldInfo = [self findResumptionInfoByNodeID:resumptionInfo.nodeID]; dispatch_sync(_storageDelegateQueue, ^{ if (oldInfo != nil) { @@ -110,7 +116,7 @@ - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo // Update our resumption info node list. [_nodesWithResumptionInfo addObject:resumptionInfo.nodeID]; [_storageDelegate controller:_controller - storeValue:_nodesWithResumptionInfo + storeValue:[NSArray arrayWithArray:_nodesWithResumptionInfo] forKey:sResumptionNodeListKey securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared]; From f2b9084a8adbb16c57d79c97da27b08b6aa4fad4 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 31 Aug 2023 14:02:17 -0400 Subject: [PATCH 09/11] Add some validation when deserializing node IDs and CATs. --- .../CHIP/MTRDeviceControllerDataStore.mm | 80 ++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm index addc3109c397bd..0a15aa48f97f3f 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -17,6 +17,10 @@ #include "MTRDeviceControllerDataStore.h" #import "MTRLogging_Internal.h" +#include +#include +#include + // FIXME: Are these good key strings? https://github.com/project-chip/connectedhomeip/issues/28973 static NSString * sResumptionNodeListKey = @"caseResumptionNodeList"; static NSString * sLastLocallyUsedNOCKey = @"lastLocallyUsedControllerNOC"; @@ -32,6 +36,60 @@ [NSString stringWithFormat:@"caseResumptionByResumptionID/%s", [resumptionID base64EncodedStringWithOptions:0].UTF8String]; } +static bool IsUnsignedIntegerNumber(NSNumber * _Nullable number) +{ + if (number == nil) { + return false; + } + + // Not sure how to check for the number being an integer. + + if ([number compare:@(0)] == NSOrderedAscending) { + return false; + } + + return true; +} + +static bool IsValidNodeIDNumber(NSNumber * _Nullable number) +{ + // Node IDs cannot be negative. + if (!IsUnsignedIntegerNumber(number)) { + return false; + } + + // Validate that this is a valid operational ID, not some garbage unsigned + // int value that can't be a node id. + uint64_t unsignedValue = number.unsignedLongLongValue; + if (!chip::IsOperationalNodeId(unsignedValue)) { + return false; + } + + return true; +} + +static bool IsValidCATNumber(NSNumber * _Nullable number) +{ + // CATs cannot be negative. + if (!IsUnsignedIntegerNumber(number)) { + return false; + } + + // Validate that this is a valid CAT value and, not some garbage unsigned int + // value that can't be a CAT. + uint64_t unsignedValue = number.unsignedLongLongValue; + if (!chip::CanCastTo(unsignedValue)) { + return false; + } + + auto tag = static_cast(unsignedValue); + if (!chip::IsValidCASEAuthTag(tag)) { + return false; + } + + return true; +} + @implementation MTRDeviceControllerDataStore { id _storageDelegate; dispatch_queue_t _storageDelegateQueue; @@ -69,6 +127,11 @@ - (nullable instancetype)initWithController:(MTRDeviceController *)controller MTR_LOG_ERROR("List of CASE resumption node IDs contains a non-number"); return nil; } + + if (!IsValidNodeIDNumber(value)) { + MTR_LOG_ERROR("Resumption node ID contains invalid value: %@", value); + return nil; + } } _nodesWithResumptionInfo = [NSMutableArray arrayWithCapacity:[resumptionNodeList count]]; [_nodesWithResumptionInfo addObjectsFromArray:resumptionNodeList]; @@ -222,7 +285,7 @@ + (BOOL)supportsSecureCoding return YES; } -- (instancetype)initWithCoder:(NSCoder *)decoder +- (nullable instancetype)initWithCoder:(NSCoder *)decoder { self = [super init]; if (self == nil) { @@ -230,9 +293,24 @@ - (instancetype)initWithCoder:(NSCoder *)decoder } _nodeID = [decoder decodeObjectOfClass:[NSNumber class] forKey:sNodeIDKey]; + if (!IsValidNodeIDNumber(_nodeID)) { + MTR_LOG_ERROR("MTRCASESessionResumptionInfo node ID has invalid value: %@", _nodeID); + return nil; + } + _resumptionID = [decoder decodeObjectOfClass:[NSData class] forKey:sResumptionIDKey]; _sharedSecret = [decoder decodeObjectOfClass:[NSData class] forKey:sSharedSecretKey]; auto caseAuthenticatedTagArray = [decoder decodeArrayOfObjectsOfClass:[NSNumber class] forKey:sCATsKey]; + + for (NSNumber * value in caseAuthenticatedTagArray) { + if (!IsValidCATNumber(value)) { + MTR_LOG_ERROR("MTRCASESessionResumptionInfo CASE tag has invalid value: %@", value); + return nil; + } + + // Range-checking will be done when we try to convert the set to CATValues. + } + _caseAuthenticatedTags = [NSSet setWithArray:caseAuthenticatedTagArray]; return self; } From 2ab709da18809e3fc2905119b03b6226aeb0b7fd Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 31 Aug 2023 14:40:43 -0400 Subject: [PATCH 10/11] Stop trusting the secure coding stuff to actually enforce types correctly. For built-in types it seems to not do that. --- .../CHIP/MTRDeviceControllerDataStore.mm | 46 +++++++++++++------ 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm index 0a15aa48f97f3f..bb08faad584e14 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -36,12 +36,18 @@ [NSString stringWithFormat:@"caseResumptionByResumptionID/%s", [resumptionID base64EncodedStringWithOptions:0].UTF8String]; } -static bool IsUnsignedIntegerNumber(NSNumber * _Nullable number) +static bool IsUnsignedIntegerNumber(id _Nullable value) { - if (number == nil) { + if (value == nil) { return false; } + if (![value isKindOfClass:[NSNumber class]]) { + return false; + } + + NSNumber * number = value; + // Not sure how to check for the number being an integer. if ([number compare:@(0)] == NSOrderedAscending) { @@ -51,13 +57,15 @@ static bool IsUnsignedIntegerNumber(NSNumber * _Nullable number) return true; } -static bool IsValidNodeIDNumber(NSNumber * _Nullable number) +static bool IsValidNodeIDNumber(id _Nullable value) { // Node IDs cannot be negative. - if (!IsUnsignedIntegerNumber(number)) { + if (!IsUnsignedIntegerNumber(value)) { return false; } + NSNumber * number = value; + // Validate that this is a valid operational ID, not some garbage unsigned // int value that can't be a node id. uint64_t unsignedValue = number.unsignedLongLongValue; @@ -68,13 +76,15 @@ static bool IsValidNodeIDNumber(NSNumber * _Nullable number) return true; } -static bool IsValidCATNumber(NSNumber * _Nullable number) +static bool IsValidCATNumber(id _Nullable value) { // CATs cannot be negative. - if (!IsUnsignedIntegerNumber(number)) { + if (!IsUnsignedIntegerNumber(value)) { return false; } + NSNumber * number = value; + // Validate that this is a valid CAT value and, not some garbage unsigned int // value that can't be a CAT. uint64_t unsignedValue = number.unsignedLongLongValue; @@ -123,11 +133,6 @@ - (nullable instancetype)initWithController:(MTRDeviceController *)controller return nil; } for (id value in resumptionNodeList) { - if (![value isKindOfClass:[NSNumber class]]) { - MTR_LOG_ERROR("List of CASE resumption node IDs contains a non-number"); - return nil; - } - if (!IsValidNodeIDNumber(value)) { MTR_LOG_ERROR("Resumption node ID contains invalid value: %@", value); return nil; @@ -293,16 +298,31 @@ - (nullable instancetype)initWithCoder:(NSCoder *)decoder } _nodeID = [decoder decodeObjectOfClass:[NSNumber class] forKey:sNodeIDKey]; + // For some built-in classes decoder will decode to them even if we ask for a + // different class (!). So sanity-check what we got. + if (_nodeID != nil && ![_nodeID isKindOfClass:[NSNumber class]]) { + MTR_LOG_ERROR("MTRCASESessionResumptionInfo got %@ for node ID, not NSNumber.", _nodeID); + return nil; + } if (!IsValidNodeIDNumber(_nodeID)) { MTR_LOG_ERROR("MTRCASESessionResumptionInfo node ID has invalid value: %@", _nodeID); return nil; } _resumptionID = [decoder decodeObjectOfClass:[NSData class] forKey:sResumptionIDKey]; + if (_resumptionID != nil && ![_resumptionID isKindOfClass:[NSData class]]) { + MTR_LOG_ERROR("MTRCASESessionResumptionInfo got %@ for resumption ID, not NSData.", _resumptionID); + return nil; + } + _sharedSecret = [decoder decodeObjectOfClass:[NSData class] forKey:sSharedSecretKey]; - auto caseAuthenticatedTagArray = [decoder decodeArrayOfObjectsOfClass:[NSNumber class] forKey:sCATsKey]; + if (_sharedSecret != nil && ![_sharedSecret isKindOfClass:[NSData class]]) { + MTR_LOG_ERROR("MTRCASESessionResumptionInfo got %@ for shared secret, not NSData.", _sharedSecret); + return nil; + } - for (NSNumber * value in caseAuthenticatedTagArray) { + auto caseAuthenticatedTagArray = [decoder decodeArrayOfObjectsOfClass:[NSNumber class] forKey:sCATsKey]; + for (id value in caseAuthenticatedTagArray) { if (!IsValidCATNumber(value)) { MTR_LOG_ERROR("MTRCASESessionResumptionInfo CASE tag has invalid value: %@", value); return nil; From 84795249ed32f22fe4140d9dd4f635d24d369f6c Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 31 Aug 2023 14:50:52 -0400 Subject: [PATCH 11/11] Remove NSMutableArray and NSSet from controller storage classes, since we no longer encode those. --- src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm index bb08faad584e14..4732009ade3513 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -350,7 +350,10 @@ - (void)encodeWithCoder:(NSCoder *)coder NSSet * MTRDeviceControllerStorageClasses() { static NSSet * const sStorageClasses = [NSSet setWithArray:@[ - [NSNumber class], [NSData class], [NSSet class], [MTRCASESessionResumptionInfo class], [NSMutableArray class] + [NSNumber class], + [NSData class], + [NSArray class], + [MTRCASESessionResumptionInfo class], ]]; return sStorageClasses; }