From e925f81fb61b7620d2e375b72991b6dfd2e20a6e Mon Sep 17 00:00:00 2001 From: Alex McGrath Date: Wed, 29 Mar 2023 13:01:24 +0100 Subject: [PATCH 1/7] Order sudoers lines by role name --- lib/services/role.go | 25 ++++++++++----- lib/services/role_test.go | 65 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 78 insertions(+), 12 deletions(-) diff --git a/lib/services/role.go b/lib/services/role.go index 45e3560c3b63a..2690cf7c4becf 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -2397,9 +2397,19 @@ func (set RoleSet) EnhancedRecordingSet() map[string]bool { // a role disallows host user creation func (set RoleSet) HostUsers(s types.Server) (*HostUsersInfo, error) { groups := make(map[string]struct{}) - sudoers := make(map[string]struct{}) + var sudoers []string serverLabels := s.GetAllLabels() - for _, role := range set { + + roleSet := make([]types.Role, len(set)) + copy(roleSet, set) + slices.SortStableFunc(roleSet, func(a types.Role, b types.Role) bool { + if cmp := strings.Compare(a.GetName(), b.GetName()); cmp != -1 { + return false + } + return true + }) + + for _, role := range roleSet { result, _, err := MatchLabels(role.GetNodeLabels(types.Allow), serverLabels) if err != nil { return nil, trace.Wrap(err) @@ -2417,11 +2427,9 @@ func (set RoleSet) HostUsers(s types.Server) (*HostUsersInfo, error) { for _, group := range role.GetHostGroups(types.Allow) { groups[group] = struct{}{} } - for _, sudoer := range role.GetHostSudoers(types.Allow) { - sudoers[sudoer] = struct{}{} - } + sudoers = append(sudoers, role.GetHostSudoers(types.Allow)...) } - for _, role := range set { + for _, role := range roleSet { result, _, err := MatchLabels(role.GetNodeLabels(types.Deny), serverLabels) if err != nil { return nil, trace.Wrap(err) @@ -2437,13 +2445,14 @@ func (set RoleSet) HostUsers(s types.Server) (*HostUsersInfo, error) { sudoers = nil break } - delete(sudoers, sudoer) + sudoerIndex := slices.Index(sudoers, sudoer) + sudoers = slices.Delete(sudoers, sudoerIndex, sudoerIndex+1) } } return &HostUsersInfo{ Groups: utils.StringsSliceFromSet(groups), - Sudoers: utils.StringsSliceFromSet(sudoers), + Sudoers: sudoers, }, nil } diff --git a/lib/services/role_test.go b/lib/services/role_test.go index aaacdc4a64413..29aa467efbe8b 100644 --- a/lib/services/role_test.go +++ b/lib/services/role_test.go @@ -4735,7 +4735,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { server types.Server }{ { - test: "test exact match, one sudoer entry, one role", + test: "test exact match, one sudoer entry, one role", sudoers: []string{"%sudo ALL=(ALL) ALL"}, roles: NewRoleSet(&types.RoleV5{ Spec: types.RoleSpecV5{ @@ -4743,7 +4743,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { CreateHostUser: types.NewBoolOption(true), }, Allow: types.RoleConditions{ - NodeLabels: types.Labels{"success": []string{"abc"}}, + NodeLabels: types.Labels{"success": []string{"abc"}}, HostSudoers: []string{"%sudo ALL=(ALL) ALL"}, }, }, @@ -4760,6 +4760,9 @@ func TestHostUsers_HostSudoers(t *testing.T) { test: "multiple roles, one not matching", sudoers: []string{"sudoers entry 1", "sudoers entry 2"}, roles: NewRoleSet(&types.RoleV5{ + Metadata: types.Metadata{ + Name: "a", + }, Spec: types.RoleSpecV5{ Options: types.RoleOptions{ CreateHostUser: types.NewBoolOption(true), @@ -4770,6 +4773,9 @@ func TestHostUsers_HostSudoers(t *testing.T) { }, }, }, &types.RoleV5{ + Metadata: types.Metadata{ + Name: "b", + }, Spec: types.RoleSpecV5{ Options: types.RoleOptions{ CreateHostUser: types.NewBoolOption(true), @@ -4807,7 +4813,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { CreateHostUser: types.NewBoolOption(true), }, Allow: types.RoleConditions{ - NodeLabels: types.Labels{"success": []string{"abc"}}, + NodeLabels: types.Labels{"success": []string{"abc"}}, HostSudoers: []string{"%sudo ALL=(ALL) ALL"}, }, }, @@ -4831,7 +4837,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { }, }, { - test: "line deny", + test: "line deny", sudoers: []string{"%sudo ALL=(ALL) ALL"}, roles: NewRoleSet(&types.RoleV5{ Spec: types.RoleSpecV5{ @@ -4864,6 +4870,57 @@ func TestHostUsers_HostSudoers(t *testing.T) { }, }, }, + { + test: "multiple roles, order preserved by role name", + sudoers: []string{"sudoers entry 1", "sudoers entry 2", "sudoers entry 3", "sudoers entry 4"}, + roles: NewRoleSet(&types.RoleV5{ + Metadata: types.Metadata{ + Name: "a", + }, + Spec: types.RoleSpecV5{ + Options: types.RoleOptions{ + CreateHostUser: types.NewBoolOption(true), + }, + Allow: types.RoleConditions{ + NodeLabels: types.Labels{"success": []string{"abc"}}, + HostSudoers: []string{"sudoers entry 1"}, + }, + }, + }, &types.RoleV5{ + Metadata: types.Metadata{ + Name: "c", + }, + Spec: types.RoleSpecV5{ + Options: types.RoleOptions{ + CreateHostUser: types.NewBoolOption(true), + }, + Allow: types.RoleConditions{ + NodeLabels: types.Labels{types.Wildcard: []string{types.Wildcard}}, + HostSudoers: []string{"sudoers entry 4"}, + }, + }, + }, &types.RoleV5{ + Metadata: types.Metadata{ + Name: "b", + }, + Spec: types.RoleSpecV5{ + Options: types.RoleOptions{ + CreateHostUser: types.NewBoolOption(true), + }, + Allow: types.RoleConditions{ + NodeLabels: types.Labels{types.Wildcard: []string{types.Wildcard}}, + HostSudoers: []string{"sudoers entry 2", "sudoers entry 3"}, + }, + }, + }), + server: &types.ServerV2{ + Metadata: types.Metadata{ + Labels: map[string]string{ + "success": "abc", + }, + }, + }, + }, } { t.Run(tc.test, func(t *testing.T) { From e79fc668a98e2243e658ef78da76429f014f88e1 Mon Sep 17 00:00:00 2001 From: Alex McGrath Date: Mon, 3 Apr 2023 11:27:55 +0100 Subject: [PATCH 2/7] Add a warning in the docs to explain order sudoers roles are written as --- docs/pages/server-access/guides/host-user-creation.mdx | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/pages/server-access/guides/host-user-creation.mdx b/docs/pages/server-access/guides/host-user-creation.mdx index e31512b81dfd7..18795b514bfab 100644 --- a/docs/pages/server-access/guides/host-user-creation.mdx +++ b/docs/pages/server-access/guides/host-user-creation.mdx @@ -86,6 +86,13 @@ will be disabled. Roles that do not match the Node will not be checked. + + +When multiple roles contain `host_sudoers` entries, the sudoers file +will have the entries written to it ordered by role name + + + If a role includes a `deny` rule that sets `host_sudoers` to `'*'`, the user will have all sudoers entries removed when accessing matching Nodes, otherwise `deny` rules are matched literally when filtering: From 914c23031587de8dc7063a9efe5836493b2fdd71 Mon Sep 17 00:00:00 2001 From: Alex McGrath Date: Mon, 3 Apr 2023 12:57:20 +0100 Subject: [PATCH 3/7] Resolve comments --- lib/services/role.go | 24 ++++++++++-------- lib/services/role_test.go | 51 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 10 deletions(-) diff --git a/lib/services/role.go b/lib/services/role.go index 2690cf7c4becf..83e647dd983e5 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -2403,10 +2403,7 @@ func (set RoleSet) HostUsers(s types.Server) (*HostUsersInfo, error) { roleSet := make([]types.Role, len(set)) copy(roleSet, set) slices.SortStableFunc(roleSet, func(a types.Role, b types.Role) bool { - if cmp := strings.Compare(a.GetName(), b.GetName()); cmp != -1 { - return false - } - return true + return strings.Compare(a.GetName(), b.GetName()) == -1 }) for _, role := range roleSet { @@ -2440,14 +2437,21 @@ func (set RoleSet) HostUsers(s types.Server) (*HostUsersInfo, error) { for _, group := range role.GetHostGroups(types.Deny) { delete(groups, group) } - for _, sudoer := range role.GetHostSudoers(types.Deny) { - if sudoer == "*" { - sudoers = nil - break + + var finalSudoers []string + outer: + for _, sudoer := range sudoers { + for _, deniedSudoer := range role.GetHostSudoers(types.Deny) { + if deniedSudoer == "*" { + finalSudoers = nil + break outer + } + if sudoer != deniedSudoer { + finalSudoers = append(finalSudoers, sudoer) + } } - sudoerIndex := slices.Index(sudoers, sudoer) - sudoers = slices.Delete(sudoers, sudoerIndex, sudoerIndex+1) } + sudoers = finalSudoers } return &HostUsersInfo{ diff --git a/lib/services/role_test.go b/lib/services/role_test.go index 29aa467efbe8b..7650ab8a0acd2 100644 --- a/lib/services/role_test.go +++ b/lib/services/role_test.go @@ -4921,6 +4921,57 @@ func TestHostUsers_HostSudoers(t *testing.T) { }, }, }, + { + test: "duplication handled", + sudoers: []string{"sudoers entry 2"}, + roles: NewRoleSet(&types.RoleV6{ + Metadata: types.Metadata{ + Name: "a", + }, + Spec: types.RoleSpecV6{ + Options: types.RoleOptions{ + CreateHostUser: types.NewBoolOption(true), + }, + Allow: types.RoleConditions{ + NodeLabels: types.Labels{"success": []string{"abc"}}, + HostSudoers: []string{"sudoers entry 1"}, + }, + }, + }, &types.RoleV6{ // DENY sudoers entry 1 + Metadata: types.Metadata{ + Name: "d", + }, + Spec: types.RoleSpecV6{ + Options: types.RoleOptions{ + CreateHostUser: types.NewBoolOption(true), + }, + Deny: types.RoleConditions{ + NodeLabels: types.Labels{"success": []string{"abc"}}, + HostSudoers: []string{"sudoers entry 1"}, + }, + }, + }, &types.RoleV6{ // duplicate sudoers entry 1 case also gets removed + Metadata: types.Metadata{ + Name: "c", + }, + Spec: types.RoleSpecV6{ + Options: types.RoleOptions{ + CreateHostUser: types.NewBoolOption(true), + }, + Allow: types.RoleConditions{ + NodeLabels: types.Labels{types.Wildcard: []string{types.Wildcard}}, + HostSudoers: []string{"sudoers entry 1", "sudoers entry 2"}, + }, + }, + }), + server: &types.ServerV2{ + Metadata: types.Metadata{ + Labels: map[string]string{ + "success": "abc", + }, + }, + }, + }, } { t.Run(tc.test, func(t *testing.T) { From 8e1a78ebdd7730e4358a460baf5a4be1fcd89901 Mon Sep 17 00:00:00 2001 From: Alex McGrath Date: Mon, 3 Apr 2023 16:48:58 +0100 Subject: [PATCH 4/7] Resolve comments --- lib/services/role.go | 12 ++++++++++-- lib/services/role_test.go | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/services/role.go b/lib/services/role.go index 83e647dd983e5..9a215c0c602cc 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -2406,6 +2406,7 @@ func (set RoleSet) HostUsers(s types.Server) (*HostUsersInfo, error) { return strings.Compare(a.GetName(), b.GetName()) == -1 }) + seenSudoers := make(map[string]struct{}) for _, role := range roleSet { result, _, err := MatchLabels(role.GetNodeLabels(types.Allow), serverLabels) if err != nil { @@ -2424,8 +2425,16 @@ func (set RoleSet) HostUsers(s types.Server) (*HostUsersInfo, error) { for _, group := range role.GetHostGroups(types.Allow) { groups[group] = struct{}{} } - sudoers = append(sudoers, role.GetHostSudoers(types.Allow)...) + for _, sudoer := range role.GetHostSudoers(types.Allow) { + if _, ok := seenSudoers[sudoer]; ok { + continue + } + seenSudoers[sudoer] = struct{}{} + sudoers = append(sudoers, sudoer) + } } + + var finalSudoers []string for _, role := range roleSet { result, _, err := MatchLabels(role.GetNodeLabels(types.Deny), serverLabels) if err != nil { @@ -2438,7 +2447,6 @@ func (set RoleSet) HostUsers(s types.Server) (*HostUsersInfo, error) { delete(groups, group) } - var finalSudoers []string outer: for _, sudoer := range sudoers { for _, deniedSudoer := range role.GetHostSudoers(types.Deny) { diff --git a/lib/services/role_test.go b/lib/services/role_test.go index 7650ab8a0acd2..5cad7f5b4f49d 100644 --- a/lib/services/role_test.go +++ b/lib/services/role_test.go @@ -4896,7 +4896,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { }, Allow: types.RoleConditions{ NodeLabels: types.Labels{types.Wildcard: []string{types.Wildcard}}, - HostSudoers: []string{"sudoers entry 4"}, + HostSudoers: []string{"sudoers entry 4", "sudoers entry 1"}, }, }, }, &types.RoleV5{ From bcbd0cdcc2ea6b72421c385edc5e005b910aa97f Mon Sep 17 00:00:00 2001 From: Alex McGrath Date: Tue, 4 Apr 2023 12:39:17 +0100 Subject: [PATCH 5/7] fix lints --- lib/services/role_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/services/role_test.go b/lib/services/role_test.go index 5cad7f5b4f49d..9aa62d48d8304 100644 --- a/lib/services/role_test.go +++ b/lib/services/role_test.go @@ -4735,7 +4735,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { server types.Server }{ { - test: "test exact match, one sudoer entry, one role", + test: "test exact match, one sudoer entry, one role", sudoers: []string{"%sudo ALL=(ALL) ALL"}, roles: NewRoleSet(&types.RoleV5{ Spec: types.RoleSpecV5{ @@ -4743,7 +4743,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { CreateHostUser: types.NewBoolOption(true), }, Allow: types.RoleConditions{ - NodeLabels: types.Labels{"success": []string{"abc"}}, + NodeLabels: types.Labels{"success": []string{"abc"}}, HostSudoers: []string{"%sudo ALL=(ALL) ALL"}, }, }, @@ -4813,7 +4813,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { CreateHostUser: types.NewBoolOption(true), }, Allow: types.RoleConditions{ - NodeLabels: types.Labels{"success": []string{"abc"}}, + NodeLabels: types.Labels{"success": []string{"abc"}}, HostSudoers: []string{"%sudo ALL=(ALL) ALL"}, }, }, @@ -4837,7 +4837,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { }, }, { - test: "line deny", + test: "line deny", sudoers: []string{"%sudo ALL=(ALL) ALL"}, roles: NewRoleSet(&types.RoleV5{ Spec: types.RoleSpecV5{ From 357c46e84d952703ca8597da4102fe616239bea6 Mon Sep 17 00:00:00 2001 From: Alex McGrath Date: Wed, 19 Apr 2023 10:23:39 +0100 Subject: [PATCH 6/7] backport fixes --- lib/services/role.go | 4 ++-- lib/services/role_test.go | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/services/role.go b/lib/services/role.go index 9a215c0c602cc..2b67634c18446 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -2402,8 +2402,8 @@ func (set RoleSet) HostUsers(s types.Server) (*HostUsersInfo, error) { roleSet := make([]types.Role, len(set)) copy(roleSet, set) - slices.SortStableFunc(roleSet, func(a types.Role, b types.Role) bool { - return strings.Compare(a.GetName(), b.GetName()) == -1 + sort.SliceStable(roleSet, func(i, j int) bool { + return strings.Compare(roleSet[i].GetName(), roleSet[j].GetName()) == -1 }) seenSudoers := make(map[string]struct{}) diff --git a/lib/services/role_test.go b/lib/services/role_test.go index 9aa62d48d8304..6ccb0006f18e6 100644 --- a/lib/services/role_test.go +++ b/lib/services/role_test.go @@ -4735,7 +4735,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { server types.Server }{ { - test: "test exact match, one sudoer entry, one role", + test: "test exact match, one sudoer entry, one role", sudoers: []string{"%sudo ALL=(ALL) ALL"}, roles: NewRoleSet(&types.RoleV5{ Spec: types.RoleSpecV5{ @@ -4743,7 +4743,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { CreateHostUser: types.NewBoolOption(true), }, Allow: types.RoleConditions{ - NodeLabels: types.Labels{"success": []string{"abc"}}, + NodeLabels: types.Labels{"success": []string{"abc"}}, HostSudoers: []string{"%sudo ALL=(ALL) ALL"}, }, }, @@ -4813,7 +4813,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { CreateHostUser: types.NewBoolOption(true), }, Allow: types.RoleConditions{ - NodeLabels: types.Labels{"success": []string{"abc"}}, + NodeLabels: types.Labels{"success": []string{"abc"}}, HostSudoers: []string{"%sudo ALL=(ALL) ALL"}, }, }, @@ -4837,7 +4837,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { }, }, { - test: "line deny", + test: "line deny", sudoers: []string{"%sudo ALL=(ALL) ALL"}, roles: NewRoleSet(&types.RoleV5{ Spec: types.RoleSpecV5{ @@ -4924,11 +4924,11 @@ func TestHostUsers_HostSudoers(t *testing.T) { { test: "duplication handled", sudoers: []string{"sudoers entry 2"}, - roles: NewRoleSet(&types.RoleV6{ + roles: NewRoleSet(&types.RoleV5{ Metadata: types.Metadata{ Name: "a", }, - Spec: types.RoleSpecV6{ + Spec: types.RoleSpecV5{ Options: types.RoleOptions{ CreateHostUser: types.NewBoolOption(true), }, @@ -4937,11 +4937,11 @@ func TestHostUsers_HostSudoers(t *testing.T) { HostSudoers: []string{"sudoers entry 1"}, }, }, - }, &types.RoleV6{ // DENY sudoers entry 1 + }, &types.RoleV5{ // DENY sudoers entry 1 Metadata: types.Metadata{ Name: "d", }, - Spec: types.RoleSpecV6{ + Spec: types.RoleSpecV5{ Options: types.RoleOptions{ CreateHostUser: types.NewBoolOption(true), }, @@ -4950,11 +4950,11 @@ func TestHostUsers_HostSudoers(t *testing.T) { HostSudoers: []string{"sudoers entry 1"}, }, }, - }, &types.RoleV6{ // duplicate sudoers entry 1 case also gets removed + }, &types.RoleV5{ // duplicate sudoers entry 1 case also gets removed Metadata: types.Metadata{ Name: "c", }, - Spec: types.RoleSpecV6{ + Spec: types.RoleSpecV5{ Options: types.RoleOptions{ CreateHostUser: types.NewBoolOption(true), }, From 95e24fb05d547f530485f9ad54a82f48c99e987a Mon Sep 17 00:00:00 2001 From: Alex McGrath Date: Wed, 26 Apr 2023 11:36:47 +0100 Subject: [PATCH 7/7] lints --- lib/services/role_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/services/role_test.go b/lib/services/role_test.go index 6ccb0006f18e6..ea523f5e05187 100644 --- a/lib/services/role_test.go +++ b/lib/services/role_test.go @@ -4735,7 +4735,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { server types.Server }{ { - test: "test exact match, one sudoer entry, one role", + test: "test exact match, one sudoer entry, one role", sudoers: []string{"%sudo ALL=(ALL) ALL"}, roles: NewRoleSet(&types.RoleV5{ Spec: types.RoleSpecV5{ @@ -4743,7 +4743,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { CreateHostUser: types.NewBoolOption(true), }, Allow: types.RoleConditions{ - NodeLabels: types.Labels{"success": []string{"abc"}}, + NodeLabels: types.Labels{"success": []string{"abc"}}, HostSudoers: []string{"%sudo ALL=(ALL) ALL"}, }, }, @@ -4813,7 +4813,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { CreateHostUser: types.NewBoolOption(true), }, Allow: types.RoleConditions{ - NodeLabels: types.Labels{"success": []string{"abc"}}, + NodeLabels: types.Labels{"success": []string{"abc"}}, HostSudoers: []string{"%sudo ALL=(ALL) ALL"}, }, }, @@ -4837,7 +4837,7 @@ func TestHostUsers_HostSudoers(t *testing.T) { }, }, { - test: "line deny", + test: "line deny", sudoers: []string{"%sudo ALL=(ALL) ALL"}, roles: NewRoleSet(&types.RoleV5{ Spec: types.RoleSpecV5{