[Dnsmasq-discuss] [PATCH] Support Cisco Umbrella/OpenDNS Device ID & Remote IP

Brian Hartvigsen brian.andrew at brianandjenny.com
Wed Apr 7 23:32:10 UTC 2021



> On Apr 7, 2021, at 15:48, Simon Kelley <simon at thekelleys.org.uk> wrote:
> 
> 1) the version field is set to zero, but
> https://docs.umbrella.com/umbrella-api/docs/identifying-dns-traffic says
> it should be one.

Version 0 uses a 1-byte field for the sub option (organization id, device id, asset id).  Version 1 uses an 2-byte (unsigned short) for the sub option.  That's the only difference relevant to this code.  I can update it to use version 1 just to match the current documentation if that is better for inclusion.

> 2) I don't like the umbrella_data[512] declaration. I know it can't
> overflow, but declaring the array to the exact maximum size (and
> defining the calculation for that in a comment) makes it less likely
> that future modifiers of the code will assume they can add stuff without
> checking. I'd go further and declare a struct with the fixed stuff (the
> magic number, flags and version and a char array of the size needed for
> the longest set of sub-options.

I actually had this at first (as shown in the PR on GitHub) and took it out for reasons I can't remember (original code is a couple years old now.)  I was wondering the same thing when I did the patch but really wanted to get it out there.  I'll get to work on converting to a struct since it shouldn't be that much work.

> 3) Why is umbrella_device being converted from a text string to a byte
> array during packet-manipulation? That would surely be better done in
> option.c during option parsing, with some error checking for non-hex
> characters as well.

Not for any good reason, because I didn't think to do that?  I'll work on that too!

> 4) Your code starts each field with a single byte id, for instance 0x04
> for UMBRELLA_ASSET, but
> https://docs.umbrella.com/umbrella-api/docs/identifying-dns-traffic says
> it should be two bytes, 0x00, 0x04

Same as #1

> 5) You are modifying queries with per-client data (addresses) so you
> need to set cacheable to zero in add_edns0_config() so that data which
> is valid for only one client doesn't get returned to another client from
> the cache.
> 
> 6) Consider using the PUTLONG and PUTSHORT macros instead of memcpy()
> calls, to match the rest of the code.

Will correct both of these, thank you!  I really appreciate your feedback on this.  One question, for purposes of submitting additional patches, is it okay to do a patch that applies on top of the current patch or is it preferred to send a patch that has all of my changes in it?

-- Brian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20210407/e693e73d/attachment.sig>


More information about the Dnsmasq-discuss mailing list