Pull request: home: add bootstrap to mobileconfig, imp code

Updates #3568.

Squashed commit of the following:

commit ec342e6223e2b2efe9a8bf833d5406a44c6417e4
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Sep 13 15:16:07 2021 +0300

    home: imp tests

commit 67cd771e631938d3e8a5340315314210de796174
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Sep 13 14:34:03 2021 +0300

    home: add bootstrap to mobileconfig, imp code

CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to
 
 ### Added
 
+- Bootstrap DNS server IPs to the `mobileconfig` API responses ([#3568]).
 - Setting the timeout for IP address pinging in the "Fastest IP address" mode
   through the new `fastest_timeout` field in the configuration file ([#1992]).
 - Static IP address detection on FreeBSD ([#3289]).
@@ -189,6 +190,7 @@ In this release, the schema version has changed from 10 to 12.
 [#3506]: https://github.com/AdguardTeam/AdGuardHome/issues/3506
 [#3551]: https://github.com/AdguardTeam/AdGuardHome/issues/3551
 [#3564]: https://github.com/AdguardTeam/AdGuardHome/issues/3564
+[#3568]: https://github.com/AdguardTeam/AdGuardHome/issues/3568
 
 
 

internal/aghnet/etchostscontainer_test.go

@@ -117,8 +117,8 @@ func TestEtcHostsContainerFSNotify(t *testing.T) {
 	assertWriting(t, f, "127.0.0.2   newhost\n")
 	require.NoError(t, f.Sync())
 
-	// Wait until fsnotify has triggerred and processed the
+	// Wait until fsnotify has triggered and processed the file-modification
-	// file-modification event.
+	// event.
 	time.Sleep(50 * time.Millisecond)
 
 	t.Run("notified", func(t *testing.T) {

internal/aghtest/upstream.go

@@ -167,7 +167,7 @@ func (u *TestBlockUpstream) RequestsCount() int {
 // TestErrUpstream implements upstream.Upstream interface for replacing real
 // upstream in tests.
 type TestErrUpstream struct {
-	// The error returned by Exchange may be unwraped to the Err.
+	// The error returned by Exchange may be unwrapped to the Err.
 	Err error
 }
 

internal/aghtime/duration.go

@@ -38,7 +38,7 @@ func (d Duration) String() (str string) {
 		rounded == 0,
 		rounded*time.Second != d.Duration,
 		rounded%60 != 0:
-		// Return the uncutted value if it's either equal to zero or has
+		// Return the uncut value if it's either equal to zero or has
 		// fractions of a second or even whole seconds in it.
 		return str
 
@@ -60,7 +60,7 @@ func (d Duration) MarshalText() (text []byte, err error) {
 //
 // TODO(e.burkov): Make it able to parse larger units like days.
 func (d *Duration) UnmarshalText(b []byte) (err error) {
-	defer func() { err = errors.Annotate(err, "unmarshalling duration: %w") }()
+	defer func() { err = errors.Annotate(err, "unmarshaling duration: %w") }()
 
 	d.Duration, err = time.ParseDuration(string(b))
 

internal/aghtime/duration_test.go

@@ -60,7 +60,7 @@ func TestDuration_String(t *testing.T) {
 }
 
 // durationEncodingTester is a helper struct to simplify testing different
-// Duration marshalling and unmarshalling cases.
+// Duration marshalling and unmarshaling cases.
 type durationEncodingTester struct {
 	PtrMap   map[string]*Duration `json:"ptr_map"   yaml:"ptr_map"`
 	PtrSlice []*Duration          `json:"ptr_slice" yaml:"ptr_slice"`
@@ -104,7 +104,7 @@ const (
 // Duration.
 const defaultTestDur = time.Millisecond
 
-// checkFields verifies m's fields.  It expects the m to be unmarshalled from
+// checkFields verifies m's fields.  It expects the m to be unmarshaled from
 // one of the constant strings above.
 func (m *durationEncodingTester) checkFields(t *testing.T, d Duration) {
 	t.Run("pointers_map", func(t *testing.T) {

internal/dnsforward/dns.go

@@ -340,7 +340,7 @@ func (s *Server) processRestrictLocal(ctx *dnsContext) (rc resultCode) {
 
 	// Restrict an access to local addresses for external clients.  We also
 	// assume that all the DHCP leases we give are locally-served or at
-	// least don't need to be unaccessable externally.
+	// least don't need to be inaccessible externally.
 	if s.subnetDetector.IsLocallyServedNetwork(ip) {
 		if !ctx.isLocalClient {
 			log.Debug("dns: %q requests for internal ip", d.Addr)

internal/dnsforward/dnsforward.go

@@ -364,7 +364,7 @@ func (s *Server) startLocked() error {
 const defaultLocalTimeout = 1 * time.Second
 
 // collectDNSIPAddrs returns IP addresses the server is listening on without
-// port numbersю  For internal use only.
+// port numbers.  For internal use only.
 func (s *Server) collectDNSIPAddrs() (addrs []string, err error) {
 	addrs = make([]string, len(s.conf.TCPListenAddrs)+len(s.conf.UDPListenAddrs))
 	var i int

internal/dnsforward/dnsforward_test.go

@@ -275,7 +275,7 @@ func TestServer(t *testing.T) {
 			client := dns.Client{Net: tc.net}
 
 			reply, _, err := client.Exchange(createGoogleATestMessage(), addr.String())
-			require.NoErrorf(t, err, "сouldn't talk to server %s: %s", addr, err)
+			require.NoErrorf(t, err, "couldn't talk to server %s: %s", addr, err)
 
 			assertGoogleAResponse(t, reply)
 		})
@@ -330,7 +330,7 @@ func TestServerWithProtectionDisabled(t *testing.T) {
 	client := &dns.Client{}
 
 	reply, _, err := client.Exchange(req, addr.String())
-	require.NoErrorf(t, err, "сouldn't talk to server %s: %s", addr, err)
+	require.NoErrorf(t, err, "couldn't talk to server %s: %s", addr, err)
 	assertGoogleAResponse(t, reply)
 }
 

internal/home/config.go

@@ -162,8 +162,10 @@ type tlsConfigSettings struct {
 	dnsforward.TLSConfig `yaml:",inline" json:",inline"`
 }
 
-// initialize to default values, will be changed later when reading config or parsing command line
+// config is the global configuration structure.
-var config = configuration{
+//
+// TODO(a.garipov, e.burkov): This global is afwul and must be removed.
+var config = &configuration{
 	BindPort:     3000,
 	BetaBindPort: 0,
 	BindHost:     net.IP{0, 0, 0, 0},

internal/home/controlinstall.go

@@ -15,8 +15,8 @@ import (
 	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
+	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
-	"github.com/AdguardTeam/golibs/netutil"
 )
 
 // getAddrsResponse is the response for /install/get_addresses endpoint.
@@ -286,55 +286,29 @@ func shutdownSrv(ctx context.Context, cancel context.CancelFunc, srv *http.Serve
 
 // Apply new configuration, start DNS server, restart Web server
 func (web *Web) handleInstallConfigure(w http.ResponseWriter, r *http.Request) {
-	req := applyConfigReq{}
+	req, restartHTTP, err := decodeApplyConfigReq(r.Body)
-	err := json.NewDecoder(r.Body).Decode(&req)
 	if err != nil {
-		httpError(w, http.StatusBadRequest, "Failed to parse 'configure' JSON: %s", err)
+		httpError(w, http.StatusBadRequest, "%s", err)
+
 		return
 	}
 
-	if req.Web.Port == 0 || req.DNS.Port == 0 {
-		httpError(w, http.StatusBadRequest, "port value can't be 0")
-		return
-	}
-
-	restartHTTP := true
-	if config.BindHost.Equal(req.Web.IP) && config.BindPort == req.Web.Port {
-		// no need to rebind
-		restartHTTP = false
-	}
-
-	// validate that hosts and ports are bindable
-	if restartHTTP {
-		err = aghnet.CheckPortAvailable(req.Web.IP, req.Web.Port)
-		if err != nil {
-			httpError(
-				w,
-				http.StatusBadRequest,
-				"can not listen on IP:port %s: %s",
-				netutil.JoinHostPort(req.Web.IP.String(), req.Web.Port),
-				err,
-			)
-
-			return
-		}
-
-	}
-
 	err = aghnet.CheckPacketPortAvailable(req.DNS.IP, req.DNS.Port)
 	if err != nil {
 		httpError(w, http.StatusBadRequest, "%s", err)
+
 		return
 	}
 
 	err = aghnet.CheckPortAvailable(req.DNS.IP, req.DNS.Port)
 	if err != nil {
 		httpError(w, http.StatusBadRequest, "%s", err)
+
 		return
 	}
 
-	var curConfig configuration
+	var curConfig *configuration
-	copyInstallSettings(&curConfig, &config)
+	copyInstallSettings(curConfig, config)
 
 	Context.firstRun = false
 	config.BindHost = req.Web.IP
@@ -349,8 +323,9 @@ func (web *Web) handleInstallConfigure(w http.ResponseWriter, r *http.Request) {
 	err = StartMods()
 	if err != nil {
 		Context.firstRun = true
-		copyInstallSettings(&config, &curConfig)
+		copyInstallSettings(config, curConfig)
 		httpError(w, http.StatusInternalServerError, "%s", err)
+
 		return
 	}
 
@@ -361,8 +336,9 @@ func (web *Web) handleInstallConfigure(w http.ResponseWriter, r *http.Request) {
 	err = config.write()
 	if err != nil {
 		Context.firstRun = true
-		copyInstallSettings(&config, &curConfig)
+		copyInstallSettings(config, curConfig)
 		httpError(w, http.StatusInternalServerError, "Couldn't write config: %s", err)
+
 		return
 	}
 
@@ -387,6 +363,36 @@ func (web *Web) handleInstallConfigure(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
+// decodeApplyConfigReq decodes the configuration, validates some parameters,
+// and returns it along with the boolean indicating whether or not the HTTP
+// server must be restarted.
+func decodeApplyConfigReq(r io.Reader) (req *applyConfigReq, restartHTTP bool, err error) {
+	req = &applyConfigReq{}
+	err = json.NewDecoder(r).Decode(&req)
+	if err != nil {
+		return nil, false, fmt.Errorf("parsing request: %w", err)
+	}
+
+	if req.Web.Port == 0 || req.DNS.Port == 0 {
+		return nil, false, errors.Error("ports cannot be 0")
+	}
+
+	restartHTTP = !config.BindHost.Equal(req.Web.IP) || config.BindPort != req.Web.Port
+	if restartHTTP {
+		err = aghnet.CheckPortAvailable(req.Web.IP, req.Web.Port)
+		if err != nil {
+			return nil, false, fmt.Errorf(
+				"checking address %s:%d: %w",
+				req.Web.IP.String(),
+				req.Web.Port,
+				err,
+			)
+		}
+	}
+
+	return req, restartHTTP, err
+}
+
 func (web *Web) registerInstallHandlers() {
 	Context.mux.HandleFunc("/control/install/get_addresses", preInstall(ensureGET(web.handleInstallGetAddresses)))
 	Context.mux.HandleFunc("/control/install/check_config", preInstall(ensurePOST(web.handleInstallCheckConfig)))

internal/home/home.go

@@ -349,7 +349,7 @@ func run(args options, clientBuildFS fs.FS) {
 
 	setupContext(args)
 
-	err = configureOS(&config)
+	err = configureOS(config)
 	fatalOnError(err)
 
 	// clients package uses filtering package's static data (filtering.BlockedSvcKnown()),

internal/home/mobileconfig.go

@@ -10,35 +10,60 @@ import (
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsforward"
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/stringutil"
 	uuid "github.com/satori/go.uuid"
 	"howett.net/plist"
 )
 
+// dnsSettings is the DNSSetting.DNSSettings mobileconfig profile.
+//
+// See https://developer.apple.com/documentation/devicemanagement/dnssettings/dnssettings.
 type dnsSettings struct {
+	// DNSProtocol is the required protocol to be used.  The valid values
+	// are "HTTPS" and "TLS".
 	DNSProtocol string
-	ServerURL   string `plist:",omitempty"`
+
-	ServerName  string `plist:",omitempty"`
+	// ServerURL is the URI template of the DoH server.  It must be empty if
-	clientID    string
+	// DNSProtocol is not "HTTPS".
+	ServerURL string `plist:",omitempty"`
+
+	// ServerName is the hostname of the DoT server.  It must be empty if
+	// DNSProtocol is not "TLS".
+	ServerName string `plist:",omitempty"`
+
+	// ServerAddresses is a list of plain DNS server IP addresses used to
+	// resolve the hostname in ServerURL or ServerName.
+	ServerAddresses []string `plist:",omitempty"`
 }
 
+// payloadContent is a Device Management Profile payload.
+//
+// See https://developer.apple.com/documentation/devicemanagement/configuring_multiple_devices_using_profiles#3234127.
 type payloadContent struct {
-	Name               string
+	DNSSettings *dnsSettings
-	PayloadDescription string
+
-	PayloadDisplayName string
-	PayloadIdentifier  string
 	PayloadType        string
+	PayloadIdentifier  string
 	PayloadUUID        string
-	DNSSettings        dnsSettings
+	PayloadDisplayName string
+	PayloadDescription string
 	PayloadVersion     int
 }
 
+// dnsSettingsPayloadType is the payload type for a DNSSettings profile.
+const dnsSettingsPayloadType = "com.apple.dnsSettings.managed"
+
+// mobileConfig contains the TopLevel properties for configuring Device
+// Management Profiles.
+//
+// See https://developer.apple.com/documentation/devicemanagement/toplevel.
 type mobileConfig struct {
 	PayloadDescription       string
 	PayloadDisplayName       string
 	PayloadIdentifier        string
 	PayloadType              string
 	PayloadUUID              string
-	PayloadContent           []payloadContent
+	PayloadContent           []*payloadContent
 	PayloadVersion           int
 	PayloadRemovalDisallowed bool
 }
@@ -52,7 +77,7 @@ const (
 	dnsProtoTLS   = "TLS"
 )
 
-func getMobileConfig(d dnsSettings) ([]byte, error) {
+func encodeMobileConfig(d *dnsSettings, clientID string) ([]byte, error) {
 	var dspName string
 	switch proto := d.DNSProtocol; proto {
 	case dnsProtoHTTPS:
@@ -60,41 +85,41 @@ func getMobileConfig(d dnsSettings) ([]byte, error) {
 		u := &url.URL{
 			Scheme: schemeHTTPS,
 			Host:   d.ServerName,
-			Path:   path.Join("/dns-query", d.clientID),
+			Path:   path.Join("/dns-query", clientID),
 		}
 		d.ServerURL = u.String()
+
 		// Empty the ServerName field since it is only must be presented
 		// in DNS-over-TLS configuration.
-		//
-		// See https://developer.apple.com/documentation/devicemanagement/dnssettings/dnssettings.
 		d.ServerName = ""
 	case dnsProtoTLS:
 		dspName = fmt.Sprintf("%s DoT", d.ServerName)
-		if d.clientID != "" {
+		if clientID != "" {
-			d.ServerName = d.clientID + "." + d.ServerName
+			d.ServerName = clientID + "." + d.ServerName
 		}
 	default:
 		return nil, fmt.Errorf("bad dns protocol %q", proto)
 	}
 
-	data := mobileConfig{
+	payloadID := fmt.Sprintf("%s.%s", dnsSettingsPayloadType, genUUIDv4())
-		PayloadContent: []payloadContent{{
+	data := &mobileConfig{
-			Name:               dspName,
+		PayloadDescription: "Adds AdGuard Home to macOS Big Sur " +
-			PayloadDescription: "Configures device to use AdGuard Home",
+			"and iOS 14 or newer systems",
-			PayloadDisplayName: dspName,
+		PayloadDisplayName: dspName,
-			PayloadIdentifier:  fmt.Sprintf("com.apple.dnsSettings.managed.%s", genUUIDv4()),
+		PayloadIdentifier:  genUUIDv4(),
-			PayloadType:        "com.apple.dnsSettings.managed",
+		PayloadType:        "Configuration",
+		PayloadUUID:        genUUIDv4(),
+		PayloadContent: []*payloadContent{{
+			PayloadType:        dnsSettingsPayloadType,
+			PayloadIdentifier:  payloadID,
 			PayloadUUID:        genUUIDv4(),
+			PayloadDisplayName: dspName,
+			PayloadDescription: "Configures device to use AdGuard Home",
 			PayloadVersion:     1,
 			DNSSettings:        d,
 		}},
-		PayloadDescription:       "Adds AdGuard Home to Big Sur and iOS 14 or newer systems",
-		PayloadDisplayName:       dspName,
-		PayloadIdentifier:        genUUIDv4(),
-		PayloadRemovalDisallowed: false,
-		PayloadType:              "Configuration",
-		PayloadUUID:              genUUIDv4(),
 		PayloadVersion:           1,
+		PayloadRemovalDisallowed: false,
 	}
 
 	return plist.MarshalIndent(data, plist.XMLFormat, "\t")
@@ -133,13 +158,13 @@ func handleMobileConfig(w http.ResponseWriter, r *http.Request, dnsp string) {
 		}
 	}
 
-	d := dnsSettings{
+	d := &dnsSettings{
-		DNSProtocol: dnsp,
+		DNSProtocol:     dnsp,
-		ServerName:  host,
+		ServerName:      host,
-		clientID:    clientID,
+		ServerAddresses: cloneBootstrap(),
 	}
 
-	mobileconfig, err := getMobileConfig(d)
+	mobileconfig, err := encodeMobileConfig(d, clientID)
 	if err != nil {
 		respondJSONError(w, http.StatusInternalServerError, err.Error())
 
@@ -163,6 +188,14 @@ func handleMobileConfig(w http.ResponseWriter, r *http.Request, dnsp string) {
 	_, _ = w.Write(mobileconfig)
 }
 
+// cloneBootstrap returns a clone of the current bootstrap DNS servers.
+func cloneBootstrap() (bootstrap []string) {
+	config.RLock()
+	defer config.RUnlock()
+
+	return stringutil.CloneSlice(config.DNS.BootstrapDNS)
+}
+
 func handleMobileConfigDoH(w http.ResponseWriter, r *http.Request) {
 	handleMobileConfig(w, r, dnsProtoHTTPS)
 }

internal/home/mobileconfig_test.go

@@ -7,12 +7,39 @@ import (
 	"net/http/httptest"
 	"testing"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/dnsforward"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"howett.net/plist"
 )
 
+// testBootstrapDNS are the bootstrap plain DNS server addresses for tests.
+var testBootstrapDNS = []string{
+	"94.140.14.14",
+	"94.140.15.15",
+}
+
+// setupBootstraps is a helper that sets up the bootstrap plain DNS server
+// configuration for tests and also tears it down in a cleanup function.
+func setupBootstraps(t testing.TB) {
+	t.Helper()
+
+	prevConfig := config
+	t.Cleanup(func() {
+		config = prevConfig
+	})
+	config = &configuration{
+		DNS: dnsConfig{
+			FilteringConfig: dnsforward.FilteringConfig{
+				BootstrapDNS: testBootstrapDNS,
+			},
+		},
+	}
+}
+
 func TestHandleMobileConfigDoH(t *testing.T) {
+	setupBootstraps(t)
+
 	t.Run("success", func(t *testing.T) {
 		r, err := http.NewRequest(http.MethodGet, "https://example.com:12345/apple/doh.mobileconfig?host=example.org", nil)
 		require.NoError(t, err)
@@ -25,11 +52,16 @@ func TestHandleMobileConfigDoH(t *testing.T) {
 		var mc mobileConfig
 		_, err = plist.Unmarshal(w.Body.Bytes(), &mc)
 		require.NoError(t, err)
-
 		require.Len(t, mc.PayloadContent, 1)
-		assert.Equal(t, "example.org DoH", mc.PayloadContent[0].Name)
+
 		assert.Equal(t, "example.org DoH", mc.PayloadContent[0].PayloadDisplayName)
-		assert.Equal(t, "https://example.org/dns-query", mc.PayloadContent[0].DNSSettings.ServerURL)
+
+		s := mc.PayloadContent[0].DNSSettings
+		require.NotNil(t, s)
+
+		assert.Equal(t, testBootstrapDNS, s.ServerAddresses)
+		assert.Empty(t, s.ServerName)
+		assert.Equal(t, "https://example.org/dns-query", s.ServerURL)
 	})
 
 	t.Run("error_no_host", func(t *testing.T) {
@@ -66,15 +98,22 @@ func TestHandleMobileConfigDoH(t *testing.T) {
 		var mc mobileConfig
 		_, err = plist.Unmarshal(w.Body.Bytes(), &mc)
 		require.NoError(t, err)
-
 		require.Len(t, mc.PayloadContent, 1)
-		assert.Equal(t, "example.org DoH", mc.PayloadContent[0].Name)
+
 		assert.Equal(t, "example.org DoH", mc.PayloadContent[0].PayloadDisplayName)
-		assert.Equal(t, "https://example.org/dns-query/cli42", mc.PayloadContent[0].DNSSettings.ServerURL)
+
+		s := mc.PayloadContent[0].DNSSettings
+		require.NotNil(t, s)
+
+		assert.Equal(t, testBootstrapDNS, s.ServerAddresses)
+		assert.Empty(t, s.ServerName)
+		assert.Equal(t, "https://example.org/dns-query/cli42", s.ServerURL)
 	})
 }
 
 func TestHandleMobileConfigDoT(t *testing.T) {
+	setupBootstraps(t)
+
 	t.Run("success", func(t *testing.T) {
 		r, err := http.NewRequest(http.MethodGet, "https://example.com:12345/apple/dot.mobileconfig?host=example.org", nil)
 		require.NoError(t, err)
@@ -87,11 +126,16 @@ func TestHandleMobileConfigDoT(t *testing.T) {
 		var mc mobileConfig
 		_, err = plist.Unmarshal(w.Body.Bytes(), &mc)
 		require.NoError(t, err)
-
 		require.Len(t, mc.PayloadContent, 1)
-		assert.Equal(t, "example.org DoT", mc.PayloadContent[0].Name)
+
 		assert.Equal(t, "example.org DoT", mc.PayloadContent[0].PayloadDisplayName)
-		assert.Equal(t, "example.org", mc.PayloadContent[0].DNSSettings.ServerName)
+
+		s := mc.PayloadContent[0].DNSSettings
+		require.NotNil(t, s)
+
+		assert.Equal(t, testBootstrapDNS, s.ServerAddresses)
+		assert.Equal(t, "example.org", s.ServerName)
+		assert.Empty(t, s.ServerURL)
 	})
 
 	t.Run("error_no_host", func(t *testing.T) {
@@ -129,10 +173,15 @@ func TestHandleMobileConfigDoT(t *testing.T) {
 		var mc mobileConfig
 		_, err = plist.Unmarshal(w.Body.Bytes(), &mc)
 		require.NoError(t, err)
-
 		require.Len(t, mc.PayloadContent, 1)
-		assert.Equal(t, "example.org DoT", mc.PayloadContent[0].Name)
+
 		assert.Equal(t, "example.org DoT", mc.PayloadContent[0].PayloadDisplayName)
-		assert.Equal(t, "cli42.example.org", mc.PayloadContent[0].DNSSettings.ServerName)
+
+		s := mc.PayloadContent[0].DNSSettings
+		require.NotNil(t, s)
+
+		assert.Equal(t, testBootstrapDNS, s.ServerAddresses)
+		assert.Equal(t, "cli42.example.org", s.ServerName)
+		assert.Empty(t, s.ServerURL)
 	})
 }