From 45bcc2c09a599acbb98355d5bf8d854cf764c426 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Fri, 26 Aug 2022 14:30:31 +0300 Subject: [PATCH] Pull request: 4337 Add del option Merge in DNS/adguard-home from 4337-dhcp-cli-id to master Updates #4337. Squashed commit of the following: commit c393bf7c5964b64f6b4528db0418e54416dd246c Author: Eugene Burkov Date: Fri Aug 26 14:18:48 2022 +0300 dhcpd: ip docs commit bfeef3e881ed04eab6285c1ac62005c7cb57c11f Author: Eugene Burkov Date: Fri Aug 26 13:54:33 2022 +0300 all: finish chlog fmt commit e5fbb7385450825ca81fc7622feb7beb390a8bad Author: Eugene Burkov Date: Fri Aug 26 13:49:10 2022 +0300 dhcpd: imp naming commit cb49eeb536afd8dc1dd2ea9169dd024c7d4a3a25 Author: Eugene Burkov Date: Fri Aug 26 12:23:54 2022 +0300 dhcpd: imp docs commit c4ea72a5e7572d40a885125c4f61ef6694e38170 Author: Eugene Burkov Date: Thu Aug 25 20:15:15 2022 +0300 dhcpd: imp code, docs commit 36d0e309e7ef0abdcdd94673e87f4d0af67b9cb3 Author: Eugene Burkov Date: Thu Aug 25 19:45:24 2022 +0300 dhcpd: add del opt --- CHANGELOG.md | 13 ++- internal/dhcpd/options_unix.go | 129 +++++++++++++---------- internal/dhcpd/options_unix_test.go | 157 +++++++++++++++++----------- internal/dhcpd/v4.go | 30 ++++-- internal/dhcpd/v4_test.go | 4 + 5 files changed, 203 insertions(+), 130 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca77782c..273790b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,11 @@ and this project adheres to ### Added +- New `del` DHCP option which removes the corresponding option from server's + response ([#4337]). + + **NOTE:** This modifier affects all the parameters in the response and not + only the requested ones. - A new HTTP API, `GET /control/blocked_services/services`, that lists all available blocked services ([#4535]). @@ -39,6 +44,7 @@ and this project adheres to - Unnecessary logging of non-critical statistics errors ([#4850]). [#2993]: https://github.com/AdguardTeam/AdGuardHome/issues/2993 +[#4337]: https://github.com/AdguardTeam/AdGuardHome/issues/4337 [#4535]: https://github.com/AdguardTeam/AdGuardHome/issues/4535 [#4745]: https://github.com/AdguardTeam/AdGuardHome/issues/4745 [#4850]: https://github.com/AdguardTeam/AdGuardHome/issues/4850 @@ -231,9 +237,10 @@ See also the [v0.107.7 GitHub milestone][ms-v0.107.7]. seconds. - Domain-specific private reverse DNS upstream servers are now validated to allow only `*.in-addr.arpa` and `*.ip6.arpa` domains pointing to - locally-served networks ([#3381]). **Note:** If you already have invalid - entries in your configuration, consider removing them manually, since they - essentially had no effect. + locally-served networks ([#3381]). + + **NOTE:** If you already have invalid entries in your configuration, consider + removing them manually, since they essentially had no effect. - Response filtering is now performed using the record types of the answer section of messages as opposed to the type of the question ([#4238]). - Instead of adding the build time information, the build scripts now use the diff --git a/internal/dhcpd/options_unix.go b/internal/dhcpd/options_unix.go index 4e15ccce..e300729b 100644 --- a/internal/dhcpd/options_unix.go +++ b/internal/dhcpd/options_unix.go @@ -18,17 +18,14 @@ import ( // The aliases for DHCP option types available for explicit declaration. const ( - hexTyp = "hex" - ipTyp = "ip" - ipsTyp = "ips" - textTyp = "text" + typHex = "hex" + typIP = "ip" + typIPs = "ips" + typText = "text" + typDel = "del" ) -// parseDHCPOptionHex parses a DHCP option as a hex-encoded string. For -// example: -// -// 252 hex 736f636b733a2f2f70726f78792e6578616d706c652e6f7267 -// +// parseDHCPOptionHex parses a DHCP option as a hex-encoded string. func parseDHCPOptionHex(s string) (val dhcpv4.OptionValue, err error) { var data []byte data, err = hex.DecodeString(s) @@ -39,10 +36,7 @@ func parseDHCPOptionHex(s string) (val dhcpv4.OptionValue, err error) { return dhcpv4.OptionGeneric{Data: data}, nil } -// parseDHCPOptionIP parses a DHCP option as a single IP address. For example: -// -// 6 ip 192.168.1.1 -// +// parseDHCPOptionIP parses a DHCP option as a single IP address. 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. @@ -58,10 +52,7 @@ func parseDHCPOptionIP(s string) (val dhcpv4.OptionValue, err error) { } // parseDHCPOptionIPs parses a DHCP option as a comma-separates list of IP -// addresses. For example: -// -// 6 ips 192.168.1.1,192.168.1.2 -// +// addresses. func parseDHCPOptionIPs(s string) (val dhcpv4.OptionValue, err error) { var ips dhcpv4.IPs var ip net.IP @@ -78,23 +69,53 @@ func parseDHCPOptionIPs(s string) (val dhcpv4.OptionValue, err error) { } // parseDHCPOptionText parses a DHCP option as a simple UTF-8 encoded -// text. For example: -// -// 252 text http://192.168.1.1/wpad.dat -// +// text. func parseDHCPOptionText(s string) (val dhcpv4.OptionValue) { return dhcpv4.OptionGeneric{Data: []byte(s)} } -// parseDHCPOption parses an option. See the documentation of parseDHCPOption* -// for more info. +// 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 +func parseDHCPOptionVal(typ, valStr string) (val dhcpv4.OptionValue, err error) { + switch typ { + case typHex: + val, err = parseDHCPOptionHex(valStr) + case typIP: + val, err = parseDHCPOptionIP(valStr) + case typIPs: + val, err = parseDHCPOptionIPs(valStr) + case typText: + val = parseDHCPOptionText(valStr) + case typDel: + val = dhcpv4.OptionGeneric{Data: nil} + default: + err = fmt.Errorf("unknown option type %q", typ) + } + + return val, err +} + +// parseDHCPOption parses an option. See the documentation of +// parseDHCPOptionVal for more info. func parseDHCPOption(s string) (opt dhcpv4.Option, err error) { defer func() { err = errors.Annotate(err, "invalid option string %q: %w", s) }() s = strings.TrimSpace(s) parts := strings.SplitN(s, " ", 3) - if len(parts) < 3 { - return opt, errors.Error("need at least three fields") + + var valStr string + if pl := len(parts); pl < 3 { + if pl < 2 || parts[1] != typDel { + return opt, errors.Error("bad option format") + } + } else { + valStr = parts[2] } var code64 uint64 @@ -103,27 +124,16 @@ func parseDHCPOption(s string) (opt dhcpv4.Option, err error) { return opt, fmt.Errorf("parsing option code: %w", err) } - var optVal dhcpv4.OptionValue - switch typ, val := parts[1], parts[2]; typ { - case hexTyp: - optVal, err = parseDHCPOptionHex(val) - case ipTyp: - optVal, err = parseDHCPOptionIP(val) - case ipsTyp: - optVal, err = parseDHCPOptionIPs(val) - case textTyp: - optVal = parseDHCPOptionText(val) - default: - return opt, fmt.Errorf("unknown option type %q", typ) - } - + 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 dhcpv4.Option{ Code: dhcpv4.GenericOptionCode(code64), - Value: optVal, + Value: val, }, nil } @@ -139,40 +149,45 @@ func prepareOptions(conf V4ServerConf) (opts dhcpv4.Options) { // 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.Options{ + opts = dhcpv4.OptionsFromList( // IP-Layer Per Host - dhcpv4.OptionNonLocalSourceRouting.Code(): []byte{0}, + + dhcpv4.OptGeneric(dhcpv4.OptionNonLocalSourceRouting, []byte{0}), // Set the current recommended default time to live for the // Internet Protocol which is 64, see // https://datatracker.ietf.org/doc/html/rfc1700. - dhcpv4.OptionDefaultIPTTL.Code(): []byte{64}, + dhcpv4.OptGeneric(dhcpv4.OptionDefaultIPTTL, []byte{0x40}), - // IP-Layer Per Interface - - dhcpv4.OptionPerformMaskDiscovery.Code(): []byte{0}, - dhcpv4.OptionMaskSupplier.Code(): []byte{0}, - dhcpv4.OptionPerformRouterDiscovery.Code(): []byte{1}, + 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. - dhcpv4.OptionRouterSolicitationAddress.Code(): netutil.IPv4allrouter(), - dhcpv4.OptionBroadcastAddress.Code(): netutil.IPv4bcast(), + dhcpv4.Option{ + Code: dhcpv4.OptionRouterSolicitationAddress, + Value: dhcpv4.IP(netutil.IPv4allrouter()), + }, + dhcpv4.OptBroadcastAddress(netutil.IPv4bcast()), // Link-Layer Per Interface - dhcpv4.OptionTrailerEncapsulation.Code(): []byte{0}, - dhcpv4.OptionEthernetEncapsulation.Code(): []byte{0}, + dhcpv4.OptGeneric(dhcpv4.OptionTrailerEncapsulation, []byte{0}), + dhcpv4.OptGeneric(dhcpv4.OptionEthernetEncapsulation, []byte{0}), // TCP Per Host - dhcpv4.OptionTCPKeepaliveInterval.Code(): dhcpv4.Duration(0).ToBytes(), - dhcpv4.OptionTCPKeepaliveGarbage.Code(): []byte{0}, + dhcpv4.Option{ + Code: dhcpv4.OptionTCPKeepaliveInterval, + Value: dhcpv4.Duration(0), + }, + dhcpv4.OptGeneric(dhcpv4.OptionTCPKeepaliveGarbage, []byte{0}), // Values From Configuration - dhcpv4.OptionRouter.Code(): netutil.CloneIP(conf.subnet.IP), - dhcpv4.OptionSubnetMask.Code(): dhcpv4.IPMask(conf.subnet.Mask).ToBytes(), - } + dhcpv4.OptRouter(conf.subnet.IP), + dhcpv4.OptSubnetMask(conf.subnet.Mask), + ) // Set values for explicitly configured options. for i, o := range conf.Options { diff --git a/internal/dhcpd/options_unix_test.go b/internal/dhcpd/options_unix_test.go index 6b229f44..6e97c2de 100644 --- a/internal/dhcpd/options_unix_test.go +++ b/internal/dhcpd/options_unix_test.go @@ -8,103 +8,111 @@ import ( "net" "testing" + "github.com/AdguardTeam/golibs/testutil" "github.com/insomniacslk/dhcp/dhcpv4" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func TestParseOpt(t *testing.T) { testCases := []struct { name string in string - wantErrMsg string wantOpt dhcpv4.Option + wantErrMsg string }{{ - name: "hex_success", - in: "6 hex c0a80101c0a80102", - wantErrMsg: "", - wantOpt: dhcpv4.OptDNS( - net.IP{0xC0, 0xA8, 0x01, 0x01}, - net.IP{0xC0, 0xA8, 0x01, 0x02}, + name: "hex_success", + in: "6 hex c0a80101c0a80102", + wantOpt: dhcpv4.OptGeneric( + dhcpv4.GenericOptionCode(6), + []byte{ + 0xC0, 0xA8, 0x01, 0x01, + 0xC0, 0xA8, 0x01, 0x02, + }, ), + wantErrMsg: "", }, { - name: "ip_success", - in: "6 ip 1.2.3.4", + 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}), + }, wantErrMsg: "", - wantOpt: dhcpv4.OptDNS( - net.IP{0x01, 0x02, 0x03, 0x04}, - ), }, { name: "ip_fail_v6", in: "6 ip ::1234", - wantErrMsg: "invalid option string \"6 ip ::1234\": bad ipv4 address \"::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", + 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}, + }), + }, wantErrMsg: "", - wantOpt: dhcpv4.OptDNS( - net.IP{0xC0, 0xA8, 0x01, 0x01}, - net.IP{0xC0, 0xA8, 0x01, 0x02}, - ), }, { - name: "text_success", - in: "252 text http://192.168.1.1/", - wantErrMsg: "", + name: "text_success", + in: "252 text http://192.168.1.1/", wantOpt: dhcpv4.OptGeneric( dhcpv4.GenericOptionCode(252), []byte("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}, + }, + wantErrMsg: "", }, { name: "bad_parts", in: "6 ip", - wantErrMsg: `invalid option string "6 ip": need at least three fields`, wantOpt: dhcpv4.Option{}, + wantErrMsg: `invalid option string "6 ip": bad option format`, }, { - name: "bad_code", - in: "256 ip 1.1.1.1", + name: "bad_code", + in: "256 ip 1.1.1.1", + wantOpt: dhcpv4.Option{}, wantErrMsg: `invalid option string "256 ip 1.1.1.1": parsing option code: ` + `strconv.ParseUint: parsing "256": value out of range`, - wantOpt: dhcpv4.Option{}, }, { name: "bad_type", in: "6 bad 1.1.1.1", - wantErrMsg: `invalid option string "6 bad 1.1.1.1": unknown option type "bad"`, wantOpt: dhcpv4.Option{}, + wantErrMsg: `invalid option string "6 bad 1.1.1.1": unknown option type "bad"`, }, { - name: "hex_error", - in: "6 hex ZZZ", + name: "hex_error", + in: "6 hex ZZZ", + wantOpt: dhcpv4.Option{}, wantErrMsg: `invalid option string "6 hex ZZZ": decoding hex: ` + `encoding/hex: invalid byte: U+005A 'Z'`, - wantOpt: dhcpv4.Option{}, }, { name: "ip_error", in: "6 ip 1.2.3.x", - wantErrMsg: "invalid option string \"6 ip 1.2.3.x\": bad ipv4 address \"1.2.3.x\"", wantOpt: dhcpv4.Option{}, + 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", + name: "ips_error", + in: "6 ips 192.168.1.1,192.168.1.x", + wantOpt: dhcpv4.Option{}, 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\"", - wantOpt: dhcpv4.Option{}, }} for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { opt, err := parseDHCPOption(tc.in) - if tc.wantErrMsg != "" { - require.Error(t, err) + testutil.AssertErrorMsg(t, tc.wantErrMsg, err) - assert.Equal(t, tc.wantErrMsg, err.Error()) - - return - } - - require.NoError(t, 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.Code.Code(), opt.Code.Code()) + // assert.Equal(t, tc.wantOpt.Value.ToBytes(), opt.Value.ToBytes()) + assert.Equal(t, tc.wantOpt, opt) }) } } @@ -127,51 +135,80 @@ func TestPrepareOptions(t *testing.T) { testCases := []struct { name string - opts []string checks dhcpv4.Options + opts []string }{{ name: "all_default", checks: allDefault, + opts: nil, }, { name: "configured_ip", - opts: []string{ - fmt.Sprintf("%d ip %s", dhcpv4.OptionBroadcastAddress, oneIP), - }, checks: dhcpv4.Options{ dhcpv4.OptionBroadcastAddress.Code(): oneIP, }, + opts: []string{ + fmt.Sprintf("%d ip %s", dhcpv4.OptionBroadcastAddress, oneIP), + }, }, { name: "configured_ips", - opts: []string{ - fmt.Sprintf("%d ips %s,%s", dhcpv4.OptionDomainNameServer, oneIP, otherIP), - }, checks: dhcpv4.Options{ dhcpv4.OptionDomainNameServer.Code(): append(oneIP, otherIP...), }, + opts: []string{ + fmt.Sprintf("%d ips %s,%s", dhcpv4.OptionDomainNameServer, oneIP, otherIP), + }, }, { - name: "configured_bad", + name: "configured_bad", + checks: allDefault, opts: []string{ "20 hex", "23 hex abc", "32 ips 1,2,3,4", "28 256.256.256.256", }, - checks: allDefault, + }, { + name: "configured_del", + checks: dhcpv4.Options{ + dhcpv4.OptionBroadcastAddress.Code(): nil, + }, + opts: []string{ + "28 del", + }, + }, { + name: "rewritten_del", + checks: dhcpv4.Options{ + dhcpv4.OptionBroadcastAddress.Code(): []byte{255, 255, 255, 255}, + }, + opts: []string{ + "28 del", + "28 ip 255.255.255.255", + }, + }, { + name: "configured_and_del", + checks: dhcpv4.Options{ + 123: []byte("cba"), + }, + opts: []string{ + "123 text abc", + "123 del", + "123 text cba", + }, }} for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + if tc.name == "configured_del" { + assert.True(t, true) + } opts := prepareOptions(V4ServerConf{ // Just to avoid nil pointer dereference. subnet: &net.IPNet{}, Options: tc.opts, }) for c, v := range tc.checks { - optVal := opts.Get(dhcpv4.GenericOptionCode(c)) - require.NotNil(t, optVal) - - assert.Len(t, optVal, len(v)) - assert.Equal(t, v, optVal) + val := opts.Get(dhcpv4.GenericOptionCode(c)) + assert.Lenf(t, val, len(v), "Code: %v", c) + assert.Equal(t, v, val) } }) } diff --git a/internal/dhcpd/v4.go b/internal/dhcpd/v4.go index 4fb85552..3de88bbe 100644 --- a/internal/dhcpd/v4.go +++ b/internal/dhcpd/v4.go @@ -875,28 +875,32 @@ func (s *v4Server) process(req, resp *dhcpv4.DHCPv4) int { resp.YourIPAddr = netutil.CloneIP(l.IP) } - // Set IP address lease time for all DHCPOFFER messages and DHCPACK - // messages replied for DHCPREQUEST. + // 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 { + 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 configured := s.options; configured.Has(code) { - resp.UpdateOption(dhcpv4.OptGeneric(code, configured.Get(code))) + if val := s.options.Get(code); val != nil { + resp.UpdateOption(dhcpv4.Option{ + Code: code, + Value: dhcpv4.OptionGeneric{Data: s.options.Get(code)}, + }) } } - // Update the value of Domain Name Server option separately from others if - // not assigned yet since its value is set after server's creating. - if requested.Has(dhcpv4.OptionDomainNameServer) && - !resp.Options.Has(dhcpv4.OptionDomainNameServer) { - resp.UpdateOption(dhcpv4.OptDNS(s.conf.dnsIPAddrs...)) - } return 1 } @@ -924,6 +928,7 @@ func (s *v4Server) packetHandler(conn net.PacketConn, peer net.Addr, req *dhcpv4 resp, err := dhcpv4.NewReplyFromRequest(req) if err != nil { log.Debug("dhcpv4: dhcpv4.New: %s", err) + return } @@ -1020,6 +1025,11 @@ func (s *v4Server) Start() (err error) { // No available IP addresses which may appear later. return nil } + // 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...)) + } s.conf.dnsIPAddrs = dnsIPAddrs diff --git a/internal/dhcpd/v4_test.go b/internal/dhcpd/v4_test.go index e720d113..2136b072 100644 --- a/internal/dhcpd/v4_test.go +++ b/internal/dhcpd/v4_test.go @@ -345,6 +345,8 @@ func TestV4Server_Process_optionsPriority(t *testing.T) { stringutil.WriteToBuilder(b, ",", ip.String()) } conf.Options = []string{b.String()} + } else { + defer func() { s.options.Update(dhcpv4.OptDNS(defaultIP)) }() } ss, err := v4Create(conf) @@ -407,6 +409,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...)) l := &Lease{ Hostname: "static-1.local", @@ -495,6 +498,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...)) var req, resp *dhcpv4.DHCPv4 mac := net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}