Overhaul mDNS to allow arbitrary records (supports DNS Service Discovery)

[george-palmsens]

This changes the DNS hook to return a list of records to serve over mDNS. By setting an appropriate set of records, this satisfies DNS-SD (Avahi, Bonjour, etc).

An "appropriate set of records" looks like:

static DNSRecord_t s_records[] = {
    {.usRecordType = dnsTYPE_PTR,
     .pcName       = "_services._dns-sd._udp.local",
     .xData.pcPtrRecord  = "_myservice._udp.local"},

    {.usRecordType = dnsTYPE_PTR,
     .pcName       = "_myservice._udp.local",
     .xData.pcPtrRecord  = "myhostname._myservice._udp.local"},

    {.usRecordType = dnsTYPE_SRV,
     .pcName       = "myhostname._myservice._udp.local",
     .xData.xSrvRecord   = {.pcTarget = "myhostname.local", .usPort = SERVICE_PORT}},

    {.usRecordType = dnsTYPE_TXT,
     .pcName = "myhostname._myservice._udp.local",
     .xData.pcTxtRecord = "Arbitrary service text"},
     
    {.usRecordType = dnsTYPE_A_HOST,
     .pcName = "myhostname.local"},
};

Other things that have been changed to accomodate this:

• DNS responses now check all questions, not just the first one
• DNS responses may contain many answers

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

I have tested my changes locally, and verified the device is discoverable through Avahi. I have not run an application level test suite.

I have added a shim that allows the old hooks to still work via a back-compatibility flag, I hope that is a fine approach.

I have added the necessary unit tests that reach 100% coverage. A few isolated cases had to be ignored from the coverage report since they are defensive checks that are already caught earlier in the logic.

Requests for assistance:

• How do I document this feature?

Related Issue

See #1284

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[george-palmsens]

Apologies for spamming the CI.

I have added a back compatibility shim for the old style DNS hooks. Defining ipconfigDNSQuery_BACKWARD_COMPATIBLE should hopefully mean that old code continues to work.

[george-palmsens]

Added support for "Additional RRs" in the generated response, with a corresponding hook.

This is a SHOULD in the DNS-SD spec, and clients aren't allowed to rely on it. But they still do 🤷 and so this is necessary in order to get everything working reliably with a variety of client libraries.

[htibosch]

Hi @george-palmsens , you're doing a great job. Please allow me time to review and test the changes, most importantly: keeping thing downward compatible.
Thanks so far!

[david-sawatzke]

@george-palmsens Thank you for the implementation, just tested it and found it to work great!

For documentation: Some example code with the hooks & usage of SERVE_ADDITIONAL in the MatchedHook as well documenting expected "reasonable" config options, such as increasing the ipconfigDNS_CACHE_NAME_LENGTH to the mDNS-SD query length would've made it easier for me. But I'm also unsure how this is done for freertos+tcp.

Regarding the TXT records: Currently only one printable key/value pair per TXT record is supported.

The current approach works fine for me, but the specification generally expects one TXT records for most cases (6.8.) and putting multiple key/value pairs inside the one (and recommends adding txtvers as the first key/value pair). Furthermore the value in the key value pairs can be arbitrary binary data (including \0), although this seems to be very uncommon in practice. As far as I can tell, Avahi is limited to one record.

I've thought about this for a bit, but I don't know how a nice API for this would look. Perhaps something like this for the "full" approach?

struct xDNSTXTRecordEntry {
  char *pcKey;
  void *pvValue;    // No value if nullptr (i.e. no '=' inserted), empty value if value_length == 0. nullptr takes precendence
  uint8_t ucValueLength;  // Performs strlen on value if e.g. 0xFF, an otherwise invalid value?
}

#define dnsTXTRECORDENTRY_EMPTYVALUE 0
#define dnsTXTRECORDENTRY_PRINTABLEVALUE 0xFF

#define xDNSTXTRecordPrintableEntry(KEY, VALUE) {KEY, VALUE, dnsTXTRECORDENTRY_PRINTABLEVALUE}

// ...

// DNS Record Union

union
            {
                char * pcPtrRecord;
                struct
                {
                    const char * pcTarget;
                    uint16_t usPort;
                } xSrvRecord;
                struct 
                {
                    xDNSTXTRecordEntry *pxEntries;
                    size_t uxEntryCount;
                }
            } xData;

Alternatively when ignoring binary data, I see two other options:

• Keep with one string and use a magic value prefixed for each key/value pair: "a=1\001b=2\001c=3" as laid out here. Would make it less convenient to update values, but builds on the trivial case currently supported.
• Uses a {char *key, char *value} struct array, which saves a byte & the "magic" value, as done by the ESP-IDF, misses empty value/no value differentiation.

[george-palmsens]

@david-sawatzke Thanks for test-driving the code, great to hear it works!

Where should documentation be added? Just in the relevant headers, or is there an example/website docs location I should be putting it?

You're definitely right about TXT records. I'll need to get my head back into the code and look at your suggestions and I'll get back to you 👍

[george-palmsens]

@david-sawatzke So the key/value pair thing is DNS-SD specific - this PR doesn't specifically care about DNS-SD, so I'd feel uncomfortable adding them into the actual structure of the code.

So I would aim for making the text record simply contain an arbitrary number of string entries. And if you want to use those for key=value then you can.

As such I think I would just alter the struct to take a pointer to an array of strings, rather than a single entry. So it would look like:

typedef struct xDNSRecord
{
    uint16_t usRecordType;

    /* How to serve this record.
     * See `dnsRECORD_SERVE_NO`, `dnsRECORD_SERVE_ADDITIONAL` and `dnsRECORD_SERVE_ANSWER`. */
    BaseType_t uxServeRecord;
    const char * pcName;
    union
    {
        char * pcPtrRecord;
        struct
        {
            const char * pcTarget;
            uint16_t usPort;
        } xSrvRecord;
        struct
        {
            const char * const * pcStrings;
            UBaseType_t uxNumStrings;
        } xTxtRecord;
    } xData;
} DNSRecord_t;

This would still stop us from serving strings with null bytes, but it sounds like we can tolerate that...?

[david-sawatzke]

Where should documentation be added? Just in the relevant headers, or is there an example/website docs location I should be putting it?

I don't know, but I'd guess it should be in the headers anyways so there first?

This would still stop us from serving strings with null bytes, but it sounds like we can tolerate that...?

At a quick look I haven't found a mDNS-SD service type that requires it, so I think it's fine.

So I would aim for making the text record simply contain an arbitrary number of string entries. And if you want to use those for key=value then you can.

This approach sounds good to me, a bit more annoying in the mDNS-SD usecase (as the string needs to be mutated to change the value if the value is already stored as a string), but also supports encodings outside RFC1464

[george-palmsens]

This approach sounds good to me, a bit more annoying in the mDNS-SD usecase (as the string needs to be mutated to change the value if the value is already stored as a string), but also supports encodings outside RFC1464

I haven't established how any of this works if you want to change the records being served dynamically.

I don't think the current API supports it - because at any given moment the IP task could be iterating over the records. You would need to do some sort of deep copy in the hook, which seems very impractical.

EDIT:

We could add an additional hook for when the DNS code is finished using the records. That would give an acquire/release pair that the user could work with to update the DNS records from another thread.

That would compose nicely with what we have now.

[david-sawatzke]

I don't think the current API supports it - because at any given moment the IP task could be iterating over the records. You would need to do some sort of deep copy in the hook, which seems very impractical.

Ah, you're right, I've misunderstood. I thought xApplicationDNSRecordQueryHook would be called every time. But I don't think a deep copy is required if it's only mutated in the callback, as that's all in the IP task afaict?

As a hacky approach, one could also update the contents of the records in xApplicationDNSRecordsMatchedHook, which mutatest it anyways.

Then I've probably also misunderstood xApplicationDNSRecordsMatchedHook or there's a bug, as uxServeRecord only gets reset if xApplicationDNSRecordQueryHook is called, so xApplicationDNSRecordQueryHook needs to reset uxServeRecord as well if they should only be sent out for a specific answer?

I think the best option could be to call xApplicationDNSRecordQueryHook on every request? in the trivial case, this is almost free, in the non-trivial case this is probably the best place to do it (otherwise the information might be quite out of date).

EDIT: as xSet is always cleared on every ParseDNSReply, so xApplicationsDNSRecordQueryHook already gets called on every request and I works how I thought it should work before?

[george-palmsens]

Ah, you're right, I've misunderstood. I thought xApplicationDNSRecordQueryHook would be called every time. But I don't think a deep copy is required if it's only mutated in the callback, as that's all in the IP task afaict?

Yeah it is called every request - but then the IP task iterates through the returned records.

And yeah if you do in-place mutation in the callback then you will be fine 👍 . That is a workable solution.

[david-sawatzke]

Will xSet->pxDNSRecords in DNS_GetRecords ever not be NULL? Even if yes, should it just be overwritten anyways?

[george-palmsens]

It is not NULL in the case where there are multiple questions. GetRecords is called in the question loop.

[george-palmsens]

@david-sawatzke Multiple strings are now present, and I've also generally tidied and improved the implementation, and added example code.

❯ avahi-browse _myservice._udp -rt                                                                                                                    FreeRTOS-Plus-TCP
+ enp34s0u2u4 IPv4 instance                                      _myservice._udp      local
= enp34s0u2u4 IPv4 instance                                      _myservice._udp      local
   hostname = [myhostname.local]
   address = [192.168.0.137]
   port = [83]
   txt = ["name=FreeRTOS" "example=DNS-SD"]

Note to reviewers, coverage is <100% in the DNS code but this is also true on main. It is not caused by this PR.

[david-sawatzke]

Tested it, also works great for me. Thank you @george-palmsens for the changes & cleanup, and the example in the header!