Canonicalize IPv4 addresses #27
Conversation
There was a problem hiding this comment.
Hi @danog , could you explain why do you want this change? Can you provide any example go code using this library which behaves incorrectly?
My main concern regarding this PR is about backward incompatible change which potentially can break something in other projects using this open source library. So, even if you want different behavior in your project, you should propose a backward compatible change (maybe with a config option), unless you can prove the current behavior is incorrect for any project, which I'm not convinced for now.
|
|
||
| func iPv4NetToUint32(n *net.IPNet) (uint32, int) { | ||
| if len(n.IP) != net.IPv4len { | ||
| ip := n.IP.To4() |
There was a problem hiding this comment.
normally, IPNet should have both IP and Mask fields of the same size - either 4 or 16 bytes. It you successfully convert 16-bytes IP to 4-bytes here, the Mask will still remain 16 bytes. Which will return 0, -1 later.
There was a problem hiding this comment.
It is super easy to pass a 16-byte address and a 4-byte mask (as specified in the tests, when using the default IPv4s exposed by the STL), it is not a user error to do so, the IP struct explicitly has a To4 method to normalize an IPv4.
The IPMask, however, does not support IPv4 masks contained in 16-byte arrays, as is obvious by the fact that it has no associated To4 method.
There was a problem hiding this comment.
Well, it's super easy in general to craft incorrect IPNets, e.g. 5-bytes IP and 7-bytes Mask, or just regular (not convertible to IPv4) 16-bytes IP and 4-bytes mask. The question is why do you pass such odd IPNets as input parameters?
In you external code, just make sure that the size of IP and Mask is the same. As you mentioned in another PR (in coredns project) ParseCIDR() returns correct IPNets in all cases, i.e. IP and Mask are of the same size.
If you create the IPNets arguments by your own, the sizes of these two fields are also under your control. For example, you may want to convert IP to 4-bytes (if possible) and then initialize the IPNet with 4-bytes IP and 4-bytes Mask
The IPMask, however, does not support IPv4 masks contained in 16-byte arrays, as is obvious by the fact that it has no associated To4 method.
It is possible to create 16-bytes mask, as I mentioned in my comment to your test change. Mask is just a mask, it is a sequence of 1 bits followed by sequence of 0 bits. Each bit in mask corresponds to one bit in IP. Method like To4() doesn't make sense for mask.
Alternatively, this lib provides public methods taking IP as parameter. Such methods convert IPv4 from 16-bytes to 4-bytes.
There was a problem hiding this comment.
The question is why do you pass such odd IPNets as input parameters?
Because something as trivial as net.IPNet{IP: net.IPv4zero, Mask: iPv4MaxMask} isn't immediately obvious to be wrong.
If you take net.IPv4zero, which is a public STL constant representing an IPv4, you would expect it to contain an IPv4, but the library treats it as an IPv6 address, because it doesn't account for 16-byte IPv4 addresses due to this quirk in the STL.
| } | ||
|
|
||
| func TestIPv4NetToUint32(t *testing.T) { | ||
| key, bits := iPv4NetToUint32(&net.IPNet{IP: net.IPv4zero, Mask: iPv4MaxMask}) |
There was a problem hiding this comment.
Here you create IPNet with 16-bytes IP, and 4-bytes Mask, which is not typical and error prone case.
If you try &net.IPNet{IP: net.IPv4(192, 168, 1, 0), Mask: net.CIDRMask(120, 128)} (i.e. both IP and Mask are 16-bytes), your change will fail
There was a problem hiding this comment.
The 16-byte IPv4 mask case is explicitly incorrect, an explicit user error.
The 16-byte IPv4 case, on the other hand, is expected behavior, and is part of the STL API.
|
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). |
There was a problem hiding this comment.
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).
I believe, in all cases when this library takes IP argument, it processes the IP correctly. Passing IPNet is not the same as passing IP. Those methods assume the IPNet being passed is constructed correctly, i.e. the fields IP and Mask are both 16 or 4 bytes.
|
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 |
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 As for |
|
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. |
2 participants
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
netpackage).