diff --git a/lib/accessmonitoring/request_mapping.go b/lib/accessmonitoring/request_mapping.go index f03e42052149e..f85594f7829db 100644 --- a/lib/accessmonitoring/request_mapping.go +++ b/lib/accessmonitoring/request_mapping.go @@ -17,10 +17,12 @@ limitations under the License. package accessmonitoring import ( + "slices" "time" "github.com/gravitational/trace" + "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/expression" "github.com/gravitational/teleport/lib/utils/typical" ) @@ -30,6 +32,7 @@ import ( type AccessRequestExpressionEnv struct { Roles []string SuggestedReviewers []string + RequestedResources []types.ResourceWithLabels Annotations map[string][]string User string RequestReason string @@ -87,10 +90,45 @@ func newRequestConditionParser() (*typical.Parser[AccessRequestExpressionEnv, an return env.Expiry, nil }), + "access_request.spec.resource_labels_union": typical.DynamicMap(func(env AccessRequestExpressionEnv) (expression.Dict, error) { + union := make(map[string][]string) + for _, resource := range env.RequestedResources { + for k, v := range resource.GetAllLabels() { + union[k] = append(union[k], v) + } + } + return expression.DictFromStringSliceMap(union), nil + }), + "access_request.spec.resource_labels_intersection": typical.DynamicMap(func(env AccessRequestExpressionEnv) (expression.Dict, error) { + if len(env.RequestedResources) == 0 { + return expression.Dict{}, nil + } + + intersection := make(map[string][]string) + + // Get first resource labels. + labels := env.RequestedResources[0].GetAllLabels() + for k, v := range labels { + intersection[k] = append(intersection[k], v) + } + + // Remove non-intersecting labels. + for _, resource := range env.RequestedResources { + labels := resource.GetAllLabels() + for k, v := range intersection { + if label, ok := labels[k]; !ok || !slices.Contains(v, label) { + delete(intersection, k) + } + } + } + return expression.DictFromStringSliceMap(intersection), nil + }), + "user.traits": typical.DynamicMap(func(env AccessRequestExpressionEnv) (expression.Dict, error) { return expression.DictFromStringSliceMap(env.UserTraits), nil }), } + defParserSpec := expression.DefaultParserSpec[AccessRequestExpressionEnv]() defParserSpec.Variables = typicalEnvVar diff --git a/lib/accessmonitoring/request_mapping_test.go b/lib/accessmonitoring/request_mapping_test.go index d2a0fb0fc796d..d9b10f23a2c5a 100644 --- a/lib/accessmonitoring/request_mapping_test.go +++ b/lib/accessmonitoring/request_mapping_test.go @@ -22,6 +22,8 @@ import ( "testing" "github.com/stretchr/testify/require" + + "github.com/gravitational/teleport/api/types" ) func TestEvaluateCondition(t *testing.T) { @@ -141,6 +143,101 @@ func TestEvaluateCondition(t *testing.T) { }, match: false, }, + { + description: "(union) single resource has label", + condition: ` + access_request.spec.resource_labels_union["env"]. + contains("test")`, + env: AccessRequestExpressionEnv{ + RequestedResources: []types.ResourceWithLabels{ + &types.ServerV2{ + Metadata: types.Metadata{ + Labels: map[string]string{"env": "test"}, + }, + }, + }, + }, + match: true, + }, + { + description: "(union) multiple resources have label", + condition: ` + access_request.spec.resource_labels_union["env"]. + contains_all(set("test", "dev"))`, + env: AccessRequestExpressionEnv{ + RequestedResources: []types.ResourceWithLabels{ + &types.ServerV2{ + Metadata: types.Metadata{ + Labels: map[string]string{"env": "test"}, + }, + }, + &types.ServerV2{ + Metadata: types.Metadata{ + Labels: map[string]string{"env": "dev"}, + }, + }, + }, + }, + match: true, + }, + { + description: "(intersection) single resource has label", + condition: ` + access_request.spec.resource_labels_intersection["env"]. + contains("test")`, + env: AccessRequestExpressionEnv{ + RequestedResources: []types.ResourceWithLabels{ + &types.ServerV2{ + Metadata: types.Metadata{ + Labels: map[string]string{"env": "test"}, + }, + }, + }, + }, + match: true, + }, + { + description: "(intersection) multiple resources have label", + condition: ` + access_request.spec.resource_labels_intersection["env"]. + contains("test")`, + env: AccessRequestExpressionEnv{ + RequestedResources: []types.ResourceWithLabels{ + &types.ServerV2{ + Metadata: types.Metadata{ + Labels: map[string]string{"env": "test"}, + }, + }, + &types.ServerV2{ + Metadata: types.Metadata{ + Labels: map[string]string{"env": "test"}, + }, + }, + }, + }, + match: true, + }, + { + description: "(intersection) multiple resource labels do not intersect", + condition: ` + access_request.spec.resource_labels_intersection["env"]. + contains("test")`, + env: AccessRequestExpressionEnv{ + RequestedResources: []types.ResourceWithLabels{ + &types.ServerV2{ + Metadata: types.Metadata{ + Labels: map[string]string{"env": "test"}, + }, + }, + &types.ServerV2{ + Metadata: types.Metadata{ + Labels: map[string]string{"env": "dev"}, + }, + }, + }, + }, + match: false, + }, } for _, test := range tests { diff --git a/lib/accessmonitoring/review/review.go b/lib/accessmonitoring/review/review.go index 269fa1f580899..37a409a22cd7d 100644 --- a/lib/accessmonitoring/review/review.go +++ b/lib/accessmonitoring/review/review.go @@ -29,6 +29,8 @@ import ( "github.com/gravitational/trace" "github.com/gravitational/teleport" + "github.com/gravitational/teleport/api/accessrequest" + "github.com/gravitational/teleport/api/client" accessmonitoringrulesv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/accessmonitoringrules/v1" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/accessmonitoring" @@ -41,6 +43,7 @@ type Client interface { SubmitAccessReview(ctx context.Context, params types.AccessReviewSubmission) (types.AccessRequest, error) ListAccessMonitoringRulesWithFilter(ctx context.Context, req *accessmonitoringrulesv1.ListAccessMonitoringRulesWithFilterRequest) ([]*accessmonitoringrulesv1.AccessMonitoringRule, string, error) GetUser(ctx context.Context, name string, withSecrets bool) (types.User, error) + client.ListResourcesClient } // Config specifies access review handler configuration. @@ -190,18 +193,11 @@ func (handler *Handler) onPendingRequest(ctx context.Context, req types.AccessRe "req_id", req.GetName(), "user", req.GetUser()) - // Automatic reviews are only supported with role requests. - if len(req.GetRequestedResourceIDs()) > 0 { - return trace.BadParameter("cannot automatically review access requests for resources other than 'roles'") - } - - const withSecretsFalse = false - user, err := handler.Client.GetUser(ctx, req.GetUser(), withSecretsFalse) + env, err := handler.newExpressionEnv(ctx, req) if err != nil { return trace.Wrap(err) } - env := getAccessRequestExpressionEnv(req, user.GetTraits()) reviewRule := handler.getMatchingRule(ctx, env) if reviewRule == nil { // This access request does not match any access monitoring rules. @@ -211,7 +207,9 @@ func (handler *Handler) onPendingRequest(ctx context.Context, req types.AccessRe review, err := newAccessReview( req.GetUser(), reviewRule.GetMetadata().GetName(), - reviewRule.GetSpec().GetAutomaticReview().GetDecision()) + reviewRule.GetSpec().GetAutomaticReview().GetDecision(), + time.Now(), + ) if err != nil { return trace.Wrap(err, "failed to create new access review") } @@ -267,7 +265,7 @@ func (handler *Handler) getMatchingRule( return reviewRule } -func newAccessReview(userName, ruleName, state string) (types.AccessReview, error) { +func newAccessReview(userName, ruleName, state string, created time.Time) (types.AccessReview, error) { var proposedState types.RequestState switch state { case types.RequestState_APPROVED.String(): @@ -284,7 +282,7 @@ func newAccessReview(userName, ruleName, state string) (types.AccessReview, erro Reason: fmt.Sprintf("Access request has been automatically %[4]s by %[1]q. "+ "User %[2]q is %[4]s by access_monitoring_rule %[3]q.", teleport.SystemAccessApproverUserName, userName, ruleName, strings.ToLower(state)), - Created: time.Now(), + Created: created, }, nil } @@ -296,16 +294,27 @@ func isAlreadyReviewedError(err error) bool { return trace.IsAlreadyExists(err) || strings.HasSuffix(err.Error(), "has already reviewed this request") } -// getAccessRequestExpressionEnv returns the expression env of the access request. -func getAccessRequestExpressionEnv(req types.AccessRequest, traits map[string][]string) accessmonitoring.AccessRequestExpressionEnv { +func (handler *Handler) newExpressionEnv(ctx context.Context, req types.AccessRequest) (accessmonitoring.AccessRequestExpressionEnv, error) { + const withSecretsFalse = false + user, err := handler.Client.GetUser(ctx, req.GetUser(), withSecretsFalse) + if err != nil { + return accessmonitoring.AccessRequestExpressionEnv{}, trace.Wrap(err) + } + + requestedResources, err := accessrequest.GetResourcesByResourceIDs(ctx, handler.Client, req.GetRequestedResourceIDs()) + if err != nil { + return accessmonitoring.AccessRequestExpressionEnv{}, trace.Wrap(err) + } + return accessmonitoring.AccessRequestExpressionEnv{ Roles: req.GetRoles(), + RequestedResources: requestedResources, SuggestedReviewers: req.GetSuggestedReviewers(), Annotations: req.GetSystemAnnotations(), User: req.GetUser(), RequestReason: req.GetRequestReason(), CreationTime: req.GetCreationTime(), Expiry: req.Expiry(), - UserTraits: traits, - } + UserTraits: user.GetTraits(), + }, nil } diff --git a/lib/accessmonitoring/review/review_test.go b/lib/accessmonitoring/review/review_test.go index 95748581c467e..fb06bdb7bd080 100644 --- a/lib/accessmonitoring/review/review_test.go +++ b/lib/accessmonitoring/review/review_test.go @@ -30,6 +30,7 @@ import ( "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" + pb "github.com/gravitational/teleport/api/client/proto" accessmonitoringrulesv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/accessmonitoringrules/v1" headerv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/header/v1" "github.com/gravitational/teleport/api/types" @@ -170,9 +171,10 @@ func TestConflictingRules(t *testing.T) { review, err := newAccessReview( requesterUserName, deniedRule.GetMetadata().GetName(), - deniedRule.GetSpec().GetAutomaticReview().GetDecision()) + deniedRule.GetSpec().GetAutomaticReview().GetDecision(), + time.Time{}, + ) require.NoError(t, err) - review.Created = time.Time{} client.On("SubmitAccessReview", mock.Anything, types.AccessReviewSubmission{ RequestID: testReqID, @@ -201,39 +203,136 @@ func TestResourceRequest(t *testing.T) { t.Cleanup(cancel) testReqID := uuid.New().String() + testRuleName := "test-rule" withSecretsFalse := false requesterUserName := "requester" - cache := accessmonitoring.NewCache() - cache.Put([]*accessmonitoringrulesv1.AccessMonitoringRule{ - newApprovedRule("approved-rule", "true"), - }) - requester, err := types.NewUser(requesterUserName) require.NoError(t, err) - client := &mockClient{} - client.On("GetUser", mock.Anything, requesterUserName, withSecretsFalse). - Return(requester, nil) + testRule := newApprovedRule( + testRuleName, + `access_request.spec.resource_labels_intersection["env"].contains("test")`) - handler, err := NewHandler(Config{ - HandlerName: handlerName, - Client: client, - Cache: cache, - }) - require.NoError(t, err) + cache := accessmonitoring.NewCache() + cache.Put([]*accessmonitoringrulesv1.AccessMonitoringRule{testRule}) - // Create a request for both a role and node. - req, err := types.NewAccessRequest(testReqID, "requester", "role") - require.NoError(t, err) - req.SetRequestedResourceIDs([]types.ResourceID{{Kind: "node"}}) + tests := []struct { + description string + setupMock func(m *mockClient) + assertErr require.ErrorAssertionFunc + }{ + { + description: "test 0 requested resources", + setupMock: func(m *mockClient) { + m.On("GetUser", mock.Anything, requesterUserName, withSecretsFalse). + Return(requester, nil) - event := types.Event{ - Type: types.OpPut, - Resource: req, + m.On("ListResources", mock.Anything, mock.Anything). + Return(&types.ListResourcesResponse{ + Resources: []types.ResourceWithLabels{}, + TotalCount: 0, + }, nil) + + m.AssertNotCalled(t, "SubmitAccessReview") + }, + assertErr: require.NoError, + }, + { + description: "test !matching resource labels", + setupMock: func(m *mockClient) { + m.On("GetUser", mock.Anything, requesterUserName, withSecretsFalse). + Return(requester, nil) + + m.On("ListResources", mock.Anything, mock.Anything). + Return(&types.ListResourcesResponse{ + Resources: []types.ResourceWithLabels{ + &types.ServerV2{ + Metadata: types.Metadata{ + Labels: map[string]string{"env": "prod"}, + }, + }, + }, + TotalCount: 1, + }, nil) + + m.AssertNotCalled(t, "SubmitAccessReview") + }, + assertErr: require.NoError, + }, + { + description: "test matching resource labels", + setupMock: func(m *mockClient) { + m.On("GetUser", mock.Anything, requesterUserName, withSecretsFalse). + Return(requester, nil) + + m.On("ListResources", mock.Anything, mock.Anything). + Return(&types.ListResourcesResponse{ + Resources: []types.ResourceWithLabels{ + &types.ServerV2{ + Metadata: types.Metadata{ + Labels: map[string]string{"env": "test"}, + }, + }, + }, + TotalCount: 1, + }, nil) + + review, err := newAccessReview( + requesterUserName, + testRuleName, + types.RequestState_APPROVED.String(), + time.Time{}, + ) + require.NoError(t, err) + + m.On("SubmitAccessReview", mock.Anything, types.AccessReviewSubmission{ + RequestID: testReqID, + Review: review, + }).Return(mock.Anything, nil) + }, + assertErr: require.NoError, + }, + } + + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + t.Parallel() + + client := &mockClient{} + if test.setupMock != nil { + test.setupMock(client) + } + + handler, err := NewHandler(Config{ + HandlerName: handlerName, + Client: client, + Cache: cache, + }) + require.NoError(t, err) + + req, err := types.NewAccessRequestWithResources( + testReqID, + requesterUserName, + []string{"role"}, + []types.ResourceID{ + { + ClusterName: "test-cluster", + Kind: types.KindNode, + Name: "test-node", + SubResourceName: types.SubKindTeleportNode, + }, + }, + ) + require.NoError(t, err) + + test.assertErr(t, handler.HandleAccessRequest(ctx, types.Event{ + Type: types.OpPut, + Resource: req, + })) + client.AssertExpectations(t) + }) } - require.ErrorContains(t, handler.HandleAccessRequest(ctx, event), - "cannot automatically review access requests for resources other than 'roles") } func TestHandleAccessRequest(t *testing.T) { @@ -327,9 +426,13 @@ func TestHandleAccessRequest(t *testing.T) { m.On("GetUser", mock.Anything, approvedUserName, withSecretsFalse). Return(approvedUser, nil) - review, err := newAccessReview(approvedUserName, testRuleName, types.RequestState_APPROVED.String()) + review, err := newAccessReview( + approvedUserName, + testRuleName, + types.RequestState_APPROVED.String(), + time.Time{}, + ) require.NoError(t, err) - review.Created = time.Time{} m.On("SubmitAccessReview", mock.Anything, types.AccessReviewSubmission{ RequestID: testReqID, @@ -430,3 +533,12 @@ func (m *mockClient) GetUser(ctx context.Context, name string, withSecrets bool) } return nil, args.Error(1) } + +func (m *mockClient) ListResources(ctx context.Context, req pb.ListResourcesRequest) (*types.ListResourcesResponse, error) { + args := m.Called(ctx, req) + resp, ok := args.Get(0).(*types.ListResourcesResponse) + if ok { + return resp, args.Error(1) + } + return nil, args.Error(1) +}