diff --git a/CHANGELOG.md b/CHANGELOG.md index 84057c5c..47f3d767 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,8 +22,12 @@ and this project adheres to ### Added +- New `bool`, `dur`, `u8`, and `u16` DHCP options to provide more convenience on + options control by setting values in a human-readable format ([#4705]). See + also a [Wiki page][wiki-dhcp-opts]. + - New `del` DHCP option which removes the corresponding option from server's - response ([#4337]). + response ([#4337]). See also a [Wiki page][wiki-dhcp-opts]. **NOTE:** This modifier affects all the parameters in the response and not only the requested ones. @@ -32,6 +36,7 @@ and this project adheres to ### Changed +- The DHCP options handling is now closer to the [RFC 2131][rfc-2131] ([#4705]). - When the DHCP server is enabled, queries for domain names under `dhcp.local_domain_name` not pointing to real DHCP client hostnames are now processed by filters ([#4865]). @@ -57,12 +62,14 @@ and this project adheres to [#4337]: https://github.com/AdguardTeam/AdGuardHome/issues/4337 [#4403]: https://github.com/AdguardTeam/AdGuardHome/issues/4403 [#4535]: https://github.com/AdguardTeam/AdGuardHome/issues/4535 +[#4705]: https://github.com/AdguardTeam/AdGuardHome/issues/4705 [#4745]: https://github.com/AdguardTeam/AdGuardHome/issues/4745 [#4850]: https://github.com/AdguardTeam/AdGuardHome/issues/4850 [#4863]: https://github.com/AdguardTeam/AdGuardHome/issues/4863 [#4865]: https://github.com/AdguardTeam/AdGuardHome/issues/4865 -[rfc-2131]: https://datatracker.ietf.org/doc/html/rfc2131 +[rfc-2131]: https://datatracker.ietf.org/doc/html/rfc2131 +[wiki-dhcp-opts]: https://github.com/adguardTeam/adGuardHome/wiki/DHCP#config-4 diff --git a/internal/dhcpd/options_unix.go b/internal/dhcpd/options_unix.go index e300729b..dc06c429 100644 --- a/internal/dhcpd/options_unix.go +++ b/internal/dhcpd/options_unix.go @@ -9,20 +9,28 @@ import ( "net" "strconv" "strings" + "time" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" "github.com/AdguardTeam/golibs/netutil" + "github.com/AdguardTeam/golibs/timeutil" "github.com/insomniacslk/dhcp/dhcpv4" ) // The aliases for DHCP option types available for explicit declaration. +// +// TODO(e.burkov): Add an option for classless routes. const ( + typDel = "del" + typBool = "bool" + typDur = "dur" typHex = "hex" typIP = "ip" typIPs = "ips" typText = "text" - typDel = "del" + typU8 = "u8" + typU16 = "u16" ) // parseDHCPOptionHex parses a DHCP option as a hex-encoded string. @@ -40,8 +48,8 @@ func parseDHCPOptionHex(s string) (val dhcpv4.OptionValue, err error) { func parseDHCPOptionIP(s string) (val dhcpv4.OptionValue, err error) { var ip net.IP // All DHCPv4 options require IPv4, so don't put the 16-byte version. - // Otherwise, the clients will receive weird data that looks like four - // IPv4 addresses. + // Otherwise, the clients will receive weird data that looks like four IPv4 + // addresses. // // See https://github.com/AdguardTeam/AdGuardHome/issues/2688. if ip, err = netutil.ParseIPv4(s); err != nil { @@ -55,35 +63,76 @@ func parseDHCPOptionIP(s string) (val dhcpv4.OptionValue, err error) { // addresses. func parseDHCPOptionIPs(s string) (val dhcpv4.OptionValue, err error) { var ips dhcpv4.IPs - var ip net.IP + var ip dhcpv4.OptionValue for i, ipStr := range strings.Split(s, ",") { - // See notes in the ipDHCPOptionParserHandler. - if ip, err = netutil.ParseIPv4(ipStr); err != nil { + ip, err = parseDHCPOptionIP(ipStr) + if err != nil { return nil, fmt.Errorf("parsing ip at index %d: %w", i, err) } - ips = append(ips, ip) + ips = append(ips, net.IP(ip.(dhcpv4.IP))) } return ips, nil } -// parseDHCPOptionText parses a DHCP option as a simple UTF-8 encoded -// text. -func parseDHCPOptionText(s string) (val dhcpv4.OptionValue) { - return dhcpv4.OptionGeneric{Data: []byte(s)} +// parseDHCPOptionDur parses a DHCP option as a duration in a human-readable +// form. +func parseDHCPOptionDur(s string) (val dhcpv4.OptionValue, err error) { + var v timeutil.Duration + err = v.UnmarshalText([]byte(s)) + if err != nil { + return nil, fmt.Errorf("decoding dur: %w", err) + } + + return dhcpv4.Duration(v.Duration), nil } -// parseDHCPOptionVal parses a DHCP option value considering typ. For the del -// option the value string is ignored. The examples of possible value pairs: -// -// - hex 736f636b733a2f2f70726f78792e6578616d706c652e6f7267 -// - ip 192.168.1.1 -// - ips 192.168.1.1,192.168.1.2 -// - text http://192.168.1.1/wpad.dat -// - del +// parseDHCPOptionUint parses a DHCP option as an unsigned integer. bitSize is +// expected to be 8 or 16. +func parseDHCPOptionUint(s string, bitSize int) (val dhcpv4.OptionValue, err error) { + var v uint64 + v, err = strconv.ParseUint(s, 10, bitSize) + if err != nil { + return nil, fmt.Errorf("decoding u%d: %w", bitSize, err) + } + + switch bitSize { + case 8: + return dhcpv4.OptionGeneric{Data: []byte{uint8(v)}}, nil + case 16: + return dhcpv4.Uint16(v), nil + default: + return nil, fmt.Errorf("unsupported size of integer %d", bitSize) + } +} + +// parseDHCPOptionBool parses a DHCP option as a boolean value. See +// [strconv.ParseBool] for available values. +func parseDHCPOptionBool(s string) (val dhcpv4.OptionValue, err error) { + var v bool + v, err = strconv.ParseBool(s) + if err != nil { + return nil, fmt.Errorf("decoding bool: %w", err) + } + + rawVal := [1]byte{} + if v { + rawVal[0] = 1 + } + + return dhcpv4.OptionGeneric{Data: rawVal[:]}, nil +} + +// parseDHCPOptionVal parses a DHCP option value considering typ. func parseDHCPOptionVal(typ, valStr string) (val dhcpv4.OptionValue, err error) { switch typ { + case typBool: + val, err = parseDHCPOptionBool(valStr) + case typDel: + val = dhcpv4.OptionGeneric{Data: nil} + case typDur: + val, err = parseDHCPOptionDur(valStr) case typHex: val, err = parseDHCPOptionHex(valStr) case typIP: @@ -91,9 +140,11 @@ func parseDHCPOptionVal(typ, valStr string) (val dhcpv4.OptionValue, err error) case typIPs: val, err = parseDHCPOptionIPs(valStr) case typText: - val = parseDHCPOptionText(valStr) - case typDel: - val = dhcpv4.OptionGeneric{Data: nil} + val = dhcpv4.String(valStr) + case typU8: + val, err = parseDHCPOptionUint(valStr, 8) + case typU16: + val, err = parseDHCPOptionUint(valStr, 16) default: err = fmt.Errorf("unknown option type %q", typ) } @@ -101,9 +152,19 @@ func parseDHCPOptionVal(typ, valStr string) (val dhcpv4.OptionValue, err error) return val, err } -// parseDHCPOption parses an option. See the documentation of -// parseDHCPOptionVal for more info. -func parseDHCPOption(s string) (opt dhcpv4.Option, err error) { +// parseDHCPOption parses an option. For the del option value is ignored. The +// examples of possible option strings: +// +// - 1 bool true +// - 2 del +// - 3 dur 2h5s +// - 4 hex 736f636b733a2f2f70726f78792e6578616d706c652e6f7267 +// - 5 ip 192.168.1.1 +// - 6 ips 192.168.1.1,192.168.1.2 +// - 7 text http://192.168.1.1/wpad.dat +// - 8 u8 255 +// - 9 u16 65535 +func parseDHCPOption(s string) (code dhcpv4.OptionCode, val dhcpv4.OptionValue, err error) { defer func() { err = errors.Annotate(err, "invalid option string %q: %w", s) }() s = strings.TrimSpace(s) @@ -112,7 +173,7 @@ func parseDHCPOption(s string) (opt dhcpv4.Option, err error) { var valStr string if pl := len(parts); pl < 3 { if pl < 2 || parts[1] != typDel { - return opt, errors.Error("bad option format") + return nil, nil, errors.Error("bad option format") } } else { valStr = parts[2] @@ -121,85 +182,226 @@ func parseDHCPOption(s string) (opt dhcpv4.Option, err error) { var code64 uint64 code64, err = strconv.ParseUint(parts[0], 10, 8) if err != nil { - return opt, fmt.Errorf("parsing option code: %w", err) + return nil, nil, fmt.Errorf("parsing option code: %w", err) } - val, err := parseDHCPOptionVal(parts[1], valStr) + val, err = parseDHCPOptionVal(parts[1], valStr) if err != nil { // Don't wrap an error since it's informative enough as is and there // also the deferred annotation. - return opt, err + return nil, nil, err } - return dhcpv4.Option{ - Code: dhcpv4.GenericOptionCode(code64), - Value: val, - }, nil + return dhcpv4.GenericOptionCode(code64), val, nil } // prepareOptions builds the set of DHCP options according to host requirements // document and values from conf. -func prepareOptions(conf V4ServerConf) (opts dhcpv4.Options) { - // Set default values for host configuration parameters listed in Appendix - // A of RFC-2131. Those parameters, if requested by client, should be - // returned with values defined by Host Requirements Document. - // - // See https://datatracker.ietf.org/doc/html/rfc2131#appendix-A. - // - // See also https://datatracker.ietf.org/doc/html/rfc1122, - // https://datatracker.ietf.org/doc/html/rfc1123, and - // https://datatracker.ietf.org/doc/html/rfc2132. - opts = dhcpv4.OptionsFromList( +func prepareOptions(conf V4ServerConf) (implicit, explicit dhcpv4.Options) { + // Set default values of host configuration parameters listed in Appendix A + // of RFC-2131. + implicit = dhcpv4.OptionsFromList( // IP-Layer Per Host - dhcpv4.OptGeneric(dhcpv4.OptionNonLocalSourceRouting, []byte{0}), + // An Internet host that includes embedded gateway code MUST have a + // configuration switch to disable the gateway function, and this switch + // MUST default to the non-gateway mode. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-3.3.5. + dhcpv4.OptGeneric(dhcpv4.OptionIPForwarding, []byte{0x0}), - // Set the current recommended default time to live for the - // Internet Protocol which is 64, see - // https://datatracker.ietf.org/doc/html/rfc1700. + // A host that supports non-local source-routing MUST have a + // configurable switch to disable forwarding, and this switch MUST + // default to disabled. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-3.3.5. + dhcpv4.OptGeneric(dhcpv4.OptionNonLocalSourceRouting, []byte{0x0}), + + // Do not set the Policy Filter Option since it only makes sense when + // the non-local source routing is enabled. + + // The minimum legal value is 576. + // + // See https://datatracker.ietf.org/doc/html/rfc2132#section-4.4. + dhcpv4.Option{ + Code: dhcpv4.OptionMaximumDatagramAssemblySize, + Value: dhcpv4.Uint16(576), + }, + + // Set the current recommended default time to live for the Internet + // Protocol which is 64. + // + // See https://www.iana.org/assignments/ip-parameters/ip-parameters.xhtml#ip-parameters-2. dhcpv4.OptGeneric(dhcpv4.OptionDefaultIPTTL, []byte{0x40}), - dhcpv4.OptGeneric(dhcpv4.OptionPerformMaskDiscovery, []byte{0}), - dhcpv4.OptGeneric(dhcpv4.OptionMaskSupplier, []byte{0}), - dhcpv4.OptGeneric(dhcpv4.OptionPerformRouterDiscovery, []byte{1}), - // The all-routers address is preferred wherever possible, see - // https://datatracker.ietf.org/doc/html/rfc1256#section-5.1. + // For example, after the PTMU estimate is decreased, the timeout should + // be set to 10 minutes; once this timer expires and a larger MTU is + // attempted, the timeout can be set to a much smaller value. + // + // See https://datatracker.ietf.org/doc/html/rfc1191#section-6.6. + dhcpv4.Option{ + Code: dhcpv4.OptionPathMTUAgingTimeout, + Value: dhcpv4.Duration(10 * time.Minute), + }, + + // There is a table describing the MTU values representing all major + // data-link technologies in use in the Internet so that each set of + // similar MTUs is associated with a plateau value equal to the lowest + // MTU in the group. + // + // See https://datatracker.ietf.org/doc/html/rfc1191#section-7. + dhcpv4.OptGeneric(dhcpv4.OptionPathMTUPlateauTable, []byte{ + 0x0, 0x44, + 0x1, 0x28, + 0x1, 0xFC, + 0x3, 0xEE, + 0x5, 0xD4, + 0x7, 0xD2, + 0x11, 0x0, + 0x1F, 0xE6, + 0x45, 0xFA, + }), + + // IP-Layer Per Interface + + // Since nearly all networks in the Internet currently support an MTU of + // 576 or greater, we strongly recommend the use of 576 for datagrams + // sent to non-local networks. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-3.3.3. + dhcpv4.Option{ + Code: dhcpv4.OptionInterfaceMTU, + Value: dhcpv4.Uint16(576), + }, + + // Set the All Subnets Are Local Option to false since commonly the + // connected hosts aren't expected to be multihomed. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-3.3.3. + dhcpv4.OptGeneric(dhcpv4.OptionAllSubnetsAreLocal, []byte{0x00}), + + // Set the Perform Mask Discovery Option to false to provide the subnet + // mask by options only. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-3.2.2.9. + dhcpv4.OptGeneric(dhcpv4.OptionPerformMaskDiscovery, []byte{0x00}), + + // A system MUST NOT send an Address Mask Reply unless it is an + // authoritative agent for address masks. An authoritative agent may be + // a host or a gateway, but it MUST be explicitly configured as a + // address mask agent. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-3.2.2.9. + dhcpv4.OptGeneric(dhcpv4.OptionMaskSupplier, []byte{0x00}), + + // Set the Perform Router Discovery Option to true as per Router + // Discovery Document. + // + // See https://datatracker.ietf.org/doc/html/rfc1256#section-5.1. + dhcpv4.OptGeneric(dhcpv4.OptionPerformRouterDiscovery, []byte{0x01}), + + // The all-routers address is preferred wherever possible. + // + // See https://datatracker.ietf.org/doc/html/rfc1256#section-5.1. dhcpv4.Option{ Code: dhcpv4.OptionRouterSolicitationAddress, Value: dhcpv4.IP(netutil.IPv4allrouter()), }, + + // Don't set the Static Routes Option since it should be set up by + // system administrator. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-3.3.1.2. + + // A datagram with the destination address of limited broadcast will be + // received by every host on the connected physical network but will not + // be forwarded outside that network. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-3.2.1.3. dhcpv4.OptBroadcastAddress(netutil.IPv4bcast()), // Link-Layer Per Interface - dhcpv4.OptGeneric(dhcpv4.OptionTrailerEncapsulation, []byte{0}), - dhcpv4.OptGeneric(dhcpv4.OptionEthernetEncapsulation, []byte{0}), + // If the system does not dynamically negotiate use of the trailer + // protocol on a per-destination basis, the default configuration MUST + // disable the protocol. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-2.3.1. + dhcpv4.OptGeneric(dhcpv4.OptionTrailerEncapsulation, []byte{0x00}), + + // For proxy ARP situations, the timeout needs to be on the order of a + // minute. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-2.3.2.1. + dhcpv4.Option{ + Code: dhcpv4.OptionArpCacheTimeout, + Value: dhcpv4.Duration(time.Minute), + }, + + // An Internet host that implements sending both the RFC-894 and the + // RFC-1042 encapsulations MUST provide a configuration switch to select + // which is sent, and this switch MUST default to RFC-894. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-2.3.3. + dhcpv4.OptGeneric(dhcpv4.OptionEthernetEncapsulation, []byte{0x00}), // TCP Per Host + // A fixed value must be at least big enough for the Internet diameter, + // i.e., the longest possible path. A reasonable value is about twice + // the diameter, to allow for continued Internet growth. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-3.2.1.7. + dhcpv4.Option{ + Code: dhcpv4.OptionDefaulTCPTTL, + Value: dhcpv4.Duration(60 * time.Second), + }, + + // The interval MUST be configurable and MUST default to no less than + // two hours. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-4.2.3.6. dhcpv4.Option{ Code: dhcpv4.OptionTCPKeepaliveInterval, - Value: dhcpv4.Duration(0), + Value: dhcpv4.Duration(2 * time.Hour), }, - dhcpv4.OptGeneric(dhcpv4.OptionTCPKeepaliveGarbage, []byte{0}), + + // Unfortunately, some misbehaved TCP implementations fail to respond to + // a probe segment unless it contains data. + // + // See https://datatracker.ietf.org/doc/html/rfc1122#section-4.2.3.6. + dhcpv4.OptGeneric(dhcpv4.OptionTCPKeepaliveGarbage, []byte{0x01}), // Values From Configuration + // Set the Router Option to working subnet's IP since it's initialized + // with the address of the gateway. dhcpv4.OptRouter(conf.subnet.IP), + dhcpv4.OptSubnetMask(conf.subnet.Mask), ) // Set values for explicitly configured options. + explicit = dhcpv4.Options{} for i, o := range conf.Options { - opt, err := parseDHCPOption(o) + code, val, err := parseDHCPOption(o) if err != nil { log.Error("dhcpv4: bad option string at index %d: %s", i, err) continue } - opts.Update(opt) + explicit.Update(dhcpv4.Option{Code: code, Value: val}) + // Remove those from the implicit options. + delete(implicit, code.Code()) } - return opts + log.Debug("dhcpv4: implicit options:\n%s", implicit.Summary(nil)) + log.Debug("dhcpv4: explicit options:\n%s", explicit.Summary(nil)) + + if len(explicit) == 0 { + explicit = nil + } + + return implicit, explicit } diff --git a/internal/dhcpd/options_unix_test.go b/internal/dhcpd/options_unix_test.go index 6e97c2de..e901284c 100644 --- a/internal/dhcpd/options_unix_test.go +++ b/internal/dhcpd/options_unix_test.go @@ -7,7 +7,9 @@ import ( "fmt" "net" "testing" + "time" + "github.com/AdguardTeam/golibs/netutil" "github.com/AdguardTeam/golibs/testutil" "github.com/insomniacslk/dhcp/dhcpv4" "github.com/stretchr/testify/assert" @@ -17,150 +19,199 @@ func TestParseOpt(t *testing.T) { testCases := []struct { name string in string - wantOpt dhcpv4.Option + wantCode dhcpv4.OptionCode + wantVal dhcpv4.OptionValue wantErrMsg string }{{ - name: "hex_success", - in: "6 hex c0a80101c0a80102", - wantOpt: dhcpv4.OptGeneric( - dhcpv4.GenericOptionCode(6), - []byte{ - 0xC0, 0xA8, 0x01, 0x01, - 0xC0, 0xA8, 0x01, 0x02, - }, - ), + name: "hex_success", + in: "6 hex c0a80101c0a80102", + wantCode: dhcpv4.GenericOptionCode(6), + wantVal: dhcpv4.OptionGeneric{Data: []byte{ + 0xC0, 0xA8, 0x01, 0x01, + 0xC0, 0xA8, 0x01, 0x02, + }}, wantErrMsg: "", }, { - name: "ip_success", - in: "6 ip 1.2.3.4", - wantOpt: dhcpv4.Option{ - Code: dhcpv4.GenericOptionCode(6), - Value: dhcpv4.IP(net.IP{0x01, 0x02, 0x03, 0x04}), - }, + name: "ip_success", + in: "6 ip 1.2.3.4", + wantCode: dhcpv4.GenericOptionCode(6), + wantVal: dhcpv4.IP(net.IP{0x01, 0x02, 0x03, 0x04}), wantErrMsg: "", }, { - name: "ip_fail_v6", - in: "6 ip ::1234", - wantOpt: dhcpv4.Option{}, - wantErrMsg: "invalid option string \"6 ip ::1234\": bad ipv4 address \"::1234\"", - }, { - name: "ips_success", - in: "6 ips 192.168.1.1,192.168.1.2", - wantOpt: dhcpv4.Option{ - Code: dhcpv4.GenericOptionCode(6), - Value: dhcpv4.IPs([]net.IP{ - {0xC0, 0xA8, 0x01, 0x01}, - {0xC0, 0xA8, 0x01, 0x02}, - }), - }, + name: "ips_success", + in: "6 ips 192.168.1.1,192.168.1.2", + wantCode: dhcpv4.GenericOptionCode(6), + wantVal: dhcpv4.IPs([]net.IP{ + {0xC0, 0xA8, 0x01, 0x01}, + {0xC0, 0xA8, 0x01, 0x02}, + }), wantErrMsg: "", }, { - name: "text_success", - in: "252 text http://192.168.1.1/", - wantOpt: dhcpv4.OptGeneric( - dhcpv4.GenericOptionCode(252), - []byte("http://192.168.1.1/"), - ), + name: "text_success", + in: "252 text http://192.168.1.1/", + wantCode: dhcpv4.GenericOptionCode(252), + wantVal: dhcpv4.String("http://192.168.1.1/"), wantErrMsg: "", }, { - name: "del_success", - in: "61 del", - wantOpt: dhcpv4.Option{ - Code: dhcpv4.GenericOptionCode(dhcpv4.OptionClientIdentifier), - Value: dhcpv4.OptionGeneric{Data: nil}, - }, + name: "del_success", + in: "61 del", + wantCode: dhcpv4.GenericOptionCode(dhcpv4.OptionClientIdentifier), + wantVal: dhcpv4.OptionGeneric{Data: nil}, + wantErrMsg: "", + }, { + name: "bool_success", + in: "19 bool true", + wantCode: dhcpv4.GenericOptionCode(dhcpv4.OptionIPForwarding), + wantVal: dhcpv4.OptionGeneric{Data: []byte{0x01}}, + wantErrMsg: "", + }, { + name: "bool_success_false", + in: "19 bool F", + wantCode: dhcpv4.GenericOptionCode(dhcpv4.OptionIPForwarding), + wantVal: dhcpv4.OptionGeneric{Data: []byte{0x00}}, + wantErrMsg: "", + }, { + name: "dur_success", + in: "24 dur 2h5s", + wantCode: dhcpv4.GenericOptionCode(dhcpv4.OptionPathMTUAgingTimeout), + wantVal: dhcpv4.Duration(2*time.Hour + 5*time.Second), + wantErrMsg: "", + }, { + name: "u8_success", + in: "23 u8 64", + wantCode: dhcpv4.GenericOptionCode(dhcpv4.OptionDefaultIPTTL), + wantVal: dhcpv4.OptionGeneric{Data: []byte{0x40}}, + wantErrMsg: "", + }, { + name: "u16_success", + in: "22 u16 1234", + wantCode: dhcpv4.GenericOptionCode(dhcpv4.OptionMaximumDatagramAssemblySize), + wantVal: dhcpv4.Uint16(1234), wantErrMsg: "", }, { name: "bad_parts", in: "6 ip", - wantOpt: dhcpv4.Option{}, + wantCode: nil, + wantVal: nil, wantErrMsg: `invalid option string "6 ip": bad option format`, }, { - name: "bad_code", - in: "256 ip 1.1.1.1", - wantOpt: dhcpv4.Option{}, + name: "bad_code", + in: "256 ip 1.1.1.1", + wantCode: nil, + wantVal: nil, wantErrMsg: `invalid option string "256 ip 1.1.1.1": parsing option code: ` + `strconv.ParseUint: parsing "256": value out of range`, }, { name: "bad_type", in: "6 bad 1.1.1.1", - wantOpt: dhcpv4.Option{}, + wantCode: nil, + wantVal: nil, wantErrMsg: `invalid option string "6 bad 1.1.1.1": unknown option type "bad"`, }, { - name: "hex_error", - in: "6 hex ZZZ", - wantOpt: dhcpv4.Option{}, + name: "hex_error", + in: "6 hex ZZZ", + wantCode: nil, + wantVal: nil, wantErrMsg: `invalid option string "6 hex ZZZ": decoding hex: ` + `encoding/hex: invalid byte: U+005A 'Z'`, }, { name: "ip_error", in: "6 ip 1.2.3.x", - wantOpt: dhcpv4.Option{}, + wantCode: nil, + wantVal: nil, wantErrMsg: "invalid option string \"6 ip 1.2.3.x\": bad ipv4 address \"1.2.3.x\"", }, { - name: "ips_error", - in: "6 ips 192.168.1.1,192.168.1.x", - wantOpt: dhcpv4.Option{}, + name: "ip_error_v6", + in: "6 ip ::1234", + wantCode: nil, + wantVal: nil, + wantErrMsg: "invalid option string \"6 ip ::1234\": bad ipv4 address \"::1234\"", + }, { + name: "ips_error", + in: "6 ips 192.168.1.1,192.168.1.x", + wantCode: nil, + wantVal: nil, wantErrMsg: "invalid option string \"6 ips 192.168.1.1,192.168.1.x\": " + "parsing ip at index 1: bad ipv4 address \"192.168.1.x\"", + }, { + name: "bool_error", + in: "19 bool yes", + wantCode: nil, + wantVal: nil, + wantErrMsg: "invalid option string \"19 bool yes\": decoding bool: " + + "strconv.ParseBool: parsing \"yes\": invalid syntax", + }, { + name: "dur_error", + in: "24 dur 3y", + wantCode: nil, + wantVal: nil, + wantErrMsg: "invalid option string \"24 dur 3y\": decoding dur: " + + "unmarshaling duration: time: unknown unit \"y\" in duration \"3y\"", + }, { + name: "u8_error", + in: "23 u8 256", + wantCode: nil, + wantVal: nil, + wantErrMsg: "invalid option string \"23 u8 256\": decoding u8: " + + "strconv.ParseUint: parsing \"256\": value out of range", + }, { + name: "u16_error", + in: "23 u16 65536", + wantCode: nil, + wantVal: nil, + wantErrMsg: "invalid option string \"23 u16 65536\": decoding u16: " + + "strconv.ParseUint: parsing \"65536\": value out of range", }} for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - opt, err := parseDHCPOption(tc.in) + code, val, err := parseDHCPOption(tc.in) testutil.AssertErrorMsg(t, tc.wantErrMsg, err) - // assert.Equal(t, tc.wantOpt.Code.Code(), opt.Code.Code()) - // assert.Equal(t, tc.wantOpt.Value.ToBytes(), opt.Value.ToBytes()) - assert.Equal(t, tc.wantOpt, opt) + assert.Equal(t, tc.wantCode, code) + assert.Equal(t, tc.wantVal, val) }) } } func TestPrepareOptions(t *testing.T) { - allDefault := dhcpv4.Options{ - dhcpv4.OptionNonLocalSourceRouting.Code(): []byte{0}, - dhcpv4.OptionDefaultIPTTL.Code(): []byte{64}, - dhcpv4.OptionPerformMaskDiscovery.Code(): []byte{0}, - dhcpv4.OptionMaskSupplier.Code(): []byte{0}, - dhcpv4.OptionPerformRouterDiscovery.Code(): []byte{1}, - dhcpv4.OptionRouterSolicitationAddress.Code(): []byte{224, 0, 0, 2}, - dhcpv4.OptionBroadcastAddress.Code(): []byte{255, 255, 255, 255}, - dhcpv4.OptionTrailerEncapsulation.Code(): []byte{0}, - dhcpv4.OptionEthernetEncapsulation.Code(): []byte{0}, - dhcpv4.OptionTCPKeepaliveInterval.Code(): []byte{0, 0, 0, 0}, - dhcpv4.OptionTCPKeepaliveGarbage.Code(): []byte{0}, - } oneIP, otherIP := net.IP{1, 2, 3, 4}, net.IP{5, 6, 7, 8} testCases := []struct { - name string - checks dhcpv4.Options - opts []string + name string + wantExplicit dhcpv4.Options + opts []string }{{ - name: "all_default", - checks: allDefault, - opts: nil, + name: "all_default", + wantExplicit: nil, + opts: nil, }, { name: "configured_ip", - checks: dhcpv4.Options{ - dhcpv4.OptionBroadcastAddress.Code(): oneIP, - }, + wantExplicit: dhcpv4.OptionsFromList( + dhcpv4.OptBroadcastAddress(oneIP), + ), opts: []string{ fmt.Sprintf("%d ip %s", dhcpv4.OptionBroadcastAddress, oneIP), }, }, { name: "configured_ips", - checks: dhcpv4.Options{ - dhcpv4.OptionDomainNameServer.Code(): append(oneIP, otherIP...), - }, + wantExplicit: dhcpv4.OptionsFromList( + dhcpv4.Option{ + Code: dhcpv4.OptionDomainNameServer, + Value: dhcpv4.IPs{oneIP, otherIP}, + }, + ), opts: []string{ fmt.Sprintf("%d ips %s,%s", dhcpv4.OptionDomainNameServer, oneIP, otherIP), }, }, { - name: "configured_bad", - checks: allDefault, + name: "configured_bad", + wantExplicit: nil, opts: []string{ + "19 bool yes", + "24 dur 3y", + "23 u8 256", + "23 u16 65536", "20 hex", "23 hex abc", "32 ips 1,2,3,4", @@ -168,26 +219,29 @@ func TestPrepareOptions(t *testing.T) { }, }, { name: "configured_del", - checks: dhcpv4.Options{ - dhcpv4.OptionBroadcastAddress.Code(): nil, - }, + wantExplicit: dhcpv4.OptionsFromList( + dhcpv4.OptBroadcastAddress(nil), + ), opts: []string{ "28 del", }, }, { name: "rewritten_del", - checks: dhcpv4.Options{ - dhcpv4.OptionBroadcastAddress.Code(): []byte{255, 255, 255, 255}, - }, + wantExplicit: dhcpv4.OptionsFromList( + dhcpv4.OptBroadcastAddress(netutil.IPv4bcast()), + ), opts: []string{ "28 del", "28 ip 255.255.255.255", }, }, { name: "configured_and_del", - checks: dhcpv4.Options{ - 123: []byte("cba"), - }, + wantExplicit: dhcpv4.OptionsFromList( + dhcpv4.Option{ + Code: dhcpv4.OptionGeoConf, + Value: dhcpv4.String("cba"), + }, + ), opts: []string{ "123 text abc", "123 del", @@ -197,18 +251,16 @@ func TestPrepareOptions(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - if tc.name == "configured_del" { - assert.True(t, true) - } - opts := prepareOptions(V4ServerConf{ + implicit, explicit := prepareOptions(V4ServerConf{ // Just to avoid nil pointer dereference. subnet: &net.IPNet{}, Options: tc.opts, }) - for c, v := range tc.checks { - val := opts.Get(dhcpv4.GenericOptionCode(c)) - assert.Lenf(t, val, len(v), "Code: %v", c) - assert.Equal(t, v, val) + + assert.Equal(t, tc.wantExplicit, explicit) + + for c := range explicit { + assert.NotContains(t, implicit, c) } }) } diff --git a/internal/dhcpd/v4.go b/internal/dhcpd/v4.go index 5ea55ddd..31c4d0b2 100644 --- a/internal/dhcpd/v4.go +++ b/internal/dhcpd/v4.go @@ -33,6 +33,17 @@ type v4Server struct { conf V4ServerConf srv *server4.Server + // implicitOpts are the options listed in Appendix A of RFC 2131 initialized + // with default values. It must not have intersections with [explicitOpts]. + implicitOpts dhcpv4.Options + + // explicitOpts are the options parsed from the configuration. It must not + // have intersections with [implicitOpts]. + explicitOpts dhcpv4.Options + + // leasesLock protects leases, leaseHosts, and leasedOffsets. + leasesLock sync.Mutex + // leasedOffsets contains offsets from conf.ipRange.start that have been // leased. leasedOffsets *bitSet @@ -42,12 +53,6 @@ type v4Server struct { // leases contains all dynamic and static leases. leases []*Lease - - // leasesLock protects leases, leaseHosts, and leasedOffsets. - leasesLock sync.Mutex - - // options holds predefined DHCP options to return to clients. - options dhcpv4.Options } // WriteDiskConfig4 - write configuration @@ -999,34 +1004,42 @@ func (s *v4Server) handle(req, resp *dhcpv4.DHCPv4) int { resp.YourIPAddr = netutil.CloneIP(l.IP) } + s.updateOptions(req, resp) + + return 1 +} + +// updateOptions updates the options of the response in accordance with the +// request and RFC 2131. +// +// See https://datatracker.ietf.org/doc/html/rfc2131#section-4.3.1. +func (s *v4Server) updateOptions(req, resp *dhcpv4.DHCPv4) { // Set IP address lease time for all DHCPOFFER messages and DHCPACK messages // replied for DHCPREQUEST. // // TODO(e.burkov): Inspect why this is always set to configured value. resp.UpdateOption(dhcpv4.OptIPAddressLeaseTime(s.conf.leaseTime)) - // Delete options explicitly configured to be removed. - for code := range resp.Options { - if val, ok := s.options[code]; ok && val == nil { + // If the server recognizes the parameter as a parameter defined in the Host + // Requirements Document, the server MUST include the default value for that + // parameter. + for _, code := range req.ParameterRequestList() { + if val := s.implicitOpts.Get(code); val != nil { + resp.UpdateOption(dhcpv4.OptGeneric(code, val)) + } + } + + // If the server has been explicitly configured with a default value for the + // parameter or the parameter has a non-default value on the client's + // subnet, the server MUST include that value in an appropriate option. + for code, val := range s.explicitOpts { + if val != nil { + resp.Options[code] = val + } else { + // Delete options explicitly configured to be removed. delete(resp.Options, code) } } - - // Update values for each explicitly configured parameter requested by - // client. - // - // See https://datatracker.ietf.org/doc/html/rfc2131#section-4.3.1. - requested := req.ParameterRequestList() - for _, code := range requested { - if val := s.options.Get(code); val != nil { - resp.UpdateOption(dhcpv4.Option{ - Code: code, - Value: dhcpv4.OptionGeneric{Data: s.options.Get(code)}, - }) - } - } - - return 1 } // client(0.0.0.0:68) -> (Request:ClientMAC,Type=Discover,ClientID,ReqIP,HostName) -> server(255.255.255.255:67) @@ -1151,8 +1164,11 @@ func (s *v4Server) Start() (err error) { } // Update the value of Domain Name Server option separately from others if // not assigned yet since its value is available only at server's start. - if !s.options.Has(dhcpv4.OptionDomainNameServer) { - s.options.Update(dhcpv4.OptDNS(dnsIPAddrs...)) + // + // TODO(e.burkov): Initialize as implicit option with the rest of default + // options when it will be possible to do before the call to Start. + if !s.explicitOpts.Has(dhcpv4.OptionDomainNameServer) { + s.implicitOpts.Update(dhcpv4.OptDNS(dnsIPAddrs...)) } s.conf.dnsIPAddrs = dnsIPAddrs @@ -1279,7 +1295,7 @@ func v4Create(conf V4ServerConf) (srv DHCPServer, err error) { s.conf.leaseTime = time.Second * time.Duration(conf.LeaseDuration) } - s.options = prepareOptions(s.conf) + s.implicitOpts, s.explicitOpts = prepareOptions(s.conf) return s, nil } diff --git a/internal/dhcpd/v4_test.go b/internal/dhcpd/v4_test.go index 98859ca6..6d8f513e 100644 --- a/internal/dhcpd/v4_test.go +++ b/internal/dhcpd/v4_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/AdguardTeam/AdGuardHome/internal/aghnet" + "github.com/AdguardTeam/golibs/netutil" "github.com/AdguardTeam/golibs/stringutil" "github.com/AdguardTeam/golibs/testutil" "github.com/insomniacslk/dhcp/dhcpv4" @@ -346,7 +347,7 @@ func TestV4Server_handle_optionsPriority(t *testing.T) { } conf.Options = []string{b.String()} } else { - defer func() { s.options.Update(dhcpv4.OptDNS(defaultIP)) }() + defer func() { s.implicitOpts.Update(dhcpv4.OptDNS(defaultIP)) }() } ss, err := v4Create(conf) @@ -402,6 +403,111 @@ func TestV4Server_handle_optionsPriority(t *testing.T) { }) } +func TestV4Server_updateOptions(t *testing.T) { + testIP := net.IP{1, 2, 3, 4} + + dontWant := func(c dhcpv4.OptionCode) (opt dhcpv4.Option) { + return dhcpv4.OptGeneric(c, nil) + } + + testCases := []struct { + name string + wantOpts dhcpv4.Options + reqMods []dhcpv4.Modifier + confOpts []string + }{{ + name: "requested_default", + wantOpts: dhcpv4.OptionsFromList( + dhcpv4.OptBroadcastAddress(netutil.IPv4bcast()), + ), + reqMods: []dhcpv4.Modifier{ + dhcpv4.WithRequestedOptions(dhcpv4.OptionBroadcastAddress), + }, + confOpts: nil, + }, { + name: "requested_non-default", + wantOpts: dhcpv4.OptionsFromList( + dhcpv4.OptBroadcastAddress(testIP), + ), + reqMods: []dhcpv4.Modifier{ + dhcpv4.WithRequestedOptions(dhcpv4.OptionBroadcastAddress), + }, + confOpts: []string{ + fmt.Sprintf("%d ip %s", dhcpv4.OptionBroadcastAddress, testIP), + }, + }, { + name: "non-requested_default", + wantOpts: dhcpv4.OptionsFromList( + dontWant(dhcpv4.OptionBroadcastAddress), + ), + reqMods: nil, + confOpts: nil, + }, { + name: "non-requested_non-default", + wantOpts: dhcpv4.OptionsFromList( + dhcpv4.OptBroadcastAddress(testIP), + ), + reqMods: nil, + confOpts: []string{ + fmt.Sprintf("%d ip %s", dhcpv4.OptionBroadcastAddress, testIP), + }, + }, { + name: "requested_deleted", + wantOpts: dhcpv4.OptionsFromList( + dontWant(dhcpv4.OptionBroadcastAddress), + ), + reqMods: []dhcpv4.Modifier{ + dhcpv4.WithRequestedOptions(dhcpv4.OptionBroadcastAddress), + }, + confOpts: []string{ + fmt.Sprintf("%d del", dhcpv4.OptionBroadcastAddress), + }, + }, { + name: "requested_non-default_deleted", + wantOpts: dhcpv4.OptionsFromList( + dontWant(dhcpv4.OptionBroadcastAddress), + ), + reqMods: []dhcpv4.Modifier{ + dhcpv4.WithRequestedOptions(dhcpv4.OptionBroadcastAddress), + }, + confOpts: []string{ + fmt.Sprintf("%d ip %s", dhcpv4.OptionBroadcastAddress, testIP), + fmt.Sprintf("%d del", dhcpv4.OptionBroadcastAddress), + }, + }} + + for _, tc := range testCases { + req, err := dhcpv4.New(tc.reqMods...) + require.NoError(t, err) + + resp, err := dhcpv4.NewReplyFromRequest(req) + require.NoError(t, err) + + conf := defaultV4ServerConf() + conf.Options = tc.confOpts + + s, err := v4Create(conf) + require.NoError(t, err) + + require.IsType(t, (*v4Server)(nil), s) + s4, _ := s.(*v4Server) + + t.Run(tc.name, func(t *testing.T) { + s4.updateOptions(req, resp) + + for c, v := range tc.wantOpts { + if v == nil { + assert.NotContains(t, resp.Options, c) + + continue + } + + assert.Equal(t, v, resp.Options.Get(dhcpv4.GenericOptionCode(c))) + } + }) + } +} + func TestV4StaticLease_Get(t *testing.T) { sIface := defaultSrv(t) @@ -409,7 +515,7 @@ func TestV4StaticLease_Get(t *testing.T) { require.True(t, ok) s.conf.dnsIPAddrs = []net.IP{{192, 168, 10, 1}} - s.options.Update(dhcpv4.OptDNS(s.conf.dnsIPAddrs...)) + s.implicitOpts.Update(dhcpv4.OptDNS(s.conf.dnsIPAddrs...)) l := &Lease{ Hostname: "static-1.local", @@ -498,7 +604,7 @@ func TestV4DynamicLease_Get(t *testing.T) { require.True(t, ok) s.conf.dnsIPAddrs = []net.IP{{192, 168, 10, 1}} - s.options.Update(dhcpv4.OptDNS(s.conf.dnsIPAddrs...)) + s.implicitOpts.Update(dhcpv4.OptDNS(s.conf.dnsIPAddrs...)) var req, resp *dhcpv4.DHCPv4 mac := net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA} diff --git a/internal/stats/stats_internal_test.go b/internal/stats/stats_internal_test.go index 28a556d3..7d5d9fef 100644 --- a/internal/stats/stats_internal_test.go +++ b/internal/stats/stats_internal_test.go @@ -1,8 +1,14 @@ package stats import ( + "fmt" + "path/filepath" + "sync" + "sync/atomic" "testing" + "time" + "github.com/AdguardTeam/golibs/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -24,3 +30,76 @@ func TestStatsCollector(t *testing.T) { } }) } + +func TestStats_races(t *testing.T) { + var r uint32 + idGen := func() (id uint32) { return atomic.LoadUint32(&r) } + conf := Config{ + UnitID: idGen, + Filename: filepath.Join(t.TempDir(), "./stats.db"), + LimitDays: 1, + } + + s, err := New(conf) + require.NoError(t, err) + + s.Start() + startTime := time.Now() + testutil.CleanupAndRequireSuccess(t, s.Close) + + type signal = struct{} + + writeFunc := func(start, fin *sync.WaitGroup, waitCh <-chan unit, i int) { + e := Entry{ + Domain: fmt.Sprintf("example-%d.org", i), + Client: fmt.Sprintf("client_%d", i), + Result: Result(i)%(resultLast-1) + 1, + Time: uint32(time.Since(startTime).Milliseconds()), + } + + start.Done() + defer fin.Done() + + <-waitCh + + s.Update(e) + } + readFunc := func(start, fin *sync.WaitGroup, waitCh <-chan unit) { + start.Done() + defer fin.Done() + + <-waitCh + + _, _ = s.getData(24) + } + + const ( + roundsNum = 3 + + writersNum = 10 + readersNum = 5 + ) + + for round := 0; round < roundsNum; round++ { + atomic.StoreUint32(&r, uint32(round)) + + startWG, finWG := &sync.WaitGroup{}, &sync.WaitGroup{} + waitCh := make(chan unit) + + for i := 0; i < writersNum; i++ { + startWG.Add(1) + finWG.Add(1) + go writeFunc(startWG, finWG, waitCh, i) + } + + for i := 0; i < readersNum; i++ { + startWG.Add(1) + finWG.Add(1) + go readFunc(startWG, finWG, waitCh) + } + + startWG.Wait() + close(waitCh) + finWG.Wait() + } +} diff --git a/scripts/make/go-lint.sh b/scripts/make/go-lint.sh index 687a65c2..63e32dd9 100644 --- a/scripts/make/go-lint.sh +++ b/scripts/make/go-lint.sh @@ -220,9 +220,9 @@ exit_on_output gofumpt --extra -e -l . "$GO" vet ./... # Apply more lax standards to the code we haven't properly refactored yet. -gocyclo --over 17 ./internal/dhcpd ./internal/querylog/ +gocyclo --over 17 ./internal/querylog/ gocyclo --over 16 ./internal/dnsforward/ -gocyclo --over 15 ./internal/home/ +gocyclo --over 15 ./internal/home/ ./internal/dhcpd gocyclo --over 13 ./internal/filtering/ # Apply stricter standards to new or somewhat refactored code.