Release branch v1.5.5

cmd/cli/cli.go

@@ -451,18 +451,7 @@ func run(appCallback *AppCallback, stopCh chan struct{}) {
 	p.onStopped = append(p.onStopped, func() {
 		// restore static DNS settings or DHCP
 		p.resetDNS(false, true)
-		// Iterate over all physical interfaces and restore static DNS if a saved static config exists.
-		withEachPhysicalInterfaces("", "restore static DNS", func(i *net.Interface) error {
-			file := savedStaticDnsSettingsFilePath(i)
-			if _, err := os.Stat(file); err == nil {
-				if err := restoreDNS(i); err != nil {
-					mainLog.Load().Error().Err(err).Msgf("Could not restore static DNS on interface %s", i.Name)
-				} else {
-					mainLog.Load().Debug().Msgf("Restored static DNS on interface %s successfully", i.Name)
-				}
-			}
-			return nil
-		})
+		restoreSavedStaticDNS("", false)
 	})
 
 	close(waitCh)
@@ -826,7 +815,7 @@ func processLogAndCacheFlags(v *viper.Viper, cfg *ctrld.Config) {
 }
 
 func netInterface(ifaceName string) (*net.Interface, error) {
-	if ifaceName == "auto" {
+	if ifaceName == autoIface {
 		ifaceName = defaultIfaceName()
 	}
 	var iface *net.Interface
@@ -1112,23 +1101,7 @@ func uninstall(p *prog, s service.Service) {
 		}
 		// restore static DNS settings or DHCP
 		p.resetDNS(false, true)
-
-		// Iterate over all physical interfaces and restore DNS if a saved static config exists.
-		withEachPhysicalInterfaces(p.runningIface, "restore static DNS", func(i *net.Interface) error {
-			file := savedStaticDnsSettingsFilePath(i)
-			if _, err := os.Stat(file); err == nil {
-				if err := restoreDNS(i); err != nil {
-					mainLog.Load().Error().Err(err).Msgf("Could not restore static DNS on interface %s", i.Name)
-				} else {
-					mainLog.Load().Debug().Msgf("Restored static DNS on interface %s successfully", i.Name)
-					err = os.Remove(file)
-					if err != nil {
-						mainLog.Load().Debug().Err(err).Msgf("Could not remove saved static DNS file for interface %s", i.Name)
-					}
-				}
-			}
-			return nil
-		})
+		restoreSavedStaticDNS(p.runningIface, true)
 
 		if router.Name() != "" {
 			mainLog.Load().Debug().Msg("Router cleanup")
@@ -1141,6 +1114,26 @@ func uninstall(p *prog, s service.Service) {
 	}
 }
 
+// restoreSavedStaticDNS restores DNS from saved static config files on physical interfaces.
+func restoreSavedStaticDNS(excludeIfaceName string, removeSaved bool) {
+	withEachPhysicalInterfaces(excludeIfaceName, "restore static DNS", func(i *net.Interface) error {
+		file := savedStaticDnsSettingsFilePath(i)
+		if _, err := os.Stat(file); err == nil {
+			if err := restoreDNS(i); err != nil {
+				mainLog.Load().Error().Err(err).Msgf("Could not restore static DNS on interface %s", i.Name)
+			} else {
+				mainLog.Load().Debug().Msgf("Restored static DNS on interface %s successfully", i.Name)
+				if removeSaved {
+					if err := os.Remove(file); err != nil {
+						mainLog.Load().Debug().Err(err).Msgf("Could not remove saved static DNS file for interface %s", i.Name)
+					}
+				}
+			}
+		}
+		return nil
+	})
+}
+
 func validateConfig(cfg *ctrld.Config) error {
 	if err := ctrld.ValidateConfig(validator.New(), cfg); err != nil {
 		var ve validator.ValidationErrors
@@ -2069,15 +2062,37 @@ func doValidateCdRemoteConfig(cdUID string, fatal bool) error {
 	return nil
 }
 
+// ensureRunningIfaceForInvalidUninstall populates p.runningIface before the
+// invalid-device self-uninstall resets DNS. This path can run early during
+// service startup (e.g. right after a reboot) before the running interface is
+// otherwise known. resetDNS, via resetDNSForRunningIface, silently skips DNS
+// restoration when p.runningIface is empty, which would leave the OS pointed at
+// ctrld's local listener after the service is removed. See issue-556.
+func ensureRunningIfaceForInvalidUninstall(p *prog, s service.Service) {
+	if iface == "" {
+		iface = autoIface
+	}
+	p.preRun()
+	if ir := runningIface(s); ir != nil {
+		p.runningIface = ir.Name
+		p.requiredMultiNICsConfig = ir.All
+	}
+}
+
 // uninstallInvalidCdUID performs self-uninstallation because the ControlD device does not exist.
 func uninstallInvalidCdUID(p *prog, logger zerolog.Logger, doStop bool) bool {
 	s, err := newService(p, svcConfig)
 	if err != nil {
 		logger.Warn().Err(err).Msg("failed to create new service")
 		return false
 	}
+	ensureRunningIfaceForInvalidUninstall(p, s)
 	// restore static DNS settings or DHCP
 	p.resetDNS(false, true)
+	// The invalid-device path may run early during service startup before runningIface
+	// is known. Restore every saved static DNS file so uninstalling does not leave the
+	// OS pointed at ctrld's local listener after the service is removed.
+	restoreSavedStaticDNS("", true)
 
 	tasks := []task{{s.Uninstall, true, "Uninstall"}}
 	if doTasks(tasks) {

cmd/cli/cli_intercept_listener_test.go

@@ -1,6 +1,10 @@
 package cli
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/Control-D-Inc/ctrld"
+)
 
 func TestIsExplicitInterceptListener(t *testing.T) {
 	tests := []struct {
@@ -26,3 +30,87 @@ func TestIsExplicitInterceptListener(t *testing.T) {
 		})
 	}
 }
+
+// TestPreserveBoundListeners is a regression test for #551: on reload, the on-disk
+// generated config still declares 127.0.0.1:53, but the running listener has fallen back
+// to 127.0.0.1:5354. preserveBoundListeners must keep the in-memory config on the actual
+// bound port so pf rdr rules and probes do not target the dead default port.
+func TestPreserveBoundListeners(t *testing.T) {
+	// cur = actual running listener (fell back to 5354); newCfg = freshly read from disk (53).
+	cur := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}}
+	newListeners := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 53}}
+
+	preserveBoundListeners(newListeners, cur)
+
+	if got := newListeners["0"].Port; got != 5354 {
+		t.Errorf("listener port after reload = %d, want 5354 (actual bound port)", got)
+	}
+	if got := newListeners["0"].IP; got != "127.0.0.1" {
+		t.Errorf("listener IP after reload = %q, want 127.0.0.1", got)
+	}
+}
+
+// TestPreserveBoundListeners_NoChange verifies that when the on-disk config matches the
+// running listener, the config is left untouched (a legitimate reload with the same port).
+func TestPreserveBoundListeners_NoChange(t *testing.T) {
+	cur := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}}
+	newListeners := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}}
+
+	preserveBoundListeners(newListeners, cur)
+
+	if got := newListeners["0"].Port; got != 5354 {
+		t.Errorf("listener port = %d, want 5354", got)
+	}
+}
+
+// TestPreserveBoundListeners_MissingCurrent verifies that a listener present on disk but not
+// in the current running set (e.g. newly added) is left as configured.
+func TestPreserveBoundListeners_MissingCurrent(t *testing.T) {
+	cur := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}}
+	newListeners := map[string]*ctrld.ListenerConfig{
+		"0": {IP: "127.0.0.1", Port: 53},
+		"1": {IP: "127.0.0.1", Port: 5355},
+	}
+
+	preserveBoundListeners(newListeners, cur)
+
+	if got := newListeners["0"].Port; got != 5354 {
+		t.Errorf("listener 0 port = %d, want 5354 (preserved)", got)
+	}
+	if got := newListeners["1"].Port; got != 5355 {
+		t.Errorf("listener 1 port = %d, want 5355 (unchanged, no current binding)", got)
+	}
+}
+
+// TestPreserveBoundListeners_ExplicitChangeNotMasked verifies that an explicit, non-default
+// listener in the reloaded config is applied rather than reverted to the old bound listener.
+// Reverting an explicit change would make the control-server reload comparison return 200
+// instead of 201, silently dropping the new listener. Regression guard for #551 review.
+func TestPreserveBoundListeners_ExplicitChangeNotMasked(t *testing.T) {
+	// Running listener fell back to 5354; user reloads with an explicit new listener.
+	cur := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}}
+	newListeners := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.2", Port: 5399}}
+
+	preserveBoundListeners(newListeners, cur)
+
+	if got := newListeners["0"].IP; got != "127.0.0.2" {
+		t.Errorf("explicit listener IP = %q, want 127.0.0.2 (not reverted)", got)
+	}
+	if got := newListeners["0"].Port; got != 5399 {
+		t.Errorf("explicit listener port = %d, want 5399 (not reverted)", got)
+	}
+}
+
+// TestPreserveBoundListeners_ExplicitDefaultPreserved verifies that the default
+// 127.0.0.1:53 listener remains fallback-eligible: when it diverges from the running
+// fallback port it is still preserved (isExplicitInterceptListener treats :53 as non-explicit).
+func TestPreserveBoundListeners_ExplicitDefaultPreserved(t *testing.T) {
+	cur := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}}
+	newListeners := map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 53}}
+
+	preserveBoundListeners(newListeners, cur)
+
+	if got := newListeners["0"].Port; got != 5354 {
+		t.Errorf("default listener port = %d, want 5354 (preserved fallback)", got)
+	}
+}

cmd/cli/commands.go

@@ -399,7 +399,7 @@ NOTE: running "ctrld start" without any arguments will start already installed c
 			reportSetDnsOk := func(sockDir string) {
 				if cc := newSocketControlClient(ctx, s, sockDir); cc != nil {
 					if resp, _ := cc.post(ifacePath, nil); resp != nil && resp.StatusCode == http.StatusOK {
-						if iface == "auto" {
+						if iface == autoIface {
 							iface = defaultIfaceName()
 						}
 						res := &ifaceResponse{}
@@ -748,7 +748,7 @@ NOTE: running "ctrld start" without any arguments will start already installed c
 			startCmd.Run(cmd, args)
 		},
 	}
-	startCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", "auto", `Update DNS setting for iface, "auto" means the default interface gateway`)
+	startCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", autoIface, `Update DNS setting for iface, "auto" means the default interface gateway`)
 	startCmdAlias.Flags().AddFlagSet(startCmd.Flags())
 	rootCmd.AddCommand(startCmdAlias)
 
@@ -833,7 +833,7 @@ func initStopCmd() *cobra.Command {
 			stopCmd.Run(cmd, args)
 		},
 	}
-	stopCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", "auto", `Reset DNS setting for iface, "auto" means the default interface gateway`)
+	stopCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", autoIface, `Reset DNS setting for iface, "auto" means the default interface gateway`)
 	stopCmdAlias.Flags().AddFlagSet(stopCmd.Flags())
 	rootCmd.AddCommand(stopCmdAlias)
 
@@ -865,7 +865,7 @@ func initRestartCmd() *cobra.Command {
 				return
 			}
 			if iface == "" {
-				iface = "auto"
+				iface = autoIface
 			}
 			p.preRun()
 			if ir := runningIface(s); ir != nil {
@@ -1111,7 +1111,7 @@ NOTE: Uninstalling will set DNS to values provided by DHCP.`,
 				return
 			}
 			if iface == "" {
-				iface = "auto"
+				iface = autoIface
 			}
 			p.preRun()
 			if ir := runningIface(s); ir != nil {
@@ -1207,7 +1207,7 @@ NOTE: Uninstalling will set DNS to values provided by DHCP.`,
 			uninstallCmd.Run(cmd, args)
 		},
 	}
-	uninstallCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", "auto", `Reset DNS setting for iface, "auto" means the default interface gateway`)
+	uninstallCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", autoIface, `Reset DNS setting for iface, "auto" means the default interface gateway`)
 	uninstallCmdAlias.Flags().AddFlagSet(uninstallCmd.Flags())
 	rootCmd.AddCommand(uninstallCmdAlias)
 
@@ -1400,7 +1400,7 @@ func initUpgradeCmd() *cobra.Command {
 				return
 			}
 			if iface == "" {
-				iface = "auto"
+				iface = autoIface
 			}
 			p.preRun()
 			if ir := runningIface(s); ir != nil {
@@ -1595,7 +1595,7 @@ func onlyInterceptFlags(args []string) bool {
 			} else {
 				return false
 			}
-		case arg == "--iface=auto" || arg == "--iface" || arg == "auto":
+		case arg == "--iface="+autoIface || arg == "--iface" || arg == autoIface:
 			// Auto-added by startCmdAlias or its value; safe to ignore.
 			continue
 		default:

cmd/cli/dns_intercept_darwin_test.go

@@ -123,6 +123,35 @@ func TestPFBuildAnchorRules_Ordering(t *testing.T) {
 	}
 }
 
+// TestPFBuildAnchorRules_FallbackPort verifies that when the listener falls back
+// to an alternate local port (e.g. 5354 because mDNSResponder owns *:53), the pf
+// rdr rules redirect DNS to the ACTUAL bound port, not the configured default 53.
+// Regression test for #551: pf redirected to a dead port after listener fallback.
+func TestPFBuildAnchorRules_FallbackPort(t *testing.T) {
+	// Configured/generated listener is 127.0.0.1:53, but the runtime bound port is 5354.
+	p := &prog{cfg: &ctrld.Config{Listener: map[string]*ctrld.ListenerConfig{"0": {IP: "127.0.0.1", Port: 5354}}}}
+	rules := p.buildPFAnchorRules(nil)
+
+	// rdr must redirect to the actual bound port 5354.
+	if !strings.Contains(rules, "rdr on lo0 inet proto udp from any to ! 127.0.0.1 port 53 -> 127.0.0.1 port 5354") {
+		t.Errorf("UDP rdr must redirect to bound port 5354, got:\n%s", rules)
+	}
+	if !strings.Contains(rules, "rdr on lo0 inet proto tcp from any to ! 127.0.0.1 port 53 -> 127.0.0.1 port 5354") {
+		t.Errorf("TCP rdr must redirect to bound port 5354, got:\n%s", rules)
+	}
+
+	// The rdr redirect target must NOT point at the dead default port 53.
+	// Match the exact port at line end so "port 5354" is not a false positive.
+	if strings.Contains(rules, "-> 127.0.0.1 port 53\n") {
+		t.Errorf("rdr must not redirect to dead port 53 after fallback, got:\n%s", rules)
+	}
+
+	// The inbound accept rule must also target the actual bound port.
+	if !strings.Contains(rules, "127.0.0.1 port 5354") {
+		t.Errorf("pass in rule must reference bound port 5354, got:\n%s", rules)
+	}
+}
+
 // TestPFAddressFamily tests the pfAddressFamily helper.
 func TestPFAddressFamily(t *testing.T) {
 	tests := []struct {