Skip to content

Canonicalize IPv4 addresses #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Canonicalize IPv4 addresses #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93afea8
Canonicalize IPv4 addresses
danog Dec 24, 2024
d9775fe
Add tests
danog Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions iptree/iptree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,8 @@ func (t *Tree) enumerate(ch chan Pair) {
}

func iPv4NetToUint32(n *net.IPNet) (uint32, int) {
if len(n.IP) != net.IPv4len {
ip := n.IP.To4()
Copy link
Contributor

@rdrozhdzh rdrozhdzh Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@danog danog Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@rdrozhdzh rdrozhdzh Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@danog danog Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ip == nil {
return 0, -1
}

Expand All @@ -370,7 +371,7 @@ func iPv4NetToUint32(n *net.IPNet) (uint32, int) {
return 0, -1
}

return packIPToUint32(n.IP), ones
return packIPToUint32(ip), ones
}

func packIPToUint32(x net.IP) uint32 {
Expand Down
7 changes: 6 additions & 1 deletion iptree/iptree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,8 +818,13 @@ func TestTreeByIP(t *testing.T) {
}

func TestIPv4NetToUint32(t *testing.T) {
key, bits := iPv4NetToUint32(&net.IPNet{IP: net.IPv4zero, Mask: iPv4MaxMask})
Copy link
Contributor

@rdrozhdzh rdrozhdzh Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@danog danog Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if key != 0 || bits != 32 {
t.Errorf("Expected 0x0, 32 pair but got 0x%08x, %d", key, bits)
}

_, n, _ := net.ParseCIDR("192.0.2.0/24")
key, bits := iPv4NetToUint32(n)
key, bits = iPv4NetToUint32(n)
if key != 0xc0000200 || bits != 24 {
t.Errorf("Expected 0xc0000200, 24 pair but got 0x%08x, %d", key, bits)
}
Expand Down
5 changes: 3 additions & 2 deletions uintX/iptree16/iptree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ func (t *Tree) enumerate(ch chan Pair) {
}

func iPv4NetToUint32(n *net.IPNet) (uint32, int) {
if len(n.IP) != net.IPv4len {
ip := n.IP.To4()
if ip == nil {
return 0, -1
}

Expand All @@ -226,7 +227,7 @@ func iPv4NetToUint32(n *net.IPNet) (uint32, int) {
return 0, -1
}

return packIPToUint32(n.IP), ones
return packIPToUint32(ip), ones
}

func packIPToUint32(x net.IP) uint32 {
Expand Down
7 changes: 6 additions & 1 deletion uintX/iptree16/iptree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,13 @@ func TestTreeByIP(t *testing.T) {
}

func TestIPv4NetToUint32(t *testing.T) {
key, bits := iPv4NetToUint32(&net.IPNet{IP: net.IPv4zero, Mask: iPv4MaxMask})
if key != 0 || bits != 32 {
t.Errorf("Expected 0x0, 32 pair but got 0x%08x, %d", key, bits)
}

_, n, _ := net.ParseCIDR("192.0.2.0/24")
key, bits := iPv4NetToUint32(n)
key, bits = iPv4NetToUint32(n)
if key != 0xc0000200 || bits != 24 {
t.Errorf("Expected 0xc0000200, 24 pair but got 0x%08x, %d", key, bits)
}
Expand Down
5 changes: 3 additions & 2 deletions uintX/iptree32/iptree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ func (t *Tree) enumerate(ch chan Pair) {
}

func iPv4NetToUint32(n *net.IPNet) (uint32, int) {
if len(n.IP) != net.IPv4len {
ip := n.IP.To4()
if ip == nil {
return 0, -1
}

Expand All @@ -226,7 +227,7 @@ func iPv4NetToUint32(n *net.IPNet) (uint32, int) {
return 0, -1
}

return packIPToUint32(n.IP), ones
return packIPToUint32(ip), ones
}

func packIPToUint32(x net.IP) uint32 {
Expand Down
7 changes: 6 additions & 1 deletion uintX/iptree32/iptree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,13 @@ func TestTreeByIP(t *testing.T) {
}

func TestIPv4NetToUint32(t *testing.T) {
key, bits := iPv4NetToUint32(&net.IPNet{IP: net.IPv4zero, Mask: iPv4MaxMask})
if key != 0 || bits != 32 {
t.Errorf("Expected 0x0, 32 pair but got 0x%08x, %d", key, bits)
}

_, n, _ := net.ParseCIDR("192.0.2.0/24")
key, bits := iPv4NetToUint32(n)
key, bits = iPv4NetToUint32(n)
if key != 0xc0000200 || bits != 24 {
t.Errorf("Expected 0xc0000200, 24 pair but got 0x%08x, %d", key, bits)
}
Expand Down
5 changes: 3 additions & 2 deletions uintX/iptree64/iptree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ func (t *Tree) enumerate(ch chan Pair) {
}

func iPv4NetToUint32(n *net.IPNet) (uint32, int) {
if len(n.IP) != net.IPv4len {
ip := n.IP.To4()
if ip == nil {
return 0, -1
}

Expand All @@ -226,7 +227,7 @@ func iPv4NetToUint32(n *net.IPNet) (uint32, int) {
return 0, -1
}

return packIPToUint32(n.IP), ones
return packIPToUint32(ip), ones
}

func packIPToUint32(x net.IP) uint32 {
Expand Down
7 changes: 6 additions & 1 deletion uintX/iptree64/iptree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,13 @@ func TestTreeByIP(t *testing.T) {
}

func TestIPv4NetToUint32(t *testing.T) {
key, bits := iPv4NetToUint32(&net.IPNet{IP: net.IPv4zero, Mask: iPv4MaxMask})
if key != 0 || bits != 32 {
t.Errorf("Expected 0x0, 32 pair but got 0x%08x, %d", key, bits)
}

_, n, _ := net.ParseCIDR("192.0.2.0/24")
key, bits := iPv4NetToUint32(n)
key, bits = iPv4NetToUint32(n)
if key != 0xc0000200 || bits != 24 {
t.Errorf("Expected 0xc0000200, 24 pair but got 0x%08x, %d", key, bits)
}
Expand Down
5 changes: 3 additions & 2 deletions uintX/iptree8/iptree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ func (t *Tree) enumerate(ch chan Pair) {
}

func iPv4NetToUint32(n *net.IPNet) (uint32, int) {
if len(n.IP) != net.IPv4len {
ip := n.IP.To4()
if ip == nil {
return 0, -1
}

Expand All @@ -226,7 +227,7 @@ func iPv4NetToUint32(n *net.IPNet) (uint32, int) {
return 0, -1
}

return packIPToUint32(n.IP), ones
return packIPToUint32(ip), ones
}

func packIPToUint32(x net.IP) uint32 {
Expand Down
7 changes: 6 additions & 1 deletion uintX/iptree8/iptree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,13 @@ func TestTreeByIP(t *testing.T) {
}

func TestIPv4NetToUint32(t *testing.T) {
key, bits := iPv4NetToUint32(&net.IPNet{IP: net.IPv4zero, Mask: iPv4MaxMask})
if key != 0 || bits != 32 {
t.Errorf("Expected 0x0, 32 pair but got 0x%08x, %d", key, bits)
}

_, n, _ := net.ParseCIDR("192.0.2.0/24")
key, bits := iPv4NetToUint32(n)
key, bits = iPv4NetToUint32(n)
if key != 0xc0000200 || bits != 24 {
t.Errorf("Expected 0xc0000200, 24 pair but got 0x%08x, %d", key, bits)
}
Expand Down
5 changes: 3 additions & 2 deletions uintX/iptree{{.bits}}/iptree.{{.ext}}
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ func (t *Tree) enumerate(ch chan Pair) {
}

func iPv4NetToUint32(n *net.IPNet) (uint32, int) {
if len(n.IP) != net.IPv4len {
ip := n.IP.To4()
if ip == nil {
return 0, -1
}

Expand All @@ -226,7 +227,7 @@ func iPv4NetToUint32(n *net.IPNet) (uint32, int) {
return 0, -1
}

return packIPToUint32(n.IP), ones
return packIPToUint32(ip), ones
}

func packIPToUint32(x net.IP) uint32 {
Expand Down
7 changes: 6 additions & 1 deletion uintX/iptree{{.bits}}/iptree_test.{{.ext}}
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,13 @@ func TestTreeByIP(t *testing.T) {
}

func TestIPv4NetToUint32(t *testing.T) {
key, bits := iPv4NetToUint32(&net.IPNet{IP: net.IPv4zero, Mask: iPv4MaxMask})
if key != 0 || bits != 32 {
t.Errorf("Expected 0x0, 32 pair but got 0x%08x, %d", key, bits)
}

_, n, _ := net.ParseCIDR("192.0.2.0/24")
key, bits := iPv4NetToUint32(n)
key, bits = iPv4NetToUint32(n)
if key != 0xc0000200 || bits != 24 {
t.Errorf("Expected 0xc0000200, 24 pair but got 0x%08x, %d", key, bits)
}
Expand Down