From d804784bc38e0dd83184d47edf8338e2093b5ea4 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Thu, 19 Sep 2024 13:48:31 -0600 Subject: [PATCH 1/3] tsh: Deduplicate the list of request IDs It's possible to specify the same request multiple times with tsh request create. The duplicates eventually get resolved before we generate a certificate, but they do exist in the access request resource. This can cause the size of the resource to exceed the limits of a gRPC message and break listing. --- lib/services/access_request.go | 15 +- lib/services/access_request_test.go | 36 ++++ tool/tsh/common/tsh.go | 1 + tool/tsh/common/tsh_test.go | 304 ++++++++++++++-------------- 4 files changed, 201 insertions(+), 155 deletions(-) diff --git a/lib/services/access_request.go b/lib/services/access_request.go index 93ffaae46568..8108af6a7b93 100644 --- a/lib/services/access_request.go +++ b/lib/services/access_request.go @@ -75,6 +75,7 @@ func ValidateAccessRequest(ar types.AccessRequest) error { if len(ar.GetResolveReason()) > maxAccessRequestReasonSize { return trace.BadParameter("access request resolve reason is too long, max %v bytes", maxAccessRequestReasonSize) } + // TODO: consider enforcing a maximum number of requested resources in a single request return nil } @@ -1106,11 +1107,23 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest return trace.BadParameter("only promoted requests can set the promoted access list title") } + // 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) + // check for "wildcard request" (`roles=*`). wildcard requests // 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. diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go index 7facc5ed7c4f..fcd06e1e0b60 100644 --- a/lib/services/access_request_test.go +++ b/lib/services/access_request_test.go @@ -2155,6 +2155,42 @@ 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", + } + testRole, err := types.NewRole("testRole", types.RoleSpecV6{}) + 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 true duplicate + {ClusterName: "someCluster", Kind: "app", Name: "resource1"}, // not a duplicate + }) + require.NoError(t, err) + + require.NoError(t, ValidateAccessRequestForUser(context.Background(), clock, g, req, identity)) + require.Len(t, req.GetRequestedResourceIDs(), 2) + require.Equal(t, "/someCluster/node/resource1", types.ResourceIDToString(req.GetRequestedResourceIDs()[0])) + require.Equal(t, "/someCluster/app/resource1", types.ResourceIDToString(req.GetRequestedResourceIDs()[1])) +} + func TestValidateAccessRequestClusterNames(t *testing.T) { for _, tc := range []struct { name string diff --git a/tool/tsh/common/tsh.go b/tool/tsh/common/tsh.go index 13de3b78de8f..da6b3135ffa8 100644 --- a/tool/tsh/common/tsh.go +++ b/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) diff --git a/tool/tsh/common/tsh_test.go b/tool/tsh/common/tsh_test.go index c2f96b070a3c..9a401c945263 100644 --- a/tool/tsh/common/tsh_test.go +++ b/tool/tsh/common/tsh_test.go @@ -4384,15 +4384,14 @@ func TestSerializeProfilesNoOthers(t *testing.T) { expected := ` { "active": { - "profile_url": "example.com", - "username": "test", - "cluster": "main", - "kubernetes_enabled": false, - "valid_until": "1970-01-01T00:00:00Z" - }, + "profile_url": "example.com", + "username": "test", + "cluster": "main", + "kubernetes_enabled": false, + "valid_until": "1970-01-01T00:00:00Z" + }, "profiles": [] - } - ` + }` aTime := time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC) p, err := url.Parse("example.com") require.NoError(t, err) @@ -4479,26 +4478,25 @@ func TestSerializeAccessRequests(t *testing.T) { expected := ` { - "kind": "access_request", - "version": "v3", - "metadata": { - "name": "test" - }, - "spec": { - "user": "user", - "roles": [ - "a", - "b", - "c" - ], - "state": 1, - "created": "0001-01-01T00:00:00Z", - "expires": "0001-01-01T00:00:00Z", - "max_duration": "0001-01-01T00:00:00Z", - "session_ttl": "0001-01-01T00:00:00Z" - } - } - ` + "kind": "access_request", + "version": "v3", + "metadata": { + "name": "test" + }, + "spec": { + "user": "user", + "roles": [ + "a", + "b", + "c" + ], + "state": 1, + "created": "0001-01-01T00:00:00Z", + "expires": "0001-01-01T00:00:00Z", + "max_duration": "0001-01-01T00:00:00Z", + "session_ttl": "0001-01-01T00:00:00Z" + } + }` req, err := types.NewAccessRequest("test", "user", "a", "b", "c") require.NoError(t, err) testSerialization(t, expected, func(f string) (string, error) { @@ -4516,68 +4514,67 @@ func TestSerializeKubeSessions(t *testing.T) { aTime := time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC) expected := ` [ - { - "kind": "session_tracker", - "version": "v1", - "metadata": { - "name": "id", - "expires": "1970-01-01T00:00:00Z" - }, - "spec": { - "session_id": "id", - "kind": "session-kind", - "state": 1, - "created": "1970-01-01T00:00:00Z", - "expires": "1970-01-01T00:00:00Z", - "attached": "arbitrary attached data", - "reason": "some reason", - "invited": [ - "a", - "b", - "c" - ], - "target_hostname": "example.com", - "target_address": "https://example.com", - "cluster_name": "cluster", - "login": "login", - "participants": [ - { - "id": "some-id", - "user": "test", - "mode": "mode", - "last_active": "1970-01-01T00:00:00Z" - } - ], - "kubernetes_cluster": "kc", - "host_user": "test", - "host_roles": [ - { - "name": "policy", - "version": "v1", - "require_session_join": [ - { - "name": "policy", - "filter": "filter", - "kinds": [ - "x", - "y", - "z" - ], - "count": 1, - "modes": [ - "mode", - "mode-1", - "mode-2" - ], - "on_leave": "do something" - } - ] - } - ] - } - } -] - ` + { + "kind": "session_tracker", + "version": "v1", + "metadata": { + "name": "id", + "expires": "1970-01-01T00:00:00Z" + }, + "spec": { + "session_id": "id", + "kind": "session-kind", + "state": 1, + "created": "1970-01-01T00:00:00Z", + "expires": "1970-01-01T00:00:00Z", + "attached": "arbitrary attached data", + "reason": "some reason", + "invited": [ + "a", + "b", + "c" + ], + "target_hostname": "example.com", + "target_address": "https://example.com", + "cluster_name": "cluster", + "login": "login", + "participants": [ + { + "id": "some-id", + "user": "test", + "mode": "mode", + "last_active": "1970-01-01T00:00:00Z" + } + ], + "kubernetes_cluster": "kc", + "host_user": "test", + "host_roles": [ + { + "name": "policy", + "version": "v1", + "require_session_join": [ + { + "name": "policy", + "filter": "filter", + "kinds": [ + "x", + "y", + "z" + ], + "count": 1, + "modes": [ + "mode", + "mode-1", + "mode-2" + ], + "on_leave": "do something" + } + ] + } + ] + } + } + ]` tracker, err := types.NewSessionTracker(types.SessionTrackerSpecV1{ SessionID: "id", Kind: "session-kind", @@ -4681,7 +4678,7 @@ func TestSerializeMFADevices(t *testing.T) { aTime := time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC) expected := ` [ - {"metadata":{"Name":"my device"},"id":"id","addedAt":"1970-01-01T00:00:00Z","lastUsed":"1970-01-01T00:00:00Z"} + {"metadata":{"Name":"my device"},"id":"id","addedAt":"1970-01-01T00:00:00Z","lastUsed":"1970-01-01T00:00:00Z"} ] ` dev := types.NewMFADevice("my device", "id", aTime) @@ -5121,68 +5118,67 @@ func TestShowSessions(t *testing.T) { t.Parallel() expected := `[ - { - "ei": 0, - "event": "", - "uid": "someID1", - "time": "0001-01-01T00:00:00Z", - "sid": "", - "server_id": "", - "enhanced_recording": false, - "interactive": false, - "participants": [ - "someParticipant" - ], - "session_start": "0001-01-01T00:00:00Z", - "session_stop": "0001-01-01T00:00:00Z" - }, - { - "ei": 0, - "event": "", - "uid": "someID2", - "time": "0001-01-01T00:00:00Z", - "sid": "", - "server_id": "", - "enhanced_recording": false, - "interactive": false, - "participants": [ - "someParticipant" - ], - "session_start": "0001-01-01T00:00:00Z", - "session_stop": "0001-01-01T00:00:00Z" - }, - { - "ei": 0, - "event": "", - "uid": "someID3", - "time": "0001-01-01T00:00:00Z", - "sid": "", - "windows_desktop_service": "", - "desktop_addr": "", - "windows_domain": "", - "windows_user": "", - "desktop_labels": null, - "session_start": "0001-01-01T00:00:00Z", - "session_stop": "0001-01-01T00:00:00Z", - "desktop_name": "", - "recorded": false, - "participants": [ - "someParticipant" - ] - }, - { - "ei": 0, - "event": "", - "uid": "someID4", - "time": "0001-01-01T00:00:00Z", - "user": "someUser", - "sid": "", - "db_protocol": "postgres", - "db_uri": "", - "session_start": "0001-01-01T00:00:00Z", - "session_stop": "0001-01-01T00:00:00Z" - } -]` + { + "ei": 0, + "event": "", + "uid": "someID1", + "time": "0001-01-01T00:00:00Z", + "sid": "", + "server_id": "", + "enhanced_recording": false, + "interactive": false, + "participants": [ + "someParticipant" + ], + "session_start": "0001-01-01T00:00:00Z", + "session_stop": "0001-01-01T00:00:00Z" + }, + { + "ei": 0, + "event": "", + "uid": "someID2", + "time": "0001-01-01T00:00:00Z", + "sid": "", + "server_id": "", + "enhanced_recording": false, + "interactive": false, + "participants": [ + "someParticipant" + ], + "session_start": "0001-01-01T00:00:00Z", + "session_stop": "0001-01-01T00:00:00Z" + }, + { + "ei": 0, + "event": "", + "uid": "someID3", + "time": "0001-01-01T00:00:00Z", + "sid": "", + "windows_desktop_service": "", + "desktop_addr": "", + "windows_domain": "", + "windows_user": "", + "desktop_labels": null, + "session_start": "0001-01-01T00:00:00Z", + "session_stop": "0001-01-01T00:00:00Z", + "desktop_name": "", + "recorded": false, + "participants": [ + "someParticipant" + ] + }, + { + "ei": 0, + "event": "", + "uid": "someID4", + "time": "0001-01-01T00:00:00Z", + "user": "someUser", + "sid": "", + "db_protocol": "postgres", + "db_uri": "", + "session_start": "0001-01-01T00:00:00Z", + "session_stop": "0001-01-01T00:00:00Z" + } ]` sessions := []events.AuditEvent{ &events.SessionEnd{ Metadata: events.Metadata{ @@ -5225,7 +5221,7 @@ func TestShowSessions(t *testing.T) { var buf bytes.Buffer err := common.ShowSessions(sessions, teleport.JSON, &buf) require.NoError(t, err) - require.Equal(t, expected, buf.String()) + require.JSONEq(t, expected, buf.String()) } func TestMakeProfileInfo_NoInternalLogins(t *testing.T) { From 05f24f883ba58e84a8f5127e22f9f905fe707751 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Wed, 2 Oct 2024 09:48:08 -0600 Subject: [PATCH 2/3] Limit access requests to a maximum of 300 resources --- lib/services/access_request.go | 7 +++++-- lib/services/access_request_test.go | 13 ++++++++----- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/services/access_request.go b/lib/services/access_request.go index 8108af6a7b93..d9343e20168f 100644 --- a/lib/services/access_request.go +++ b/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,7 +68,7 @@ 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) @@ -75,7 +76,9 @@ func ValidateAccessRequest(ar types.AccessRequest) error { if len(ar.GetResolveReason()) > maxAccessRequestReasonSize { return trace.BadParameter("access request resolve reason is too long, max %v bytes", maxAccessRequestReasonSize) } - // TODO: consider enforcing a maximum number of requested resources in a single request + if l := len(ar.GetRequestedResourceIDs()); l > maxResourcesPerRequest { + return trace.BadParameter("access request contains too many resources (%v), max %v", l, maxResourcesPerRequest) + } return nil } diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go index fcd06e1e0b60..7268ec3223a0 100644 --- a/lib/services/access_request_test.go +++ b/lib/services/access_request_test.go @@ -21,6 +21,7 @@ package services import ( "context" "fmt" + "slices" "strconv" "strings" "testing" @@ -792,12 +793,14 @@ 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(strings.Repeat("a", maxAccessRequestReasonSize)) + require.Error(t, ValidateAccessRequest(req)) + + resourceIDs, err := types.ResourceIDsFromString(`["/cluster/node/some-node", "/cluster/node/some-other-node" ]`) + require.NoError(t, err) - req.SetRequestReason(string(s)) + req.SetRequestReason("") + req.SetRequestedResourceIDs(slices.Repeat(resourceIDs, maxResourcesPerRequest/2+1)) require.Error(t, ValidateAccessRequest(req)) } From c93999971ec472e4058973dc682544b028951650 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Wed, 2 Oct 2024 14:52:26 -0600 Subject: [PATCH 3/3] Only run deduplication when the request is being created --- lib/services/access_request.go | 26 ++++++++-------- lib/services/access_request_test.go | 46 +++++++++++++++++++++-------- 2 files changed, 46 insertions(+), 26 deletions(-) diff --git a/lib/services/access_request.go b/lib/services/access_request.go index d9343e20168f..2c78aacad0cb 100644 --- a/lib/services/access_request.go +++ b/lib/services/access_request.go @@ -1110,19 +1110,6 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest return trace.BadParameter("only promoted requests can set the promoted access list title") } - // 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) - // check for "wildcard request" (`roles=*`). wildcard requests // need to be expanded into a list consisting of all existing roles // that the user does not hold and is allowed to request. @@ -1168,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 { diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go index 7268ec3223a0..1d1712bb6f72 100644 --- a/lib/services/access_request_test.go +++ b/lib/services/access_request_test.go @@ -21,7 +21,6 @@ package services import ( "context" "fmt" - "slices" "strconv" "strings" "testing" @@ -795,13 +794,6 @@ func TestMaxLength(t *testing.T) { req.SetRequestReason(strings.Repeat("a", maxAccessRequestReasonSize)) require.Error(t, ValidateAccessRequest(req)) - - resourceIDs, err := types.ResourceIDsFromString(`["/cluster/node/some-node", "/cluster/node/some-other-node" ]`) - require.NoError(t, err) - - req.SetRequestReason("") - req.SetRequestedResourceIDs(slices.Repeat(resourceIDs, maxResourcesPerRequest/2+1)) - require.Error(t, ValidateAccessRequest(req)) } // TestThresholdReviewFilter verifies basic filter syntax. @@ -2170,9 +2162,37 @@ func TestValidateDuplicateRequestedResources(t *testing.T) { desktops: make(map[string]types.WindowsDesktop), clusterName: "someCluster", } - testRole, err := types.NewRole("testRole", types.RoleSpecV6{}) + + 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() @@ -2183,15 +2203,15 @@ func TestValidateDuplicateRequestedResources(t *testing.T) { req, err := types.NewAccessRequestWithResources("name", user, nil, /* roles */ []types.ResourceID{ {ClusterName: "someCluster", Kind: "node", Name: "resource1"}, - {ClusterName: "someCluster", Kind: "node", Name: "resource1"}, // a true duplicate - {ClusterName: "someCluster", Kind: "app", Name: "resource1"}, // not a duplicate + {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)) + 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/app/resource1", types.ResourceIDToString(req.GetRequestedResourceIDs()[1])) + require.Equal(t, "/someCluster/node/resource2", types.ResourceIDToString(req.GetRequestedResourceIDs()[1])) } func TestValidateAccessRequestClusterNames(t *testing.T) {