diff --git a/hscontrol/policy/v2/sshtest_test.go b/hscontrol/policy/v2/sshtest_test.go index d2d5595e6..99e4a6fde 100644 --- a/hscontrol/policy/v2/sshtest_test.go +++ b/hscontrol/policy/v2/sshtest_test.go @@ -748,32 +748,9 @@ func TestRunSSHTests(t *testing.T) { wantPass: false, wantErrSub: []string{"autogroup:self", "resolved to no nodes"}, }, - { - // A src naming address space is evaluated through the node - // addresses inside it, never address by address: ::/0 - // holds alice's IPv6, which the rule allows. - name: "src-cidr-deny-fails-on-covered-node", - policy: `{ - "tagOwners": { "tag:server": ["alice@headscale.net"] }, - "ssh": [{ - "action": "accept", - "src": ["alice@headscale.net"], - "dst": ["tag:server"], - "users": ["root"] - }], - "sshTests": [{ - "src": "::/0", - "dst": ["tag:server"], - "deny": ["root"] - }] - }`, - wantPass: false, - wantErrSub: []string{"::/0", "expected DENIED, got ALLOWED"}, - }, { name: "src-address-space-deny-pass", policy: `{ - "hosts": { "lan": "10.0.0.0/8" }, "tagOwners": { "tag:server": ["alice@headscale.net"] }, "ssh": [{ "action": "accept", @@ -784,8 +761,7 @@ func TestRunSSHTests(t *testing.T) { "sshTests": [ { "src": "*", "dst": ["tag:server"], "deny": ["ubuntu"] }, { "src": "autogroup:danger-all", "dst": ["tag:server"], "deny": ["ubuntu"] }, - { "src": "autogroup:internet", "dst": ["tag:server"], "deny": ["root"] }, - { "src": "lan", "dst": ["tag:server"], "deny": ["root"] } + { "src": "autogroup:internet", "dst": ["tag:server"], "deny": ["root"] } ] }`, wantPass: true, @@ -811,25 +787,6 @@ func TestRunSSHTests(t *testing.T) { wantPass: false, wantErrSub: []string{"expected ALLOWED, got DENIED"}, }, - { - // 100.64.0.4/31 is exactly alice-tablet and thor-laptop. - name: "src-cidr-fully-assigned-accept-pass", - policy: `{ - "tagOwners": { "tag:server": ["alice@headscale.net"] }, - "ssh": [{ - "action": "accept", - "src": ["autogroup:member"], - "dst": ["tag:server"], - "users": ["root"] - }], - "sshTests": [{ - "src": "100.64.0.4/31", - "dst": ["tag:server"], - "accept": ["root"] - }] - }`, - wantPass: true, - }, } for _, tt := range tests { diff --git a/hscontrol/policy/v2/sshtester_compat_test.go b/hscontrol/policy/v2/sshtester_compat_test.go index 9503bc398..49befe3e8 100644 --- a/hscontrol/policy/v2/sshtester_compat_test.go +++ b/hscontrol/policy/v2/sshtester_compat_test.go @@ -31,16 +31,6 @@ var knownSSHTesterDivergences = map[string]string{ "sshtest-report-multi/label": sshTestNodeLabel, "sshtest-src-unassigned-ip-accept/label": sshTestNodeLabel, - "sshtest-src-cidr24-deny/message": sshTestSrcRange, - "sshtest-src-cidr31-empty-accept/message": sshTestSrcRange, - "sshtest-src-cidr31-empty-deny/verdict": sshTestSrcRange, - "sshtest-src-cidr31-hole-accept/message": sshTestSrcRange, - "sshtest-src-cidr31-members-accept/verdict": sshTestSrcRange, - "sshtest-src-hosts-cidr-deny/verdict": sshTestSrcRange, - - "sshtest-fail-label-autogroup-member/message": sshTestDstMember, - "sshtest-check-reason-mixed-alias/message": sshTestDstMember, - "sshtest-fail-label-same-node-twice/breakdown": "SSHTEST_PER_ALIAS: SaaS reports each dst alias, also two naming one node; headscale checks each node once", "sshtest-fail-label-ip-no-device/breakdown": sshTestNoDevice, "sshtest-report-multi/breakdown": "SSHTEST_PER_ALIAS+SSHTEST_NO_DEVICE: SaaS reports one line per dst alias and a deviceless alias as not reached; headscale one per node and an error", @@ -49,8 +39,6 @@ var knownSSHTesterDivergences = map[string]string{ const ( sshTestNodeLabel = "SSHTEST_NODE_LABEL: headscale names each failing dst node by hostname; SaaS names the dst as written" - sshTestSrcRange = "SSHTEST_SRC_RANGE: SaaS refuses a CIDR, or a host alias holding one, as an sshTests source when parsing; headscale evaluates it" - sshTestDstMember = "SSHTEST_DST_MEMBER: SaaS refuses autogroup:member as an sshTests destination when parsing; headscale evaluates it" sshTestNoDevice = "SSHTEST_NO_DEVICE: SaaS reports a dst that selects no device as not reached; headscale as an error" ) diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index fadf5dcf2..9e40e4b47 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -156,6 +156,7 @@ var ( 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") + ErrSSHTestSrcDisallowedElement = errors.New("SSH tests src contains disallowed element") ) type resolved struct { @@ -3397,6 +3398,10 @@ func validateSSHTests(pol *Policy, tests []SSHPolicyTest) error { for i, t := range tests { if t.Src == nil { errs = append(errs, fmt.Errorf("sshTest %d: %w", i, ErrSSHTestEmptySrc)) + } else if isAddressRange(pol, t.Src) { + // Tailscale SaaS refuses a range: a test speaks for one source. + errs = append(errs, fmt.Errorf("sshTest %d: %w %q", + i, ErrSSHTestSrcDisallowedElement, t.Src.String())) } if len(t.Dst) == 0 { @@ -3418,31 +3423,54 @@ func validateSSHTests(pol *Policy, tests []SSHPolicyTest) error { return nil } +// isAddressRange reports whether alias is a CIDR, or a hosts: entry +// holding one, that names more than one address. A bare IP parses as a +// `/BitLen` prefix and names one. +func isAddressRange(pol *Policy, alias Alias) bool { + var p netip.Prefix + + switch a := alias.(type) { + case *Prefix: + p = netip.Prefix(*a) + case *Host: + if pol == nil { + return false + } + + pref, ok := pol.Hosts[*a] + if !ok { + return false + } + + p = netip.Prefix(pref) + default: + return false + } + + return p.Bits() < p.Addr().BitLen() +} + // validateSSHTestDestination rejects sshTests dst shapes that cannot // name a single SSH-reachable host: // // - `host:port` suffixes (parsed as an unknown tag), // - multi-host CIDRs (raw `/N` or a hosts: entry resolving wider), -// - autogroup:internet (valid as ACL dst only). +// - autogroup:internet (valid as ACL dst only), +// - autogroup:member, which Tailscale SaaS refuses here. // // A bare IP literal (single-host /BitLen prefix) is accepted. Tag // entries must exist in tagOwners. func validateSSHTestDestination(pol *Policy, alias Alias) error { dst := alias.String() + if isAddressRange(pol, alias) { + return fmt.Errorf("%w %q", ErrSSHTestDstDisallowedElement, dst) + } + switch a := alias.(type) { case *AutoGroup: - // autogroup:internet is the only autogroup not valid here; - // member/tagged/self/nonroot pass to engine evaluation. - if *a == AutoGroupInternet { - return fmt.Errorf("%w %q", ErrSSHTestDstDisallowedElement, dst) - } - - case *Prefix: - // A bare IP parses as `/BitLen` and is a valid single-host dst; - // any narrower CIDR is a multi-host range and is rejected. - p := netip.Prefix(*a) - if p.Bits() < p.Addr().BitLen() { + // tagged/self/nonroot pass to engine evaluation. + if *a == AutoGroupInternet || *a == AutoGroupMember { return fmt.Errorf("%w %q", ErrSSHTestDstDisallowedElement, dst) } @@ -3458,23 +3486,6 @@ func validateSSHTestDestination(pol *Policy, alias Alias) error { if err != nil { return fmt.Errorf("%w %q", ErrSSHTestDstUnknownTag, string(*a)) } - - case *Host: - // A hosts: alias that resolves to multiple addresses is a CIDR - // in disguise. - 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 diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index 057307598..f0f19a911 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -6032,6 +6032,42 @@ func TestUnmarshalPolicySSHTests(t *testing.T) { require.Equal(t, "fd7a:115c:a1e0::10/128", pref.String()) }, }, + { + name: "src-cidr", + input: ` +{ + "tagOwners": {"tag:server": ["admin@example.org"]}, + "sshTests": [ + {"src": "100.64.0.0/24", "dst": ["tag:server"], "deny": ["root"]} + ] +} +`, + wantErr: ErrSSHTestSrcDisallowedElement, + }, + { + name: "src-hosts-cidr", + input: ` +{ + "hosts": {"lan": "192.168.1.0/24"}, + "tagOwners": {"tag:server": ["admin@example.org"]}, + "sshTests": [ + {"src": "lan", "dst": ["tag:server"], "deny": ["root"]} + ] +} +`, + wantErr: ErrSSHTestSrcDisallowedElement, + }, + { + name: "src-bare-ip-accepted", + input: ` +{ + "tagOwners": {"tag:server": ["admin@example.org"]}, + "sshTests": [ + {"src": "100.64.0.30", "dst": ["tag:server"], "accept": ["root"]} + ] +} +`, + }, { name: "dst-autogroup-internet", input: ` @@ -6041,6 +6077,18 @@ func TestUnmarshalPolicySSHTests(t *testing.T) { {"src": "thor@example.org", "dst": ["autogroup:internet"], "accept": ["root"]} ] } +`, + wantErr: ErrSSHTestDstDisallowedElement, + }, + { + name: "dst-autogroup-member", + input: ` +{ + "tagOwners": {"tag:server": ["admin@example.org"]}, + "sshTests": [ + {"src": "thor@example.org", "dst": ["autogroup:member"], "accept": ["root"]} + ] +} `, wantErr: ErrSSHTestDstDisallowedElement, }, diff --git a/hscontrol/servertest/policy_test.go b/hscontrol/servertest/policy_test.go index 8a12f87e2..b7d76fd9f 100644 --- a/hscontrol/servertest/policy_test.go +++ b/hscontrol/servertest/policy_test.go @@ -327,10 +327,6 @@ var rejectedPolicySkipReasons = map[string]string{ "policytest-src-cidr-accept/refused": "TEST_SRC_SHAPE: headscale evaluates a CIDR tests source", "policytest-src-wildcard-deny/refused": "TEST_SRC_SHAPE: headscale evaluates \"*\" as a tests source", "policytest-dst-multinode-partial-accept/refused": "TEST_DST_EVERY_NODE: headscale accepts when one node of the dst alias is reached", - - "sshtest-src-cidr31-empty-deny/refused": "SSHTEST_SRC_RANGE: headscale evaluates a CIDR sshTests source", - "sshtest-src-cidr31-members-accept/refused": "SSHTEST_SRC_RANGE: headscale evaluates a CIDR sshTests source", - "sshtest-src-hosts-cidr-deny/refused": "SSHTEST_SRC_RANGE: headscale evaluates a host alias holding a CIDR as an sshTests source", } const noTailnetDomains = "NO_TAILNET_DOMAINS: headscale does not check user:*@domain or localpart:*@domain against the tailnet's domains" diff --git a/integration/cli_policy_test.go b/integration/cli_policy_test.go index 13f20dad1..4811355c1 100644 --- a/integration/cli_policy_test.go +++ b/integration/cli_policy_test.go @@ -201,8 +201,8 @@ func TestPolicyCheckCommand(t *testing.T) { // TestSSHTestsRejectFailingPolicy asserts `headscale policy set` rejects // a policy whose sshTests fail, surfaces the engine's "test(s) failed" -// sentinel, and leaves the stored policy unchanged. autogroup:member as -// dst lets every scenario node count, so no tagged node is needed. +// sentinel, and leaves the stored policy unchanged. The tests name user1's +// node by its user, so no tagged node is needed. func TestSSHTestsRejectFailingPolicy(t *testing.T) { IntegrationSkip(t) @@ -226,7 +226,7 @@ func TestSSHTestsRejectFailingPolicy(t *testing.T) { SSHTests: []policyv2.SSHPolicyTest{ { Src: usernamep(user1), - Dst: policyv2.SSHTestDestinations{new(policyv2.AutoGroupMember)}, + Dst: policyv2.SSHTestDestinations{usernamep(user1)}, Accept: []policyv2.SSHUser{policyv2.SSHUser("root")}, }, }, @@ -238,7 +238,7 @@ func TestSSHTestsRejectFailingPolicy(t *testing.T) { badPolicy.SSHTests = []policyv2.SSHPolicyTest{ { Src: usernamep(user2), - Dst: policyv2.SSHTestDestinations{new(policyv2.AutoGroupMember)}, + Dst: policyv2.SSHTestDestinations{usernamep(user1)}, Accept: []policyv2.SSHUser{policyv2.SSHUser("root")}, }, }