From 8a5e940c84128d43fe9df83700138c5ba3100966 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Wed, 2 Sep 2026 17:52:51 +0200 Subject: [PATCH] [management] remove old math rand lib (#6836) --- .../modules/agentnetwork/labelgen/labelgen.go | 16 ++--- .../agentnetwork/labelgen/labelgen_test.go | 39 +++--------- .../internals/modules/agentnetwork/manager.go | 11 +--- management/server/account.go | 6 +- management/server/group_ipv6_test.go | 3 +- management/server/idp/util.go | 16 ++--- management/server/types/network.go | 61 +++++++++++-------- management/server/types/network_test.go | 28 +++++++++ management/server/util/util.go | 15 +++++ 9 files changed, 111 insertions(+), 84 deletions(-) diff --git a/management/internals/modules/agentnetwork/labelgen/labelgen.go b/management/internals/modules/agentnetwork/labelgen/labelgen.go index 549767096..bd5b0129d 100644 --- a/management/internals/modules/agentnetwork/labelgen/labelgen.go +++ b/management/internals/modules/agentnetwork/labelgen/labelgen.go @@ -3,9 +3,10 @@ package labelgen import ( "fmt" - "math/rand" "sort" "sync" + + "github.com/netbirdio/netbird/management/server/util" ) // pickAttempts caps the random retries before falling back to the @@ -40,16 +41,15 @@ func uniqueWords() []string { // PickUnique selects a label not already in `taken`. It tries up to // pickAttempts random picks; on exhaustion it scans the deduplicated // wordlist for any remaining free entry, and if none is left appends -// `-` to a deterministic word and returns. The caller -// is responsible for seeding rng (math/rand). -func PickUnique(rng *rand.Rand, taken map[string]struct{}, fallbackSuffix string) string { +// `-` to a random word and returns. +func PickUnique(taken map[string]struct{}, fallbackSuffix string) string { pool := uniqueWords() if len(pool) == 0 { return fallbackSuffix } for i := 0; i < pickAttempts; i++ { - w := pool[rng.Intn(len(pool))] + w := pool[util.RandIntn(len(pool))] if _, ok := taken[w]; !ok { return w } @@ -61,7 +61,7 @@ func PickUnique(rng *rand.Rand, taken map[string]struct{}, fallbackSuffix string } } - w := pool[rng.Intn(len(pool))] + w := pool[util.RandIntn(len(pool))] return fmt.Sprintf("%s-%s", w, fallbackSuffix) } @@ -74,10 +74,10 @@ func PickUnique(rng *rand.Rand, taken map[string]struct{}, fallbackSuffix string // a noun spans len(adjectives) * 857 instead. Uniqueness is enforced by a // database constraint and retried by the caller, rather than guessed from a // pre-read set that a concurrent allocation can invalidate. -func PickTuple(rng *rand.Rand) string { +func PickTuple() string { nouns := uniqueWords() if len(nouns) == 0 || len(adjectives) == 0 { return "" } - return adjectives[rng.Intn(len(adjectives))] + "-" + nouns[rng.Intn(len(nouns))] + return adjectives[util.RandIntn(len(adjectives))] + "-" + nouns[util.RandIntn(len(nouns))] } diff --git a/management/internals/modules/agentnetwork/labelgen/labelgen_test.go b/management/internals/modules/agentnetwork/labelgen/labelgen_test.go index 7e12fc133..dda0f09ba 100644 --- a/management/internals/modules/agentnetwork/labelgen/labelgen_test.go +++ b/management/internals/modules/agentnetwork/labelgen/labelgen_test.go @@ -1,7 +1,7 @@ package labelgen import ( - "math/rand" + "slices" "strings" "testing" @@ -9,19 +9,12 @@ import ( "github.com/stretchr/testify/require" ) -// TestPickUnique_DeterministicWithSeededRng locks the property the -// caller relies on: same seed + same taken set → same pick. Without -// that, the bootstrap flow can't reproduce a label across retries. -func TestPickUnique_DeterministicWithSeededRng(t *testing.T) { - taken := map[string]struct{}{} +// TestPickUnique_ReturnsWordFromPool confirms a pick against an empty +// taken set is always drawn verbatim from the wordlist. +func TestPickUnique_ReturnsWordFromPool(t *testing.T) { + got := PickUnique(map[string]struct{}{}, "abcd") - rngA := rand.New(rand.NewSource(42)) - rngB := rand.New(rand.NewSource(42)) - - a := PickUnique(rngA, taken, "abcd") - b := PickUnique(rngB, taken, "abcd") - - assert.Equal(t, a, b, "Same seed and taken set must produce identical pick") + assert.True(t, slices.Contains(uniqueWords(), got), "Pick %q must be drawn from the wordlist", got) } // TestPickUnique_AvoidsTakenWordsWhenMostAreReserved seeds taken with @@ -46,8 +39,7 @@ func TestPickUnique_AvoidsTakenWordsWhenMostAreReserved(t *testing.T) { taken[w] = struct{}{} } - rng := rand.New(rand.NewSource(7)) - got := PickUnique(rng, taken, "abcd") + got := PickUnique(taken, "abcd") _, isFree := free[got] assert.True(t, isFree, "PickUnique must return one of the free words; got %q", got) @@ -65,8 +57,7 @@ func TestPickUnique_FallsBackWhenAllReserved(t *testing.T) { taken[w] = struct{}{} } - rng := rand.New(rand.NewSource(99)) - got := PickUnique(rng, taken, "abcd") + got := PickUnique(taken, "abcd") assert.True(t, strings.HasSuffix(got, "-abcd"), "Exhausted pool must produce -; got %q", got) @@ -114,9 +105,8 @@ func TestPickTuple_ShapeAndPoolMembership(t *testing.T) { inAdjectives[a] = struct{}{} } - rng := rand.New(rand.NewSource(7)) for i := 0; i < 200; i++ { - got := PickTuple(rng) + got := PickTuple() parts := strings.Split(got, "-") require.Len(t, parts, 2, "PickTuple must produce exactly two hyphen-joined words; got %q", got) @@ -158,22 +148,13 @@ func TestAdjectives_AreDNSSafeAndDeduplicated(t *testing.T) { assert.Greater(t, len(adjectives), 150, "Adjective pool too small to give a useful namespace") } -// TestPickTuple_DeterministicWithSeededRng documents that generation is a pure -// function of the rng, which is what makes allocation retries reproducible in tests. -func TestPickTuple_DeterministicWithSeededRng(t *testing.T) { - a := PickTuple(rand.New(rand.NewSource(42))) - b := PickTuple(rand.New(rand.NewSource(42))) - assert.Equal(t, a, b, "Same seed must yield the same tuple") -} - // TestPickTuple_SpansALargeNamespace guards the reason we moved to tuples: a // single-word pool caps the GLOBAL namespace at 857. Drawing many tuples must // yield overwhelmingly distinct values. func TestPickTuple_SpansALargeNamespace(t *testing.T) { - rng := rand.New(rand.NewSource(11)) seen := make(map[string]struct{}, 2000) for i := 0; i < 2000; i++ { - seen[PickTuple(rng)] = struct{}{} + seen[PickTuple()] = struct{}{} } assert.Greater(t, len(seen), 1900, "2000 draws should be nearly all distinct across a ~200k namespace; got %d unique", len(seen)) diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 98aca7f5d..833b4e53a 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "math/rand" "slices" "strings" "sync" @@ -146,11 +145,6 @@ type managerImpl struct { // of serving proxy can be diffed without re-deriving it. reconcileMu sync.Mutex reconcileCache map[string]map[string]syntheticMapping - - // labelRngMu guards labelRng. PickUnique consumes math/rand.Source - // state; concurrent provider creates would otherwise race. - labelRngMu sync.Mutex - labelRng *rand.Rand } // NewManager constructs the persistent Agent Network manager. The @@ -171,7 +165,6 @@ func NewManager( proxyController: proxyController, modelDiscovery: &modeldiscovery.Client{}, reconcileCache: make(map[string]map[string]syntheticMapping), - labelRng: rand.New(rand.NewSource(time.Now().UnixNano())), } } @@ -986,9 +979,7 @@ func (m *managerImpl) bootstrapLabeled(ctx context.Context, settings *types.Sett } for attempt := 1; attempt <= maxDomainAllocationAttempts; attempt++ { - m.labelRngMu.Lock() - label := labelgen.PickTuple(m.labelRng) - m.labelRngMu.Unlock() + label := labelgen.PickTuple() if label == "" { // Only reachable if either word pool were emptied. An empty label // would produce a broken endpoint like ".example.com", so fail diff --git a/management/server/account.go b/management/server/account.go index 4fe0e5338..58698e899 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "math/rand" "net" "net/netip" "os" @@ -65,7 +64,7 @@ const ( type userLoggedInOnce bool func cacheEntryExpiration() time.Duration { - r := rand.Intn(int(nbcache.DefaultIDPCacheExpirationMax.Milliseconds()-nbcache.DefaultIDPCacheExpirationMin.Milliseconds())) + int(nbcache.DefaultIDPCacheExpirationMin.Milliseconds()) + r := util.RandIntn(int(nbcache.DefaultIDPCacheExpirationMax.Milliseconds()-nbcache.DefaultIDPCacheExpirationMin.Milliseconds())) + int(nbcache.DefaultIDPCacheExpirationMin.Milliseconds()) return time.Duration(r) * time.Millisecond } @@ -2470,8 +2469,7 @@ func (am *DefaultAccountManager) ensureIPv6Subnet(ctx context.Context, transacti return transaction.UpdateAccountNetworkV6(ctx, accountID, network.NetV6) } if network.NetV6.IP == nil { - r := rand.New(rand.NewSource(time.Now().UnixNano())) - network.NetV6 = types.AllocateIPv6Subnet(r) + network.NetV6 = types.AllocateIPv6Subnet() // Sync settings to match the allocated subnet so SaveAccountSettings persists it. ones, _ := network.NetV6.Mask.Size() diff --git a/management/server/group_ipv6_test.go b/management/server/group_ipv6_test.go index dfb436060..2679aa7c2 100644 --- a/management/server/group_ipv6_test.go +++ b/management/server/group_ipv6_test.go @@ -2,7 +2,6 @@ package server import ( "context" - "math/rand" "testing" "time" @@ -28,7 +27,7 @@ func TestGroupIPv6Assignment(t *testing.T) { require.NoError(t, err) // Allocate IPv6 subnet for the account - account.Network.NetV6 = types.AllocateIPv6Subnet(rand.New(rand.NewSource(time.Now().UnixNano()))) + account.Network.NetV6 = types.AllocateIPv6Subnet() require.NoError(t, am.Store.SaveAccount(ctx, account)) // Create setup key diff --git a/management/server/idp/util.go b/management/server/idp/util.go index ed82fb9e3..6545c2a69 100644 --- a/management/server/idp/util.go +++ b/management/server/idp/util.go @@ -2,11 +2,12 @@ package idp import ( "encoding/json" - "math/rand" "net/url" "os" "strings" "time" + + "github.com/netbirdio/netbird/management/server/util" ) var ( @@ -33,31 +34,32 @@ func GeneratePassword(passwordLength, minSpecialChar, minNum, minUpperCase int) //Set special character for i := 0; i < minSpecialChar; i++ { - random := rand.Intn(len(specialCharSet)) + random := util.RandIntn(len(specialCharSet)) password.WriteString(string(specialCharSet[random])) } //Set numeric for i := 0; i < minNum; i++ { - random := rand.Intn(len(numberSet)) + random := util.RandIntn(len(numberSet)) password.WriteString(string(numberSet[random])) } //Set uppercase for i := 0; i < minUpperCase; i++ { - random := rand.Intn(len(upperCharSet)) + random := util.RandIntn(len(upperCharSet)) password.WriteString(string(upperCharSet[random])) } remainingLength := passwordLength - minSpecialChar - minNum - minUpperCase for i := 0; i < remainingLength; i++ { - random := rand.Intn(len(allCharSet)) + random := util.RandIntn(len(allCharSet)) password.WriteString(string(allCharSet[random])) } inRune := []rune(password.String()) - rand.Shuffle(len(inRune), func(i, j int) { + for i := len(inRune) - 1; i > 0; i-- { + j := util.RandIntn(i + 1) inRune[i], inRune[j] = inRune[j], inRune[i] - }) + } return string(inRune) } diff --git a/management/server/types/network.go b/management/server/types/network.go index 72ca1af85..1ce6465b5 100644 --- a/management/server/types/network.go +++ b/management/server/types/network.go @@ -1,18 +1,18 @@ package types import ( + "crypto/rand" "encoding/binary" "fmt" - "math/rand" "net" "net/netip" "slices" "sync" - "time" "github.com/c-robinson/iplib" "github.com/rs/xid" + "github.com/netbirdio/netbird/management/server/util" "github.com/netbirdio/netbird/shared/management/status" ) @@ -47,14 +47,12 @@ func NewNetwork() *Network { n := iplib.NewNet4(net.ParseIP("100.64.0.0"), NetSize) sub, _ := n.Subnet(SubnetSize) - s := rand.NewSource(time.Now().UnixNano()) - r := rand.New(s) - intn := r.Intn(len(sub)) + intn := util.RandIntn(len(sub)) return &Network{ Identifier: xid.New().String(), Net: sub[intn].IPNet, - NetV6: AllocateIPv6Subnet(r), + NetV6: AllocateIPv6Subnet(), Dns: "", Serial: 0, } @@ -64,18 +62,13 @@ func NewNetwork() *Network { // The format follows RFC 4193 section 3.1: fd + 40-bit Global ID + 16-bit Subnet ID. // The Global ID and Subnet ID are randomized (simplified from the SHA-1 algorithm // in section 3.2.2), giving 2^56 possible /64 subnets across all accounts. -func AllocateIPv6Subnet(r *rand.Rand) net.IPNet { +func AllocateIPv6Subnet() net.IPNet { ip := make(net.IP, 16) ip[0] = 0xfd - // Bytes 1-5: 40-bit random Global ID - ip[1] = byte(r.Intn(256)) - ip[2] = byte(r.Intn(256)) - ip[3] = byte(r.Intn(256)) - ip[4] = byte(r.Intn(256)) - ip[5] = byte(r.Intn(256)) - // Bytes 6-7: 16-bit random Subnet ID - ip[6] = byte(r.Intn(256)) - ip[7] = byte(r.Intn(256)) + // Bytes 1-5: 40-bit random Global ID, bytes 6-7: 16-bit random Subnet ID + if _, err := rand.Read(ip[1:8]); err != nil { + panic(err) + } return net.IPNet{ IP: ip, @@ -109,10 +102,22 @@ func (n *Network) Copy() *Network { } } +// validateIPv4Prefix ensures the prefix is an IPv4 network with assignable host addresses. +func validateIPv4Prefix(prefix netip.Prefix) error { + if !prefix.IsValid() || !prefix.Addr().Is4() || prefix.Bits() < 1 || prefix.Bits() >= 31 { + return fmt.Errorf("invalid IPv4 subnet: %s", prefix.String()) + } + return nil +} + // AllocatePeerIP picks an available IP from a netip.Prefix. // This method considers already taken IPs and reuses IPs if there are gaps in takenIps. // E.g. if prefix=100.30.0.0/16 and takenIps=[100.30.0.1, 100.30.0.4] then the result would be 100.30.0.2 or 100.30.0.3. func AllocatePeerIP(prefix netip.Prefix, takenIps []netip.Addr) (netip.Addr, error) { + if err := validateIPv4Prefix(prefix); err != nil { + return netip.Addr{}, err + } + b := prefix.Masked().Addr().As4() baseIP := binary.BigEndian.Uint32(b[:]) hostBits := 32 - prefix.Bits() @@ -123,15 +128,17 @@ func AllocatePeerIP(prefix netip.Prefix, takenIps []netip.Addr) (netip.Addr, err taken[baseIP+totalIPs-1] = struct{}{} // reserve broadcast IP for _, ip := range takenIps { + if !ip.Is4() { + continue + } ab := ip.As4() taken[binary.BigEndian.Uint32(ab[:])] = struct{}{} } - rng := rand.New(rand.NewSource(time.Now().UnixNano())) maxAttempts := (int(totalIPs) - len(taken)) / 100 for i := 0; i < maxAttempts; i++ { - offset := uint32(rng.Intn(int(totalIPs-2))) + 1 + offset := uint32(util.RandIntn(int(totalIPs-2))) + 1 candidate := baseIP + offset if _, exists := taken[candidate]; !exists { return uint32ToIP(candidate), nil @@ -150,13 +157,16 @@ func AllocatePeerIP(prefix netip.Prefix, takenIps []netip.Addr) (netip.Addr, err // AllocateRandomPeerIP picks a random available IP from a netip.Prefix. func AllocateRandomPeerIP(prefix netip.Prefix) (netip.Addr, error) { + if err := validateIPv4Prefix(prefix); err != nil { + return netip.Addr{}, err + } + b := prefix.Masked().Addr().As4() baseIP := binary.BigEndian.Uint32(b[:]) hostBits := 32 - prefix.Bits() totalIPs := uint32(1 << hostBits) - rng := rand.New(rand.NewSource(time.Now().UnixNano())) - offset := uint32(rng.Intn(int(totalIPs-2))) + 1 + offset := uint32(util.RandIntn(int(totalIPs-2))) + 1 candidate := baseIP + offset return uint32ToIP(candidate), nil @@ -172,23 +182,26 @@ func AllocateRandomPeerIPv6(prefix netip.Prefix) (netip.Addr, error) { ip := prefix.Addr().As16() - rng := rand.New(rand.NewSource(time.Now().UnixNano())) - // Determine which byte the host bits start in firstHostByte := ones / 8 // If the prefix doesn't end on a byte boundary, handle the partial byte partialBits := ones % 8 + var rnd [16]byte + if _, err := rand.Read(rnd[firstHostByte:]); err != nil { + return netip.Addr{}, err + } + if partialBits > 0 { // Keep the network bits in the partial byte, randomize the rest hostMask := byte(0xff >> partialBits) - ip[firstHostByte] = (ip[firstHostByte] & ^hostMask) | (byte(rng.Intn(256)) & hostMask) + ip[firstHostByte] = (ip[firstHostByte] & ^hostMask) | (rnd[firstHostByte] & hostMask) firstHostByte++ } // Randomize remaining full host bytes for i := firstHostByte; i < 16; i++ { - ip[i] = byte(rng.Intn(256)) + ip[i] = rnd[i] } // Avoid all-zeros and all-ones host parts by checking only host bits. diff --git a/management/server/types/network_test.go b/management/server/types/network_test.go index d8a06dbbc..239f72426 100644 --- a/management/server/types/network_test.go +++ b/management/server/types/network_test.go @@ -143,6 +143,34 @@ func TestAllocatePeerIPVariousCIDRs(t *testing.T) { } } +func TestAllocateIPv4InvalidPrefixes(t *testing.T) { + prefixes := []netip.Prefix{ + {}, + netip.MustParsePrefix("0.0.0.0/0"), + netip.MustParsePrefix("192.168.1.0/31"), + netip.MustParsePrefix("192.168.1.1/32"), + netip.MustParsePrefix("fd12:3456:7890:abcd::/64"), + } + + for _, prefix := range prefixes { + t.Run(prefix.String(), func(t *testing.T) { + _, err := AllocatePeerIP(prefix, nil) + assert.Error(t, err) + + _, err = AllocateRandomPeerIP(prefix) + assert.Error(t, err) + }) + } +} + +func TestAllocatePeerIPIgnoresNonIPv4TakenIPs(t *testing.T) { + prefix := netip.MustParsePrefix("192.168.1.0/29") + + ip, err := AllocatePeerIP(prefix, []netip.Addr{netip.MustParseAddr("fd12:3456:7890:abcd::1")}) + require.NoError(t, err) + assert.True(t, prefix.Contains(ip)) +} + func TestGenerateIPs(t *testing.T) { ipNet := net.IPNet{IP: net.ParseIP("100.64.0.0"), Mask: net.IPMask{255, 255, 255, 0}} ips, ipsLen := generateIPs(&ipNet, map[string]struct{}{"100.64.0.0": {}}) diff --git a/management/server/util/util.go b/management/server/util/util.go index d85b55f02..38cd3de58 100644 --- a/management/server/util/util.go +++ b/management/server/util/util.go @@ -1,5 +1,20 @@ package util +import ( + "crypto/rand" + "math/big" +) + +// RandIntn returns a uniformly distributed int in [0, n) sourced from +// crypto/rand. It panics if n <= 0 or the platform randomness source fails. +func RandIntn(n int) int { + v, err := rand.Int(rand.Reader, big.NewInt(int64(n))) + if err != nil { + panic(err) + } + return int(v.Int64()) +} + // Difference returns the elements in `a` that aren't in `b`. func Difference(a, b []string) []string { mb := make(map[string]struct{}, len(b))