cmd/tailscale: improve `tailscale lock` error message if no keys

Previously, running `add/remove/revoke-keys` without passing any keys
would fail with an unhelpful error:

```console
$ tailscale lock revoke-keys
generation of recovery AUM failed: sending generate-recovery-aum: 500 Internal Server Error: no provided key is currently trusted
```

or

```console
$ tailscale lock revoke-keys
generation of recovery AUM failed: sending generate-recovery-aum: 500 Internal Server Error: network-lock is not active
```

Now they fail with a more useful error:

```console
$ tailscale lock revoke-keys
missing argument, expected one or more tailnet lock keys
```

Fixes #19130

Change-Id: I9d81fe2f5b92a335854e71cbc6928e7e77e537e3
Signed-off-by: Alex Chan <alexc@tailscale.com>

cmd/tailscale/cli/network-lock.go

@@ -305,9 +305,7 @@ var nlAddCmd = &ffcli.Command{
 	Name:       "add",
 	ShortUsage: "tailscale lock add <public-key>...",
 	ShortHelp:  "Add one or more trusted signing keys to tailnet lock",
-	Exec: func(ctx context.Context, args []string) error {
+	Exec:       runNetworkLockAdd,
-		return runNetworkLockModify(ctx, args, nil)
-	},
 }
 
 var nlRemoveArgs struct {
@@ -331,6 +329,9 @@ func runNetworkLockRemove(ctx context.Context, args []string) error {
 	if err != nil {
 		return err
 	}
+	if len(removeKeys) == 0 {
+		return fmt.Errorf("missing argument, expected one or more tailnet lock keys")
+	}
 	st, err := localClient.NetworkLockStatus(ctx)
 	if err != nil {
 		return fixTailscaledConnectError(err)
@@ -445,25 +446,24 @@ func parseNLArgs(args []string, parseKeys, parseDisablements bool) (keys []tka.K
 	return keys, disablements, nil
 }
 
-func runNetworkLockModify(ctx context.Context, addArgs, removeArgs []string) error {
+func runNetworkLockAdd(ctx context.Context, addArgs []string) error {
-	st, err := localClient.NetworkLockStatus(ctx)
+	addKeys, _, err := parseNLArgs(addArgs, true, false)
 	if err != nil {
-		return fixTailscaledConnectError(err)
+		return err
 	}
-	if !st.Enabled {
+	if len(addKeys) == 0 {
-		return errors.New("tailnet lock is not enabled")
+		return fmt.Errorf("missing argument, expected one or more tailnet lock keys")
 	}
 
-	addKeys, _, err := parseNLArgs(addArgs, true, false)
+	st, err := localClient.NetworkLockStatus(ctx)
 	if err != nil {
-		return err
+		return fixTailscaledConnectError(err)
 	}
-	removeKeys, _, err := parseNLArgs(removeArgs, true, false)
+	if !st.Enabled {
-	if err != nil {
+		return errors.New("tailnet lock is not enabled")
-		return err
 	}
 
-	if err := localClient.NetworkLockModify(ctx, addKeys, removeKeys); err != nil {
+	if err := localClient.NetworkLockModify(ctx, addKeys, nil); err != nil {
 		return err
 	}
 	return nil
@@ -819,13 +819,17 @@ Revocation is a multi-step process that requires several signing nodes to ` + "`
 func runNetworkLockRevokeKeys(ctx context.Context, args []string) error {
 	// First step in the process
 	if !nlRevokeKeysArgs.cosign && !nlRevokeKeysArgs.finish {
-		removeKeys, _, err := parseNLArgs(args, true, false)
+		revokeKeys, _, err := parseNLArgs(args, true, false)
 		if err != nil {
 			return err
 		}
 
-		keyIDs := make([]tkatype.KeyID, len(removeKeys))
+		if len(revokeKeys) == 0 {
-		for i, k := range removeKeys {
+			return fmt.Errorf("missing argument, expected one or more tailnet lock keys")
+		}
+
+		keyIDs := make([]tkatype.KeyID, len(revokeKeys))
+		for i, k := range revokeKeys {
 			keyIDs[i], err = k.ID()
 			if err != nil {
 				return fmt.Errorf("generating keyID: %v", err)

tstest/integration/integration_test.go

@@ -2374,6 +2374,38 @@ func TestTailnetLock(t *testing.T) {
 			t.Fatalf("ping node3 -> signing1: expected success, got err: %v", err)
 		}
 	})
+
+	// If you run `tailscale lock (add|remove|revoke-keys)` but don't pass any keys,
+	// we print a helpful error message.
+	//
+	// Regression test for tailscale/tailscale#19130
+	t.Run("no-keys-is-error", func(t *testing.T) {
+		for _, verb := range []string{"add", "remove", "revoke-keys"} {
+			t.Run(verb, func(t *testing.T) {
+				tstest.Shard(t)
+				t.Parallel()
+
+				env := NewTestEnv(t)
+				n1 := NewTestNode(t, env)
+				d1 := n1.StartDaemon()
+				defer d1.MustCleanShutdown(t)
+
+				n1.MustUp()
+				n1.AwaitRunning()
+
+				revokeCmd := n1.Tailscale("lock", verb)
+				out, err := revokeCmd.CombinedOutput()
+				if err == nil {
+					t.Fatal("expected command to fail, but succeeded")
+				}
+				want := "missing argument"
+				got := string(out)
+				if !strings.Contains(string(out), want) {
+					t.Fatalf("expected output to contain %q, got %q", want, got)
+				}
+			})
+		}
+	})
 }
 
 func TestNodeWithBadStateFile(t *testing.T) {