mirror of
https://github.com/mulgadc/spinifex.git
synced 2026-10-22 12:16:52 +00:00
fix: rewrite utils.HashMAC to SHA-256 / 46-bit output
The old 24-bit rolling hash with literal class prefix had birthday-paradox collision probability of 1% at ~580 ids and 50% at 4096 — well within the range of a single deployment's resource population (LRPs, ENIs, dev/mgmt NICs). On a shared L2 segment, two clusters could trivially produce duplicate MACs, breaking neighbour discovery and DHCP. Switch HashMAC to SHA-256 across the entire input, taking the low 6 bytes and forcing the IEEE 802 reserved bits (bit0=0 unicast, bit1=1 LAA) on the first octet. Output entropy goes from 24 bits to 46 bits — 1% collision boundary moves from ~580 to ~1.2M ids. Drop the redundant prefix arg: it was no longer the literal output prefix under the new scheme, just folded into the hash input. Class disambiguation moves to the caller, where it's explicit. Three of four callers had nothing to disambiguate (eni-, subnet-, gw-vpc- ids are already unique); dev and mgmt NICs share instance.ID and now compose "dev:" / "mgmt:" tags. Also consolidate vpcd.generateMAC, which was a local duplicate of the broken algorithm rather than a HashMAC wrapper, into a HashMAC delegation. Plan: docs/development/bugs/vpcd-mac-collisions-shared-l2.md Bead: mulga-siv-27
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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())
|
||||
}
|
||||
|
||||
@@ -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"))
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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 ---
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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) {
|
||||
|
||||
+18
-9
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user