From aad9bd5025bf036779739c88db34310ed6964bf9 Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Tue, 10 Sep 2024 13:47:09 +0200 Subject: [PATCH] Fix max timeout for open commissioning window Commissioning window can be opened using timeout exceeding the maximum value of 900 s defined by the spec. This can happen if selected transport is IP, but the device uses BLE extended announcement feature. Added isBle flag to be able to determine what max timeout should be used for the particular scenario. Fixes: #35505 --- .../esp32/main/DeviceWithDisplay.cpp | 2 +- .../esp32/main/DeviceWithDisplay.cpp | 2 +- .../esp32/main/DeviceWithDisplay.cpp | 2 +- examples/platform/asr/shell/matter_shell.cpp | 2 +- .../administrator-commissioning-server.cpp | 21 +++++++++---------- src/app/server/CommissioningWindowManager.cpp | 12 ++++++----- src/app/server/CommissioningWindowManager.h | 14 +++++++------ .../tests/TestCommissioningWindowManager.cpp | 8 +++---- 8 files changed, 33 insertions(+), 30 deletions(-) diff --git a/examples/all-clusters-app/esp32/main/DeviceWithDisplay.cpp b/examples/all-clusters-app/esp32/main/DeviceWithDisplay.cpp index 6f35995a445110..316bcad21cd192 100644 --- a/examples/all-clusters-app/esp32/main/DeviceWithDisplay.cpp +++ b/examples/all-clusters-app/esp32/main/DeviceWithDisplay.cpp @@ -604,7 +604,7 @@ class SetupListModel : public TouchesMatterStackModel { chip::Server::GetInstance().GetFabricTable().DeleteAllFabrics(); auto & commissionMgr = chip::Server::GetInstance().GetCommissioningWindowManager(); - commissionMgr.OpenBasicCommissioningWindow(commissionMgr.MaxCommissioningTimeout(), + commissionMgr.OpenBasicCommissioningWindow(commissionMgr.MaxCommissioningTimeout(false), CommissioningWindowAdvertisement::kDnssdOnly); } } diff --git a/examples/all-clusters-minimal-app/esp32/main/DeviceWithDisplay.cpp b/examples/all-clusters-minimal-app/esp32/main/DeviceWithDisplay.cpp index f56a3e69a023e7..9c9088111aafa7 100644 --- a/examples/all-clusters-minimal-app/esp32/main/DeviceWithDisplay.cpp +++ b/examples/all-clusters-minimal-app/esp32/main/DeviceWithDisplay.cpp @@ -450,7 +450,7 @@ class SetupListModel : public TouchesMatterStackModel { chip::Server::GetInstance().GetFabricTable().DeleteAllFabrics(); auto & commissionMgr = chip::Server::GetInstance().GetCommissioningWindowManager(); - commissionMgr.OpenBasicCommissioningWindow(commissionMgr.MaxCommissioningTimeout(), + commissionMgr.OpenBasicCommissioningWindow(commissionMgr.MaxCommissioningTimeout(false), CommissioningWindowAdvertisement::kDnssdOnly); } } diff --git a/examples/lighting-app/esp32/main/DeviceWithDisplay.cpp b/examples/lighting-app/esp32/main/DeviceWithDisplay.cpp index 2027bbf0883ef4..4c081255932697 100644 --- a/examples/lighting-app/esp32/main/DeviceWithDisplay.cpp +++ b/examples/lighting-app/esp32/main/DeviceWithDisplay.cpp @@ -141,7 +141,7 @@ class SetupListModel : public TouchesMatterStackModel { chip::Server::GetInstance().GetFabricTable().DeleteAllFabrics(); auto & commissionMgr = chip::Server::GetInstance().GetCommissioningWindowManager(); - commissionMgr.OpenBasicCommissioningWindow(commissionMgr.MaxCommissioningTimeout(), + commissionMgr.OpenBasicCommissioningWindow(commissionMgr.MaxCommissioningTimeout(false), CommissioningWindowAdvertisement::kDnssdOnly); } } diff --git a/examples/platform/asr/shell/matter_shell.cpp b/examples/platform/asr/shell/matter_shell.cpp index bfa138f92ff0e5..6800b309d03bd5 100644 --- a/examples/platform/asr/shell/matter_shell.cpp +++ b/examples/platform/asr/shell/matter_shell.cpp @@ -67,7 +67,7 @@ void asr_matter_reset(Reset_t type) { chip::Server::GetInstance().GetFabricTable().DeleteAllFabrics(); auto & commissionMgr = chip::Server::GetInstance().GetCommissioningWindowManager(); - commissionMgr.OpenBasicCommissioningWindow(commissionMgr.MaxCommissioningTimeout(), + commissionMgr.OpenBasicCommissioningWindow(commissionMgr.MaxCommissioningTimeout(false), CommissioningWindowAdvertisement::kDnssdOnly); } } diff --git a/src/app/clusters/administrator-commissioning-server/administrator-commissioning-server.cpp b/src/app/clusters/administrator-commissioning-server/administrator-commissioning-server.cpp index 57a230de547743..ab5b1cc17e8019 100644 --- a/src/app/clusters/administrator-commissioning-server/administrator-commissioning-server.cpp +++ b/src/app/clusters/administrator-commissioning-server/administrator-commissioning-server.cpp @@ -90,6 +90,7 @@ bool emberAfAdministratorCommissioningClusterOpenCommissioningWindowCallback( auto & discriminator = commandData.discriminator; auto & iterations = commandData.iterations; auto & salt = commandData.salt; + CHIP_ERROR err = CHIP_NO_ERROR; Optional status = Optional::Missing(); Status globalStatus = Status::Success; @@ -110,14 +111,12 @@ bool emberAfAdministratorCommissioningClusterOpenCommissioningWindowCallback( VerifyOrExit(iterations <= kSpake2p_Max_PBKDF_Iterations, status.Emplace(StatusCode::kPAKEParameterError)); VerifyOrExit(salt.size() >= kSpake2p_Min_PBKDF_Salt_Length, status.Emplace(StatusCode::kPAKEParameterError)); VerifyOrExit(salt.size() <= kSpake2p_Max_PBKDF_Salt_Length, status.Emplace(StatusCode::kPAKEParameterError)); - VerifyOrExit(commissioningTimeout <= commissionMgr.MaxCommissioningTimeout(), globalStatus = Status::InvalidCommand); - VerifyOrExit(commissioningTimeout >= commissionMgr.MinCommissioningTimeout(), globalStatus = Status::InvalidCommand); VerifyOrExit(discriminator <= kMaxDiscriminatorValue, globalStatus = Status::InvalidCommand); - VerifyOrExit(verifier.Deserialize(pakeVerifier) == CHIP_NO_ERROR, status.Emplace(StatusCode::kPAKEParameterError)); - VerifyOrExit(commissionMgr.OpenEnhancedCommissioningWindow(commissioningTimeout, discriminator, verifier, iterations, salt, - fabricIndex, fabricInfo->GetVendorId()) == CHIP_NO_ERROR, - status.Emplace(StatusCode::kPAKEParameterError)); + err = commissionMgr.OpenEnhancedCommissioningWindow(commissioningTimeout, discriminator, verifier, iterations, salt, + fabricIndex, fabricInfo->GetVendorId()); + VerifyOrExit(err != CHIP_ERROR_INVALID_ARGUMENT, globalStatus = Status::InvalidCommand); + VerifyOrExit(err == CHIP_NO_ERROR, status.Emplace(StatusCode::kPAKEParameterError)); ChipLogProgress(Zcl, "Commissioning window is now open"); exit: @@ -153,16 +152,16 @@ bool emberAfAdministratorCommissioningClusterOpenBasicCommissioningWindowCallbac const FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex); auto & failSafeContext = Server::GetInstance().GetFailSafeContext(); auto & commissionMgr = Server::GetInstance().GetCommissioningWindowManager(); + CHIP_ERROR err = CHIP_NO_ERROR; VerifyOrExit(fabricInfo != nullptr, status.Emplace(StatusCode::kPAKEParameterError)); VerifyOrExit(!commissionMgr.IsCommissioningWindowOpen(), status.Emplace(StatusCode::kBusy)); VerifyOrExit(failSafeContext.IsFailSafeFullyDisarmed(), status.Emplace(StatusCode::kBusy)); - VerifyOrExit(commissioningTimeout <= commissionMgr.MaxCommissioningTimeout(), globalStatus = Status::InvalidCommand); - VerifyOrExit(commissioningTimeout >= commissionMgr.MinCommissioningTimeout(), globalStatus = Status::InvalidCommand); - VerifyOrExit(commissionMgr.OpenBasicCommissioningWindowForAdministratorCommissioningCluster( - commissioningTimeout, fabricIndex, fabricInfo->GetVendorId()) == CHIP_NO_ERROR, - status.Emplace(StatusCode::kPAKEParameterError)); + err = commissionMgr.OpenBasicCommissioningWindowForAdministratorCommissioningCluster(commissioningTimeout, fabricIndex, + fabricInfo->GetVendorId()); + VerifyOrExit(err != CHIP_ERROR_INVALID_ARGUMENT, globalStatus = Status::InvalidCommand); + VerifyOrExit(err == CHIP_NO_ERROR, status.Emplace(StatusCode::kPAKEParameterError)); ChipLogProgress(Zcl, "Commissioning window is now open"); exit: diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp index 01bff4bf07db46..94359e947f0ed7 100644 --- a/src/app/server/CommissioningWindowManager.cpp +++ b/src/app/server/CommissioningWindowManager.cpp @@ -242,9 +242,9 @@ void CommissioningWindowManager::OnSessionEstablished(const SessionHandle & sess } } -CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow(Seconds32 commissioningTimeout) +CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow(Seconds32 commissioningTimeout, bool isBle) { - VerifyOrReturnError(commissioningTimeout <= MaxCommissioningTimeout() && commissioningTimeout >= MinCommissioningTimeout(), + VerifyOrReturnError(commissioningTimeout <= MaxCommissioningTimeout(isBle) && commissioningTimeout >= MinCommissioningTimeout(), CHIP_ERROR_INVALID_ARGUMENT); auto & failSafeContext = Server::GetInstance().GetFailSafeContext(); VerifyOrReturnError(failSafeContext.IsFailSafeFullyDisarmed(), CHIP_ERROR_INCORRECT_STATE); @@ -306,11 +306,13 @@ CHIP_ERROR CommissioningWindowManager::OpenBasicCommissioningWindow(Seconds32 co CommissioningWindowAdvertisement advertisementMode) { RestoreDiscriminator(); + bool isBle = false; #if CONFIG_NETWORK_LAYER_BLE // Enable BLE advertisements if commissioning window is to be opened on all supported // transports, and BLE is supported on the current device. - SetBLE(advertisementMode == chip::CommissioningWindowAdvertisement::kAllSupported); + isBle = advertisementMode == chip::CommissioningWindowAdvertisement::kAllSupported; + SetBLE(isBle); #else SetBLE(false); #endif // CONFIG_NETWORK_LAYER_BLE @@ -319,7 +321,7 @@ CHIP_ERROR CommissioningWindowManager::OpenBasicCommissioningWindow(Seconds32 co mUseECM = false; - CHIP_ERROR err = OpenCommissioningWindow(commissioningTimeout); + CHIP_ERROR err = OpenCommissioningWindow(commissioningTimeout, isBle); if (err != CHIP_NO_ERROR) { Cleanup(); @@ -362,7 +364,7 @@ CHIP_ERROR CommissioningWindowManager::OpenEnhancedCommissioningWindow(Seconds32 mUseECM = true; - CHIP_ERROR err = OpenCommissioningWindow(commissioningTimeout); + CHIP_ERROR err = OpenCommissioningWindow(commissioningTimeout, false); if (err != CHIP_NO_ERROR) { Cleanup(); diff --git a/src/app/server/CommissioningWindowManager.h b/src/app/server/CommissioningWindowManager.h index 51efb44b19376e..e9b63d3818f40d 100644 --- a/src/app/server/CommissioningWindowManager.h +++ b/src/app/server/CommissioningWindowManager.h @@ -58,15 +58,17 @@ class CommissioningWindowManager : public Messaging::UnsolicitedMessageHandler, return CHIP_NO_ERROR; } - static constexpr System::Clock::Seconds32 MaxCommissioningTimeout() + static constexpr System::Clock::Seconds32 MaxCommissioningTimeout(bool isBle) { #if CHIP_DEVICE_CONFIG_BLE_EXT_ADVERTISING - // Specification section 2.3.1 - Extended Announcement Duration up to 48h - return System::Clock::Seconds32(60 * 60 * 48); -#else + if (isBle) + { + // Specification section 2.3.1 - Extended Announcement Duration up to 48h + return System::Clock::Seconds32(60 * 60 * 48); + } +#endif // Specification section 5.4.2.3. Announcement Duration says 15 minutes. return System::Clock::Seconds32(15 * 60); -#endif } System::Clock::Seconds32 MinCommissioningTimeout() const @@ -146,7 +148,7 @@ class CommissioningWindowManager : public Messaging::UnsolicitedMessageHandler, // Start a timer that will call HandleCommissioningWindowTimeout, and then // start advertising and listen for PASE. - CHIP_ERROR OpenCommissioningWindow(System::Clock::Seconds32 commissioningTimeout); + CHIP_ERROR OpenCommissioningWindow(System::Clock::Seconds32 commissioningTimeout, bool isBle); // Start advertising and listening for PASE connections. Should only be // called when a commissioning window timeout timer is running. diff --git a/src/app/tests/TestCommissioningWindowManager.cpp b/src/app/tests/TestCommissioningWindowManager.cpp index 5df7a4b6ce861d..369af295534d6c 100644 --- a/src/app/tests/TestCommissioningWindowManager.cpp +++ b/src/app/tests/TestCommissioningWindowManager.cpp @@ -156,7 +156,7 @@ void CheckCommissioningWindowManagerBasicWindowOpenCloseTask(intptr_t context) CommissioningWindowManager & commissionMgr = Server::GetInstance().GetCommissioningWindowManager(); - EXPECT_EQ(commissionMgr.OpenBasicCommissioningWindow(commissionMgr.MaxCommissioningTimeout(), + EXPECT_EQ(commissionMgr.OpenBasicCommissioningWindow(commissionMgr.MaxCommissioningTimeout(false), CommissioningWindowAdvertisement::kDnssdOnly), CHIP_NO_ERROR); EXPECT_TRUE(commissionMgr.IsCommissioningWindowOpen()); @@ -194,7 +194,7 @@ void CheckCommissioningWindowManagerBasicWindowOpenCloseFromClusterTask(intptr_t constexpr auto fabricIndex = static_cast(1); constexpr auto vendorId = static_cast(0xFFF3); EXPECT_EQ(commissionMgr.OpenBasicCommissioningWindowForAdministratorCommissioningCluster( - commissionMgr.MaxCommissioningTimeout(), fabricIndex, vendorId), + commissionMgr.MaxCommissioningTimeout(false), fabricIndex, vendorId), CHIP_NO_ERROR); EXPECT_TRUE(commissionMgr.IsCommissioningWindowOpen()); EXPECT_EQ(commissionMgr.CommissioningWindowStatusForCluster(), @@ -350,8 +350,8 @@ void CheckCommissioningWindowManagerEnhancedWindowTask(intptr_t context) constexpr auto fabricIndex = static_cast(1); constexpr auto vendorId = static_cast(0xFFF3); - EXPECT_EQ(commissionMgr.OpenEnhancedCommissioningWindow(commissionMgr.MaxCommissioningTimeout(), newDiscriminator, verifier, - kIterations, saltData, fabricIndex, vendorId), + EXPECT_EQ(commissionMgr.OpenEnhancedCommissioningWindow(commissionMgr.MaxCommissioningTimeout(false), newDiscriminator, + verifier, kIterations, saltData, fabricIndex, vendorId), CHIP_NO_ERROR); EXPECT_TRUE(commissionMgr.IsCommissioningWindowOpen()); EXPECT_EQ(commissionMgr.CommissioningWindowStatusForCluster(),