go-trees
go-trees copied to clipboard
infobloxopen
Canonicalize IPv4 addresses
This fixes an issue where non-canonicalized IPv4 addresses could not be used in trees (16-byte arrays containing an ipv4 address, the default format for all IPv4 constants from the net package).
The current behavior unfortunately is affected by one of the nastiest case of misdesigned APIs I've ever seen in the go stdlib: certain IPv4 addresses may be represented as an array of 16 bytes, making comparison against IPv4len completely useless; in fact, all of the IPv4 addresses exposed by the STL like IPv4Zero are in this 16-byte format.
It is extremely easy to misuse this library by passing IPv4 addresses in this format, introducing subtle and hard to debug bugs (wasted around an hour while adding ECS support to coredns due to this precise issue).
An IPNet with a 16-byte IPv4 and a 4-byte mask is still a correctly constructed IPNet, at least according to the current API of the STL.
An IPNet with a 16-byte IPv4 and a 4-byte mask is still a correctly constructed IPNet, at least according to the current API of the STL.
Can you provide a reference to a doc, or give a code example?
https://cs.opensource.google/go/go/+/master:src/net/ip.go;l=457; the STL is clearly expecting to find 16-byte IPv4 addresses in a net.IPNet, as To4 conversion is used for IPs in IPNet.
I know IPNet.String() can handle 16-bytes IP (convertible to IPv4) with 4-bytes Mask. But I still think this is not a normal IPNet, and the String() rather workarounds incorrectly built IPNets (just my opinion).
workarounds incorrectly built IPNets (just my opinion).
That is precisely the point, this part of the STL is badly designed, and it is very easy for users to misuse it, and even easier to do so when using this library in particular, because due to the lack of error reporting when InsertNet is passed a 16-byte IPv4 and 4-byte mask, it just silently ignores the passed IPs, instead of returning an error or panicking.
Well, as for net package, it is a know problem. Recently, the package netip was released which solves some problems of net.IP. But this new package is not widely used yet.
As for iptree lib, the only enhancement which looks reasonable to me, would be adding checks for input IPNet parameters and returning error in case of IP and Mask mismatch. Though, this is also risky, and may break some existing applications. If you really want this check to be implemented, this should be configurable
I agree that the method should at least return an error, but I disagree that the behavior should be configurable: if the issue is that it would be a breaking change, just tag a new major.
I can submit a pull request that adds proper error reporting, as long as it gets merged.