[squid-dev] [PATCH] IPv6 NDP lookups

[Amos Jeffries]

On 22/12/2016 1:42 a.m., Steve Hill wrote:
> 
> The attached patch is against Squid 3.5.  Given Squid 3.5's status as a
> stable release, this probably won't be integrated into the vanilla Squid
> release, but I'm posting here in case anyone finds it useful, or if
> someone wants to port it to Squid 4.
> 

Yay. Thank you.

> 
> Squid currently supports ACLs and logformat specifiers which rely on the
> EUI-48 (MAC address) for IPv4 traffic, and EUI-64 for IPv6 traffic.
> 
> For IPv4, Squid queries the ARP cache for the client's address.  For
> IPv6, Squid extracts the EUI-64 from site-local SLAAC addresses.  This
> isn't going to work for most clients, since site-local addresses are
> rarely used in the real world.  This patch brings the IPv6 functionality
> in line with the IPv4 functionality by querying the neighbour table
> using rtnetlink.
> 
> Open question: we could also pull the EUI-64 from a global scope SLAAC
> address.  Would it be trustworthy enough?  Is it worth doing?  Since
> most clients now use privacy extensions it's probably not worthwhile.
> 

I considered it at the time. But so far it has not been worth doing. At
least nobody has mentioned wanting to do it over the global spaces.


> 
> Notes:
> 
> - We have to examine the entire neighbour table since (as far as I can
> tell) the kernel doesn't allow querying a specific IP address.  This
> could be slow if there are a lot of neighbours.
> 
> - The IPv4 neighbour table can be retrieved in the same way, so there is
> scope for unifying the IPv6 and IPv4 code.
> 
> - The neighbour table contains MAC addresses (i.e. EUI-48), not EUI-64
> addresses.  This patch converts the retrieved EUI-48 into an EUI-64 by
> inserting 0xfffe into the middle.
> 
> - If the IPv4/IPv6 code is to be unified in the future, consider
> converting everything to EUI-64 instead of making a distinction between
> EUI-48 and EUI-64.
> 
> - This code is useful where users are being authenticated through a
> mechanism other than HTTP proxy auth.  For example, a client can
> identify itself through a captive portal, but then use a combination of
> IPv4 and numerous IPv6 addresses (due to privacy extensions) thereafter.
>  The client can be linked back to their portal login through their MAC,
> irrespective of the IP address they are using for any given request.
> 
> - Obviously the client needs to be on the same layer 2 network as Squid,
> so this doesn't help in situations where clients are behind a router.
> 


Some design things:

* do we really have to open a new rtnetlink_sock on each lookup? or can
we keep that connection open and send multiple requests over it?
 - if we can keep it open, use a static function to do the opening and
create a Runner + comm close handler for closing it.
  + the test 'if (rtnetlink_sock < 0)' should return false immediately.
    that will remove one level of indent.

 - please call fd_note(rtnetlink_sock, "NDP lookup socket") after
opening it to record what the socket FD is/was used for.


* are these netlink structs and macros generically available on all
Linux systems? I suspect not,
 - the code blocks wrapper #if condition may need to include the
HAVE_FOO_H macros relevant for the netlink .h file(s) required.


* for the 'req' structure, is there an rtnetlink header defined type we
can use instead of defining our own 'custom' packet structure?


* the buf array can be made a static to reduce stack allocations
 - why is buf 16K? please document what decision was made to get that
number. arbitrary value is fine, but we need to have a record in case
something changes years from now.


* both variables found_ip and success are being used to mean the success
case.
 - found_ip can be reset to false after that inner for loop if !mac_ptr.
That will leave it with identical value to success in all cases after
the while-loop finishes.


And some code polish:

* please replace all '(! done)' with just '!done'

* you have added a comment documenting what the while is looping over,
please do the same for the two internal for loops.

* use true/false keywords to initialize the bool variables instead of
0/1 constants.

* please move variables from the top of the code block down to their
scope or time of first use. This is C++.

* also please use C++ casting if it cannot be dropped entirely.

* the debugs() at the end of the code block should be printed in all
cases, just a with a (success? "found":"fail") dynamic line portion to
say what the result was.


NP: whoever applies this patch will need to run the maintenance script
first to cleanup a bunch of line wrappings and other whitespace stuff.

Amos