Add negative RBAC label matches #4253

awly · 2020-08-24T22:21:35Z

Normal node_labels matches are positive only - you must provide an
exact label or a regexp that matches all valid labels.

This is problematic when you want to grant access to "any label value
except X". You could put "X" in the deny section of a role, but this
prevents any other roles from granting access to "X". For example, users
may have many dynamically-named environments and one "prod", and they
want to only grant access to "prod" via workflow API.

This PR adds support for {{regexp.not_match(...)}} function (along with {{regexp.match(...)}}, for example {{regexp.not_match("prod")}}.

Fixes #3454

docs/4.3/enterprise/ssh_rbac.md

benarent · 2020-08-26T22:50:15Z

lib/utils/replace.go

@@ -48,8 +48,19 @@ func ReplaceRegexp(expression string, replaceWith string, input string) (string,

 // SliceMatchesRegex checks if input matches any of the expressions. The
 // match is always evaluated as a regex either an exact match or regexp.
+//
+// If any expression starts with "+not ", the match is negated.


Need to update the comments to reflect the updated expression.

klizhentas · 2020-08-27T00:56:34Z

@awly can you take a look at:

https://github.com/gravitational/teleport/blob/master/lib/utils/parse/parse.go

I would like us to use the "ast" approach for all function calls. As you can spot above, there is a function "email.local" that also supports variable syntax, so it will support {{regexp.not(}} as well.

Ping me if you want to discuss in more detail.

awly · 2020-08-27T16:50:57Z

@klizhentas WDYT about going all-in on https://golang.org/pkg/text/template/ instead?

It looks like https://github.com/gravitational/teleport/blob/master/lib/utils/parse/parse.go just reimplements a subset of that, except for input data access ({{.external.foo}} being {{external.foo}}, which we can plumb as another func: https://play.golang.org/p/YwBaMRbj3g7)

awly · 2020-09-08T22:50:31Z

Chatted with @klizhentas, we can use text/template as long as it's backwards-compatible.
A small prototype to show how to get that working with our claims syntax: https://play.golang.org/p/vOtNwdvr9Dq

I'll flesh that out and re-open this PR.

awly · 2020-09-08T23:55:13Z

After more chatting, @klizhentas convinced me to go with our custom DSL.
text/template is flexible but its syntax is unfamiliar to non-Go developers.
A custom DSL gives us better UX at the cost of internal complexity.

awly · 2020-09-17T20:22:41Z

PTAL everyone, migrated this PR to use lib/utils/parse.
The new format supports {{regexp.match(...)}} and {{regexp.not_match(...)}}, along with the previous behavior.

webvictim · 2020-09-18T14:22:59Z

LGTM, I'll leave it for others to take a look.

benarent

So based on the issue example, this would be

role: {{regexp.not_match(`^critical$`)}}

awly · 2020-09-29T16:37:26Z

@benarent almost:

kind: role
version: v3
metadata:
  name: dev
spec:
  allow:
    node_labels:
      'environment': '{{regexp.not_match("^prod$")}}'

lib/utils/parse/parse.go

No need to handle literal expressions (e.g. without "{{foo.bar}}" substitutions) at the higher level. Something like "foo" is a valid expression which always returns "foo" regardless of traits.

Matchers use a similar syntax to Expressions, but behave differently: - Expressions get evaluated - they interpolate some values and return a final string. - Matchers check whether some string matches a value Matchers implement the same logic as utils.SliceMatchesRegex and add 2 new functions: - {{regexp.match("foo")}} - match input against a raw regex - {{regexp.not_match("foo")}} - same as match, but inverts the result

This should be backwards-compatible plus add the {{regexp.match(...)}} and {{regexp.not_match(...)}} functions.

klizhentas

@awly Please take a look at my comments when you have a chance.

klizhentas · 2020-09-30T03:32:57Z

lib/services/user.go

@@ -491,8 +491,8 @@ func (u *UserV1) Check() error {
 		return trace.BadParameter("user name cannot be empty")
 	}
 	for _, login := range u.AllowedLogins {
-		_, err := parse.RoleVariable(login)
-		if err == nil {
+		e, err := parse.NewExpression(login)


New logic only returns error in case of grammatic errors and accepts literal values. Doesn't it mean that now err should return error here if it's not nil? Otherwise the following will be accepted: {{regexp_not_match(...) which probably should not be ignored as it's a typo and we should let users know

klizhentas · 2020-09-30T03:36:10Z

lib/utils/parse/parse.go

+
+	re, err := regexp.Compile(raw)
+	if err != nil {
+		return nil, trace.BadParameter("failed parsing regexp %q: %v", raw, err)


May make sense to send a link to our documentation with examples. This will make our message actionable and reduce amount of support.

klizhentas · 2020-09-30T03:47:36Z

lib/utils/parse/parse.go

+						return nil, trace.Wrap(err)
+					}
+					// If this is not_match, wrap the regexpMatcher to invert
+					// it.


Extra newline?

awly · 2020-10-01T18:03:25Z

@klizhentas thanks for the comments, addressed in #4430

Also improve error output and user validation. Based on missed feedback in #4253 (review)

This change was not backwards compatible - variable interpolation should work in node_labels. This commit partially reverts #4253 and #4430

awly requested review from klizhentas and benarent August 24, 2020 22:21

awly requested review from fspmarshall, russjones and webvictim as code owners August 24, 2020 22:21

benarent reviewed Aug 24, 2020

View reviewed changes

docs/4.3/enterprise/ssh_rbac.md Outdated Show resolved Hide resolved

awly force-pushed the andrew/rbac_negative_regexp branch 2 times, most recently from 8dc73ba to c52a4ea Compare August 26, 2020 21:28

awly requested a review from benarent August 26, 2020 21:29

benarent reviewed Aug 26, 2020

View reviewed changes

awly force-pushed the andrew/rbac_negative_regexp branch from c52a4ea to b2b5213 Compare September 8, 2020 19:01

awly marked this pull request as draft September 8, 2020 22:50

awly force-pushed the andrew/rbac_negative_regexp branch from b2b5213 to 9d71803 Compare September 17, 2020 20:19

awly marked this pull request as ready for review September 17, 2020 20:20

awly requested a review from benarent September 17, 2020 20:21

benarent reviewed Sep 28, 2020

View reviewed changes

benarent approved these changes Sep 29, 2020

View reviewed changes

fspmarshall approved these changes Sep 29, 2020

View reviewed changes

lib/utils/parse/parse.go Outdated Show resolved Hide resolved

Andrew Lytvynov added 4 commits September 29, 2020 09:53

utils/parse: rewrite tests in the new RFD style

0c54ea8

utils/parse: support string literal expressions

5d54673

No need to handle literal expressions (e.g. without "{{foo.bar}}" substitutions) at the higher level. Something like "foo" is a valid expression which always returns "foo" regardless of traits.

Migrate services.MatchLabels to parse.Matcher

45cf28b

This should be backwards-compatible plus add the {{regexp.match(...)}} and {{regexp.not_match(...)}} functions.

awly force-pushed the andrew/rbac_negative_regexp branch from 9d71803 to 45cf28b Compare September 29, 2020 16:56

awly merged commit 75d7fbb into master Sep 29, 2020

awly deleted the andrew/rbac_negative_regexp branch September 29, 2020 21:25

benarent added this to the 4.4 "Rome" milestone Sep 30, 2020

benarent added the docs-required label Sep 30, 2020

klizhentas reviewed Sep 30, 2020

View reviewed changes

awly mentioned this pull request Oct 1, 2020

Validate node_labels syntax at role creation time #4430

Merged

awly pushed a commit that referenced this pull request Oct 2, 2020

Validate node_labels syntax at role creation time

f7a3e5c

Also improve error output and user validation. Based on missed feedback in #4253 (review)

awly pushed a commit that referenced this pull request Oct 5, 2020

Validate node_labels syntax at role creation time

63da432

Also improve error output and user validation. Based on missed feedback in #4253 (review)

awly mentioned this pull request Oct 15, 2020

Re-init the cache on error: "{{ external.email }}" is not a valid matcher expression #4493

Closed

awly pushed a commit that referenced this pull request Oct 15, 2020

Partial revert of negative regexps in RBAC labels

2832505

This change was not backwards compatible - variable interpolation should work in node_labels. This commit partially reverts #4253 and #4430

awly mentioned this pull request Oct 15, 2020

Partial revert of negative regexps in RBAC labels #4564

Merged

awly pushed a commit that referenced this pull request Oct 15, 2020

Partial revert of negative regexps in RBAC labels

01516c5

This change was not backwards compatible - variable interpolation should work in node_labels. This commit partially reverts #4253 and #4430

awly pushed a commit that referenced this pull request Oct 15, 2020

Partial revert of negative regexps in RBAC labels

f9141d7

This change was not backwards compatible - variable interpolation should work in node_labels. This commit partially reverts #4253 and #4430

awly mentioned this pull request Oct 15, 2020

Backport: partial revert of negative regexps in RBAC labels #4567

Merged

russjones pushed a commit that referenced this pull request Oct 16, 2020

Partial revert of negative regexps in RBAC labels

91fab7c

This change was not backwards compatible - variable interpolation should work in node_labels. This commit partially reverts #4253 and #4430

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add negative RBAC label matches #4253

Add negative RBAC label matches #4253

awly commented Aug 24, 2020 •

edited by benarent

Loading

benarent Aug 26, 2020

klizhentas commented Aug 27, 2020

awly commented Aug 27, 2020

awly commented Sep 8, 2020

awly commented Sep 8, 2020

awly commented Sep 17, 2020

webvictim commented Sep 18, 2020

benarent left a comment

awly commented Sep 29, 2020

klizhentas left a comment

klizhentas Sep 30, 2020

klizhentas Sep 30, 2020

klizhentas Sep 30, 2020

awly commented Oct 1, 2020

Add negative RBAC label matches #4253

Add negative RBAC label matches #4253

Conversation

awly commented Aug 24, 2020 • edited by benarent Loading

benarent Aug 26, 2020

Choose a reason for hiding this comment

klizhentas commented Aug 27, 2020

awly commented Aug 27, 2020

awly commented Sep 8, 2020

awly commented Sep 8, 2020

awly commented Sep 17, 2020

webvictim commented Sep 18, 2020

benarent left a comment

Choose a reason for hiding this comment

awly commented Sep 29, 2020

klizhentas left a comment

Choose a reason for hiding this comment

klizhentas Sep 30, 2020

Choose a reason for hiding this comment

klizhentas Sep 30, 2020

Choose a reason for hiding this comment

klizhentas Sep 30, 2020

Choose a reason for hiding this comment

awly commented Oct 1, 2020

awly commented Aug 24, 2020 •

edited by benarent

Loading