Address CRs

lib/services/access_request.go

@@ -22,6 +22,7 @@ import (
 	"context"
 	"fmt"
 	"log/slog"
+	"maps"
 	"slices"
 	"sort"
 	"strings"
@@ -1045,10 +1046,11 @@ type RequestValidator struct {
 	// role that defined the search_as_role, is respected.
 	// An empty map or list means nothing was configured.
 	kubernetesResource struct {
-		// Collection of search as roles mapped to list of allowed resources.
-		// found in the static roles.
+		// allow is a map from the user's allowed search_as_roles to the list of
+		// kubernetes resource kinds the user is allowed to request with that role.
 		allow map[string][]types.RequestKubernetesResource
-		// Collection of denied resources from all of the user's static roles.
+		// deny is the list of kubernetes resource kinds the user is explicitly
+		// denied from requesting.
 		deny []types.RequestKubernetesResource
 	}
 	autoRequest bool
@@ -1179,7 +1181,7 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
 		}
 	}
 
-	// verify that for each requested roles, it allows requesting to every requested kube resource kinds.
+	// Verify that each requested role allows requesting every requested kube resource kind.
 	if len(req.GetRequestedResourceIDs()) > 0 && len(req.GetRoles()) > 0 {
 		// If there were pruned roles, then the request will be rejected.
 		// A pruned role meant that role did not allow requesting to all of requested kube resource.
@@ -1666,13 +1668,16 @@ func (m *RequestValidator) setRolesForResourceRequest(ctx context.Context, req t
 // lets user know which kinds are allowed for each requested roles.
 func (m *RequestValidator) pruneRequestedRolesNotMatchingKubernetesResourceKinds(requestedResourceIDs []types.ResourceID, requestedRoles []string) ([]string, map[string][]string) {
 	// Filter for the kube_cluster and its subresource kinds.
-	requestedKubeKinds := make([]string, 0, len(requestedResourceIDs))
+	requestedKubeKinds := make(map[string]struct{})
 	for _, resourceID := range requestedResourceIDs {
 		if resourceID.Kind == types.KindKubernetesCluster || slices.Contains(types.KubernetesResourcesKinds, resourceID.Kind) {
-			requestedKubeKinds = append(requestedKubeKinds, resourceID.Kind)
+			requestedKubeKinds[resourceID.Kind] = struct{}{}
 		}
 	}
-	requestedKubeKinds = apiutils.Deduplicate(requestedKubeKinds)
+
+	if len(requestedKubeKinds) == 0 {
+		return requestedRoles, nil
+	}
 
 	goodRoles := make(map[string]struct{})
 	mappedRequestedRolesToAllowedKinds := make(map[string][]string)
@@ -1699,8 +1704,8 @@ func (m *RequestValidator) pruneRequestedRolesNotMatchingKubernetesResourceKinds
 		mappedRequestedRolesToAllowedKinds[requestedRoleName] = allowedKinds
 
 		roleIsDenied := false
-		for _, requestedKubeKind := range requestedKubeKinds {
-			if slices.Contains(deniedKinds, requestedKubeKind) || !slices.Contains(allowedKinds, requestedKubeKind) {
+		for requestedKubeKind := range requestedKubeKinds {
+			if !slices.Contains(allowedKinds, requestedKubeKind) {
 				roleIsDenied = true
 				continue
 			}
@@ -1711,12 +1716,7 @@ func (m *RequestValidator) pruneRequestedRolesNotMatchingKubernetesResourceKinds
 		}
 	}
 
-	prunedRoles := make([]string, 0, len(goodRoles))
-	for role := range goodRoles {
-		prunedRoles = append(prunedRoles, role)
-	}
-
-	return prunedRoles, mappedRequestedRolesToAllowedKinds
+	return slices.Collect(maps.Keys(goodRoles)), mappedRequestedRolesToAllowedKinds
 }
 
 // thresholdCollector is a helper that assembles the Thresholds array for a request.
@@ -2045,17 +2045,17 @@ func getInvalidKubeKindAccessRequestsError(mappedRequestedRolesToAllowedKinds ma
 		InvalidKubernetesKindAccessRequest, requestWord, allowedStr)
 }
 
-// pruneResourceRequestRoles takes an access request and does one of two things:
-//  1. If it is a role request, returns it unchanged.
-//  2. If it is a resource request, all available `search_as_roles` for the user
-//     should have been populated on the request by `ValidateAccessReqeustForUser`.
-//     This function will attempt to prune these roles to a minimal necessary set
-//     based on the following rules:
-//     - If a role does not grant access to any resources in the set, it is pruned.
-//     - If the request includes a LoginHint, access to a node with that login
-//     should be satisfied by exactly 1 role. The first such role will be
-//     requested, all others will be pruned unless they are necessary to access
-//     a different resource in the set.
+// pruneResourceRequestRoles takes a list of requested resource IDs and
+// a list of candidate roles to request, and returns a "pruned" list of roles.
+//
+// Candidate roles are *always* pruned when the user is not allowed to
+// request the role with all requested resources.
+//
+// A best-effort attempt is made to prune roles that would not allow
+// access to any of the requested resources, this is skipped when any
+// resource is in a leaf cluster.
+//
+// If loginHint is provided, it will attempt to prune the list to a single role.
 func (m *RequestValidator) pruneResourceRequestRoles(
 	ctx context.Context,
 	resourceIDs []types.ResourceID,
@@ -2067,7 +2067,6 @@ func (m *RequestValidator) pruneResourceRequestRoles(
 		return roles, nil
 	}
 
-	var err error
 	var mappedRequestedRolesToAllowedKinds map[string][]string
 	roles, mappedRequestedRolesToAllowedKinds = m.pruneRequestedRolesNotMatchingKubernetesResourceKinds(resourceIDs, roles)
 	if len(roles) == 0 { // all roles got pruned from not matching every kube requested kind.
@@ -2211,18 +2210,14 @@ func getKubeResourceKinds(kubernetesResources []types.RequestKubernetesResource)
 // getAllowedKubeResourceKinds returns only the allowed kinds that were not in the
 // denied list.
 func getAllowedKubeResourceKinds(allowedKinds []string, deniedKinds []string) []string {
-	denied := make(map[string]struct{})
-	for _, kind := range deniedKinds {
-		denied[kind] = struct{}{}
-	}
-
-	allowed := make([]string, 0, len(allowedKinds))
+	allowed := make(map[string]struct{}, len(allowedKinds))
 	for _, kind := range allowedKinds {
-		if _, denied := denied[kind]; !denied {
-			allowed = append(allowed, kind)
-		}
+		allowed[kind] = struct{}{}
+	}
+	for _, kind := range deniedKinds {
+		delete(allowed, kind)
 	}
-	return apiutils.Deduplicate(allowed)
+	return slices.Collect(maps.Keys(allowed))
 }
 
 func (m *RequestValidator) roleAllowsResource(

lib/services/access_request_test.go

@@ -2817,7 +2817,7 @@ func TestValidate_WithAllowRequestKubernetesResources(t *testing.T) {
 			},
 		},
 		{
-			desc:                 "request.kubernetes_resources undefined takes precedence over configured allow filed (allows anything)",
+			desc:                 "request.kubernetes_resources undefined takes precedence over configured allow field (allows anything)",
 			userStaticRoles:      []string{"request-undefined_search-wildcard", "request-secret_search-wildcard"},
 			expectedRequestRoles: []string{"kube-access-wildcard", "db-access-wildcard"},
 			requestResourceIDs: []types.ResourceID{