Skip to content

Commit

Permalink
fix(ipnet): tighten the checking of IP addresses (#942)
Browse files Browse the repository at this point in the history
  • Loading branch information
@favonia
favonia authored Sep 23, 2024
1 parent 2d95d69 commit 640d30b
Show file tree
Hide file tree
Showing 5 changed files with 129 additions and 80 deletions.
42 changes: 30 additions & 12 deletions internal/ipnet/ipnet.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ func (t Type) UDPNetwork() string {

// Matches checks whether an IP belongs to it.
func (t Type) Matches(ip netip.Addr) bool {
ip = ip.Unmap()
switch t {
case IP4:
return ip.Is4()
Expand All @@ -79,9 +80,9 @@ func (t Type) Matches(ip netip.Addr) bool {
// NormalizeDetectedIP normalizes an IP into an IPv4 or IPv6 address.
func (t Type) NormalizeDetectedIP(ppfmt pp.PP, ip netip.Addr) (netip.Addr, bool) {
if !ip.IsValid() {
ppfmt.Noticef(
pp.EmojiImpossible,
`Detected IP address is not valid`,
ppfmt.Noticef(pp.EmojiImpossible,
`Detected IP address is not valid; this should not happen and please report it at %s`,
pp.IssueReportingURL,
)
return netip.Addr{}, false
}
Expand Down Expand Up @@ -115,21 +116,38 @@ func (t Type) NormalizeDetectedIP(ppfmt pp.PP, ip netip.Addr) (netip.Addr, bool)
return netip.Addr{}, false
}

if ip.IsUnspecified() {
ppfmt.Noticef(
pp.EmojiImpossible,
`Detected IP address %s is an unspecified %s address`,
ip.String(),
t.Describe(),
switch {
case ip.IsUnspecified():
ppfmt.Noticef(pp.EmojiError,
`Detected %s address %s is an unspecified address`,
t.Describe(), ip.String(),
)
return netip.Addr{}, false
case ip.IsLoopback():
ppfmt.Noticef(pp.EmojiError,
`Detected %s address %s is a loopback address`,
t.Describe(), ip.String(),
)
return netip.Addr{}, false
case ip.IsInterfaceLocalMulticast():
ppfmt.Noticef(pp.EmojiError,
`Detected %s address %s is an interface-local multicast address`,
t.Describe(), ip.String(),
)
return netip.Addr{}, false
case ip.IsLinkLocalMulticast(), ip.IsLinkLocalUnicast():
ppfmt.Noticef(pp.EmojiError,
`Detected %s address %s is a link-local address`,
t.Describe(), ip.String(),
)
return netip.Addr{}, false
}

if !ip.IsGlobalUnicast() {
ppfmt.Noticef(
pp.EmojiUserWarning,
`Detected IP address %s does not look like a global unicast IP address.`,
ip.String(),
pp.EmojiWarning,
`Detected %s address %s does not look like a global unicast address`,
t.Describe(), ip.String(),
)
}

Expand Down
98 changes: 72 additions & 26 deletions internal/ipnet/ipnet_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,35 +86,75 @@ func TestUDPNetwork(t *testing.T) {

func TestNormalizeDetectedIP(t *testing.T) {
t.Parallel()

var invalidIP netip.Addr

for name, tc := range map[string]struct {
ipNet ipnet.Type
ip netip.Addr
expected netip.Addr
ok bool
expected netip.Addr
prepareMockPP func(*mocks.MockPP)
}{
"4-invalid": {
ipnet.IP4, invalidIP,
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiImpossible,
`Detected IP address is not valid; this should not happen and please report it at %s`,
pp.IssueReportingURL)
},
},
"4-1::2": {
ipnet.IP4, mustIP("1::2"),
netip.Addr{},
false,
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiError, "Detected IP address %s is not a valid IPv4 address", "1::2")
},
},
"4-::ffff:0a0a:0a0a": {ipnet.IP4, mustIP("::ffff:0a0a:0a0a"), mustIP("10.10.10.10"), true, nil},
"6-1::2": {ipnet.IP6, mustIP("1::2"), mustIP("1::2"), true, nil},
"4-::ffff:0a0a:0a0a": {ipnet.IP4, mustIP("::ffff:0a0a:0a0a"), true, mustIP("10.10.10.10"), nil},
"4-0.0.0.0": {
ipnet.IP4, mustIP("0.0.0.0"),
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiError,
"Detected %s address %s is an unspecified address", "IPv4", "0.0.0.0")
},
},
"4-127.0.0.1": {
ipnet.IP4, mustIP("127.0.0.1"),
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiError, "Detected %s address %s is a loopback address", "IPv4", "127.0.0.1")
},
},
"4-224.0.0.1": {
ipnet.IP4, mustIP("224.0.0.1"),
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiError, "Detected %s address %s is a link-local address", "IPv4", "224.0.0.1")
},
},
"6-invalid": {
ipnet.IP6, invalidIP,
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiImpossible,
`Detected IP address is not valid; this should not happen and please report it at %s`,
pp.IssueReportingURL)
},
},
"6-1::2": {ipnet.IP6, mustIP("1::2"), true, mustIP("1::2"), nil},
"6-10.10.10.10": {
ipnet.IP6, mustIP("10.10.10.10"),
netip.Addr{},
false,
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiError, "Detected IP address %s is not a valid IPv6 address", "10.10.10.10")
},
},
"6-::ffff:10.10.10.10": {
ipnet.IP6, mustIP("::ffff:10.10.10.10"),
netip.Addr{},
false,
false, invalidIP,
func(m *mocks.MockPP) {
gomock.InOrder(
m.EXPECT().Noticef(pp.EmojiError,
Expand All @@ -128,30 +168,36 @@ func TestNormalizeDetectedIP(t *testing.T) {
)
},
},
"6-invalid": {
ipnet.IP6,
netip.Addr{},
netip.Addr{},
false,
"6-::1": {
ipnet.IP6, mustIP("::1"),
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiImpossible, "Detected IP address is not valid")
m.EXPECT().Noticef(pp.EmojiError, "Detected %s address %s is a loopback address", "IPv6", "::1")
},
},
"100-10.10.10.10": {
100, mustIP("10.10.10.10"),
netip.Addr{},
false,
nil,
"6-ff01::1": {
ipnet.IP6, mustIP("ff01::1"),
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiError,
"Detected %s address %s is an interface-local multicast address",
"IPv6", "ff01::1")
},
},
"4-0.0.0.0": {
ipnet.IP4, mustIP("0.0.0.0"),
netip.Addr{},
false,
"6-ff03::1": {
ipnet.IP6, mustIP("ff03::1"),
true, mustIP("ff03::1"),
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiImpossible,
"Detected IP address %s is an unspecified %s address", "0.0.0.0", "IPv4")
m.EXPECT().Noticef(pp.EmojiWarning,
"Detected %s address %s does not look like a global unicast address",
"IPv6", "ff03::1")
},
},
"100-10.10.10.10": {
100, mustIP("10.10.10.10"),
false, invalidIP,
nil,
},
} {
t.Run(name, func(t *testing.T) {
t.Parallel()
Expand Down
2 changes: 1 addition & 1 deletion internal/provider/happy_eyeballs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func (p happyEyeballs) GetIP(ctx context.Context, ppfmt pp.PP, ipNet ipnet.Type)
queuedPP[r.method].Flush()

if r.method == MethodAlternative {
ppfmt.Infof(pp.EmojiNow, "The server 1.0.0.1 responded before 1.1.1.1 does and will be used from now on.")
ppfmt.Infof(pp.EmojiNow, "The server 1.0.0.1 responded before 1.1.1.1 does and will be used from now on")
}

return r.ip, r.method, r.ok
Expand Down
6 changes: 3 additions & 3 deletions internal/provider/happy_eyeballs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func TestHappyEyeballs(t *testing.T) {
return ip1001, true
}).Times(2)
ppfmt.EXPECT().Infof(pp.EmojiGood, "Got 1.0.0.1!")
ppfmt.EXPECT().Infof(pp.EmojiNow, "The server 1.0.0.1 responded before 1.1.1.1 does and will be used from now on.")
ppfmt.EXPECT().Infof(pp.EmojiNow, "The server 1.0.0.1 responded before 1.1.1.1 does and will be used from now on")
p.EXPECT().HasAlternative(ipNet).Return(true)
ppfmt.EXPECT().Infof(pp.EmojiGood, "Got 1.0.0.1!")
},
Expand Down Expand Up @@ -150,7 +150,7 @@ func TestHappyEyeballs(t *testing.T) {
return ip1001, true
}).Times(2)
ppfmt.EXPECT().Infof(pp.EmojiGood, "Got 1.0.0.1!")
ppfmt.EXPECT().Infof(pp.EmojiNow, "The server 1.0.0.1 responded before 1.1.1.1 does and will be used from now on.")
ppfmt.EXPECT().Infof(pp.EmojiNow, "The server 1.0.0.1 responded before 1.1.1.1 does and will be used from now on")
p.EXPECT().HasAlternative(ipNet).Return(true)
ppfmt.EXPECT().Infof(pp.EmojiGood, "Got 1.0.0.1!")
},
Expand Down Expand Up @@ -192,7 +192,7 @@ func TestHappyEyeballs(t *testing.T) {
return ip1001, true
}).Times(2)
ppfmt.EXPECT().Infof(pp.EmojiGood, "Got 1.0.0.1!")
ppfmt.EXPECT().Infof(pp.EmojiNow, "The server 1.0.0.1 responded before 1.1.1.1 does and will be used from now on.")
ppfmt.EXPECT().Infof(pp.EmojiNow, "The server 1.0.0.1 responded before 1.1.1.1 does and will be used from now on")
p.EXPECT().HasAlternative(ipNet).Return(true)
ppfmt.EXPECT().Infof(pp.EmojiGood, "Got 1.0.0.1!")
},
Expand Down
61 changes: 23 additions & 38 deletions internal/provider/protocol/local_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,71 +28,56 @@ func TestLocalName(t *testing.T) {
func TestLocalGetIP(t *testing.T) {
t.Parallel()

ip4Loopback := netip.MustParseAddr("127.0.0.1")
ip6Loopback := netip.MustParseAddr("::1")
invalidIP := netip.Addr{}

for name, tc := range map[string]struct {
addrKey ipnet.Type
addr string
ipNet ipnet.Type
expected gomock.Matcher
ok bool
expected netip.Addr
prepareMockPP func(*mocks.MockPP)
}{
"4": {
ipnet.IP4, "127.0.0.1:80", ipnet.IP4, gomock.Eq(ip4Loopback), true,
"loopback/4": {
ipnet.IP4, "127.0.0.1:80", ipnet.IP4,
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiUserWarning,
"Detected IP address %s does not look like a global unicast IP address.", "127.0.0.1")
m.EXPECT().Noticef(pp.EmojiError,
"Detected %s address %s is a loopback address", "IPv4", "127.0.0.1")
},
},
"6": {
"loopback/6": {
ipnet.IP6, "[::1]:80", ipnet.IP6,
gomock.AnyOf(
ip6Loopback,
gomock.Cond(func(x any) bool {
a, ok := x.(netip.Addr)
if !ok {
return false
}
return a.IsLinkLocalUnicast()
})),
true,
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiUserWarning,
"Detected IP address %s does not look like a global unicast IP address.",
gomock.AnyOf(
"::1",
gomock.Cond(func(x any) bool {
s, ok := x.(string)
if !ok {
return false
}
return netip.MustParseAddr(s).IsLinkLocalUnicast()
})))
m.EXPECT().Noticef(pp.EmojiError,
"Detected %s address %s is a loopback address", "IPv6", "::1")
},
},
"4-nil1": {
ipnet.IP4, "", ipnet.IP4, gomock.Eq(invalidIP), false,
"empty/4": {
ipnet.IP4, "", ipnet.IP4,
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiError, "Failed to detect a local %s address: %v", "IPv4", gomock.Any())
},
},
"6-nil1": {
ipnet.IP6, "", ipnet.IP6, gomock.Eq(invalidIP), false,
"empty/6": {
ipnet.IP6, "", ipnet.IP6,
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiError, "Failed to detect a local %s address: %v", "IPv6", gomock.Any())
},
},
"4-nil2": {
ipnet.IP4, "127.0.0.1:80", ipnet.IP6, gomock.Eq(invalidIP), false,
"mismatch/6": {
ipnet.IP4, "127.0.0.1:80", ipnet.IP6,
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiImpossible, "Unhandled IP network: %s", "IPv6")
},
},
"6-nil2": {
ipnet.IP6, "::1:80", ipnet.IP4, gomock.Eq(invalidIP), false,
"mismatch/4": {
ipnet.IP6, "::1:80", ipnet.IP4,
false, invalidIP,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiImpossible, "Unhandled IP network: %s", "IPv4")
},
Expand All @@ -114,7 +99,7 @@ func TestLocalGetIP(t *testing.T) {
tc.prepareMockPP(mockPP)
}
ip, method, ok := provider.GetIP(context.Background(), mockPP, tc.ipNet)
require.True(t, tc.expected.Matches(ip))
require.Equal(t, tc.expected, ip)
require.NotEqual(t, protocol.MethodAlternative, method)
require.Equal(t, tc.ok, ok)
})
Expand Down

0 comments on commit 640d30b

Please sign in to comment.