Skip to content

Commit

Permalink
Extend resource access request validation checks (#48079)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
zmb3 authored Oct 30, 2024
1 parent e3e6211 commit 83333c4
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 1 deletion.
10 changes: 10 additions & 0 deletions lib/services/access_request.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1157,16 +1158,25 @@ 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 {
continue
}
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 {
Expand Down
13 changes: 12 additions & 1 deletion lib/services/access_request_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 83333c4

Please sign in to comment.