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 4205745d82038..bb99df85f88b9 100644 --- a/lib/services/traits.go +++ b/lib/services/traits.go @@ -18,6 +18,7 @@ package services import ( "fmt" + "regexp" "github.com/gravitational/trace" log "github.com/sirupsen/logrus" @@ -28,6 +29,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 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. // `warnings` optionally contains the list of warnings potentially interesting to the user. @@ -74,32 +79,57 @@ 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) { +TraitMappingLoop: for _, mapping := range ms { + var regexpIgnoreCase *regexp.Regexp + var regexp *regexp.Regexp + for traitName, traitValues := range traits { if traitName != mapping.Trait { continue } + var mismatched []string 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 - 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) { - 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 != "": + // 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. // 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 } @@ -108,9 +138,21 @@ func traitsToRoles(ms types.TraitMappingSet, traits map[string][]string, collect } } } + + // show at most maxMismatchedTraitValuesLogged trait values to prevent huge log lines + switch l := len(mismatched); { + 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.WithField("expression", mapping.Value). + WithField("values", mismatched). + Debugf("%d trait value(s) did not match", len(mismatched)) + } } } - 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 fa40f9520dd19..e302097b41dd7 100644 --- a/lib/services/user_test.go +++ b/lib/services/user_test.go @@ -70,170 +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: "empty expands are skipped", - mappings: []types.ClaimMapping{ - {Claim: "role", Value: "^admin-(.*)$", Roles: []string{"$2", "bob"}}, + }, + { + 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, + }, + { + 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"}, }, - inputs: []input{ - { - comment: "value with capture match", - claims: jose.Claims{"role": "admin-hello"}, - expectedRoles: []string{"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: "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, @@ -258,6 +288,26 @@ func TestOIDCMapping(t *testing.T) { } } } +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 diff --git a/lib/utils/replace.go b/lib/utils/replace.go index 4d22501f29eef..47d6ac004645d 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 @@ -35,19 +35,23 @@ 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. +// 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 // for plain strings, and quote all characters that could @@ -59,11 +63,16 @@ 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 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.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