From e41ffa0213e3702a756b09a3da96652219883660 Mon Sep 17 00:00:00 2001
From: Zac Bergquist <zac.bergquist@goteleport.com>
Date: Fri, 1 Nov 2024 08:56:26 -0600
Subject: [PATCH] Extend resource access request validation checks (#48138)

In #46780 we put a cap on the total number of resources that can
be requested in a single request, which helps avoid situations
where a large access request can exceed resource size limits and
break request listing.

This change expands on the validations by also checking that the sum
of the lengths of all of the requested IDs stays below a reasonable
limit.
---
 lib/services/access_request.go      | 10 ++++++++++
 lib/services/access_request_test.go | 13 ++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/lib/services/access_request.go b/lib/services/access_request.go
index 4de491ecdcc05..9a62cc22c11f7 100644
--- a/lib/services/access_request.go
+++ b/lib/services/access_request.go
@@ -46,6 +46,7 @@ import (
 const (
 	maxAccessRequestReasonSize = 4096
 	maxResourcesPerRequest     = 300
+	maxResourcesLength         = 2048
 
 	// A day is sometimes 23 hours, sometimes 25 hours, usually 24 hours.
 	day = 24 * time.Hour
@@ -1157,6 +1158,7 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
 		// deduplicate requested resource IDs
 		var deduplicated []types.ResourceID
 		seen := make(map[string]struct{})
+		resourcesLen := 0
 		for _, resource := range req.GetRequestedResourceIDs() {
 			id := types.ResourceIDToString(resource)
 			if _, isDuplicate := seen[id]; isDuplicate {
@@ -1164,9 +1166,17 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
 			}
 			seen[id] = struct{}{}
 			deduplicated = append(deduplicated, resource)
+			resourcesLen += len(id)
 		}
 		req.SetRequestedResourceIDs(deduplicated)
 
+		// In addition to capping the maximum number of resources in a single request,
+		// we also need to ensure that the sum of the resource IDs in the request doesn't
+		// get too big.
+		if resourcesLen > maxResourcesLength {
+			return trace.BadParameter("access request exceeds maximum length: try reducing the number of resources in the request")
+		}
+
 		// determine the roles which should be requested for a resource access
 		// request, and write them to the request
 		if err := m.setRolesForResourceRequest(ctx, req); err != nil {
diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go
index e413c8e66f3bf..5c40d17115ee9 100644
--- a/lib/services/access_request_test.go
+++ b/lib/services/access_request_test.go
@@ -2158,7 +2158,7 @@ func (mcg mockClusterGetter) GetRemoteCluster(ctx context.Context, clusterName s
 	return nil, trace.NotFound("remote cluster %q was not found", clusterName)
 }
 
-func TestValidateDuplicateRequestedResources(t *testing.T) {
+func TestValidateResourceRequestSizeLimits(t *testing.T) {
 	g := &mockGetter{
 		roles:       make(map[string]types.Role),
 		userStates:  make(map[string]*userloginstate.UserLoginState),
@@ -2220,6 +2220,17 @@ func TestValidateDuplicateRequestedResources(t *testing.T) {
 	require.Len(t, req.GetRequestedResourceIDs(), 2)
 	require.Equal(t, "/someCluster/node/resource1", types.ResourceIDToString(req.GetRequestedResourceIDs()[0]))
 	require.Equal(t, "/someCluster/node/resource2", types.ResourceIDToString(req.GetRequestedResourceIDs()[1]))
+
+	var requestedResourceIDs []types.ResourceID
+	for i := 0; i < 200; i++ {
+		requestedResourceIDs = append(requestedResourceIDs, types.ResourceID{
+			ClusterName: "someCluster",
+			Kind:        "node",
+			Name:        "resource" + strconv.Itoa(i),
+		})
+	}
+	req.SetRequestedResourceIDs(requestedResourceIDs)
+	require.ErrorContains(t, ValidateAccessRequestForUser(context.Background(), clock, g, req, identity, ExpandVars(true)), "access request exceeds maximum length")
 }
 
 func TestValidateAccessRequestClusterNames(t *testing.T) {