From 033826f7c77a4fee4a676da70c4715477bbf6fba Mon Sep 17 00:00:00 2001 From: Pawel Kopiczko Date: Fri, 22 Nov 2024 19:06:22 +0000 Subject: [PATCH] Add support for requiring reason for access requests --- lib/services/access_request.go | 106 ++++++- lib/services/access_request_test.go | 279 +++++++++++++++++- ...nally-require-reason-for-access-request.md | 28 +- .../RequestCheckout/RequestCheckout.tsx | 5 +- 4 files changed, 376 insertions(+), 42 deletions(-) diff --git a/lib/services/access_request.go b/lib/services/access_request.go index 692575c1a87e4..dd5d10fff1aca 100644 --- a/lib/services/access_request.go +++ b/lib/services/access_request.go @@ -275,7 +275,20 @@ func CalculateAccessCapabilities(ctx context.Context, clock clockwork.Clock, clt caps.SuggestedReviewers = v.SuggestedReviewers } - caps.RequireReason = v.requireReason + if v.requireReasonForAllRoles { + caps.RequireReason = true + } else if req.RequestableRoles { + caps.RequireReason, _, err = v.isReasonRequired(ctx, caps.RequestableRoles, nil) + if err != nil { + return nil, trace.Wrap(err) + } + } else { + caps.RequireReason, _, err = v.isReasonRequired(ctx, caps.ApplicableRolesForResources, nil) + if err != nil { + return nil, trace.Wrap(err) + } + } + caps.RequestPrompt = v.prompt caps.AutoRequest = v.autoRequest @@ -302,6 +315,8 @@ func (m *RequestValidator) allowedSearchAsRoles() ([]string, error) { // applicableSearchAsRoles prunes the search_as_roles and only returns those // applicable for the given list of resourceIDs. +// +// If loginHint is provided, it will attempt to prune the list to a single role. func (m *RequestValidator) applicableSearchAsRoles(ctx context.Context, resourceIDs []types.ResourceID, loginHint string) ([]string, error) { rolesToRequest, err := m.allowedSearchAsRoles() if err != nil { @@ -1041,10 +1056,23 @@ func (c *ReviewPermissionChecker) push(role types.Role) error { // a set of simple Allow/Deny datastructures. These, in turn, // are used to validate and expand the access request. type RequestValidator struct { - clock clockwork.Clock - getter RequestValidatorGetter - userState UserState - requireReason bool + clock clockwork.Clock + getter RequestValidatorGetter + userState UserState + // requireReasonForAllRoles indicates that non-empty reason is required for all access + // requests. This happens if any of the user roles has options.request_access "always" or + // "reason". + requireReasonForAllRoles bool + // requiringReasonRoles is a set of role names, which require non-empty reason to be + // specified when requested. The same applies to all requested resources allowed by those + // roles. Such roles are all requestable roles and search_as_roles allowed by a role + // assigned to a user and having spec.allow.request.reason.mode="required" set. + // + // Please note this means, roles having spec.allow.request.reason.mode="required" don't + // necessarily require reason when they are requested themselves. Instead they mark roles + // in spec.allow.request.roles and spec.allow.request.search_as_roles as roles requiring + // reason. + requiringReasonRoles map[string]struct{} // Used to enforce that the configuration found in the static // role that defined the search_as_role, is respected. // An empty map or list means nothing was configured. @@ -1097,6 +1125,8 @@ func NewRequestValidator(ctx context.Context, clock clockwork.Clock, getter Requ getter: getter, userState: uls, logger: slog.With(teleport.ComponentKey, "request.validator"), + + requiringReasonRoles: make(map[string]struct{}), } for _, opt := range opts { opt(&m) @@ -1132,8 +1162,14 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest return trace.BadParameter("request validator configured for different user (this is a bug)") } - if m.requireReason && req.GetRequestReason() == "" { - return trace.BadParameter("request reason must be specified (required by static role configuration)") + if len(req.GetRequestReason()) == 0 { + required, explanation, err := m.isReasonRequired(ctx, req.GetRoles(), req.GetRequestedResourceIDs()) + if err != nil { + return trace.Wrap(err) + } + if required { + return trace.BadParameter(explanation) + } } if !req.GetState().IsPromoted() && req.GetPromotedAccessListTitle() != "" { @@ -1313,6 +1349,36 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest return nil } +// isReasonValid checks if the reason is valid. If it isn't valid it also returns an explanation +// why. +func (m *RequestValidator) isReasonRequired(ctx context.Context, requestedRoles []string, requestedResourceIDs []types.ResourceID) (required bool, explanation string, err error) { + if m.requireReasonForAllRoles { + return true, "request reason must be specified (required request_access option in one of the roles)", nil + } + + allApplicableRoles := requestedRoles + if len(requestedResourceIDs) > 0 { + // Do not provide loginHint. We want all matching search_as_roles for those resources. + roles, err := m.applicableSearchAsRoles(ctx, requestedResourceIDs, "") + if err != nil { + return false, "", trace.Wrap(err) + } + if len(allApplicableRoles) == 0 { + allApplicableRoles = roles + } else { + allApplicableRoles = append(allApplicableRoles, roles...) + } + } + + for _, r := range allApplicableRoles { + if _, ok := m.requiringReasonRoles[r]; ok { + return true, fmt.Sprintf("request reason must be specified (required for role %q)", r), nil + } + } + + return false, "", nil +} + // calculateMaxAccessDuration calculates the maximum time for the access request. // The max duration time is the minimum of the max_duration time set on the request // and the max_duration time set on the request role. @@ -1518,7 +1584,7 @@ func (m *RequestValidator) GetRequestableRoles(ctx context.Context, identity tls roleAllowsAccess := true for _, resource := range filteredResources { - access, err := m.roleAllowsResource(ctx, role, resource, loginHint) + access, err := m.roleAllowsResource(role, resource, loginHint) if err != nil { return nil, trace.Wrap(err) } @@ -1564,7 +1630,7 @@ func setAllowRequestKubeResourceLookup(allowKubernetesResources []types.RequestK func (m *RequestValidator) push(ctx context.Context, role types.Role) error { var err error - m.requireReason = m.requireReason || role.GetOptions().RequestAccess.RequireReason() + m.requireReasonForAllRoles = m.requireReasonForAllRoles || role.GetOptions().RequestAccess.RequireReason() m.autoRequest = m.autoRequest || role.GetOptions().RequestAccess.ShouldAutoRequest() if m.prompt == "" { m.prompt = role.GetOptions().RequestPrompt @@ -1572,6 +1638,15 @@ func (m *RequestValidator) push(ctx context.Context, role types.Role) error { allow, deny := role.GetAccessRequestConditions(types.Allow), role.GetAccessRequestConditions(types.Deny) + if allow.Reason != nil && allow.Reason.Mode.Required() { + for _, r := range allow.Roles { + m.requiringReasonRoles[r] = struct{}{} + } + for _, r := range allow.SearchAsRoles { + m.requiringReasonRoles[r] = struct{}{} + } + } + setAllowRequestKubeResourceLookup(allow.KubernetesResources, allow.SearchAsRoles, m.kubernetesResource.allow) if len(deny.KubernetesResources) > 0 { @@ -1951,10 +2026,10 @@ func (m *RequestValidator) SystemAnnotations(req types.AccessRequest) (map[strin type ValidateRequestOption func(*RequestValidator) -// ExpandVars toggles variable expansion during request validation. Variable expansion -// includes expanding wildcard requests, setting system annotations, and gathering -// threshold information. Variable expansion should be run by the auth server prior -// to storing an access request for the first time. +// ExpandVars toggles variable expansion during request validation. Variable expansion includes +// expanding wildcard requests, setting system annotations, finding applicable roles for +// resource-based requests and gathering threshold information. Variable expansion should be run +// by the auth server prior to storing an access request for the first time. func ExpandVars(expand bool) ValidateRequestOption { return func(v *RequestValidator) { v.opts.expandVars = expand @@ -1962,7 +2037,7 @@ func ExpandVars(expand bool) ValidateRequestOption { } // ValidateAccessRequestForUser validates an access request against the associated users's -// *statically assigned* roles. If expandRoles is true, it will also expand wildcard +// *statically assigned* roles. If [[ExpandVars]] is set to true, it will also expand wildcard // requests, setting their role list to include all roles the user is allowed to request. // Expansion should be performed before an access request is initially placed in the backend. func ValidateAccessRequestForUser(ctx context.Context, clock clockwork.Clock, getter RequestValidatorGetter, req types.AccessRequest, identity tlsca.Identity, opts ...ValidateRequestOption) error { @@ -2121,7 +2196,7 @@ func (m *RequestValidator) pruneResourceRequestRoles( } for _, role := range allRoles { - roleAllowsAccess, err := m.roleAllowsResource(ctx, role, resource, loginHint, resourceMatcherToMatcherSlice(resourceMatcher)...) + roleAllowsAccess, err := m.roleAllowsResource(role, resource, loginHint, resourceMatcherToMatcherSlice(resourceMatcher)...) if err != nil { return nil, trace.Wrap(err) } @@ -2228,7 +2303,6 @@ func getAllowedKubeResourceKinds(allowedKinds []string, deniedKinds []string) [] } func (m *RequestValidator) roleAllowsResource( - ctx context.Context, role types.Role, resource types.ResourceWithLabels, loginHint string, diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go index d4fdf79bc4de6..3f7e04c941591 100644 --- a/lib/services/access_request_test.go +++ b/lib/services/access_request_test.go @@ -36,6 +36,7 @@ import ( "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/api/types/header" + "github.com/gravitational/teleport/api/types/trait" "github.com/gravitational/teleport/api/types/userloginstate" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/fixtures" @@ -2074,7 +2075,7 @@ func TestAutoRequest(t *testing.T) { { name: "no roles", assertion: func(t *testing.T, validator *RequestValidator) { - require.False(t, validator.requireReason) + require.False(t, validator.requireReasonForAllRoles) require.False(t, validator.autoRequest) require.Empty(t, validator.prompt) }, @@ -2083,7 +2084,7 @@ func TestAutoRequest(t *testing.T) { name: "with prompt", roles: []types.Role{empty, optionalRole, promptRole}, assertion: func(t *testing.T, validator *RequestValidator) { - require.False(t, validator.requireReason) + require.False(t, validator.requireReasonForAllRoles) require.False(t, validator.autoRequest) require.Equal(t, "test prompt", validator.prompt) }, @@ -2092,7 +2093,7 @@ func TestAutoRequest(t *testing.T) { name: "with auto request", roles: []types.Role{alwaysRole}, assertion: func(t *testing.T, validator *RequestValidator) { - require.False(t, validator.requireReason) + require.False(t, validator.requireReasonForAllRoles) require.True(t, validator.autoRequest) require.Empty(t, validator.prompt) }, @@ -2101,7 +2102,7 @@ func TestAutoRequest(t *testing.T) { name: "with prompt and auto request", roles: []types.Role{promptRole, alwaysRole}, assertion: func(t *testing.T, validator *RequestValidator) { - require.False(t, validator.requireReason) + require.False(t, validator.requireReasonForAllRoles) require.True(t, validator.autoRequest) require.Equal(t, "test prompt", validator.prompt) }, @@ -2110,7 +2111,7 @@ func TestAutoRequest(t *testing.T) { name: "with reason and auto prompt", roles: []types.Role{reasonRole}, assertion: func(t *testing.T, validator *RequestValidator) { - require.True(t, validator.requireReason) + require.True(t, validator.requireReasonForAllRoles) require.True(t, validator.autoRequest) require.Empty(t, validator.prompt) }, @@ -2142,6 +2143,274 @@ func TestAutoRequest(t *testing.T) { } +func TestReasonRequired(t *testing.T) { + t.Parallel() + + clusterName := "my-test-cluster" + + g := &mockGetter{ + roles: make(map[string]types.Role), + userStates: make(map[string]*userloginstate.UserLoginState), + users: make(map[string]types.User), + nodes: make(map[string]types.Server), + kubeServers: make(map[string]types.KubeServer), + dbServers: make(map[string]types.DatabaseServer), + appServers: make(map[string]types.AppServer), + desktops: make(map[string]types.WindowsDesktop), + clusterName: clusterName, + } + + nodeDesc := []struct { + name string + labels map[string]string + }{ + { + name: "fork-node", + labels: map[string]string{ + "cutlery": "fork", + }, + }, + { + name: "spoon-node", + labels: map[string]string{ + "cutlery": "spoon", + }, + }, + } + for _, desc := range nodeDesc { + node, err := types.NewServerWithLabels(desc.name, types.KindNode, types.ServerSpecV2{}, desc.labels) + require.NoError(t, err) + g.nodes[desc.name] = node + } + + roleDesc := map[string]types.RoleSpecV6{ + "cutlery-access": { + Allow: types.RoleConditions{ + NodeLabels: types.Labels{ + "cutlery": []string{types.Wildcard}, + }, + }, + }, + "fork-access": { + Allow: types.RoleConditions{ + NodeLabels: types.Labels{ + "cutlery": []string{"fork"}, + }, + }, + }, + + "cutlery-access-requester": { + Allow: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + Roles: []string{"cutlery-access"}, + // "optional" is the default + // Reason: &types.AccessRequestConditionsReason{ + // Mode: "optional", + // }, + }, + }, + }, + "cutlery-node-requester": { + Allow: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + SearchAsRoles: []string{"cutlery-access"}, + // set "optional" explicitly + Reason: &types.AccessRequestConditionsReason{ + Mode: "optional", + }, + }, + }, + }, + + "fork-node-requester": { + Allow: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + SearchAsRoles: []string{"fork-access"}, + // everything not-"required" is basically "optional" + Reason: &types.AccessRequestConditionsReason{ + Mode: "not-recognized-is-optional", + }, + }, + }, + }, + "fork-node-requester-with-reason": { + Allow: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + SearchAsRoles: []string{"fork-access"}, + Reason: &types.AccessRequestConditionsReason{ + Mode: "required", + }, + }, + }, + }, + "fork-access-requester": { + Allow: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + Roles: []string{"fork-access"}, + }, + }, + }, + "fork-access-requester-with-reason": { + Allow: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + Roles: []string{"fork-access"}, + Reason: &types.AccessRequestConditionsReason{ + Mode: "required", + }, + }, + }, + }, + } + for name, spec := range roleDesc { + role, err := types.NewRole(name, spec) + require.NoError(t, err) + g.roles[name] = role + } + + testCases := []struct { + name string + currentRoles []string + requestRoles []string + requestResourceIDs []types.ResourceID + expectError error + }{ + { + name: "role request: require reason when role has reason.required", + currentRoles: []string{"fork-access-requester-with-reason"}, + requestRoles: []string{"fork-access"}, + expectError: trace.BadParameter(`request reason must be specified (required for role "fork-access")`), + }, + { + name: "resource request: require reason when role has reason.required", + currentRoles: []string{"fork-node-requester-with-reason"}, + requestResourceIDs: []types.ResourceID{ + {ClusterName: clusterName, Kind: types.KindNode, Name: "fork-node"}, + }, + expectError: trace.BadParameter(`request reason must be specified (required for role "fork-access")`), + }, + { + name: "role request: do not require reason when role does not have reason.required", + currentRoles: []string{"fork-access-requester"}, + requestRoles: []string{"fork-access"}, + expectError: nil, + }, + { + name: "resource request: do not require reason when role does not have reason.required", + currentRoles: []string{"fork-node-requester"}, + requestResourceIDs: []types.ResourceID{ + {ClusterName: clusterName, Kind: types.KindNode, Name: "fork-node"}, + }, + expectError: nil, + }, + { + name: "resource request: but require reason when another role allowing _role_ access requires reason for the role", + currentRoles: []string{"fork-node-requester", "fork-access-requester-with-reason"}, + requestResourceIDs: []types.ResourceID{ + {ClusterName: clusterName, Kind: types.KindNode, Name: "fork-node"}, + }, + expectError: trace.BadParameter(`request reason must be specified (required for role "fork-access")`), + }, + { + name: "role request: require reason when _any_ role has reason.required", + currentRoles: []string{"fork-access-requester", "fork-access-requester-with-reason", "cutlery-access-requester"}, + requestRoles: []string{"fork-access"}, + expectError: trace.BadParameter(`request reason must be specified (required for role "fork-access")`), + }, + { + name: "resource request: require reason when _any_ role has reason.required", + currentRoles: []string{"fork-node-requester", "fork-node-requester-with-reason", "cutlery-node-requester"}, + requestResourceIDs: []types.ResourceID{ + {ClusterName: clusterName, Kind: types.KindNode, Name: "fork-node"}, + }, + expectError: trace.BadParameter(`request reason must be specified (required for role "fork-access")`), + }, + { + name: "role request: do not require reason when all roles don't have reason.required", + currentRoles: []string{"fork-access-requester", "cutlery-access-requester"}, + requestRoles: []string{"fork-access"}, + expectError: nil, + }, + { + name: "resource request: do not require reason when all roles don't have reason.required", + currentRoles: []string{"fork-node-requester", "cutlery-node-requester"}, + requestResourceIDs: []types.ResourceID{ + {ClusterName: clusterName, Kind: types.KindNode, Name: "fork-node"}, + }, + expectError: nil, + }, + { + name: "role request: require reason when _any_ role with reason.required matches _any_ roles", + currentRoles: []string{"fork-access-requester-with-reason", "cutlery-access-requester"}, + requestRoles: []string{"fork-access", "cutlery-access"}, + expectError: trace.BadParameter(`request reason must be specified (required for role "fork-access")`), + }, + { + name: "resource request: require reason when _any_ role with reason.required matches _any_ resource", + currentRoles: []string{"fork-node-requester-with-reason", "cutlery-node-requester"}, + requestResourceIDs: []types.ResourceID{ + {ClusterName: clusterName, Kind: types.KindNode, Name: "fork-node"}, + {ClusterName: clusterName, Kind: types.KindNode, Name: "spoon-node"}, + }, + expectError: trace.BadParameter(`request reason must be specified (required for role "fork-access")`), + }, + { + name: "role request: do not require reason when _all_ roles do not require reason for _all_ roles", + currentRoles: []string{"fork-access-requester", "cutlery-access-requester"}, + requestRoles: []string{"fork-access", "cutlery-access"}, + expectError: nil, + }, + { + name: "resource request: do not require reason when _all_ roles do not require reason for _all_ resources", + currentRoles: []string{"fork-node-requester", "cutlery-node-requester"}, + requestResourceIDs: []types.ResourceID{ + {ClusterName: clusterName, Kind: types.KindNode, Name: "fork-node"}, + {ClusterName: clusterName, Kind: types.KindNode, Name: "spoon-node"}, + }, + expectError: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + uls, err := userloginstate.New(header.Metadata{ + Name: "test-user", + }, userloginstate.Spec{ + Roles: tc.currentRoles, + Traits: trait.Traits{ + "logins": []string{"abcd"}, + }, + }) + require.NoError(t, err) + g.userStates[uls.GetName()] = uls + + req, err := types.NewAccessRequestWithResources( + "some-id", uls.GetName(), tc.requestRoles, tc.requestResourceIDs) + require.NoError(t, err) + + clock := clockwork.NewFakeClock() + identity := tlsca.Identity{ + Expires: clock.Now().UTC().Add(8 * time.Hour), + } + + validator, err := NewRequestValidator(context.Background(), clock, g, uls.GetName(), ExpandVars(true)) + require.NoError(t, err) + + required, explanation, err := validator.isReasonRequired(context.Background(), tc.requestRoles, tc.requestResourceIDs) + require.NoError(t, err) + if tc.expectError != nil { + require.True(t, required) + require.Equal(t, tc.expectError.Error(), explanation) + } else { + require.False(t, required) + require.Empty(t, explanation) + } + + err = validator.Validate(context.Background(), req, identity) + require.ErrorIs(t, err, tc.expectError) + }) + } +} + type mockClusterGetter struct { localCluster types.ClusterName remoteClusters map[string]types.RemoteCluster diff --git a/rfd/0186-optionally-require-reason-for-access-request.md b/rfd/0186-optionally-require-reason-for-access-request.md index 61b482b946e39..be54f8de828ee 100644 --- a/rfd/0186-optionally-require-reason-for-access-request.md +++ b/rfd/0186-optionally-require-reason-for-access-request.md @@ -117,10 +117,10 @@ Waiting for request approval... ### Possible implementations -#### 1. Add a new role.spec.allow.request.reason.required setting (chosen) +#### 1. Add a new role.spec.allow.request.reason.mode setting (chosen) -This is pretty self-explanatory. role.spec.allow.reason.required could be set -to true/false and being false by default. +Introduce a new role.spec.allow.reason.mode setting which could be set +to "required" or "optional" (default). Example: @@ -135,15 +135,14 @@ spec: roles: - kube-access reason: - required: true + mode: "required" ``` The problems with the approach: -- It isn't clear what should happen when when there is a role with - `options.request_access: always` and a role with - `allow.request.reason_required: true`. Should the reason be requested - regardless during login? +- It isn't clear what should happen when there is a role with + `options.request_access: "always"` and a role with `allow.request.mode: + "required"`. Should the reason be requested regardless during login? (yes) - The setting still spans across roles to some extend. If there is more than one role allowing requesting access to the same resource/role but only _some_ of them require reason, the reason is required. @@ -151,10 +150,6 @@ The problems with the approach: advanced requirements when it comes to providing a reason, e.g. using regular expressions. In case of `options.request_access: reason` - how to satisfy all of them? -- Minor: it will be possible to set `role.spec.deny.request.require.reason` (the same - type is being reused in the code) but it will do nothing. - - #### 2. Add a new value to role.options.request_access @@ -320,7 +315,7 @@ The problems with the approach: The chosen implementation is: ``` -3. Add a new role.spec.allow.request.reason.required setting +1. Add a new role.spec.allow.request.reason.mode setting ``` It seems to be least confusing, declarative and straight-forward. @@ -339,10 +334,9 @@ reason` in any of the roles. The IGS section of the test plan needs to be extended with these items: - [ ] Access Requests - - [ ] Verify when `role.spec.allow.request.reason.required: true`: - - [ ] Web UI doesn't display reason as optional - - [ ] Web UI highlights reason input when not provided but required - - [ ] CLI fails to create an access request. + - [ ] Verify when `role.spec.allow.request.reason.mode: "rquired"`: + - [ ] Web UI displays user-friendly error when reason is not provided + - [ ] CLI fails to create an access request when reason is not provided - [ ] Other roles allowing requesting the same resources/roles without `reason.required` set or with `reason.required: false` don't affect the behaviour. diff --git a/web/packages/shared/components/AccessRequests/NewRequest/RequestCheckout/RequestCheckout.tsx b/web/packages/shared/components/AccessRequests/NewRequest/RequestCheckout/RequestCheckout.tsx index 1ac5859a9274b..775c10f356267 100644 --- a/web/packages/shared/components/AccessRequests/NewRequest/RequestCheckout/RequestCheckout.tsx +++ b/web/packages/shared/components/AccessRequests/NewRequest/RequestCheckout/RequestCheckout.tsx @@ -734,9 +734,6 @@ function TextBox({ const hasError = !valid; const labelText = hasError ? message : 'Request Reason'; - const optionalText = requireReason ? '' : ' (optional)'; - const placeholder = `Describe your request...${optionalText}`; - return ( {labelText} @@ -749,7 +746,7 @@ function TextBox({ color="text.main" border={hasError ? '2px solid' : '1px solid'} borderColor={hasError ? 'error.main' : 'text.muted'} - placeholder={placeholder} + placeholder="Describe your request..." value={reason} onChange={e => updateReason(e.target.value)} css={`