diff --git a/cmd/dhcptest/main.go b/cmd/dhcptest/main.go index 7e7a3ed03..941e07f3e 100644 --- a/cmd/dhcptest/main.go +++ b/cmd/dhcptest/main.go @@ -6,7 +6,7 @@ // Usage (must run as root — needs AF_PACKET): // // sudo dhcptest --iface=br-wan -// sudo dhcptest --iface=br-wan --mac=02:00:00:7c:b7:b9 --promisc +// sudo dhcptest --iface=br-wan --mac=02:54:df:71:22:5c --promisc // sudo dhcptest --iface=br-wan --spinifex-id=gateway-wan1 package main @@ -21,8 +21,8 @@ import ( "strings" "time" - "github.com/insomniacslk/dhcp/dhcpv4" "github.com/insomniacslk/dhcp/dhcpv4/nclient4" + "github.com/mulgadc/spinifex/spinifex/network/external/dhcp" ) func main() { @@ -44,9 +44,11 @@ func main() { switch { case *spinifexID != "": - hwAddr, err = net.ParseMAC(generateMAC(*spinifexID)) + // vpcd's own derivation, so the probe leases under the MAC the daemon + // would have used for this id. + hwAddr, err = dhcp.DeriveMAC(*spinifexID) if err != nil { - slog.Error("Could not parse generated MAC", "spinifex-id", *spinifexID, "error", err) + slog.Error("Could not derive MAC", "spinifex-id", *spinifexID, "error", err) os.Exit(1) } fmt.Printf("spinifex virtual MAC for %q: %s\n", *spinifexID, hwAddr) @@ -109,17 +111,9 @@ func main() { } defer client.Close() - // Identity modifiers — client-id / hostname / vendor-class for upstream lease tagging. - var mods []dhcpv4.Modifier - if cid != "" { - mods = append(mods, dhcpv4.WithOption(dhcpv4.OptClientIdentifier([]byte(cid)))) - } - if *hostname != "" { - mods = append(mods, dhcpv4.WithOption(dhcpv4.OptHostName(*hostname))) - } - if *vendorClass != "" { - mods = append(mods, dhcpv4.WithOption(dhcpv4.OptClassIdentifier(*vendorClass))) - } + // Identity modifiers — client-id / hostname / vendor-class for upstream lease + // tagging. Built by the vpcd code path so the probe cannot drift from it. + mods := dhcp.IdentityModifiers(cid, *hostname, *vendorClass, hwAddr) fmt.Println("Sending DHCP Discover...") start := time.Now() @@ -153,16 +147,6 @@ func (slogPrintfer) Printf(format string, v ...any) { slog.Info(fmt.Sprintf(format, v...), "component", "nclient4") } -// generateMAC is the same deterministic hash used by vpcd's topology.go. -// It produces a locally-administered MAC (02:00:00:xx:xx:xx) from a resource ID. -func generateMAC(resourceID string) string { - h := uint32(0) - for _, c := range resourceID { - h = h*31 + uint32(c) - } - return fmt.Sprintf("02:00:00:%02x:%02x:%02x", (h>>16)&0xff, (h>>8)&0xff, h&0xff) -} - func ifaceMAC(name string) (net.HardwareAddr, error) { ifc, err := net.InterfaceByName(name) if err != nil { diff --git a/spinifex/network/external/dhcp/nclient4.go b/spinifex/network/external/dhcp/nclient4.go index 4a17d1d0f..63a5be355 100644 --- a/spinifex/network/external/dhcp/nclient4.go +++ b/spinifex/network/external/dhcp/nclient4.go @@ -33,6 +33,30 @@ func NewNClient4(timeout time.Duration) *NClient4Client { return &NClient4Client{timeout: timeout} } +// socketTimeoutGrace keeps the socket deadline strictly behind the caller's, so +// ctx.Done() is what ends a timed-out attempt. Matching the two exactly makes +// them expire together and the reported error becomes a coin toss between +// context.DeadlineExceeded and nclient4's ErrNoResponse, which reads as a +// packet-matching fault rather than the plain timeout it is. +const socketTimeoutGrace = time.Second + +// socketTimeout is the read deadline handed to nclient4 for one DORA. It must +// track the caller's deadline: nclient4 races its own deadline against ctx and +// the shorter one ends the attempt, so a fixed value below the caller's budget +// silently truncates every window in Manager's retransmission schedule. +func (c *NClient4Client) socketTimeout(ctx context.Context) time.Duration { + deadline, ok := ctx.Deadline() + if !ok { + return c.timeout + } + // A deadline already in the past is left to ctx.Done(); nclient4 rejects a + // non-positive timeout, so keep the fallback rather than pass it through. + if remaining := time.Until(deadline); remaining > 0 { + return remaining + socketTimeoutGrace + } + return c.timeout +} + func (c *NClient4Client) Acquire(ctx context.Context, req AcquireRequest) (*Lease, error) { if req.Bridge == "" { return nil, fmt.Errorf("dhcp acquire: bridge is required") @@ -73,7 +97,7 @@ func (c *NClient4Client) Acquire(ctx context.Context, req AcquireRequest) (*Leas client, err := nclient4.New(req.Bridge, nclient4.WithHWAddr(req.HWAddr), - nclient4.WithTimeout(c.timeout), + nclient4.WithTimeout(c.socketTimeout(ctx)), nclient4.WithRetry(nclient4Retries), ) if err != nil { @@ -83,7 +107,7 @@ func (c *NClient4Client) Acquire(ctx context.Context, req AcquireRequest) (*Leas // Broadcast flag forces ff:ff:ff:ff:ff:ff destination — without it the // server unicasts to the derived chaddr MAC which the NIC drops in hw. - mods := append(identityModifiers(req.ClientID, req.Hostname, req.VendorClass), dhcpv4.WithBroadcast(true)) + mods := append(IdentityModifiers(req.ClientID, req.Hostname, req.VendorClass, req.HWAddr), dhcpv4.WithBroadcast(true)) lease, err := client.Request(ctx, mods...) if err != nil { return nil, fmt.Errorf("dhcp DORA on %s (client=%s): %w", req.Bridge, req.ClientID, err) @@ -113,7 +137,7 @@ func (c *NClient4Client) Renew(ctx context.Context, lease *Lease) (*Lease, error client, err := nclient4.New(lease.Bridge, nclient4.WithHWAddr(lease.HWAddr), - nclient4.WithTimeout(c.timeout), + nclient4.WithTimeout(c.socketTimeout(ctx)), nclient4.WithRetry(nclient4Retries), ) if err != nil { @@ -122,7 +146,7 @@ func (c *NClient4Client) Renew(ctx context.Context, lease *Lease) (*Lease, error defer func() { _ = client.Close() }() renewed, err := client.Renew(ctx, nclient4Lease, - identityModifiers(lease.ClientID, lease.Hostname, lease.VendorClass)...) + IdentityModifiers(lease.ClientID, lease.Hostname, lease.VendorClass, lease.HWAddr)...) if err != nil { return nil, fmt.Errorf("dhcp renew on %s (client=%s): %w", lease.Bridge, lease.ClientID, err) } @@ -166,8 +190,10 @@ func (c *NClient4Client) Release(_ context.Context, lease *Lease) error { } defer func() { _ = client.Close() }() + // The server matches a RELEASE to a lease by client-id, so this must encode + // identically to the DISCOVER/REQUEST that took the lease out. if err := client.Release(nclient4Lease, - dhcpv4.WithOption(dhcpv4.OptClientIdentifier([]byte(lease.ClientID)))); err != nil { + dhcpv4.WithOption(clientIDOption(lease.ClientID, lease.HWAddr))); err != nil { return fmt.Errorf("dhcp release on %s (client=%s): %w", lease.Bridge, lease.ClientID, err) } return nil @@ -184,20 +210,49 @@ func DeriveMAC(clientID string) (net.HardwareAddr, error) { return hw, nil } -// identityModifiers builds the three identifying DHCP options set on every +// defaultVendorClass marks a lease as ours in the upstream lease table when the +// caller supplies nothing more specific. +const defaultVendorClass = "mulga-spinifex" + +// clientIDOption encodes option 61 as RFC 2132 §9.14 requires: a type byte +// followed by the identifier. Ethernet (type 1) paired with the chaddr lets the +// server bind the lease to a hardware address, so it lists with a real MAC. +// +// Sending a bare string instead makes the server read its first byte as the +// hardware type — "dhcp-gw-lrp-vpc-x" arrives as hardware-type 100 ('d') with +// the identifier truncated to "hcp-gw-lrp-vpc-x" — and since that is not +// Ethernet, no hardware address is recorded against the lease at all. +func clientIDOption(clientID string, hwAddr net.HardwareAddr) dhcpv4.Option { + if len(hwAddr) == 6 { + return dhcpv4.OptClientIdentifier(append([]byte{0x01}, hwAddr...)) + } + // Type 0 marks an identifier that is not a hardware address (RFC 4361 §6.1). + return dhcpv4.OptClientIdentifier(append([]byte{0x00}, clientID...)) +} + +// IdentityModifiers builds the three identifying DHCP options set on every // outbound message: option 61 (client-id), option 12 (hostname), option 60 -// (vendor class). -func identityModifiers(clientID, hostname, vendorClass string) []dhcpv4.Modifier { +// (vendor class). Hostname and vendor class carry the human-readable identity, +// since option 61 encodes the chaddr rather than the client-id string; without +// them a lease is unattributable in the upstream table. +// +// Exported so the dhcptest probe puts the same bytes on the wire as vpcd does; +// a probe that builds its own options can only confirm its own behaviour. +func IdentityModifiers(clientID, hostname, vendorClass string, hwAddr net.HardwareAddr) []dhcpv4.Modifier { var mods []dhcpv4.Modifier if clientID != "" { - mods = append(mods, dhcpv4.WithOption(dhcpv4.OptClientIdentifier([]byte(clientID)))) + mods = append(mods, dhcpv4.WithOption(clientIDOption(clientID, hwAddr))) + } + if hostname == "" { + hostname = clientID } if hostname != "" { mods = append(mods, dhcpv4.WithOption(dhcpv4.OptHostName(hostname))) } - if vendorClass != "" { - mods = append(mods, dhcpv4.WithOption(dhcpv4.OptClassIdentifier(vendorClass))) + if vendorClass == "" { + vendorClass = defaultVendorClass } + mods = append(mods, dhcpv4.WithOption(dhcpv4.OptClassIdentifier(vendorClass))) return mods } diff --git a/spinifex/network/external/dhcp/nclient4_internal_test.go b/spinifex/network/external/dhcp/nclient4_internal_test.go new file mode 100644 index 000000000..a9c3e9a1a --- /dev/null +++ b/spinifex/network/external/dhcp/nclient4_internal_test.go @@ -0,0 +1,130 @@ +package dhcp + +import ( + "bytes" + "context" + "net" + "testing" + "time" + + "github.com/insomniacslk/dhcp/dhcpv4" +) + +// socketTimeout must never cap the caller's budget, because nclient4 ends an +// attempt on whichever of the two deadlines fires first. +func TestSocketTimeoutTracksContextDeadline(t *testing.T) { + c := NewNClient4(5 * time.Second) + + t.Run("no deadline falls back to the configured timeout", func(t *testing.T) { + if got := c.socketTimeout(context.Background()); got != 5*time.Second { + t.Fatalf("socketTimeout = %v, want 5s", got) + } + }) + + t.Run("longer deadline is honoured rather than capped", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 32*time.Second) + defer cancel() + got := c.socketTimeout(ctx) + if got <= 5*time.Second { + t.Fatalf("socketTimeout = %v, want the remaining ~32s, not the 5s fallback", got) + } + // Strictly beyond the caller's deadline so ctx.Done() reports the timeout. + if got <= 32*time.Second { + t.Fatalf("socketTimeout = %v, want more than the remaining 32s", got) + } + }) + + t.Run("shorter deadline shortens the read", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + got := c.socketTimeout(ctx) + if got <= time.Second || got > 2*time.Second { + t.Fatalf("socketTimeout = %v, want just beyond the remaining 1s", got) + } + }) + + t.Run("expired deadline yields a positive timeout", func(t *testing.T) { + ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancel() + // nclient4 rejects a non-positive read deadline; ctx.Done() ends the + // attempt immediately regardless of what is passed here. + if got := c.socketTimeout(ctx); got != 5*time.Second { + t.Fatalf("socketTimeout = %v, want the 5s fallback", got) + } + }) +} + +// Option 61 carries a leading hardware-type byte. Omitting it makes the server +// consume the identifier's first character as the type, which is how leases end +// up in the upstream table with no hardware address at all. +func TestClientIDOptionCarriesHardwareType(t *testing.T) { + hw, err := net.ParseMAC("02:0c:46:55:cf:ed") + if err != nil { + t.Fatalf("parse mac: %v", err) + } + + t.Run("ethernet address is typed 1 and sent verbatim", func(t *testing.T) { + got := clientIDOption("dhcp-gw-lrp-vpc-abc", hw).Value.ToBytes() + want := append([]byte{0x01}, hw...) + if !bytes.Equal(got, want) { + t.Fatalf("client-id = %v, want %v", got, want) + } + }) + + t.Run("without a hardware address the id is typed 0", func(t *testing.T) { + got := clientIDOption("eni-1234", nil).Value.ToBytes() + want := append([]byte{0x00}, "eni-1234"...) + if !bytes.Equal(got, want) { + t.Fatalf("client-id = %v, want %v", got, want) + } + }) + + t.Run("no leading byte is mistaken for a hardware type", func(t *testing.T) { + // 'd' is 0x64, so the unfixed encoding announced hardware type 100. + got := clientIDOption("dhcp-gw-lrp-vpc-abc", hw).Value.ToBytes() + if got[0] == 'd' { + t.Fatalf("client-id starts with the identifier text, not a type byte: %v", got) + } + }) +} + +// The readable identity moves to options 12 and 60 once option 61 carries the +// chaddr, so a lease stays attributable in the upstream table. +func TestIdentityModifiersAlwaysCarryReadableIdentity(t *testing.T) { + hw, err := net.ParseMAC("02:0c:46:55:cf:ed") + if err != nil { + t.Fatalf("parse mac: %v", err) + } + + build := func(clientID, hostname, vendorClass string) *dhcpv4.DHCPv4 { + msg, err := dhcpv4.New(IdentityModifiers(clientID, hostname, vendorClass, hw)...) + if err != nil { + t.Fatalf("build message: %v", err) + } + return msg + } + + t.Run("hostname defaults to the client-id", func(t *testing.T) { + msg := build("dhcp-gw-lrp-vpc-abc", "", "") + if got := msg.HostName(); got != "dhcp-gw-lrp-vpc-abc" { + t.Fatalf("hostname = %q, want the client-id", got) + } + }) + + t.Run("vendor class defaults so leases are identifiable as ours", func(t *testing.T) { + msg := build("eni-1234", "", "") + if got := msg.ClassIdentifier(); got != defaultVendorClass { + t.Fatalf("vendor class = %q, want %q", got, defaultVendorClass) + } + }) + + t.Run("explicit values win over the defaults", func(t *testing.T) { + msg := build("eni-1234", "host-a", "acme") + if got := msg.HostName(); got != "host-a" { + t.Fatalf("hostname = %q, want host-a", got) + } + if got := msg.ClassIdentifier(); got != "acme" { + t.Fatalf("vendor class = %q, want acme", got) + } + }) +}