Fix DNS-SD behavior relating to commissioning (#8888)

* cleanup fixes relating to udc

* add tests, fix platform eventloop conflict

* build fix

* restyled fix

* merge with latest

* fix CM and AC in DNS-SD

* address PR comments

* fix breakage, change from bitmap to enum

* PR feedback, read port values from platform config

* straggler

* fix darwin build

* more signature change fixes

* move port config to Mdns and Server

* add command line options for setting ports

* cleanup naming of CM/AC options

* fix builds without MDNS

examples/minimal-mdns/advertiser.cpp

@@ -274,7 +274,8 @@ int main(int argc, char ** args)
                 .SetMac(chip::ByteSpan(gOptions.mac, 6))
                 .SetVendorId(gOptions.vendorId)
                 .SetProductId(gOptions.productId)
-                .SetCommissioningMode(gOptions.commissioningMode, gOptions.commissioningModeOpenWindow)
+                .SetCommissioningMode(gOptions.commissioningMode)
+                .SetAdditionalCommissioning(gOptions.commissioningModeOpenWindow)
                 .SetDeviceType(gOptions.deviceType)
                 .SetDeviceName(gOptions.deviceName)
                 .SetRotatingId(gOptions.rotatingId)

examples/platform/linux/AppMain.cpp

@@ -188,8 +188,10 @@ CHIP_ERROR InitCommissioner()
 
     ReturnErrorOnFailure(gOpCredsIssuer.Initialize(gServerStorage));
 
-    ReturnErrorOnFailure(gCommissioner.SetUdpListenPort(CHIP_PORT + 2));
-    ReturnErrorOnFailure(gCommissioner.SetUdcListenPort(CHIP_PORT + 3));
+    // use a different listen port for the commissioner.
+    ReturnErrorOnFailure(gCommissioner.SetUdpListenPort(LinuxDeviceOptions::GetInstance().securedCommissionerPort));
+    // No need to explicitly set the UDC port since we will use default
+    ReturnErrorOnFailure(gCommissioner.SetUdcListenPort(LinuxDeviceOptions::GetInstance().unsecuredCommissionerPort));
 
     chip::Platform::ScopedMemoryBuffer<uint8_t> noc;
     VerifyOrReturnError(noc.Alloc(chip::Controller::kMaxCHIPDERCertLength), CHIP_ERROR_NO_MEMORY);
@@ -234,6 +236,14 @@ void ChipLinuxAppMainLoop()
     chip::Shell::RegisterCommissioneeCommands();
 #endif
 
+#if CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
+    // use a different service port to make testing possible with other sample devices running on same host
+    ServerConfigParams params;
+    params.securedServicePort   = LinuxDeviceOptions::GetInstance().securedDevicePort;
+    params.unsecuredServicePort = LinuxDeviceOptions::GetInstance().unsecuredCommissionerPort;
+    SetServerConfig(params);
+#endif // CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
+
     // Init ZCL Data Model and CHIP App Server
     InitServer();
 

examples/platform/linux/CommissioneeShellCommands.cpp

@@ -20,6 +20,7 @@
  */
 
 #include <CommissioneeShellCommands.h>
+#include <app/server/Mdns.h>
 #include <app/server/Server.h>
 #include <inttypes.h>
 #include <lib/core/CHIPCore.h>
@@ -61,6 +62,10 @@ static CHIP_ERROR PrintAllCommands()
     streamer_printf(sout,
                     "  sendudc <address> <port>   Send UDC message to address. Usage: commissionee sendudc 127.0.0.1 5543\r\n");
 #endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY_CLIENT
+    streamer_printf(sout,
+                    "  restartmdns <commissioningMode> (disabled|enabled_basic|enabled_enhanced)   Start Mdns with given "
+                    "settings. Usage: commissionee "
+                    "restartmdns enabled_basic\r\n");
     streamer_printf(sout, "\r\n");
 
     return CHIP_NO_ERROR;
@@ -84,6 +89,29 @@ static CHIP_ERROR CommissioneeHandler(int argc, char ** argv)
         return error  = SendUDC(true, chip::Transport::PeerAddress::UDP(commissioner, port));
     }
 #endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY_CLIENT
+    else if (strcmp(argv[0], "restartmdns") == 0)
+    {
+        if (argc < 2)
+        {
+            return PrintAllCommands();
+        }
+        if (strcmp(argv[1], "disabled") == 0)
+        {
+            chip::app::Mdns::StartServer(chip::app::Mdns::CommissioningMode::kDisabled);
+            return CHIP_NO_ERROR;
+        }
+        if (strcmp(argv[1], "enabled_basic") == 0)
+        {
+            chip::app::Mdns::StartServer(chip::app::Mdns::CommissioningMode::kEnabledBasic);
+            return CHIP_NO_ERROR;
+        }
+        else if (strcmp(argv[1], "enabled_enhanced") == 0)
+        {
+            chip::app::Mdns::StartServer(chip::app::Mdns::CommissioningMode::kEnabledEnhanced);
+            return CHIP_NO_ERROR;
+        }
+        return PrintAllCommands();
+    }
     else
     {
         return CHIP_ERROR_INVALID_ARGUMENT;

examples/platform/linux/Options.cpp

@@ -33,16 +33,19 @@ LinuxDeviceOptions gDeviceOptions;
 // Follow the code style of command line arguments in case we need to add more options in the future.
 enum
 {
-    kDeviceOption_BleDevice     = 0x1000,
-    kDeviceOption_WiFi          = 0x1001,
-    kDeviceOption_Thread        = 0x1002,
-    kDeviceOption_Version       = 0x1003,
-    kDeviceOption_VendorID      = 0x1004,
-    kDeviceOption_ProductID     = 0x1005,
-    kDeviceOption_CustomFlow    = 0x1006,
-    kDeviceOption_Capabilities  = 0x1007,
-    kDeviceOption_Discriminator = 0x1008,
-    kDeviceOption_Passcode      = 0x1009
+    kDeviceOption_BleDevice                 = 0x1000,
+    kDeviceOption_WiFi                      = 0x1001,
+    kDeviceOption_Thread                    = 0x1002,
+    kDeviceOption_Version                   = 0x1003,
+    kDeviceOption_VendorID                  = 0x1004,
+    kDeviceOption_ProductID                 = 0x1005,
+    kDeviceOption_CustomFlow                = 0x1006,
+    kDeviceOption_Capabilities              = 0x1007,
+    kDeviceOption_Discriminator             = 0x1008,
+    kDeviceOption_Passcode                  = 0x1009,
+    kDeviceOption_SecuredDevicePort         = 0x100a,
+    kDeviceOption_SecuredCommissionerPort   = 0x100b,
+    kDeviceOption_UnsecuredCommissionerPort = 0x100c
 };
 
 constexpr unsigned kAppUsageLength = 64;
@@ -61,6 +64,9 @@ OptionDef sDeviceOptionDefs[] = { { "ble-device", kArgumentRequired, kDeviceOpti
                                   { "capabilities", kArgumentRequired, kDeviceOption_Capabilities },
                                   { "discriminator", kArgumentRequired, kDeviceOption_Discriminator },
                                   { "passcode", kArgumentRequired, kDeviceOption_Passcode },
+                                  { "secured-device-port", kArgumentRequired, kDeviceOption_SecuredDevicePort },
+                                  { "secured-commissioner-port", kArgumentRequired, kDeviceOption_SecuredCommissionerPort },
+                                  { "unsecured-commissioner-port", kArgumentRequired, kDeviceOption_UnsecuredCommissionerPort },
                                   {} };
 
 const char * sDeviceOptionHelp =
@@ -97,6 +103,16 @@ const char * sDeviceOptionHelp =
     "\n"
     "  --passcode <passcode>\n"
     "       A 27-bit unsigned integer, which serves as proof of possession during commissioning.\n"
+    "\n"
+    "  --secured-device-port <port>\n"
+    "       A 16-bit unsigned integer specifying the listen port to use for secure device messages (default is 5540).\n"
+    "\n"
+    "  --secured-commissioner-port <port>\n"
+    "       A 16-bit unsigned integer specifying the listen port to use for secure commissioner messages (default is 5542). Only "
+    "valid when app is both device and commissioner\n"
+    "\n"
+    "  --unsecured-commissioner-port <port>\n"
+    "       A 16-bit unsigned integer specifying the port to use for unsecured commissioner messages (default is 5550).\n"
     "\n";
 
 bool HandleOption(const char * aProgram, OptionSet * aOptions, int aIdentifier, const char * aName, const char * aValue)
@@ -150,6 +166,18 @@ bool HandleOption(const char * aProgram, OptionSet * aOptions, int aIdentifier,
         LinuxDeviceOptions::GetInstance().payload.setUpPINCode = static_cast<uint32_t>(atoi(aValue));
         break;
 
+    case kDeviceOption_SecuredDevicePort:
+        LinuxDeviceOptions::GetInstance().securedDevicePort = static_cast<uint16_t>(atoi(aValue));
+        break;
+
+    case kDeviceOption_SecuredCommissionerPort:
+        LinuxDeviceOptions::GetInstance().securedCommissionerPort = static_cast<uint16_t>(atoi(aValue));
+        break;
+
+    case kDeviceOption_UnsecuredCommissionerPort:
+        LinuxDeviceOptions::GetInstance().unsecuredCommissionerPort = static_cast<uint16_t>(atoi(aValue));
+        break;
+
     default:
         PrintArgError("%s: INTERNAL ERROR: Unhandled option: %s\n", aProgram, aName);
         retval = false;

examples/platform/linux/Options.h

@@ -32,9 +32,12 @@
 struct LinuxDeviceOptions
 {
     chip::SetupPayload payload;
-    uint32_t mBleDevice = 0;
-    bool mWiFi          = false;
-    bool mThread        = false;
+    uint32_t mBleDevice                = 0;
+    bool mWiFi                         = false;
+    bool mThread                       = false;
+    uint32_t securedDevicePort         = CHIP_PORT;
+    uint32_t securedCommissionerPort   = CHIP_PORT + 2;
+    uint32_t unsecuredCommissionerPort = CHIP_UDC_PORT;
 
     static LinuxDeviceOptions & GetInstance();
 };

src/app/server/Mdns.cpp

@@ -78,6 +78,29 @@ chip::ByteSpan FillMAC(uint8_t (&mac)[8])
 
 } // namespace
 
+uint16_t gSecuredPort   = CHIP_PORT;
+uint16_t gUnsecuredPort = CHIP_UDC_PORT;
+
+void SetSecuredPort(uint16_t port)
+{
+    gSecuredPort = port;
+}
+
+uint16_t GetSecuredPort()
+{
+    return gSecuredPort;
+}
+
+void SetUnsecuredPort(uint16_t port)
+{
+    gUnsecuredPort = port;
+}
+
+uint16_t GetUnsecuredPort()
+{
+    return gUnsecuredPort;
+}
+
 CHIP_ERROR GetCommissionableInstanceName(char * buffer, size_t bufferLen)
 {
     auto & mdnsAdvertiser = chip::Mdns::ServiceAdvertiser::Instance();
@@ -97,7 +120,7 @@ CHIP_ERROR AdvertiseOperational()
                 chip::Mdns::OperationalAdvertisingParameters()
                     .SetPeerId(PeerId().SetFabricId(fabricInfo.GetFabricId()).SetNodeId(fabricInfo.GetNodeId()))
                     .SetMac(FillMAC(mac))
-                    .SetPort(CHIP_PORT)
+                    .SetPort(GetSecuredPort())
                     .SetMRPRetryIntervals(CHIP_CONFIG_MRP_DEFAULT_ACTIVE_RETRY_INTERVAL,
                                           CHIP_CONFIG_MRP_DEFAULT_ACTIVE_RETRY_INTERVAL)
                     .EnableIpV4(true);
@@ -112,35 +135,27 @@ CHIP_ERROR AdvertiseOperational()
             ReturnErrorOnFailure(mdnsAdvertiser.Advertise(advertiseParameters));
         }
     }
-
     return CHIP_NO_ERROR;
 }
 
-/// Set MDNS commissioner advertisement
-CHIP_ERROR AdvertiseCommisioner()
-{
-    return Advertise(false);
-}
-
-/// Set MDNS commissionable node advertisement
-CHIP_ERROR AdvertiseCommissionableNode()
+/// Overloaded utility method for commissioner and commissionable advertisement
+/// This method is used for both commissioner discovery and commissionable node discovery since
+/// they share many fields.
+///   commissionableNode = true : advertise commissionable node
+///   commissionableNode = false : advertise commissioner
+CHIP_ERROR Advertise(bool commissionableNode, CommissioningMode mode)
 {
-    return Advertise(true);
-}
+    bool commissioningMode       = (mode != CommissioningMode::kDisabled);
+    bool additionalCommissioning = (mode == CommissioningMode::kEnabledEnhanced);
 
-/// commissionableNode
-// CHIP_ERROR Advertise(chip::Mdns::CommssionAdvertiseMode mode)
-CHIP_ERROR Advertise(bool commissionableNode)
-{
-    auto advertiseParameters = chip::Mdns::CommissionAdvertisingParameters().SetPort(CHIP_PORT).EnableIpV4(true);
+    auto advertiseParameters = chip::Mdns::CommissionAdvertisingParameters()
+                                   .SetPort(commissionableNode ? GetSecuredPort() : GetUnsecuredPort())
+                                   .EnableIpV4(true);
     advertiseParameters.SetCommissionAdvertiseMode(commissionableNode ? chip::Mdns::CommssionAdvertiseMode::kCommissionableNode
                                                                       : chip::Mdns::CommssionAdvertiseMode::kCommissioner);
 
-    // TODO: device can re-enter commissioning mode after being fully provisioned
-    // (additionalPairing == true)
-    bool notYetCommissioned = !DeviceLayer::ConfigurationMgr().IsFullyProvisioned();
-    bool additionalPairing  = false;
-    advertiseParameters.SetCommissioningMode(notYetCommissioned, additionalPairing);
+    advertiseParameters.SetCommissioningMode(commissioningMode);
+    advertiseParameters.SetAdditionalCommissioning(additionalCommissioning);
 
     char pairingInst[chip::Mdns::kKeyPairingInstructionMaxLength + 1];
 
@@ -192,7 +207,7 @@ CHIP_ERROR Advertise(bool commissionableNode)
     advertiseParameters.SetRotatingId(chip::Optional<const char *>::Value(rotatingDeviceIdHexBuffer));
 #endif
 
-    if (notYetCommissioned)
+    if (!additionalCommissioning)
     {
         if (DeviceLayer::ConfigurationMgr().GetInitialPairingHint(value) != CHIP_NO_ERROR)
         {
@@ -241,10 +256,23 @@ CHIP_ERROR Advertise(bool commissionableNode)
     return mdnsAdvertiser.Advertise(advertiseParameters);
 }
 
+/// Set MDNS commissioner advertisement
+CHIP_ERROR AdvertiseCommissioner()
+{
+    return Advertise(false /* commissionableNode */, CommissioningMode::kDisabled);
+}
+
+/// Set MDNS commissionable node advertisement
+CHIP_ERROR AdvertiseCommissionableNode(CommissioningMode mode)
+{
+    return Advertise(true /* commissionableNode */, mode);
+}
+
 /// (Re-)starts the minmdns server
-void StartServer()
+/// - 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(CommissioningMode mode)
 {
-    ChipLogProgress(Discovery, "Start dns-sd server");
     CHIP_ERROR err = chip::Mdns::ServiceAdvertiser::Instance().Start(&chip::DeviceLayer::InetLayer, chip::Mdns::kMdnsPort);
 
     err = app::Mdns::AdvertiseOperational();
@@ -255,25 +283,44 @@ void StartServer()
 
     if (HaveOperationalCredentials())
     {
+        if (mode != CommissioningMode::kDisabled)
+        {
+            err = app::Mdns::AdvertiseCommissionableNode(mode);
+            if (err != CHIP_NO_ERROR)
+            {
+                ChipLogError(Discovery, "Failed to advertise commissionable node: %s", chip::ErrorStr(err));
+            }
+        }
 #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
-        err = app::Mdns::AdvertiseCommissionableNode();
-#endif
+        else
+        {
+            err = app::Mdns::AdvertiseCommissionableNode(mode);
+            if (err != CHIP_NO_ERROR)
+            {
+                ChipLogError(Discovery, "Failed to advertise extended commissionable node: %s", chip::ErrorStr(err));
+            }
+        }
+#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
     }
     else
     {
 #if CHIP_DEVICE_CONFIG_ENABLE_UNPROVISIONED_MDNS
-        err = app::Mdns::AdvertiseCommissionableNode();
+        ChipLogProgress(Discovery, "Start dns-sd server - no current nodeId");
+        err = app::Mdns::AdvertiseCommissionableNode(CommissioningMode::kEnabledBasic);
+        if (err != CHIP_NO_ERROR)
+        {
+            ChipLogError(Discovery, "Failed to advertise unprovisioned commissionable node: %s", chip::ErrorStr(err));
+        }
 #endif
     }
 
 #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY
-    err = app::Mdns::AdvertiseCommisioner();
-#endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY
-
+    err = app::Mdns::AdvertiseCommissioner();
     if (err != CHIP_NO_ERROR)
     {
-        ChipLogError(Discovery, "Failed to start mDNS server: %s", chip::ErrorStr(err));
+        ChipLogError(Discovery, "Failed to advertise commissioner: %s", chip::ErrorStr(err));
     }
+#endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY
 }
 
 #if CHIP_ENABLE_ROTATING_DEVICE_ID

src/app/server/Mdns.h

@@ -24,21 +24,38 @@ namespace chip {
 namespace app {
 namespace Mdns {
 
+enum class CommissioningMode
+{
+    kDisabled,       // Commissioning Mode is disabled, CM=0, AC=0 in DNS-SD key/value pairs
+    kEnabledBasic,   // Basic Commissioning Mode, CM=1, AC=0 in DNS-SD key/value pairs
+    kEnabledEnhanced // Enhanced Commissioning Mode, CM=1, AC=1 in DNS-SD key/value pairs
+};
+
+/// Sets the secure Matter port
+void SetSecuredPort(uint16_t port);
+
+/// Gets the secure Matter port
+uint16_t GetSecuredPort();
+
+/// Sets the unsecure Matter port
+void SetUnsecuredPort(uint16_t port);
+
+/// Gets the unsecure Matter port
+uint16_t GetUnsecuredPort();
+
 /// Start operational advertising
 CHIP_ERROR AdvertiseOperational();
 
 /// Set MDNS commissioner advertisement
 CHIP_ERROR AdvertiseCommissioner();
 
 /// Set MDNS commissionable node advertisement
-CHIP_ERROR AdvertiseCommissionableNode();
-
-/// Set MDNS advertisement
-// CHIP_ERROR Advertise(chip::Mdns::CommssionAdvertiseMode mode);
-CHIP_ERROR Advertise(bool commissionableNode);
+CHIP_ERROR AdvertiseCommissionableNode(CommissioningMode mode);
 
 /// (Re-)starts the minmdns server
-void StartServer();
+/// - 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(CommissioningMode mode = CommissioningMode::kDisabled);
 
 CHIP_ERROR GenerateRotatingDeviceId(char rotatingDeviceIdHexBuffer[], size_t rotatingDeviceIdHexBufferSize);