[squid-dev] [PATCH] IPv6 NDP lookups
squid3 at treenet.co.nz
Fri Dec 23 05:49:08 UTC 2016
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.
> - 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
- 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
* 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.
More information about the squid-dev