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).

src/app/server/CommissioningWindowManager.cpp

@@ -171,17 +171,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<ReliableMessageProtocolConfig>::Value(gDefaultMRPConfig), this));
-
-        // reset all advertising, indicating we are in commissioningMode
-        app::DnssdServer::Instance().StartServer(Dnssd::CommissioningMode::kEnabledEnhanced);
     }
     else
     {
@@ -192,11 +187,10 @@ CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow()
             pinCode, kSpake2p_Iteration_Count,
             ByteSpan(reinterpret_cast<const uint8_t *>(kSpake2pKeyExchangeSalt), strlen(kSpake2pKeyExchangeSalt)), keyID,
             Optional<ReliableMessageProtocolConfig>::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 +278,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
@@ -313,6 +313,11 @@ CHIP_ERROR CommissioningWindowManager::StopAdvertisement()
     DeviceLayer::ConnectivityMgr().RequestSEDFastPollingMode(false);
 #endif
 
+    // 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));

src/app/server/CommissioningWindowManager.h

@@ -66,6 +66,8 @@ class CommissioningWindowManager : public SessionEstablishmentDelegate
     void OnSessionEstablishmentStarted() override;
     void OnSessionEstablished() override;
 
+    // Shutdown must be called before shutting down the DNS-SD subsystem, since
+    // we may need to touch DNS-SD as we shut down.
     void Shutdown();
     void Cleanup();
 

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<int>(mode));
+    return StartServer(NullOptional);
+}
+
+void DnssdServer::StartServer(Dnssd::CommissioningMode mode)
+{
+    return StartServer(MakeOptional(mode));
+}
+
+void DnssdServer::StartServer(Optional<Dnssd::CommissioningMode> 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<int>(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));
@@ -453,7 +466,7 @@ void DnssdServer::StartServer(chip::Dnssd::CommissioningMode mode)
         }
 #endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
     }
-    else
+    else if (!mode.HasValue())
     {
 #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY
         ChipLogProgress(Discovery, "Start dns-sd server - no current nodeId");

src/app/server/Dnssd.h

@@ -18,6 +18,7 @@
 #pragma once
 
 #include <lib/core/CHIPError.h>
+#include <lib/core/Optional.h>
 #include <lib/dnssd/Advertiser.h>
 #include <platform/CHIPDeviceLayer.h>
 #include <stddef.h>
@@ -79,8 +80,11 @@ class DLL_EXPORT DnssdServer
 
     /// (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 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<Dnssd::CommissioningMode> mode);
+
     uint16_t mSecuredPort   = CHIP_PORT;
     uint16_t mUnsecuredPort = CHIP_UDC_PORT;
 

src/app/server/Server.cpp

@@ -252,12 +252,14 @@ CHIP_ERROR Server::Init(AppDelegate * delegate, uint16_t secureServicePort, uint
 
 void Server::Shutdown()
 {
+    // We need to shutdown mCommissioningWindowManager before we shut down
+    // DNS-SD bits, because its shutdown touches DNS-SD advertising.
+    mCommissioningWindowManager.Shutdown();
     chip::Dnssd::ServiceAdvertiser::Instance().Shutdown();
     chip::app::InteractionModelEngine::GetInstance()->Shutdown();
     mExchangeMgr.Shutdown();
     mSessions.Shutdown();
     mTransports.Close();
-    mCommissioningWindowManager.Shutdown();
     chip::Platform::MemoryShutdown();
 }