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