From 57743594b5d6fd6fb4dd8bb8d8275f3cdd61a3dd Mon Sep 17 00:00:00 2001 From: C Freeman Date: Tue, 21 Nov 2023 16:15:47 +0100 Subject: [PATCH] AutoCommissioner: Set default NTP to internal buffer (#29954) * AutoCommissioner: Set default NTP to internal buffer Also, it would be good to actually set the time zone to our own buffer. * Apply suggestions from code review Co-authored-by: Boris Zbarsky * Size check and fail big strings, add tests, fix bug - neither DefaultNTP nor the time zone name is useful to the device if they're truncated, so don't attempt to send anything unless the value is spec compliant - NOTE: Right now, we still attempt to commission with the remaining parameters, but I'm open to returning an error here too - add tests for too-long names AND max size names - fix a bug in the server default NTP handling that prevented domain names from ever being set - turn on dns resolution in the all-clusters. Note that the all clusters app will never actually DO anything with these names, but it's still nice to have it available for testing. * Restyled by clang-format * Apply suggestions from code review Co-authored-by: Boris Zbarsky * Address review comments * Restyled by clang-format --------- Co-authored-by: Boris Zbarsky Co-authored-by: Restyled.io --- .../all-clusters-app.matter | 2 +- .../all-clusters-common/all-clusters-app.zap | 3 +- .../DefaultTimeSyncDelegate.cpp | 6 ++- .../time-synchronization-server.cpp | 23 ++++---- src/controller/AutoCommissioner.cpp | 30 +++++++++-- src/controller/AutoCommissioner.h | 3 ++ src/controller/CommissioningDelegate.h | 1 + .../ChipDeviceController-ScriptBinding.cpp | 17 +++++- src/controller/python/chip/ChipDeviceCtrl.py | 8 +-- src/python_testing/TC_TIMESYNC_2_6.py | 2 +- .../TestCommissioningTimeSync.py | 53 ++++++++++++++++++- 11 files changed, 119 insertions(+), 29 deletions(-) diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter index 3a6187874db2b0..1161b93c5119ee 100644 --- a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter +++ b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter @@ -5525,7 +5525,7 @@ endpoint 0 { ram attribute timeZoneDatabase default = 0; callback attribute timeZoneListMaxSize default = 3; callback attribute DSTOffsetListMaxSize; - ram attribute supportsDNSResolve default = false; + ram attribute supportsDNSResolve default = true; callback attribute generatedCommandList; callback attribute acceptedCommandList; callback attribute attributeList; diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap b/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap index 5f391a52fa267a..fe400ec5019b28 100644 --- a/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap +++ b/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap @@ -4926,7 +4926,7 @@ "storageOption": "RAM", "singleton": 0, "bounded": 0, - "defaultValue": "false", + "defaultValue": "true", "reportable": 1, "minInterval": 1, "maxInterval": 65534, @@ -21585,6 +21585,7 @@ "endpointId": 65534, "networkId": 0 } + ] } diff --git a/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp b/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp index 6b00f661eea412..82164afc2427b2 100644 --- a/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp +++ b/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp @@ -44,8 +44,10 @@ bool DefaultTimeSyncDelegate::IsNTPAddressValid(chip::CharSpan ntp) bool DefaultTimeSyncDelegate::IsNTPAddressDomain(chip::CharSpan ntp) { - // placeholder implementation - return false; + // For now, assume anything that includes a . is a domain name. + // Delegates are free to evaluate this properly if they actually HAVE domain + // name resolution, rather than just implementing a dummy for testing. + return !IsNTPAddressValid(ntp) && (memchr(ntp.data(), '.', ntp.size()) != nullptr); } CHIP_ERROR DefaultTimeSyncDelegate::UpdateTimeFromPlatformSource(chip::Callback::Callback * callback) diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp index 7750c56be352d0..707abfb875045b 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp @@ -1284,24 +1284,19 @@ bool emberAfTimeSynchronizationClusterSetDefaultNTPCallback( commandObj->AddStatus(commandPath, Status::ConstraintError); return true; } - if (!GetDelegate()->IsNTPAddressValid(dNtpChar.Value())) + bool dnsResolve; + if (EMBER_ZCL_STATUS_SUCCESS != SupportsDNSResolve::Get(commandPath.mEndpointId, &dnsResolve)) { - commandObj->AddStatus(commandPath, Status::InvalidCommand); + commandObj->AddStatus(commandPath, Status::Failure); return true; } - if (GetDelegate()->IsNTPAddressDomain(dNtpChar.Value())) + bool isDomain = GetDelegate()->IsNTPAddressDomain(dNtpChar.Value()); + bool isIPv6 = GetDelegate()->IsNTPAddressValid(dNtpChar.Value()); + bool useable = isIPv6 || (isDomain && dnsResolve); + if (!useable) { - bool dnsResolve; - if (EMBER_ZCL_STATUS_SUCCESS != SupportsDNSResolve::Get(commandPath.mEndpointId, &dnsResolve)) - { - commandObj->AddStatus(commandPath, Status::Failure); - return true; - } - if (!dnsResolve) - { - commandObj->AddStatus(commandPath, Status::InvalidCommand); - return true; - } + commandObj->AddStatus(commandPath, Status::InvalidCommand); + return true; } } diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index b97b4d544b7b4e..397497ab80f206 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -27,6 +27,8 @@ namespace chip { namespace Controller { using namespace chip::app::Clusters; +using chip::app::DataModel::MakeNullable; +using chip::app::DataModel::NullNullable; AutoCommissioner::AutoCommissioner() { @@ -87,7 +89,9 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam IsUnsafeSpan(params.GetAttestationSignature(), mParams.GetAttestationSignature()) || IsUnsafeSpan(params.GetPAI(), mParams.GetPAI()) || IsUnsafeSpan(params.GetDAC(), mParams.GetDAC()) || IsUnsafeSpan(params.GetTimeZone(), mParams.GetTimeZone()) || - IsUnsafeSpan(params.GetDSTOffsets(), mParams.GetDSTOffsets())); + IsUnsafeSpan(params.GetDSTOffsets(), mParams.GetDSTOffsets()) || + (params.GetDefaultNTP().HasValue() && !params.GetDefaultNTP().Value().IsNull() && + params.GetDefaultNTP().Value().Value().data() != mDefaultNtp)); mParams = params; @@ -194,16 +198,36 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam for (size_t i = 0; i < size; ++i) { mTimeZoneBuf[i] = params.GetTimeZone().Value()[i]; - if (mTimeZoneBuf[i].name.HasValue()) + if (params.GetTimeZone().Value()[i].name.HasValue() && + params.GetTimeZone().Value()[i].name.Value().size() <= kMaxTimeZoneNameLen) { auto span = MutableCharSpan(mTimeZoneNames[i], kMaxTimeZoneNameLen); - CopyCharSpanToMutableCharSpan(mTimeZoneBuf[i].name.Value(), span); + // The buffer backing "span" is statically allocated and is of size kMaxSupportedTimeZones, so this should never + // fail. + CopyCharSpanToMutableCharSpan(params.GetTimeZone().Value()[i].name.Value(), span); mTimeZoneBuf[i].name.SetValue(span); } + else + { + mTimeZoneBuf[i].name.ClearValue(); + } } auto list = app::DataModel::List(mTimeZoneBuf, size); mParams.SetTimeZone(list); } + if (params.GetDefaultNTP().HasValue()) + { + ChipLogProgress(Controller, "Setting Default NTP from parameters"); + // This parameter is an optional nullable, so we need to go two levels deep here. + if (!params.GetDefaultNTP().Value().IsNull() && params.GetDefaultNTP().Value().Value().size() <= kMaxDefaultNtpSize) + { + // The buffer backing "span" is statically allocated and is of size kMaxDefaultNtpSize. + auto span = MutableCharSpan(mDefaultNtp, kMaxDefaultNtpSize); + CopyCharSpanToMutableCharSpan(params.GetDefaultNTP().Value().Value(), span); + auto default_ntp = MakeNullable(CharSpan(mDefaultNtp, params.GetDefaultNTP().Value().Value().size())); + mParams.SetDefaultNTP(default_ntp); + } + } return CHIP_NO_ERROR; } diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index 2e13445a113f5d..f86ebead1d011a 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -113,6 +113,9 @@ class AutoCommissioner : public CommissioningDelegate static constexpr size_t kMaxSupportedDstStructs = 10; app::Clusters::TimeSynchronization::Structs::DSTOffsetStruct::Type mDstOffsetsBuf[kMaxSupportedDstStructs]; + static constexpr size_t kMaxDefaultNtpSize = 128; + char mDefaultNtp[kMaxDefaultNtpSize]; + bool mNeedsNetworkSetup = false; ReadCommissioningInfo mDeviceCommissioningInfo; bool mNeedsDST = false; diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index bb95f5eb8fe8f2..cbfff76adc6b3b 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -527,6 +527,7 @@ class CommissioningParameters mDAC.ClearValue(); mTimeZone.ClearValue(); mDSTOffsets.ClearValue(); + mDefaultNTP.ClearValue(); } private: diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index a7732a48361db0..2d6857d17a4fe6 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -100,6 +100,7 @@ chip::Platform::ScopedMemoryBuffer sThreadBuf; chip::Platform::ScopedMemoryBuffer sDefaultNTPBuf; app::Clusters::TimeSynchronization::Structs::DSTOffsetStruct::Type sDSTBuf; app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type sTimeZoneBuf; +chip::Platform::ScopedMemoryBuffer sTimeZoneNameBuf; chip::Controller::CommissioningParameters sCommissioningParameters; } // namespace @@ -140,7 +141,7 @@ PyChipError pychip_DeviceController_UnpairDevice(chip::Controller::DeviceCommiss DeviceUnpairingCompleteFunct callback); PyChipError pychip_DeviceController_SetThreadOperationalDataset(const char * threadOperationalDataset, uint32_t size); PyChipError pychip_DeviceController_SetWiFiCredentials(const char * ssid, const char * credentials); -PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt); +PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt, const char * name); PyChipError pychip_DeviceController_SetDSTOffset(int32_t offset, uint64_t validStarting, uint64_t validUntil); PyChipError pychip_DeviceController_SetDefaultNtp(const char * defaultNTP); PyChipError pychip_DeviceController_SetTrustedTimeSource(chip::NodeId nodeId, chip::EndpointId endpoint); @@ -509,10 +510,22 @@ PyChipError pychip_DeviceController_SetWiFiCredentials(const char * ssid, const return ToPyChipError(CHIP_NO_ERROR); } -PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt) +PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt, const char * name) { sTimeZoneBuf.offset = offset; sTimeZoneBuf.validAt = validAt; + if (strcmp(name, "") == 0) + { + sTimeZoneNameBuf.Free(); + sTimeZoneBuf.name = NullOptional; + } + else + { + size_t len = strlen(name); + ReturnErrorCodeIf(!sTimeZoneNameBuf.Alloc(len), ToPyChipError(CHIP_ERROR_NO_MEMORY)); + memcpy(sTimeZoneNameBuf.Get(), name, len); + sTimeZoneBuf.name.SetValue(CharSpan(sTimeZoneNameBuf.Get(), len)); + } app::DataModel::List list(&sTimeZoneBuf, 1); sCommissioningParameters.SetTimeZone(list); return ToPyChipError(CHIP_NO_ERROR); diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py index d6e8faa1742317..4db95205179a46 100644 --- a/src/controller/python/chip/ChipDeviceCtrl.py +++ b/src/controller/python/chip/ChipDeviceCtrl.py @@ -1421,10 +1421,10 @@ def _InitLib(self): c_char_p, c_char_p] self._dmLib.pychip_DeviceController_SetWiFiCredentials.restype = PyChipError - # Currently only supports 1 list item, no name + # Currently only supports 1 list item self._dmLib.pychip_DeviceController_SetTimeZone.restype = PyChipError self._dmLib.pychip_DeviceController_SetTimeZone.argtypes = [ - c_int32, c_uint64] + c_int32, c_uint64, c_char_p] # Currently only supports 1 list item self._dmLib.pychip_DeviceController_SetDSTOffset.restype = PyChipError @@ -1737,11 +1737,11 @@ def ResetCommissioningParameters(self): lambda: self._dmLib.pychip_DeviceController_ResetCommissioningParameters() ).raise_on_error() - def SetTimeZone(self, offset: int, validAt: int): + def SetTimeZone(self, offset: int, validAt: int, name: str = ""): ''' Set the time zone to set during commissioning. Currently only one time zone entry is supported''' self.CheckIsActive() self._ChipStack.Call( - lambda: self._dmLib.pychip_DeviceController_SetTimeZone(offset, validAt) + lambda: self._dmLib.pychip_DeviceController_SetTimeZone(offset, validAt, name.encode("utf-8")) ).raise_on_error() def SetDSTOffset(self, offset: int, validStarting: int, validUntil: int): diff --git a/src/python_testing/TC_TIMESYNC_2_6.py b/src/python_testing/TC_TIMESYNC_2_6.py index 23ed1a5575b6dd..ebd02ff3622f83 100644 --- a/src/python_testing/TC_TIMESYNC_2_6.py +++ b/src/python_testing/TC_TIMESYNC_2_6.py @@ -35,7 +35,7 @@ async def send_set_default_ntp_cmd(self, ntp: typing.Union[Nullable, str]) -> No async def send_set_default_ntp_cmd_expect_error(self, ntp: typing.Union[Nullable, str], error: Status) -> None: try: await self.send_single_cmd(cmd=Clusters.Objects.TimeSynchronization.Commands.SetDefaultNTP(defaultNTP=ntp)) - asserts.assert_true(False, "Unexpected SetTimeZone command success") + asserts.assert_true(False, "Unexpected SetDefaultNTP command success") except InteractionModelError as e: asserts.assert_equal(e.status, error, "Unexpected error returned") pass diff --git a/src/python_testing/TestCommissioningTimeSync.py b/src/python_testing/TestCommissioningTimeSync.py index dfd7c55f8d21f4..509aabfc6aa500 100644 --- a/src/python_testing/TestCommissioningTimeSync.py +++ b/src/python_testing/TestCommissioningTimeSync.py @@ -89,7 +89,7 @@ async def commission_and_base_checks(self): async def create_commissioner(self): if self.commissioner: - self.destroy_current_commissioner() + await self.destroy_current_commissioner() new_certificate_authority = self.certificate_authority_manager.NewCertificateAuthority() new_fabric_admin = new_certificate_authority.NewFabricAdmin(vendorId=0xFFF1, fabricId=2) self.commissioner = new_fabric_admin.NewController(nodeId=112233, useTestCommissioner=True) @@ -230,6 +230,57 @@ async def test_FabricCheckStage(self): kCheckForMatchingFabric), "Incorrectly ran check for matching fabric stage") asserts.assert_equal(self.commissioner.GetFabricCheckResult(), -1, "Fabric check result incorrectly set") + @async_test_body + async def test_TimeZoneName(self): + await self.create_commissioner() + self.commissioner.SetTimeZone(offset=3600, validAt=0, name="test") + await self.commission_and_base_checks() + asserts.assert_true(self.commissioner.CheckStageSuccessful(kConfigureTimeZone), 'Time zone was not successfully set') + + received = await self.read_single_attribute_check_success(cluster=Clusters.TimeSynchronization, attribute=Clusters.TimeSynchronization.Attributes.TimeZone) + expected = [Clusters.TimeSynchronization.Structs.TimeZoneStruct(offset=3600, validAt=0, name="test")] + asserts.assert_equal(received, expected, "Time zone was not correctly set by commissioner") + + await self.create_commissioner() + # name is max 64 per the spec + sixty_five_byte_string = "x" * 65 + self.commissioner.SetTimeZone(offset=3600, validAt=0, name=sixty_five_byte_string) + await self.commission_and_base_checks() + asserts.assert_true(self.commissioner.CheckStageSuccessful(kConfigureTimeZone), 'Time zone was not successfully set') + + received = await self.read_single_attribute_check_success(cluster=Clusters.TimeSynchronization, attribute=Clusters.TimeSynchronization.Attributes.TimeZone) + expected = [Clusters.TimeSynchronization.Structs.TimeZoneStruct(offset=3600, validAt=0, name=None)] + asserts.assert_equal(received, expected, "Commissioner did not ignore too-long name") + + await self.create_commissioner() + # name is max 64 per the spec + sixty_four_byte_string = "x" * 64 + self.commissioner.SetTimeZone(offset=3600, validAt=0, name=sixty_four_byte_string) + await self.commission_and_base_checks() + asserts.assert_true(self.commissioner.CheckStageSuccessful(kConfigureTimeZone), 'Time zone was not successfully set') + + received = await self.read_single_attribute_check_success(cluster=Clusters.TimeSynchronization, attribute=Clusters.TimeSynchronization.Attributes.TimeZone) + expected = [Clusters.TimeSynchronization.Structs.TimeZoneStruct(offset=3600, validAt=0, name=sixty_four_byte_string)] + asserts.assert_equal(received, expected, "Time zone 64 byte name was not correctly set") + + @async_test_body + async def test_DefaultNtpSize(self): + await self.create_commissioner() + too_long_name = "x." + "x" * 127 + self.commissioner.SetDefaultNTP(too_long_name) + await self.commission_and_base_checks() + asserts.assert_false(self.commissioner.CheckStageSuccessful(kConfigureDefaultNTP), + 'Commissioner attempted to set default NTP to a too long value') + + await self.create_commissioner() + just_fits_name = "x." + "x" * 126 + self.commissioner.SetDefaultNTP(just_fits_name) + await self.commission_and_base_checks() + asserts.assert_true(self.commissioner.CheckStageSuccessful(kConfigureDefaultNTP), + 'Commissioner did not correctly set default NTP') + received = await self.read_single_attribute_check_success(cluster=Clusters.TimeSynchronization, attribute=Clusters.TimeSynchronization.Attributes.DefaultNTP) + asserts.assert_equal(received, just_fits_name, 'Commissioner incorrectly set default NTP name') + # TODO(cecille): Test - Add hooks to change the time zone response to indicate no DST is needed # TODO(cecille): Test - Set commissioningParameters TimeZone and DST list size to > node list size to ensure they get truncated