diff --git a/client/internal/debug/debug.go b/client/internal/debug/debug.go index 384f31bec..ce86fd697 100644 --- a/client/internal/debug/debug.go +++ b/client/internal/debug/debug.go @@ -1262,7 +1262,9 @@ func anonymizePeerConfig(config *mgmProto.PeerConfig, anonymizer *anonymize.Anon if v6Prefix, err := netiputil.DecodePrefix(config.GetAddressV6()); err == nil { anonV6 := anonymizer.AnonymizeIP(v6Prefix.Addr()) - config.AddressV6 = netiputil.EncodePrefix(netip.PrefixFrom(anonV6, v6Prefix.Bits())) + if b, err := netiputil.EncodePrefix(netip.PrefixFrom(anonV6, v6Prefix.Bits())); err == nil { + config.AddressV6 = b + } } anonymizeSSHConfig(config.SshConfig) diff --git a/client/internal/debug/debug_test.go b/client/internal/debug/debug_test.go index 0a7bf24c5..716a63f24 100644 --- a/client/internal/debug/debug_test.go +++ b/client/internal/debug/debug_test.go @@ -20,6 +20,13 @@ import ( "github.com/netbirdio/netbird/shared/netiputil" ) +func mustEncodePrefix(t *testing.T, p netip.Prefix) []byte { + t.Helper() + b, err := netiputil.EncodePrefix(p) + require.NoError(t, err) + return b +} + func TestAnonymizeStateFile(t *testing.T) { testState := map[string]json.RawMessage{ "null_state": json.RawMessage("null"), @@ -278,7 +285,7 @@ func TestAnonymizeNetworkMap(t *testing.T) { networkMap := &mgmProto.NetworkMap{ PeerConfig: &mgmProto.PeerConfig{ Address: "203.0.113.5", - AddressV6: netiputil.EncodePrefix(origV6Prefix), + AddressV6: mustEncodePrefix(t, origV6Prefix), Dns: "1.2.3.4", Fqdn: "peer1.corp.example.com", SshConfig: &mgmProto.SSHConfig{ diff --git a/client/internal/engine_test.go b/client/internal/engine_test.go index bf1bf6c89..96bf2f563 100644 --- a/client/internal/engine_test.go +++ b/client/internal/engine_test.go @@ -1701,6 +1701,13 @@ func getPeers(e *Engine) int { return len(e.peerStore.PeersPubKey()) } +func mustEncodePrefix(t *testing.T, p netip.Prefix) []byte { + t.Helper() + b, err := netiputil.EncodePrefix(p) + require.NoError(t, err) + return b +} + func TestEngine_hasIPv6Changed(t *testing.T) { v4Only := wgaddr.MustParseWGAddress("100.64.0.1/16") @@ -1723,7 +1730,7 @@ func TestEngine_hasIPv6Changed(t *testing.T) { { name: "no v6 before, v6 added", current: v4Only, - confV6: netiputil.EncodePrefix(netip.MustParsePrefix("fd00::1/64")), + confV6: mustEncodePrefix(t, netip.MustParsePrefix("fd00::1/64")), expected: true, }, { @@ -1735,19 +1742,19 @@ func TestEngine_hasIPv6Changed(t *testing.T) { { name: "had v6, same v6", current: v4v6, - confV6: netiputil.EncodePrefix(netip.MustParsePrefix("fd00::1/64")), + confV6: mustEncodePrefix(t, netip.MustParsePrefix("fd00::1/64")), expected: false, }, { name: "had v6, different v6", current: v4v6, - confV6: netiputil.EncodePrefix(netip.MustParsePrefix("fd00::2/64")), + confV6: mustEncodePrefix(t, netip.MustParsePrefix("fd00::2/64")), expected: true, }, { name: "same v6 addr, different prefix length", current: v4v6, - confV6: netiputil.EncodePrefix(netip.MustParsePrefix("fd00::1/80")), + confV6: mustEncodePrefix(t, netip.MustParsePrefix("fd00::1/80")), expected: true, }, { diff --git a/management/internals/shared/grpc/conversion.go b/management/internals/shared/grpc/conversion.go index f60a20e33..12402b420 100644 --- a/management/internals/shared/grpc/conversion.go +++ b/management/internals/shared/grpc/conversion.go @@ -119,7 +119,9 @@ func toPeerConfig(peer *nbpeer.Peer, network *types.Network, dnsName string, set if peer.SupportsIPv6() && peer.IPv6.IsValid() && network.NetV6.IP != nil { ones, _ := network.NetV6.Mask.Size() v6Prefix := netip.PrefixFrom(peer.IPv6.Unmap(), ones) - peerConfig.AddressV6 = netiputil.EncodePrefix(v6Prefix) + if b, err := netiputil.EncodePrefix(v6Prefix); err == nil { + peerConfig.AddressV6 = b + } } return peerConfig @@ -344,9 +346,9 @@ func populateSourcePrefixes(fwRule *proto.FirewallRule, rule *types.FirewallRule return nil } - fwRule.SourcePrefixes = [][]byte{ - netiputil.EncodePrefix(netip.PrefixFrom(netip.IPv4Unspecified(), 0)), - } + // IPv4Unspecified/0 is always valid, error is impossible. + v4Wildcard, _ := netiputil.EncodePrefix(netip.PrefixFrom(netip.IPv4Unspecified(), 0)) + fwRule.SourcePrefixes = [][]byte{v4Wildcard} if !includeIPv6 { return nil @@ -354,9 +356,9 @@ func populateSourcePrefixes(fwRule *proto.FirewallRule, rule *types.FirewallRule v6Rule := goproto.Clone(fwRule).(*proto.FirewallRule) v6Rule.PeerIP = "::" //nolint:staticcheck // populated for backward compatibility - v6Rule.SourcePrefixes = [][]byte{ - netiputil.EncodePrefix(netip.PrefixFrom(netip.IPv6Unspecified(), 0)), - } + // IPv6Unspecified/0 is always valid, error is impossible. + v6Wildcard, _ := netiputil.EncodePrefix(netip.PrefixFrom(netip.IPv6Unspecified(), 0)) + v6Rule.SourcePrefixes = [][]byte{v6Wildcard} if shouldUsePortRange(v6Rule) { v6Rule.PortInfo = rule.PortRange.ToProto() } diff --git a/shared/netiputil/compact.go b/shared/netiputil/compact.go index 355d2ead1..0cd2b8a20 100644 --- a/shared/netiputil/compact.go +++ b/shared/netiputil/compact.go @@ -14,15 +14,14 @@ import ( ) // EncodePrefix encodes a netip.Prefix into compact bytes. -// The address is always unmapped before encoding. If unmapping produces a v4 -// address, the prefix length is clamped to 32. -func EncodePrefix(p netip.Prefix) []byte { +// The address is always unmapped before encoding. +func EncodePrefix(p netip.Prefix) ([]byte, error) { addr := p.Addr().Unmap() bits := p.Bits() if addr.Is4() && bits > 32 { - bits = 32 + return nil, fmt.Errorf("invalid prefix length %d for IPv4 address %s (max 32)", bits, addr) } - return append(addr.AsSlice(), byte(bits)) + return append(addr.AsSlice(), byte(bits)), nil } // DecodePrefix decodes compact bytes into a netip.Prefix. @@ -43,7 +42,7 @@ func DecodePrefix(b []byte) (netip.Prefix, error) { bits := int(b[len(b)-1]) if addr.Is4() { if bits > 32 { - bits = 32 + return netip.Prefix{}, fmt.Errorf("invalid prefix length %d for v4-mapped address (max 32)", bits) } } else if bits > 128 { return netip.Prefix{}, fmt.Errorf("invalid IPv6 prefix length %d (max 128)", bits) @@ -62,7 +61,9 @@ func EncodeAddr(a netip.Addr) []byte { if a.Is4() { bits = 32 } - return EncodePrefix(netip.PrefixFrom(a, bits)) + // Host prefix lengths are always valid for the address family, so error is impossible. + b, _ := EncodePrefix(netip.PrefixFrom(a, bits)) + return b } // DecodeAddr decodes compact prefix bytes and returns only the address, diff --git a/shared/netiputil/compact_test.go b/shared/netiputil/compact_test.go index d5a4756c0..1e7c7ed82 100644 --- a/shared/netiputil/compact_test.go +++ b/shared/netiputil/compact_test.go @@ -59,7 +59,8 @@ func TestEncodeDecodePrefix(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { p := netip.MustParsePrefix(tt.prefix) - b := EncodePrefix(p) + b, err := EncodePrefix(p) + require.NoError(t, err) assert.Equal(t, tt.size, len(b), "encoded size") decoded, err := DecodePrefix(b) @@ -72,7 +73,8 @@ func TestEncodeDecodePrefix(t *testing.T) { func TestEncodePrefixUnmaps(t *testing.T) { // v4-mapped v6 address should encode as v4 mapped := netip.MustParsePrefix("::ffff:10.1.2.3/32") - b := EncodePrefix(mapped) + b, err := EncodePrefix(mapped) + require.NoError(t, err) assert.Equal(t, 5, len(b), "v4-mapped should encode as 5 bytes") decoded, err := DecodePrefix(b) @@ -80,24 +82,26 @@ func TestEncodePrefixUnmaps(t *testing.T) { assert.Equal(t, netip.MustParsePrefix("10.1.2.3/32"), decoded) } -func TestEncodePrefixUnmapsClampsBits(t *testing.T) { - // v4-mapped v6 with bits > 32 should clamp to /32 - mapped := netip.MustParsePrefix("::ffff:10.1.2.3/128") - b := EncodePrefix(mapped) +func TestEncodePrefixUnmapsRejectsInvalidBits(t *testing.T) { + // v4-mapped v6 with bits > 32 should return an error + mapped128 := netip.MustParsePrefix("::ffff:10.1.2.3/128") + _, err := EncodePrefix(mapped128) + require.Error(t, err) + + // v4-mapped v6 with bits=96 should also return an error + mapped96 := netip.MustParsePrefix("::ffff:10.0.0.0/96") + _, err = EncodePrefix(mapped96) + require.Error(t, err) + + // v4-mapped v6 with bits=32 should succeed + mapped32 := netip.MustParsePrefix("::ffff:10.1.2.3/32") + b, err := EncodePrefix(mapped32) + require.NoError(t, err) assert.Equal(t, 5, len(b), "v4-mapped should encode as 5 bytes") decoded, err := DecodePrefix(b) require.NoError(t, err) assert.Equal(t, netip.MustParsePrefix("10.1.2.3/32"), decoded) - - // v4-mapped v6 with bits=96 should also clamp to /32 - mapped96 := netip.MustParsePrefix("::ffff:10.0.0.0/96") - b96 := EncodePrefix(mapped96) - assert.Equal(t, 5, len(b96)) - - decoded96, err := DecodePrefix(b96) - require.NoError(t, err) - assert.Equal(t, 32, decoded96.Bits()) } func TestDecodeAddr(t *testing.T) { @@ -147,16 +151,24 @@ func TestDecodePrefixInvalidBits(t *testing.T) { } func TestDecodePrefixUnmapsV6Input(t *testing.T) { - // If someone encodes a v4-mapped v6 as 17 bytes, decode should unmap it - // and clamp the prefix length to 32 for v4 addr := netip.MustParseAddr("::ffff:192.168.1.1") + // v4-mapped v6 with bits > 32 should return an error raw := addr.As16() - b := make([]byte, 17) - copy(b, raw[:]) - b[16] = 128 + bInvalid := make([]byte, 17) + copy(bInvalid, raw[:]) + bInvalid[16] = 128 - decoded, err := DecodePrefix(b) + _, err := DecodePrefix(bInvalid) + require.Error(t, err, "v4-mapped address with /128 prefix should be rejected") + assert.Contains(t, err.Error(), "invalid prefix length") + + // v4-mapped v6 with valid /32 should decode and unmap correctly + bValid := make([]byte, 17) + copy(bValid, raw[:]) + bValid[16] = 32 + + decoded, err := DecodePrefix(bValid) require.NoError(t, err) assert.True(t, decoded.Addr().Is4(), "should be unmapped to v4") assert.Equal(t, netip.MustParsePrefix("192.168.1.1/32"), decoded)