From 7514699c0979758d93ddc85d65e07ea75f1a60da Mon Sep 17 00:00:00 2001 From: Jerry Johns Date: Thu, 7 Jul 2022 09:41:56 -0700 Subject: [PATCH] Default initialize controller vendor ID (#20017) * Default initialize controller vendor ID Problem: The Python REPL was failing to commission a device when built with detail logging disabled. Cause: It was not correctly initializing the Controller::SetupParams::controllerVendorId when creating a commissioner instance. This resulted in it using garbage values that sometimes, were correct and not zero. Fix: As per discussion here, this default initializes that field to VendorId::Unspecified as well as adding an early check for a valid value at controller creation time. This ensures that applications creating commissioners are forced to explicitly provide a value for their vendor ID. It also ensures we catch applications who don't do so early at commissioner instantiation time. Testing: Validate that the REPL and REPL tests pass correctly. For other platforms, will have to check the various PR tests and see which ones fail. * Review feedback * Rebase and fix-up test logic that got added recently --- src/controller/CHIPDeviceControllerFactory.cpp | 5 +++++ src/controller/CHIPDeviceControllerFactory.h | 6 +++++- src/controller/python/OpCredsBinding.cpp | 5 +++-- src/controller/python/chip/ChipDeviceCtrl.py | 4 ++-- src/controller/python/chip/ChipReplStartup.py | 10 +++++++--- src/controller/python/chip/FabricAdmin.py | 18 +++++++++++++----- .../python/test/test_scripts/base.py | 17 +++++++++-------- 7 files changed, 44 insertions(+), 21 deletions(-) diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index ef9aaaa77ae72c..84f67a96883003 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -291,6 +291,8 @@ void DeviceControllerFactory::PopulateInitParams(ControllerInitParams & controll CHIP_ERROR DeviceControllerFactory::SetupController(SetupParams params, DeviceController & controller) { VerifyOrReturnError(mSystemState != nullptr, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(params.controllerVendorId != VendorId::Unspecified, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorOnFailure(InitSystemState()); ControllerInitParams controllerParams; @@ -303,9 +305,12 @@ CHIP_ERROR DeviceControllerFactory::SetupController(SetupParams params, DeviceCo CHIP_ERROR DeviceControllerFactory::SetupCommissioner(SetupParams params, DeviceCommissioner & commissioner) { VerifyOrReturnError(mSystemState != nullptr, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(params.controllerVendorId != VendorId::Unspecified, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorOnFailure(InitSystemState()); CommissionerInitParams commissionerParams; + // PopulateInitParams works against ControllerInitParams base class of CommissionerInitParams only PopulateInitParams(commissionerParams, params); diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index f5cbdbd3ecae03..75eeef74bb2ddc 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -60,7 +60,11 @@ struct SetupParams ByteSpan controllerICAC; ByteSpan controllerRCAC; - chip::VendorId controllerVendorId; + // + // This must be set to a valid, operational VendorId value associated with + // the controller/commissioner. + // + chip::VendorId controllerVendorId = VendorId::Unspecified; // The Device Pairing Delegated used to initialize a Commissioner DevicePairingDelegate * pairingDelegate = nullptr; diff --git a/src/controller/python/OpCredsBinding.cpp b/src/controller/python/OpCredsBinding.cpp index 966950fc241e94..e507967ef9f325 100644 --- a/src/controller/python/OpCredsBinding.cpp +++ b/src/controller/python/OpCredsBinding.cpp @@ -325,8 +325,8 @@ void pychip_OnCommissioningStatusUpdate(chip::PeerId peerId, chip::Controller::C ChipError::StorageType pychip_OpCreds_AllocateController(OpCredsContext * context, chip::Controller::DeviceCommissioner ** outDevCtrl, uint8_t fabricIndex, - FabricId fabricId, chip::NodeId nodeId, const char * paaTrustStorePath, - bool useTestCommissioner) + FabricId fabricId, chip::NodeId nodeId, chip::VendorId adminVendorId, + const char * paaTrustStorePath, bool useTestCommissioner) { ChipLogDetail(Controller, "Creating New Device Controller"); @@ -373,6 +373,7 @@ ChipError::StorageType pychip_OpCreds_AllocateController(OpCredsContext * contex initParams.controllerICAC = icacSpan; initParams.controllerNOC = nocSpan; initParams.enableServerInteractions = true; + initParams.controllerVendorId = adminVendorId; if (useTestCommissioner) { diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py index 70aa7cada7b5f3..0cfa53f3bd8002 100644 --- a/src/controller/python/chip/ChipDeviceCtrl.py +++ b/src/controller/python/chip/ChipDeviceCtrl.py @@ -120,7 +120,7 @@ class DCState(enum.IntEnum): class ChipDeviceController(): activeList = set() - def __init__(self, opCredsContext: ctypes.c_void_p, fabricId: int, fabricIndex: int, nodeId: int, paaTrustStorePath: str = "", useTestCommissioner: bool = False): + def __init__(self, opCredsContext: ctypes.c_void_p, fabricId: int, fabricIndex: int, nodeId: int, adminVendorId: int, paaTrustStorePath: str = "", useTestCommissioner: bool = False): self.state = DCState.NOT_INITIALIZED self.devCtrl = None self._ChipStack = builtins.chipStack @@ -134,7 +134,7 @@ def __init__(self, opCredsContext: ctypes.c_void_p, fabricId: int, fabricIndex: res = self._ChipStack.Call( lambda: self._dmLib.pychip_OpCreds_AllocateController(ctypes.c_void_p( - opCredsContext), pointer(devCtrl), fabricIndex, fabricId, nodeId, ctypes.c_char_p(None if len(paaTrustStorePath) == 0 else str.encode(paaTrustStorePath)), useTestCommissioner) + opCredsContext), pointer(devCtrl), fabricIndex, fabricId, nodeId, adminVendorId, ctypes.c_char_p(None if len(paaTrustStorePath) == 0 else str.encode(paaTrustStorePath)), useTestCommissioner) ) if res != 0: diff --git a/src/controller/python/chip/ChipReplStartup.py b/src/controller/python/chip/ChipReplStartup.py index efdf8ea4cb3b6b..2e0a09a26ca542 100644 --- a/src/controller/python/chip/ChipReplStartup.py +++ b/src/controller/python/chip/ChipReplStartup.py @@ -37,7 +37,11 @@ def LoadFabricAdmins(): except KeyError: console.print( "\n[purple]No previous fabric admins discovered in persistent storage - creating a new one...") - _fabricAdmins.append(chip.FabricAdmin.FabricAdmin()) + + # + # Initialite a FabricAdmin with a VendorID of TestVendor1 (0xfff1) + # + _fabricAdmins.append(chip.FabricAdmin.FabricAdmin(0XFFF1)) return _fabricAdmins console.print('\n') @@ -45,8 +49,8 @@ def LoadFabricAdmins(): for k in adminList: console.print( f"[purple]Restoring FabricAdmin from storage to manage FabricId {adminList[k]['fabricId']}, FabricIndex {k}...") - _fabricAdmins.append(chip.FabricAdmin.FabricAdmin( - fabricId=adminList[k]['fabricId'], fabricIndex=int(k))) + _fabricAdmins.append(chip.FabricAdmin.FabricAdmin(vendorId=int(adminList[k]['vendorId']), + fabricId=adminList[k]['fabricId'], fabricIndex=int(k))) console.print( '\n[blue]Fabric Admins have been loaded and are available at [red]fabricAdmins') diff --git a/src/controller/python/chip/FabricAdmin.py b/src/controller/python/chip/FabricAdmin.py index 49cd924e561354..cd0a5f7b7aa81c 100644 --- a/src/controller/python/chip/FabricAdmin.py +++ b/src/controller/python/chip/FabricAdmin.py @@ -65,10 +65,11 @@ class FabricAdmin: ''' _handle = chip.native.GetLibraryHandle() + _isActive = False activeFabricIndexList = set() activeFabricIdList = set() - activeAdmins = set() + vendorId = None def AllocateNextFabricIndex(self): ''' Allocate the next un-used fabric index. @@ -87,10 +88,11 @@ def AllocateNextFabricId(self): nextFabricId = nextFabricId + 1 return nextFabricId - def __init__(self, rcac: bytes = None, icac: bytes = None, fabricIndex: int = None, fabricId: int = None): + def __init__(self, vendorId: int, rcac: bytes = None, icac: bytes = None, fabricIndex: int = None, fabricId: int = None): ''' Creates a valid FabricAdmin object with valid RCAC/ICAC, and registers itself as an OperationalCredentialsDelegate for other parts of the system (notably, DeviceController) to vend NOCs. + vendorId: Valid operational Vendor ID associated with this fabric. rcac, icac: Specify the RCAC and ICAC to be used with this fabric (not-supported). If not specified, an RCAC and ICAC will be automatically generated. @@ -101,6 +103,12 @@ def __init__(self, rcac: bytes = None, icac: bytes = None, fabricIndex: int = No raise ValueError( "Providing valid rcac/icac values is not supported right now!") + if (vendorId is None or vendorId == 0): + raise ValueError( + f"Invalid VendorID ({vendorId}) provided!") + + self.vendorId = vendorId + if (fabricId is None): self._fabricId = self.AllocateNextFabricId() else: @@ -124,7 +132,7 @@ def __init__(self, rcac: bytes = None, icac: bytes = None, fabricIndex: int = No FabricAdmin.activeFabricIndexList.add(self._fabricIndex) print( - f"New FabricAdmin: FabricId: {self._fabricId}({self._fabricIndex})") + f"New FabricAdmin: FabricId: {self._fabricId}({self._fabricIndex}), VendorId = {hex(self.vendorId)}") self._handle.pychip_OpCreds_InitializeDelegate.restype = c_void_p self.closure = builtins.chipStack.Call( @@ -144,7 +152,7 @@ def __init__(self, rcac: bytes = None, icac: bytes = None, fabricIndex: int = No adminList = {str(self._fabricIndex): {'fabricId': self._fabricId}} builtins.chipStack.GetStorageManager().SetReplKey('fabricAdmins', adminList) - adminList[str(self._fabricIndex)] = {'fabricId': self._fabricId} + adminList[str(self._fabricIndex)] = {'fabricId': self._fabricId, 'vendorId': self.vendorId} builtins.chipStack.GetStorageManager().SetReplKey('fabricAdmins', adminList) self._isActive = True @@ -166,7 +174,7 @@ def NewController(self, nodeId: int = None, paaTrustStorePath: str = "", useTest print( f"Allocating new controller with FabricId: {self._fabricId}({self._fabricIndex}), NodeId: {nodeId}") controller = ChipDeviceCtrl.ChipDeviceController( - self.closure, self._fabricId, self._fabricIndex, nodeId, paaTrustStorePath, useTestCommissioner) + self.closure, self._fabricId, self._fabricIndex, nodeId, self.vendorId, paaTrustStorePath, useTestCommissioner) return controller def ShutdownAll(): diff --git a/src/controller/python/test/test_scripts/base.py b/src/controller/python/test/test_scripts/base.py index 84bc98328122b9..1bceb3d7e748e8 100644 --- a/src/controller/python/test/test_scripts/base.py +++ b/src/controller/python/test/test_scripts/base.py @@ -172,8 +172,8 @@ def assertValueEqual(self, expected): class BaseTestHelper: def __init__(self, nodeid: int, paaTrustStorePath: str, testCommissioner: bool = False): self.chipStack = ChipStack('/tmp/repl_storage.json') - self.fabricAdmin = chip.FabricAdmin.FabricAdmin( - fabricId=1, fabricIndex=1) + self.fabricAdmin = chip.FabricAdmin.FabricAdmin(vendorId=0XFFF1, + fabricId=1, fabricIndex=1) self.devCtrl = self.fabricAdmin.NewController( nodeid, paaTrustStorePath, testCommissioner) self.controllerNodeId = nodeid @@ -369,7 +369,7 @@ async def TestAddUpdateRemoveFabric(self, nodeid: int): self.logger.info("Waiting for attribute read for CommissionedFabrics") startOfTestFabricCount = await self._GetCommissonedFabricCount(nodeid) - tempFabric = chip.FabricAdmin.FabricAdmin(fabricId=3, fabricIndex=3) + tempFabric = chip.FabricAdmin.FabricAdmin(vendorId=0xFFF1, fabricId=3, fabricIndex=3) tempDevCtrl = tempFabric.NewController(self.controllerNodeId, self.paaTrustStorePath) self.logger.info("Setting failsafe on CASE connection") @@ -590,8 +590,8 @@ async def TestMultiFabric(self, ip: str, setuppin: int, nodeid: int): await self.devCtrl.SendCommand(nodeid, 0, Clusters.AdministratorCommissioning.Commands.OpenBasicCommissioningWindow(180), timedRequestTimeoutMs=10000) self.logger.info("Creating 2nd Fabric Admin") - self.fabricAdmin2 = chip.FabricAdmin.FabricAdmin( - fabricId=2, fabricIndex=2) + self.fabricAdmin2 = chip.FabricAdmin.FabricAdmin(vendorId=0xFFF1, + fabricId=2, fabricIndex=2) self.logger.info("Creating Device Controller on 2nd Fabric") self.devCtrl2 = self.fabricAdmin2.NewController( @@ -613,9 +613,10 @@ async def TestMultiFabric(self, ip: str, setuppin: int, nodeid: int): self.logger.info("Shutdown completed, starting new controllers...") - self.fabricAdmin = chip.FabricAdmin.FabricAdmin( - fabricId=1, fabricIndex=1) - fabricAdmin2 = chip.FabricAdmin.FabricAdmin(fabricId=2, fabricIndex=2) + self.fabricAdmin = chip.FabricAdmin.FabricAdmin(vendorId=0XFFF1, + fabricId=1, fabricIndex=1) + fabricAdmin2 = chip.FabricAdmin.FabricAdmin(vendorId=0xFFF1, + fabricId=2, fabricIndex=2) self.devCtrl = self.fabricAdmin.NewController( self.controllerNodeId, self.paaTrustStorePath)