From 2664331529e1e574fbd17e9aa6aaf641b7544792 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 19 Jun 2023 11:32:19 -0400 Subject: [PATCH] MinMDNS: reduce amount of data in the mdns advertising packets (and make 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 Co-authored-by: Restyled.io --- examples/minimal-mdns/advertiser.cpp | 116 ++++++++++++------ src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp | 3 +- src/lib/dnssd/minimal_mdns/Parser.h | 6 +- src/lib/dnssd/minimal_mdns/QueryReplyFilter.h | 2 +- src/lib/dnssd/minimal_mdns/ResponseSender.cpp | 54 +++++++- src/lib/dnssd/minimal_mdns/ResponseSender.h | 25 ++++ src/lib/dnssd/minimal_mdns/responders/IP.cpp | 12 ++ src/lib/dnssd/minimal_mdns/responders/Ptr.h | 6 + .../responders/QueryResponder.cpp | 7 ++ .../dnssd/minimal_mdns/responders/Responder.h | 33 +++-- src/lib/dnssd/minimal_mdns/responders/Srv.h | 6 + src/lib/dnssd/minimal_mdns/responders/Txt.h | 6 + 12 files changed, 220 insertions(+), 56 deletions(-) diff --git a/examples/minimal-mdns/advertiser.cpp b/examples/minimal-mdns/advertiser.cpp index 0194a3d7fb4c9a..e9cc880cae81ed 100644 --- a/examples/minimal-mdns/advertiser.cpp +++ b/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 \n" - " --advertising-mode \n" - " Advertise in this mode (operational or commissionable-node or commissioner).\n" - " --short-discriminator \n" - " -s \n" - " Commissioning/commissionable short discriminator.\n" - " --long-discriminator \n" - " -l \n" - " Commissioning/commissionable long discriminator.\n" - " --vendor-id \n" - " Commissioning/commissionable vendor id.\n" - " --product-id \n" - " -p \n" - " Commissioning/commissionable product id.\n" - " --commissioning-mode \n" - " Commissioning Mode (0=disabled, 1=basic, 2=enhanced).\n" - " --device-type \n" - " Device type id.\n" - " --device-name \n" - " Name of device.\n" - " --rotating-id \n" - " Rotating Id.\n" - " --pairing-instruction \n" - " Commissionable pairing instruction.\n" - " --pairing-hint \n" - " Commissionable pairing hint.\n" - " --fabric-id \n" - " -f \n" - " Operational fabric id.\n" - " --node-id \n" - " -n \n" - " Operational node id.\n" - " -t \n" - " --trace-to \n" - " trace to the given destination (supported: " SUPPORTED_COMMAND_LINE_TRACING_TARGETS ").\n" - "\n" }; + " -m \n" + " --advertising-mode \n" + " Advertise in this mode (operational, operational-multi-admin, commissionable-node or commissioner).\n" + " --short-discriminator \n" + " -s \n" + " Commissioning/commissionable short discriminator.\n" + " --long-discriminator \n" + " -l \n" + " Commissioning/commissionable long discriminator.\n" + " --vendor-id \n" + " Commissioning/commissionable vendor id.\n" + " --product-id \n" + " -p \n" + " Commissioning/commissionable product id.\n" + " --commissioning-mode \n" + " Commissioning Mode (0=disabled, 1=basic, 2=enhanced).\n" + " --device-type \n" + " Device type id.\n" + " --device-name \n" + " Name of device.\n" + " --rotating-id \n" + " Rotating Id.\n" + " --pairing-instruction \n" + " Commissionable pairing instruction.\n" + " --pairing-hint \n" + " Commissionable pairing hint.\n" + " --fabric-id \n" + " -f \n" + " Operational fabric id.\n" + " --node-id \n" + " -n \n" + " Operational node id.\n" + " -t \n" + " --trace-to \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"); diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp index 33f037587858b5..c3817ce4c3811a 100644 --- a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp +++ b/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()); diff --git a/src/lib/dnssd/minimal_mdns/Parser.h b/src/lib/dnssd/minimal_mdns/Parser.h index 509633b9e192fa..879c6858a2912f 100644 --- a/src/lib/dnssd/minimal_mdns/Parser.h +++ b/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 diff --git a/src/lib/dnssd/minimal_mdns/QueryReplyFilter.h b/src/lib/dnssd/minimal_mdns/QueryReplyFilter.h index c7eb0a98af0151..a9cf13842d3527 100644 --- a/src/lib/dnssd/minimal_mdns/QueryReplyFilter.h +++ b/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; } diff --git a/src/lib/dnssd/minimal_mdns/ResponseSender.cpp b/src/lib/dnssd/minimal_mdns/ResponseSender.cpp index 722ca973902fb3..7aec6142f0b555 100644 --- a/src/lib/dnssd/minimal_mdns/ResponseSender.cpp +++ b/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()); diff --git a/src/lib/dnssd/minimal_mdns/ResponseSender.h b/src/lib/dnssd/minimal_mdns/ResponseSender.h index 636acaaf7acf14..487b3b5b104f81 100644 --- a/src/lib/dnssd/minimal_mdns/ResponseSender.h +++ b/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 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; } diff --git a/src/lib/dnssd/minimal_mdns/responders/IP.cpp b/src/lib/dnssd/minimal_mdns/responders/IP.cpp index 8290db94e0aa40..80e57e15223737 100644 --- a/src/lib/dnssd/minimal_mdns/responders/IP.cpp +++ b/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 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 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 diff --git a/src/lib/dnssd/minimal_mdns/responders/Ptr.h b/src/lib/dnssd/minimal_mdns/responders/Ptr.h index 1fd50a0b5e142c..80e91e2349b16e 100644 --- a/src/lib/dnssd/minimal_mdns/responders/Ptr.h +++ b/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: diff --git a/src/lib/dnssd/minimal_mdns/responders/QueryResponder.cpp b/src/lib/dnssd/minimal_mdns/responders/QueryResponder.cpp index 47731c7a42ddfd..eb0ed5ac065118 100644 --- a/src/lib/dnssd/minimal_mdns/responders/QueryResponder.cpp +++ b/src/lib/dnssd/minimal_mdns/responders/QueryResponder.cpp @@ -142,6 +142,11 @@ void QueryResponderBase::MarkAdditionalRepliesFor(QueryResponderIterator it) void QueryResponderBase::AddAllResponses(const chip::Inet::IPPacketInfo * source, ResponderDelegate * delegate, const ResponseConfiguration & configuration) { + if (!delegate->ShouldSend(*this)) + { + return; + } + // reply to dns-sd service list request for (size_t i = 0; i < mResponderInfoSize; i++) { @@ -159,6 +164,8 @@ void QueryResponderBase::AddAllResponses(const chip::Inet::IPPacketInfo * source configuration.Adjust(record); delegate->AddResponse(record); } + + delegate->ResponsesAdded(*this); } void QueryResponderBase::ClearBroadcastThrottle() diff --git a/src/lib/dnssd/minimal_mdns/responders/Responder.h b/src/lib/dnssd/minimal_mdns/responders/Responder.h index 4a1b4e015dbe59..534b661df64535 100644 --- a/src/lib/dnssd/minimal_mdns/responders/Responder.h +++ b/src/lib/dnssd/minimal_mdns/responders/Responder.h @@ -26,15 +26,6 @@ namespace mdns { namespace Minimal { -class ResponderDelegate -{ -public: - virtual ~ResponderDelegate() {} - - /// Add the specified resource record to the response - virtual void AddResponse(const ResourceRecord & record) = 0; -}; - /// Controls specific options for responding to mDNS queries /// class ResponseConfiguration @@ -67,6 +58,9 @@ class ResponseConfiguration chip::Optional mTtlSecondsOverride; }; +// Delegates that responders can write themselves to +class ResponderDelegate; + /// Adds ability to respond with specific types of data class Responder { @@ -81,7 +75,7 @@ class Responder /// Domain name is generally just 'local' FullQName GetQName() const { return mQName; } - /// Report all reponses maintained by this responder + /// Report all responses maintained by this responder /// /// Responses are associated with the objects type/class/qname. virtual void AddAllResponses(const chip::Inet::IPPacketInfo * source, ResponderDelegate * delegate, @@ -92,5 +86,24 @@ class Responder const FullQName mQName; }; +class ResponderDelegate +{ +public: + virtual ~ResponderDelegate() {} + + /// Add the specified resource record to the response + virtual void AddResponse(const ResourceRecord & record) = 0; + + /// Accept to add responses for the particular responder. + /// + /// This will be called before responders serialize their records. + virtual bool ShouldSend(const Responder &) const { return true; } + + /// Called when all responses were added for a particular responder + /// + /// Only called if a previous accept returned true. + virtual void ResponsesAdded(const Responder &) {} +}; + } // namespace Minimal } // namespace mdns diff --git a/src/lib/dnssd/minimal_mdns/responders/Srv.h b/src/lib/dnssd/minimal_mdns/responders/Srv.h index d23226092b3e09..868bf878302aaf 100644 --- a/src/lib/dnssd/minimal_mdns/responders/Srv.h +++ b/src/lib/dnssd/minimal_mdns/responders/Srv.h @@ -31,9 +31,15 @@ class SrvResponder : public RecordResponder void AddAllResponses(const chip::Inet::IPPacketInfo * source, ResponderDelegate * delegate, const ResponseConfiguration & configuration) override { + if (!delegate->ShouldSend(*this)) + { + return; + } + SrvResourceRecord record = mRecord; configuration.Adjust(record); delegate->AddResponse(record); + delegate->ResponsesAdded(*this); } private: diff --git a/src/lib/dnssd/minimal_mdns/responders/Txt.h b/src/lib/dnssd/minimal_mdns/responders/Txt.h index 5b607265b8c55c..b2a920ab96c0fd 100644 --- a/src/lib/dnssd/minimal_mdns/responders/Txt.h +++ b/src/lib/dnssd/minimal_mdns/responders/Txt.h @@ -31,9 +31,15 @@ class TxtResponder : public RecordResponder void AddAllResponses(const chip::Inet::IPPacketInfo * source, ResponderDelegate * delegate, const ResponseConfiguration & configuration) override { + if (!delegate->ShouldSend(*this)) + { + return; + } + TxtResourceRecord record = mRecord; configuration.Adjust(record); delegate->AddResponse(record); + delegate->ResponsesAdded(*this); } private: