From 60dc17bab62acb13b5d2e37ea59ac15dccd066ff Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 16 Jan 2026 14:39:13 +0000 Subject: [PATCH] policy/v2: add SSH src/dst validation for Tailscale parity Add validateSSHSrcDstCombination() to enforce Tailscale's SSH policy rules at validation time: - dst=tag: any src allowed - dst=autogroup:self: handled at runtime (different rules per node) - dst=username: src must contain ONLY the same username Per Tailscale docs (https://tailscale.com/kb/1337/policy-syntax#dst-1): "The destination can be a tag, autogroup:self (if the source contains only users or groups), or a single named user (if the source contains only the same named user)." Invalid combinations now rejected with appropriate error messages: - Tag src -> User dst: "tags in src cannot SSH to user-owned devices" - Group src -> User dst: "groups in src cannot SSH to user-owned devices" - Different user src -> User dst: "src X cannot SSH to dst users Y" - Autogroup src -> User dst: "autogroup X in src cannot SSH to user-owned devices" Updates #3010 --- hscontrol/policy/v2/types.go | 74 ++++++++ hscontrol/policy/v2/types_test.go | 301 +++++++++++++++++++++++++++--- 2 files changed, 352 insertions(+), 23 deletions(-) diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 75b16bc1..d86bc358 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -37,6 +37,11 @@ var ErrCircularReference = errors.New("circular reference detected") var ErrUndefinedTagReference = errors.New("references undefined tag") +// ErrSSHUserDstRequiresSameUserSrc is returned when an SSH rule has a username +// destination but the source is not the same username. +// Per Tailscale docs: "users in dst are only allowed from the same user". +var ErrSSHUserDstRequiresSameUserSrc = errors.New("users in dst are only allowed from the same user") + type Asterix int func (a Asterix) Validate() error { @@ -1613,6 +1618,68 @@ func validateAutogroupForSSHUser(user *AutoGroup) error { return nil } +// validateSSHSrcDstCombination validates that SSH rule src/dst combinations +// follow Tailscale's rules: +// - dst=tag: any src allowed +// - dst=autogroup:self: src must be users or groups (no tags) - handled at runtime in filter.go +// - dst=username: src must be ONLY the same username +// +// See: https://tailscale.com/kb/1337/policy-syntax#dst-1 +// "The destination can be a tag, autogroup:self (if the source contains only users or groups), +// or a single named user (if the source contains only the same named user)." +// +// This function validates explicit username destinations. +// The autogroup:self case is handled at runtime in filter.go because it results in +// different rules for different nodes (tagged nodes get no rules). +// The * (Asterix) case is also handled at runtime in filter.go - see #3009. +func validateSSHSrcDstCombination(ssh SSH) error { + // Find all usernames in dst + var dstUsernames []Username + + for _, dst := range ssh.Destinations { + if u, ok := dst.(*Username); ok { + dstUsernames = append(dstUsernames, *u) + } + } + + // If no usernames in dst, no restriction on src (validation-wise) + // Note: * and autogroup:self are handled at runtime in filter.go + if len(dstUsernames) == 0 { + return nil + } + + // dst has username(s) - src must contain ONLY the same username(s) + for _, src := range ssh.Sources { + switch s := src.(type) { + case *Username: + // Check if this username is in dstUsernames + if !slices.Contains(dstUsernames, *s) { + return fmt.Errorf( + "%w; src %q cannot SSH to dst users %v", + ErrSSHUserDstRequiresSameUserSrc, *s, dstUsernames, + ) + } + case *Tag: + return fmt.Errorf( + "%w; tags in src cannot SSH to user-owned devices", + ErrSSHUserDstRequiresSameUserSrc, + ) + case *Group: + return fmt.Errorf( + "%w; groups in src cannot SSH to user-owned devices", + ErrSSHUserDstRequiresSameUserSrc, + ) + case *AutoGroup: + return fmt.Errorf( + "%w; autogroup %q in src cannot SSH to user-owned devices", + ErrSSHUserDstRequiresSameUserSrc, *s, + ) + } + } + + return nil +} + // validate reports if there are any errors in a policy after // the unmarshaling process. // It runs through all rules and checks if there are any inconsistencies @@ -1754,6 +1821,13 @@ func (p *Policy) validate() error { } } } + + // Validate SSH src/dst combination rules (#3010) + // This rejects policies where tags/groups/different-users are in src when dst is a username + err := validateSSHSrcDstCombination(ssh) + if err != nil { + errs = append(errs, err) + } } for _, tagOwners := range p.TagOwners { diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index 664f76b7..a68f963f 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -660,7 +660,9 @@ func TestUnmarshalPolicy(t *testing.T) { }, }, { - name: "ssh-with-tag-and-user", + // #3010: Tag src -> User dst should be rejected + // Tailscale rule: "users in dst are only allowed from the same user" + name: "ssh-tag-src-user-dst-rejected", input: ` { "tagOwners": { @@ -680,32 +682,16 @@ func TestUnmarshalPolicy(t *testing.T) { ] } `, - want: &Policy{ - TagOwners: TagOwners{ - Tag("tag:web"): Owners{ptr.To(Username("admin@example.com"))}, - }, - SSHs: []SSH{ - { - Action: "accept", - Sources: SSHSrcAliases{ - tp("tag:web"), - }, - Destinations: SSHDstAliases{ - ptr.To(Username("admin@example.com")), - }, - Users: []SSHUser{ - SSHUser("*"), - }, - }, - }, - }, + wantErr: "users in dst are only allowed from the same user", }, { - name: "ssh-with-check-period", + // #3010: Group src -> User dst should be rejected + // Groups contain multiple users, so they can't SSH to a single user's devices + name: "ssh-group-src-user-dst-rejected", input: ` { "groups": { - "group:admins": ["admin@example.com"] + "group:admins": ["admin@example.com", "other@example.com"] }, "ssh": [ { @@ -716,6 +702,272 @@ func TestUnmarshalPolicy(t *testing.T) { "dst": [ "admin@example.com" ], + "users": ["*"] + } + ] +} +`, + wantErr: "users in dst are only allowed from the same user", + }, + { + // #3010: Different user src -> User dst should be rejected + name: "ssh-different-user-src-dst-rejected", + input: ` +{ + "ssh": [ + { + "action": "accept", + "src": [ + "bob@example.com" + ], + "dst": [ + "alice@example.com" + ], + "users": ["*"] + } + ] +} +`, + wantErr: "users in dst are only allowed from the same user", + }, + { + // #3010: Same user src -> Same user dst should be allowed + name: "ssh-same-user-src-dst-allowed", + input: ` +{ + "ssh": [ + { + "action": "accept", + "src": [ + "alice@example.com" + ], + "dst": [ + "alice@example.com" + ], + "users": ["*"] + } + ] +} +`, + want: &Policy{ + SSHs: []SSH{ + { + Action: "accept", + Sources: SSHSrcAliases{ + ptr.To(Username("alice@example.com")), + }, + Destinations: SSHDstAliases{ + ptr.To(Username("alice@example.com")), + }, + Users: []SSHUser{ + SSHUser("*"), + }, + }, + }, + }, + }, + { + // #3010: Tag src -> Tag dst should be allowed + name: "ssh-tag-src-tag-dst-allowed", + input: ` +{ + "tagOwners": { + "tag:web": ["admin@example.com"], + "tag:server": ["admin@example.com"] + }, + "ssh": [ + { + "action": "accept", + "src": [ + "tag:web" + ], + "dst": [ + "tag:server" + ], + "users": ["root"] + } + ] +} +`, + want: &Policy{ + TagOwners: TagOwners{ + Tag("tag:web"): Owners{ptr.To(Username("admin@example.com"))}, + Tag("tag:server"): Owners{ptr.To(Username("admin@example.com"))}, + }, + SSHs: []SSH{ + { + Action: "accept", + Sources: SSHSrcAliases{ + tp("tag:web"), + }, + Destinations: SSHDstAliases{ + tp("tag:server"), + }, + Users: []SSHUser{ + SSHUser("root"), + }, + }, + }, + }, + }, + { + // #3010: User src -> Tag dst should be allowed + name: "ssh-user-src-tag-dst-allowed", + input: ` +{ + "tagOwners": { + "tag:server": ["admin@example.com"] + }, + "ssh": [ + { + "action": "accept", + "src": [ + "admin@example.com" + ], + "dst": [ + "tag:server" + ], + "users": ["root"] + } + ] +} +`, + want: &Policy{ + TagOwners: TagOwners{ + Tag("tag:server"): Owners{ptr.To(Username("admin@example.com"))}, + }, + SSHs: []SSH{ + { + Action: "accept", + Sources: SSHSrcAliases{ + ptr.To(Username("admin@example.com")), + }, + Destinations: SSHDstAliases{ + tp("tag:server"), + }, + Users: []SSHUser{ + SSHUser("root"), + }, + }, + }, + }, + }, + { + // #3010: Group src -> Tag dst should be allowed + name: "ssh-group-src-tag-dst-allowed", + input: ` +{ + "groups": { + "group:admins": ["admin@example.com"] + }, + "tagOwners": { + "tag:server": ["admin@example.com"] + }, + "ssh": [ + { + "action": "accept", + "src": [ + "group:admins" + ], + "dst": [ + "tag:server" + ], + "users": ["root"] + } + ] +} +`, + want: &Policy{ + Groups: Groups{ + Group("group:admins"): []Username{Username("admin@example.com")}, + }, + TagOwners: TagOwners{ + Tag("tag:server"): Owners{ptr.To(Username("admin@example.com"))}, + }, + SSHs: []SSH{ + { + Action: "accept", + Sources: SSHSrcAliases{ + gp("group:admins"), + }, + Destinations: SSHDstAliases{ + tp("tag:server"), + }, + Users: []SSHUser{ + SSHUser("root"), + }, + }, + }, + }, + }, + { + // #3010: Mixed src (tag + user) -> User dst should be rejected + // Even if the user matches, having a tag in src invalidates it + name: "ssh-mixed-src-user-dst-rejected", + input: ` +{ + "tagOwners": { + "tag:web": ["admin@example.com"] + }, + "ssh": [ + { + "action": "accept", + "src": [ + "tag:web", + "admin@example.com" + ], + "dst": [ + "admin@example.com" + ], + "users": ["*"] + } + ] +} +`, + wantErr: "users in dst are only allowed from the same user", + }, + { + // #3010: Autogroup in src -> User dst should be rejected + // autogroup:member contains multiple users + name: "ssh-autogroup-src-user-dst-rejected", + input: ` +{ + "ssh": [ + { + "action": "accept", + "src": [ + "autogroup:member" + ], + "dst": [ + "admin@example.com" + ], + "users": ["*"] + } + ] +} +`, + wantErr: "users in dst are only allowed from the same user", + }, + { + // Test checkPeriod with valid src/dst combination (group -> tag) + name: "ssh-with-check-period", + input: ` +{ + "groups": { + "group:admins": ["admin@example.com"] + }, + "tagOwners": { + "tag:server": ["admin@example.com"] + }, + "ssh": [ + { + "action": "accept", + "src": [ + "group:admins" + ], + "dst": [ + "tag:server" + ], "users": ["root"], "checkPeriod": "24h" } @@ -726,6 +978,9 @@ func TestUnmarshalPolicy(t *testing.T) { Groups: Groups{ Group("group:admins"): []Username{Username("admin@example.com")}, }, + TagOwners: TagOwners{ + Tag("tag:server"): Owners{ptr.To(Username("admin@example.com"))}, + }, SSHs: []SSH{ { Action: "accept", @@ -733,7 +988,7 @@ func TestUnmarshalPolicy(t *testing.T) { gp("group:admins"), }, Destinations: SSHDstAliases{ - ptr.To(Username("admin@example.com")), + tp("tag:server"), }, Users: []SSHUser{ SSHUser("root"),