From ee26fefc5e72373d6512c9ada03b81dcdc1e0745 Mon Sep 17 00:00:00 2001 From: "Sorawis Nilparuk (Bo)" Date: Mon, 23 Jun 2025 14:03:33 -0700 Subject: [PATCH] bug: fix list target permissions resource ID overriding all resource grants (#5877) * fix list target not handling all resources permissions * add test * fix same bug for session package * split target tests to two separate tests --- .../handlers/sessions/grants_test.go | 47 ++++++++++++++--- .../handlers/targets/tcp/grants_test.go | 50 +++++++++++++++++++ internal/session/repository.go | 2 +- internal/target/repository.go | 2 +- 4 files changed, 91 insertions(+), 10 deletions(-) diff --git a/internal/daemon/controller/handlers/sessions/grants_test.go b/internal/daemon/controller/handlers/sessions/grants_test.go index f1c6d02fc4..6a2f95d24b 100644 --- a/internal/daemon/controller/handlers/sessions/grants_test.go +++ b/internal/daemon/controller/handlers/sessions/grants_test.go @@ -135,6 +135,7 @@ func TestGrants_ReadActions(t *testing.T) { sessionRepo, err := sessionRepoFn() require.NoError(t, err) proj1Session := testSession(t, conn, kmsCache, wrap, targetRepo, sessionRepo, proj1.PublicId, true) + proj1Session2 := testSession(t, conn, kmsCache, wrap, targetRepo, sessionRepo, proj1.PublicId, true) proj2Session := testSession(t, conn, kmsCache, wrap, targetRepo, sessionRepo, proj2.PublicId, true) proj3Session := testSession(t, conn, kmsCache, wrap, targetRepo, sessionRepo, proj3.PublicId, false) @@ -306,9 +307,10 @@ func TestGrants_ReadActions(t *testing.T) { }, }), idOutputFieldsMap: map[string][]string{ - proj1Session.PublicId: {globals.IdField, globals.TargetIdField, globals.ScopeField, globals.CreatedTimeField, globals.UpdatedTimeField}, - proj2Session.PublicId: {globals.IdField, globals.TargetIdField, globals.ScopeField, globals.CreatedTimeField, globals.UpdatedTimeField}, - proj3Session.PublicId: {globals.IdField, globals.TargetIdField, globals.ScopeField, globals.CreatedTimeField, globals.UpdatedTimeField}, + proj1Session.PublicId: {globals.IdField, globals.TargetIdField, globals.ScopeField, globals.CreatedTimeField, globals.UpdatedTimeField}, + proj1Session2.PublicId: {globals.IdField, globals.TargetIdField, globals.ScopeField, globals.CreatedTimeField, globals.UpdatedTimeField}, + proj2Session.PublicId: {globals.IdField, globals.TargetIdField, globals.ScopeField, globals.CreatedTimeField, globals.UpdatedTimeField}, + proj3Session.PublicId: {globals.IdField, globals.TargetIdField, globals.ScopeField, globals.CreatedTimeField, globals.UpdatedTimeField}, }, }, { @@ -326,7 +328,8 @@ func TestGrants_ReadActions(t *testing.T) { IncludeTerminated: true, }, idOutputFieldsMap: map[string][]string{ - proj1Session.PublicId: {globals.IdField, globals.ExpirationTimeField, globals.AuthTokenIdField, globals.UserIdField, globals.HostSetIdField, globals.HostIdsField}, + proj1Session.PublicId: {globals.IdField, globals.ExpirationTimeField, globals.AuthTokenIdField, globals.UserIdField, globals.HostSetIdField, globals.HostIdsField}, + proj1Session2.PublicId: {globals.IdField, globals.ExpirationTimeField, globals.AuthTokenIdField, globals.UserIdField, globals.HostSetIdField, globals.HostIdsField}, }, }, { @@ -363,7 +366,8 @@ func TestGrants_ReadActions(t *testing.T) { }, }), idOutputFieldsMap: map[string][]string{ - proj1Session.PublicId: {globals.IdField, globals.VersionField, globals.TypeField, globals.ScopeIdField, globals.EndpointField, globals.StatesField, globals.StatusField, globals.CertificateField, globals.AuthorizedActionsField}, + proj1Session.PublicId: {globals.IdField, globals.VersionField, globals.TypeField, globals.ScopeIdField, globals.EndpointField, globals.StatesField, globals.StatusField, globals.CertificateField, globals.AuthorizedActionsField}, + proj1Session2.PublicId: {globals.IdField, globals.VersionField, globals.TypeField, globals.ScopeIdField, globals.EndpointField, globals.StatesField, globals.StatusField, globals.CertificateField, globals.AuthorizedActionsField}, }, }, { @@ -391,9 +395,36 @@ func TestGrants_ReadActions(t *testing.T) { }, }), idOutputFieldsMap: map[string][]string{ - proj1Session.PublicId: {globals.IdField, globals.AuthorizedActionsField}, - proj2Session.PublicId: {globals.IdField, globals.VersionField}, - proj3Session.PublicId: {globals.IdField, globals.StateField}, + proj1Session.PublicId: {globals.IdField, globals.AuthorizedActionsField}, + proj1Session2.PublicId: {globals.IdField, globals.AuthorizedActionsField}, + proj2Session.PublicId: {globals.IdField, globals.VersionField}, + proj3Session.PublicId: {globals.IdField, globals.StateField}, + }, + }, + { + name: "iss 5003 resource id grant should not override all ids", + input: &pbs.ListSessionsRequest{ + ScopeId: globals.GlobalPrefix, + Recursive: true, + IncludeTerminated: true, + }, + userFunc: iam.TestUserManagedGroupGrantsFunc(t, conn, kmsCache, globals.GlobalPrefix, ldap.TestAuthMethodWithAccountInManagedGroup, []iam.TestRoleGrantsRequest{ + { + RoleScopeId: globals.GlobalPrefix, + Grants: []string{"id=*;type=*;actions=*;output_fields=id,authorized_actions"}, + GrantScopes: []string{globals.GrantScopeThis, globals.GrantScopeDescendants}, + }, + { + RoleScopeId: globals.GlobalPrefix, + Grants: []string{fmt.Sprintf("ids=%s;actions=*;output_fields=id,version", proj1Session2.PublicId)}, + GrantScopes: []string{globals.GrantScopeThis, globals.GrantScopeDescendants}, + }, + }), + idOutputFieldsMap: map[string][]string{ + proj1Session.PublicId: {globals.IdField, globals.AuthorizedActionsField}, + proj1Session2.PublicId: {globals.IdField, globals.AuthorizedActionsField}, + proj2Session.PublicId: {globals.IdField, globals.AuthorizedActionsField}, + proj3Session.PublicId: {globals.IdField, globals.AuthorizedActionsField}, }, }, { diff --git a/internal/daemon/controller/handlers/targets/tcp/grants_test.go b/internal/daemon/controller/handlers/targets/tcp/grants_test.go index c05c268a28..d1af452356 100644 --- a/internal/daemon/controller/handlers/targets/tcp/grants_test.go +++ b/internal/daemon/controller/handlers/targets/tcp/grants_test.go @@ -164,6 +164,56 @@ func TestGrants_ReadActions(t *testing.T) { userFunc: iam.TestUserDirectGrantsFunc(t, conn, kmsCache, globals.GlobalPrefix, password.TestAuthMethodWithAccount, []iam.TestRoleGrantsRequest{}), wantErr: handlers.ForbiddenError(), }, + { + name: "iss 5003 less permissive grants should not override more permissive grants with specific type", + input: &pbs.ListTargetsRequest{ + ScopeId: globals.GlobalPrefix, + Recursive: true, + }, + userFunc: iam.TestUserGroupGrantsFunc(t, conn, kmsCache, globals.GlobalPrefix, password.TestAuthMethodWithAccount, []iam.TestRoleGrantsRequest{ + { + RoleScopeId: globals.GlobalPrefix, + Grants: []string{fmt.Sprintf("ids=%s;actions=authorize-session;output_fields=id,authorized_actions,scope_id,address", target1.GetPublicId())}, + GrantScopes: []string{globals.GrantScopeDescendants}, + }, + { + RoleScopeId: globals.GlobalPrefix, + Grants: []string{"ids=*;type=target;actions=read,list;output_fields=id,authorized_actions,scope_id,address"}, + GrantScopes: []string{globals.GrantScopeDescendants}, + }, + }), + wantErr: nil, + wantIdOutputFields: map[string][]string{ + target1.GetPublicId(): {globals.IdField, globals.AuthorizedActionsField, globals.ScopeIdField, globals.AddressField}, + target2.GetPublicId(): {globals.IdField, globals.AuthorizedActionsField, globals.ScopeIdField, globals.AddressField}, + target3.GetPublicId(): {globals.IdField, globals.AuthorizedActionsField, globals.ScopeIdField, globals.AddressField}, + }, + }, + { + name: "iss 5003 less permissive grants should not override more permissive grants", + input: &pbs.ListTargetsRequest{ + ScopeId: globals.GlobalPrefix, + Recursive: true, + }, + userFunc: iam.TestUserGroupGrantsFunc(t, conn, kmsCache, globals.GlobalPrefix, password.TestAuthMethodWithAccount, []iam.TestRoleGrantsRequest{ + { + RoleScopeId: globals.GlobalPrefix, + Grants: []string{"ids=*;type=*;actions=*;output_fields=id,authorized_actions,scope_id,address"}, + GrantScopes: []string{globals.GrantScopeDescendants}, + }, + { + RoleScopeId: globals.GlobalPrefix, + Grants: []string{fmt.Sprintf("ids=%s;actions=authorize-session;output_fields=id,authorized_actions,scope_id,address", target1.GetPublicId())}, + GrantScopes: []string{globals.GrantScopeDescendants}, + }, + }), + wantErr: nil, + wantIdOutputFields: map[string][]string{ + target1.GetPublicId(): {globals.IdField, globals.AuthorizedActionsField, globals.ScopeIdField, globals.AddressField}, + target2.GetPublicId(): {globals.IdField, globals.AuthorizedActionsField, globals.ScopeIdField, globals.AddressField}, + target3.GetPublicId(): {globals.IdField, globals.AuthorizedActionsField, globals.ScopeIdField, globals.AddressField}, + }, + }, } for _, tc := range testcases { diff --git a/internal/session/repository.go b/internal/session/repository.go index 79a266eb2c..5a4b0f9567 100644 --- a/internal/session/repository.go +++ b/internal/session/repository.go @@ -100,7 +100,7 @@ func (r *Repository) listPermissionWhereClauses() ([]string, []any) { clauses = append(clauses, fmt.Sprintf("project_id = @project_id_%d", inClauseCnt)) args = append(args, sql.Named(fmt.Sprintf("project_id_%d", inClauseCnt), p.GrantScopeId)) - if len(p.ResourceIds) > 0 { + if len(p.ResourceIds) > 0 && !p.All { clauses = append(clauses, fmt.Sprintf("public_id = any(@public_id_%d)", inClauseCnt)) args = append(args, sql.Named(fmt.Sprintf("public_id_%d", inClauseCnt), "{"+strings.Join(p.ResourceIds, ",")+"}")) } diff --git a/internal/target/repository.go b/internal/target/repository.go index d1ec66708c..a79c3932f9 100644 --- a/internal/target/repository.go +++ b/internal/target/repository.go @@ -390,7 +390,7 @@ func (r *Repository) listPermissionWhereClauses() ([]string, []any) { clauses = append(clauses, fmt.Sprintf("project_id = @project_id_%d", inClauseCnt)) args = append(args, sql.Named(fmt.Sprintf("project_id_%d", inClauseCnt), p.GrantScopeId)) - if len(p.ResourceIds) > 0 { + if len(p.ResourceIds) > 0 && !p.All { clauses = append(clauses, fmt.Sprintf("public_id = any(@public_id_%d)", inClauseCnt)) args = append(args, sql.Named(fmt.Sprintf("public_id_%d", inClauseCnt), "{"+strings.Join(p.ResourceIds, ",")+"}")) }