MinMDNS: reduce amount of data in the mdns advertising packets (and m…

…ake it more RFC compliant) (#27274)

* Make test advertiser support multi-admin logic to test packet content

* Make IP addresses sent only once

* Update advertiser to make srv port unique

* Update comment a bit

* Do not list dnssd service listing at boot time advertisement

* Clang-format and mark boot time advertisements to have everything as answers instead of additional records

* Add missing break

* Restyled by clang-format

* rg InternalBroadcast --files-with-matches | xargs sd 'InternalBroadcast' 'AnnounceBroadcast'

* Rename Accept to ShouldSend

---------

Co-authored-by: Andrei Litvin <andreilitvin@google.com>
Co-authored-by: Restyled.io <commits@restyled.io>

examples/minimal-mdns/advertiser.cpp

@@ -35,6 +35,7 @@ enum class AdvertisingMode
 {
     kCommissionableNode,
     kOperational,
+    kOperationalMultiAdmin,
     kCommissioner,
 };
 
@@ -100,6 +101,10 @@ bool HandleOptions(const char * aProgram, OptionSet * aOptions, int aIdentifier,
         {
             gOptions.advertisingMode = AdvertisingMode::kOperational;
         }
+        else if (strcmp(aValue, "operational-multi-admin") == 0)
+        {
+            gOptions.advertisingMode = AdvertisingMode::kOperationalMultiAdmin;
+        }
         else if (strcmp(aValue, "commissionable-node") == 0)
         {
             gOptions.advertisingMode = AdvertisingMode::kCommissionableNode;
@@ -196,48 +201,50 @@ OptionDef cmdLineOptionsDef[] = {
     {},
 };
 
-OptionSet cmdLineOptions = { HandleOptions, cmdLineOptionsDef, "PROGRAM OPTIONS",
+OptionSet cmdLineOptions = {
+    HandleOptions, cmdLineOptionsDef, "PROGRAM OPTIONS",
 #if INET_CONFIG_ENABLE_IPV4
-                             "  -4\n"
-                             "  --enable-ip-v4\n"
-                             "        enable listening on IPv4\n"
+    "  -4\n"
+    "  --enable-ip-v4\n"
+    "        enable listening on IPv4\n"
 #endif
-                             "  -m <mode>\n"
-                             "  --advertising-mode <mode>\n"
-                             "        Advertise in this mode (operational or commissionable-node or commissioner).\n"
-                             "  --short-discriminator <value>\n"
-                             "  -s <value>\n"
-                             "        Commissioning/commissionable short discriminator.\n"
-                             "  --long-discriminator <value>\n"
-                             "  -l <value>\n"
-                             "        Commissioning/commissionable long discriminator.\n"
-                             "  --vendor-id <value>\n"
-                             "        Commissioning/commissionable vendor id.\n"
-                             "  --product-id <value>\n"
-                             "  -p <value>\n"
-                             "        Commissioning/commissionable product id.\n"
-                             "  --commissioning-mode <value>\n"
-                             "        Commissioning Mode (0=disabled, 1=basic, 2=enhanced).\n"
-                             "  --device-type <value>\n"
-                             "        Device type id.\n"
-                             "  --device-name <value>\n"
-                             "        Name of device.\n"
-                             "  --rotating-id <value>\n"
-                             "        Rotating Id.\n"
-                             "  --pairing-instruction <value>\n"
-                             "        Commissionable pairing instruction.\n"
-                             "  --pairing-hint <value>\n"
-                             "        Commissionable pairing hint.\n"
-                             "  --fabric-id <value>\n"
-                             "  -f <value>\n"
-                             "        Operational fabric id.\n"
-                             "  --node-id <value>\n"
-                             "  -n <value>\n"
-                             "        Operational node id.\n"
-                             "  -t <dest>\n"
-                             "  --trace-to <dest>\n"
-                             "        trace to the given destination (supported: " SUPPORTED_COMMAND_LINE_TRACING_TARGETS ").\n"
-                             "\n" };
+    "  -m <mode>\n"
+    "  --advertising-mode <mode>\n"
+    "        Advertise in this mode (operational, operational-multi-admin, commissionable-node or commissioner).\n"
+    "  --short-discriminator <value>\n"
+    "  -s <value>\n"
+    "        Commissioning/commissionable short discriminator.\n"
+    "  --long-discriminator <value>\n"
+    "  -l <value>\n"
+    "        Commissioning/commissionable long discriminator.\n"
+    "  --vendor-id <value>\n"
+    "        Commissioning/commissionable vendor id.\n"
+    "  --product-id <value>\n"
+    "  -p <value>\n"
+    "        Commissioning/commissionable product id.\n"
+    "  --commissioning-mode <value>\n"
+    "        Commissioning Mode (0=disabled, 1=basic, 2=enhanced).\n"
+    "  --device-type <value>\n"
+    "        Device type id.\n"
+    "  --device-name <value>\n"
+    "        Name of device.\n"
+    "  --rotating-id <value>\n"
+    "        Rotating Id.\n"
+    "  --pairing-instruction <value>\n"
+    "        Commissionable pairing instruction.\n"
+    "  --pairing-hint <value>\n"
+    "        Commissionable pairing hint.\n"
+    "  --fabric-id <value>\n"
+    "  -f <value>\n"
+    "        Operational fabric id.\n"
+    "  --node-id <value>\n"
+    "  -n <value>\n"
+    "        Operational node id.\n"
+    "  -t <dest>\n"
+    "  --trace-to <dest>\n"
+    "        trace to the given destination (supported: " SUPPORTED_COMMAND_LINE_TRACING_TARGETS ").\n"
+    "\n"
+};
 
 HelpOptions helpOptions("advertiser", "Usage: advertiser [options]", "1.0");
 
@@ -304,6 +311,35 @@ int main(int argc, char ** args)
                 .SetMac(chip::ByteSpan(gOptions.mac, 6))
                 .SetPeerId(PeerId().SetCompressedFabricId(gOptions.fabricId).SetNodeId(gOptions.nodeId)));
     }
+    else if (gOptions.advertisingMode == AdvertisingMode::kOperationalMultiAdmin)
+    {
+        err = chip::Dnssd::ServiceAdvertiser::Instance().Advertise(
+            chip::Dnssd::OperationalAdvertisingParameters()
+                .EnableIpV4(gOptions.enableIpV4)
+                .SetPort(CHIP_PORT)
+                .SetMac(chip::ByteSpan(gOptions.mac, 6))
+                .SetPeerId(PeerId().SetCompressedFabricId(gOptions.fabricId).SetNodeId(gOptions.nodeId)));
+
+        if (err == CHIP_NO_ERROR)
+        {
+            err = chip::Dnssd::ServiceAdvertiser::Instance().Advertise(
+                chip::Dnssd::OperationalAdvertisingParameters()
+                    .EnableIpV4(gOptions.enableIpV4)
+                    .SetPort(CHIP_PORT + 1)
+                    .SetMac(chip::ByteSpan(gOptions.mac, 6))
+                    .SetPeerId(PeerId().SetCompressedFabricId(gOptions.fabricId + 1).SetNodeId(gOptions.nodeId + 1)));
+        }
+
+        if (err == CHIP_NO_ERROR)
+        {
+            err = chip::Dnssd::ServiceAdvertiser::Instance().Advertise(
+                chip::Dnssd::OperationalAdvertisingParameters()
+                    .EnableIpV4(gOptions.enableIpV4)
+                    .SetPort(CHIP_PORT + 2)
+                    .SetMac(chip::ByteSpan(gOptions.mac, 6))
+                    .SetPeerId(PeerId().SetCompressedFabricId(gOptions.fabricId + 2).SetNodeId(gOptions.nodeId + 2)));
+        }
+    }
     else if (gOptions.advertisingMode == AdvertisingMode::kCommissioner)
     {
         printf("Advertise Commissioner\n");

src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp

@@ -906,7 +906,7 @@ void AdvertiserMinMdns::AdvertiseRecords(BroadcastAdvertiseType type)
         // This optimization likely will take more logic and state storage, so
         // for now it is not done.
         QueryData queryData(QType::PTR, QClass::IN, false /* unicast */);
-        queryData.SetIsInternalBroadcast(true);
+        queryData.SetIsAnnounceBroadcast(true);
 
         for (auto & it : mOperationalResponders)
         {
@@ -916,6 +916,7 @@ void AdvertiserMinMdns::AdvertiseRecords(BroadcastAdvertiseType type)
         mQueryResponderAllocatorCommissioner.GetQueryResponder()->ClearBroadcastThrottle();
 
         CHIP_ERROR err = mResponseSender.Respond(0, queryData, &packetInfo, responseConfiguration);
+
         if (err != CHIP_NO_ERROR)
         {
             ChipLogError(Discovery, "Failed to advertise records: %" CHIP_ERROR_FORMAT, err.Format());

src/lib/dnssd/minimal_mdns/Parser.h

@@ -46,8 +46,8 @@ class QueryData
     /// any broadcast filtering. Intent is for paths such as:
     ///   - boot time advertisement: advertise all services available
     ///   - stop-time advertisement: advertise a TTL of 0 as services are removed
-    bool IsInternalBroadcast() const { return mIsInternalBroadcast; }
-    void SetIsInternalBroadcast(bool isInternalBroadcast) { mIsInternalBroadcast = isInternalBroadcast; }
+    bool IsAnnounceBroadcast() const { return mIsAnnounceBroadcast; }
+    void SetIsAnnounceBroadcast(bool isAnnounceBroadcast) { mIsAnnounceBroadcast = isAnnounceBroadcast; }
 
     SerializedQNameIterator GetName() const { return mNameIterator; }
 
@@ -69,7 +69,7 @@ class QueryData
 
     /// Flag as an internal broadcast, controls reply construction (e.g. no
     /// filtering applied)
-    bool mIsInternalBroadcast = false;
+    bool mIsAnnounceBroadcast = false;
 };
 
 class ResourceData

src/lib/dnssd/minimal_mdns/QueryReplyFilter.h

@@ -81,7 +81,7 @@ class QueryReplyFilter : public ReplyFilter
 
     bool AcceptablePath(FullQName qname)
     {
-        if (mIgnoreNameMatch || mQueryData.IsInternalBroadcast())
+        if (mIgnoreNameMatch || mQueryData.IsAnnounceBroadcast())
         {
             return true;
         }

src/lib/dnssd/minimal_mdns/ResponseSender.cpp

@@ -26,6 +26,8 @@ namespace Minimal {
 
 namespace {
 
+using namespace mdns::Minimal::Internal;
+
 constexpr uint16_t kMdnsStandardPort = 5353;
 
 // Restriction for UDP packets:  https://tools.ietf.org/html/rfc1035#section-4.2.1
@@ -105,6 +107,12 @@ CHIP_ERROR ResponseSender::Respond(uint16_t messageId, const QueryData & query,
 {
     mSendState.Reset(messageId, query, querySource);
 
+    if (query.IsAnnounceBroadcast())
+    {
+        // Deny listing large amount of data
+        mSendState.MarkWasSent(ResponseItemsSent::kServiceListingData);
+    }
+
     // Responder has a stateful 'additional replies required' that is used within the response
     // loop. 'no additionals required' is set at the start and additionals are marked as the query
     // reply is built.
@@ -158,7 +166,12 @@ CHIP_ERROR ResponseSender::Respond(uint16_t messageId, const QueryData & query,
 
     // send all 'Additional' replies
     {
-        mSendState.SetResourceType(ResourceType::kAdditional);
+        if (!query.IsAnnounceBroadcast())
+        {
+            // Initial service broadcast should keep adding data as 'Answers' rather
+            // than addtional data (https://datatracker.ietf.org/doc/html/rfc6762#section-8.3)
+            mSendState.SetResourceType(ResourceType::kAdditional);
+        }
 
         QueryReplyFilter queryReplyFilter(query);
 
@@ -232,6 +245,45 @@ CHIP_ERROR ResponseSender::PrepareNewReplyPacket()
     return CHIP_NO_ERROR;
 }
 
+bool ResponseSender::ShouldSend(const Responder & responder) const
+{
+    switch (responder.GetQType())
+    {
+    case QType::A:
+        return !mSendState.GetWasSent(ResponseItemsSent::kIPv4Addresses);
+    case QType::AAAA:
+        return !mSendState.GetWasSent(ResponseItemsSent::kIPv6Addresses);
+    case QType::PTR: {
+        static const QNamePart kDnsSdQueryPath[] = { "_services", "_dns-sd", "_udp", "local" };
+
+        if (responder.GetQName() == FullQName(kDnsSdQueryPath))
+        {
+            return !mSendState.GetWasSent(ResponseItemsSent::kServiceListingData);
+        }
+        break;
+    }
+    default:
+        break;
+    }
+
+    return true;
+}
+
+void ResponseSender::ResponsesAdded(const Responder & responder)
+{
+    switch (responder.GetQType())
+    {
+    case QType::A:
+        mSendState.MarkWasSent(ResponseItemsSent::kIPv4Addresses);
+        break;
+    case QType::AAAA:
+        mSendState.MarkWasSent(ResponseItemsSent::kIPv6Addresses);
+        break;
+    default:
+        break;
+    }
+}
+
 void ResponseSender::AddResponse(const ResourceRecord & record)
 {
     ReturnOnFailure(mSendState.GetError());

src/lib/dnssd/minimal_mdns/ResponseSender.h

@@ -47,6 +47,24 @@ namespace Minimal {
 
 namespace Internal {
 
+// Flags for keeping track of items having been sent as DNSSD responses
+//
+// We rely on knowing Matter DNSSD only sends the same set of data
+// for some instances like A/AAAA always being the same.
+//
+enum class ResponseItemsSent : uint8_t
+{
+    // DNSSD may have different servers referenced by IP addresses,
+    // however we know the matter dnssd server name is fixed and
+    // the same even across SRV records.
+    kIPv4Addresses = 0x01,
+    kIPv6Addresses = 0x02,
+
+    // Boot time advertisement filters. We allow multiple of these
+    // however we also allow filtering them out at response start
+    kServiceListingData = 0x04,
+};
+
 /// Represents the internal state for sending a currently active request
 class ResponseSendingState
 {
@@ -60,6 +78,7 @@ class ResponseSendingState
         mSource       = packet;
         mSendError    = CHIP_NO_ERROR;
         mResourceType = ResourceType::kAnswer;
+        mSentItems.ClearAll();
     }
 
     void SetResourceType(ResourceType resourceType) { mResourceType = resourceType; }
@@ -88,12 +107,16 @@ class ResponseSendingState
     const chip::Inet::IPAddress & GetSourceAddress() const { return mSource->SrcAddress; }
     chip::Inet::InterfaceId GetSourceInterfaceId() const { return mSource->Interface; }
 
+    bool GetWasSent(ResponseItemsSent item) const { return mSentItems.Has(item); }
+    void MarkWasSent(ResponseItemsSent item) { mSentItems.Set(item); }
+
 private:
     const QueryData * mQuery                 = nullptr;               // query being replied to
     const chip::Inet::IPPacketInfo * mSource = nullptr;               // Where to send the reply (if unicast)
     uint16_t mMessageId                      = 0;                     // message id for the reply
     ResourceType mResourceType               = ResourceType::kAnswer; // what is being sent right now
     CHIP_ERROR mSendError                    = CHIP_NO_ERROR;
+    chip::BitFlags<ResponseItemsSent> mSentItems;
 };
 
 } // namespace Internal
@@ -117,6 +140,8 @@ class ResponseSender : public ResponderDelegate
 
     // Implementation of ResponderDelegate
     void AddResponse(const ResourceRecord & record) override;
+    bool ShouldSend(const Responder &) const override;
+    void ResponsesAdded(const Responder &) override;
 
     void SetServer(ServerBase * server) { mServer = server; }
 

src/lib/dnssd/minimal_mdns/responders/IP.cpp

@@ -30,6 +30,11 @@ void IPv4Responder::AddAllResponses(const chip::Inet::IPPacketInfo * source, Res
                                     const ResponseConfiguration & configuration)
 {
 #if INET_CONFIG_ENABLE_IPV4
+    if (!delegate->ShouldSend(*this))
+    {
+        return;
+    }
+
     chip::Inet::IPAddress addr;
     UniquePtr<IpAddressIterator> ips =
         GetAddressPolicy()->GetIpAddressesForEndpoint(source->Interface, chip::Inet::IPAddressType::kIPv4);
@@ -46,12 +51,18 @@ void IPv4Responder::AddAllResponses(const chip::Inet::IPPacketInfo * source, Res
         configuration.Adjust(record);
         delegate->AddResponse(record);
     }
+    delegate->ResponsesAdded(*this);
 #endif
 }
 
 void IPv6Responder::AddAllResponses(const chip::Inet::IPPacketInfo * source, ResponderDelegate * delegate,
                                     const ResponseConfiguration & configuration)
 {
+    if (!delegate->ShouldSend(*this))
+    {
+        return;
+    }
+
     chip::Inet::IPAddress addr;
     UniquePtr<IpAddressIterator> ips =
         GetAddressPolicy()->GetIpAddressesForEndpoint(source->Interface, chip::Inet::IPAddressType::kIPv6);
@@ -68,6 +79,7 @@ void IPv6Responder::AddAllResponses(const chip::Inet::IPPacketInfo * source, Res
         configuration.Adjust(record);
         delegate->AddResponse(record);
     }
+    delegate->ResponsesAdded(*this);
 }
 
 } // namespace Minimal

src/lib/dnssd/minimal_mdns/responders/Ptr.h

@@ -31,9 +31,15 @@ class PtrResponder : public RecordResponder
     void AddAllResponses(const chip::Inet::IPPacketInfo * source, ResponderDelegate * delegate,
                          const ResponseConfiguration & configuration) override
     {
+        if (!delegate->ShouldSend(*this))
+        {
+            return;
+        }
+
         PtrResourceRecord record(GetQName(), mTarget);
         configuration.Adjust(record);
         delegate->AddResponse(record);
+        delegate->ResponsesAdded(*this);
     }
 
 private: