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

Simon Kelley simon at thekelleys.org.uk
Wed Apr 7 21:48:32 UTC 2021


On 07/04/2021 03:45, Brian Hartvigsen wrote:
> This is based on the information at https://docs.umbrella.com/umbrella-api/docs/identifying-dns-traffic and https://docs.umbrella.com/umbrella-api/docs/identifying-dns-traffic2 . Using --umbrella by itself will enable Remote IP reporting. This can not be used for any policy filtering in Cisco Umbrella/OpenDNS. Additional information can be supplied using specific option specifications, multiple can be separated by a comma:
> 
> --umbrella=orgid:1234,deviceid=0123456789abcdef
> 
> Specifies that you want to report organization 1234 using device 0123456789abcdef. For Cisco Umbrella Enterprise, see "Register (Create) a device" (https://docs.umbrella.com/umbrella-api/docs/create-a-device) for how to get a Device ID and "Organization ID endpoint" (https://docs.umbrella.com/umbrella-api/docs/organization-endpoint) to get organizations ID. For OpenDNS Home Users, there is no organization, see Registration API endpoint (https://docs.umbrella.com/umbrella-api/docs/registration-api-endpoint2) for how to get a Device ID. Asset ID should be ignored unless specifically instructed to use by support.


I think that this is fine to go in in principle.

I have some queries about the actual code, as follows:

1) the version field is set to zero, but
https://docs.umbrella.com/umbrella-api/docs/identifying-dns-traffic says
it should be one.

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.

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.

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

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.



Cheers,

Simon.




More information about the Dnsmasq-discuss mailing list