policy/v2: validate that no undefined group or tag is used (#2576)

* policy/v2: allow Username as ssh source

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

* policy/v2: validate that no undefined group or tag is used

Fixes #2570

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

* policy: fixup tests which violated tag constraing

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

---------

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

hscontrol/policy/policy_test.go

@@ -709,6 +709,9 @@ func TestReduceFilterRules(t *testing.T) {
 			name: "1817-reduce-breaks-32-mask",
 			pol: `
 {
+  "tagOwners": {
+    "tag:access-servers": ["user100@"],
+  },
   "groups": {
     "group:access": [
       "user1@"
@@ -1688,6 +1691,9 @@ func TestSSHPolicyRules(t *testing.T) {
 			targetNode: taggedServer,
 			peers:      types.Nodes{&nodeUser1, &nodeUser2},
 			policy: `{
+				"tagOwners": {
+				  "tag:server": ["user3@"],
+				},
 				"groups": {
 					"group:users": ["user1@", "user2@"]
 				},
@@ -1726,6 +1732,9 @@ func TestSSHPolicyRules(t *testing.T) {
 			targetNode: nodeUser1,
 			peers:      types.Nodes{&taggedClient},
 			policy: `{
+			    "tagOwners": {
+					"tag:client": ["user1@"],
+				},
 				"ssh": [
 					{
 						"action": "accept",
@@ -1756,6 +1765,10 @@ func TestSSHPolicyRules(t *testing.T) {
 			targetNode: taggedServer,
 			peers:      types.Nodes{&taggedClient},
 			policy: `{
+				"tagOwners": {
+					"tag:client": ["user2@"],
+					"tag:server": ["user3@"],
+				},
 				"ssh": [
 					{
 						"action": "accept",
@@ -1818,29 +1831,14 @@ func TestSSHPolicyRules(t *testing.T) {
 			// 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: `{
+				"tagOwners": {
+					"tag:client": ["user1@"],
+				},
 				"ssh": [
 					{
 						"action": "check",
@@ -1873,6 +1871,9 @@ func TestSSHPolicyRules(t *testing.T) {
 			targetNode: nodeUser2,
 			peers:      types.Nodes{&nodeUser1},
 			policy: `{
+			    "tagOwners": {
+			    	"tag:client": ["user1@"],
+			    },
 				"ssh": [
 					{
 						"action": "accept",
@@ -1926,6 +1927,9 @@ func TestSSHPolicyRules(t *testing.T) {
 			targetNode: nodeUser1,
 			peers:      types.Nodes{&taggedClient},
 			policy: `{
+			"tagOwners": {
+				"tag:client": ["user1@"],
+			},
         	"ssh": [
         	    {
         	        "action": "accept",

hscontrol/policy/v2/types.go

@@ -720,6 +720,20 @@ type Usernames []Username
 // Groups are a map of Group to a list of Username.
 type Groups map[Group]Usernames
 
+func (g Groups) Contains(group *Group) error {
+	if group == nil {
+		return nil
+	}
+
+	for defined := range map[Group]Usernames(g) {
+		if defined == *group {
+			return nil
+		}
+	}
+
+	return fmt.Errorf(`Group %q is not defined in the Policy, please define or remove the reference to it`, group)
+}
+
 // UnmarshalJSON overrides the default JSON unmarshalling for Groups to ensure
 // that each group name is validated using the isGroup function. This ensures
 // that all group names conform to the expected format, which is always prefixed
@@ -791,6 +805,20 @@ func (h Hosts) exist(name Host) bool {
 // TagOwners are a map of Tag to a list of the UserEntities that own the tag.
 type TagOwners map[Tag]Owners
 
+func (to TagOwners) Contains(tagOwner *Tag) error {
+	if tagOwner == nil {
+		return nil
+	}
+
+	for defined := range map[Tag]Owners(to) {
+		if defined == *tagOwner {
+			return nil
+		}
+	}
+
+	return fmt.Errorf(`Tag %q is not defined in the Policy, please define or remove the reference to it`, tagOwner)
+}
+
 // resolveTagOwners resolves the TagOwners to a map of Tag to netipx.IPSet.
 // The resulting map can be used to quickly look up the IPSet for a given Tag.
 // It is intended for internal use in a PolicyManager.
@@ -1047,6 +1075,16 @@ func (p *Policy) validate() error {
 					errs = append(errs, err)
 					continue
 				}
+			case *Group:
+				g := src.(*Group)
+				if err := p.Groups.Contains(g); err != nil {
+					errs = append(errs, err)
+				}
+			case *Tag:
+				tagOwner := src.(*Tag)
+				if err := p.TagOwners.Contains(tagOwner); err != nil {
+					errs = append(errs, err)
+				}
 			}
 		}
 
@@ -1069,6 +1107,16 @@ func (p *Policy) validate() error {
 					errs = append(errs, err)
 					continue
 				}
+			case *Group:
+				g := dst.Alias.(*Group)
+				if err := p.Groups.Contains(g); err != nil {
+					errs = append(errs, err)
+				}
+			case *Tag:
+				tagOwner := dst.Alias.(*Tag)
+				if err := p.TagOwners.Contains(tagOwner); err != nil {
+					errs = append(errs, err)
+				}
 			}
 		}
 	}
@@ -1102,6 +1150,16 @@ func (p *Policy) validate() error {
 					errs = append(errs, err)
 					continue
 				}
+			case *Group:
+				g := src.(*Group)
+				if err := p.Groups.Contains(g); err != nil {
+					errs = append(errs, err)
+				}
+			case *Tag:
+				tagOwner := src.(*Tag)
+				if err := p.TagOwners.Contains(tagOwner); err != nil {
+					errs = append(errs, err)
+				}
 			}
 		}
 		for _, dst := range ssh.Destinations {
@@ -1117,6 +1175,55 @@ func (p *Policy) validate() error {
 					errs = append(errs, err)
 					continue
 				}
+			case *Tag:
+				tagOwner := dst.(*Tag)
+				if err := p.TagOwners.Contains(tagOwner); err != nil {
+					errs = append(errs, err)
+				}
+			}
+		}
+	}
+
+	for _, tagOwners := range p.TagOwners {
+		for _, tagOwner := range tagOwners {
+			switch tagOwner.(type) {
+			case *Group:
+				g := tagOwner.(*Group)
+				if err := p.Groups.Contains(g); err != nil {
+					errs = append(errs, err)
+				}
+			}
+		}
+	}
+
+	for _, approvers := range p.AutoApprovers.Routes {
+		for _, approver := range approvers {
+			switch approver.(type) {
+			case *Group:
+				g := approver.(*Group)
+				if err := p.Groups.Contains(g); err != nil {
+					errs = append(errs, err)
+				}
+			case *Tag:
+				tagOwner := approver.(*Tag)
+				if err := p.TagOwners.Contains(tagOwner); err != nil {
+					errs = append(errs, err)
+				}
+			}
+		}
+	}
+
+	for _, approver := range p.AutoApprovers.ExitNode {
+		switch approver.(type) {
+		case *Group:
+			g := approver.(*Group)
+			if err := p.Groups.Contains(g); err != nil {
+				errs = append(errs, err)
+			}
+		case *Tag:
+			tagOwner := approver.(*Tag)
+			if err := p.TagOwners.Contains(tagOwner); err != nil {
+				errs = append(errs, err)
 			}
 		}
 	}
@@ -1152,7 +1259,7 @@ func (a *SSHSrcAliases) UnmarshalJSON(b []byte) error {
 	*a = make([]Alias, len(aliases))
 	for i, alias := range aliases {
 		switch alias.Alias.(type) {
-		case *Group, *Tag, *AutoGroup:
+		case *Username, *Group, *Tag, *AutoGroup:
 			(*a)[i] = alias.Alias
 		default:
 			return fmt.Errorf("type %T not supported", alias.Alias)

hscontrol/policy/v2/types_test.go

@@ -511,6 +511,201 @@ func TestUnmarshalPolicy(t *testing.T) {
 `,
 			wantErr: `"autogroup:internet" used in SSH destination, it can only be used in ACL destinations`,
 		},
+		{
+			name: "group-must-be-defined-acl-src",
+			input: `
+{
+  "acls": [
+    {
+      "action": "accept",
+      "src": [
+        "group:notdefined"
+      ],
+      "dst": [
+        "autogroup:internet:*"
+      ]
+    }
+  ]
+}
+`,
+			wantErr: `Group "group:notdefined" is not defined in the Policy, please define or remove the reference to it`,
+		},
+		{
+			name: "group-must-be-defined-acl-dst",
+			input: `
+{
+  "acls": [
+    {
+      "action": "accept",
+      "src": [
+        "*"
+      ],
+      "dst": [
+        "group:notdefined:*"
+      ]
+    }
+  ]
+}
+`,
+			wantErr: `Group "group:notdefined" is not defined in the Policy, please define or remove the reference to it`,
+		},
+		{
+			name: "group-must-be-defined-acl-ssh-src",
+			input: `
+{
+  "ssh": [
+    {
+      "action": "accept",
+      "src": [
+        "group:notdefined"
+      ],
+      "dst": [
+        "user@"
+      ]
+    }
+  ]
+}
+`,
+			wantErr: `Group "group:notdefined" is not defined in the Policy, please define or remove the reference to it`,
+		},
+		{
+			name: "group-must-be-defined-acl-tagOwner",
+			input: `
+{
+  "tagOwners": {
+    "tag:test": ["group:notdefined"],
+  },
+}
+`,
+			wantErr: `Group "group:notdefined" is not defined in the Policy, please define or remove the reference to it`,
+		},
+		{
+			name: "group-must-be-defined-acl-autoapprover-route",
+			input: `
+{
+  "autoApprovers": {
+    "routes": {
+      "10.0.0.0/16": ["group:notdefined"]
+    }
+  },
+}
+`,
+			wantErr: `Group "group:notdefined" is not defined in the Policy, please define or remove the reference to it`,
+		},
+		{
+			name: "group-must-be-defined-acl-autoapprover-exitnode",
+			input: `
+{
+  "autoApprovers": {
+    "exitNode": ["group:notdefined"]
+   },
+}
+`,
+			wantErr: `Group "group:notdefined" is not defined in the Policy, please define or remove the reference to it`,
+		},
+		{
+			name: "tag-must-be-defined-acl-src",
+			input: `
+{
+  "acls": [
+    {
+      "action": "accept",
+      "src": [
+        "tag:notdefined"
+      ],
+      "dst": [
+        "autogroup:internet:*"
+      ]
+    }
+  ]
+}
+`,
+			wantErr: `Tag "tag:notdefined" is not defined in the Policy, please define or remove the reference to it`,
+		},
+		{
+			name: "tag-must-be-defined-acl-dst",
+			input: `
+{
+  "acls": [
+    {
+      "action": "accept",
+      "src": [
+        "*"
+      ],
+      "dst": [
+        "tag:notdefined:*"
+      ]
+    }
+  ]
+}
+`,
+			wantErr: `Tag "tag:notdefined" is not defined in the Policy, please define or remove the reference to it`,
+		},
+		{
+			name: "tag-must-be-defined-acl-ssh-src",
+			input: `
+{
+  "ssh": [
+    {
+      "action": "accept",
+      "src": [
+        "tag:notdefined"
+      ],
+      "dst": [
+        "user@"
+      ]
+    }
+  ]
+}
+`,
+			wantErr: `Tag "tag:notdefined" is not defined in the Policy, please define or remove the reference to it`,
+		},
+		{
+			name: "tag-must-be-defined-acl-ssh-dst",
+			input: `
+{
+  "groups": {
+  	"group:defined": ["user@"],
+  },
+  "ssh": [
+    {
+      "action": "accept",
+      "src": [
+        "group:defined"
+      ],
+      "dst": [
+        "tag:notdefined",
+      ],
+    }
+  ]
+}
+`,
+			wantErr: `Tag "tag:notdefined" is not defined in the Policy, please define or remove the reference to it`,
+		},
+		{
+			name: "tag-must-be-defined-acl-autoapprover-route",
+			input: `
+{
+  "autoApprovers": {
+    "routes": {
+      "10.0.0.0/16": ["tag:notdefined"]
+    }
+  },
+}
+`,
+			wantErr: `Tag "tag:notdefined" is not defined in the Policy, please define or remove the reference to it`,
+		},
+		{
+			name: "tag-must-be-defined-acl-autoapprover-exitnode",
+			input: `
+{
+  "autoApprovers": {
+    "exitNode": ["tag:notdefined"]
+   },
+}
+`,
+			wantErr: `Tag "tag:notdefined" is not defined in the Policy, please define or remove the reference to it`,
+		},
 	}
 
 	cmps := append(util.Comparers, cmp.Comparer(func(x, y Prefix) bool {