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 5449e524a84ec..d329069b93f28 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 c4b01066d3742..7facc5ed7c4fd 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.