policy/v2: validate sshTests at parse

Adds SSHPolicyTest with typed Alias src, SSHTestDestinations dst, and []SSHUser accept/deny/check. Parse rejects empty assertions, port/CIDR/autogroup-internet destinations, and empty user.
2026-07-07 16:40:21 +09:00 · 2026-05-13 12:41:42 +00:00
parent e4a1171ba4
commit 295716bcca
3 changed files with 371 additions and 2 deletions
@@ -29,9 +29,15 @@ import (
 // errors. The Error() prefix is "test(s) failed", the same string Tailscale
 // SaaS returns in the api_response_body.message — see
 // hscontrol/policy/v2/testdata/policytest_results/.
+//
+// errSSHPolicyTestsFailed wraps sshTests failures. Tailscale SaaS returns the
+// same literal "test(s) failed" body for both ACL tests and SSH tests, but
+// the two sentinels are kept as distinct values so callers can use errors.Is
+// to tell them apart while still matching the SaaS body byte-for-byte.
 var (
-	errPolicyTestsFailed   = errors.New("test(s) failed")
-	errTestDestinationNoIP = errors.New("destination resolved to no IP addresses")
+	errPolicyTestsFailed    = errors.New("test(s) failed")
+	errSSHPolicyTestsFailed = errors.New("test(s) failed")
+	errTestDestinationNoIP  = errors.New("destination resolved to no IP addresses")
 )

 // PolicyTest is one entry in the policy's `tests` block.
@@ -53,6 +59,36 @@ type PolicyTest struct {
 	Deny []string `json:"deny,omitempty"`
 }

+// SSHPolicyTest is one entry in the policy's `sshTests` block. Unlike the
+// ACL `tests` block, sshTests describe SSH login attempts: a source alias
+// connects to each destination host and tries each named login user. The
+// accept / deny / check arrays carry usernames, not destinations — every
+// listed user is asserted against every entry in dst.
+type SSHPolicyTest struct {
+	// Src is a single source alias (user, group, tag, host, or IP). Same
+	// shape as PolicyTest.Src — Tailscale only supports one src per entry.
+	Src string `json:"src"`
+
+	// Dst lists destination host aliases the test exercises. Tags, hosts,
+	// and the SSH-compatible autogroups are valid; ports, CIDR ranges, and
+	// autogroup:internet are rejected at parse time.
+	Dst []string `json:"dst"`
+
+	// Accept lists SSH login users that must be allowed by an action:accept
+	// or action:check rule when Src connects to each entry in Dst.
+	Accept []string `json:"accept,omitempty"`
+
+	// Deny lists SSH login users that must NOT be allowed by any rule when
+	// Src connects to each entry in Dst.
+	Deny []string `json:"deny,omitempty"`
+
+	// Check lists SSH login users that must be allowed by an action:check
+	// rule specifically. action:accept matches do not satisfy a check
+	// assertion. Engine evaluation is not implemented yet; parse-time
+	// validation accepts the field so policies can be authored ahead of it.
+	Check []string `json:"check,omitempty"`
+}
+
 // PolicyTestResult is the outcome of a single PolicyTest.
 type PolicyTestResult struct {
 	Src    string   `json:"src"`
@@ -153,6 +153,10 @@ var (
 	ErrTestDestinationMultiPort    = errors.New("test destination port must be a single port")
 	ErrTestDestinationCIDR         = errors.New("test destination must be a single host, not a CIDR range")
 	ErrAutogroupInternetTestDst    = errors.New("autogroup:internet not valid as a test destination")
+	ErrSSHTestEmptySrc             = errors.New("SSH tests entry must have a non-empty src")
+	ErrSSHTestEmptyDst             = errors.New("SSH tests entry must have at least one dst")
+	ErrSSHTestDstUnknownTag        = errors.New("SSH tests dst contains unknown tag")
+	ErrSSHTestDstDisallowedElement = errors.New("SSH tests dst contains disallowed element")
 )

 type resolved struct {
@@ -2092,6 +2096,7 @@ type Policy struct {
 	AutoApprovers       AutoApproverPolicy `json:"autoApprovers"`
 	SSHs                []SSH              `json:"ssh,omitempty"`
 	Tests               []PolicyTest       `json:"tests,omitempty"`
+	SSHTests            []SSHPolicyTest    `json:"sshTests,omitempty"`
 	RandomizeClientPort bool               `json:"randomizeClientPort,omitempty"`
 }

@@ -2910,6 +2915,10 @@ func (p *Policy) validate() error {
 		errs = append(errs, err)
 	}

+	if err := validateSSHTests(p, p.SSHTests); err != nil { //nolint:noinlineerr
+		errs = append(errs, err)
+	}
+
 	if len(errs) > 0 {
 		return multierr.New(errs...)
 	}
@@ -3418,3 +3427,105 @@ func validateTestDestination(pol *Policy, dst string) error {

 	return nil
 }
+
+// validateSSHTests enforces the parse-time shape rules an sshTests entry
+// must satisfy: a non-empty src alias, at least one dst, and a dst list
+// whose entries each describe a single SSH-reachable host. Login-user
+// assertions (accept/deny/check) are not validated here — SaaS reports
+// empty assertion arrays and empty user strings through the same
+// "test(s) failed" body it uses for true evaluation failures, so those
+// cases stay with the engine. Both the parse-time errors and the
+// engine-time failures share the errSSHPolicyTestsFailed wrapper so
+// callers see one consistent body.
+func validateSSHTests(pol *Policy, tests []SSHPolicyTest) error {
+	var errs []error
+
+	for i, t := range tests {
+		if t.Src == "" {
+			errs = append(errs, fmt.Errorf("sshTest %d: %w", i, ErrSSHTestEmptySrc))
+		}
+
+		if len(t.Dst) == 0 {
+			errs = append(errs, fmt.Errorf("sshTest %d: %w", i, ErrSSHTestEmptyDst))
+		}
+
+		for _, dst := range t.Dst {
+			err := validateSSHTestDestination(pol, dst)
+			if err != nil {
+				errs = append(errs, fmt.Errorf("sshTest %d: %w", i, err))
+			}
+		}
+	}
+
+	if len(errs) > 0 {
+		return fmt.Errorf("%w:\n%w", errSSHPolicyTestsFailed, multierr.New(errs...))
+	}
+
+	return nil
+}
+
+// validateSSHTestDestination enforces that an sshTests dst entry names a
+// single SSH-reachable host. Tailscale SaaS rejects three shapes at parse
+// time: a `:port` suffix (read by the parser as an unknown tag, hence the
+// "unknown tag" error wording); a CIDR-shaped value (raw `/N` or a
+// `hosts:` entry whose RHS is a multi-host prefix); and autogroup:internet
+// (only valid in ACL destinations, not SSH ones). Tag entries must
+// reference a tag that exists in tagOwners; bare hosts must resolve to a
+// single-address prefix.
+func validateSSHTestDestination(pol *Policy, dst string) error {
+	alias, err := parseAlias(dst)
+	if err != nil {
+		return fmt.Errorf("%w %q", ErrSSHTestDstDisallowedElement, dst)
+	}
+
+	switch a := alias.(type) {
+	case *AutoGroup:
+		// autogroup:internet is the only autogroup SaaS rejects at parse.
+		// Other autogroups (member, tagged, self, nonroot) are valid SSH
+		// dst aliases and pass through to engine evaluation.
+		if *a == AutoGroupInternet {
+			return fmt.Errorf("%w %q", ErrSSHTestDstDisallowedElement, dst)
+		}
+
+	case *Prefix:
+		// A CIDR literal in dst is rejected. A bare IP parses as a Prefix
+		// with no slash in the input string — distinguish on the raw text
+		// the same way validateTestDestination does.
+		if strings.Contains(dst, "/") {
+			return fmt.Errorf("%w %q", ErrSSHTestDstDisallowedElement, dst)
+		}
+
+	case *Tag:
+		// A tag must be declared in tagOwners. The `tag:server:22` shape
+		// reaches this branch because isTag only checks the `tag:` prefix
+		// — the colon-port suffix is preserved in the Tag string and the
+		// tagOwners lookup misses, reproducing the SaaS error wording.
+		if pol == nil {
+			return fmt.Errorf("%w %q", ErrSSHTestDstUnknownTag, string(*a))
+		}
+
+		err := pol.TagOwners.Contains(a)
+		if err != nil {
+			return fmt.Errorf("%w %q", ErrSSHTestDstUnknownTag, string(*a))
+		}
+
+	case *Host:
+		// A hosts: entry that resolves to a multi-host prefix is a CIDR
+		// in disguise — reject it the same way as raw `/N`.
+		if pol == nil {
+			return nil
+		}
+
+		pref, ok := pol.Hosts[*a]
+		if !ok {
+			return nil
+		}
+
+		p := netip.Prefix(pref)
+		if p.Bits() < p.Addr().BitLen() {
+			return fmt.Errorf("%w %q", ErrSSHTestDstDisallowedElement, dst)
+		}
+	}
+
+	return nil
+}
@@ -5973,3 +5973,225 @@ func TestUnmarshalPolicyEmptyArrays(t *testing.T) {
 		})
 	}
 }
+
+// TestUnmarshalPolicySSHTests covers the parse-time shape rules for the
+// sshTests block. Positive rows confirm the SSHPolicyTest struct fields
+// round-trip through JSON. Rejection rows pin each parse-time sentinel
+// against a representative malformed input. SaaS evaluation-time failures
+// (empty assertions, empty user strings) are deliberately accepted at
+// parse — they share the "test(s) failed" body with true failures and
+// land with the engine.
+func TestUnmarshalPolicySSHTests(t *testing.T) {
+	cases := []struct {
+		name           string
+		input          string
+		wantErr        error   // sentinel for errors.Is; nil means parse must succeed
+		extraSentinels []error // additional sentinels reachable via errors.Is
+		check          func(t *testing.T, pol *Policy)
+	}{
+		{
+			name: "valid-minimal-shape",
+			input: `
+{
+  "tagOwners": {"tag:server": ["admin@example.org"]},
+  "sshTests": [
+    {"src": "thor@example.org", "dst": ["tag:server"], "accept": ["root"]}
+  ]
+}
+`,
+			check: func(t *testing.T, pol *Policy) {
+				t.Helper()
+				require.Len(t, pol.SSHTests, 1)
+				got := pol.SSHTests[0]
+				require.Equal(t, "thor@example.org", got.Src)
+				require.Equal(t, []string{"tag:server"}, got.Dst)
+				require.Equal(t, []string{"root"}, got.Accept)
+				require.Empty(t, got.Deny)
+				require.Empty(t, got.Check)
+			},
+		},
+		{
+			name: "valid-all-three-action-arrays",
+			input: `
+{
+  "tagOwners": {"tag:server": ["admin@example.org"]},
+  "sshTests": [
+    {
+      "src": "thor@example.org",
+      "dst": ["tag:server"],
+      "accept": ["root"],
+      "deny":   ["nobody"],
+      "check":  ["alice"]
+    }
+  ]
+}
+`,
+			check: func(t *testing.T, pol *Policy) {
+				t.Helper()
+				require.Len(t, pol.SSHTests, 1)
+				got := pol.SSHTests[0]
+				require.Equal(t, []string{"root"}, got.Accept)
+				require.Equal(t, []string{"nobody"}, got.Deny)
+				require.Equal(t, []string{"alice"}, got.Check)
+			},
+		},
+		{
+			// Empty accept+deny+check is rejected by SaaS at evaluation,
+			// not at parse — the captured body is the same "test(s) failed"
+			// that true evaluation failures emit. The parse layer must let
+			// this through so the engine reports it consistently.
+			name: "valid-empty-arrays-engine-deferred",
+			input: `
+{
+  "tagOwners": {"tag:server": ["admin@example.org"]},
+  "sshTests": [
+    {"src": "thor@example.org", "dst": ["tag:server"]}
+  ]
+}
+`,
+			check: func(t *testing.T, pol *Policy) {
+				t.Helper()
+				require.Len(t, pol.SSHTests, 1)
+				got := pol.SSHTests[0]
+				require.Empty(t, got.Accept)
+				require.Empty(t, got.Deny)
+				require.Empty(t, got.Check)
+			},
+		},
+		{
+			// `tag:server:22` parses as a Tag because isTag only checks
+			// the `tag:` prefix; the colon-port suffix is retained in the
+			// tag string and the tagOwners lookup misses. SaaS reports
+			// this as an unknown tag with the bad value quoted.
+			name: "dst-with-port",
+			input: `
+{
+  "tagOwners": {"tag:server": ["admin@example.org"]},
+  "sshTests": [
+    {"src": "thor@example.org", "dst": ["tag:server:22"], "accept": ["root"]}
+  ]
+}
+`,
+			wantErr: ErrSSHTestDstUnknownTag,
+		},
+		{
+			name: "dst-cidr",
+			input: `
+{
+  "tagOwners": {"tag:server": ["admin@example.org"]},
+  "sshTests": [
+    {"src": "thor@example.org", "dst": ["10.0.0.0/8"], "accept": ["root"]}
+  ]
+}
+`,
+			wantErr: ErrSSHTestDstDisallowedElement,
+		},
+		{
+			name: "dst-autogroup-internet",
+			input: `
+{
+  "tagOwners": {"tag:server": ["admin@example.org"]},
+  "sshTests": [
+    {"src": "thor@example.org", "dst": ["autogroup:internet"], "accept": ["root"]}
+  ]
+}
+`,
+			wantErr: ErrSSHTestDstDisallowedElement,
+		},
+		{
+			name: "dst-unknown-tag",
+			input: `
+{
+  "tagOwners": {"tag:server": ["admin@example.org"]},
+  "sshTests": [
+    {"src": "thor@example.org", "dst": ["tag:not-in-tagOwners"], "accept": ["root"]}
+  ]
+}
+`,
+			wantErr: ErrSSHTestDstUnknownTag,
+		},
+		{
+			name: "empty-src",
+			input: `
+{
+  "tagOwners": {"tag:server": ["admin@example.org"]},
+  "sshTests": [
+    {"src": "", "dst": ["tag:server"], "accept": ["root"]}
+  ]
+}
+`,
+			wantErr: ErrSSHTestEmptySrc,
+		},
+		{
+			name: "empty-dst",
+			input: `
+{
+  "tagOwners": {"tag:server": ["admin@example.org"]},
+  "sshTests": [
+    {"src": "thor@example.org", "dst": [], "accept": ["root"]}
+  ]
+}
+`,
+			wantErr: ErrSSHTestEmptyDst,
+		},
+		{
+			// Multiple shape failures in one entry must aggregate through
+			// multierr.New under errSSHPolicyTestsFailed so the surfaced
+			// body matches the SaaS body byte-for-byte and every
+			// individual sentinel remains reachable via errors.Is.
+			name: "multierr-wrap",
+			input: `
+{
+  "tagOwners": {"tag:server": ["admin@example.org"]},
+  "sshTests": [
+    {"src": "", "dst": ["10.0.0.0/8", "autogroup:internet"], "accept": ["root"]}
+  ]
+}
+`,
+			wantErr:        ErrSSHTestEmptySrc,
+			extraSentinels: []error{ErrSSHTestDstDisallowedElement},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			pol, err := unmarshalPolicy([]byte(tc.input))
+
+			if tc.wantErr == nil {
+				require.NoError(t, err)
+				require.NotNil(t, pol)
+
+				if tc.check != nil {
+					tc.check(t, pol)
+				}
+
+				return
+			}
+
+			require.Error(t, err)
+			require.ErrorIs(
+				t,
+				err, tc.wantErr,
+				"want errors.Is(err, %v); got %v",
+				tc.wantErr,
+				err,
+			)
+			require.Contains(
+				t,
+				err.Error(), "test(s) failed",
+				`want err to contain "test(s) failed"; got %q`,
+				err.Error(),
+			)
+
+			for _, sentinel := range tc.extraSentinels {
+				require.ErrorIs(
+					t,
+					err, sentinel,
+					"want errors.Is(err, %v); got %v",
+					sentinel,
+					err,
+				)
+			}
+		})
+	}
+}