[v16] tsh: Deduplicate the list of request IDs

lib/services/access_request.go

@@ -46,6 +46,7 @@ import (
 
 const (
 	maxAccessRequestReasonSize = 4096
+	maxResourcesPerRequest     = 300
 
 	// A day is sometimes 23 hours, sometimes 25 hours, usually 24 hours.
 	day = 24 * time.Hour
@@ -67,14 +68,17 @@ func ValidateAccessRequest(ar types.AccessRequest) error {
 
 	_, err := uuid.Parse(ar.GetName())
 	if err != nil {
-		return trace.BadParameter("invalid access request id %q", ar.GetName())
+		return trace.BadParameter("invalid access request ID %q", ar.GetName())
 	}
 	if len(ar.GetRequestReason()) > maxAccessRequestReasonSize {
 		return trace.BadParameter("access request reason is too long, max %v bytes", maxAccessRequestReasonSize)
 	}
 	if len(ar.GetResolveReason()) > maxAccessRequestReasonSize {
 		return trace.BadParameter("access request resolve reason is too long, max %v bytes", maxAccessRequestReasonSize)
 	}
+	if l := len(ar.GetRequestedResourceIDs()); l > maxResourcesPerRequest {
+		return trace.BadParameter("access request contains too many resources (%v), max %v", l, maxResourcesPerRequest)
+	}
 	return nil
 }
 
@@ -1110,7 +1114,6 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
 	// need to be expanded into a list consisting of all existing roles
 	// that the user does not hold and is allowed to request.
 	if r := req.GetRoles(); len(r) == 1 && r[0] == types.Wildcard {
-
 		if !req.GetState().IsPending() {
 			// expansion is only permitted in pending requests.  once resolved,
 			// a request's role list must be immutable.
@@ -1152,6 +1155,19 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
 	}
 
 	if m.opts.expandVars {
+		// deduplicate requested resource IDs
+		var deduplicated []types.ResourceID
+		seen := make(map[string]struct{})
+		for _, resource := range req.GetRequestedResourceIDs() {
+			id := types.ResourceIDToString(resource)
+			if _, isDuplicate := seen[id]; isDuplicate {
+				continue
+			}
+			seen[id] = struct{}{}
+			deduplicated = append(deduplicated, resource)
+		}
+		req.SetRequestedResourceIDs(deduplicated)
+
 		// 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 {

lib/services/access_request_test.go

@@ -792,12 +792,7 @@ func TestMaxLength(t *testing.T) {
 	req, err := types.NewAccessRequest("some-id", "dave", "dictator", "never")
 	require.NoError(t, err)
 
-	var s []byte
-	for i := 0; i <= maxAccessRequestReasonSize; i++ {
-		s = append(s, 'a')
-	}
-
-	req.SetRequestReason(string(s))
+	req.SetRequestReason(strings.Repeat("a", maxAccessRequestReasonSize))
 	require.Error(t, ValidateAccessRequest(req))
 }
 
@@ -2155,6 +2150,70 @@ 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) {
+	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: "someCluster",
+	}
+
+	for i := 1; i < 3; i++ {
+		node, err := types.NewServerWithLabels(
+			fmt.Sprintf("resource%d", i),
+			types.KindNode,
+			types.ServerSpecV2{},
+			map[string]string{"foo": "bar"},
+		)
+		require.NoError(t, err)
+		g.nodes[node.GetName()] = node
+	}
+
+	searchAsRole, err := types.NewRole("searchAs", types.RoleSpecV6{
+		Allow: types.RoleConditions{
+			Logins:     []string{"root"},
+			NodeLabels: types.Labels{"*": []string{"*"}},
+		},
+	})
+	require.NoError(t, err)
+	g.roles[searchAsRole.GetName()] = searchAsRole
+
+	testRole, err := types.NewRole("testRole", types.RoleSpecV6{
+		Allow: types.RoleConditions{
+			Request: &types.AccessRequestConditions{
+				SearchAsRoles: []string{searchAsRole.GetName()},
+			},
+		},
+	})
+	require.NoError(t, err)
+	g.roles[testRole.GetName()] = testRole
+
+	user := g.user(t, testRole.GetName())
+
+	clock := clockwork.NewFakeClock()
+	identity := tlsca.Identity{
+		Expires: clock.Now().UTC().Add(8 * time.Hour),
+	}
+
+	req, err := types.NewAccessRequestWithResources("name", user, nil, /* roles */
+		[]types.ResourceID{
+			{ClusterName: "someCluster", Kind: "node", Name: "resource1"},
+			{ClusterName: "someCluster", Kind: "node", Name: "resource1"}, // a  duplicate
+			{ClusterName: "someCluster", Kind: "node", Name: "resource2"}, // not a duplicate
+		})
+	require.NoError(t, err)
+
+	require.NoError(t, ValidateAccessRequestForUser(context.Background(), clock, g, req, identity, ExpandVars(true)))
+	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]))
+}
+
 func TestValidateAccessRequestClusterNames(t *testing.T) {
 	for _, tc := range []struct {
 		name               string

tool/tsh/common/tsh.go

@@ -2546,6 +2546,7 @@ func createAccessRequest(cf *CLIConf) (types.AccessRequest, error) {
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
+
 	req, err := services.NewAccessRequestWithResources(cf.Username, roles, requestedResourceIDs)
 	if err != nil {
 		return nil, trace.Wrap(err)