cmd/tailscale/cli: return error on duplicate multi-value flags (#15534)

Some CLI flags support multiple values separated by commas. These flags
are intended to be declared only once and will silently ignore subsequent
instances. This will now throw an error if multiple instances of advertise-tags
and advertise-routes are detected.

Fixes #6813

Signed-off-by: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com>

cmd/tailscale/cli/cli_test.go

@@ -657,6 +657,13 @@ func upArgsFromOSArgs(goos string, flagArgs ...string) (args upArgsT) {
 	return
 }
 
+func newSingleUseStringForTest(v string) singleUseStringFlag {
+	return singleUseStringFlag{
+		set:   true,
+		value: v,
+	}
+}
+
 func TestPrefsFromUpArgs(t *testing.T) {
 	tests := []struct {
 		name     string
@@ -721,14 +728,14 @@ func TestPrefsFromUpArgs(t *testing.T) {
 		{
 			name: "error_advertise_route_invalid_ip",
 			args: upArgsT{
-				advertiseRoutes: "foo",
+				advertiseRoutes: newSingleUseStringForTest("foo"),
 			},
 			wantErr: `"foo" is not a valid IP address or CIDR prefix`,
 		},
 		{
 			name: "error_advertise_route_unmasked_bits",
 			args: upArgsT{
-				advertiseRoutes: "1.2.3.4/16",
+				advertiseRoutes: newSingleUseStringForTest("1.2.3.4/16"),
 			},
 			wantErr: `1.2.3.4/16 has non-address bits set; expected 1.2.0.0/16`,
 		},
@@ -749,7 +756,7 @@ func TestPrefsFromUpArgs(t *testing.T) {
 		{
 			name: "error_tag_prefix",
 			args: upArgsT{
-				advertiseTags: "foo",
+				advertiseTags: newSingleUseStringForTest("foo"),
 			},
 			wantErr: `tag: "foo": tags must start with 'tag:'`,
 		},
@@ -829,7 +836,7 @@ func TestPrefsFromUpArgs(t *testing.T) {
 			name: "via_route_good",
 			goos: "linux",
 			args: upArgsT{
-				advertiseRoutes: "fd7a:115c:a1e0:b1a::bb:10.0.0.0/112",
+				advertiseRoutes: newSingleUseStringForTest("fd7a:115c:a1e0:b1a::bb:10.0.0.0/112"),
 				netfilterMode:   "off",
 			},
 			want: &ipn.Prefs{
@@ -848,7 +855,7 @@ func TestPrefsFromUpArgs(t *testing.T) {
 			name: "via_route_good_16_bit",
 			goos: "linux",
 			args: upArgsT{
-				advertiseRoutes: "fd7a:115c:a1e0:b1a::aabb:10.0.0.0/112",
+				advertiseRoutes: newSingleUseStringForTest("fd7a:115c:a1e0:b1a::aabb:10.0.0.0/112"),
 				netfilterMode:   "off",
 			},
 			want: &ipn.Prefs{
@@ -867,7 +874,7 @@ func TestPrefsFromUpArgs(t *testing.T) {
 			name: "via_route_short_prefix",
 			goos: "linux",
 			args: upArgsT{
-				advertiseRoutes: "fd7a:115c:a1e0:b1a::/64",
+				advertiseRoutes: newSingleUseStringForTest("fd7a:115c:a1e0:b1a::/64"),
 				netfilterMode:   "off",
 			},
 			wantErr: "fd7a:115c:a1e0:b1a::/64 4-in-6 prefix must be at least a /96",
@@ -876,7 +883,7 @@ func TestPrefsFromUpArgs(t *testing.T) {
 			name: "via_route_short_reserved_siteid",
 			goos: "linux",
 			args: upArgsT{
-				advertiseRoutes: "fd7a:115c:a1e0:b1a:1234:5678::/112",
+				advertiseRoutes: newSingleUseStringForTest("fd7a:115c:a1e0:b1a:1234:5678::/112"),
 				netfilterMode:   "off",
 			},
 			wantErr: "route fd7a:115c:a1e0:b1a:1234:5678::/112 contains invalid site ID 12345678; must be 0xffff or less",
@@ -1106,6 +1113,7 @@ func TestUpdatePrefs(t *testing.T) {
 			},
 			env: upCheckEnv{backendState: "Running"},
 		},
+
 		{
 			// Issue 3808: explicitly empty --operator= should clear value.
 			name:  "explicit_empty_operator",
@@ -1598,3 +1606,56 @@ func TestDepsNoCapture(t *testing.T) {
 	}.Check(t)
 
 }
+
+func TestSingleUseStringFlag(t *testing.T) {
+	tests := []struct {
+		name      string
+		setValues []string
+		wantValue string
+		wantErr   bool
+	}{
+		{
+			name:      "set once",
+			setValues: []string{"foo"},
+			wantValue: "foo",
+			wantErr:   false,
+		},
+		{
+			name:      "set twice",
+			setValues: []string{"foo", "bar"},
+			wantValue: "foo",
+			wantErr:   true,
+		},
+		{
+			name:      "set nothing",
+			setValues: []string{},
+			wantValue: "",
+			wantErr:   false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var flag singleUseStringFlag
+			var lastErr error
+
+			for _, val := range tt.setValues {
+				lastErr = flag.Set(val)
+			}
+
+			if tt.wantErr {
+				if lastErr == nil {
+					t.Errorf("expected error on final Set, got nil")
+				}
+			} else {
+				if lastErr != nil {
+					t.Errorf("unexpected error on final Set: %v", lastErr)
+				}
+			}
+
+			if got := flag.String(); got != tt.wantValue {
+				t.Errorf("String() = %q, want %q", got, tt.wantValue)
+			}
+		})
+	}
+}

cmd/tailscale/cli/set.go

@@ -49,7 +49,7 @@ type setArgsT struct {
 	runSSH                 bool
 	runWebClient           bool
 	hostname               string
-	advertiseRoutes        string
+	advertiseRoutes        singleUseStringFlag
 	advertiseDefaultRoute  bool
 	advertiseConnector     bool
 	opUser                 string
@@ -75,7 +75,7 @@ func newSetFlagSet(goos string, setArgs *setArgsT) *flag.FlagSet {
 	setf.BoolVar(&setArgs.shieldsUp, "shields-up", false, "don't allow incoming connections")
 	setf.BoolVar(&setArgs.runSSH, "ssh", false, "run an SSH server, permitting access per tailnet admin's declared policy")
 	setf.StringVar(&setArgs.hostname, "hostname", "", "hostname to use instead of the one provided by the OS")
-	setf.StringVar(&setArgs.advertiseRoutes, "advertise-routes", "", "routes to advertise to other nodes (comma-separated, e.g. \"10.0.0.0/8,192.168.0.0/24\") or empty string to not advertise routes")
+	setf.Var(&setArgs.advertiseRoutes, "advertise-routes", "routes to advertise to other nodes (comma-separated, e.g. \"10.0.0.0/8,192.168.0.0/24\") or empty string to not advertise routes")
 	setf.BoolVar(&setArgs.advertiseDefaultRoute, "advertise-exit-node", false, "offer to be an exit node for internet traffic for the tailnet")
 	setf.BoolVar(&setArgs.advertiseConnector, "advertise-connector", false, "offer to be an app connector for domain specific internet traffic for the tailnet")
 	setf.BoolVar(&setArgs.updateCheck, "update-check", true, "notify about available Tailscale updates")
@@ -259,11 +259,11 @@ func runSet(ctx context.Context, args []string) (retErr error) {
 // setArgs is the parsed command-line arguments.
 func calcAdvertiseRoutesForSet(advertiseExitNodeSet, advertiseRoutesSet bool, curPrefs *ipn.Prefs, setArgs setArgsT) (routes []netip.Prefix, err error) {
 	if advertiseExitNodeSet && advertiseRoutesSet {
-		return netutil.CalcAdvertiseRoutes(setArgs.advertiseRoutes, setArgs.advertiseDefaultRoute)
+		return netutil.CalcAdvertiseRoutes(setArgs.advertiseRoutes.String(), setArgs.advertiseDefaultRoute)
 
 	}
 	if advertiseRoutesSet {
-		return netutil.CalcAdvertiseRoutes(setArgs.advertiseRoutes, curPrefs.AdvertisesExitNode())
+		return netutil.CalcAdvertiseRoutes(setArgs.advertiseRoutes.String(), curPrefs.AdvertisesExitNode())
 	}
 	if advertiseExitNodeSet {
 		alreadyAdvertisesExitNode := curPrefs.AdvertisesExitNode()

cmd/tailscale/cli/set_test.go

@@ -116,7 +116,7 @@ func TestCalcAdvertiseRoutesForSet(t *testing.T) {
 				sa.advertiseDefaultRoute = *tc.setExit
 			}
 			if tc.setRoutes != nil {
-				sa.advertiseRoutes = *tc.setRoutes
+				sa.advertiseRoutes = newSingleUseStringForTest(*tc.setRoutes)
 			}
 			got, err := calcAdvertiseRoutesForSet(tc.setExit != nil, tc.setRoutes != nil, curPrefs, sa)
 			if err != nil {

cmd/tailscale/cli/up.go

@@ -82,6 +82,25 @@ func acceptRouteDefault(goos string) bool {
 	return p.DefaultRouteAll(goos)
 }
 
+// singleUseStringFlag will throw an error if the flag is specified more than once.
+type singleUseStringFlag struct {
+	set   bool
+	value string
+}
+
+func (s singleUseStringFlag) String() string {
+	return s.value
+}
+
+func (s *singleUseStringFlag) Set(v string) error {
+	if s.set {
+		return errors.New("flag can only be specified once")
+	}
+	s.set = true
+	s.value = v
+	return nil
+}
+
 var upFlagSet = newUpFlagSet(effectiveGOOS(), &upArgsGlobal, "up")
 
 // newUpFlagSet returns a new flag set for the "up" and "login" commands.
@@ -104,9 +123,9 @@ func newUpFlagSet(goos string, upArgs *upArgsT, cmd string) *flag.FlagSet {
 	upf.BoolVar(&upArgs.exitNodeAllowLANAccess, "exit-node-allow-lan-access", false, "Allow direct access to the local network when routing traffic via an exit node")
 	upf.BoolVar(&upArgs.shieldsUp, "shields-up", false, "don't allow incoming connections")
 	upf.BoolVar(&upArgs.runSSH, "ssh", false, "run an SSH server, permitting access per tailnet admin's declared policy")
-	upf.StringVar(&upArgs.advertiseTags, "advertise-tags", "", "comma-separated ACL tags to request; each must start with \"tag:\" (e.g. \"tag:eng,tag:montreal,tag:ssh\")")
+	upf.Var(&upArgs.advertiseTags, "advertise-tags", "comma-separated ACL tags to request; each must start with \"tag:\" (e.g. \"tag:eng,tag:montreal,tag:ssh\")")
 	upf.StringVar(&upArgs.hostname, "hostname", "", "hostname to use instead of the one provided by the OS")
-	upf.StringVar(&upArgs.advertiseRoutes, "advertise-routes", "", "routes to advertise to other nodes (comma-separated, e.g. \"10.0.0.0/8,192.168.0.0/24\") or empty string to not advertise routes")
+	upf.Var(&upArgs.advertiseRoutes, "advertise-routes", "routes to advertise to other nodes (comma-separated, e.g. \"10.0.0.0/8,192.168.0.0/24\") or empty string to not advertise routes")
 	upf.BoolVar(&upArgs.advertiseConnector, "advertise-connector", false, "advertise this node as an app connector")
 	upf.BoolVar(&upArgs.advertiseDefaultRoute, "advertise-exit-node", false, "offer to be an exit node for internet traffic for the tailnet")
 	upf.BoolVar(&upArgs.postureChecking, "posture-checking", false, hidden+"allow management plane to gather device posture information")
@@ -174,9 +193,9 @@ type upArgsT struct {
 	runWebClient           bool
 	forceReauth            bool
 	forceDaemon            bool
-	advertiseRoutes        string
+	advertiseRoutes        singleUseStringFlag
 	advertiseDefaultRoute  bool
-	advertiseTags          string
+	advertiseTags          singleUseStringFlag
 	advertiseConnector     bool
 	snat                   bool
 	statefulFiltering      bool
@@ -244,7 +263,7 @@ func warnf(format string, args ...any) {
 // function exists for testing and should have no side effects or
 // outside interactions (e.g. no making Tailscale LocalAPI calls).
 func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goos string) (*ipn.Prefs, error) {
-	routes, err := netutil.CalcAdvertiseRoutes(upArgs.advertiseRoutes, upArgs.advertiseDefaultRoute)
+	routes, err := netutil.CalcAdvertiseRoutes(upArgs.advertiseRoutes.String(), upArgs.advertiseDefaultRoute)
 	if err != nil {
 		return nil, err
 	}
@@ -254,8 +273,8 @@ func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goo
 	}
 
 	var tags []string
-	if upArgs.advertiseTags != "" {
+	if upArgs.advertiseTags.String() != "" {
-		tags = strings.Split(upArgs.advertiseTags, ",")
+		tags = strings.Split(upArgs.advertiseTags.String(), ",")
 		for _, tag := range tags {
 			err := tailcfg.CheckTag(tag)
 			if err != nil {
@@ -555,7 +574,7 @@ func runUp(ctx context.Context, cmd string, args []string, upArgs upArgsT) (retE
 		if err != nil {
 			return err
 		}
-		authKey, err = resolveAuthKey(ctx, authKey, upArgs.advertiseTags)
+		authKey, err = resolveAuthKey(ctx, authKey, upArgs.advertiseTags.String())
 		if err != nil {
 			return err
 		}