From 132ceef75076642f2eb85a5714c2e0aea075be4f Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Fri, 2 Dec 2022 13:42:21 +0000 Subject: [PATCH 01/15] Compile regexp once --- lib/services/traits.go | 15 +++++++++++++-- lib/utils/replace.go | 20 ++++++++++++++------ 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/lib/services/traits.go b/lib/services/traits.go index 4205745d82038..47e887b7b1e6c 100644 --- a/lib/services/traits.go +++ b/lib/services/traits.go @@ -75,6 +75,17 @@ func TraitsToRoleMatchers(ms types.TraitMappingSet, traits map[string][]string) // traitsToRoles maps the supplied traits to teleport role names and passes them to a collector. func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect func(role string, expanded bool)) (warnings []string) { for _, mapping := range ms { + regexpIgnoreCase, err := utils.RegexpWithConfig(mapping.Value, utils.RegexpConfig{IgnoreCase: true}) + if err != nil { + // TODO: return warning + continue + } + regexp, err := utils.RegexpWithConfig(mapping.Value, utils.RegexpConfig{}) + if err != nil { + // TODO: return warning + continue + } + for traitName, traitValues := range traits { if traitName != mapping.Trait { continue @@ -85,7 +96,7 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect // Run the initial replacement case-insensitively. Doing so will filter out all literal non-matches // but will match on case discrepancies. We do another case-sensitive match below to see if the // case is different - outRole, err := utils.ReplaceRegexpWithConfig(mapping.Value, role, traitValue, utils.RegexpConfig{IgnoreCase: true}) + outRole, err := utils.ReplaceRegexpWith(regexpIgnoreCase, role, traitValue) switch { case err != nil: if trace.IsNotFound(err) { @@ -99,7 +110,7 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect // If there's no match, the trait specifies a mapping which is case-sensitive; // we should log a warning but return an error. // See https://github.com/gravitational/teleport/issues/6016 for details. - if _, err := utils.ReplaceRegexp(mapping.Value, role, traitValue); err != nil { + if _, err := utils.ReplaceRegexpWith(regexp, role, traitValue); err != nil { warnings = append(warnings, fmt.Sprintf("trait %q matches value %q case-insensitively and would have yielded %q role", traitValue, mapping.Value, outRole)) continue } diff --git a/lib/utils/replace.go b/lib/utils/replace.go index 4d22501f29eef..81dc540089b9f 100644 --- a/lib/utils/replace.go +++ b/lib/utils/replace.go @@ -35,19 +35,22 @@ func GlobToRegexp(in string) string { } // ReplaceRegexp replaces value in string, accepts regular expression and simplified -// wildcard syntax, it has several important differeneces with standard lib +// wildcard syntax, it has several important differences with standard lib // regexp replacer: // * Wildcard globs '*' are treated as regular expression .* expression // * Expression is treated as regular expression if it starts with ^ and ends with $ // * Full match is expected, partial replacements ignored // * If there is no match, returns a NotFound error func ReplaceRegexp(expression string, replaceWith string, input string) (string, error) { - return ReplaceRegexpWithConfig(expression, replaceWith, input, RegexpConfig{}) + expr, err := RegexpWithConfig(expression, RegexpConfig{}) + if err != nil { + return "", trace.Wrap(err) + } + return ReplaceRegexpWith(expr, replaceWith, input) } -// ReplaceRegexpWithConfig behaves exactly like ReplaceRegexp but its behavior -// can be customized -func ReplaceRegexpWithConfig(expression string, replaceWith string, input string, config RegexpConfig) (string, error) { +// RegexpWithConfig compiles a regular expression given some configuration. +func RegexpWithConfig(expression string, config RegexpConfig) (*regexp.Regexp, error) { if !strings.HasPrefix(expression, "^") || !strings.HasSuffix(expression, "$") { // replace glob-style wildcards with regexp wildcards // for plain strings, and quote all characters that could @@ -59,8 +62,13 @@ func ReplaceRegexpWithConfig(expression string, replaceWith string, input string } expr, err := regexp.Compile(expression) if err != nil { - return "", trace.BadParameter(err.Error()) + return nil, trace.BadParameter(err.Error()) } + return expr, nil +} + +// ReplaceRegexp replaces value in string given some regexp. +func ReplaceRegexpWith(expr *regexp.Regexp, replaceWith string, input string) (string, error) { // if there is no match, return NotFound error index := expr.FindAllStringIndex(input, -1) if len(index) == 0 { From 449f3522a764f029643399cc71f404f16ceb3f3f Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Fri, 2 Dec 2022 17:41:15 +0000 Subject: [PATCH 02/15] Use FindStringIndex instead of FindAllStringIndex --- lib/utils/replace.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/utils/replace.go b/lib/utils/replace.go index 81dc540089b9f..f4cc15a04d5d7 100644 --- a/lib/utils/replace.go +++ b/lib/utils/replace.go @@ -24,7 +24,7 @@ import ( // ContainsExpansion returns true if value contains // expansion syntax, e.g. $1 or ${10} func ContainsExpansion(val string) bool { - return reExpansion.FindAllStringIndex(val, -1) != nil + return reExpansion.FindStringIndex(val) != nil } // GlobToRegexp replaces glob-style standalone wildcard values @@ -70,8 +70,8 @@ func RegexpWithConfig(expression string, config RegexpConfig) (*regexp.Regexp, e // ReplaceRegexp replaces value in string given some regexp. func ReplaceRegexpWith(expr *regexp.Regexp, replaceWith string, input string) (string, error) { // if there is no match, return NotFound error - index := expr.FindAllStringIndex(input, -1) - if len(index) == 0 { + index := expr.FindStringIndex(input) + if index == nil { return "", trace.NotFound("no match found") } return expr.ReplaceAllString(input, replaceWith), nil From 12795562c8fffcf2a3ae09c1db7845a534c39528 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Mon, 5 Dec 2022 18:47:02 +0000 Subject: [PATCH 03/15] Replace log.Debug line with a single warning --- lib/services/traits.go | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/services/traits.go b/lib/services/traits.go index 47e887b7b1e6c..f0a8aabf34f0e 100644 --- a/lib/services/traits.go +++ b/lib/services/traits.go @@ -20,7 +20,6 @@ import ( "fmt" "github.com/gravitational/trace" - log "github.com/sirupsen/logrus" "github.com/gravitational/teleport/api/types" apiutils "github.com/gravitational/teleport/api/utils" @@ -28,6 +27,10 @@ import ( "github.com/gravitational/teleport/lib/utils/parse" ) +// maxMismatchedTraitValuesLogged indicates the maximum number of trait values that do not match a +// certain expression to be shown in a warning +const maxMismatchedTraitValuesWarned = 100 + // TraitsToRoles maps the supplied traits to a list of teleport role names. // Returns the list of roles mapped from traits. // `warnings` optionally contains the list of warnings potentially interesting to the user. @@ -77,12 +80,12 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect for _, mapping := range ms { regexpIgnoreCase, err := utils.RegexpWithConfig(mapping.Value, utils.RegexpConfig{IgnoreCase: true}) if err != nil { - // TODO: return warning + warnings = append(warnings, fmt.Sprintf("case-insensitive expression %q is not a valid regexp", mapping.Value)) continue } regexp, err := utils.RegexpWithConfig(mapping.Value, utils.RegexpConfig{}) if err != nil { - // TODO: return warning + warnings = append(warnings, fmt.Sprintf("case-sensitive expression %q is not a valid regexp", mapping.Value)) continue } @@ -90,6 +93,7 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect if traitName != mapping.Trait { continue } + var mismatched []string TraitLoop: for _, traitValue := range traitValues { for _, role := range mapping.Roles { @@ -99,10 +103,8 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect outRole, err := utils.ReplaceRegexpWith(regexpIgnoreCase, role, traitValue) switch { case err != nil: - if trace.IsNotFound(err) { - log.WithError(err).Debugf("Failed to match expression %q, replace with: %q input: %q.", mapping.Value, role, traitValue) - } // this trait value clearly did not match, move on to another + mismatched = append(mismatched, traitValue) continue TraitLoop case outRole == "": case outRole != "": @@ -119,6 +121,13 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect } } } + + // warn at most maxMismatchedTraitValuesWarned trait values to prevent huge warning lines + if len(mismatched) > maxMismatchedTraitValuesWarned { + warnings = append(warnings, fmt.Sprintf("%d trait values did not match expression %q: %s (first %d values)", len(mismatched), mapping.Value, mismatched[0:maxMismatchedTraitValuesWarned], maxMismatchedTraitValuesWarned)) + } else if len(mismatched) > 0 { + warnings = append(warnings, fmt.Sprintf("%d trait values did not match expression %q: %s", len(mismatched), mapping.Value, mismatched)) + } } } return warnings From d02466c7c89382dd00298202176364bcd8074d86 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Tue, 6 Dec 2022 10:36:57 +0000 Subject: [PATCH 04/15] Improve warning output --- lib/services/traits.go | 17 ++++++++++--- lib/services/user_test.go | 51 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/lib/services/traits.go b/lib/services/traits.go index f0a8aabf34f0e..3215d19ea0dd4 100644 --- a/lib/services/traits.go +++ b/lib/services/traits.go @@ -104,7 +104,7 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect switch { case err != nil: // this trait value clearly did not match, move on to another - mismatched = append(mismatched, traitValue) + mismatched = append(mismatched, fmt.Sprintf("%q", traitValue)) continue TraitLoop case outRole == "": case outRole != "": @@ -124,9 +124,20 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect // warn at most maxMismatchedTraitValuesWarned trait values to prevent huge warning lines if len(mismatched) > maxMismatchedTraitValuesWarned { - warnings = append(warnings, fmt.Sprintf("%d trait values did not match expression %q: %s (first %d values)", len(mismatched), mapping.Value, mismatched[0:maxMismatchedTraitValuesWarned], maxMismatchedTraitValuesWarned)) + warnings = append(warnings, fmt.Sprintf( + "%d trait value(s) did not match expression %q: [%s] (first %d values)", + len(mismatched), + mapping.Value, + mismatched[0:maxMismatchedTraitValuesWarned], + maxMismatchedTraitValuesWarned, + )) } else if len(mismatched) > 0 { - warnings = append(warnings, fmt.Sprintf("%d trait values did not match expression %q: %s", len(mismatched), mapping.Value, mismatched)) + warnings = append(warnings, fmt.Sprintf( + "%d trait value(s) did not match expression %q: %s", + len(mismatched), + mapping.Value, + mismatched, + )) } } } diff --git a/lib/services/user_test.go b/lib/services/user_test.go index fa40f9520dd19..1ea2bd686cd3f 100644 --- a/lib/services/user_test.go +++ b/lib/services/user_test.go @@ -109,21 +109,34 @@ func TestOIDCMapping(t *testing.T) { comment: "no value match", claims: jose.Claims{"role": "b"}, expectedRoles: nil, + warnings: []string{ + `1 trait value(s) did not match expression "admin": ["b"]`, + `1 trait value(s) did not match expression "user": ["b"]`, + }, }, { comment: "direct admin value match", claims: jose.Claims{"role": "admin"}, expectedRoles: []string{"admin", "bob"}, + warnings: []string{ + `1 trait value(s) did not match expression "user": ["admin"]`, + }, }, { comment: "direct user value match", claims: jose.Claims{"role": "user"}, expectedRoles: []string{"user"}, + warnings: []string{ + `1 trait value(s) did not match expression "admin": ["user"]`, + }, }, { comment: "direct user value match with array", claims: jose.Claims{"role": []string{"user"}}, expectedRoles: []string{"user"}, + warnings: []string{ + `1 trait value(s) did not match expression "admin": ["user"]`, + }, }, }, }, @@ -142,6 +155,9 @@ func TestOIDCMapping(t *testing.T) { comment: "no match - subprefix", claims: jose.Claims{"role": "adminz"}, expectedRoles: nil, + warnings: []string{ + `1 trait value(s) did not match expression "^admin-(.*)$": ["adminz"]`, + }, }, { comment: "value with capture match", @@ -157,6 +173,17 @@ func TestOIDCMapping(t *testing.T) { comment: "first matches, second does not", claims: jose.Claims{"role": []string{"hello", "admin-ola"}}, expectedRoles: []string{"role-ola", "bob"}, + warnings: []string{ + `1 trait value(s) did not match expression "^admin-(.*)$": ["hello"]`, + }, + }, + { + comment: "no match with array", + claims: jose.Claims{"role": []string{"foo", "bar", "baz"}}, + expectedRoles: nil, + warnings: []string{ + `3 trait value(s) did not match expression "^admin-(.*)$": ["foo" "bar" "baz"]`, + }, }, }, }, @@ -206,6 +233,12 @@ func TestOIDCMapping(t *testing.T) { "groups": []string{"DemoCorp - Backend Engineers", "DemoCorp Infrastructure"}, }, expectedRoles: []string{"backend", "approver"}, + warnings: []string{ + `1 trait value(s) did not match expression "DemoCorp - Backend Engineers": ["DemoCorp Infrastructure"]`, + `2 trait value(s) did not match expression "DemoCorp - SRE Managers": ["DemoCorp - Backend Engineers" "DemoCorp Infrastructure"]`, + `2 trait value(s) did not match expression "DemoCorp - SRE": ["DemoCorp - Backend Engineers" "DemoCorp Infrastructure"]`, + `1 trait value(s) did not match expression "DemoCorp Infrastructure": ["DemoCorp - Backend Engineers"]`, + }, }, { comment: "Matches one group", @@ -213,6 +246,12 @@ func TestOIDCMapping(t *testing.T) { "groups": []string{"DemoCorp - SRE"}, }, expectedRoles: []string{"approver"}, + warnings: []string{ + + `1 trait value(s) did not match expression "DemoCorp - Backend Engineers": ["DemoCorp - SRE"]`, + `1 trait value(s) did not match expression "DemoCorp - SRE Managers": ["DemoCorp - SRE"]`, + `1 trait value(s) did not match expression "DemoCorp Infrastructure": ["DemoCorp - SRE"]`, + }, }, { comment: "Matches one group with multiple roles", @@ -220,6 +259,11 @@ func TestOIDCMapping(t *testing.T) { "groups": []string{"DemoCorp Infrastructure"}, }, expectedRoles: []string{"approver", "backend"}, + warnings: []string{ + `1 trait value(s) did not match expression "DemoCorp - Backend Engineers": ["DemoCorp Infrastructure"]`, + `1 trait value(s) did not match expression "DemoCorp - SRE Managers": ["DemoCorp Infrastructure"]`, + `1 trait value(s) did not match expression "DemoCorp - SRE": ["DemoCorp Infrastructure"]`, + }, }, { comment: "No match only due to case-sensitivity", @@ -227,7 +271,12 @@ func TestOIDCMapping(t *testing.T) { "groups": []string{"Democorp - SRE"}, }, expectedRoles: []string(nil), - warnings: []string{`trait "Democorp - SRE" matches value "DemoCorp - SRE" case-insensitively and would have yielded "approver" role`}, + warnings: []string{ + `1 trait value(s) did not match expression "DemoCorp - Backend Engineers": ["Democorp - SRE"]`, + `1 trait value(s) did not match expression "DemoCorp - SRE Managers": ["Democorp - SRE"]`, + `trait "Democorp - SRE" matches value "DemoCorp - SRE" case-insensitively and would have yielded "approver" role`, + `1 trait value(s) did not match expression "DemoCorp Infrastructure": ["Democorp - SRE"]`, + }, }, }, }, From 511a52caaaaf741862f0b3cef3d977429f76dee4 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Tue, 6 Dec 2022 10:54:38 +0000 Subject: [PATCH 05/15] Add invalid regexp test case --- lib/services/user_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/services/user_test.go b/lib/services/user_test.go index 1ea2bd686cd3f..d9fe0bac1a31c 100644 --- a/lib/services/user_test.go +++ b/lib/services/user_test.go @@ -218,6 +218,22 @@ func TestOIDCMapping(t *testing.T) { }, }, }, + { + comment: "invalid regexp", + mappings: []types.ClaimMapping{ + {Claim: "role", Value: `^admin-(?!)$`, Roles: []string{"admin"}}, + }, + inputs: []input{ + { + comment: "invalid regexp", + claims: jose.Claims{}, + expectedRoles: nil, + warnings: []string{ + `case-insensitive expression "^admin-(?!)$" is not a valid regexp`, + }, + }, + }, + }, { comment: "Whitespace/dashes", mappings: []types.ClaimMapping{ From 5f38379a7b56327addcba5f9004a411c33ccc21f Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Tue, 6 Dec 2022 11:07:48 +0000 Subject: [PATCH 06/15] Add array trimming in warning test --- lib/services/suite/presence_test.go | 2 +- lib/services/traits.go | 2 +- lib/services/user_test.go | 10 +++++++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/services/suite/presence_test.go b/lib/services/suite/presence_test.go index f3396168ce7d1..1665a2659eb19 100644 --- a/lib/services/suite/presence_test.go +++ b/lib/services/suite/presence_test.go @@ -44,7 +44,7 @@ func TestServerLabels(t *testing.T) { }, Spec: types.ServerSpecV2{ CmdLabels: map[string]types.CommandLabelV2{ - "time": types.CommandLabelV2{ + "time": { Period: types.NewDuration(time.Second), Command: []string{"time"}, Result: "now", diff --git a/lib/services/traits.go b/lib/services/traits.go index 3215d19ea0dd4..3553502fdae83 100644 --- a/lib/services/traits.go +++ b/lib/services/traits.go @@ -125,7 +125,7 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect // warn at most maxMismatchedTraitValuesWarned trait values to prevent huge warning lines if len(mismatched) > maxMismatchedTraitValuesWarned { warnings = append(warnings, fmt.Sprintf( - "%d trait value(s) did not match expression %q: [%s] (first %d values)", + "%d trait value(s) did not match expression %q: %s (first %d values)", len(mismatched), mapping.Value, mismatched[0:maxMismatchedTraitValuesWarned], diff --git a/lib/services/user_test.go b/lib/services/user_test.go index d9fe0bac1a31c..c720078dcc50e 100644 --- a/lib/services/user_test.go +++ b/lib/services/user_test.go @@ -178,13 +178,21 @@ func TestOIDCMapping(t *testing.T) { }, }, { - comment: "no match with array", + comment: "no match with array in warning", claims: jose.Claims{"role": []string{"foo", "bar", "baz"}}, expectedRoles: nil, warnings: []string{ `3 trait value(s) did not match expression "^admin-(.*)$": ["foo" "bar" "baz"]`, }, }, + { + comment: "no match with big array trimmed in warning", + claims: jose.Claims{"role": []string{"a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a10", "a11", "a12", "a13", "a14", "a15", "a16", "a17", "a18", "a19", "a20", "a21", "a22", "a23", "a24", "a25", "a26", "a27", "a28", "a29", "a30", "a31", "a32", "a33", "a34", "a35", "a36", "a37", "a38", "a39", "a40", "a41", "a42", "a43", "a44", "a45", "a46", "a47", "a48", "a49", "a50", "a51", "a52", "a53", "a54", "a55", "a56", "a57", "a58", "a59", "a60", "a61", "a62", "a63", "a64", "a65", "a66", "a67", "a68", "a69", "a70", "a71", "a72", "a73", "a74", "a75", "a76", "a77", "a78", "a79", "a80", "a81", "a82", "a83", "a84", "a85", "a86", "a87", "a88", "a89", "a90", "a91", "a92", "a93", "a94", "a95", "a96", "a97", "a98", "a99", "a100", "a101"}}, + expectedRoles: nil, + warnings: []string{ + `101 trait value(s) did not match expression "^admin-(.*)$": ["a1" "a2" "a3" "a4" "a5" "a6" "a7" "a8" "a9" "a10" "a11" "a12" "a13" "a14" "a15" "a16" "a17" "a18" "a19" "a20" "a21" "a22" "a23" "a24" "a25" "a26" "a27" "a28" "a29" "a30" "a31" "a32" "a33" "a34" "a35" "a36" "a37" "a38" "a39" "a40" "a41" "a42" "a43" "a44" "a45" "a46" "a47" "a48" "a49" "a50" "a51" "a52" "a53" "a54" "a55" "a56" "a57" "a58" "a59" "a60" "a61" "a62" "a63" "a64" "a65" "a66" "a67" "a68" "a69" "a70" "a71" "a72" "a73" "a74" "a75" "a76" "a77" "a78" "a79" "a80" "a81" "a82" "a83" "a84" "a85" "a86" "a87" "a88" "a89" "a90" "a91" "a92" "a93" "a94" "a95" "a96" "a97" "a98" "a99" "a100"] (first 100 values)`, + }, + }, }, }, { From af4b809897bc6d1e80ee753ae64f97055dc5b01b Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Tue, 6 Dec 2022 11:15:35 +0000 Subject: [PATCH 07/15] Improve docs --- lib/services/traits.go | 4 ++-- lib/services/user_test.go | 1 - lib/utils/replace.go | 3 ++- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/services/traits.go b/lib/services/traits.go index 3553502fdae83..8df6450c5b5a4 100644 --- a/lib/services/traits.go +++ b/lib/services/traits.go @@ -27,8 +27,8 @@ import ( "github.com/gravitational/teleport/lib/utils/parse" ) -// maxMismatchedTraitValuesLogged indicates the maximum number of trait values that do not match a -// certain expression to be shown in a warning +// maxMismatchedTraitValuesLogged indicates the maximum number of trait values (that do not match a +// certain expression) to be shown in a warning const maxMismatchedTraitValuesWarned = 100 // TraitsToRoles maps the supplied traits to a list of teleport role names. diff --git a/lib/services/user_test.go b/lib/services/user_test.go index c720078dcc50e..99479b1d13a4e 100644 --- a/lib/services/user_test.go +++ b/lib/services/user_test.go @@ -271,7 +271,6 @@ func TestOIDCMapping(t *testing.T) { }, expectedRoles: []string{"approver"}, warnings: []string{ - `1 trait value(s) did not match expression "DemoCorp - Backend Engineers": ["DemoCorp - SRE"]`, `1 trait value(s) did not match expression "DemoCorp - SRE Managers": ["DemoCorp - SRE"]`, `1 trait value(s) did not match expression "DemoCorp Infrastructure": ["DemoCorp - SRE"]`, diff --git a/lib/utils/replace.go b/lib/utils/replace.go index f4cc15a04d5d7..47d6ac004645d 100644 --- a/lib/utils/replace.go +++ b/lib/utils/replace.go @@ -50,6 +50,7 @@ func ReplaceRegexp(expression string, replaceWith string, input string) (string, } // RegexpWithConfig compiles a regular expression given some configuration. +// There are several important differences with standard lib (see ReplaceRegexp). func RegexpWithConfig(expression string, config RegexpConfig) (*regexp.Regexp, error) { if !strings.HasPrefix(expression, "^") || !strings.HasSuffix(expression, "$") { // replace glob-style wildcards with regexp wildcards @@ -67,7 +68,7 @@ func RegexpWithConfig(expression string, config RegexpConfig) (*regexp.Regexp, e return expr, nil } -// ReplaceRegexp replaces value in string given some regexp. +// ReplaceRegexp replaces string in a given regexp. func ReplaceRegexpWith(expr *regexp.Regexp, replaceWith string, input string) (string, error) { // if there is no match, return NotFound error index := expr.FindStringIndex(input) From 8ec52b19d04d082643b1fe8ef4498d3285e92664 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Tue, 6 Dec 2022 11:29:40 +0000 Subject: [PATCH 08/15] Avoid compiling trait mapping values if no traits --- lib/services/traits.go | 8 +++++++- lib/services/user_test.go | 16 +++++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/lib/services/traits.go b/lib/services/traits.go index 8df6450c5b5a4..89cb35f03c9bb 100644 --- a/lib/services/traits.go +++ b/lib/services/traits.go @@ -77,7 +77,13 @@ func TraitsToRoleMatchers(ms types.TraitMappingSet, traits map[string][]string) // traitsToRoles maps the supplied traits to teleport role names and passes them to a collector. func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect func(role string, expanded bool)) (warnings []string) { + // if no traits, avoid compiling of trait mapping values as regular expressions + if len(traits) == 0 { + return + } + for _, mapping := range ms { + // compile each trait mapping value exactly twice regexpIgnoreCase, err := utils.RegexpWithConfig(mapping.Value, utils.RegexpConfig{IgnoreCase: true}) if err != nil { warnings = append(warnings, fmt.Sprintf("case-insensitive expression %q is not a valid regexp", mapping.Value)) @@ -141,7 +147,7 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect } } } - return warnings + return } // literalMatcher is used to "escape" values which are not allowed to diff --git a/lib/services/user_test.go b/lib/services/user_test.go index 99479b1d13a4e..efccedc29d7c2 100644 --- a/lib/services/user_test.go +++ b/lib/services/user_test.go @@ -93,6 +93,20 @@ func TestOIDCMapping(t *testing.T) { }, }, }, + { + comment: "no traits", + mappings: []types.ClaimMapping{ + {Claim: "role", Value: "admin", Roles: []string{"admin", "bob"}}, + {Claim: "role", Value: "user", Roles: []string{"user"}}, + }, + inputs: []input{ + { + comment: "no traits", + claims: jose.Claims{}, + expectedRoles: nil, + }, + }, + }, { comment: "simple mappings", mappings: []types.ClaimMapping{ @@ -234,7 +248,7 @@ func TestOIDCMapping(t *testing.T) { inputs: []input{ { comment: "invalid regexp", - claims: jose.Claims{}, + claims: jose.Claims{"role": ""}, expectedRoles: nil, warnings: []string{ `case-insensitive expression "^admin-(?!)$" is not a valid regexp`, From fde2482a61089c562e2fbd85ffcdd337cc8255ab Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Tue, 6 Dec 2022 11:55:36 +0000 Subject: [PATCH 09/15] Ensure regexps are compiled at-most once, and only if strictly needed --- lib/services/traits.go | 42 +++++++++++++++++++++++--------------- lib/services/user_test.go | 43 ++++++++++++++++++++++++--------------- 2 files changed, 53 insertions(+), 32 deletions(-) diff --git a/lib/services/traits.go b/lib/services/traits.go index 89cb35f03c9bb..7e8243a2cc139 100644 --- a/lib/services/traits.go +++ b/lib/services/traits.go @@ -18,6 +18,7 @@ package services import ( "fmt" + "regexp" "github.com/gravitational/trace" @@ -77,23 +78,10 @@ func TraitsToRoleMatchers(ms types.TraitMappingSet, traits map[string][]string) // traitsToRoles maps the supplied traits to teleport role names and passes them to a collector. func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect func(role string, expanded bool)) (warnings []string) { - // if no traits, avoid compiling of trait mapping values as regular expressions - if len(traits) == 0 { - return - } - +TraitMappingLoop: for _, mapping := range ms { - // compile each trait mapping value exactly twice - regexpIgnoreCase, err := utils.RegexpWithConfig(mapping.Value, utils.RegexpConfig{IgnoreCase: true}) - if err != nil { - warnings = append(warnings, fmt.Sprintf("case-insensitive expression %q is not a valid regexp", mapping.Value)) - continue - } - regexp, err := utils.RegexpWithConfig(mapping.Value, utils.RegexpConfig{}) - if err != nil { - warnings = append(warnings, fmt.Sprintf("case-sensitive expression %q is not a valid regexp", mapping.Value)) - continue - } + var regexpIgnoreCase *regexp.Regexp + var regexp *regexp.Regexp for traitName, traitValues := range traits { if traitName != mapping.Trait { @@ -103,6 +91,17 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect TraitLoop: for _, traitValue := range traitValues { for _, role := range mapping.Roles { + // this ensures that the case-insensitive regexp is compiled at most once, and only if strictly needed; + // after this if, regexpIgnoreCase must be non-nil + if regexpIgnoreCase == nil { + var err error + regexpIgnoreCase, err = utils.RegexpWithConfig(mapping.Value, utils.RegexpConfig{IgnoreCase: true}) + if err != nil { + warnings = append(warnings, fmt.Sprintf("case-insensitive expression %q is not a valid regexp", mapping.Value)) + continue TraitMappingLoop + } + } + // Run the initial replacement case-insensitively. Doing so will filter out all literal non-matches // but will match on case discrepancies. We do another case-sensitive match below to see if the // case is different @@ -114,6 +113,17 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect continue TraitLoop case outRole == "": case outRole != "": + // this ensures that the case-sensitive regexp is compiled at most once, and only if strictly needed; + // after this if, regexp must be non-nil + if regexp == nil { + var err error + regexp, err = utils.RegexpWithConfig(mapping.Value, utils.RegexpConfig{}) + if err != nil { + warnings = append(warnings, fmt.Sprintf("case-sensitive expression %q is not a valid regexp", mapping.Value)) + continue TraitMappingLoop + } + } + // Run the replacement case-sensitively to see if it matches. // If there's no match, the trait specifies a mapping which is case-sensitive; // we should log a warning but return an error. diff --git a/lib/services/user_test.go b/lib/services/user_test.go index efccedc29d7c2..b98527e2e82d8 100644 --- a/lib/services/user_test.go +++ b/lib/services/user_test.go @@ -209,6 +209,33 @@ func TestOIDCMapping(t *testing.T) { }, }, }, + { + comment: "regexp compilation", + mappings: []types.ClaimMapping{ + {Claim: "role", Value: `^admin-(?!)$`, Roles: []string{"admin"}}, + {Claim: "role", Value: "^admin-(.*)$", Roles: []string{"role-$1", "bob"}}, + {Claim: "role", Value: `^admin2-(?!)$`, Roles: []string{"admin2"}}, + }, + inputs: []input{ + { + comment: "invalid regexp", + claims: jose.Claims{"role": []string{"admin-hello", "dev"}}, + expectedRoles: []string{"role-hello", "bob"}, + warnings: []string{ + `case-insensitive expression "^admin-(?!)$" is not a valid regexp`, + `1 trait value(s) did not match expression "^admin-(.*)$": ["dev"]`, + `case-insensitive expression "^admin2-(?!)$" is not a valid regexp`, + }, + }, + { + comment: "regexp are not compiled if not needed", + claims: jose.Claims{}, + expectedRoles: nil, + // if the regexp were compiled, we would have the same warnings as above + warnings: nil, + }, + }, + }, { comment: "empty expands are skipped", mappings: []types.ClaimMapping{ @@ -240,22 +267,6 @@ func TestOIDCMapping(t *testing.T) { }, }, }, - { - comment: "invalid regexp", - mappings: []types.ClaimMapping{ - {Claim: "role", Value: `^admin-(?!)$`, Roles: []string{"admin"}}, - }, - inputs: []input{ - { - comment: "invalid regexp", - claims: jose.Claims{"role": ""}, - expectedRoles: nil, - warnings: []string{ - `case-insensitive expression "^admin-(?!)$" is not a valid regexp`, - }, - }, - }, - }, { comment: "Whitespace/dashes", mappings: []types.ClaimMapping{ From c677bebc878e33e187f2112398309969b3470154 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Tue, 6 Dec 2022 12:26:20 +0000 Subject: [PATCH 10/15] gofmt --- lib/services/user_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/services/user_test.go b/lib/services/user_test.go index b98527e2e82d8..bda966e4f85c5 100644 --- a/lib/services/user_test.go +++ b/lib/services/user_test.go @@ -232,7 +232,7 @@ func TestOIDCMapping(t *testing.T) { claims: jose.Claims{}, expectedRoles: nil, // if the regexp were compiled, we would have the same warnings as above - warnings: nil, + warnings: nil, }, }, }, From 8a231c74f59273dc2491eba5b6c4a3fe58099ab8 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Wed, 7 Dec 2022 09:04:03 +0000 Subject: [PATCH 11/15] Improve test readability. Co-authored-by: Alan Parra --- lib/services/user_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/services/user_test.go b/lib/services/user_test.go index bda966e4f85c5..4afc72e446118 100644 --- a/lib/services/user_test.go +++ b/lib/services/user_test.go @@ -212,9 +212,9 @@ func TestOIDCMapping(t *testing.T) { { comment: "regexp compilation", mappings: []types.ClaimMapping{ - {Claim: "role", Value: `^admin-(?!)$`, Roles: []string{"admin"}}, + {Claim: "role", Value: `^admin-(?!)$`, Roles: []string{"admin"}}, // "?!" is invalid. {Claim: "role", Value: "^admin-(.*)$", Roles: []string{"role-$1", "bob"}}, - {Claim: "role", Value: `^admin2-(?!)$`, Roles: []string{"admin2"}}, + {Claim: "role", Value: `^admin2-(?!)$`, Roles: []string{"admin2"}}, // "?!" is invalid. }, inputs: []input{ { From 15df5389a3dc66a3785349e20d4a1bb88ca3e22b Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Wed, 7 Dec 2022 09:15:16 +0000 Subject: [PATCH 12/15] log.Debug mismatch warnings --- lib/services/traits.go | 16 +++++---- lib/services/user_test.go | 69 --------------------------------------- 2 files changed, 9 insertions(+), 76 deletions(-) diff --git a/lib/services/traits.go b/lib/services/traits.go index 7e8243a2cc139..a8dfd33f14997 100644 --- a/lib/services/traits.go +++ b/lib/services/traits.go @@ -21,6 +21,7 @@ import ( "regexp" "github.com/gravitational/trace" + log "github.com/sirupsen/logrus" "github.com/gravitational/teleport/api/types" apiutils "github.com/gravitational/teleport/api/utils" @@ -138,22 +139,23 @@ TraitMappingLoop: } } - // warn at most maxMismatchedTraitValuesWarned trait values to prevent huge warning lines - if len(mismatched) > maxMismatchedTraitValuesWarned { - warnings = append(warnings, fmt.Sprintf( + // show at most maxMismatchedTraitValuesWarned trait values to prevent huge log lines + switch l := len(mismatched); { + case l > maxMismatchedTraitValuesWarned: + log.Debugf( "%d trait value(s) did not match expression %q: %s (first %d values)", len(mismatched), mapping.Value, mismatched[0:maxMismatchedTraitValuesWarned], maxMismatchedTraitValuesWarned, - )) - } else if len(mismatched) > 0 { - warnings = append(warnings, fmt.Sprintf( + ) + case l > 0: + log.Debugf( "%d trait value(s) did not match expression %q: %s", len(mismatched), mapping.Value, mismatched, - )) + ) } } } diff --git a/lib/services/user_test.go b/lib/services/user_test.go index 4afc72e446118..c68dda713a395 100644 --- a/lib/services/user_test.go +++ b/lib/services/user_test.go @@ -93,20 +93,6 @@ func TestOIDCMapping(t *testing.T) { }, }, }, - { - comment: "no traits", - mappings: []types.ClaimMapping{ - {Claim: "role", Value: "admin", Roles: []string{"admin", "bob"}}, - {Claim: "role", Value: "user", Roles: []string{"user"}}, - }, - inputs: []input{ - { - comment: "no traits", - claims: jose.Claims{}, - expectedRoles: nil, - }, - }, - }, { comment: "simple mappings", mappings: []types.ClaimMapping{ @@ -123,34 +109,21 @@ func TestOIDCMapping(t *testing.T) { comment: "no value match", claims: jose.Claims{"role": "b"}, expectedRoles: nil, - warnings: []string{ - `1 trait value(s) did not match expression "admin": ["b"]`, - `1 trait value(s) did not match expression "user": ["b"]`, - }, }, { comment: "direct admin value match", claims: jose.Claims{"role": "admin"}, expectedRoles: []string{"admin", "bob"}, - warnings: []string{ - `1 trait value(s) did not match expression "user": ["admin"]`, - }, }, { comment: "direct user value match", claims: jose.Claims{"role": "user"}, expectedRoles: []string{"user"}, - warnings: []string{ - `1 trait value(s) did not match expression "admin": ["user"]`, - }, }, { comment: "direct user value match with array", claims: jose.Claims{"role": []string{"user"}}, expectedRoles: []string{"user"}, - warnings: []string{ - `1 trait value(s) did not match expression "admin": ["user"]`, - }, }, }, }, @@ -169,9 +142,6 @@ func TestOIDCMapping(t *testing.T) { comment: "no match - subprefix", claims: jose.Claims{"role": "adminz"}, expectedRoles: nil, - warnings: []string{ - `1 trait value(s) did not match expression "^admin-(.*)$": ["adminz"]`, - }, }, { comment: "value with capture match", @@ -187,25 +157,6 @@ func TestOIDCMapping(t *testing.T) { comment: "first matches, second does not", claims: jose.Claims{"role": []string{"hello", "admin-ola"}}, expectedRoles: []string{"role-ola", "bob"}, - warnings: []string{ - `1 trait value(s) did not match expression "^admin-(.*)$": ["hello"]`, - }, - }, - { - comment: "no match with array in warning", - claims: jose.Claims{"role": []string{"foo", "bar", "baz"}}, - expectedRoles: nil, - warnings: []string{ - `3 trait value(s) did not match expression "^admin-(.*)$": ["foo" "bar" "baz"]`, - }, - }, - { - comment: "no match with big array trimmed in warning", - claims: jose.Claims{"role": []string{"a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a10", "a11", "a12", "a13", "a14", "a15", "a16", "a17", "a18", "a19", "a20", "a21", "a22", "a23", "a24", "a25", "a26", "a27", "a28", "a29", "a30", "a31", "a32", "a33", "a34", "a35", "a36", "a37", "a38", "a39", "a40", "a41", "a42", "a43", "a44", "a45", "a46", "a47", "a48", "a49", "a50", "a51", "a52", "a53", "a54", "a55", "a56", "a57", "a58", "a59", "a60", "a61", "a62", "a63", "a64", "a65", "a66", "a67", "a68", "a69", "a70", "a71", "a72", "a73", "a74", "a75", "a76", "a77", "a78", "a79", "a80", "a81", "a82", "a83", "a84", "a85", "a86", "a87", "a88", "a89", "a90", "a91", "a92", "a93", "a94", "a95", "a96", "a97", "a98", "a99", "a100", "a101"}}, - expectedRoles: nil, - warnings: []string{ - `101 trait value(s) did not match expression "^admin-(.*)$": ["a1" "a2" "a3" "a4" "a5" "a6" "a7" "a8" "a9" "a10" "a11" "a12" "a13" "a14" "a15" "a16" "a17" "a18" "a19" "a20" "a21" "a22" "a23" "a24" "a25" "a26" "a27" "a28" "a29" "a30" "a31" "a32" "a33" "a34" "a35" "a36" "a37" "a38" "a39" "a40" "a41" "a42" "a43" "a44" "a45" "a46" "a47" "a48" "a49" "a50" "a51" "a52" "a53" "a54" "a55" "a56" "a57" "a58" "a59" "a60" "a61" "a62" "a63" "a64" "a65" "a66" "a67" "a68" "a69" "a70" "a71" "a72" "a73" "a74" "a75" "a76" "a77" "a78" "a79" "a80" "a81" "a82" "a83" "a84" "a85" "a86" "a87" "a88" "a89" "a90" "a91" "a92" "a93" "a94" "a95" "a96" "a97" "a98" "a99" "a100"] (first 100 values)`, - }, }, }, }, @@ -223,7 +174,6 @@ func TestOIDCMapping(t *testing.T) { expectedRoles: []string{"role-hello", "bob"}, warnings: []string{ `case-insensitive expression "^admin-(?!)$" is not a valid regexp`, - `1 trait value(s) did not match expression "^admin-(.*)$": ["dev"]`, `case-insensitive expression "^admin2-(?!)$" is not a valid regexp`, }, }, @@ -282,12 +232,6 @@ func TestOIDCMapping(t *testing.T) { "groups": []string{"DemoCorp - Backend Engineers", "DemoCorp Infrastructure"}, }, expectedRoles: []string{"backend", "approver"}, - warnings: []string{ - `1 trait value(s) did not match expression "DemoCorp - Backend Engineers": ["DemoCorp Infrastructure"]`, - `2 trait value(s) did not match expression "DemoCorp - SRE Managers": ["DemoCorp - Backend Engineers" "DemoCorp Infrastructure"]`, - `2 trait value(s) did not match expression "DemoCorp - SRE": ["DemoCorp - Backend Engineers" "DemoCorp Infrastructure"]`, - `1 trait value(s) did not match expression "DemoCorp Infrastructure": ["DemoCorp - Backend Engineers"]`, - }, }, { comment: "Matches one group", @@ -295,11 +239,6 @@ func TestOIDCMapping(t *testing.T) { "groups": []string{"DemoCorp - SRE"}, }, expectedRoles: []string{"approver"}, - warnings: []string{ - `1 trait value(s) did not match expression "DemoCorp - Backend Engineers": ["DemoCorp - SRE"]`, - `1 trait value(s) did not match expression "DemoCorp - SRE Managers": ["DemoCorp - SRE"]`, - `1 trait value(s) did not match expression "DemoCorp Infrastructure": ["DemoCorp - SRE"]`, - }, }, { comment: "Matches one group with multiple roles", @@ -307,11 +246,6 @@ func TestOIDCMapping(t *testing.T) { "groups": []string{"DemoCorp Infrastructure"}, }, expectedRoles: []string{"approver", "backend"}, - warnings: []string{ - `1 trait value(s) did not match expression "DemoCorp - Backend Engineers": ["DemoCorp Infrastructure"]`, - `1 trait value(s) did not match expression "DemoCorp - SRE Managers": ["DemoCorp Infrastructure"]`, - `1 trait value(s) did not match expression "DemoCorp - SRE": ["DemoCorp Infrastructure"]`, - }, }, { comment: "No match only due to case-sensitivity", @@ -320,10 +254,7 @@ func TestOIDCMapping(t *testing.T) { }, expectedRoles: []string(nil), warnings: []string{ - `1 trait value(s) did not match expression "DemoCorp - Backend Engineers": ["Democorp - SRE"]`, - `1 trait value(s) did not match expression "DemoCorp - SRE Managers": ["Democorp - SRE"]`, `trait "Democorp - SRE" matches value "DemoCorp - SRE" case-insensitively and would have yielded "approver" role`, - `1 trait value(s) did not match expression "DemoCorp Infrastructure": ["Democorp - SRE"]`, }, }, }, From 5958806a45f0a0ae303fad9092642a07326103e2 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Wed, 7 Dec 2022 09:55:53 +0000 Subject: [PATCH 13/15] Use WithField to log mismatched trait values --- lib/services/traits.go | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/lib/services/traits.go b/lib/services/traits.go index a8dfd33f14997..bb99df85f88b9 100644 --- a/lib/services/traits.go +++ b/lib/services/traits.go @@ -30,8 +30,8 @@ import ( ) // maxMismatchedTraitValuesLogged indicates the maximum number of trait values (that do not match a -// certain expression) to be shown in a warning -const maxMismatchedTraitValuesWarned = 100 +// certain expression) to be shown in the log +const maxMismatchedTraitValuesLogged = 100 // TraitsToRoles maps the supplied traits to a list of teleport role names. // Returns the list of roles mapped from traits. @@ -110,7 +110,7 @@ TraitMappingLoop: switch { case err != nil: // this trait value clearly did not match, move on to another - mismatched = append(mismatched, fmt.Sprintf("%q", traitValue)) + mismatched = append(mismatched, traitValue) continue TraitLoop case outRole == "": case outRole != "": @@ -139,23 +139,16 @@ TraitMappingLoop: } } - // show at most maxMismatchedTraitValuesWarned trait values to prevent huge log lines + // show at most maxMismatchedTraitValuesLogged trait values to prevent huge log lines switch l := len(mismatched); { - case l > maxMismatchedTraitValuesWarned: - log.Debugf( - "%d trait value(s) did not match expression %q: %s (first %d values)", - len(mismatched), - mapping.Value, - mismatched[0:maxMismatchedTraitValuesWarned], - maxMismatchedTraitValuesWarned, - ) + case l > maxMismatchedTraitValuesLogged: + log.WithField("expression", mapping.Value). + WithField("values", mismatched[0:maxMismatchedTraitValuesLogged]). + Debugf("%d trait value(s) did not match (showing first %d values)", len(mismatched), maxMismatchedTraitValuesLogged) case l > 0: - log.Debugf( - "%d trait value(s) did not match expression %q: %s", - len(mismatched), - mapping.Value, - mismatched, - ) + log.WithField("expression", mapping.Value). + WithField("values", mismatched). + Debugf("%d trait value(s) did not match", len(mismatched)) } } } From ce86dc0777bdaaae4e8d28488e911f81173110de Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Wed, 7 Dec 2022 10:45:59 +0000 Subject: [PATCH 14/15] Turn TestOIDCMapping test cases into a benchmark --- lib/services/user_test.go | 384 +++++++++++++++++++++----------------- 1 file changed, 217 insertions(+), 167 deletions(-) diff --git a/lib/services/user_test.go b/lib/services/user_test.go index c68dda713a395..5aca82d50a6cd 100644 --- a/lib/services/user_test.go +++ b/lib/services/user_test.go @@ -70,198 +70,200 @@ func TestTraits(t *testing.T) { } } -func TestOIDCMapping(t *testing.T) { - t.Parallel() +type oidcInput struct { + comment string + claims jose.Claims + expectedRoles []string + warnings []string +} - type input struct { - comment string - claims jose.Claims - expectedRoles []string - warnings []string - } - testCases := []struct { - comment string - mappings []types.ClaimMapping - inputs []input - }{ - { - comment: "no mappings", - inputs: []input{ - { - claims: jose.Claims{"a": "b"}, - expectedRoles: nil, - }, +var oidcTestCases = []struct { + comment string + mappings []types.ClaimMapping + inputs []oidcInput +}{ + { + comment: "no mappings", + inputs: []oidcInput{ + { + comment: "no match", + claims: jose.Claims{"a": "b"}, + expectedRoles: nil, }, }, - { - comment: "simple mappings", - mappings: []types.ClaimMapping{ - {Claim: "role", Value: "admin", Roles: []string{"admin", "bob"}}, - {Claim: "role", Value: "user", Roles: []string{"user"}}, + }, + { + comment: "simple mappings", + mappings: []types.ClaimMapping{ + {Claim: "role", Value: "admin", Roles: []string{"admin", "bob"}}, + {Claim: "role", Value: "user", Roles: []string{"user"}}, + }, + inputs: []oidcInput{ + { + comment: "no match", + claims: jose.Claims{"a": "b"}, + expectedRoles: nil, }, - inputs: []input{ - { - comment: "no match", - claims: jose.Claims{"a": "b"}, - expectedRoles: nil, - }, - { - comment: "no value match", - claims: jose.Claims{"role": "b"}, - expectedRoles: nil, - }, - { - comment: "direct admin value match", - claims: jose.Claims{"role": "admin"}, - expectedRoles: []string{"admin", "bob"}, - }, - { - comment: "direct user value match", - claims: jose.Claims{"role": "user"}, - expectedRoles: []string{"user"}, - }, - { - comment: "direct user value match with array", - claims: jose.Claims{"role": []string{"user"}}, - expectedRoles: []string{"user"}, - }, + { + comment: "no value match", + claims: jose.Claims{"role": "b"}, + expectedRoles: nil, }, - }, - { - comment: "regexp mappings match", - mappings: []types.ClaimMapping{ - {Claim: "role", Value: "^admin-(.*)$", Roles: []string{"role-$1", "bob"}}, + { + comment: "direct admin value match", + claims: jose.Claims{"role": "admin"}, + expectedRoles: []string{"admin", "bob"}, }, - inputs: []input{ - { - comment: "no match", - claims: jose.Claims{"a": "b"}, - expectedRoles: nil, - }, - { - comment: "no match - subprefix", - claims: jose.Claims{"role": "adminz"}, - expectedRoles: nil, - }, - { - comment: "value with capture match", - claims: jose.Claims{"role": "admin-hello"}, - expectedRoles: []string{"role-hello", "bob"}, - }, - { - comment: "multiple value with capture match, deduplication", - claims: jose.Claims{"role": []string{"admin-hello", "admin-ola"}}, - expectedRoles: []string{"role-hello", "bob", "role-ola"}, - }, - { - comment: "first matches, second does not", - claims: jose.Claims{"role": []string{"hello", "admin-ola"}}, - expectedRoles: []string{"role-ola", "bob"}, - }, + { + comment: "direct user value match", + claims: jose.Claims{"role": "user"}, + expectedRoles: []string{"user"}, + }, + { + comment: "direct user value match with array", + claims: jose.Claims{"role": []string{"user"}}, + expectedRoles: []string{"user"}, }, }, - { - comment: "regexp compilation", - mappings: []types.ClaimMapping{ - {Claim: "role", Value: `^admin-(?!)$`, Roles: []string{"admin"}}, // "?!" is invalid. - {Claim: "role", Value: "^admin-(.*)$", Roles: []string{"role-$1", "bob"}}, - {Claim: "role", Value: `^admin2-(?!)$`, Roles: []string{"admin2"}}, // "?!" is invalid. + }, + { + comment: "regexp mappings match", + mappings: []types.ClaimMapping{ + {Claim: "role", Value: "^admin-(.*)$", Roles: []string{"role-$1", "bob"}}, + }, + inputs: []oidcInput{ + { + comment: "no match", + claims: jose.Claims{"a": "b"}, + expectedRoles: nil, }, - inputs: []input{ - { - comment: "invalid regexp", - claims: jose.Claims{"role": []string{"admin-hello", "dev"}}, - expectedRoles: []string{"role-hello", "bob"}, - warnings: []string{ - `case-insensitive expression "^admin-(?!)$" is not a valid regexp`, - `case-insensitive expression "^admin2-(?!)$" is not a valid regexp`, - }, - }, - { - comment: "regexp are not compiled if not needed", - claims: jose.Claims{}, - expectedRoles: nil, - // if the regexp were compiled, we would have the same warnings as above - warnings: nil, - }, + { + comment: "no match - subprefix", + claims: jose.Claims{"role": "adminz"}, + expectedRoles: nil, }, - }, - { - comment: "empty expands are skipped", - mappings: []types.ClaimMapping{ - {Claim: "role", Value: "^admin-(.*)$", Roles: []string{"$2", "bob"}}, + { + comment: "value with capture match", + claims: jose.Claims{"role": "admin-hello"}, + expectedRoles: []string{"role-hello", "bob"}, + }, + { + comment: "multiple value with capture match, deduplication", + claims: jose.Claims{"role": []string{"admin-hello", "admin-ola"}}, + expectedRoles: []string{"role-hello", "bob", "role-ola"}, }, - inputs: []input{ - { - comment: "value with capture match", - claims: jose.Claims{"role": "admin-hello"}, - expectedRoles: []string{"bob"}, + { + comment: "first matches, second does not", + claims: jose.Claims{"role": []string{"hello", "admin-ola"}}, + expectedRoles: []string{"role-ola", "bob"}, + }, + }, + }, + { + comment: "regexp compilation", + mappings: []types.ClaimMapping{ + {Claim: "role", Value: `^admin-(?!)$`, Roles: []string{"admin"}}, // "?!" is invalid. + {Claim: "role", Value: "^admin-(.*)$", Roles: []string{"role-$1", "bob"}}, + {Claim: "role", Value: `^admin2-(?!)$`, Roles: []string{"admin2"}}, // "?!" is invalid. + }, + inputs: []oidcInput{ + { + comment: "invalid regexp", + claims: jose.Claims{"role": []string{"admin-hello", "dev"}}, + expectedRoles: []string{"role-hello", "bob"}, + warnings: []string{ + `case-insensitive expression "^admin-(?!)$" is not a valid regexp`, + `case-insensitive expression "^admin2-(?!)$" is not a valid regexp`, }, }, + { + comment: "regexp are not compiled if not needed", + claims: jose.Claims{}, + expectedRoles: nil, + // if the regexp were compiled, we would have the same warnings as above + warnings: nil, + }, }, - { - comment: "glob wildcard match", - mappings: []types.ClaimMapping{ - {Claim: "role", Value: "*", Roles: []string{"admin"}}, + }, + { + comment: "empty expands are skipped", + mappings: []types.ClaimMapping{ + {Claim: "role", Value: "^admin-(.*)$", Roles: []string{"$2", "bob"}}, + }, + inputs: []oidcInput{ + { + comment: "value with capture match", + claims: jose.Claims{"role": "admin-hello"}, + expectedRoles: []string{"bob"}, }, - inputs: []input{ - { - comment: "empty value match", - claims: jose.Claims{"role": ""}, - expectedRoles: []string{"admin"}, - }, - { - comment: "any value match", - claims: jose.Claims{"role": "zz"}, - expectedRoles: []string{"admin"}, - }, + }, + }, + { + comment: "glob wildcard match", + mappings: []types.ClaimMapping{ + {Claim: "role", Value: "*", Roles: []string{"admin"}}, + }, + inputs: []oidcInput{ + { + comment: "empty value match", + claims: jose.Claims{"role": ""}, + expectedRoles: []string{"admin"}, + }, + { + comment: "any value match", + claims: jose.Claims{"role": "zz"}, + expectedRoles: []string{"admin"}, }, }, - { - comment: "Whitespace/dashes", - mappings: []types.ClaimMapping{ - {Claim: "groups", Value: "DemoCorp - Backend Engineers", Roles: []string{"backend"}}, - {Claim: "groups", Value: "DemoCorp - SRE Managers", Roles: []string{"approver"}}, - {Claim: "groups", Value: "DemoCorp - SRE", Roles: []string{"approver"}}, - {Claim: "groups", Value: "DemoCorp Infrastructure", Roles: []string{"approver", "backend"}}, + }, + { + comment: "Whitespace/dashes", + mappings: []types.ClaimMapping{ + {Claim: "groups", Value: "DemoCorp - Backend Engineers", Roles: []string{"backend"}}, + {Claim: "groups", Value: "DemoCorp - SRE Managers", Roles: []string{"approver"}}, + {Claim: "groups", Value: "DemoCorp - SRE", Roles: []string{"approver"}}, + {Claim: "groups", Value: "DemoCorp Infrastructure", Roles: []string{"approver", "backend"}}, + }, + inputs: []oidcInput{ + { + comment: "Matches multiple groups", + claims: jose.Claims{ + "groups": []string{"DemoCorp - Backend Engineers", "DemoCorp Infrastructure"}, + }, + expectedRoles: []string{"backend", "approver"}, }, - inputs: []input{ - { - comment: "Matches multiple groups", - claims: jose.Claims{ - "groups": []string{"DemoCorp - Backend Engineers", "DemoCorp Infrastructure"}, - }, - expectedRoles: []string{"backend", "approver"}, + { + comment: "Matches one group", + claims: jose.Claims{ + "groups": []string{"DemoCorp - SRE"}, }, - { - comment: "Matches one group", - claims: jose.Claims{ - "groups": []string{"DemoCorp - SRE"}, - }, - expectedRoles: []string{"approver"}, + expectedRoles: []string{"approver"}, + }, + { + comment: "Matches one group with multiple roles", + claims: jose.Claims{ + "groups": []string{"DemoCorp Infrastructure"}, }, - { - comment: "Matches one group with multiple roles", - claims: jose.Claims{ - "groups": []string{"DemoCorp Infrastructure"}, - }, - expectedRoles: []string{"approver", "backend"}, + expectedRoles: []string{"approver", "backend"}, + }, + { + comment: "No match only due to case-sensitivity", + claims: jose.Claims{ + "groups": []string{"Democorp - SRE"}, }, - { - comment: "No match only due to case-sensitivity", - claims: jose.Claims{ - "groups": []string{"Democorp - SRE"}, - }, - expectedRoles: []string(nil), - warnings: []string{ - `trait "Democorp - SRE" matches value "DemoCorp - SRE" case-insensitively and would have yielded "approver" role`, - }, + expectedRoles: []string(nil), + warnings: []string{ + `trait "Democorp - SRE" matches value "DemoCorp - SRE" case-insensitively and would have yielded "approver" role`, }, }, }, - } + }, +} + +func TestOIDCMapping(t *testing.T) { + t.Parallel() - for i, testCase := range testCases { + for i, testCase := range oidcTestCases { conn := types.OIDCConnectorV3{ Spec: types.OIDCConnectorSpecV3{ ClaimsToRoles: testCase.mappings, @@ -287,6 +289,54 @@ func TestOIDCMapping(t *testing.T) { } } + +/* +goos: darwin +goarch: arm64 + +❯ go test -bench ^BenchmarkTraitToRoles$ github.com/gravitational/teleport/lib/services | grep BenchmarkTraitToRoles | awk '{printf "%-100s%-15s %7s %s\n", $1, $2, $3, $4}' +BenchmarkTraitToRoles/no_mappings_no_match-10 374511232 3.049 ns/op +BenchmarkTraitToRoles/simple_mappings_no_match-10 18636192 64.57 ns/op +BenchmarkTraitToRoles/simple_mappings_no_value_match-10 97311 12264 ns/op +BenchmarkTraitToRoles/simple_mappings_direct_admin_value_match-10 92966 12802 ns/op +BenchmarkTraitToRoles/simple_mappings_direct_user_value_match-10 100602 11805 ns/op +BenchmarkTraitToRoles/simple_mappings_direct_user_value_match_with_array-10 100832 11814 ns/op +BenchmarkTraitToRoles/regexp_mappings_match_no_match-10 36021477 33.60 ns/op +BenchmarkTraitToRoles/regexp_mappings_match_no_match_-_subprefix-10 166546 7118 ns/op +BenchmarkTraitToRoles/regexp_mappings_match_value_with_capture_match-10 134823 8808 ns/op +BenchmarkTraitToRoles/regexp_mappings_match_multiple_value_with_capture_match,_deduplication-10 115350 10400 ns/op +BenchmarkTraitToRoles/regexp_mappings_match_first_matches,_second_does_not-10 98511 12234 ns/op +BenchmarkTraitToRoles/regexp_compilation_invalid_regexp-10 58684 20419 ns/op +BenchmarkTraitToRoles/regexp_compilation_regexp_are_not_compiled_if_not_needed-10 42062830 28.59 ns/op +BenchmarkTraitToRoles/empty_expands_are_skipped_value_with_capture_match-10 148668 8227 ns/op +BenchmarkTraitToRoles/glob_wildcard_match_empty_value_match-10 230226 5171 ns/op +BenchmarkTraitToRoles/glob_wildcard_match_any_value_match-10 222951 5325 ns/op +BenchmarkTraitToRoles/Whitespace/dashes_Matches_multiple_groups-10 19573 61473 ns/op +BenchmarkTraitToRoles/Whitespace/dashes_Matches_one_group-10 28248 42368 ns/op +BenchmarkTraitToRoles/Whitespace/dashes_Matches_one_group_with_multiple_roles-10 27547 44463 ns/op +BenchmarkTraitToRoles/Whitespace/dashes_No_match_only_due_to_case-sensitivity-10 26466 44907 ns/op +*/ +func BenchmarkTraitToRoles(b *testing.B) { + for _, testCase := range oidcTestCases { + samlConn := types.SAMLConnectorV2{ + Spec: types.SAMLConnectorSpecV2{ + AttributesToRoles: claimMappingsToAttributeMappings(testCase.mappings), + }, + } + mappings := samlConn.GetTraitMappings() + for _, input := range testCase.inputs { + testCaseInputName := fmt.Sprintf("%s %s", testCase.comment, input.comment) + traits := SAMLAssertionsToTraits(claimsToAttributes(input.claims)) + + b.Run(testCaseInputName, func(b *testing.B) { + for i := 0; i < b.N; i++ { + TraitsToRoles(mappings, traits) + } + }) + } + } +} + // claimMappingsToAttributeMappings converts oidc claim mappings to // attribute mappings, used in tests func claimMappingsToAttributeMappings(in []types.ClaimMapping) []types.AttributeMapping { From 7eaa6ea9df5f4a18b2c55f85c85e5e19d9628d99 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Wed, 7 Dec 2022 15:38:07 +0000 Subject: [PATCH 15/15] Remove bench results --- lib/services/user_test.go | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/lib/services/user_test.go b/lib/services/user_test.go index 5aca82d50a6cd..e302097b41dd7 100644 --- a/lib/services/user_test.go +++ b/lib/services/user_test.go @@ -288,34 +288,6 @@ func TestOIDCMapping(t *testing.T) { } } } - - -/* -goos: darwin -goarch: arm64 - -❯ go test -bench ^BenchmarkTraitToRoles$ github.com/gravitational/teleport/lib/services | grep BenchmarkTraitToRoles | awk '{printf "%-100s%-15s %7s %s\n", $1, $2, $3, $4}' -BenchmarkTraitToRoles/no_mappings_no_match-10 374511232 3.049 ns/op -BenchmarkTraitToRoles/simple_mappings_no_match-10 18636192 64.57 ns/op -BenchmarkTraitToRoles/simple_mappings_no_value_match-10 97311 12264 ns/op -BenchmarkTraitToRoles/simple_mappings_direct_admin_value_match-10 92966 12802 ns/op -BenchmarkTraitToRoles/simple_mappings_direct_user_value_match-10 100602 11805 ns/op -BenchmarkTraitToRoles/simple_mappings_direct_user_value_match_with_array-10 100832 11814 ns/op -BenchmarkTraitToRoles/regexp_mappings_match_no_match-10 36021477 33.60 ns/op -BenchmarkTraitToRoles/regexp_mappings_match_no_match_-_subprefix-10 166546 7118 ns/op -BenchmarkTraitToRoles/regexp_mappings_match_value_with_capture_match-10 134823 8808 ns/op -BenchmarkTraitToRoles/regexp_mappings_match_multiple_value_with_capture_match,_deduplication-10 115350 10400 ns/op -BenchmarkTraitToRoles/regexp_mappings_match_first_matches,_second_does_not-10 98511 12234 ns/op -BenchmarkTraitToRoles/regexp_compilation_invalid_regexp-10 58684 20419 ns/op -BenchmarkTraitToRoles/regexp_compilation_regexp_are_not_compiled_if_not_needed-10 42062830 28.59 ns/op -BenchmarkTraitToRoles/empty_expands_are_skipped_value_with_capture_match-10 148668 8227 ns/op -BenchmarkTraitToRoles/glob_wildcard_match_empty_value_match-10 230226 5171 ns/op -BenchmarkTraitToRoles/glob_wildcard_match_any_value_match-10 222951 5325 ns/op -BenchmarkTraitToRoles/Whitespace/dashes_Matches_multiple_groups-10 19573 61473 ns/op -BenchmarkTraitToRoles/Whitespace/dashes_Matches_one_group-10 28248 42368 ns/op -BenchmarkTraitToRoles/Whitespace/dashes_Matches_one_group_with_multiple_roles-10 27547 44463 ns/op -BenchmarkTraitToRoles/Whitespace/dashes_No_match_only_due_to_case-sensitivity-10 26466 44907 ns/op -*/ func BenchmarkTraitToRoles(b *testing.B) { for _, testCase := range oidcTestCases { samlConn := types.SAMLConnectorV2{