Make more granular SSH tests for both Policies (#2555)

* policy/v1: dont consider empty if ssh has rules

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* policy/v2: replace time.Duration with model.Duration

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* policy/v2: add autogroup and ssh validation

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* policy/v2: replace time.Duration with model.Duration

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* policy: replace old ssh tests with more granular test

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* policy: skip v1 tests expected to fail (missing error handling)

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* policy: skip v1 group tests, old bugs wont be fixed

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* integration: user valid policy for ssh

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* Changelog, add ssh section

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* nix update

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

---------

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

CHANGELOG.md

@@ -62,6 +62,20 @@ new policy code passes all of our tests.
     `@` should be appended at the end. For example, if your user is `john`, it
     must be written as `john@` in the policy.
 
+**SSH**
+
+The SSH policy has been reworked to be more consistent with the rest of the
+policy. In addition, several inconsistencies between our implementation and
+Tailscale's upstream has been closed and this might be a breaking change for
+some users. Please refer to the
+[upstream documentation](https://tailscale.com/kb/1337/acl-syntax#tailscale-ssh)
+for more information on which types are allowed in `src`, `dst` and `users`.
+
+There is one large inconsistency left, we allow `*` as a destination as we
+currently do not support `autogroup:self`, `autogroup:member` and
+`autogroup:tagged`. The support for `*` will be removed when we have support for
+the autogroups.
+
 **Current state**
 
 The new policy is passing all tests, both integration and unit tests. This does
@@ -70,8 +84,6 @@ working in v1 and not tested might be broken in v2 (and vice versa).
 
 **We do need help testing this code**
 
-
-
 #### Other breaking changes
 
 - Disallow `server_url` and `base_domain` to be equal

flake.lock

@@ -20,11 +20,11 @@
     },
     "nixpkgs": {
       "locked": {
-        "lastModified": 1746152631,
+        "lastModified": 1746300365,
-        "narHash": "sha256-zBuvmL6+CUsk2J8GINpyy8Hs1Zp4PP6iBWSmZ4SCQ/s=",
+        "narHash": "sha256-thYTdWqCRipwPRxWiTiH1vusLuAy0okjOyzRx4hLWh4=",
         "owner": "NixOS",
         "repo": "nixpkgs",
-        "rev": "032bc6539bd5f14e9d0c51bd79cfe9a055b094c3",
+        "rev": "f21e4546e3ede7ae34d12a84602a22246b31f7e0",
         "type": "github"
       },
       "original": {

hscontrol/policy/policy_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"net/netip"
 	"testing"
+	"time"
 
 	"github.com/juanfont/headscale/hscontrol/policy/matcher"
 
@@ -1540,3 +1541,408 @@ func TestFilterNodesByACL(t *testing.T) {
 		})
 	}
 }
+
+func TestSSHPolicyRules(t *testing.T) {
+	users := []types.User{
+		{Name: "user1", Model: gorm.Model{ID: 1}},
+		{Name: "user2", Model: gorm.Model{ID: 2}},
+		{Name: "user3", Model: gorm.Model{ID: 3}},
+	}
+
+	// Create standard node setups used across tests
+	nodeUser1 := types.Node{
+		Hostname: "user1-device",
+		IPv4:     ap("100.64.0.1"),
+		UserID:   1,
+		User:     users[0],
+	}
+	nodeUser2 := types.Node{
+		Hostname: "user2-device",
+		IPv4:     ap("100.64.0.2"),
+		UserID:   2,
+		User:     users[1],
+	}
+	taggedServer := types.Node{
+		Hostname:   "tagged-server",
+		IPv4:       ap("100.64.0.3"),
+		UserID:     3,
+		User:       users[2],
+		ForcedTags: []string{"tag:server"},
+	}
+	taggedClient := types.Node{
+		Hostname:   "tagged-client",
+		IPv4:       ap("100.64.0.4"),
+		UserID:     2,
+		User:       users[1],
+		ForcedTags: []string{"tag:client"},
+	}
+
+	tests := []struct {
+		name         string
+		targetNode   types.Node
+		peers        types.Nodes
+		policy       string
+		wantSSH      *tailcfg.SSHPolicy
+		expectErr    bool
+		errorMessage string
+
+		// There are some tests that will not pass on V1 since we do not
+		// have the same kind of error handling as V2, so we skip them.
+		skipV1 bool
+	}{
+		{
+			name:       "group-to-user",
+			targetNode: nodeUser1,
+			peers:      types.Nodes{&nodeUser2},
+			policy: `{
+				"groups": {
+					"group:admins": ["user2@"]
+				},
+				"ssh": [
+					{
+						"action": "accept",
+						"src": ["group:admins"],
+						"dst": ["user1@"],
+						"users": ["autogroup:nonroot"]
+					}
+				]
+			}`,
+			wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{
+				{
+					Principals: []*tailcfg.SSHPrincipal{
+						{NodeIP: "100.64.0.2"},
+					},
+					SSHUsers: map[string]string{
+						"autogroup:nonroot": "=",
+					},
+					Action: &tailcfg.SSHAction{
+						Accept:                   true,
+						AllowAgentForwarding:     true,
+						AllowLocalPortForwarding: true,
+					},
+				},
+			}},
+
+			// It looks like the group implementation in v1 is broken, so
+			// we skip this test for v1 and not let it hold up v2 replacing it.
+			skipV1: true,
+		},
+		{
+			name:       "group-to-tag",
+			targetNode: taggedServer,
+			peers:      types.Nodes{&nodeUser1, &nodeUser2},
+			policy: `{
+				"groups": {
+					"group:users": ["user1@", "user2@"]
+				},
+				"ssh": [
+					{
+						"action": "accept",
+						"src": ["group:users"],
+						"dst": ["tag:server"],
+						"users": ["autogroup:nonroot"]
+					}
+				]
+			}`,
+			wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{
+				{
+					Principals: []*tailcfg.SSHPrincipal{
+						{NodeIP: "100.64.0.1"},
+						{NodeIP: "100.64.0.2"},
+					},
+					SSHUsers: map[string]string{
+						"autogroup:nonroot": "=",
+					},
+					Action: &tailcfg.SSHAction{
+						Accept:                   true,
+						AllowAgentForwarding:     true,
+						AllowLocalPortForwarding: true,
+					},
+				},
+			}},
+
+			// It looks like the group implementation in v1 is broken, so
+			// we skip this test for v1 and not let it hold up v2 replacing it.
+			skipV1: true,
+		},
+		{
+			name:       "tag-to-user",
+			targetNode: nodeUser1,
+			peers:      types.Nodes{&taggedClient},
+			policy: `{
+				"ssh": [
+					{
+						"action": "accept",
+						"src": ["tag:client"],
+						"dst": ["user1@"],
+						"users": ["autogroup:nonroot"]
+					}
+				]
+			}`,
+			wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{
+				{
+					Principals: []*tailcfg.SSHPrincipal{
+						{NodeIP: "100.64.0.4"},
+					},
+					SSHUsers: map[string]string{
+						"autogroup:nonroot": "=",
+					},
+					Action: &tailcfg.SSHAction{
+						Accept:                   true,
+						AllowAgentForwarding:     true,
+						AllowLocalPortForwarding: true,
+					},
+				},
+			}},
+		},
+		{
+			name:       "tag-to-tag",
+			targetNode: taggedServer,
+			peers:      types.Nodes{&taggedClient},
+			policy: `{
+				"ssh": [
+					{
+						"action": "accept",
+						"src": ["tag:client"],
+						"dst": ["tag:server"],
+						"users": ["autogroup:nonroot"]
+					}
+				]
+			}`,
+			wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{
+				{
+					Principals: []*tailcfg.SSHPrincipal{
+						{NodeIP: "100.64.0.4"},
+					},
+					SSHUsers: map[string]string{
+						"autogroup:nonroot": "=",
+					},
+					Action: &tailcfg.SSHAction{
+						Accept:                   true,
+						AllowAgentForwarding:     true,
+						AllowLocalPortForwarding: true,
+					},
+				},
+			}},
+		},
+		{
+			name:       "group-to-wildcard",
+			targetNode: nodeUser1,
+			peers:      types.Nodes{&nodeUser2, &taggedClient},
+			policy: `{
+				"groups": {
+					"group:admins": ["user2@"]
+				},
+				"ssh": [
+					{
+						"action": "accept",
+						"src": ["group:admins"],
+						"dst": ["*"],
+						"users": ["autogroup:nonroot"]
+					}
+				]
+			}`,
+			wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{
+				{
+					Principals: []*tailcfg.SSHPrincipal{
+						{NodeIP: "100.64.0.2"},
+					},
+					SSHUsers: map[string]string{
+						"autogroup:nonroot": "=",
+					},
+					Action: &tailcfg.SSHAction{
+						Accept:                   true,
+						AllowAgentForwarding:     true,
+						AllowLocalPortForwarding: true,
+					},
+				},
+			}},
+
+			// It looks like the group implementation in v1 is broken, so
+			// we skip this test for v1 and not let it hold up v2 replacing it.
+			skipV1: true,
+		},
+		{
+			name:       "invalid-source-user-not-allowed",
+			targetNode: nodeUser1,
+			peers:      types.Nodes{&nodeUser2},
+			policy: `{
+				"ssh": [
+					{
+						"action": "accept",
+						"src": ["user2@"],
+						"dst": ["user1@"],
+						"users": ["autogroup:nonroot"]
+					}
+				]
+			}`,
+			expectErr:    true,
+			errorMessage: "not supported",
+			skipV1:       true,
+		},
+		{
+			name:       "check-period-specified",
+			targetNode: nodeUser1,
+			peers:      types.Nodes{&taggedClient},
+			policy: `{
+				"ssh": [
+					{
+						"action": "check",
+						"checkPeriod": "24h",
+						"src": ["tag:client"],
+						"dst": ["user1@"],
+						"users": ["autogroup:nonroot"]
+					}
+				]
+			}`,
+			wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{
+				{
+					Principals: []*tailcfg.SSHPrincipal{
+						{NodeIP: "100.64.0.4"},
+					},
+					SSHUsers: map[string]string{
+						"autogroup:nonroot": "=",
+					},
+					Action: &tailcfg.SSHAction{
+						Accept:                   true,
+						SessionDuration:          24 * time.Hour,
+						AllowAgentForwarding:     true,
+						AllowLocalPortForwarding: true,
+					},
+				},
+			}},
+		},
+		{
+			name:       "no-matching-rules",
+			targetNode: nodeUser2,
+			peers:      types.Nodes{&nodeUser1},
+			policy: `{
+				"ssh": [
+					{
+						"action": "accept",
+						"src": ["tag:client"],
+						"dst": ["user1@"],
+						"users": ["autogroup:nonroot"]
+					}
+				]
+			}`,
+			wantSSH: &tailcfg.SSHPolicy{Rules: nil},
+		},
+		{
+			name:       "invalid-action",
+			targetNode: nodeUser1,
+			peers:      types.Nodes{&nodeUser2},
+			policy: `{
+				"ssh": [
+					{
+						"action": "invalid",
+						"src": ["group:admins"],
+						"dst": ["user1@"],
+						"users": ["autogroup:nonroot"]
+					}
+				]
+			}`,
+			expectErr:    true,
+			errorMessage: `SSH action "invalid" is not valid, must be accept or check`,
+			skipV1:       true,
+		},
+		{
+			name:       "invalid-check-period",
+			targetNode: nodeUser1,
+			peers:      types.Nodes{&nodeUser2},
+			policy: `{
+				"ssh": [
+					{
+						"action": "check",
+						"checkPeriod": "invalid",
+						"src": ["group:admins"],
+						"dst": ["user1@"],
+						"users": ["autogroup:nonroot"]
+					}
+				]
+			}`,
+			expectErr:    true,
+			errorMessage: "not a valid duration string",
+			skipV1:       true,
+		},
+		{
+			name:       "multiple-ssh-users-with-autogroup",
+			targetNode: nodeUser1,
+			peers:      types.Nodes{&taggedClient},
+			policy: `{
+        "ssh": [
+            {
+                "action": "accept",
+                "src": ["tag:client"],
+                "dst": ["user1@"],
+                "users": ["alice", "bob"]
+            }
+        ]
+    }`,
+			wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{
+				{
+					Principals: []*tailcfg.SSHPrincipal{
+						{NodeIP: "100.64.0.4"},
+					},
+					SSHUsers: map[string]string{
+						"alice": "=",
+						"bob":   "=",
+					},
+					Action: &tailcfg.SSHAction{
+						Accept:                   true,
+						AllowAgentForwarding:     true,
+						AllowLocalPortForwarding: true,
+					},
+				},
+			}},
+		},
+		{
+			name:       "unsupported-autogroup",
+			targetNode: nodeUser1,
+			peers:      types.Nodes{&taggedClient},
+			policy: `{
+        "ssh": [
+            {
+                "action": "accept",
+                "src": ["tag:client"],
+                "dst": ["user1@"],
+                "users": ["autogroup:invalid"]
+            }
+        ]
+    }`,
+			expectErr:    true,
+			errorMessage: "autogroup \"autogroup:invalid\" is not supported",
+			skipV1:       true,
+		},
+	}
+
+	for _, tt := range tests {
+		for idx, pmf := range PolicyManagerFuncsForTest([]byte(tt.policy)) {
+			version := idx + 1
+			t.Run(fmt.Sprintf("%s-v%d", tt.name, version), func(t *testing.T) {
+				if version == 1 && tt.skipV1 {
+					t.Skip()
+				}
+
+				var pm PolicyManager
+				var err error
+				pm, err = pmf(users, append(tt.peers, &tt.targetNode))
+
+				if tt.expectErr {
+					require.Error(t, err)
+					require.Contains(t, err.Error(), tt.errorMessage)
+					return
+				}
+
+				require.NoError(t, err)
+
+				got, err := pm.SSHPolicy(&tt.targetNode)
+				require.NoError(t, err)
+
+				if diff := cmp.Diff(tt.wantSSH, got); diff != "" {
+					t.Errorf("SSHPolicy() unexpected result (-want +got):\n%s", diff)
+				}
+			})
+		}
+	}
+}

hscontrol/policy/v1/acls_test.go

@@ -2159,200 +2159,6 @@ func Test_getTags(t *testing.T) {
 	}
 }
 
-func TestSSHRules(t *testing.T) {
-	users := []types.User{
-		{
-			Name: "user1",
-		},
-	}
-	tests := []struct {
-		name  string
-		node  types.Node
-		peers types.Nodes
-		pol   ACLPolicy
-		want  *tailcfg.SSHPolicy
-	}{
-		{
-			name: "peers-can-connect",
-			node: types.Node{
-				Hostname: "testnodes",
-				IPv4:     iap("100.64.99.42"),
-				UserID:   0,
-				User:     users[0],
-			},
-			peers: types.Nodes{
-				&types.Node{
-					Hostname: "testnodes2",
-					IPv4:     iap("100.64.0.1"),
-					UserID:   0,
-					User:     users[0],
-				},
-			},
-			pol: ACLPolicy{
-				Groups: Groups{
-					"group:test": []string{"user1"},
-				},
-				Hosts: Hosts{
-					"client": netip.PrefixFrom(netip.MustParseAddr("100.64.99.42"), 32),
-				},
-				ACLs: []ACL{
-					{
-						Action:       "accept",
-						Sources:      []string{"*"},
-						Destinations: []string{"*:*"},
-					},
-				},
-				SSHs: []SSH{
-					{
-						Action:       "accept",
-						Sources:      []string{"group:test"},
-						Destinations: []string{"client"},
-						Users:        []string{"autogroup:nonroot"},
-					},
-					{
-						Action:       "accept",
-						Sources:      []string{"*"},
-						Destinations: []string{"client"},
-						Users:        []string{"autogroup:nonroot"},
-					},
-					{
-						Action:       "accept",
-						Sources:      []string{"group:test"},
-						Destinations: []string{"100.64.99.42"},
-						Users:        []string{"autogroup:nonroot"},
-					},
-					{
-						Action:       "accept",
-						Sources:      []string{"*"},
-						Destinations: []string{"100.64.99.42"},
-						Users:        []string{"autogroup:nonroot"},
-					},
-				},
-			},
-			want: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{
-				{
-					Principals: []*tailcfg.SSHPrincipal{
-						{
-							UserLogin: "user1",
-						},
-					},
-					SSHUsers: map[string]string{
-						"autogroup:nonroot": "=",
-					},
-					Action: &tailcfg.SSHAction{
-						Accept:                   true,
-						AllowAgentForwarding:     true,
-						AllowLocalPortForwarding: true,
-					},
-				},
-				{
-					SSHUsers: map[string]string{
-						"autogroup:nonroot": "=",
-					},
-					Principals: []*tailcfg.SSHPrincipal{
-						{
-							Any: true,
-						},
-					},
-					Action: &tailcfg.SSHAction{
-						Accept:                   true,
-						AllowAgentForwarding:     true,
-						AllowLocalPortForwarding: true,
-					},
-				},
-				{
-					Principals: []*tailcfg.SSHPrincipal{
-						{
-							UserLogin: "user1",
-						},
-					},
-					SSHUsers: map[string]string{
-						"autogroup:nonroot": "=",
-					},
-					Action: &tailcfg.SSHAction{
-						Accept:                   true,
-						AllowAgentForwarding:     true,
-						AllowLocalPortForwarding: true,
-					},
-				},
-				{
-					SSHUsers: map[string]string{
-						"autogroup:nonroot": "=",
-					},
-					Principals: []*tailcfg.SSHPrincipal{
-						{
-							Any: true,
-						},
-					},
-					Action: &tailcfg.SSHAction{
-						Accept:                   true,
-						AllowAgentForwarding:     true,
-						AllowLocalPortForwarding: true,
-					},
-				},
-			}},
-		},
-		{
-			name: "peers-cannot-connect",
-			node: types.Node{
-				Hostname: "testnodes",
-				IPv4:     iap("100.64.0.1"),
-				UserID:   0,
-				User:     users[0],
-			},
-			peers: types.Nodes{
-				&types.Node{
-					Hostname: "testnodes2",
-					IPv4:     iap("100.64.99.42"),
-					UserID:   0,
-					User:     users[0],
-				},
-			},
-			pol: ACLPolicy{
-				Groups: Groups{
-					"group:test": []string{"user1"},
-				},
-				Hosts: Hosts{
-					"client": netip.PrefixFrom(netip.MustParseAddr("100.64.99.42"), 32),
-				},
-				ACLs: []ACL{
-					{
-						Action:       "accept",
-						Sources:      []string{"*"},
-						Destinations: []string{"*:*"},
-					},
-				},
-				SSHs: []SSH{
-					{
-						Action:       "accept",
-						Sources:      []string{"group:test"},
-						Destinations: []string{"100.64.99.42"},
-						Users:        []string{"autogroup:nonroot"},
-					},
-					{
-						Action:       "accept",
-						Sources:      []string{"*"},
-						Destinations: []string{"100.64.99.42"},
-						Users:        []string{"autogroup:nonroot"},
-					},
-				},
-			},
-			want: &tailcfg.SSHPolicy{Rules: nil},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got, err := tt.pol.CompileSSHPolicy(&tt.node, users, tt.peers)
-			require.NoError(t, err)
-
-			if diff := cmp.Diff(tt.want, got); diff != "" {
-				t.Errorf("TestSSHRules() unexpected result (-want +got):\n%s", diff)
-			}
-		})
-	}
-}
-
 func TestParseDestination(t *testing.T) {
 	tests := []struct {
 		dest      string

hscontrol/policy/v1/acls_types.go

@@ -90,7 +90,7 @@ func (hosts *Hosts) UnmarshalJSON(data []byte) error {
 
 // IsZero is perhaps a bit naive here.
 func (pol ACLPolicy) IsZero() bool {
-	if len(pol.Groups) == 0 && len(pol.Hosts) == 0 && len(pol.ACLs) == 0 {
+	if len(pol.Groups) == 0 && len(pol.Hosts) == 0 && len(pol.ACLs) == 0 && len(pol.SSHs) == 0 {
 		return true
 	}
 

hscontrol/policy/v2/filter.go

@@ -130,7 +130,7 @@ func (pol *Policy) compileSSHPolicy(
 		case "accept":
 			action = sshAction(true, 0)
 		case "check":
-			action = sshAction(true, rule.CheckPeriod)
+			action = sshAction(true, time.Duration(rule.CheckPeriod))
 		default:
 			return nil, fmt.Errorf("parsing SSH policy, unknown action %q, index: %d: %w", rule.Action, index, err)
 		}

hscontrol/policy/v2/types.go

@@ -6,12 +6,12 @@ import (
 	"fmt"
 	"net/netip"
 	"strings"
-	"time"
 
 	"slices"
 
 	"github.com/juanfont/headscale/hscontrol/types"
 	"github.com/juanfont/headscale/hscontrol/util"
+	"github.com/prometheus/common/model"
 	"github.com/tailscale/hujson"
 	"go4.org/netipx"
 	"tailscale.com/net/tsaddr"
@@ -383,6 +383,12 @@ type AutoGroup string
 
 const (
 	AutoGroupInternet AutoGroup = "autogroup:internet"
+	AutoGroupNonRoot  AutoGroup = "autogroup:nonroot"
+
+	// These are not yet implemented.
+	AutoGroupSelf   AutoGroup = "autogroup:self"
+	AutoGroupMember AutoGroup = "autogroup:member"
+	AutoGroupTagged AutoGroup = "autogroup:tagged"
 )
 
 var autogroups = []AutoGroup{AutoGroupInternet}
@@ -915,6 +921,99 @@ type Policy struct {
 	SSHs          []SSH              `json:"ssh"`
 }
 
+var (
+	autogroupForSrc       = []AutoGroup{}
+	autogroupForDst       = []AutoGroup{AutoGroupInternet}
+	autogroupForSSHSrc    = []AutoGroup{}
+	autogroupForSSHDst    = []AutoGroup{}
+	autogroupForSSHUser   = []AutoGroup{AutoGroupNonRoot}
+	autogroupNotSupported = []AutoGroup{AutoGroupSelf, AutoGroupMember, AutoGroupTagged}
+)
+
+func validateAutogroupSupported(ag *AutoGroup) error {
+	if ag == nil {
+		return nil
+	}
+
+	if slices.Contains(autogroupNotSupported, *ag) {
+		return fmt.Errorf("autogroup %q is not supported in headscale", *ag)
+	}
+
+	return nil
+}
+
+func validateAutogroupForSrc(src *AutoGroup) error {
+	if src == nil {
+		return nil
+	}
+
+	if src.Is(AutoGroupInternet) {
+		return fmt.Errorf(`"autogroup:internet" used in source, it can only be used in ACL destinations`)
+	}
+
+	if !slices.Contains(autogroupForSrc, *src) {
+		return fmt.Errorf("autogroup %q is not supported for ACL sources, can be %v", *src, autogroupForSrc)
+	}
+
+	return nil
+}
+
+func validateAutogroupForDst(dst *AutoGroup) error {
+	if dst == nil {
+		return nil
+	}
+
+	if !slices.Contains(autogroupForDst, *dst) {
+		return fmt.Errorf("autogroup %q is not supported for ACL destinations, can be %v", *dst, autogroupForDst)
+	}
+
+	return nil
+}
+
+func validateAutogroupForSSHSrc(src *AutoGroup) error {
+	if src == nil {
+		return nil
+	}
+
+	if src.Is(AutoGroupInternet) {
+		return fmt.Errorf(`"autogroup:internet" used in SSH source, it can only be used in ACL destinations`)
+	}
+
+	if !slices.Contains(autogroupForSSHSrc, *src) {
+		return fmt.Errorf("autogroup %q is not supported for SSH sources, can be %v", *src, autogroupForSSHSrc)
+	}
+
+	return nil
+}
+
+func validateAutogroupForSSHDst(dst *AutoGroup) error {
+	if dst == nil {
+		return nil
+	}
+
+	if dst.Is(AutoGroupInternet) {
+		return fmt.Errorf(`"autogroup:internet" used in SSH destination, it can only be used in ACL destinations`)
+	}
+
+	if !slices.Contains(autogroupForSSHDst, *dst) {
+		return fmt.Errorf("autogroup %q is not supported for SSH sources, can be %v", *dst, autogroupForSSHDst)
+	}
+
+	return nil
+}
+
+func validateAutogroupForSSHUser(user *AutoGroup) error {
+	if user == nil {
+		return nil
+	}
+
+	if !slices.Contains(autogroupForSSHUser, *user) {
+		return fmt.Errorf("autogroup %q is not supported for SSH user, can be %v", *user, autogroupForSSHUser)
+	}
+
+	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
@@ -938,20 +1037,70 @@ func (p *Policy) validate() error {
 				}
 			case *AutoGroup:
 				ag := src.(*AutoGroup)
-				if ag.Is(AutoGroupInternet) {
+
-					errs = append(errs, fmt.Errorf(`"autogroup:internet" used in source, it can only be used in ACL destinations`))
+				if err := validateAutogroupSupported(ag); err != nil {
+					errs = append(errs, err)
+					continue
+				}
+
+				if err := validateAutogroupForSrc(ag); err != nil {
+					errs = append(errs, err)
+					continue
+				}
+			}
+		}
+
+		for _, dst := range acl.Destinations {
+			switch dst.Alias.(type) {
+			case *Host:
+				h := dst.Alias.(*Host)
+				if !p.Hosts.exist(*h) {
+					errs = append(errs, fmt.Errorf(`Host %q is not defined in the Policy, please define or remove the reference to it`, *h))
+				}
+			case *AutoGroup:
+				ag := dst.Alias.(*AutoGroup)
+
+				if err := validateAutogroupSupported(ag); err != nil {
+					errs = append(errs, err)
+					continue
+				}
+
+				if err := validateAutogroupForDst(ag); err != nil {
+					errs = append(errs, err)
+					continue
 				}
 			}
 		}
 	}
 
 	for _, ssh := range p.SSHs {
+		if ssh.Action != "accept" && ssh.Action != "check" {
+			errs = append(errs, fmt.Errorf("SSH action %q is not valid, must be accept or check", ssh.Action))
+		}
+
+		for _, user := range ssh.Users {
+			if strings.HasPrefix(string(user), "autogroup:") {
+				maybeAuto := AutoGroup(user)
+				if err := validateAutogroupForSSHUser(&maybeAuto); err != nil {
+					errs = append(errs, err)
+					continue
+				}
+			}
+		}
+
 		for _, src := range ssh.Sources {
 			switch src.(type) {
 			case *AutoGroup:
 				ag := src.(*AutoGroup)
-				if ag.Is(AutoGroupInternet) {
+
-					errs = append(errs, fmt.Errorf(`"autogroup:internet" used in SSH source, it can only be used in ACL destinations`))
+				if err := validateAutogroupSupported(ag); err != nil {
+					errs = append(errs, err)
+					continue
+				}
+
+				if err := validateAutogroupForSSHSrc(ag); err != nil {
+					errs = append(errs, err)
+					continue
 				}
 			}
 		}
@@ -959,8 +1108,14 @@ func (p *Policy) validate() error {
 			switch dst.(type) {
 			case *AutoGroup:
 				ag := dst.(*AutoGroup)
-				if ag.Is(AutoGroupInternet) {
+				if err := validateAutogroupSupported(ag); err != nil {
-					errs = append(errs, fmt.Errorf(`"autogroup:internet" used in SSH destination, it can only be used in ACL destinations`))
+					errs = append(errs, err)
+					continue
+				}
+
+				if err := validateAutogroupForSSHDst(ag); err != nil {
+					errs = append(errs, err)
+					continue
 				}
 			}
 		}
@@ -976,11 +1131,11 @@ func (p *Policy) validate() error {
 
 // SSH controls who can ssh into which machines.
 type SSH struct {
-	Action       string        `json:"action"` // TODO(kradalby): add strict type
+	Action       string         `json:"action"`
 	Sources      SSHSrcAliases  `json:"src"`
 	Destinations SSHDstAliases  `json:"dst"`
 	Users        []SSHUser      `json:"users"`
-	CheckPeriod  time.Duration `json:"checkPeriod,omitempty"`
+	CheckPeriod  model.Duration `json:"checkPeriod,omitempty"`
 }
 
 // SSHSrcAliases is a list of aliases that can be used as sources in an SSH rule.
@@ -997,7 +1152,7 @@ func (a *SSHSrcAliases) UnmarshalJSON(b []byte) error {
 	*a = make([]Alias, len(aliases))
 	for i, alias := range aliases {
 		switch alias.Alias.(type) {
-		case *Username, *Group, *Tag, *AutoGroup:
+		case *Group, *Tag, *AutoGroup:
 			(*a)[i] = alias.Alias
 		default:
 			return fmt.Errorf("type %T not supported", alias.Alias)
@@ -1042,8 +1197,8 @@ func (a *SSHDstAliases) UnmarshalJSON(b []byte) error {
 			// so we will leave it in as there is no other option
 			// to dynamically give all access
 			// https://tailscale.com/kb/1193/tailscale-ssh#dst
-			Asterix,
+			// TODO(kradalby): remove this when we support autogroup:tagged and autogroup:member
-			*Group:
+			Asterix:
 			(*a)[i] = alias.Alias
 		default:
 			return fmt.Errorf("type %T not supported", alias.Alias)

integration/ssh_test.go

@@ -172,7 +172,7 @@ func TestSSHMultipleUsersAllToAll(t *testing.T) {
 				{
 					Action:       "accept",
 					Sources:      []string{"group:integration-test"},
-					Destinations: []string{"group:integration-test"},
+					Destinations: []string{"user1@", "user2@"},
 					Users:        []string{"ssh-it-user"},
 				},
 			},
@@ -267,7 +267,7 @@ func TestSSHIsBlockedInACL(t *testing.T) {
 				{
 					Action:       "accept",
 					Sources:      []string{"group:integration-test"},
-					Destinations: []string{"group:integration-test"},
+					Destinations: []string{"user1@"},
 					Users:        []string{"ssh-it-user"},
 				},
 			},
@@ -317,13 +317,13 @@ func TestSSHUserOnlyIsolation(t *testing.T) {
 				{
 					Action:       "accept",
 					Sources:      []string{"group:ssh1"},
-					Destinations: []string{"group:ssh1"},
+					Destinations: []string{"user1@"},
 					Users:        []string{"ssh-it-user"},
 				},
 				{
 					Action:       "accept",
 					Sources:      []string{"group:ssh2"},
-					Destinations: []string{"group:ssh2"},
+					Destinations: []string{"user2@"},
 					Users:        []string{"ssh-it-user"},
 				},
 			},