From 56db4ed0f149448c624d10c949ab54779909c5a3 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 9 May 2025 12:51:30 +0300 Subject: [PATCH] policy/v2: validate that no undefined group or tag is used (#2576) * policy/v2: allow Username as ssh source Signed-off-by: Kristoffer Dalby * policy/v2: validate that no undefined group or tag is used Fixes #2570 Signed-off-by: Kristoffer Dalby * policy: fixup tests which violated tag constraing Signed-off-by: Kristoffer Dalby --------- Signed-off-by: Kristoffer Dalby --- hscontrol/policy/policy_test.go | 56 +++++---- hscontrol/policy/v2/types.go | 109 ++++++++++++++++- hscontrol/policy/v2/types_test.go | 195 ++++++++++++++++++++++++++++++ 3 files changed, 333 insertions(+), 27 deletions(-) diff --git a/hscontrol/policy/policy_test.go b/hscontrol/policy/policy_test.go index c1000334..00c00f78 100644 --- a/hscontrol/policy/policy_test.go +++ b/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,14 +1927,17 @@ func TestSSHPolicyRules(t *testing.T) { targetNode: nodeUser1, peers: types.Nodes{&taggedClient}, policy: `{ - "ssh": [ - { - "action": "accept", - "src": ["tag:client"], - "dst": ["user1@"], - "users": ["alice", "bob"] - } - ] + "tagOwners": { + "tag:client": ["user1@"], + }, + "ssh": [ + { + "action": "accept", + "src": ["tag:client"], + "dst": ["user1@"], + "users": ["alice", "bob"] + } + ] }`, wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{ { diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 511e19bb..78b1fdbe 100644 --- a/hscontrol/policy/v2/types.go +++ b/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) diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index b428c55a..c25c14a9 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/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 {