From 1701673d0d6c2d91df24baec8bfb01ad3c3a7a3e Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 21 Jan 2022 21:41:30 -0500 Subject: [PATCH] Make sure we don't advertise with CM=1 when we're not accepting PASE connections. (#13813) * Make sure we don't advertise with CM=1 when we're not accepting PASE connections. Right now it's possible for a device to both be advertising over dns-sd with CM=1 and not be accepting PASE connections. Which makes the CM=1 not exactly true. Summary of changes: 1. Change DnssdServer to clearly differentiate between "Start with default commissioning mode behavior based on whether we have operational credentials" and "Start with commissioning mode disabled", instead of treating the two as synonyms. I audited all callsites that explicitly passed kDisabled to StartServer() instead of no args, and they all seem to want to actually disable commissioning mode. 2. Change CommissioningWindowManager to start/stop DNS-SD commissioning advertising in the same codepaths as BLE advertising. This means we stop advertising with CM=1 or CM=2 when we stop accepting PASE connections, and start doing it again when we start accepting PASE connections again (e.g. if commissioning fails). * Apply review suggestion. Co-authored-by: chrisdecenzo <61757564+chrisdecenzo@users.noreply.github.com> Co-authored-by: chrisdecenzo <61757564+chrisdecenzo@users.noreply.github.com> --- src/app/server/CommissioningWindowManager.cpp | 45 ++++++++++++------- src/app/server/CommissioningWindowManager.h | 7 ++- src/app/server/Dnssd.cpp | 25 ++++++++--- src/app/server/Dnssd.h | 13 ++++-- 4 files changed, 64 insertions(+), 26 deletions(-) diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp index 91a92c4dafd758..7b3eec0a6b96da 100644 --- a/src/app/server/CommissioningWindowManager.cpp +++ b/src/app/server/CommissioningWindowManager.cpp @@ -73,8 +73,13 @@ void CommissioningWindowManager::OnPlatformEvent(const DeviceLayer::ChipDeviceEv void CommissioningWindowManager::Shutdown() { - StopAdvertisement(); + StopAdvertisement(/* aShuttingDown = */ true); + ResetState(); +} + +void CommissioningWindowManager::ResetState() +{ mUseECM = false; mECMDiscriminator = 0; @@ -88,10 +93,9 @@ void CommissioningWindowManager::Shutdown() void CommissioningWindowManager::Cleanup() { - Shutdown(); + StopAdvertisement(/* aShuttingDown = */ false); - // reset all advertising - app::DnssdServer::Instance().StartServer(Dnssd::CommissioningMode::kDisabled); + ResetState(); } void CommissioningWindowManager::OnSessionEstablishmentError(CHIP_ERROR err) @@ -151,7 +155,7 @@ void CommissioningWindowManager::OnSessionEstablished() DeviceLayer::PlatformMgr().AddEventHandler(OnPlatformEventWrapper, reinterpret_cast(this)); - StopAdvertisement(); + StopAdvertisement(/* aShuttingDown = */ true); ChipLogProgress(AppServer, "Device completed Rendezvous process"); } @@ -171,17 +175,12 @@ CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow() ReturnErrorOnFailure(mServer->GetExchangeManager().RegisterUnsolicitedMessageHandlerForType( Protocols::SecureChannel::MsgType::PBKDFParamRequest, &mPairingSession)); - ReturnErrorOnFailure(StartAdvertisement()); - if (mUseECM) { ReturnErrorOnFailure(SetTemporaryDiscriminator(mECMDiscriminator)); ReturnErrorOnFailure( mPairingSession.WaitForPairing(mECMPASEVerifier, mECMIterations, ByteSpan(mECMSalt, mECMSaltLength), mECMPasscodeID, keyID, Optional::Value(gDefaultMRPConfig), this)); - - // reset all advertising, indicating we are in commissioningMode - app::DnssdServer::Instance().StartServer(Dnssd::CommissioningMode::kEnabledEnhanced); } else { @@ -192,11 +191,10 @@ CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow() pinCode, kSpake2p_Iteration_Count, ByteSpan(reinterpret_cast(kSpake2pKeyExchangeSalt), strlen(kSpake2pKeyExchangeSalt)), keyID, Optional::Value(gDefaultMRPConfig), this)); - - // reset all advertising, indicating we are in commissioningMode - app::DnssdServer::Instance().StartServer(Dnssd::CommissioningMode::kEnabledBasic); } + ReturnErrorOnFailure(StartAdvertisement()); + return CHIP_NO_ERROR; } @@ -284,15 +282,21 @@ CHIP_ERROR CommissioningWindowManager::StartAdvertisement() mAppDelegate->OnPairingWindowOpened(); } + Dnssd::CommissioningMode commissioningMode; if (mUseECM) { - mWindowStatus = app::Clusters::AdministratorCommissioning::CommissioningWindowStatus::kEnhancedWindowOpen; + mWindowStatus = app::Clusters::AdministratorCommissioning::CommissioningWindowStatus::kEnhancedWindowOpen; + commissioningMode = Dnssd::CommissioningMode::kEnabledEnhanced; } else { - mWindowStatus = app::Clusters::AdministratorCommissioning::CommissioningWindowStatus::kBasicWindowOpen; + mWindowStatus = app::Clusters::AdministratorCommissioning::CommissioningWindowStatus::kBasicWindowOpen; + commissioningMode = Dnssd::CommissioningMode::kEnabledBasic; } + // reset all advertising, indicating we are in commissioningMode + app::DnssdServer::Instance().StartServer(commissioningMode); + #if CHIP_DEVICE_CONFIG_ENABLE_SED DeviceLayer::ConnectivityMgr().RequestSEDFastPollingMode(true); #endif @@ -300,7 +304,7 @@ CHIP_ERROR CommissioningWindowManager::StartAdvertisement() return CHIP_NO_ERROR; } -CHIP_ERROR CommissioningWindowManager::StopAdvertisement() +CHIP_ERROR CommissioningWindowManager::StopAdvertisement(bool aShuttingDown) { RestoreDiscriminator(); @@ -313,6 +317,15 @@ CHIP_ERROR CommissioningWindowManager::StopAdvertisement() DeviceLayer::ConnectivityMgr().RequestSEDFastPollingMode(false); #endif + // If aShuttingDown, don't try to change our DNS-SD advertisements. + if (!aShuttingDown) + { + // Stop advertising commissioning mode, since we're not accepting PASE + // connections right now. If we start accepting them again (via + // OpenCommissioningWindow) that will call StartAdvertisement as needed. + app::DnssdServer::Instance().StartServer(Dnssd::CommissioningMode::kDisabled); + } + if (mIsBLE) { ReturnErrorOnFailure(chip::DeviceLayer::ConnectivityMgr().SetBLEAdvertisingEnabled(false)); diff --git a/src/app/server/CommissioningWindowManager.h b/src/app/server/CommissioningWindowManager.h index 99b324f2ac9c93..1361a488669869 100644 --- a/src/app/server/CommissioningWindowManager.h +++ b/src/app/server/CommissioningWindowManager.h @@ -78,10 +78,15 @@ class CommissioningWindowManager : public SessionEstablishmentDelegate CHIP_ERROR StartAdvertisement(); - CHIP_ERROR StopAdvertisement(); + CHIP_ERROR StopAdvertisement(bool aShuttingDown); CHIP_ERROR OpenCommissioningWindow(); + // Helper for Shutdown and Cleanup. Does not do anything with + // advertisements, because Shutdown and Cleanup want to handle those + // differently. + void ResetState(); + AppDelegate * mAppDelegate = nullptr; Server * mServer = nullptr; diff --git a/src/app/server/Dnssd.cpp b/src/app/server/Dnssd.cpp index 3ce263b7cf3afc..20c5dae999fc10 100644 --- a/src/app/server/Dnssd.cpp +++ b/src/app/server/Dnssd.cpp @@ -402,9 +402,22 @@ CHIP_ERROR DnssdServer::AdvertiseCommissionableNode(chip::Dnssd::CommissioningMo return Advertise(true /* commissionableNode */, mode); } -void DnssdServer::StartServer(chip::Dnssd::CommissioningMode mode) +void DnssdServer::StartServer() { - ChipLogDetail(Discovery, "DNS-SD StartServer mode=%d", static_cast(mode)); + return StartServer(NullOptional); +} + +void DnssdServer::StartServer(Dnssd::CommissioningMode mode) +{ + return StartServer(MakeOptional(mode)); +} + +void DnssdServer::StartServer(Optional mode) +{ + // Default to CommissioningMode::kDisabled if no value is provided. + Dnssd::CommissioningMode modeValue = mode.ValueOr(Dnssd::CommissioningMode::kDisabled); + + ChipLogDetail(Discovery, "DNS-SD StartServer modeHasValue=%d modeValue=%d", mode.HasValue(), static_cast(modeValue)); ClearTimeouts(); @@ -431,9 +444,9 @@ void DnssdServer::StartServer(chip::Dnssd::CommissioningMode mode) if (HaveOperationalCredentials()) { ChipLogProgress(Discovery, "Have operational credentials"); - if (mode != chip::Dnssd::CommissioningMode::kDisabled) + if (modeValue != chip::Dnssd::CommissioningMode::kDisabled) { - err = AdvertiseCommissionableNode(mode); + err = AdvertiseCommissionableNode(modeValue); if (err != CHIP_NO_ERROR) { ChipLogError(Discovery, "Failed to advertise commissionable node: %s", chip::ErrorStr(err)); @@ -443,7 +456,7 @@ void DnssdServer::StartServer(chip::Dnssd::CommissioningMode mode) #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY else if (GetExtendedDiscoveryTimeoutSecs() != CHIP_DEVICE_CONFIG_DISCOVERY_DISABLED) { - err = AdvertiseCommissionableNode(mode); + err = AdvertiseCommissionableNode(modeValue); if (err != CHIP_NO_ERROR) { ChipLogError(Discovery, "Failed to advertise extended commissionable node: %s", chip::ErrorStr(err)); @@ -457,7 +470,7 @@ void DnssdServer::StartServer(chip::Dnssd::CommissioningMode mode) { #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY ChipLogProgress(Discovery, "Start dns-sd server - no current nodeId"); - err = AdvertiseCommissionableNode(chip::Dnssd::CommissioningMode::kEnabledBasic); + err = AdvertiseCommissionableNode(mode.ValueOr(chip::Dnssd::CommissioningMode::kEnabledBasic)); if (err != CHIP_NO_ERROR) { ChipLogError(Discovery, "Failed to advertise unprovisioned commissionable node: %s", chip::ErrorStr(err)); diff --git a/src/app/server/Dnssd.h b/src/app/server/Dnssd.h index 7fce0ecb8255e3..b92410306690d7 100644 --- a/src/app/server/Dnssd.h +++ b/src/app/server/Dnssd.h @@ -18,6 +18,7 @@ #pragma once #include +#include #include #include #include @@ -78,9 +79,12 @@ class DLL_EXPORT DnssdServer CHIP_ERROR AdvertiseOperational(); /// (Re-)starts the Dnssd server - /// - if device has not yet been commissioned, then commissioning mode will show as enabled (CM=1, AC=0) - /// - if device has been commissioned, then commissioning mode will reflect the state of mode argument - void StartServer(chip::Dnssd::CommissioningMode mode = chip::Dnssd::CommissioningMode::kDisabled); + /// - if device has not yet been commissioned, then commissioning mode will show as enabled (CM=1) + /// - if device has been commissioned, then commissioning mode will be disabled. + void StartServer(); + + /// (Re-)starts the Dnssd server, using the provided commissioning mode. + void StartServer(Dnssd::CommissioningMode mode); CHIP_ERROR GenerateRotatingDeviceId(char rotatingDeviceIdHexBuffer[], size_t rotatingDeviceIdHexBufferSize); @@ -111,6 +115,9 @@ class DLL_EXPORT DnssdServer #endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY } + // Helper for StartServer. + void StartServer(Optional mode); + uint16_t mSecuredPort = CHIP_PORT; uint16_t mUnsecuredPort = CHIP_UDC_PORT;