From c8b57c441a926f1f1576c9fd1b1859b920f9ec15 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 9 May 2025 09:30:50 +0200 Subject: [PATCH] policy/v2: validate that no undefined group or tag is used Fixes #2570 Signed-off-by: Kristoffer Dalby --- hscontrol/policy/v2/types.go | 107 ++++++++++++++++ hscontrol/policy/v2/types_test.go | 195 ++++++++++++++++++++++++++++++ 2 files changed, 302 insertions(+) diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index a00df94b..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) } } } 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 {