diff --git a/internal/dhcpsvc/config.go b/internal/dhcpsvc/config.go index 9791151bb..c3eaab619 100644 --- a/internal/dhcpsvc/config.go +++ b/internal/dhcpsvc/config.go @@ -58,22 +58,25 @@ func (conf *Config) Validate() (err error) { } errs := []error{ - validate.NotNegative("ICMPTimeout", conf.ICMPTimeout), + validate.NotNegative("conf.ICMPTimeout", conf.ICMPTimeout), + validate.NotEmpty("conf.DBFilePath", conf.DBFilePath), + validate.NotNil("conf.Logger", conf.Logger), + validate.NotNilInterface("conf.NetworkDeviceManager", conf.NetworkDeviceManager), } err = netutil.ValidateDomainName(conf.LocalDomainName) if err != nil { - errs = append(errs, fmt.Errorf("LocalDomainName: %w", err)) + errs = append(errs, fmt.Errorf("conf.LocalDomainName: %w", err)) } // This is a best-effort check for the file accessibility. The file will be // checked again when it is opened later. if _, err = os.Stat(conf.DBFilePath); err != nil && !errors.Is(err, os.ErrNotExist) { - errs = append(errs, fmt.Errorf("DBFilePath %q: %w", conf.DBFilePath, err)) + errs = append(errs, fmt.Errorf("conf.DBFilePath %q: %w", conf.DBFilePath, err)) } if len(conf.Interfaces) == 0 { - err = fmt.Errorf("interfaces: %w", errors.ErrEmptyValue) + err = fmt.Errorf("conf.Interfaces: %w", errors.ErrEmptyValue) errs = append(errs, err) return errors.Join(errs...) @@ -81,7 +84,7 @@ func (conf *Config) Validate() (err error) { for _, iface := range slices.Sorted(maps.Keys(conf.Interfaces)) { ifaceConf := conf.Interfaces[iface] - errs = validate.Append(errs, iface, ifaceConf) + errs = validate.Append(errs, "conf.Interfaces."+iface, ifaceConf) } return errors.Join(errs...) @@ -106,7 +109,7 @@ func (ic *InterfaceConfig) Validate() (err error) { } return errors.Join( - errors.Annotate(ic.IPv4.Validate(), "ipv4: %w"), - errors.Annotate(ic.IPv6.Validate(), "ipv6: %w"), + errors.Annotate(ic.IPv4.Validate(), "IPv4: %w"), + errors.Annotate(ic.IPv6.Validate(), "IPv6: %w"), ) } diff --git a/internal/dhcpsvc/config_test.go b/internal/dhcpsvc/config_test.go index dd59c35f5..6e3cfc6cb 100644 --- a/internal/dhcpsvc/config_test.go +++ b/internal/dhcpsvc/config_test.go @@ -8,183 +8,256 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/dhcpsvc" "github.com/AdguardTeam/golibs/testutil" - "github.com/AdguardTeam/golibs/timeutil" ) -// TODO(e.burkov): Split into several tests for each part of the configuration. +func TestIPv4Config_Validate(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + conf *dhcpsvc.IPv4Config + wantErrMsg string + }{{ + name: "nil", + conf: nil, + wantErrMsg: "no value", + }, { + name: "disabled", + conf: &dhcpsvc.IPv4Config{Enabled: false}, + wantErrMsg: "", + }, { + name: "nil_clock", + conf: &dhcpsvc.IPv4Config{ + Enabled: true, + Clock: nil, + GatewayIP: testIPv4Conf.GatewayIP, + SubnetMask: testIPv4Conf.SubnetMask, + RangeStart: testIPv4Conf.RangeStart, + RangeEnd: testIPv4Conf.RangeEnd, + LeaseDuration: testIPv4Conf.LeaseDuration, + }, + wantErrMsg: "clock: no value", + }, { + name: "bad_lease_duration", + conf: &dhcpsvc.IPv4Config{ + Enabled: true, + Clock: testIPv4Conf.Clock, + GatewayIP: testIPv4Conf.GatewayIP, + SubnetMask: testIPv4Conf.SubnetMask, + RangeStart: testIPv4Conf.RangeStart, + RangeEnd: testIPv4Conf.RangeEnd, + LeaseDuration: 0, + }, + wantErrMsg: "lease duration: not positive: 0s", + }, { + name: "bad_gateway_ip", + conf: &dhcpsvc.IPv4Config{ + Enabled: true, + Clock: testIPv4Conf.Clock, + GatewayIP: netip.MustParseAddr("2001:db8::1"), + SubnetMask: testIPv4Conf.SubnetMask, + RangeStart: testIPv4Conf.RangeStart, + RangeEnd: testIPv4Conf.RangeEnd, + LeaseDuration: testIPv4Conf.LeaseDuration, + }, + wantErrMsg: "gateway ip 2001:db8::1 must be a valid ipv4" + "\n" + + "range start 192.168.0.2 is not within 2001:db8::1/24", + }, { + name: "bad_subnet_mask", + conf: &dhcpsvc.IPv4Config{ + Enabled: true, + Clock: testIPv4Conf.Clock, + GatewayIP: testIPv4Conf.GatewayIP, + SubnetMask: netip.MustParseAddr("2001:db8::1"), + RangeStart: testIPv4Conf.RangeStart, + RangeEnd: testIPv4Conf.RangeEnd, + LeaseDuration: testIPv4Conf.LeaseDuration, + }, + wantErrMsg: "subnet mask 2001:db8::1 must be a valid ipv4 cidr mask", + }, { + name: "bad_range_start", + conf: &dhcpsvc.IPv4Config{ + Enabled: true, + Clock: testIPv4Conf.Clock, + GatewayIP: testIPv4Conf.GatewayIP, + SubnetMask: testIPv4Conf.SubnetMask, + RangeStart: netip.MustParseAddr("2001:db8::1"), + RangeEnd: testIPv4Conf.RangeEnd, + LeaseDuration: testIPv4Conf.LeaseDuration, + }, + wantErrMsg: "range start 2001:db8::1 must be a valid ipv4" + "\n" + + "range start 2001:db8::1 is not within 192.168.0.1/24" + "\n" + + "invalid ip range: 2001:db8::1 and 192.168.0.254 must be within the same address family", + }, { + name: "bad_range_end", + conf: &dhcpsvc.IPv4Config{ + Enabled: true, + Clock: testIPv4Conf.Clock, + GatewayIP: testIPv4Conf.GatewayIP, + SubnetMask: testIPv4Conf.SubnetMask, + RangeStart: testIPv4Conf.RangeStart, + RangeEnd: netip.MustParseAddr("2001:db8::1"), + LeaseDuration: testIPv4Conf.LeaseDuration, + }, + wantErrMsg: "range end 2001:db8::1 must be a valid ipv4" + "\n" + + "range end 2001:db8::1 is not within 192.168.0.1/24" + "\n" + + "invalid ip range: 192.168.0.2 and 2001:db8::1 must be within the same address family", + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + testutil.AssertErrorMsg(t, tc.wantErrMsg, tc.conf.Validate()) + }) + } +} + +func TestIPv6Config_Validate(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + conf *dhcpsvc.IPv6Config + wantErrMsg string + }{{ + name: "nil", + conf: nil, + wantErrMsg: "no value", + }, { + name: "disabled", + conf: &dhcpsvc.IPv6Config{Enabled: false}, + wantErrMsg: "", + }, { + name: "bad_range_start", + conf: &dhcpsvc.IPv6Config{ + Enabled: true, + RangeStart: netip.MustParseAddr("192.168.0.1"), + LeaseDuration: 1 * time.Hour, + }, + wantErrMsg: "range start 192.168.0.1 should be a valid ipv6", + }, { + name: "bad_lease_duration", + conf: &dhcpsvc.IPv6Config{ + Enabled: true, + RangeStart: netip.MustParseAddr("2001:db8::1"), + LeaseDuration: 0, + }, + wantErrMsg: "lease duration 0s must be positive", + }, { + name: "valid", + conf: &dhcpsvc.IPv6Config{ + Enabled: true, + RangeStart: netip.MustParseAddr("2001:db8::1"), + LeaseDuration: 1 * time.Hour, + }, + wantErrMsg: "", + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + testutil.AssertErrorMsg(t, tc.wantErrMsg, tc.conf.Validate()) + }) + } +} + func TestConfig_Validate(t *testing.T) { - validIPv4Conf := &dhcpsvc.IPv4Config{ - Enabled: true, - Clock: timeutil.SystemClock{}, - GatewayIP: netip.MustParseAddr("192.168.0.1"), - SubnetMask: netip.MustParseAddr("255.255.255.0"), - RangeStart: netip.MustParseAddr("192.168.0.2"), - RangeEnd: netip.MustParseAddr("192.168.0.254"), - LeaseDuration: 1 * time.Hour, - } - gwInRangeConf := &dhcpsvc.IPv4Config{ - Enabled: true, - Clock: timeutil.SystemClock{}, - GatewayIP: netip.MustParseAddr("192.168.0.100"), - SubnetMask: netip.MustParseAddr("255.255.255.0"), - RangeStart: netip.MustParseAddr("192.168.0.1"), - RangeEnd: netip.MustParseAddr("192.168.0.254"), - LeaseDuration: 1 * time.Hour, - } - badStartConf := &dhcpsvc.IPv4Config{ - Enabled: true, - Clock: timeutil.SystemClock{}, - GatewayIP: netip.MustParseAddr("192.168.0.1"), - SubnetMask: netip.MustParseAddr("255.255.255.0"), - RangeStart: netip.MustParseAddr("127.0.0.1"), - RangeEnd: netip.MustParseAddr("192.168.0.254"), - LeaseDuration: 1 * time.Hour, - } + t.Parallel() - validIPv6Conf := &dhcpsvc.IPv6Config{ - Enabled: true, - RangeStart: netip.MustParseAddr("2001:db8::1"), - LeaseDuration: 1 * time.Hour, - RAAllowSLAAC: true, - RASLAACOnly: true, + valid := &dhcpsvc.Config{ + Interfaces: testInterfaceConf, + NetworkDeviceManager: dhcpsvc.EmptyNetworkDeviceManager{}, + Logger: testLogger, + LocalDomainName: testLocalTLD, + DBFilePath: filepath.Join(t.TempDir(), "leases.json"), + ICMPTimeout: 1 * time.Second, + Enabled: true, } - leasesPath := filepath.Join(t.TempDir(), "leases.json") - testCases := []struct { name string conf *dhcpsvc.Config wantErrMsg string }{{ - name: "nil_config", + name: "disabled", + conf: &dhcpsvc.Config{Enabled: false}, + wantErrMsg: "", + }, { + name: "nil", conf: nil, wantErrMsg: "no value", }, { - name: "disabled", - conf: &dhcpsvc.Config{}, - wantErrMsg: "", - }, { - name: "empty", - conf: &dhcpsvc.Config{ - Enabled: true, - Interfaces: testInterfaceConf, - DBFilePath: leasesPath, - }, - wantErrMsg: `LocalDomainName: bad domain name "": domain name is empty`, - }, { - conf: &dhcpsvc.Config{ - Enabled: true, - LocalDomainName: testLocalTLD, - Interfaces: nil, - DBFilePath: leasesPath, - }, - name: "no_interfaces", - wantErrMsg: "interfaces: empty value", - }, { - conf: &dhcpsvc.Config{ - Enabled: true, - LocalDomainName: testLocalTLD, - Interfaces: map[string]*dhcpsvc.InterfaceConfig{ - "eth0": nil, - }, - DBFilePath: leasesPath, - }, - name: "nil_interface", - wantErrMsg: `eth0: no value`, - }, { - conf: &dhcpsvc.Config{ - Enabled: true, - LocalDomainName: testLocalTLD, - Interfaces: map[string]*dhcpsvc.InterfaceConfig{ - "eth0": { - IPv4: nil, - IPv6: &dhcpsvc.IPv6Config{Enabled: false}, - }, - }, - DBFilePath: leasesPath, - }, - name: "nil_ipv4", - wantErrMsg: `eth0: ipv4: no value`, - }, { - conf: &dhcpsvc.Config{ - Enabled: true, - LocalDomainName: testLocalTLD, - Interfaces: map[string]*dhcpsvc.InterfaceConfig{ - "eth0": { - IPv4: &dhcpsvc.IPv4Config{Enabled: false}, - IPv6: nil, - }, - }, - DBFilePath: leasesPath, - }, - name: "nil_ipv6", - wantErrMsg: `eth0: ipv6: no value`, - }, { - conf: &dhcpsvc.Config{ - Enabled: true, - Logger: testLogger, - LocalDomainName: testLocalTLD, - Interfaces: map[string]*dhcpsvc.InterfaceConfig{ - "eth0": { - IPv4: validIPv4Conf, - IPv6: validIPv6Conf, - }, - }, - DBFilePath: leasesPath, - }, name: "valid", + conf: valid, wantErrMsg: "", }, { + name: "bad_icmp_timeout", conf: &dhcpsvc.Config{ - Enabled: true, - Logger: testLogger, - LocalDomainName: testLocalTLD, - Interfaces: map[string]*dhcpsvc.InterfaceConfig{ - "eth0": { - IPv4: &dhcpsvc.IPv4Config{Enabled: false}, - IPv6: &dhcpsvc.IPv6Config{Enabled: false}, - }, - }, - DBFilePath: leasesPath, + Interfaces: valid.Interfaces, + NetworkDeviceManager: valid.NetworkDeviceManager, + Logger: valid.Logger, + LocalDomainName: valid.LocalDomainName, + DBFilePath: valid.DBFilePath, + ICMPTimeout: -1 * time.Second, + Enabled: valid.Enabled, }, - name: "disabled_interfaces", - wantErrMsg: "", + wantErrMsg: "conf.ICMPTimeout: negative value: -1s", }, { + name: "bad_db_filepath", conf: &dhcpsvc.Config{ - Enabled: true, - Logger: testLogger, - LocalDomainName: testLocalTLD, - Interfaces: map[string]*dhcpsvc.InterfaceConfig{ - "eth0": { - IPv4: gwInRangeConf, - IPv6: validIPv6Conf, - }, - }, - DBFilePath: leasesPath, + Interfaces: valid.Interfaces, + NetworkDeviceManager: valid.NetworkDeviceManager, + Logger: valid.Logger, + LocalDomainName: valid.LocalDomainName, + DBFilePath: "", + ICMPTimeout: valid.ICMPTimeout, + Enabled: valid.Enabled, }, - name: "gateway_within_range", - wantErrMsg: "eth0: ipv4: gateway ip 192.168.0.100 in the ip range " + - "192.168.0.1-192.168.0.254", + wantErrMsg: "conf.DBFilePath: empty value", }, { + name: "no_interfaces", conf: &dhcpsvc.Config{ - Enabled: true, - Logger: testLogger, - LocalDomainName: testLocalTLD, - Interfaces: map[string]*dhcpsvc.InterfaceConfig{ - "eth0": { - IPv4: badStartConf, - IPv6: validIPv6Conf, - }, - }, - DBFilePath: leasesPath, + Interfaces: nil, + NetworkDeviceManager: valid.NetworkDeviceManager, + Logger: valid.Logger, + LocalDomainName: valid.LocalDomainName, + DBFilePath: valid.DBFilePath, + ICMPTimeout: valid.ICMPTimeout, + Enabled: valid.Enabled, }, - name: "bad_start", - wantErrMsg: "eth0: ipv4: range start 127.0.0.1 is not within 192.168.0.1/24" + "\n" + - "gateway ip 192.168.0.1 in the ip range 127.0.0.1-192.168.0.254", + wantErrMsg: "conf.Interfaces: empty value", + }, { + name: "nil_network_manager", + conf: &dhcpsvc.Config{ + Interfaces: valid.Interfaces, + NetworkDeviceManager: nil, + Logger: valid.Logger, + LocalDomainName: valid.LocalDomainName, + DBFilePath: valid.DBFilePath, + ICMPTimeout: valid.ICMPTimeout, + Enabled: valid.Enabled, + }, + wantErrMsg: "conf.NetworkDeviceManager: no value", + }, { + name: "no_logger", + conf: &dhcpsvc.Config{ + Interfaces: valid.Interfaces, + NetworkDeviceManager: valid.NetworkDeviceManager, + Logger: nil, + LocalDomainName: valid.LocalDomainName, + DBFilePath: valid.DBFilePath, + ICMPTimeout: valid.ICMPTimeout, + Enabled: valid.Enabled, + }, + wantErrMsg: "conf.Logger: no value", }} for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + t.Parallel() + testutil.AssertErrorMsg(t, tc.wantErrMsg, tc.conf.Validate()) }) } diff --git a/internal/dhcpsvc/dhcpsvc_test.go b/internal/dhcpsvc/dhcpsvc_test.go index 198ca165e..8909b1e96 100644 --- a/internal/dhcpsvc/dhcpsvc_test.go +++ b/internal/dhcpsvc/dhcpsvc_test.go @@ -13,6 +13,7 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/dhcpsvc" "github.com/AdguardTeam/golibs/logutil/slogutil" "github.com/AdguardTeam/golibs/testutil" + "github.com/AdguardTeam/golibs/testutil/faketime" "github.com/AdguardTeam/golibs/timeutil" "github.com/google/gopacket" "github.com/google/gopacket/layers" @@ -40,27 +41,47 @@ var testLogger = slogutil.NewDiscardLogger() // testdata is a filesystem containing data for tests. var testdata = os.DirFS("testdata") -// testInterfaceConf is a common set of interface configurations for tests. -var testInterfaceConf = map[string]*dhcpsvc.InterfaceConfig{ - "eth0": { - IPv4: &dhcpsvc.IPv4Config{ - Enabled: true, - Clock: timeutil.SystemClock{}, - GatewayIP: netip.MustParseAddr("192.168.0.1"), - SubnetMask: netip.MustParseAddr("255.255.255.0"), - RangeStart: netip.MustParseAddr("192.168.0.2"), - RangeEnd: netip.MustParseAddr("192.168.0.254"), - LeaseDuration: 1 * time.Hour, - }, - IPv6: &dhcpsvc.IPv6Config{ - Enabled: true, - RangeStart: netip.MustParseAddr("2001:db8::1"), - LeaseDuration: 1 * time.Hour, - RAAllowSLAAC: true, - RASLAACOnly: true, - }, +// testCurrentTime is the fixed time returned by [testClock] to ensure +// reproducible tests. +var testCurrentTime = time.Date(2025, 1, 1, 1, 1, 1, 0, time.UTC) + +// testClock is the test [timeutil.Clock] that always returns [testCurrentTime]. +var testClock = &faketime.Clock{ + OnNow: func() (now time.Time) { + return testCurrentTime }, - "eth1": { +} + +// testIPv4Conf is a common valid IPv4 part of the interface configuration for +// tests. +var testIPv4Conf = &dhcpsvc.IPv4Config{ + Enabled: true, + Clock: timeutil.SystemClock{}, + GatewayIP: netip.MustParseAddr("192.168.0.1"), + SubnetMask: netip.MustParseAddr("255.255.255.0"), + RangeStart: netip.MustParseAddr("192.168.0.2"), + RangeEnd: netip.MustParseAddr("192.168.0.254"), + LeaseDuration: testLeaseTTL, +} + +// testIPv6Conf is a common valid IPv6 part of the interface configuration for +// tests. +var testIPv6Conf = &dhcpsvc.IPv6Config{ + Enabled: true, + RangeStart: netip.MustParseAddr("2001:db8::1"), + LeaseDuration: testLeaseTTL, + RAAllowSLAAC: true, + RASLAACOnly: true, +} + +// testInterfaceConf is a common valid set of interface configurations for +// tests. +var testInterfaceConf = map[string]*dhcpsvc.InterfaceConfig{ + testIfaceName: { + IPv4: testIPv4Conf, + IPv6: testIPv6Conf, + }, + "iface1": { IPv4: &dhcpsvc.IPv4Config{ Enabled: true, Clock: timeutil.SystemClock{}, @@ -106,7 +127,7 @@ func newTempDB(tb testing.TB) (dst string) { dst = filepath.Join(tb.TempDir(), filename) - err = os.WriteFile(dst, data, dhcpsvc.DatabasePerm) + err = os.WriteFile(dst, data, 0o640) require.NoError(tb, err) return dst diff --git a/internal/dhcpsvc/handle.go b/internal/dhcpsvc/handle.go index 58da5f2d8..d5fcac09c 100644 --- a/internal/dhcpsvc/handle.go +++ b/internal/dhcpsvc/handle.go @@ -10,7 +10,8 @@ import ( // serveEther4 handles the incoming ethernet packets and dispatches them to the // appropriate handler. It's used to run in a separate goroutine as it blocks -// until packets channel is closed. iface and nd must not be nil. +// until packets channel is closed. iface and nd must not be nil. nd must have +// at least a single address returned by its Addresses method. func (srv *DHCPServer) serveEther4(ctx context.Context, iface *dhcpInterfaceV4, nd NetworkDevice) { defer slogutil.RecoverAndLog(ctx, srv.logger) diff --git a/internal/dhcpsvc/handler4.go b/internal/dhcpsvc/handler4.go index 90ceac414..ddd8ead0d 100644 --- a/internal/dhcpsvc/handler4.go +++ b/internal/dhcpsvc/handler4.go @@ -126,8 +126,8 @@ func (iface *dhcpInterfaceV4) handleRequest( default: // Server identifier MUST NOT be filled in, requested IP address option // MUST NOT be filled in. - ip, _ := netip.AddrFromSlice(req.ClientIP.To4()) - if !iface.subnet.Contains(ip) { + ip, ok := netip.AddrFromSlice(req.ClientIP.To4()) + if !ok || !iface.subnet.Contains(ip) { l.DebugContext(ctx, "skipping renew request", "clientip", ip) return @@ -245,16 +245,10 @@ func (iface *dhcpInterfaceV4) handleInitReboot( ) { l := iface.common.logger - if !reqIP.Is4() { - l.DebugContext(ctx, "bad requested address", "requestedip", reqIP) - - return - } - // ciaddr must be zero. The client is seeking to verify a previously // allocated, cached configuration. - ciaddr, _ := netip.AddrFromSlice(req.ClientIP) - if ciaddr.IsValid() && !ciaddr.IsUnspecified() { + ciaddr, ok := netip.AddrFromSlice(req.ClientIP) + if ok && !ciaddr.IsUnspecified() { l.DebugContext(ctx, "unexpected ciaddr in init-reboot request", "ciaddr", ciaddr) return diff --git a/internal/dhcpsvc/handler4_test.go b/internal/dhcpsvc/handler4_test.go index c92c68686..1be3f5f2c 100644 --- a/internal/dhcpsvc/handler4_test.go +++ b/internal/dhcpsvc/handler4_test.go @@ -8,7 +8,6 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/dhcpsvc" "github.com/AdguardTeam/golibs/testutil" - "github.com/AdguardTeam/golibs/testutil/faketime" "github.com/AdguardTeam/golibs/testutil/servicetest" "github.com/google/gopacket" "github.com/google/gopacket/layers" @@ -16,17 +15,6 @@ import ( "github.com/stretchr/testify/require" ) -// testCurrentTime is the fixed time returned by [testClock] to ensure -// reproducible tests. -var testCurrentTime = time.Date(2025, 1, 1, 1, 1, 1, 0, time.UTC) - -// testClock is the test [timeutil.Clock] that always returns [testCurrentTime]. -var testClock = &faketime.Clock{ - OnNow: func() (now time.Time) { - return testCurrentTime - }, -} - func TestDHCPServer_ServeEther4_discover(t *testing.T) { t.Parallel() @@ -57,6 +45,8 @@ func TestDHCPServer_ServeEther4_discover(t *testing.T) { hwAddrExpired = net.HardwareAddr{0x3, 0x4, 0x5, 0x6, 0x7, 0x8} ) + ifaceAddr := netip.MustParseAddr("192.168.0.2") + // NOTE: Keep in sync with testdata. dynamicLeaseExpiry := time.Date(2025, 1, 1, 10, 1, 1, 0, time.UTC) dynamicLeaseTTL := dynamicLeaseExpiry.Sub(testCurrentTime) @@ -83,7 +73,7 @@ func TestDHCPServer_ServeEther4_discover(t *testing.T) { in: newDHCPDISCOVER(t, hwAddrUnknown), wantOpts: layers.DHCPOptions{ newOptMessageType(t, layers.DHCPMsgTypeOffer), - newOptServerID(t, ipv4Conf.GatewayIP), + newOptServerID(t, ifaceAddr), newOptLeaseTime(t, testLeaseTTL), }, }, { @@ -91,7 +81,7 @@ func TestDHCPServer_ServeEther4_discover(t *testing.T) { in: newDHCPDISCOVER(t, hwAddrStatic), wantOpts: layers.DHCPOptions{ newOptMessageType(t, layers.DHCPMsgTypeOffer), - newOptServerID(t, ipv4Conf.GatewayIP), + newOptServerID(t, ifaceAddr), newOptLeaseTime(t, testLeaseTTL), newOptHostname(t, leaseHostnameStatic), }, @@ -100,7 +90,7 @@ func TestDHCPServer_ServeEther4_discover(t *testing.T) { in: newDHCPDISCOVER(t, hwAddrDynamic), wantOpts: layers.DHCPOptions{ newOptMessageType(t, layers.DHCPMsgTypeOffer), - newOptServerID(t, ipv4Conf.GatewayIP), + newOptServerID(t, ifaceAddr), newOptLeaseTime(t, dynamicLeaseTTL), newOptHostname(t, leaseHostnameDynamic), }, @@ -109,7 +99,7 @@ func TestDHCPServer_ServeEther4_discover(t *testing.T) { in: newDHCPDISCOVER(t, hwAddrExpired), wantOpts: layers.DHCPOptions{ newOptMessageType(t, layers.DHCPMsgTypeOffer), - newOptServerID(t, ipv4Conf.GatewayIP), + newOptServerID(t, ifaceAddr), newOptLeaseTime(t, testLeaseTTL), newOptHostname(t, leaseHostnameExpired), }, @@ -118,7 +108,7 @@ func TestDHCPServer_ServeEther4_discover(t *testing.T) { for _, tc := range testCases { req := testutil.RequireTypeAssert[*layers.DHCPv4](t, tc.in.Layer(layers.LayerTypeDHCPv4)) - ndMgr, inCh, outCh := newTestNetworkDeviceManager(t, testIfaceName) + ndMgr, inCh, outCh := newTestNetworkDeviceManager(t, testIfaceName, ifaceAddr) srv := newTestDHCPServer(t, &dhcpsvc.Config{ Interfaces: ifacesConfig, NetworkDeviceManager: ndMgr, @@ -150,10 +140,12 @@ func TestDHCPServer_ServeEther4_discoverExpired(t *testing.T) { // NOTE: Keep in sync with testdata. hwAddrUnknown := net.HardwareAddr{0x0, 0x1, 0x2, 0x3, 0x4, 0x5} + ifaceAddr := netip.MustParseAddr("192.168.0.2") + pkt := newDHCPDISCOVER(t, hwAddrUnknown) req := testutil.RequireTypeAssert[*layers.DHCPv4](t, pkt.Layer(layers.LayerTypeDHCPv4)) - ndMgr, inCh, outCh := newTestNetworkDeviceManager(t, testIfaceName) + ndMgr, inCh, outCh := newTestNetworkDeviceManager(t, testIfaceName, ifaceAddr) ipv4Conf := &dhcpsvc.IPv4Config{ Clock: testClock, @@ -181,12 +173,133 @@ func TestDHCPServer_ServeEther4_discoverExpired(t *testing.T) { assertValidOffer(t, req, respData, layers.DHCPOptions{ newOptMessageType(t, layers.DHCPMsgTypeOffer), - newOptServerID(t, ipv4Conf.GatewayIP), + newOptServerID(t, ifaceAddr), newOptLeaseTime(t, testLeaseTTL), }) } -// TODO(e.burkov): Add tests for DHCPREQUEST, DHCPRELEASE, DHCPDECLINE. +func TestDHCPServer_ServeEther4_release(t *testing.T) { + t.Parallel() + + // NOTE: Keep in sync with testdata. + leaseExpiry := time.Date(2025, 1, 1, 10, 1, 1, 0, time.UTC) + + // NOTE: Keep in sync with testdata. + var ( + // hwAddrSuccess is the MAC address for a lease to be released + // successfully. + hwAddrSuccess = net.HardwareAddr{0x02, 0x03, 0x04, 0x05, 0x06, 0x07} + + // ipSuccess matches the lease IP. + ipSuccess = netip.MustParseAddr("192.168.0.102") + + // ipMismatch is the IP of the lease used in the mismatch cases. + ipMismatch = netip.MustParseAddr("192.168.0.103") + + // hwAddrMismatch is the MAC address for a lease with mismatched IP. + hwAddrMismatch = net.HardwareAddr{0x03, 0x04, 0x05, 0x06, 0x07, 0x08} + + // ipMismatchReq is the IP requested for release, which differs from the + // lease IP. + ipMismatchReq = netip.MustParseAddr("192.168.0.104") + + // hwAddrUnknown is an unknown MAC. + hwAddrUnknown = net.HardwareAddr{0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff} + ) + + ipv4Conf := &dhcpsvc.IPv4Config{ + Clock: testClock, + SubnetMask: netip.MustParseAddr("255.255.255.0"), + GatewayIP: netip.MustParseAddr("192.168.0.1"), + RangeStart: netip.MustParseAddr("192.168.0.100"), + RangeEnd: netip.MustParseAddr("192.168.0.200"), + LeaseDuration: testLeaseTTL, + Enabled: true, + } + ifacesConfig := map[string]*dhcpsvc.InterfaceConfig{ + testIfaceName: {IPv4: ipv4Conf, IPv6: disabledIPv6Config}, + } + + ifaceHWAddr := net.HardwareAddr{0x01, 0x02, 0x03, 0x04, 0x05, 0x06} + ifaceAddr := netip.MustParseAddr("192.168.0.2") + anotherSubnetAddr := netip.MustParseAddr("10.0.0.1") + + var ( + leaseSuccess = &dhcpsvc.Lease{ + Expiry: leaseExpiry, + IP: ipSuccess, + Hostname: "success", + HWAddr: hwAddrSuccess, + IsStatic: false, + } + leaseMismatch = &dhcpsvc.Lease{ + Expiry: leaseExpiry, + IP: ipMismatch, + Hostname: "mismatch", + HWAddr: hwAddrMismatch, + IsStatic: false, + } + ) + + testCases := []struct { + name string + req gopacket.Packet + wantLeases []*dhcpsvc.Lease + }{{ + name: "success", + req: newDHCPRELEASE(t, hwAddrSuccess, ipSuccess, ifaceHWAddr, ifaceAddr), + wantLeases: []*dhcpsvc.Lease{ + leaseMismatch, + }, + }, { + name: "not_found", + req: newDHCPRELEASE(t, hwAddrUnknown, ipSuccess, ifaceHWAddr, ifaceAddr), + wantLeases: []*dhcpsvc.Lease{ + leaseSuccess, + leaseMismatch, + }, + }, { + name: "mismatch_ip", + req: newDHCPRELEASE(t, hwAddrMismatch, ipMismatchReq, ifaceHWAddr, ifaceAddr), + wantLeases: []*dhcpsvc.Lease{ + leaseSuccess, + leaseMismatch, + }, + }, { + name: "bad_subnet", + req: newDHCPRELEASE(t, hwAddrSuccess, anotherSubnetAddr, ifaceHWAddr, ifaceAddr), + wantLeases: []*dhcpsvc.Lease{ + leaseSuccess, + leaseMismatch, + }, + }} + + for _, tc := range testCases { + ndMgr, inCh, _ := newTestNetworkDeviceManager(t, testIfaceName, ifaceAddr) + srv := newTestDHCPServer(t, &dhcpsvc.Config{ + Interfaces: ifacesConfig, + NetworkDeviceManager: ndMgr, + DBFilePath: newTempDB(t), + Enabled: true, + }) + + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + servicetest.RequireRun(t, srv, testTimeout) + + testutil.RequireSend(t, inCh, tc.req, testTimeout) + + // TODO(e.burkov): Improve the test to ensure that the DHCPDISCOVER + // actually receives the released address. + assert.EventuallyWithT(t, func(ct *assert.CollectT) { + assert.Equal(ct, tc.wantLeases, srv.Leases()) + }, testTimeout/2, testTimeout/20) + }) + } +} + +// TODO(e.burkov): Add tests for DHCPREQUEST, DHCPDECLINE. // TODO(e.burkov): Add tests for wrong packets. @@ -221,10 +334,50 @@ func newDHCPDISCOVER(tb testing.TB, clientHWAddr net.HardwareAddr) (pkt gopacket Xid: testXid, ClientHWAddr: clientHWAddr, Options: layers.DHCPOptions{ - layers.NewDHCPOption( - layers.DHCPOptMessageType, - []byte{byte(layers.DHCPMsgTypeDiscover)}, - ), + newOptMessageType(tb, layers.DHCPMsgTypeDiscover), + }, + } + + return newTestPacket(tb, layers.LinkTypeEthernet, eth, ip, udp, dhcp) +} + +// newDHCPRELEASE creates a new DHCPRELEASE packet for testing. +func newDHCPRELEASE( + tb testing.TB, + clientHWAddr net.HardwareAddr, + clientIP netip.Addr, + serverHWAddr net.HardwareAddr, + serverIP netip.Addr, +) (pkt gopacket.Packet) { + tb.Helper() + + eth := &layers.Ethernet{ + SrcMAC: clientHWAddr, + DstMAC: serverHWAddr, + EthernetType: layers.EthernetTypeIPv4, + } + ip := &layers.IPv4{ + Version: 4, + TTL: dhcpsvc.IPv4DefaultTTL, + SrcIP: clientIP.AsSlice(), + DstIP: serverIP.AsSlice(), + Protocol: layers.IPProtocolUDP, + } + udp := &layers.UDP{ + SrcPort: dhcpsvc.ClientPortV4, + DstPort: dhcpsvc.ServerPortV4, + } + _ = udp.SetNetworkLayerForChecksum(ip) + + dhcp := &layers.DHCPv4{ + Operation: layers.DHCPOpRequest, + HardwareType: layers.LinkTypeEthernet, + HardwareLen: dhcpsvc.EUI48AddrLen, + Xid: testXid, + ClientHWAddr: clientHWAddr, + ClientIP: clientIP.AsSlice(), + Options: layers.DHCPOptions{ + newOptMessageType(tb, layers.DHCPMsgTypeRelease), }, } diff --git a/internal/dhcpsvc/networkdevice.go b/internal/dhcpsvc/networkdevice.go index a90087923..f811439eb 100644 --- a/internal/dhcpsvc/networkdevice.go +++ b/internal/dhcpsvc/networkdevice.go @@ -2,6 +2,7 @@ package dhcpsvc import ( "context" + "net/netip" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/validate" @@ -58,6 +59,9 @@ func (EmptyNetworkDeviceManager) Open( type NetworkDevice interface { gopacket.PacketDataSource + // Addresses returns all IP addresses assigned to the device. + Addresses() (ips []netip.Addr) + // LinkType returns the link type of the network interface. LinkType() (lt layers.LinkType) @@ -78,6 +82,12 @@ func (EmptyNetworkDevice) ReadPacketData() (data []byte, ci gopacket.CaptureInfo return nil, gopacket.CaptureInfo{}, nil } +// Addresses implements the [NetworkDevice] interface for [EmptyNetworkDevice]. +// It always returns nil. +func (EmptyNetworkDevice) Addresses() (ips []netip.Addr) { + return nil +} + // LinkType implements the [NetworkDevice] interface for [EmptyNetworkDevice]. // It always returns [layers.LinkTypeNull]. func (EmptyNetworkDevice) LinkType() (lt layers.LinkType) { diff --git a/internal/dhcpsvc/networkdevice_test.go b/internal/dhcpsvc/networkdevice_test.go index a5e427d1f..009787021 100644 --- a/internal/dhcpsvc/networkdevice_test.go +++ b/internal/dhcpsvc/networkdevice_test.go @@ -2,6 +2,7 @@ package dhcpsvc_test import ( "context" + "net/netip" "testing" "github.com/AdguardTeam/AdGuardHome/internal/dhcpsvc" @@ -40,6 +41,7 @@ func (ndm *testNetworkDeviceManager) Open( // TODO(e.burkov): Move to aghtest. type testNetworkDevice struct { onReadPacketData func() (data []byte, ci gopacket.CaptureInfo, err error) + onAddresses func() (ips []netip.Addr) onLinkType func() (lt layers.LinkType) onWritePacketData func(data []byte) (err error) } @@ -47,12 +49,18 @@ type testNetworkDevice struct { // type check var _ dhcpsvc.NetworkDevice = (*testNetworkDevice)(nil) -// ReadPacketData implements the [dhcpsvc.NetworkDevice] interface for +// ReadPacketData implements the [gopacket.PacketDataSource] interface for // *testNetworkDevice. func (nd *testNetworkDevice) ReadPacketData() (data []byte, ci gopacket.CaptureInfo, err error) { return nd.onReadPacketData() } +// Addresses implements the [dhcpsvc.NetworkDevice] interface for +// *testNetworkDevice. +func (nd *testNetworkDevice) Addresses() (ips []netip.Addr) { + return nd.onAddresses() +} + // WritePacketData implements the [dhcpsvc.NetworkDevice] interface for // *testNetworkDevice. func (nd *testNetworkDevice) WritePacketData(data []byte) (err error) { @@ -72,6 +80,7 @@ func (nd *testNetworkDevice) LinkType() (lt layers.LinkType) { func newTestNetworkDeviceManager( tb testing.TB, deviceName string, + addr netip.Addr, ) (ndMgr dhcpsvc.NetworkDeviceManager, inCh chan gopacket.Packet, outCh chan []byte) { tb.Helper() @@ -79,6 +88,7 @@ func newTestNetworkDeviceManager( outCh = make(chan []byte) pt := testutil.PanicT{} + addrs := []netip.Addr{addr} dev := &testNetworkDevice{ onReadPacketData: func() (data []byte, ci gopacket.CaptureInfo, err error) { @@ -93,6 +103,9 @@ func newTestNetworkDeviceManager( return data, ci, nil }, + onAddresses: func() (ips []netip.Addr) { + return addrs + }, onLinkType: func() (lt layers.LinkType) { return layers.LinkTypeEthernet }, diff --git a/internal/dhcpsvc/testdata/TestDHCPServer_ServeEther4_release/leases.json b/internal/dhcpsvc/testdata/TestDHCPServer_ServeEther4_release/leases.json new file mode 100644 index 000000000..c2eabac3c --- /dev/null +++ b/internal/dhcpsvc/testdata/TestDHCPServer_ServeEther4_release/leases.json @@ -0,0 +1,19 @@ +{ + "leases": [ + { + "expires": "2025-01-01T10:01:01Z", + "ip": "192.168.0.102", + "hostname": "success", + "mac": "02:03:04:05:06:07", + "static": false + }, + { + "expires": "2025-01-01T10:01:01Z", + "ip": "192.168.0.103", + "hostname": "mismatch", + "mac": "03:04:05:06:07:08", + "static": false + } + ], + "version": 1 +} diff --git a/internal/dhcpsvc/v4.go b/internal/dhcpsvc/v4.go index bf763e5ea..083b6f1aa 100644 --- a/internal/dhcpsvc/v4.go +++ b/internal/dhcpsvc/v4.go @@ -214,7 +214,7 @@ func (iface *dhcpInterfaceV4) respondOffer( fd *frameData, l *Lease, ) { - resp := iface.buildResponse(req, l, layers.DHCPMsgTypeOffer) + resp := iface.buildResponse(req, l, fd.device, layers.DHCPMsgTypeOffer) err := respond4(fd, resp) if err != nil { @@ -233,7 +233,7 @@ func (iface *dhcpInterfaceV4) respondACK( fd *frameData, l *Lease, ) { - resp := iface.buildResponse(req, l, layers.DHCPMsgTypeAck) + resp := iface.buildResponse(req, l, fd.device, layers.DHCPMsgTypeAck) if err := respond4(fd, resp); err != nil { iface.common.logger.ErrorContext(ctx, "writing ack", "error", err) } @@ -280,6 +280,7 @@ func (iface *dhcpInterfaceV4) respondNAK( func (iface *dhcpInterfaceV4) buildResponse( req *layers.DHCPv4, l *Lease, + nd NetworkDevice, msgType layers.DHCPMsgType, ) (resp *layers.DHCPv4) { resp = &layers.DHCPv4{ @@ -294,8 +295,7 @@ func (iface *dhcpInterfaceV4) buildResponse( resp.Options = append( resp.Options, layers.NewDHCPOption(layers.DHCPOptMessageType, []byte{byte(msgType)}), - // TODO(e.burkov): Use network device address. - layers.NewDHCPOption(layers.DHCPOptServerID, iface.gateway.AsSlice()), + layers.NewDHCPOption(layers.DHCPOptServerID, nd.Addresses()[0].AsSlice()), ) iface.appendLeaseTime(resp, l)