From 2eb21ef40901e893069e9e4b6fa902d759d510b5 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Thu, 11 Feb 2021 20:49:03 +0300 Subject: [PATCH] Pull request: dhcpd: do not override ra-slaac settings Merge in DNS/adguard-home from 2653-ra-slaac to master Updates #2653. Squashed commit of the following: commit f261413a58dc813e37cc848606ed490b8c0ac9f3 Author: Ainar Garipov Date: Thu Feb 11 20:37:13 2021 +0300 all: doc changes, rm debug commit 4a8c6e4897579493c1ca242fb8f0f440c3b51a74 Author: Ainar Garipov Date: Thu Feb 11 20:11:46 2021 +0300 dhcpd: do not override ra-slaac settings --- CHANGELOG.md | 4 +- internal/dhcpd/dhcpd.go | 19 +++--- internal/dhcpd/dhcphttp.go | 106 +++++++++++++++++++------------- internal/dhcpd/nullbool.go | 58 +++++++++++++++++ internal/dhcpd/nullbool_test.go | 69 +++++++++++++++++++++ internal/dhcpd/routeradv.go | 10 +-- internal/dhcpd/server.go | 4 +- internal/dhcpd/v6.go | 6 +- internal/sysutil/net_linux.go | 12 ++-- staticcheck.conf | 2 + 10 files changed, 223 insertions(+), 67 deletions(-) create mode 100644 internal/dhcpd/nullbool.go create mode 100644 internal/dhcpd/nullbool_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f1e8d9c8..097f5c46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,13 +26,15 @@ and this project adheres to ### Fixed +- DHCPv6 server's `ra_slaac_only` and `ra_allow_slaac` settings aren't reset to + `false` on update any more ([#2653]). - The `Vary` header is now added along with `Access-Control-Allow-Origin` to prevent cache-related and other issues in browsers ([#2658]). - domain, but with an HTTP scheme as opposed to `*` ([#2484]). - The request body size limit is now set for HTTPS requests as well. - Incorrect version tag in the Docker release ([#2663]). - DNSCrypt queries weren't marked as such in logs ([#2662]). +[#2653]: https://github.com/AdguardTeam/AdGuardHome/issues/2653 [#2658]: https://github.com/AdguardTeam/AdGuardHome/issues/2658 [#2662]: https://github.com/AdguardTeam/AdGuardHome/issues/2662 [#2663]: https://github.com/AdguardTeam/AdGuardHome/issues/2663 diff --git a/internal/dhcpd/dhcpd.go b/internal/dhcpd/dhcpd.go index 18509991..bcd35d1c 100644 --- a/internal/dhcpd/dhcpd.go +++ b/internal/dhcpd/dhcpd.go @@ -117,14 +117,14 @@ type ServerInterface interface { } // Create - create object -func Create(config ServerConfig) *Server { +func Create(conf ServerConfig) *Server { s := &Server{} - s.conf.Enabled = config.Enabled - s.conf.InterfaceName = config.InterfaceName - s.conf.HTTPRegister = config.HTTPRegister - s.conf.ConfigModified = config.ConfigModified - s.conf.DBFilePath = filepath.Join(config.WorkDir, dbFilename) + s.conf.Enabled = conf.Enabled + s.conf.InterfaceName = conf.InterfaceName + s.conf.HTTPRegister = conf.HTTPRegister + s.conf.ConfigModified = conf.ConfigModified + s.conf.DBFilePath = filepath.Join(conf.WorkDir, dbFilename) if !webHandlersRegistered && s.conf.HTTPRegister != nil { if runtime.GOOS == "windows" { @@ -145,7 +145,7 @@ func Create(config ServerConfig) *Server { } var err4, err6 error - v4conf := config.Conf4 + v4conf := conf.Conf4 v4conf.Enabled = s.conf.Enabled if len(v4conf.RangeStart) == 0 { v4conf.Enabled = false @@ -154,7 +154,7 @@ func Create(config ServerConfig) *Server { v4conf.notify = s.onNotify s.srv4, err4 = v4Create(v4conf) - v6conf := config.Conf6 + v6conf := conf.Conf6 v6conf.Enabled = s.conf.Enabled if len(v6conf.RangeStart) == 0 { v6conf.Enabled = false @@ -172,6 +172,9 @@ func Create(config ServerConfig) *Server { return nil } + s.conf.Conf4 = conf.Conf4 + s.conf.Conf6 = conf.Conf6 + if s.conf.Enabled && !v4conf.Enabled && !v6conf.Enabled { log.Error("Can't enable DHCP server because neither DHCPv4 nor DHCPv6 servers are configured") return nil diff --git a/internal/dhcpd/dhcphttp.go b/internal/dhcpd/dhcphttp.go index b6b5c729..0a182cf0 100644 --- a/internal/dhcpd/dhcphttp.go +++ b/internal/dhcpd/dhcphttp.go @@ -11,7 +11,6 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/sysutil" "github.com/AdguardTeam/AdGuardHome/internal/util" - "github.com/AdguardTeam/golibs/jsonutil" "github.com/AdguardTeam/golibs/log" ) @@ -29,7 +28,11 @@ type v4ServerConfJSON struct { LeaseDuration uint32 `json:"lease_duration"` } -func v4JSONToServerConf(j v4ServerConfJSON) V4ServerConf { +func v4JSONToServerConf(j *v4ServerConfJSON) V4ServerConf { + if j == nil { + return V4ServerConf{} + } + return V4ServerConf{ GatewayIP: j.GatewayIP, SubnetMask: j.SubnetMask, @@ -44,7 +47,11 @@ type v6ServerConfJSON struct { LeaseDuration uint32 `json:"lease_duration"` } -func v6JSONToServerConf(j v6ServerConfJSON) V6ServerConf { +func v6JSONToServerConf(j *v6ServerConfJSON) V6ServerConf { + if j == nil { + return V6ServerConf{} + } + return V6ServerConf{ RangeStart: j.RangeStart, LeaseDuration: j.LeaseDuration, @@ -83,13 +90,6 @@ func (s *Server) handleDHCPStatus(w http.ResponseWriter, r *http.Request) { } } -type dhcpServerConfigJSON struct { - Enabled bool `json:"enabled"` - InterfaceName string `json:"interface_name"` - V4 v4ServerConfJSON `json:"v4"` - V6 v6ServerConfJSON `json:"v6"` -} - func (s *Server) enableDHCP(ifaceName string) (code int, err error) { var hasStaticIP bool hasStaticIP, err = sysutil.IfaceHasStaticIP(ifaceName) @@ -112,14 +112,22 @@ func (s *Server) enableDHCP(ifaceName string) (code int, err error) { return 0, nil } -func (s *Server) handleDHCPSetConfig(w http.ResponseWriter, r *http.Request) { - newconfig := dhcpServerConfigJSON{} - newconfig.Enabled = s.conf.Enabled - newconfig.InterfaceName = s.conf.InterfaceName +type dhcpServerConfigJSON struct { + V4 *v4ServerConfJSON `json:"v4"` + V6 *v6ServerConfJSON `json:"v6"` + InterfaceName string `json:"interface_name"` + Enabled nullBool `json:"enabled"` +} - js, err := jsonutil.DecodeObject(&newconfig, r.Body) +func (s *Server) handleDHCPSetConfig(w http.ResponseWriter, r *http.Request) { + conf := dhcpServerConfigJSON{} + conf.Enabled = boolToNullBool(s.conf.Enabled) + conf.InterfaceName = s.conf.InterfaceName + + err := json.NewDecoder(r.Body).Decode(&conf) if err != nil { - httpError(r, w, http.StatusBadRequest, "Failed to parse new DHCP config json: %s", err) + httpError(r, w, http.StatusBadRequest, + "failed to parse new dhcp config json: %s", err) return } @@ -129,62 +137,72 @@ func (s *Server) handleDHCPSetConfig(w http.ResponseWriter, r *http.Request) { v4Enabled := false v6Enabled := false - if js.Exists("v4") { - v4conf := v4JSONToServerConf(newconfig.V4) - v4conf.Enabled = newconfig.Enabled - if len(v4conf.RangeStart) == 0 { - v4conf.Enabled = false + if conf.V4 != nil { + v4Conf := v4JSONToServerConf(conf.V4) + v4Conf.Enabled = conf.Enabled == nbTrue + if len(v4Conf.RangeStart) == 0 { + v4Conf.Enabled = false } - v4Enabled = v4conf.Enabled - v4conf.InterfaceName = newconfig.InterfaceName + v4Enabled = v4Conf.Enabled + v4Conf.InterfaceName = conf.InterfaceName c4 := V4ServerConf{} s.srv4.WriteDiskConfig4(&c4) - v4conf.notify = c4.notify - v4conf.ICMPTimeout = c4.ICMPTimeout + v4Conf.notify = c4.notify + v4Conf.ICMPTimeout = c4.ICMPTimeout - s4, err = v4Create(v4conf) + s4, err = v4Create(v4Conf) if err != nil { - httpError(r, w, http.StatusBadRequest, "invalid dhcpv4 configuration: %s", err) + httpError(r, w, http.StatusBadRequest, + "invalid dhcpv4 configuration: %s", err) return } } - if js.Exists("v6") { - v6conf := v6JSONToServerConf(newconfig.V6) - v6conf.Enabled = newconfig.Enabled - if len(v6conf.RangeStart) == 0 { - v6conf.Enabled = false + if conf.V6 != nil { + v6Conf := v6JSONToServerConf(conf.V6) + v6Conf.Enabled = conf.Enabled == nbTrue + if len(v6Conf.RangeStart) == 0 { + v6Conf.Enabled = false } - v6Enabled = v6conf.Enabled - v6conf.InterfaceName = newconfig.InterfaceName - v6conf.notify = s.onNotify + // Don't overwrite the RA/SLAAC settings from the config file. + // + // TODO(a.garipov): Perhaps include them into the request to + // allow changing them from the HTTP API? + v6Conf.RASLAACOnly = s.conf.Conf6.RASLAACOnly + v6Conf.RAAllowSLAAC = s.conf.Conf6.RAAllowSLAAC - s6, err = v6Create(v6conf) + v6Enabled = v6Conf.Enabled + v6Conf.InterfaceName = conf.InterfaceName + v6Conf.notify = s.onNotify + + s6, err = v6Create(v6Conf) if err != nil { - httpError(r, w, http.StatusBadRequest, "invalid dhcpv6 configuration: %s", err) + httpError(r, w, http.StatusBadRequest, + "invalid dhcpv6 configuration: %s", err) return } } - if newconfig.Enabled && !v4Enabled && !v6Enabled { - httpError(r, w, http.StatusBadRequest, "dhcpv4 or dhcpv6 configuration must be complete") + if conf.Enabled == nbTrue && !v4Enabled && !v6Enabled { + httpError(r, w, http.StatusBadRequest, + "dhcpv4 or dhcpv6 configuration must be complete") return } s.Stop() - if js.Exists("enabled") { - s.conf.Enabled = newconfig.Enabled + if conf.Enabled != nbNull { + s.conf.Enabled = conf.Enabled == nbTrue } - if js.Exists("interface_name") { - s.conf.InterfaceName = newconfig.InterfaceName + if conf.InterfaceName != "" { + s.conf.InterfaceName = conf.InterfaceName } if s4 != nil { @@ -200,7 +218,7 @@ func (s *Server) handleDHCPSetConfig(w http.ResponseWriter, r *http.Request) { if s.conf.Enabled { var code int - code, err = s.enableDHCP(newconfig.InterfaceName) + code, err = s.enableDHCP(conf.InterfaceName) if err != nil { httpError(r, w, code, "enabling dhcp: %s", err) diff --git a/internal/dhcpd/nullbool.go b/internal/dhcpd/nullbool.go new file mode 100644 index 00000000..b07f6768 --- /dev/null +++ b/internal/dhcpd/nullbool.go @@ -0,0 +1,58 @@ +package dhcpd + +import ( + "bytes" + "fmt" +) + +// nullBool is a nullable boolean. Use these in JSON requests and responses +// instead of pointers to bool. +// +// TODO(a.garipov): Inspect uses of *bool, move this type into some new package +// if we need it somewhere else. +type nullBool uint8 + +// nullBool values +const ( + nbNull nullBool = iota + nbTrue + nbFalse +) + +// String implements the fmt.Stringer interface for nullBool. +func (nb nullBool) String() (s string) { + switch nb { + case nbNull: + return "null" + case nbTrue: + return "true" + case nbFalse: + return "false" + } + + return fmt.Sprintf("!invalid nullBool %d", uint8(nb)) +} + +// boolToNullBool converts a bool into a nullBool. +func boolToNullBool(cond bool) (nb nullBool) { + if cond { + return nbTrue + } + + return nbFalse +} + +// UnmarshalJSON implements the json.Unmarshaler interface for *nullBool. +func (nb *nullBool) UnmarshalJSON(b []byte) (err error) { + if len(b) == 0 || bytes.Equal(b, []byte("null")) { + *nb = nbNull + } else if bytes.Equal(b, []byte("true")) { + *nb = nbTrue + } else if bytes.Equal(b, []byte("false")) { + *nb = nbFalse + } else { + return fmt.Errorf("invalid nullBool value %q", b) + } + + return nil +} diff --git a/internal/dhcpd/nullbool_test.go b/internal/dhcpd/nullbool_test.go new file mode 100644 index 00000000..2570dd44 --- /dev/null +++ b/internal/dhcpd/nullbool_test.go @@ -0,0 +1,69 @@ +package dhcpd + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNullBool_UnmarshalText(t *testing.T) { + testCases := []struct { + name string + data []byte + wantErrMsg string + want nullBool + }{{ + name: "empty", + data: []byte{}, + wantErrMsg: "", + want: nbNull, + }, { + name: "null", + data: []byte("null"), + wantErrMsg: "", + want: nbNull, + }, { + name: "true", + data: []byte("true"), + wantErrMsg: "", + want: nbTrue, + }, { + name: "false", + data: []byte("false"), + wantErrMsg: "", + want: nbFalse, + }, { + name: "invalid", + data: []byte("flase"), + wantErrMsg: `invalid nullBool value "flase"`, + want: nbNull, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var got nullBool + err := got.UnmarshalJSON(tc.data) + if tc.wantErrMsg == "" { + assert.Nil(t, err) + } else { + require.NotNil(t, err) + assert.Equal(t, tc.wantErrMsg, err.Error()) + } + + assert.Equal(t, tc.want, got) + }) + } + + t.Run("json", func(t *testing.T) { + want := nbTrue + var got struct { + A nullBool + } + + err := json.Unmarshal([]byte(`{"A":true}`), &got) + require.Nil(t, err) + assert.Equal(t, want, got.A) + }) +} diff --git a/internal/dhcpd/routeradv.go b/internal/dhcpd/routeradv.go index f1d63c7d..59aad9d1 100644 --- a/internal/dhcpd/routeradv.go +++ b/internal/dhcpd/routeradv.go @@ -13,8 +13,8 @@ import ( ) type raCtx struct { - raAllowSlaac bool // send RA packets without MO flags - raSlaacOnly bool // send RA packets with MO flags + raAllowSLAAC bool // send RA packets without MO flags + raSLAACOnly bool // send RA packets with MO flags ipAddr net.IP // source IP address (link-local-unicast) dnsIPAddr net.IP // IP address for DNS Server option prefixIPAddr net.IP // IP address for Prefix option @@ -159,7 +159,7 @@ func createICMPv6RAPacket(params icmpv6RA) []byte { func (ra *raCtx) Init() error { ra.stop.Store(0) ra.conn = nil - if !(ra.raAllowSlaac || ra.raSlaacOnly) { + if !(ra.raAllowSLAAC || ra.raSLAACOnly) { return nil } @@ -167,8 +167,8 @@ func (ra *raCtx) Init() error { ra.ipAddr, ra.dnsIPAddr) params := icmpv6RA{ - managedAddressConfiguration: !ra.raSlaacOnly, - otherConfiguration: !ra.raSlaacOnly, + managedAddressConfiguration: !ra.raSLAACOnly, + otherConfiguration: !ra.raSLAACOnly, mtu: uint32(ra.iface.MTU), prefixLen: 64, recursiveDNSServer: ra.dnsIPAddr, diff --git a/internal/dhcpd/server.go b/internal/dhcpd/server.go index 4adbca5a..1d3ce460 100644 --- a/internal/dhcpd/server.go +++ b/internal/dhcpd/server.go @@ -83,8 +83,8 @@ type V6ServerConf struct { LeaseDuration uint32 `yaml:"lease_duration" json:"lease_duration"` // in seconds - RaSlaacOnly bool `yaml:"ra_slaac_only" json:"-"` // send ICMPv6.RA packets without MO flags - RaAllowSlaac bool `yaml:"ra_allow_slaac" json:"-"` // send ICMPv6.RA packets with MO flags + RASLAACOnly bool `yaml:"ra_slaac_only" json:"-"` // send ICMPv6.RA packets without MO flags + RAAllowSLAAC bool `yaml:"ra_allow_slaac" json:"-"` // send ICMPv6.RA packets with MO flags ipStart net.IP // starting IP address for dynamic leases leaseTime time.Duration // the time during which a dynamic lease is considered valid diff --git a/internal/dhcpd/v6.go b/internal/dhcpd/v6.go index 51788551..0b43d6d1 100644 --- a/internal/dhcpd/v6.go +++ b/internal/dhcpd/v6.go @@ -552,8 +552,8 @@ func (s *v6Server) initRA(iface *net.Interface) error { } } - s.ra.raAllowSlaac = s.conf.RaAllowSlaac - s.ra.raSlaacOnly = s.conf.RaSlaacOnly + s.ra.raAllowSLAAC = s.conf.RAAllowSLAAC + s.ra.raSLAACOnly = s.conf.RASLAACOnly s.ra.dnsIPAddr = s.ra.ipAddr s.ra.prefixIPAddr = s.conf.ipStart s.ra.ifaceName = s.conf.InterfaceName @@ -594,7 +594,7 @@ func (s *v6Server) Start() error { } // don't initialize DHCPv6 server if we must force the clients to use SLAAC - if s.conf.RaSlaacOnly { + if s.conf.RASLAACOnly { log.Debug("DHCPv6: not starting DHCPv6 server due to ra_slaac_only=true") return nil } diff --git a/internal/sysutil/net_linux.go b/internal/sysutil/net_linux.go index 8f47cf42..9205fd5c 100644 --- a/internal/sysutil/net_linux.go +++ b/internal/sysutil/net_linux.go @@ -33,15 +33,19 @@ func ifaceHasStaticIP(ifaceName string) (has bool, err error) { filePath: "/etc/network/interfaces", }} { f, err = os.Open(check.filePath) - if errors.Is(err, os.ErrNotExist) { - continue - } if err != nil { + if errors.Is(err, os.ErrNotExist) { + err = nil + + continue + } + return false, err } defer f.Close() - fileReadCloser, err := aghio.LimitReadCloser(f, maxConfigFileSize) + var fileReadCloser io.ReadCloser + fileReadCloser, err = aghio.LimitReadCloser(f, maxConfigFileSize) if err != nil { return false, err } diff --git a/staticcheck.conf b/staticcheck.conf index 146f83cb..4dd93176 100644 --- a/staticcheck.conf +++ b/staticcheck.conf @@ -10,7 +10,9 @@ initialisms = [ , "MX" , "PTR" , "QUIC" +, "RA" , "SDNS" +, "SLAAC" , "SVCB" ] dot_import_whitelist = []