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

john doe johndoe65534 at mail.com
Thu Apr 8 08:52:25 UTC 2021


On 4/8/2021 1:32 AM, Brian Hartvigsen wrote:
>
>
>> 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?
>

The below is under the assumption that everything that is sent can be
applied cleanly at the tip of the desired branch.

It is preferable to send a 'reroll-count'.
As all of this is code 'fixup', I would say that one patch is desirable! :)


Note that I'm not a project maintainer, so that might not be what Simon
desire.

--
John Doe



More information about the Dnsmasq-discuss mailing list