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

Simon Kelley simon at thekelleys.org.uk
Fri Apr 9 16:32:10 UTC 2021


On 08/04/2021 09:52, john doe wrote:
> 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.


I concur. A single patch for a single functional charge, so I can fulfil
my primary role of understanding exactly what is being changed. And one
which applies to the current bleeding edge, because that's where I'll be
applying it.


Simon.




More information about the Dnsmasq-discuss mailing list