From 74d1d5ee0b2c173081a739142a34ff290b45cc78 Mon Sep 17 00:00:00 2001 From: ksuzuki Date: Thu, 6 Sep 2018 23:54:47 +0900 Subject: [PATCH 1/2] wrote a test and fix a problem --- lib/services/role.go | 32 +++++++++++++++++++++++--------- lib/services/role_test.go | 38 ++++++++++++++++++++++++++++++++++++++ lib/utils/replace.go | 21 +++++++++++++++++++++ 3 files changed, 82 insertions(+), 9 deletions(-) diff --git a/lib/services/role.go b/lib/services/role.go index 7e0a47af6e93e..35350cb477382 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -1395,30 +1395,38 @@ func MatchLogin(selectors []string, login string) (bool, string) { // MatchLabels matches selector against target. Empty selector matches // nothing, wildcard matches everything. -func MatchLabels(selector Labels, target map[string]string) (bool, string) { +func MatchLabels(selector Labels, target map[string]string) (bool, string, error) { // Empty selector matches nothing. if len(selector) == 0 { - return false, "no match, empty selector" + return false, "no match, empty selector", nil } + // matchLabels, labelsMessage := MatchLabels(role.GetNodeLabels(Deny), s.GetAllLabels()) // *: * matches everything even empty target set. selectorValues := selector[Wildcard] if len(selectorValues) == 1 && selectorValues[0] == Wildcard { - return true, "matched" + return true, "matched", nil } // Perform full match. for key, selectorValues := range selector { targetVal, hasKey := target[key] + if !hasKey { - return false, fmt.Sprintf("no key match: '%v'", key) + return false, fmt.Sprintf("no key match: '%v'", key), nil } - if !utils.SliceContainsStr(selectorValues, Wildcard) && !utils.SliceContainsStr(selectorValues, targetVal) { - return false, fmt.Sprintf("no value match: got '%v' want: '%v'", targetVal, selectorValues) + + if !utils.SliceContainsStr(selectorValues, Wildcard) { + result, err := utils.SliceContainsRegexStr(targetVal, selectorValues) + if err != nil { + return false, "", trace.BadParameter(err.Error()) + } else if !result { + return false, fmt.Sprintf("no value match: got '%v' want: '%v'", targetVal, selectorValues), nil + } } } - return true, "matched" + return true, "matched", nil } // RoleNames returns a slice with role names @@ -1558,7 +1566,10 @@ func (set RoleSet) CheckAccessToServer(login string, s Server) error { // the deny role set prohibits access. for _, role := range set { matchNamespace, namespaceMessage := MatchNamespace(role.GetNamespaces(Deny), s.GetNamespace()) - matchLabels, labelsMessage := MatchLabels(role.GetNodeLabels(Deny), s.GetAllLabels()) + matchLabels, labelsMessage, err := MatchLabels(role.GetNodeLabels(Deny), s.GetAllLabels()) + if err != nil { + return trace.CompareFailed(err.Error()) + } matchLogin, loginMessage := MatchLogin(role.GetLogins(Deny), login) if matchNamespace && (matchLabels || matchLogin) { if log.GetLevel() == log.DebugLevel { @@ -1575,7 +1586,10 @@ func (set RoleSet) CheckAccessToServer(login string, s Server) error { // one role in the role set to be granted access. for _, role := range set { matchNamespace, namespaceMessage := MatchNamespace(role.GetNamespaces(Allow), s.GetNamespace()) - matchLabels, labelsMessage := MatchLabels(role.GetNodeLabels(Allow), s.GetAllLabels()) + matchLabels, labelsMessage, err := MatchLabels(role.GetNodeLabels(Allow), s.GetAllLabels()) + if err != nil { + return trace.CompareFailed(err.Error()) + } matchLogin, loginMessage := MatchLogin(role.GetLogins(Allow), login) if matchNamespace && matchLabels && matchLogin { return nil diff --git a/lib/services/role_test.go b/lib/services/role_test.go index 0e3f3aedee1bb..28c9fd45da8e7 100644 --- a/lib/services/role_test.go +++ b/lib/services/role_test.go @@ -473,6 +473,13 @@ func (s *RoleSuite) TestCheckAccess(c *C) { Labels: map[string]string{"role": "db", "status": "follower"}, }, } + serverC2 := &ServerV2{ + Metadata: Metadata{ + Name: "c2", + Namespace: namespaceC, + Labels: map[string]string{"role": "db01", "status": "follower01"}, + }, + } testCases := []struct { name string roles []RoleV3 @@ -648,6 +655,37 @@ func (s *RoleSuite) TestCheckAccess(c *C) { {server: serverC, login: "admin", hasAccess: true}, }, }, + { + name: "one role needs to access servers sharing the partially same label value", + roles: []RoleV3{ + RoleV3{ + Metadata: Metadata{ + Name: "name1", + Namespace: namespaceC, + }, + Spec: RoleSpecV3{ + Options: RoleOptions{ + MaxSessionTTL: Duration{20 * time.Hour}, + }, + Allow: RoleConditions{ + Logins: []string{"admin"}, + NodeLabels: Labels{"role": []string{"^db(.*)$"}, "status": []string{"follow*"}}, + Namespaces: []string{namespaceC}, + }, + }, + }, + }, + checks: []check{ + {server: serverA, login: "root", hasAccess: false}, + {server: serverA, login: "admin", hasAccess: false}, + {server: serverB, login: "root", hasAccess: false}, + {server: serverB, login: "admin", hasAccess: false}, + {server: serverC, login: "root", hasAccess: false}, + {server: serverC, login: "admin", hasAccess: true}, + {server: serverC2, login: "root", hasAccess: false}, + {server: serverC2, login: "admin", hasAccess: true}, + }, + }, } for i, tc := range testCases { diff --git a/lib/utils/replace.go b/lib/utils/replace.go index 4d68ae7ed0f9d..75854b1a225a6 100644 --- a/lib/utils/replace.go +++ b/lib/utils/replace.go @@ -46,5 +46,26 @@ func ReplaceRegexp(expression string, replaceWith string, input string) (string, return expr.ReplaceAllString(input, replaceWith), nil } +func SliceContainsRegexStr(input string, expressions []string) (bool, error) { + for _, expression := range expressions { + if !strings.HasPrefix(expression, "^") || !strings.HasSuffix(expression, "$") { + // replace glob-style wildcards with regexp wildcards + // for plain strings, and quote all characters that could + // be interpreted in regular expression + expression = "^" + GlobToRegexp(expression) + "$" + } + + expr, err := regexp.Compile(expression) + if err != nil { + return false, trace.BadParameter(err.Error()) + } + if expr.MatchString(input) { + return true, nil + } + } + + return false, nil +} + var replaceWildcard = regexp.MustCompile(`(\\\*)`) var reExpansion = regexp.MustCompile(`\$[^\$]+`) From ed6d35a63a5c8c9d95ae7a56d99beadcaf754729 Mon Sep 17 00:00:00 2001 From: Russell Jones Date: Sat, 29 Sep 2018 00:09:29 +0000 Subject: [PATCH 2/2] Refactor regexp node labels. --- lib/services/role.go | 9 ++++----- lib/utils/replace.go | 8 +++++++- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/services/role.go b/lib/services/role.go index 35350cb477382..192bc563be258 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -1401,7 +1401,6 @@ func MatchLabels(selector Labels, target map[string]string) (bool, string, error return false, "no match, empty selector", nil } - // matchLabels, labelsMessage := MatchLabels(role.GetNodeLabels(Deny), s.GetAllLabels()) // *: * matches everything even empty target set. selectorValues := selector[Wildcard] if len(selectorValues) == 1 && selectorValues[0] == Wildcard { @@ -1417,9 +1416,9 @@ func MatchLabels(selector Labels, target map[string]string) (bool, string, error } if !utils.SliceContainsStr(selectorValues, Wildcard) { - result, err := utils.SliceContainsRegexStr(targetVal, selectorValues) + result, err := utils.SliceMatchesRegex(targetVal, selectorValues) if err != nil { - return false, "", trace.BadParameter(err.Error()) + return false, "", trace.Wrap(err) } else if !result { return false, fmt.Sprintf("no value match: got '%v' want: '%v'", targetVal, selectorValues), nil } @@ -1568,7 +1567,7 @@ func (set RoleSet) CheckAccessToServer(login string, s Server) error { matchNamespace, namespaceMessage := MatchNamespace(role.GetNamespaces(Deny), s.GetNamespace()) matchLabels, labelsMessage, err := MatchLabels(role.GetNodeLabels(Deny), s.GetAllLabels()) if err != nil { - return trace.CompareFailed(err.Error()) + return trace.Wrap(err) } matchLogin, loginMessage := MatchLogin(role.GetLogins(Deny), login) if matchNamespace && (matchLabels || matchLogin) { @@ -1588,7 +1587,7 @@ func (set RoleSet) CheckAccessToServer(login string, s Server) error { matchNamespace, namespaceMessage := MatchNamespace(role.GetNamespaces(Allow), s.GetNamespace()) matchLabels, labelsMessage, err := MatchLabels(role.GetNodeLabels(Allow), s.GetAllLabels()) if err != nil { - return trace.CompareFailed(err.Error()) + return trace.Wrap(err) } matchLogin, loginMessage := MatchLogin(role.GetLogins(Allow), login) if matchNamespace && matchLabels && matchLogin { diff --git a/lib/utils/replace.go b/lib/utils/replace.go index 75854b1a225a6..a32c83e3b8c84 100644 --- a/lib/utils/replace.go +++ b/lib/utils/replace.go @@ -46,7 +46,9 @@ func ReplaceRegexp(expression string, replaceWith string, input string) (string, return expr.ReplaceAllString(input, replaceWith), nil } -func SliceContainsRegexStr(input string, expressions []string) (bool, error) { +// SliceMatchesRegex checks if input matches any of the expressions. The +// match is always evaluated as a regex either an exact match or regexp. +func SliceMatchesRegex(input string, expressions []string) (bool, error) { for _, expression := range expressions { if !strings.HasPrefix(expression, "^") || !strings.HasSuffix(expression, "$") { // replace glob-style wildcards with regexp wildcards @@ -59,6 +61,10 @@ func SliceContainsRegexStr(input string, expressions []string) (bool, error) { if err != nil { return false, trace.BadParameter(err.Error()) } + + // Since the expression is always surrounded by ^ and $ this is an exact + // match for either a a plain string (for example ^hello$) or for a regexp + // (for example ^hel*o$). if expr.MatchString(input) { return true, nil }