diff --git a/spinifex/daemon/network.go b/spinifex/daemon/network.go index 580152a2e..504894105 100644 --- a/spinifex/daemon/network.go +++ b/spinifex/daemon/network.go @@ -160,16 +160,18 @@ func OVSIfaceID(eniId string) string { return "port-" + eniId } -// generateDevMAC creates a locally-administered unicast MAC for the dev/hostfwd NIC. -// Uses prefix 02:de:00 to distinguish from ENI MACs (02:00:00). +// generateDevMAC creates a locally-administered unicast MAC for the +// dev/hostfwd NIC. The "dev:" tag disambiguates from the mgmt NIC of the +// same instance (which shares instanceId). func generateDevMAC(instanceId string) string { - return utils.HashMAC("02:de:00", instanceId) + return utils.HashMAC("dev:" + instanceId) } -// generateMgmtMAC creates a locally-administered unicast MAC for the management NIC. -// Uses prefix 02:a0:00 to distinguish from ENI MACs (02:00:00) and dev MACs (02:de:00). +// generateMgmtMAC creates a locally-administered unicast MAC for the +// management NIC. The "mgmt:" tag disambiguates from the dev NIC of the +// same instance (which shares instanceId). func generateMgmtMAC(instanceId string) string { - return utils.HashMAC("02:a0:00", instanceId) + return utils.HashMAC("mgmt:" + instanceId) } // MgmtTapName returns the Linux TAP device name for a management NIC. diff --git a/spinifex/daemon/network_test.go b/spinifex/daemon/network_test.go index 7d6b2222f..58ba337f3 100644 --- a/spinifex/daemon/network_test.go +++ b/spinifex/daemon/network_test.go @@ -2,6 +2,7 @@ package daemon import ( "fmt" + "net" "os" "strings" "testing" @@ -194,12 +195,19 @@ func TestGenerateDevMAC(t *testing.T) { {"i-ghi789"}, } - // All MACs should be unique and have the 02:de:v0 prefix + // All MACs must be valid locally-administered unicast and unique. First + // octet is hash-derived (not the literal class prefix of the old impl). seen := make(map[string]bool) for _, tt := range tests { mac := generateDevMAC(tt.instanceId) - if !strings.HasPrefix(mac, "02:de:00:") { - t.Errorf("generateDevMAC(%q) = %q, want prefix '02:de:00:'", tt.instanceId, mac) + hw, err := net.ParseMAC(mac) + if err != nil { + t.Errorf("generateDevMAC(%q) = %q: invalid MAC: %v", tt.instanceId, mac, err) + continue + } + if hw[0]&0x03 != 0x02 { + t.Errorf("generateDevMAC(%q) = %q: expected unicast+LAA bits, got %#x", + tt.instanceId, mac, hw[0]) } if seen[mac] { t.Errorf("generateDevMAC(%q) = %q, duplicate MAC", tt.instanceId, mac) @@ -207,6 +215,11 @@ func TestGenerateDevMAC(t *testing.T) { seen[mac] = true } + // Class separation: dev and mgmt MACs for the same instance must differ. + if generateDevMAC("i-abc123") == generateMgmtMAC("i-abc123") { + t.Error("expected dev and mgmt MACs for same instance to differ") + } + // Same input should produce same output (deterministic) mac1 := generateDevMAC("i-test123") mac2 := generateDevMAC("i-test123") @@ -456,25 +469,14 @@ func TestGenerateDevMAC_Format(t *testing.T) { } for _, id := range tests { mac := generateDevMAC(id) - // Must be 17 chars: xx:xx:xx:xx:xx:xx - if len(mac) != 17 { - t.Errorf("generateDevMAC(%q) = %q, expected 17 chars", id, mac) + hw, err := net.ParseMAC(mac) + if err != nil { + t.Errorf("generateDevMAC(%q) = %q: invalid MAC: %v", id, mac, err) + continue } - // Must start with locally-administered unicast prefix 02:de:00 - if !strings.HasPrefix(mac, "02:de:00:") { - t.Errorf("generateDevMAC(%q) = %q, expected prefix 02:de:00:", id, mac) - } - // All chars must be valid hex or colons - for i, c := range mac { - if i%3 == 2 { - if c != ':' { - t.Errorf("generateDevMAC(%q) = %q, expected ':' at pos %d", id, mac, i) - } - } else { - if (c < '0' || c > '9') && (c < 'a' || c > 'f') { - t.Errorf("generateDevMAC(%q) = %q, invalid hex char '%c' at pos %d", id, mac, c, i) - } - } + if hw[0]&0x03 != 0x02 { + t.Errorf("generateDevMAC(%q) = %q: expected unicast+LAA bits, got %#x", + id, mac, hw[0]) } } } @@ -516,11 +518,14 @@ func TestGenerateMgmtMAC(t *testing.T) { seen := make(map[string]bool) for _, id := range tests { mac := generateMgmtMAC(id) - if !strings.HasPrefix(mac, "02:a0:00:") { - t.Errorf("generateMgmtMAC(%q) = %q, want prefix '02:a0:00:'", id, mac) + hw, err := net.ParseMAC(mac) + if err != nil { + t.Errorf("generateMgmtMAC(%q) = %q: invalid MAC: %v", id, mac, err) + continue } - if len(mac) != 17 { - t.Errorf("generateMgmtMAC(%q) = %q, expected 17 chars", id, mac) + if hw[0]&0x03 != 0x02 { + t.Errorf("generateMgmtMAC(%q) = %q: expected unicast+LAA bits, got %#x", + id, mac, hw[0]) } if seen[mac] { t.Errorf("generateMgmtMAC(%q) = %q, duplicate MAC", id, mac) diff --git a/spinifex/handlers/ec2/vpc/eni.go b/spinifex/handlers/ec2/vpc/eni.go index aefa76495..a987c380f 100644 --- a/spinifex/handlers/ec2/vpc/eni.go +++ b/spinifex/handlers/ec2/vpc/eni.go @@ -553,7 +553,7 @@ func (s *VPCServiceImpl) eniRecordToEC2(record *ENIRecord, accountID string) *ec // generateENIMac creates a locally-administered unicast MAC address from an ENI ID. func generateENIMac(eniId string) string { - return utils.HashMAC("02:00:00", eniId) + return utils.HashMAC(eniId) } // publishENIEvent publishes an ENI lifecycle event to NATS for vpcd consumption. diff --git a/spinifex/handlers/ec2/vpc/eni_mac_test.go b/spinifex/handlers/ec2/vpc/eni_mac_test.go index 8f8ac9da3..ee0444358 100644 --- a/spinifex/handlers/ec2/vpc/eni_mac_test.go +++ b/spinifex/handlers/ec2/vpc/eni_mac_test.go @@ -1,10 +1,11 @@ package handlers_ec2_vpc import ( - "regexp" + "net" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestGenerateENIMac_Deterministic(t *testing.T) { @@ -21,12 +22,18 @@ func TestGenerateENIMac_DifferentInputs(t *testing.T) { func TestGenerateENIMac_LocallyAdministered(t *testing.T) { mac := generateENIMac("eni-test123") - // Must start with 02:00:00 (locally-administered unicast) - assert.Regexp(t, regexp.MustCompile(`^02:00:00:[0-9a-f]{2}:[0-9a-f]{2}:[0-9a-f]{2}$`), mac) + hw, err := net.ParseMAC(mac) + require.NoError(t, err) + // IEEE 802 reserved bits on first octet: bit0=0 unicast, bit1=1 LAA. + assert.Equal(t, byte(0x02), hw[0]&0x03) } func TestGenerateENIMac_EmptyString(t *testing.T) { mac := generateENIMac("") - // Hash of empty string = 0, so all three octets should be 00 - assert.Equal(t, "02:00:00:00:00:00", mac) + hw, err := net.ParseMAC(mac) + require.NoError(t, err) + assert.Equal(t, byte(0x02), hw[0]&0x03) + // New impl hashes the prefix-and-id; empty input no longer collapses to + // the degenerate 02:00:00:00:00:00 of the old 24-bit impl. + assert.NotEqual(t, "02:00:00:00:00:00", hw.String()) } diff --git a/spinifex/handlers/ec2/vpc/eni_test.go b/spinifex/handlers/ec2/vpc/eni_test.go index 0cce998f5..e7af80553 100644 --- a/spinifex/handlers/ec2/vpc/eni_test.go +++ b/spinifex/handlers/ec2/vpc/eni_test.go @@ -2,6 +2,7 @@ package handlers_ec2_vpc import ( "encoding/json" + "net" "testing" "time" @@ -281,7 +282,9 @@ func TestDetachENI(t *testing.T) { func TestGenerateENIMac(t *testing.T) { mac := generateENIMac("eni-test123") - assert.Regexp(t, `^02:00:00:[0-9a-f]{2}:[0-9a-f]{2}:[0-9a-f]{2}$`, mac) + hw, err := net.ParseMAC(mac) + require.NoError(t, err) + assert.Equal(t, byte(0x02), hw[0]&0x03) // Same input produces same MAC assert.Equal(t, mac, generateENIMac("eni-test123")) diff --git a/spinifex/services/vpcd/dhcp_manager_test.go b/spinifex/services/vpcd/dhcp_manager_test.go index d43860341..290101cda 100644 --- a/spinifex/services/vpcd/dhcp_manager_test.go +++ b/spinifex/services/vpcd/dhcp_manager_test.go @@ -126,8 +126,9 @@ func TestDHCPManager_HWAddrDerivedFromClientID(t *testing.T) { require.Empty(t, reply.Error) lease, ok := fake.HeldLease("eni-mac") require.True(t, ok) - // generateMAC uses the 02:00:00 locally-administered unicast prefix. - require.Equal(t, "02:00:00", lease.HWAddr.String()[:8]) + // generateMAC produces a locally-administered unicast MAC. First octet + // is hash-derived; assert the IEEE 802 reserved bits. + require.Equal(t, byte(0x02), lease.HWAddr[0]&0x03) } func TestDHCPManager_HWAddrExplicitlySuppliedIsUsed(t *testing.T) { diff --git a/spinifex/services/vpcd/helpers_test.go b/spinifex/services/vpcd/helpers_test.go index e8eb78300..b66e1d861 100644 --- a/spinifex/services/vpcd/helpers_test.go +++ b/spinifex/services/vpcd/helpers_test.go @@ -1,6 +1,7 @@ package vpcd import ( + "net" "testing" "github.com/stretchr/testify/assert" @@ -25,7 +26,13 @@ func TestSubnetGateway_IPv6(t *testing.T) { func TestGenerateMAC_EmptyString(t *testing.T) { mac := generateMAC("") - assert.Equal(t, "02:00:00:00:00:00", mac) + hw, err := net.ParseMAC(mac) + require.NoError(t, err) + // LAA + unicast bits enforced even for empty input. + assert.Equal(t, byte(0x02), hw[0]&0x03) + // Old 24-bit impl produced 02:00:00:00:00:00 (all-zero hash region) for + // empty input. New impl hashes the prefix-and-id, so output must differ. + assert.NotEqual(t, "02:00:00:00:00:00", hw.String()) } // --- dnsServer --- diff --git a/spinifex/services/vpcd/topology.go b/spinifex/services/vpcd/topology.go index f700bfa04..f0a98fb26 100644 --- a/spinifex/services/vpcd/topology.go +++ b/spinifex/services/vpcd/topology.go @@ -11,6 +11,7 @@ import ( "github.com/mulgadc/spinifex/spinifex/services/vpcd/nbdb" "github.com/mulgadc/spinifex/spinifex/types" + "github.com/mulgadc/spinifex/spinifex/utils" "github.com/nats-io/nats.go" ) @@ -1407,15 +1408,11 @@ func subnetGateway(cidr string) (string, int, error) { return gw.String(), ones, nil } -// generateMAC creates a deterministic MAC address from a resource ID. -// Uses the locally-administered unicast prefix 02:00:00. +// generateMAC creates a deterministic locally-administered unicast MAC +// from a resource ID via utils.HashMAC. Inputs are vpcd-owned ids +// (subnet-..., gw-vpc-..., eni-...) which are unique on their own. func generateMAC(resourceID string) string { - // Simple hash: use first 6 hex chars of resource ID after the prefix - h := uint32(0) - for _, c := range resourceID { - h = h*31 + uint32(c) // #nosec G115 -- intentional overflow for hashing - } - return fmt.Sprintf("02:00:00:%02x:%02x:%02x", (h>>16)&0xff, (h>>8)&0xff, h&0xff) + return utils.HashMAC(resourceID) } // respond sends a simple JSON response to a NATS request. diff --git a/spinifex/services/vpcd/topology_test.go b/spinifex/services/vpcd/topology_test.go index 1fb3a6c08..927ff8f50 100644 --- a/spinifex/services/vpcd/topology_test.go +++ b/spinifex/services/vpcd/topology_test.go @@ -3,6 +3,8 @@ package vpcd import ( "context" "encoding/json" + "fmt" + "net" "strings" "testing" @@ -409,9 +411,14 @@ func TestGenerateMAC(t *testing.T) { mac1 := generateMAC("subnet-aaa") mac2 := generateMAC("subnet-bbb") - // Must start with locally-administered unicast prefix - if mac1[:8] != "02:00:00" { - t.Errorf("expected prefix 02:00:00, got %s", mac1[:8]) + // Must be a valid locally-administered unicast MAC. First octet is + // hash-derived, not the literal "02:00:00" of the old 24-bit impl. + hw, err := net.ParseMAC(mac1) + if err != nil { + t.Fatalf("invalid MAC %q: %v", mac1, err) + } + if hw[0]&0x03 != 0x02 { + t.Errorf("expected unicast+LAA bits on first octet, got %#x", hw[0]) } // Different inputs produce different MACs @@ -424,6 +431,21 @@ func TestGenerateMAC(t *testing.T) { if mac1 != mac1b { t.Error("expected deterministic MAC") } + + // Output must use full 46-bit hash space, not the old 24-bit space that + // pinned the top 3 octets to "02:00:00". Sweep many inputs and assert + // the second octet varies — old impl always emitted 0x00 there. + seen := map[byte]struct{}{} + for i := range 1000 { + hw, err := net.ParseMAC(generateMAC(fmt.Sprintf("subnet-%d", i))) + if err != nil { + t.Fatalf("invalid MAC: %v", err) + } + seen[hw[1]] = struct{}{} + } + if len(seen) <= 1 { + t.Errorf("expected second octet to vary across inputs, got %d distinct values", len(seen)) + } } func TestTopologyHandler_CreatePort(t *testing.T) { diff --git a/spinifex/utils/utils.go b/spinifex/utils/utils.go index 8683efb51..0d6067856 100644 --- a/spinifex/utils/utils.go +++ b/spinifex/utils/utils.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "crypto/rand" + "crypto/sha256" "encoding/hex" "encoding/json" "encoding/xml" @@ -11,6 +12,7 @@ import ( "fmt" "io" "log/slog" + "net" "net/http" "os" "os/signal" @@ -302,15 +304,22 @@ func humanBytes(b uint64) string { return fmt.Sprintf("%.1f %ciB", float64(b)/float64(div), "KMGTPEZY"[exp]) } -// HashMAC generates a deterministic locally-administered unicast MAC address. -// prefix is the first 3 octets (e.g. "02:00:00"), id is hashed to produce the -// remaining 3 octets. -func HashMAC(prefix, id string) string { - h := uint32(0) - for _, c := range id { - h = h*31 + uint32(c) // #nosec G115 -- intentional overflow for hashing - } - return fmt.Sprintf("%s:%02x:%02x:%02x", prefix, (h>>16)&0xff, (h>>8)&0xff, h&0xff) +// HashMAC returns a deterministic locally-administered unicast MAC derived +// from id. Output is "02:xx:xx:xx:xx:xx" with 46 bits of SHA-256-derived +// entropy in the low bits of the first octet plus the next 5 octets, and +// the IEEE 802 reserved bits forced (bit0=0 unicast, bit1=1 LAA). +// +// id must be globally unique across all MACs the deployment produces. No +// per-deployment salt is mixed in. Callers that share a base id between +// resource classes (e.g. an instance's dev vs mgmt NIC) must compose a +// class tag into id ("dev:"+instanceID, "mgmt:"+instanceID). Hardcoded or +// low-entropy ids will collide. +func HashMAC(id string) string { + sum := sha256.Sum256([]byte(id)) + b := make([]byte, 6) + copy(b, sum[:6]) + b[0] = (b[0] & 0xFC) | 0x02 // bit0=0 unicast, bit1=1 locally-administered + return net.HardwareAddr(b).String() } func dirExists(path string) bool { diff --git a/spinifex/utils/utils_test.go b/spinifex/utils/utils_test.go index 9230a7774..4435dc521 100644 --- a/spinifex/utils/utils_test.go +++ b/spinifex/utils/utils_test.go @@ -6,6 +6,7 @@ import ( "crypto/x509" "encoding/hex" "fmt" + "net" "net/http" "net/http/httptest" "os" @@ -1003,15 +1004,74 @@ func TestReadPidFileFrom_EmptyDir(t *testing.T) { assert.Error(t, err) } -func TestHashMAC(t *testing.T) { - mac := HashMAC("02:00:00", "test-id") - assert.Regexp(t, `^02:00:00:[0-9a-f]{2}:[0-9a-f]{2}:[0-9a-f]{2}$`, mac) +func TestHashMAC_FirstOctetVariesByInput(t *testing.T) { + // Old 24-bit impl pinned first octet to literal prefix byte (always + // 0x02). New 46-bit impl derives first octet from the hash, so it + // spans many values across distinct inputs. + seen := map[byte]struct{}{} + for i := range 1000 { + hw, err := net.ParseMAC(HashMAC(fmt.Sprintf("id-%d", i))) + require.NoError(t, err) + seen[hw[0]] = struct{}{} + } + assert.Greater(t, len(seen), 10, "first octet should vary across inputs") +} - // Deterministic: same input → same output - assert.Equal(t, mac, HashMAC("02:00:00", "test-id")) +func TestHashMAC_LocallyAdministered(t *testing.T) { + // First octet must always have bit0=0 (unicast) and bit1=1 (LAA) per + // IEEE 802 reserved bits, regardless of hash output. + for i := range 1000 { + hw, err := net.ParseMAC(HashMAC(fmt.Sprintf("id-%d", i))) + require.NoError(t, err) + assert.Equal(t, byte(0x02), hw[0]&0x03, + "first octet %#x must be unicast+LAA", hw[0]) + } +} - // Different input → different output - assert.NotEqual(t, mac, HashMAC("02:00:00", "other-id")) +func TestHashMAC_Determinism(t *testing.T) { + // Same id must produce the same MAC across calls and across goroutines. + // Reconcilers across nodes rely on this — non-determinism would + // split-brain DHCP server_mac vs LRP MAC. + const id = "i-abc123" + want := HashMAC(id) + + for range 100 { + assert.Equal(t, want, HashMAC(id)) + } + + var wg sync.WaitGroup + for range 50 { + wg.Go(func() { + for range 100 { + assert.Equal(t, want, HashMAC(id)) + } + }) + } + wg.Wait() +} + +func TestHashMAC_IdSeparates(t *testing.T) { + // Different ids must yield different MACs. + a := HashMAC("subnet-aaaaaaaaaaaaaaaaa") + b := HashMAC("subnet-bbbbbbbbbbbbbbbbb") + assert.NotEqual(t, a, b) +} + +func TestHashMAC_Distribution(t *testing.T) { + // 100k random ids — 0 collisions expected. 46-bit space, birthday- + // paradox 1% at ~1.2M; 100k is well below. + if testing.Short() { + t.Skip("skipping distribution test in -short mode") + } + seen := make(map[string]string, 100_000) + for range 100_000 { + id := GenerateResourceID("res") + mac := HashMAC(id) + if prev, ok := seen[mac]; ok { + t.Fatalf("collision on %s: id=%s prev=%s", mac, id, prev) + } + seen[mac] = id + } } // writeTempImage writes content to a file named name under t.TempDir() and