From 964e597eb438cf94b015d421d821cde458d203e2 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Mon, 15 Apr 2024 14:39:56 -0700 Subject: [PATCH] feat: add `regexp.match` to access request `filter` and `where` expressions This PR updates the parsers for the role `spec.allow.request.thresholds.filter` and `spec.allow.review.where` expression fields to support a new function `regexp.match(list, re)`. It follows the typical teleport style following from role templates, `where` expressions, login rules, and label expressions: - `list` can be a `[]string` or a `string` (a single string is treated as a list of length 1). - `re` can be a glob pattern like `example-*` or a proper regex like `^example-.*$` when surrounded by `^$`. Some examples are included in the docs and tests. I have switched from using `NewJSONBoolParser` to new dedicated parsers based on `typical`. These should have better performance in terms of speed and memory, be easier to extend in the future, and provide better error messages when users write invalid expressions. These were the only uses of `NewJSONBoolParser`, so it has been removed. Changelog: add `regexp.match` to access request `filter` and `where` expressions --- .../access-request-configuration.mdx | 65 +++-- docs/pages/access-controls/reference.mdx | 4 +- lib/services/access_request.go | 242 +++++++++++++----- lib/services/access_request_test.go | 123 +++++++-- lib/services/parser.go | 49 ---- 5 files changed, 322 insertions(+), 161 deletions(-) diff --git a/docs/pages/access-controls/access-requests/access-request-configuration.mdx b/docs/pages/access-controls/access-requests/access-request-configuration.mdx index 7d781f72d1b50..901ccf3509781 100644 --- a/docs/pages/access-controls/access-requests/access-request-configuration.mdx +++ b/docs/pages/access-controls/access-requests/access-request-configuration.mdx @@ -37,10 +37,10 @@ access to these roles. ### Restrict role requests When a user submits an Access Request, they can specify the roles they would -like to request access to. +like to request access to. You can allow a user to request access to certain roles—and deny access to other -roles—by using the following configuration options: +roles—by using the following configuration options: - `allow.request.roles` - `allow.request.claims_to_roles` @@ -78,7 +78,7 @@ The Teleport Auth Service combines the values of these fields for all of a user's roles into two lists of role matchers: - **Deny:** If the requested role matches any of these, Teleport denies the - request. + request. - **Allow:** If the requested role matches any of these, and no deny matcher also matches the role, Teleport allows the request. @@ -139,7 +139,7 @@ As with [configuring role requests](#restrict-role-requests), the literal role names, wildcards, and regular expressions. The Teleport Auth Service combines the values of these fields for all of a -user's Teleport roles in order to validate the user's Access Requests. +user's Teleport roles in order to validate the user's Access Requests. When a user attempts to list Teleport resources (such as servers and databases) or Kubernetes resources (such as pods and deployments), the Auth Service checks @@ -152,7 +152,7 @@ following: 1. Collects all the roles named in `allow.request.search_as_roles`, filtering these to exclude roles specified in `deny.request.search_as_roles` or - `deny.request.roles`. + `deny.request.roles`. 1. Determines which of the remaining roles can access the requested resource. For a Resource Access Request to be valid, one of the role matchers listed in a user's `search_as_roles` configuration must match a role that permits @@ -166,7 +166,7 @@ privileges for an approved Access Request. ### Calculating the duration of elevated privileges Teleport uses the following logic to calculate how long a user has elevated -privileges: +privileges: 1. Calculate the maximum duration of elevated privileges if the Access Request were granted. This is the lowest value of: @@ -174,10 +174,10 @@ privileges: create`](../../reference/cli/tsh.mdx#tsh-request-create) command (if the user creating the request provides this flag). - The lowest value of the `request.max_duration` field included in one of - the user's requested roles. + the user's requested roles. 1. Calculate the session TTL of the certificate the user would receive if Teleport were to grant the Access Request. This calculation chooses the - lowest value of: + lowest value of: - The requested session expiration time, which is the value of the `--session-ttl` flag of [`tsh request create`](../../reference/cli/tsh.mdx#tsh-request-create). @@ -208,7 +208,7 @@ override will be chosen. The `max_duration` option indicates the maximum length of time that a user is allowed to have elevated privileges for particular roles. After a user makes a successful Access Request, the user can authenticate to Teleport with the -elevated access until the maximum duration has elapsed. +elevated access until the maximum duration has elapsed. Each time the user authenticates to Teleport, Teleport calculates the TTL of the user's Teleport session using a formula that we describe in the [previous @@ -302,7 +302,7 @@ user: You can configure a user's roles to specify the criteria that an Access Request must meet before Teleport approves or denies it. To do so, configure the -`allow.request.thresholds` field. +`allow.request.thresholds` field. ### The `allow.request.thresholds` field @@ -351,12 +351,12 @@ the request is approved is denied: When Teleport processes an Access Request for a specific role, it checks whether the request has met the criteria specified in one of the thresholds in -`allow.request.thresholds` associated with that role. +`allow.request.thresholds` associated with that role. The value of `filter` is an expression that uses the Teleport [predicate language](../../reference/predicate-language.mdx). -For example, the following configuration includes three thresholds, two of which +For example, the following configuration includes four thresholds, three of which have filters: ```yaml @@ -373,12 +373,19 @@ spec: - filter: '!equals(request.reason, "") && contains(reviewer.roles, "super-approver")' approve: 1 deny: 1 + - filter: 'regexp.match(request.reason, "^Ticket [0-9]+.*$") && !equals(review.reason, "")' + approve: 1 + deny: 1 ``` The first threshold requires three users to approve and one user to deny. However, if each reviewer has the `super-approver` role, the request only needs -two approvals. And if the request has a non-empty reason, it only needs a single -approval from a user with the `super-approver` role. +two approvals. +If the request has a non-empty reason, it only needs a single approval from a +user with the `super-approver` role. +If the request has a reason matching the regex `^Ticket [0-9]+.*$`, it only +needs a single approval from any reviewer, as long as the reviewer provides a +non-empty reason. Filter expressions can draw on the following data associated with each Access Request review: the request, the reviewer, and the review itself: @@ -399,11 +406,22 @@ functions: |Operator/Function|Description| |---|---| |`equals(val1,val2)`|Returns `true` if `val1` is equal to `val2` and `false` otherwise| -|`contains([]string, val)`|Returns `true` if the list of strings in the first argument contains the element in the second argument and `false` otherwise.| +|`contains(list, item)`|Returns `true` if `list` contains an exact match for `item`.| +|`regexp.match(list, re)`|Returns `true` if `list` contains a match for `re`.| |`expr1 && expr2`|Evaluates to `true` if both `expr1` and `expr2` evaluate to `true`.| |`expr1 \|\| expr2`|Evaluates to `true` if `expr1`, `expr2`, or both evaluate to `true`.| |`!expr`|Negates `expr`.| +Above, any argument named `list` can accept a list of values (like +`request.roles`) or a single value (like `request.reason`). + +The `re` argument to `regexp.match` supports both glob-style wildcards (the `*` +character) and [Go-style regular expressions](https://pkg.go.dev/regexp). +If an expression begins with the `^` character and ends with the `$` character, +Teleport will evaluate it as a regular expression. +Otherwise, it will evaluate it as a wildcard expression. +Wildcards match any sequence of zero or more characters. + ## Suggested reviewers You can configure a Teleport role to suggest reviewers for Access Requests. @@ -442,7 +460,7 @@ While Teleport will accept a role with a nonempty Teleport users must be authorized to review Access Requests for a particular role. You can configure a user's Teleport roles to allow the user to review Access Requests for some Teleport roles, and deny the user the ability to review -requests for other roles. +requests for other roles. ### Allowing and denying reviews for specific roles @@ -517,12 +535,23 @@ functions: |Operator/Function|Description| |---|---| |`equals(val1,val2)`|Returns `true` if `val1` is equal to `val2` and `false` otherwise| -|`contains([]string, val)`|Returns `true` if the list of strings in the first argument contains the element in the second argument and `false` otherwise.| +|`contains(list, item)`|Returns `true` if `list` contains an exact match for `item`.| +|`regexp.match(list, re)`|Returns `true` if `list` contains a match for `re`.| |`expr1 && expr2`|Evaluates to `true` if both `expr1` and `expr2` evaluate to `true`.| |`expr1 \|\| expr2`|Evaluates to `true` if `expr1`, `expr2`, or both evaluate to `true`.| |`!expr`|Evaluates to the opposite of `expr`.| -## Inspecting requested resources +Above, any argument named `list` can accept a list of values (like +`request.roles`) or a single value (like `request.reason`). + +The `re` argument to `regexp.match` supports both glob-style wildcards (the `*` +character) and [Go-style regular expressions](https://pkg.go.dev/regexp). +If an expression begins with the `^` character and ends with the `$` character, +Teleport will evaluate it as a regular expression. +Otherwise, it will evaluate it as a wildcard expression. +Wildcards match any sequence of zero or more characters. + +## Inspecting requested resources A Teleport user can view information about a resource without having access to that resource. This is useful for inspecting a resource before granting another diff --git a/docs/pages/access-controls/reference.mdx b/docs/pages/access-controls/reference.mdx index 751e5d08eeb26..af77e79758a97 100644 --- a/docs/pages/access-controls/reference.mdx +++ b/docs/pages/access-controls/reference.mdx @@ -576,7 +576,7 @@ Regular expressions support both glob-style wildcards (the `*` character) and [Go-style regular expressions](https://pkg.go.dev/regexp). If an expression begins with the `^` character and ends with the `$` character, Teleport will evaluate it as a regular expression. Otherwise, it will evaluate it as a -wildcard expression. Wildcards match a sequence of one or more characters. +wildcard expression. Wildcards match any sequence of zero or more characters. #### Variables @@ -602,5 +602,3 @@ guide for a more in depth explanation of the language. Refer to the [Second Factor - WebAuthn](./guides/webauthn.mdx#u2f) guide if you have a cluster using the legacy U2F support. - - diff --git a/lib/services/access_request.go b/lib/services/access_request.go index 04c96176a0f89..46a35c24963ed 100644 --- a/lib/services/access_request.go +++ b/lib/services/access_request.go @@ -29,7 +29,6 @@ import ( "github.com/google/uuid" "github.com/gravitational/trace" "github.com/jonboulle/clockwork" - "github.com/vulcand/predicate" "github.com/gravitational/teleport/api/accessrequest" "github.com/gravitational/teleport/api/client" @@ -40,6 +39,7 @@ import ( "github.com/gravitational/teleport/lib/tlsca" "github.com/gravitational/teleport/lib/utils" "github.com/gravitational/teleport/lib/utils/parse" + "github.com/gravitational/teleport/lib/utils/typical" ) const ( @@ -319,33 +319,33 @@ type DynamicAccessExt interface { // which represents the incoming review during review threshold // filter evaluation. type reviewParamsContext struct { - Reason string `json:"reason"` - Annotations map[string][]string `json:"annotations"` + reason string + annotations map[string][]string } // reviewAuthorContext is a simplified view of a user // resource which represents the author of a review during // review threshold filter evaluation. type reviewAuthorContext struct { - Roles []string `json:"roles"` - Traits map[string][]string `json:"traits"` + roles []string + traits map[string][]string } // reviewRequestContext is a simplified view of an access request // resource which represents the request parameters which are in-scope // during review threshold filter evaluation. type reviewRequestContext struct { - Roles []string `json:"roles"` - Reason string `json:"reason"` - SystemAnnotations map[string][]string `json:"system_annotations"` + roles []string + reason string + systemAnnotations map[string][]string } // thresholdFilterContext is the top-level context used to evaluate // review threshold filters. type thresholdFilterContext struct { - Reviewer reviewAuthorContext `json:"reviewer"` - Review reviewParamsContext `json:"review"` - Request reviewRequestContext `json:"request"` + reviewer reviewAuthorContext + review reviewParamsContext + request reviewRequestContext } // reviewPermissionContext is the top-level context used to evaluate @@ -355,8 +355,8 @@ type thresholdFilterContext struct { // a user is allowed to see, and therefore needs to be calculable prior // to construction of review parameters. type reviewPermissionContext struct { - Reviewer reviewAuthorContext `json:"reviewer"` - Request reviewRequestContext `json:"request"` + reviewer reviewAuthorContext + request reviewRequestContext } // ValidateAccessPredicates checks request & review permission predicates for @@ -367,11 +367,6 @@ type reviewPermissionContext struct { // backwards compatibility with older nodes/proxies (which never need to evaluate // these predicates). func ValidateAccessPredicates(role types.Role) error { - tp, err := NewJSONBoolParser(thresholdFilterContext{}) - if err != nil { - return trace.Wrap(err, "failed to build empty threshold predicate parser (this is a bug)") - } - if len(role.GetAccessRequestConditions(types.Deny).Thresholds) != 0 { // deny blocks never contain thresholds. a threshold which happens to describe a *denial condition* is // still part of the "allow" block. thresholds are not part of deny blocks because thresholds describe the @@ -384,24 +379,19 @@ func ValidateAccessPredicates(role types.Role) error { if t.Filter == "" { continue } - if _, err := tp.EvalBoolPredicate(t.Filter); err != nil { + if _, err := parseThresholdFilterExpression(t.Filter); err != nil { return trace.BadParameter("invalid threshold predicate: %q, %v", t.Filter, err) } } - rp, err := NewJSONBoolParser(reviewPermissionContext{}) - if err != nil { - return trace.Wrap(err, "failed to build empty review predicate parser (this is a bug)") - } - if w := role.GetAccessReviewConditions(types.Deny).Where; w != "" { - if _, err := rp.EvalBoolPredicate(w); err != nil { + if _, err := parseReviewPermissionExpression(w); err != nil { return trace.BadParameter("invalid review predicate: %q, %v", w, err) } } if w := role.GetAccessReviewConditions(types.Allow).Where; w != "" { - if _, err := rp.EvalBoolPredicate(w); err != nil { + if _, err := parseReviewPermissionExpression(w); err != nil { return trace.BadParameter("invalid review predicate: %q, %v", w, err) } } @@ -535,15 +525,10 @@ func checkReviewCompat(req types.AccessRequest, rev types.AccessReview) error { // collectReviewThresholdIndexes aggregates the indexes of all thresholds whose filters match // the supplied review (part of review application logic). func collectReviewThresholdIndexes(req types.AccessRequest, rev types.AccessReview, author UserState) ([]uint32, error) { - parser, err := newThresholdFilterParser(req, rev, author) - if err != nil { - return nil, trace.Wrap(err) - } - var tids []uint32 - + ctx := newThresholdFilterContext(req, rev, author) for i, t := range req.GetThresholds() { - match, err := accessReviewThresholdMatchesFilter(t, parser) + match, err := accessReviewThresholdMatchesFilter(t, ctx) if err != nil { return nil, trace.Wrap(err) } @@ -566,39 +551,35 @@ func collectReviewThresholdIndexes(req types.AccessRequest, rev types.AccessRevi // accessReviewThresholdMatchesFilter returns true if Filter rule matches // Empty Filter block always matches -func accessReviewThresholdMatchesFilter(t types.AccessReviewThreshold, parser predicate.Parser) (bool, error) { +func accessReviewThresholdMatchesFilter(t types.AccessReviewThreshold, ctx thresholdFilterContext) (bool, error) { if t.Filter == "" { return true, nil } - ifn, err := parser.Parse(t.Filter) + expr, err := parseThresholdFilterExpression(t.Filter) if err != nil { return false, trace.Wrap(err) } - fn, ok := ifn.(predicate.BoolPredicate) - if !ok { - return false, trace.BadParameter("unsupported type: %T", ifn) - } - return fn(), nil + return expr.Evaluate(ctx) } -// newThresholdFilterParser creates a custom parser context which exposes a simplified view of the review author +// newThresholdFilterContext creates a custom parser context which exposes a simplified view of the review author // and the request for evaluation of review threshold filters. -func newThresholdFilterParser(req types.AccessRequest, rev types.AccessReview, author UserState) (BoolPredicateParser, error) { - return NewJSONBoolParser(thresholdFilterContext{ - Reviewer: reviewAuthorContext{ - Roles: author.GetRoles(), - Traits: author.GetTraits(), +func newThresholdFilterContext(req types.AccessRequest, rev types.AccessReview, author UserState) thresholdFilterContext { + return thresholdFilterContext{ + reviewer: reviewAuthorContext{ + roles: author.GetRoles(), + traits: author.GetTraits(), }, - Review: reviewParamsContext{ - Reason: rev.Reason, - Annotations: rev.Annotations, + review: reviewParamsContext{ + reason: rev.Reason, + annotations: rev.Annotations, }, - Request: reviewRequestContext{ - Roles: req.GetOriginalRoles(), - Reason: req.GetRequestReason(), - SystemAnnotations: req.GetSystemAnnotations(), + request: reviewRequestContext{ + roles: req.GetOriginalRoles(), + reason: req.GetRequestReason(), + systemAnnotations: req.GetSystemAnnotations(), }, - }) + } } // requestResolution describes a request state-transition from @@ -860,26 +841,27 @@ func (c *ReviewPermissionChecker) CanReviewRequest(req types.AccessRequest) (boo // called, so get the role list once in advance. requestedRoles := req.GetOriginalRoles() - parser, err := NewJSONBoolParser(reviewPermissionContext{ - Reviewer: reviewAuthorContext{ - Roles: c.UserState.GetRoles(), - Traits: c.UserState.GetTraits(), + rpc := reviewPermissionContext{ + reviewer: reviewAuthorContext{ + roles: c.UserState.GetRoles(), + traits: c.UserState.GetTraits(), }, - Request: reviewRequestContext{ - Roles: requestedRoles, - Reason: req.GetRequestReason(), - SystemAnnotations: req.GetSystemAnnotations(), + request: reviewRequestContext{ + roles: requestedRoles, + reason: req.GetRequestReason(), + systemAnnotations: req.GetSystemAnnotations(), }, - }) - if err != nil { - return false, trace.Wrap(err) } // check all denial rules first. for expr, denyMatchers := range c.Roles.DenyReview { // if predicate is non-empty, it must match if expr != "" { - match, err := parser.EvalBoolPredicate(expr) + parsed, err := parseReviewPermissionExpression(expr) + if err != nil { + return false, trace.Wrap(err) + } + match, err := parsed.Evaluate(rpc) if err != nil { return false, trace.Wrap(err) } @@ -908,7 +890,11 @@ Outer: for expr, allowMatchers := range c.Roles.AllowReview { // if predicate is non-empty, it must match. if expr != "" { - match, err := parser.EvalBoolPredicate(expr) + parsed, err := parseReviewPermissionExpression(expr) + if err != nil { + return false, trace.Wrap(err) + } + match, err := parsed.Evaluate(rpc) if err != nil { return false, trace.Wrap(err) } @@ -2057,3 +2043,125 @@ func getKubeResourcesFromResourceIDs(resourceIDs []types.ResourceID, clusterName } return kubernetesResources, nil } + +func newReviewPermissionParser() (*typical.Parser[reviewPermissionContext, bool], error) { + return typical.NewParser[reviewPermissionContext, bool](typical.ParserSpec[reviewPermissionContext]{ + Variables: map[string]typical.Variable{ + "reviewer.roles": typical.DynamicVariable(func(ctx reviewPermissionContext) ([]string, error) { + return ctx.reviewer.roles, nil + }), + "reviewer.traits": typical.DynamicVariable(func(ctx reviewPermissionContext) (map[string][]string, error) { + return ctx.reviewer.traits, nil + }), + "request.roles": typical.DynamicVariable(func(ctx reviewPermissionContext) ([]string, error) { + return ctx.request.roles, nil + }), + "request.reason": typical.DynamicVariable(func(ctx reviewPermissionContext) (string, error) { + return ctx.request.reason, nil + }), + "request.system_annotations": typical.DynamicVariable(func(ctx reviewPermissionContext) (map[string][]string, error) { + return ctx.request.systemAnnotations, nil + }), + }, + Functions: map[string]typical.Function{ + "equals": typical.BinaryFunction[reviewPermissionContext](equalsFunc), + "contains": typical.BinaryFunction[reviewPermissionContext](containsFunc), + "regexp.match": typical.BinaryFunction[reviewPermissionContext](regexpMatchFunc), + }, + }) +} + +func mustNewReviewPermissionParser() *typical.Parser[reviewPermissionContext, bool] { + parser, err := newReviewPermissionParser() + if err != nil { + panic(err) + } + return parser +} + +var ( + reviewPermissionParser = mustNewReviewPermissionParser() +) + +func parseReviewPermissionExpression(expr string) (typical.Expression[reviewPermissionContext, bool], error) { + parsed, err := reviewPermissionParser.Parse(expr) + return parsed, trace.Wrap(err, "parsing review.where expression") +} + +func newThresholdFilterParser() (*typical.Parser[thresholdFilterContext, bool], error) { + return typical.NewParser[thresholdFilterContext, bool](typical.ParserSpec[thresholdFilterContext]{ + Variables: map[string]typical.Variable{ + "reviewer.roles": typical.DynamicVariable(func(ctx thresholdFilterContext) ([]string, error) { + return ctx.reviewer.roles, nil + }), + "reviewer.traits": typical.DynamicVariable(func(ctx thresholdFilterContext) (map[string][]string, error) { + return ctx.reviewer.traits, nil + }), + "review.reason": typical.DynamicVariable(func(ctx thresholdFilterContext) (string, error) { + return ctx.review.reason, nil + }), + "review.annotations": typical.DynamicVariable(func(ctx thresholdFilterContext) (map[string][]string, error) { + return ctx.review.annotations, nil + }), + "request.roles": typical.DynamicVariable(func(ctx thresholdFilterContext) ([]string, error) { + return ctx.request.roles, nil + }), + "request.reason": typical.DynamicVariable(func(ctx thresholdFilterContext) (string, error) { + return ctx.request.reason, nil + }), + "request.system_annotations": typical.DynamicVariable(func(ctx thresholdFilterContext) (map[string][]string, error) { + return ctx.request.systemAnnotations, nil + }), + }, + Functions: map[string]typical.Function{ + "equals": typical.BinaryFunction[thresholdFilterContext](equalsFunc), + "contains": typical.BinaryFunction[thresholdFilterContext](containsFunc), + "regexp.match": typical.BinaryFunction[thresholdFilterContext](regexpMatchFunc), + }, + }) +} + +func mustNewThresholdFilterParser() *typical.Parser[thresholdFilterContext, bool] { + parser, err := newThresholdFilterParser() + if err != nil { + panic(err) + } + return parser +} + +var ( + thresholdFilterParser = mustNewThresholdFilterParser() +) + +func parseThresholdFilterExpression(expr string) (typical.Expression[thresholdFilterContext, bool], error) { + parsed, err := thresholdFilterParser.Parse(expr) + return parsed, trace.Wrap(err, "parsing threshold filter expression") +} + +func equalsFunc(a, b any) (bool, error) { + switch aval := a.(type) { + case string: + bval, ok := b.(string) + if ok { + return aval == bval, nil + } + case []string: + bval, ok := b.([]string) + if ok { + return slices.Equal(aval, bval), nil + } + } + return false, trace.BadParameter("parameter types must match and be string or []string, got (%T, %T)", a, b) +} + +func containsFunc(s []string, v string) (bool, error) { + return slices.Contains(s, v), nil +} + +func regexpMatchFunc(list []string, re string) (bool, error) { + match, err := utils.RegexMatchesAny(list, re) + if err != nil { + return false, trace.Wrap(err, "invalid regular expression %q", re) + } + return match, nil +} diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go index 0b2b0388183ce..f988dfb26e2a5 100644 --- a/lib/services/access_request_test.go +++ b/lib/services/access_request_test.go @@ -214,6 +214,23 @@ func TestReviewThresholds(t *testing.T) { // no explicit thresholds defaults to single approval/denial }, }, + // rationalists can achieve dictatorship via consensus with a single + // approval if there is a good reason. + "rationalist": { + Request: &types.AccessRequestConditions{ + Roles: []string{"dictator"}, + Annotations: map[string][]string{ + "mechanism": {"consensus"}, + }, + Thresholds: []types.AccessReviewThreshold{ + { + Name: "rational consensus", + Filter: `regexp.match(request.reason, "*good*") && regexp.match(review.reason, "*good*")`, + Approve: 1, + }, + }, + }, + }, // idealists have an directive for requesting a role which // does not exist, and no threshold which permit it to be assumed. "idealist": { @@ -231,21 +248,22 @@ func TestReviewThresholds(t *testing.T) { "proletariat": { ReviewRequests: &types.AccessReviewConditions{ Roles: []string{"dictator"}, - Where: `contains(request.system_annotations["mechanisms"],"uprising")`, + Where: `contains(request.system_annotations["mechanism"],"uprising")`, }, }, - // the intelligentsia can put dictators into power via consensus + // the intelligentsia can put dictators into power via consensus, with a + // good reason. "intelligentsia": { ReviewRequests: &types.AccessReviewConditions{ - Roles: []string{"dictator"}, - Where: `contains(request.system_annotations["mechanism"],"consensus")`, + Roles: []string{"dictator", "never"}, + Where: `contains(request.system_annotations["mechanism"],"consensus") && regexp.match(request.reason, "*good*")`, }, }, // the military can put dictators into power via a coup our treachery "military": { ReviewRequests: &types.AccessReviewConditions{ Roles: []string{"dictator"}, - Where: `contains(request.system_annotations["mechanisms"],"coup") || contains(request.system_annotations["mechanism"],"treachery")`, + Where: `contains(request.system_annotations["mechanism"],"coup") || contains(request.system_annotations["mechanism"],"treachery")`, }, }, // never is the role that will never be requested @@ -285,8 +303,9 @@ func TestReviewThresholds(t *testing.T) { users := make(map[string]types.User) userDesc := map[string][]string{ - "bob": {"general", "proletariat", "intelligentsia", "military"}, - "dave": {"populist", "general", "conqueror"}, + "bob": {"general", "proletariat", "intelligentsia", "military"}, + "dave": {"populist", "general", "conqueror"}, + "frank": {"rationalist"}, } for name, roles := range userDesc { @@ -304,6 +323,7 @@ func TestReviewThresholds(t *testing.T) { } const ( + pending = types.RequestState_PENDING approve = types.RequestState_APPROVED deny = types.RequestState_DENIED promote = types.RequestState_PROMOTED @@ -320,6 +340,8 @@ func TestReviewThresholds(t *testing.T) { expect types.RequestState // assumeStartTime to apply to review assumeStartTime time.Time + // reason for the review + reason string errCheck require.ErrorAssertionFunc } @@ -331,13 +353,16 @@ func TestReviewThresholds(t *testing.T) { // requestor is the name of the requesting user requestor string // the roles to be requested (defaults to "dictator") - roles []string + roles []string + // the reason for the request + reason string reviews []review expiry time.Time }{ { desc: "populist approval via multi-threshold match", requestor: "alice", // permitted by role populist + reason: "some very good reason", reviews: []review{ { // cannot review own requests author: "alice", @@ -365,6 +390,7 @@ func TestReviewThresholds(t *testing.T) { { desc: "trying to deny an already approved request", requestor: "alice", // permitted by role populist + reason: "some very good reason", reviews: []review{ { // cannot review own requests author: "alice", @@ -418,9 +444,21 @@ func TestReviewThresholds(t *testing.T) { }, }, }, + { + desc: "intelligentsia cannot review without reason", + requestor: "alice", + reviews: []review{ + { + author: g.user(t, "intelligentsia"), + propose: approve, + noReview: true, + }, + }, + }, { desc: "populist approval via consensus threshold", requestor: "alice", // permitted by role populist + reason: "some very good reason", reviews: []review{ { // matches "consensus" threshold author: g.user(t, "intelligentsia"), @@ -448,6 +486,7 @@ func TestReviewThresholds(t *testing.T) { { desc: "general denial via coup threshold", requestor: "bob", // permitted by role general + reason: "some very good reason", reviews: []review{ { // cannot review own requests author: "bob", @@ -483,6 +522,7 @@ func TestReviewThresholds(t *testing.T) { { desc: "conqueror approval via default threshold", requestor: "carol", // permitted by role conqueror + reason: "some very good reason", reviews: []review{ { // cannot review own requests author: "carol", @@ -521,6 +561,7 @@ func TestReviewThresholds(t *testing.T) { // "smart" about which thresholds really need to pass. desc: "multi-role requestor approval via separate threshold matches", requestor: "dave", // permitted by conqueror, general, *and* populist + reason: "some very good reason", reviews: []review{ { // matches "default", "coup", and "consensus" thresholds author: g.user(t, "military"), @@ -571,6 +612,7 @@ func TestReviewThresholds(t *testing.T) { // about denying requests (i.e. erring on the side of caution). desc: "multi-role requestor denial via short-circuit on default", requestor: "dave", // permitted by conqueror, general, *and* populist + reason: "some very good reason", reviews: []review{ { // ... author: g.user(t, "intelligentsia"), @@ -586,6 +628,7 @@ func TestReviewThresholds(t *testing.T) { desc: "threshold omission related sanity-check", requestor: "erika", // permitted by combination of populist and idealist roles: []string{"dictator", "never"}, + reason: "some very good reason", reviews: []review{ { // matches default threshold from idealist author: g.user(t, "intelligentsia"), @@ -602,6 +645,7 @@ func TestReviewThresholds(t *testing.T) { // be omitted. desc: "threshold omission check", requestor: "erika", // permitted by populist, but also holds idealist + reason: "some very good reason", reviews: []review{ { // review is permitted but matches no thresholds author: g.user(t, "intelligentsia"), @@ -624,6 +668,7 @@ func TestReviewThresholds(t *testing.T) { { desc: "promoted skips the threshold check", requestor: "bob", + reason: "some very good reason", reviews: []review{ { // status should be set to promoted despite the approval threshold not being met author: g.user(t, "intelligentsia"), @@ -651,6 +696,26 @@ func TestReviewThresholds(t *testing.T) { }, }, }, + { + desc: "rationalist approval with valid reasons", + requestor: "frank", + roles: []string{"dictator"}, + reason: "some very good reason", + reviews: []review{ + { + author: g.user(t, "intelligentsia"), + propose: approve, + reason: "frank is just okay", + expect: pending, + }, + { + author: g.user(t, "intelligentsia"), + propose: approve, + reason: "frank is pretty good", + expect: approve, + }, + }, + }, } for _, tt := range tts { @@ -662,6 +727,7 @@ func TestReviewThresholds(t *testing.T) { // create a request for the specified author req, err := types.NewAccessRequest("some-id", tt.requestor, tt.roles...) require.NoError(t, err, "scenario=%q", tt.desc) + req.SetRequestReason(tt.reason) clock := clockwork.NewFakeClock() identity := tlsca.Identity{ @@ -694,11 +760,14 @@ func TestReviewThresholds(t *testing.T) { if rt.noReview { require.False(t, canReview, "scenario=%q, rev=%d", tt.desc, ri) continue Inner + } else { + require.True(t, canReview, "scenario=%q, rev=%d", tt.desc, ri) } rev := types.AccessReview{ Author: rt.author, ProposedState: rt.propose, + Reason: rt.reason, AssumeStartTime: &rt.assumeStartTime, } @@ -744,22 +813,22 @@ func TestThresholdReviewFilter(t *testing.T) { }{ { // test expected matching behavior against a basic example context ctx: thresholdFilterContext{ - Reviewer: reviewAuthorContext{ - Roles: []string{"dev"}, - Traits: map[string][]string{ + reviewer: reviewAuthorContext{ + roles: []string{"dev"}, + traits: map[string][]string{ "teams": {"staging-admin"}, }, }, - Review: reviewParamsContext{ - Reason: "ok", - Annotations: map[string][]string{ + review: reviewParamsContext{ + reason: "ok", + annotations: map[string][]string{ "constraints": {"no-admin"}, }, }, - Request: reviewRequestContext{ - Roles: []string{"dev"}, - Reason: "plz", - SystemAnnotations: map[string][]string{ + request: reviewRequestContext{ + roles: []string{"dev"}, + reason: "Ticket 123", + systemAnnotations: map[string][]string{ "teams": {"staging-dev"}, }, }, @@ -768,9 +837,11 @@ func TestThresholdReviewFilter(t *testing.T) { `contains(reviewer.roles,"dev")`, `contains(reviewer.traits["teams"],"staging-admin") && contains(request.system_annotations["teams"],"staging-dev")`, `!contains(review.annotations["constraints"],"no-admin") || !contains(request.roles,"admin")`, - `equals(request.reason,"plz") && equals(review.reason,"ok")`, + `equals(request.reason,"Ticket 123") && equals(review.reason,"ok")`, `contains(reviewer.roles,"admin") || contains(reviewer.roles,"dev")`, `!(contains(reviewer.roles,"foo") || contains(reviewer.roles,"bar"))`, + `regexp.match(request.roles, "^dev(elopers)?$")`, + `regexp.match(request.reason, "^Ticket [0-9]+.*$") && !equals(review.reason, "")`, }, wontMatch: []string{ `contains(reviewer.roles, "admin")`, @@ -793,6 +864,8 @@ func TestThresholdReviewFilter(t *testing.T) { `contains(reviewer.traits["teams"],"staging-admin") && contains(request.system_annotations["teams"],"staging-dev")`, `equals(request.reason,"plz") && equals(review.reason,"ok")`, `contains(reviewer.roles,"admin") || contains(reviewer.roles,"dev")`, + `regexp.match(request.roles, "^dev(elopers)?$")`, + `regexp.match(request.reason, "^Ticket [0-9]+.*$") && !equals(review.reason, "")`, }, // confirm that an empty context can be used to catch syntax errors wontParse: []string{ @@ -808,22 +881,24 @@ func TestThresholdReviewFilter(t *testing.T) { } for _, tt := range tts { - parser, err := NewJSONBoolParser(tt.ctx) - require.NoError(t, err) for _, expr := range tt.willMatch { - result, err := parser.EvalBoolPredicate(expr) + parsed, err := parseThresholdFilterExpression(expr) + require.NoError(t, err) + result, err := parsed.Evaluate(tt.ctx) require.NoError(t, err) require.True(t, result) } for _, expr := range tt.wontMatch { - result, err := parser.EvalBoolPredicate(expr) + parsed, err := parseThresholdFilterExpression(expr) + require.NoError(t, err) + result, err := parsed.Evaluate(tt.ctx) require.NoError(t, err) require.False(t, result) } for _, expr := range tt.wontParse { - _, err := parser.EvalBoolPredicate(expr) + _, err := parseThresholdFilterExpression(expr) require.Error(t, err) } } diff --git a/lib/services/parser.go b/lib/services/parser.go index d5a132788533e..8fba29ed3e7d4 100644 --- a/lib/services/parser.go +++ b/lib/services/parser.go @@ -578,55 +578,6 @@ func (r *EmptyResource) GetMetadata() types.Metadata { func (r *EmptyResource) CheckAndSetDefaults() error { return nil } -// BoolPredicateParser extends predicate.Parser with a convenience method -// for evaluating bool predicates. -type BoolPredicateParser interface { - predicate.Parser - EvalBoolPredicate(string) (bool, error) -} - -type boolPredicateParser struct { - predicate.Parser -} - -func (p boolPredicateParser) EvalBoolPredicate(expr string) (bool, error) { - ifn, err := p.Parse(expr) - if err != nil { - return false, trace.Wrap(err) - } - - fn, ok := ifn.(predicate.BoolPredicate) - if !ok { - return false, trace.BadParameter("expected boolean predicate, got unsupported type: %T", ifn) - } - - return fn(), nil -} - -// NewJSONBoolParser returns a generic parser for boolean expressions based on a -// json-serializable context. -func NewJSONBoolParser(ctx interface{}) (BoolPredicateParser, error) { - p, err := predicate.NewParser(predicate.Def{ - Operators: predicate.Operators{ - AND: predicate.And, - OR: predicate.Or, - NOT: predicate.Not, - }, - Functions: map[string]interface{}{ - "equals": predicate.Equals, - "contains": predicate.Contains, - }, - GetIdentifier: func(fields []string) (interface{}, error) { - return predicate.GetFieldByTag(ctx, teleport.JSON, fields) - }, - GetProperty: GetStringMapValue, - }) - if err != nil { - return nil, trace.Wrap(err) - } - return boolPredicateParser{Parser: p}, nil -} - // newParserForIdentifierSubcondition returns a parser customized for // extracting the largest admissible subexpression of a `where` condition that // involves the given identifier.