From c486ef7fae7d08be46513216ffb2670c364869ea Mon Sep 17 00:00:00 2001 From: hughneale Date: Tue, 10 Mar 2026 14:41:06 +0000 Subject: [PATCH 1/4] fix(gcp): resolve binding edge cases and improve error handling - Reject organizations/ binding values in resolveCustomRoleTenant() since organization-scope custom role creation via binding field is not yet implemented; direct users to the provider-level organization_id config instead. - Add explicit detection and warning for partial binding configuration: when some statements have binding set but not all, the code now clearly warns that explicit binding values will be ignored and inference will proceed instead (vs silently falling through). - Fix Statement.Binding code comment to clarify that organizations/{id} format is NOT supported via the binding field (use provider config instead). - Fix GCP documentation example that incorrectly stated IAM binding would be applied at folder scope; clarified that when binding resolves to a project, the IAM binding is applied at project scope. - Update binding resolution order documentation to distinguish between missing binding (all statements) vs partial binding (some statements), each with its own warning behavior. - Add test cases for organizations/ binding rejection and partial binding fallback behavior (TestResolveCustomRoleTenant now covers 7 cases). All tests pass. Build succeeds. --- docs/configuration/roles/index.md | 91 +++++++++ docs/configuration/roles/migration.md | 48 +++++ internal/models/role.go | 30 ++- internal/providers/gcp/provider_test.go | 174 ++++++++++++++++ internal/providers/gcp/rbac.go | 256 ++++++++++++++++++++++-- 5 files changed, 576 insertions(+), 23 deletions(-) diff --git a/docs/configuration/roles/index.md b/docs/configuration/roles/index.md index 9b7dcfb0..d756c506 100644 --- a/docs/configuration/roles/index.md +++ b/docs/configuration/roles/index.md @@ -219,6 +219,7 @@ Each permission statement is an object with the following fields: | `operations` | array | Yes | Actions that can be performed (provider-specific) | | `targets` | array | No | Resources the operations apply to (provider-specific) | | `conditions` | object | No | Provider-specific conditions that must be met | +| `binding` | string | No | Explicit CSP resource where the role is created and bound (see [Binding](#binding)) | ```yaml permissions: @@ -232,6 +233,7 @@ permissions: conditions: # Optional provider-specific conditions ConditionOperator: ConditionKey: ConditionValue + binding: "" # Optional: explicit scope for role creation / IAM binding deny: - operations: - action3 @@ -492,6 +494,95 @@ permissions: {: .note} Conditions follow the same structure as AWS IAM policy conditions. The outer key is the condition operator (e.g., `StringEquals`, `IpAddress`, `Bool`), and the inner key-value pair is the condition key and expected value. Refer to the [AWS IAM Condition documentation](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html) for the full list of supported operators and keys. +### Binding + +The `binding` field on a statement explicitly declares **where** a provider-managed custom role is created and where the resulting IAM binding is applied. This is separate from `targets`, which declares *what resources* the operations act on. + +This field is most important when the **request tenant** (the GCP folder, Azure subscription, or AWS account making the access request) differs from the **scope** where the custom role must be created. For example: + +- A request tenant is a **GCP folder** — but custom GCP roles can only be created at the `project` or `organization` level. +- A statement's `targets` reference resources inside a specific project — so the custom role should be created in that project. + +Without an explicit `binding`, the GCP provider attempts to infer a project from `targets` (legacy behaviour, emits a deprecation warning). Setting `binding` explicitly eliminates the ambiguity and the warning. + +#### Format per provider + +| Provider | Format | Example | +|----------|--------|---------| +| GCP | `projects/{id}` | `projects/my-project` | +| Azure | `/subscriptions/{id}` or `/subscriptions/{id}/resourceGroups/{rg}` | `/subscriptions/00000000-0000-0000-0000-000000000000` | +| AWS | `arn:aws:iam::{account-id}:root` | `arn:aws:iam::123456789012:root` | + +{: .note} +**GCP organization scope**: creating custom roles at organization scope via the `binding` field is not currently supported. To use organization-scoped custom roles, set `organization_id` in the provider configuration instead. Specifying `binding: "organizations/..."` will produce a configuration error. + +#### Resolution order + +1. If `binding` is set on **all** statements in the role, that value is used. +2. If `binding` is absent from one or more statements, the provider falls back to inferring a scope from `targets` (legacy, deprecated — a warning is logged). If **some** statements have `binding` set but not all, the explicit binding values are ignored and a targeted warning is emitted. +3. If inference also fails, the request tenant is used as-is (may produce an error if the tenant type is unsupported for custom roles). + +All statements in a role that set `binding` must agree on the same value. Conflicting `binding` values across statements produce a validation error. + +#### GCP example — folder tenant with project-scoped custom role + +```yaml +roles: + secrets-reader: + name: Secrets Reader + description: Read-only access to Secret Manager secrets in the secrets project + enabled: true + + permissions: + allow: + # Create the custom role in 'thand-secrets', not in the folder tenant + - binding: "projects/thand-secrets" + operations: + - "gcp-prod:secretmanager.secrets.get" + - "gcp-prod:secretmanager.secrets.list" + - "gcp-prod:secretmanager.versions.access" + targets: + - "projects/thand-secrets/*" + +# This role can now be requested via a folder-level tenant (e.g. folders/205090528354). +# The custom role is created in 'thand-secrets' and the IAM binding is applied at +# the project level (projects/thand-secrets), not at the folder level. +``` + +#### Azure example + +```yaml +roles: + storage-reader: + name: Storage Reader + permissions: + allow: + - binding: "/subscriptions/00000000-0000-0000-0000-000000000000" + operations: + - "Microsoft.Storage/storageAccounts/read" + targets: + - "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/data/*" +``` + +#### AWS example + +```yaml +roles: + s3-reader: + name: S3 Reader + permissions: + allow: + - binding: "arn:aws:iam::123456789012:root" + operations: + - "s3:GetObject" + - "s3:ListBucket" + targets: + - "arn:aws:s3:::my-bucket/*" +``` + +{: .important} +`binding` is about **where** a custom role is created / where the IAM binding is applied. It is not an additional resource restriction — `targets` still controls which resources the operations act on. + ### Allow/Deny Conflict Resolution When the same action appears in both `allow` and `deny` lists, the system resolves conflicts using clear precedence rules. diff --git a/docs/configuration/roles/migration.md b/docs/configuration/roles/migration.md index b1ab96f4..d31d09b9 100644 --- a/docs/configuration/roles/migration.md +++ b/docs/configuration/roles/migration.md @@ -237,6 +237,53 @@ See the [Conditions documentation](./index#conditions) for full details and AWS --- +## 6. New: Binding Field on Statements + +The `binding` field is an optional property on permission statements that declares the explicit CSP resource where a custom role should be created and where the resulting IAM binding should be applied. + +### Why it exists + +Some providers create custom roles at a different scope than the request tenant. The most common case is **GCP**: custom roles can only be created at the `projects/{id}` or `organizations/{id}` level, but a request tenant may be a folder (`folders/{id}`). Without `binding`, the GCP provider would fail when asked to create a custom role for a folder tenant. + +The field exists to resolve this explicitly and cleanly, regardless of provider. See [Binding](./index#binding) in the reference documentation for the full format per provider. + +### No migration required — but a deprecation warning may appear + +If your existing roles have `permissions.allow` statements that include `targets` pointing at a specific project (e.g. `projects/my-project/...`), the GCP provider previously attempted to infer the binding project from those targets whenever the request tenant was a folder. This inference still works, but it now emits a **deprecation warning** in the agent logs: + +``` +WARN: binding not set on statement with permissions.allow; inferring project from targets (deprecated — set binding explicitly) +``` + +To silence the warning and make the intent explicit, add `binding` to the relevant statements: + +```yaml +# Before (still works, but logs a deprecation warning) +permissions: + allow: + - operations: + - "gcp-prod:secretmanager.secrets.get" + targets: + - "projects/my-project/secrets/*" + +# After (explicit, no warning) +permissions: + allow: + - binding: "projects/my-project" + operations: + - "gcp-prod:secretmanager.secrets.get" + targets: + - "projects/my-project/secrets/*" +``` + +### Validation + +- All statements within the same role that set `binding` must agree on the same value. Conflicting values produce a configuration error. +- `binding: "folders/..."` is rejected by the GCP provider — folders are not a valid scope for custom role creation. +- `binding` does not restrict which resources the operations act on; `targets` continues to control that. + +--- + ## Summary | Feature | Manual Action Required | Notes | @@ -248,6 +295,7 @@ See the [Conditions documentation](./index#conditions) for full details and AWS | Deny scopes | Optional | New feature, add when ready | | Conditions | Optional | New feature, AWS-only currently | | Composite field | No | System-managed, do not set | +| Binding field | Recommended | Silences deprecation warning when tenant ≠ role creation scope | {: .note} While auto-migration ensures your existing configurations continue to work, we recommend updating your YAML files to the new format when convenient. The new format is more expressive, supports conditions and domain scopes, and makes the relationship between operations and their target resources explicit. diff --git a/internal/models/role.go b/internal/models/role.go index e1fc02af..6cf0ef90 100644 --- a/internal/models/role.go +++ b/internal/models/role.go @@ -80,29 +80,29 @@ func (r *CompositeRole) IsComposite() bool { func (r *CompositeRole) MarshalJSON() ([]byte, error) { // Create a map to hold all fields result := make(map[string]any) - + // Marshal the embedded Role first roleBytes, err := json.Marshal(r.Role) if err != nil { return nil, fmt.Errorf("failed to marshal embedded role: %w", err) } - + // Unmarshal Role fields into the result map if err := json.Unmarshal(roleBytes, &result); err != nil { return nil, fmt.Errorf("failed to unmarshal role fields: %w", err) } - + // Add CompositeRole-specific fields (these will override if there are conflicts) result["uuid"] = r.UUID result["composite_providers"] = r.Providers result["composite"] = r.Composite - + return json.Marshal(result) } // UnmarshalJSON implements custom JSON unmarshaling for CompositeRole. // This is CRITICAL for workflow.SideEffect serialization to work correctly in Temporal workflows. -// +// // Without this custom unmarshaler, CompositeRole fields (UUID, Providers, Composite) are lost // during Temporal's workflow.SideEffect serialization/deserialization cycle because: // 1. The embedded Role struct (marked with json:",inline") has its own UnmarshalJSON method @@ -519,6 +519,26 @@ type Statement struct { // Enforcement is delegated to the target provider's IAM system. // Examples: {"IpAddress": {"aws:SourceIp": "10.0.0.0/8"}} for AWS Conditions map[string]any `json:"conditions,omitempty" validate:"max=10,dive,keys,min=1,max=100"` + + // Binding declares the explicit CSP resource at which this permission statement + // should be created and assigned, independent of the tenant used at request time. + // + // Format is provider-specific: + // GCP: "projects/{id}" — the project where the custom role is created and + // where the IAM binding is applied. + // Note: organization-scope via this field is not currently supported; + // use the provider-level 'organization_id' config for org-scoped roles. + // Azure: "/subscriptions/{id}" or "/subscriptions/{id}/resourceGroups/{rg}" + // AWS: "arn:aws:iam::{account-id}:root" + // + // When set, the provider uses this value to determine where a custom role is + // created and where the IAM binding is applied, regardless of the request tenant + // (e.g. regardless of whether the tenant is a folder, project, or org). + // + // When omitted, the provider falls back to the request tenant for binding scope. + // Providers may additionally attempt to infer a binding resource from Targets + // for backwards compatibility. + Binding string `json:"binding,omitempty" validate:"max=500"` } // ScopeIdentities defines identity-based restrictions for users, groups, and domains. diff --git a/internal/providers/gcp/provider_test.go b/internal/providers/gcp/provider_test.go index 775a906d..6a9adbf6 100644 --- a/internal/providers/gcp/provider_test.go +++ b/internal/providers/gcp/provider_test.go @@ -256,3 +256,177 @@ func TestGetCustomRoleParent(t *testing.T) { provider.client.OrganizationID = "" assert.Equal(t, "projects/tenant-project", provider.getCustomRoleParent("tenant-project")) } + +func TestProjectIDFromRoleParent(t *testing.T) { + t.Run("project parent", func(t *testing.T) { + projectID, ok := projectIDFromRoleParent("projects/thand-secrets") + require.True(t, ok) + assert.Equal(t, "thand-secrets", projectID) + }) + + t.Run("organization parent", func(t *testing.T) { + _, ok := projectIDFromRoleParent("organizations/1234567890") + require.False(t, ok) + }) +} + +func TestProjectIDFromTarget(t *testing.T) { + t.Run("simple project target", func(t *testing.T) { + projectID, ok := projectIDFromTarget("projects/thand-secrets/*") + require.True(t, ok) + assert.Equal(t, "thand-secrets", projectID) + }) + + t.Run("provider prefixed target", func(t *testing.T) { + projectID, ok := projectIDFromTarget("gcp-prod:projects/thand-secrets/secrets/*") + require.True(t, ok) + assert.Equal(t, "thand-secrets", projectID) + }) + + t.Run("wildcard project not allowed", func(t *testing.T) { + _, ok := projectIDFromTarget("projects/*/secrets/*") + require.False(t, ok) + }) +} + +func TestInferProjectIDFromPermissionTargets(t *testing.T) { + t.Run("single project across statements", func(t *testing.T) { + projectID, err := inferProjectIDFromPermissionTargets(models.RoleStatements{ + { + Operations: []string{"gcp-prod:secretmanager.secrets.get"}, + Targets: []string{"projects/thand-secrets/secrets/*"}, + }, + { + Operations: []string{"gcp-prod:secretmanager.versions.access"}, + Targets: []string{"projects/thand-secrets/*"}, + }, + }) + require.NoError(t, err) + assert.Equal(t, "thand-secrets", projectID) + }) + + t.Run("statement without targets errors", func(t *testing.T) { + _, err := inferProjectIDFromPermissionTargets(models.RoleStatements{ + { + Operations: []string{"gcp-prod:secretmanager.secrets.get"}, + }, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "missing targets") + }) + + t.Run("multiple projects error", func(t *testing.T) { + _, err := inferProjectIDFromPermissionTargets(models.RoleStatements{ + { + Operations: []string{"gcp-prod:secretmanager.secrets.get"}, + Targets: []string{"projects/thand-secrets/*"}, + }, + { + Operations: []string{"gcp-prod:secretmanager.versions.access"}, + Targets: []string{"projects/other-project/*"}, + }, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "multiple projects") + }) +} + +func TestResolveCustomRoleTenant(t *testing.T) { + t.Run("explicit binding on all statements — single project", func(t *testing.T) { + tenant, err := resolveCustomRoleTenant(models.RoleStatements{ + { + Operations: []string{"secretmanager.secrets.get"}, + Binding: "projects/thand-secrets", + }, + { + Operations: []string{"secretmanager.versions.access"}, + Binding: "projects/thand-secrets", + }, + }, "folders/205090528354") + require.NoError(t, err) + assert.Equal(t, "thand-secrets", tenant.ID) + assert.Equal(t, "project", tenant.Type) + }) + + t.Run("explicit binding without projects/ prefix accepted", func(t *testing.T) { + tenant, err := resolveCustomRoleTenant(models.RoleStatements{ + { + Operations: []string{"secretmanager.secrets.get"}, + Binding: "thand-secrets", + }, + }, "folders/205090528354") + require.NoError(t, err) + assert.Equal(t, "thand-secrets", tenant.ID) + }) + + t.Run("folder binding rejected", func(t *testing.T) { + _, err := resolveCustomRoleTenant(models.RoleStatements{ + { + Operations: []string{"secretmanager.secrets.get"}, + Binding: "folders/205090528354", + }, + }, "folders/205090528354") + require.Error(t, err) + assert.Contains(t, err.Error(), "folder") + }) + + t.Run("conflicting bindings across statements error", func(t *testing.T) { + _, err := resolveCustomRoleTenant(models.RoleStatements{ + { + Operations: []string{"secretmanager.secrets.get"}, + Binding: "projects/proj-a", + }, + { + Operations: []string{"secretmanager.versions.access"}, + Binding: "projects/proj-b", + }, + }, "folders/205090528354") + require.Error(t, err) + assert.Contains(t, err.Error(), "conflicting bindings") + }) + + t.Run("missing binding falls back to target inference", func(t *testing.T) { + // One statement has no Binding — triggers legacy target-inference path + tenant, err := resolveCustomRoleTenant(models.RoleStatements{ + { + Operations: []string{"secretmanager.secrets.get"}, + Targets: []string{"projects/thand-secrets/*"}, + // No Binding + }, + }, "folders/205090528354") + require.NoError(t, err) + assert.Equal(t, "thand-secrets", tenant.ID) + }) + + t.Run("organization binding rejected", func(t *testing.T) { + _, err := resolveCustomRoleTenant(models.RoleStatements{ + { + Operations: []string{"secretmanager.secrets.get"}, + Binding: "organizations/123456789", + }, + }, "folders/205090528354") + require.Error(t, err) + assert.Contains(t, err.Error(), "organization") + assert.Contains(t, err.Error(), "organization_id") + }) + + t.Run("partial binding falls back to target inference with specific warning", func(t *testing.T) { + // First statement has binding, second does not — explicit binding should be + // ignored and inference used instead (with a warning logged). + tenant, err := resolveCustomRoleTenant(models.RoleStatements{ + { + Operations: []string{"secretmanager.secrets.get"}, + Targets: []string{"projects/thand-secrets/*"}, + Binding: "projects/thand-secrets", // set + }, + { + Operations: []string{"secretmanager.versions.access"}, + Targets: []string{"projects/thand-secrets/*"}, + // Binding intentionally absent + }, + }, "folders/205090528354") + require.NoError(t, err) + // Despite the explicit binding on the first statement, inference wins + assert.Equal(t, "thand-secrets", tenant.ID) + }) +} diff --git a/internal/providers/gcp/rbac.go b/internal/providers/gcp/rbac.go index 4cf06479..f2985361 100644 --- a/internal/providers/gcp/rbac.go +++ b/internal/providers/gcp/rbac.go @@ -121,8 +121,18 @@ func (p *gcpProvider) AuthorizeRole( // Folder resources cannot host custom roles; custom roles are created at // project scope (default) or organization scope when organization_id is set. + // When a folder tenant is used and permissions.allow is present, resolve the + // project from explicit statement Binding fields (preferred) or from Targets + // (legacy fallback). Non-folder tenants use the tenant directly. + customRoleTenant := tenant + customRoleResourceID := projectId if isFolderResource(tenant) && len(role.Permissions.Allow) > 0 { - return nil, fmt.Errorf("custom roles (permissions.allow) are not supported for folder-level resources (%s); custom roles are created at project or organization scope", projectId) + resolved, err := resolveCustomRoleTenant(role.Permissions.Allow, projectId) + if err != nil { + return nil, fmt.Errorf("custom roles (permissions.allow) cannot be created at folder scope (%s): %w", projectId, err) + } + customRoleTenant = resolved + customRoleResourceID = resolved.ID } var assignedRoles []string @@ -163,7 +173,7 @@ func (p *gcpProvider) AuthorizeRole( // Composite roles get a unique name; non-composite roles share a base identifier. if len(role.Permissions.Allow) > 0 { customRoleName := gcpRoleID(role.GetName()) - roleParent := p.getCustomRoleParent(projectId) + roleParent := p.getCustomRoleParent(customRoleResourceID) existingRole, err := p.getRole(localCtx, roleParent, customRoleName) if err != nil { @@ -190,7 +200,7 @@ func (p *gcpProvider) AuthorizeRole( // For project-scoped non-composite roles, record version in project labels. if !isComposite && !isOrganizationRoleParent(roleParent) { - p.setRoleVersionLabel(localCtx, projectId, customRoleName, role.GetVersionString()) + p.setRoleVersionLabel(localCtx, customRoleResourceID, customRoleName, role.GetVersionString()) } logrus.WithFields(logrus.Fields{ @@ -203,7 +213,7 @@ func (p *gcpProvider) AuthorizeRole( // Role already exists — for non-composite roles, check version first. needsUpdate := true if !isComposite && !isOrganizationRoleParent(roleParent) { - storedVersion := p.getRoleVersionLabel(localCtx, projectId, customRoleName) + storedVersion := p.getRoleVersionLabel(localCtx, customRoleResourceID, customRoleName) requestedVersion := role.GetVersionString() if storedVersion == requestedVersion { needsUpdate = false @@ -221,13 +231,13 @@ func (p *gcpProvider) AuthorizeRole( } // Update version label after patching (non-composite only). if !isComposite && !isOrganizationRoleParent(roleParent) { - p.setRoleVersionLabel(localCtx, projectId, customRoleName, role.GetVersionString()) + p.setRoleVersionLabel(localCtx, customRoleResourceID, customRoleName, role.GetVersionString()) } } } // Bind the user to the custom role via IAM policy - err = p.bindUserToRole(localCtx, projectId, user, existingRole, tenant) + err = p.bindUserToRole(localCtx, customRoleResourceID, user, existingRole, customRoleTenant) if err != nil { return nil, temporal.NewApplicationErrorWithOptions( fmt.Sprintf("failed to bind user to custom role %s: %v", existingRole.Name, err), @@ -242,7 +252,7 @@ func (p *gcpProvider) AuthorizeRole( logrus.WithFields(logrus.Fields{ "user_email": user.Email, "role": existingRole.Name, - "project_id": projectId, + "project_id": customRoleResourceID, }).Info("Successfully bound user to custom GCP role") assignedRoles = append(assignedRoles, existingRole.Name) @@ -285,6 +295,17 @@ func (p *gcpProvider) RevokeRole( projectId = tenant.ID } + customRoleTenant := tenant + customRoleResourceID := projectId + if isFolderResource(tenant) && len(role.Permissions.Allow) > 0 { + resolved, err := resolveCustomRoleTenant(role.Permissions.Allow, projectId) + if err != nil { + return nil, fmt.Errorf("custom roles (permissions.allow) cannot be revoked at folder scope (%s): %w", projectId, err) + } + customRoleTenant = resolved + customRoleResourceID = resolved.ID + } + isComposite := role.IsComposite() logrus.WithFields(logrus.Fields{ @@ -344,7 +365,18 @@ func (p *gcpProvider) RevokeRole( ) } - err = p.unbindUserFromRole(localCtx, projectId, user, existingRole, tenant) + customTenantForRole := customRoleTenant + customResourceForRole := customRoleResourceID + if roleProjectID, ok := projectIDFromRoleParent(roleParent); ok { + customTenantForRole = &models.ProviderTenant{ + ID: roleProjectID, + Type: "project", + Name: roleProjectID, + } + customResourceForRole = roleProjectID + } + + err = p.unbindUserFromRole(localCtx, customResourceForRole, user, existingRole, customTenantForRole) if err != nil { return nil, temporal.NewApplicationErrorWithOptions( fmt.Sprintf("failed to unbind user from custom role %s: %v", roleName, err), @@ -359,7 +391,7 @@ func (p *gcpProvider) RevokeRole( logrus.WithFields(logrus.Fields{ "user_email": user.Email, "role": roleName, - "project_id": projectId, + "project_id": customResourceForRole, }).Info("Successfully unbound user from custom GCP role") // Composite: delete the custom role after unbinding. @@ -457,14 +489,17 @@ func (p *gcpProvider) authorizeRoleTemporal( } stage := p.GetConfig().GetStringWithDefault("stage", "GA") - if len(role.Inherits) == 0 && len(role.Permissions.Allow) == 0 { - return nil, fmt.Errorf("role %s has no inherits or permissions defined", role.Name) + customRoleTenant := tenant + if isFolderResource(tenant) && len(role.Permissions.Allow) > 0 { + resolved, err := resolveCustomRoleTenant(role.Permissions.Allow, projectID) + if err != nil { + return nil, fmt.Errorf("custom roles (permissions.allow) cannot be created at folder scope (%s): %w", projectID, err) + } + customRoleTenant = resolved } - // Folder resources cannot host custom roles; custom roles are created at - // project scope (default) or organization scope when organization_id is set. - if isFolderResource(tenant) && len(role.Permissions.Allow) > 0 { - return nil, fmt.Errorf("custom roles (permissions.allow) are not supported for folder-level resources (%s); custom roles are created at project or organization scope", projectID) + if len(role.Inherits) == 0 && len(role.Permissions.Allow) == 0 { + return nil, fmt.Errorf("role %s has no inherits or permissions defined", role.Name) } var assignedRoles []string @@ -502,7 +537,7 @@ func (p *gcpProvider) authorizeRoleTemporal( Permissions: role.Permissions, IsComposite: isComposite, Version: role.GetVersionString(), - Tenant: tenant, + Tenant: customRoleTenant, }, ).Get(wfCtx, &resp); err != nil { return nil, fmt.Errorf("GetOrCreateAndBindCustomRole activity failed: %w", err) @@ -549,6 +584,15 @@ func (p *gcpProvider) revokeRoleTemporal( // Determine composite flag from the role. isComposite := role.IsComposite() + customRoleTenant := tenant + if isFolderResource(tenant) && len(role.Permissions.Allow) > 0 { + resolved, err := resolveCustomRoleTenant(role.Permissions.Allow, tenant.ID) + if err != nil { + return nil, fmt.Errorf("custom roles (permissions.allow) cannot be revoked at folder scope (%s): %w", tenant.ID, err) + } + customRoleTenant = resolved + } + if req.AuthorizeRoleResponse == nil || len(req.AuthorizeRoleResponse.Roles) == 0 { return nil, fmt.Errorf("no roles found in authorization response for revocation") } @@ -567,6 +611,13 @@ func (p *gcpProvider) revokeRoleTemporal( return nil, fmt.Errorf("UnbindUserFromPredefinedRole activity failed for %s: %w", roleName, err) } } else if isComposite { + tenantForRole := customRoleTenant + if roleParent, _, err := parseCustomRolePath(roleName); err == nil { + if roleProjectID, ok := projectIDFromRoleParent(roleParent); ok { + tenantForRole = &models.ProviderTenant{ID: roleProjectID, Type: "project", Name: roleProjectID} + } + } + // Composite: unbind + delete the custom role. if err := workflow.ExecuteActivity( wfCtx, @@ -574,12 +625,19 @@ func (p *gcpProvider) revokeRoleTemporal( &UnbindAndDeleteCustomRoleRequest{ User: user, RoleName: roleName, - Tenant: tenant, + Tenant: tenantForRole, }, ).Get(wfCtx, nil); err != nil { return nil, fmt.Errorf("UnbindAndDeleteCustomRole activity failed for %s: %w", roleName, err) } } else { + tenantForRole := customRoleTenant + if roleParent, _, err := parseCustomRolePath(roleName); err == nil { + if roleProjectID, ok := projectIDFromRoleParent(roleParent); ok { + tenantForRole = &models.ProviderTenant{ID: roleProjectID, Type: "project", Name: roleProjectID} + } + } + // Non-composite: unbind only; retain the custom role. if err := workflow.ExecuteActivity( wfCtx, @@ -587,7 +645,7 @@ func (p *gcpProvider) revokeRoleTemporal( &UnbindUserFromCustomRoleRequest{ User: user, RoleName: roleName, - Tenant: tenant, + Tenant: tenantForRole, }, ).Get(wfCtx, nil); err != nil { return nil, fmt.Errorf("UnbindUserFromCustomRole activity failed for %s: %w", roleName, err) @@ -798,6 +856,168 @@ func parseCustomRolePath(fullPath string) (string, string, error) { return parts[0] + "/" + parts[1], parts[3], nil } +func projectIDFromRoleParent(roleParent string) (string, bool) { + if !strings.HasPrefix(roleParent, "projects/") { + return "", false + } + projectID := strings.TrimPrefix(roleParent, "projects/") + if len(projectID) == 0 { + return "", false + } + return projectID, true +} + +// resolveCustomRoleTenant determines the project tenant to use when creating and +// binding a custom GCP role for a set of permission statements. +// +// Resolution order: +// 1. If every statement has an explicit Binding set, and all bindings resolve to +// the same project, use that project. A folders/ binding is rejected because +// GCP does not support custom roles at folder scope. +// 2. Otherwise fall back to inferring the project from Targets (legacy behaviour, +// emits a deprecation warning so operators can migrate to explicit Binding). +// 3. If neither succeeds the error from inference is returned. +func resolveCustomRoleTenant(statements models.RoleStatements, fallbackTenantID string) (*models.ProviderTenant, error) { + // ── Step 1: explicit Binding on all statements ──────────────────────────── + anyHaveBinding := false + allHaveBinding := len(statements) > 0 + for _, stmt := range statements { + if stmt.Binding != "" { + anyHaveBinding = true + } else { + allHaveBinding = false + } + } + + // Warn loudly when a user sets binding on some statements but not all — the + // explicit binding values will be silently ignored in the fallback path, + // which is almost certainly not what was intended. + if anyHaveBinding && !allHaveBinding { + logrus.WithField("fallback_tenant", fallbackTenantID).Warn( + "some permissions.allow statements have 'binding' set but not all; " + + "the explicit binding values will be ignored and the project will be inferred from targets instead. " + + "Set 'binding' on every statement or remove it from all statements.", + ) + } + + if allHaveBinding { + projectIDs := make(map[string]struct{}) + for _, stmt := range statements { + binding := strings.TrimSpace(stmt.Binding) + if strings.HasPrefix(binding, "folders/") { + return nil, fmt.Errorf("binding %q targets a folder; GCP custom roles must be created at project or organization scope", binding) + } + // Organization-level role creation via the binding field is not currently + // implemented: the org IAM API path is not wired up in bindUserToRoleByName. + // Use the provider-level 'organization_id' config to create roles at org scope. + if strings.HasPrefix(binding, "organizations/") { + return nil, fmt.Errorf( + "binding %q targets an organization; organization-level role creation via the 'binding' field is not currently supported — "+ + "set 'organization_id' in the provider configuration to create custom roles at organization scope", + binding, + ) + } + // For GCP we need a project ID, not a full resource string. + // Accept either "projects/{id}" or bare "{id}". + projectID := strings.TrimPrefix(binding, "projects/") + if len(projectID) == 0 { + return nil, fmt.Errorf("binding %q does not resolve to a project", binding) + } + projectIDs[projectID] = struct{}{} + } + + if len(projectIDs) > 1 { + projects := make([]string, 0, len(projectIDs)) + for p := range projectIDs { + projects = append(projects, p) + } + sort.Strings(projects) + return nil, fmt.Errorf("permission statements have conflicting bindings %v; all statements in a role must bind to the same project", projects) + } + + for projectID := range projectIDs { + return &models.ProviderTenant{ID: projectID, Type: "project", Name: projectID}, nil + } + } + + // ── Step 2: legacy fallback – infer from targets ────────────────────────── + logrus.WithField("fallback_tenant", fallbackTenantID).Warn( + "permissions.allow statements are missing 'binding'; inferring project from targets. " + + "Set an explicit 'binding' on each statement to remove this warning.", + ) + projectID, err := inferProjectIDFromPermissionTargets(statements) + if err != nil { + return nil, err + } + return &models.ProviderTenant{ID: projectID, Type: "project", Name: projectID}, nil +} + +func inferProjectIDFromPermissionTargets(statements models.RoleStatements) (string, error) { + projectIDs := make(map[string]struct{}) + + for _, statement := range statements { + if len(statement.Targets) == 0 { + return "", fmt.Errorf("permission statement with operations %v is missing targets", statement.Operations) + } + + for _, target := range statement.Targets { + projectID, ok := projectIDFromTarget(target) + if !ok { + return "", fmt.Errorf("target %q does not include a specific project path (expected projects/{project}/...)", target) + } + projectIDs[projectID] = struct{}{} + } + } + + if len(projectIDs) == 0 { + return "", fmt.Errorf("no project targets found") + } + if len(projectIDs) > 1 { + projects := make([]string, 0, len(projectIDs)) + for projectID := range projectIDs { + projects = append(projects, projectID) + } + sort.Strings(projects) + return "", fmt.Errorf("targets span multiple projects %v; split permissions into separate roles", projects) + } + + for projectID := range projectIDs { + return projectID, nil + } + + return "", fmt.Errorf("no project targets found") +} + +func projectIDFromTarget(target string) (string, bool) { + target = strings.TrimSpace(target) + if len(target) == 0 { + return "", false + } + + projectMarker := "projects/" + _, after, ok := strings.Cut(target, projectMarker) + if !ok { + return "", false + } + + remaining := after + if len(remaining) == 0 { + return "", false + } + + nextSeparator := strings.Index(remaining, "/") + if nextSeparator == -1 { + nextSeparator = len(remaining) + } + + projectID := remaining[:nextSeparator] + if len(projectID) == 0 || projectID == "*" { + return "", false + } + + return projectID, true +} + // ───────────────────────────────────────────────────────────────────────────── // Project-label based version tracking for non-composite roles // ───────────────────────────────────────────────────────────────────────────── From b915f0bcb0f2950f41f9be78cec2b182652358f5 Mon Sep 17 00:00:00 2001 From: hughneale Date: Tue, 10 Mar 2026 16:04:47 +0000 Subject: [PATCH 2/4] fix(gcp): address copilot review follow-ups - Avoid duplicate fallback warnings in resolveCustomRoleTenant by only logging the generic missing-binding warning when no statements define binding. - Strengthen TestResolveCustomRoleTenant to assert warning output for both missing-binding and partial-binding fallback paths. - Align binding docs with runtime behavior: inference failure now documented as configuration error, and warning examples match actual log messages. --- docs/configuration/roles/index.md | 2 +- docs/configuration/roles/migration.md | 8 +++++- internal/providers/gcp/provider_test.go | 36 +++++++++++++++++++++++++ internal/providers/gcp/rbac.go | 10 ++++--- 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/docs/configuration/roles/index.md b/docs/configuration/roles/index.md index d756c506..59360c72 100644 --- a/docs/configuration/roles/index.md +++ b/docs/configuration/roles/index.md @@ -520,7 +520,7 @@ Without an explicit `binding`, the GCP provider attempts to infer a project from 1. If `binding` is set on **all** statements in the role, that value is used. 2. If `binding` is absent from one or more statements, the provider falls back to inferring a scope from `targets` (legacy, deprecated — a warning is logged). If **some** statements have `binding` set but not all, the explicit binding values are ignored and a targeted warning is emitted. -3. If inference also fails, the request tenant is used as-is (may produce an error if the tenant type is unsupported for custom roles). +3. If inference also fails, a configuration error is returned and the request is rejected. All statements in a role that set `binding` must agree on the same value. Conflicting `binding` values across statements produce a validation error. diff --git a/docs/configuration/roles/migration.md b/docs/configuration/roles/migration.md index d31d09b9..106e77b5 100644 --- a/docs/configuration/roles/migration.md +++ b/docs/configuration/roles/migration.md @@ -252,7 +252,13 @@ The field exists to resolve this explicitly and cleanly, regardless of provider. If your existing roles have `permissions.allow` statements that include `targets` pointing at a specific project (e.g. `projects/my-project/...`), the GCP provider previously attempted to infer the binding project from those targets whenever the request tenant was a folder. This inference still works, but it now emits a **deprecation warning** in the agent logs: ``` -WARN: binding not set on statement with permissions.allow; inferring project from targets (deprecated — set binding explicitly) +level=warning msg="permissions.allow statements are missing 'binding'; inferring project from targets. Set an explicit 'binding' on each statement to remove this warning." +``` + +If only some statements in the role set `binding`, a different warning is emitted: + +``` +level=warning msg="some permissions.allow statements have 'binding' set but not all; the explicit binding values will be ignored and the project will be inferred from targets instead. Set 'binding' on every statement or remove it from all statements." ``` To silence the warning and make the intent explicit, add `binding` to the relevant statements: diff --git a/internal/providers/gcp/provider_test.go b/internal/providers/gcp/provider_test.go index 6a9adbf6..e68e5ecd 100644 --- a/internal/providers/gcp/provider_test.go +++ b/internal/providers/gcp/provider_test.go @@ -1,9 +1,12 @@ package gcp import ( + "bytes" "context" + "io" "testing" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -332,6 +335,25 @@ func TestInferProjectIDFromPermissionTargets(t *testing.T) { } func TestResolveCustomRoleTenant(t *testing.T) { + captureWarnings := func(t *testing.T) *bytes.Buffer { + t.Helper() + + logger := logrus.StandardLogger() + originalOut := logger.Out + originalLevel := logger.Level + + buffer := &bytes.Buffer{} + logger.SetOutput(buffer) + logger.SetLevel(logrus.WarnLevel) + + t.Cleanup(func() { + logger.SetOutput(originalOut) + logger.SetLevel(originalLevel) + }) + + return buffer + } + t.Run("explicit binding on all statements — single project", func(t *testing.T) { tenant, err := resolveCustomRoleTenant(models.RoleStatements{ { @@ -386,6 +408,8 @@ func TestResolveCustomRoleTenant(t *testing.T) { }) t.Run("missing binding falls back to target inference", func(t *testing.T) { + buffer := captureWarnings(t) + // One statement has no Binding — triggers legacy target-inference path tenant, err := resolveCustomRoleTenant(models.RoleStatements{ { @@ -396,6 +420,10 @@ func TestResolveCustomRoleTenant(t *testing.T) { }, "folders/205090528354") require.NoError(t, err) assert.Equal(t, "thand-secrets", tenant.ID) + + warningOutput, err := io.ReadAll(buffer) + require.NoError(t, err) + assert.Contains(t, string(warningOutput), "permissions.allow statements are missing 'binding'; inferring project from targets") }) t.Run("organization binding rejected", func(t *testing.T) { @@ -411,6 +439,8 @@ func TestResolveCustomRoleTenant(t *testing.T) { }) t.Run("partial binding falls back to target inference with specific warning", func(t *testing.T) { + buffer := captureWarnings(t) + // First statement has binding, second does not — explicit binding should be // ignored and inference used instead (with a warning logged). tenant, err := resolveCustomRoleTenant(models.RoleStatements{ @@ -428,5 +458,11 @@ func TestResolveCustomRoleTenant(t *testing.T) { require.NoError(t, err) // Despite the explicit binding on the first statement, inference wins assert.Equal(t, "thand-secrets", tenant.ID) + + warningOutput, err := io.ReadAll(buffer) + require.NoError(t, err) + warningText := string(warningOutput) + assert.Contains(t, warningText, "some permissions.allow statements have 'binding' set but not all") + assert.NotContains(t, warningText, "permissions.allow statements are missing 'binding'; inferring project from targets") }) } diff --git a/internal/providers/gcp/rbac.go b/internal/providers/gcp/rbac.go index f2985361..81179c85 100644 --- a/internal/providers/gcp/rbac.go +++ b/internal/providers/gcp/rbac.go @@ -941,10 +941,12 @@ func resolveCustomRoleTenant(statements models.RoleStatements, fallbackTenantID } // ── Step 2: legacy fallback – infer from targets ────────────────────────── - logrus.WithField("fallback_tenant", fallbackTenantID).Warn( - "permissions.allow statements are missing 'binding'; inferring project from targets. " + - "Set an explicit 'binding' on each statement to remove this warning.", - ) + if !anyHaveBinding { + logrus.WithField("fallback_tenant", fallbackTenantID).Warn( + "permissions.allow statements are missing 'binding'; inferring project from targets. " + + "Set an explicit 'binding' on each statement to remove this warning.", + ) + } projectID, err := inferProjectIDFromPermissionTargets(statements) if err != nil { return nil, err From f2d55469d567af3cb9d88229e2a7ddead54047e6 Mon Sep 17 00:00:00 2001 From: hughneale Date: Tue, 10 Mar 2026 19:26:37 +0000 Subject: [PATCH 3/4] feat: per-statement custom GCP role creation with binding and ID support - Add Statement.ID field (snake_case, max 64 chars) for per-statement role naming - Add snake_case custom validator - Add statementRoleID() helper: single stmt = base name, multi-stmt = {base}_{id} or {base}_s{index} - Add resolveStatementBindingTenant() for per-statement binding resolution - Refactor AuthorizeRole (direct + Temporal) to create one custom role per allow statement - Each statement independently resolves its binding tenant and gets its own role ID - Update RevokeRole (direct + Temporal) to derive tenant from role resource path - Add GCP targets warning (config-time + authorization-time) - Add isGcpRole() and hasTargetsInStatements() config helpers - Propagate Statement.ID across all literal constructions - Add binding and ID visibility to Slack, email notifications and static HTML pages - Update docs: statement structure table, YAML examples, migration guide sections - Add tests for snake_case validator (11 cases) and statementRoleID (5 cases) --- docs/configuration/roles/index.md | 4 +- docs/configuration/roles/migration.md | 93 +++++++++ internal/common/validator.go | 11 + internal/common/validator_test.go | 39 +++- internal/config/roles.go | 32 +++ internal/daemon/static/elevate_static.html | 7 + internal/daemon/static/role.html | 32 +++ internal/models/provider_rbac.go | 2 + internal/models/role.go | 7 + internal/providers/gcp/provider_test.go | 42 ++++ internal/providers/gcp/rbac.go | 195 +++++++++++------- .../thand/approval_email_content.html | 4 +- .../tasks/providers/thand/approvals_email.go | 6 + .../tasks/providers/thand/approvals_slack.go | 6 + .../tasks/providers/thand/authorize_email.go | 17 +- .../thand/authorize_email_content.html | 2 +- .../tasks/providers/thand/authorize_slack.go | 5 +- .../tasks/providers/thand/helpers.go | 3 + .../tasks/providers/thand/helpers_test.go | 12 ++ .../tasks/providers/thand/revoke_email.go | 15 ++ .../providers/thand/revoke_email_content.html | 2 +- 21 files changed, 451 insertions(+), 85 deletions(-) diff --git a/docs/configuration/roles/index.md b/docs/configuration/roles/index.md index 59360c72..8c237d16 100644 --- a/docs/configuration/roles/index.md +++ b/docs/configuration/roles/index.md @@ -216,6 +216,7 @@ Each permission statement is an object with the following fields: | Field | Type | Required | Description | |-------|------|----------|-------------| +| `id` | string | No | Identifier for this statement, used to derive per-statement custom role names (snake_case, max 64 chars) | | `operations` | array | Yes | Actions that can be performed (provider-specific) | | `targets` | array | No | Resources the operations apply to (provider-specific) | | `conditions` | object | No | Provider-specific conditions that must be met | @@ -224,7 +225,8 @@ Each permission statement is an object with the following fields: ```yaml permissions: allow: - - operations: # What actions to allow + - id: secret_read # Optional: identifier for per-statement role naming + operations: # What actions to allow - action1 - action2 targets: # Which resources these actions apply to diff --git a/docs/configuration/roles/migration.md b/docs/configuration/roles/migration.md index 106e77b5..9f534e75 100644 --- a/docs/configuration/roles/migration.md +++ b/docs/configuration/roles/migration.md @@ -290,6 +290,98 @@ permissions: --- +## 7. New: Statement ID Field + +The `id` field is an optional property on permission statements that provides a stable identifier for per-statement custom role naming. It is most useful when a role contains **multiple statements**, each requiring its own custom role in the provider. + +### Why it exists + +When a role has multiple `allow` statements, the provider must create a separate custom role for each one. Without `id`, the generated role names use a positional index suffix (e.g., `thand_my_role_s0`, `thand_my_role_s1`). This means reordering or inserting statements changes the generated names, which can cause unnecessary role deletions and recreations. + +Setting `id` on each statement produces **stable, human-readable** names: + +```yaml +# Without id — positional names (fragile) +permissions: + allow: + - operations: # → thand_my_role_s0 + - secretmanager.secrets.get + - operations: # → thand_my_role_s1 + - storage.buckets.get + +# With id — stable names +permissions: + allow: + - id: secrets_read # → thand_my_role_secrets_read + operations: + - secretmanager.secrets.get + - id: storage_read # → thand_my_role_storage_read + operations: + - storage.buckets.get +``` + +### Validation + +- Must be **snake_case**: lowercase letters, digits, and underscores only (regex: `^[a-z][a-z0-9_]*$`). +- Maximum **64 characters**. +- Optional — roles with a single statement do not need an `id` (the base role name is used directly). +- The value is **not shown** in notifications or user-facing messages; it is an internal identifier only. + +--- + +## 8. Provider-Specific Behavior: GCP Targets + +### GCP-Specific Note: Targets Are Metadata-Only + +For GCP roles, the `targets` field within permission statements is **preserved in the role definition but not enforced**. Only the `operations` field is used when building custom IAM roles. + +This means: +- **`targets` are metadata**: You can include targets for documentation or reference, but GCP's role creation system (IAM API) ignores them entirely. +- **`operations` are enforced**: Only the operations you list in `operations` are included in the generated custom role. +- **Use `binding` to scope IAM bindings**: To explicitly control which project or resource a custom role is assigned to a user, use the `binding` field. `binding` determines the IAM assignment scope, not `targets`. + +### Example: GCP Role with Targets (Targets Ignored) + +```yaml +permissions: + allow: + - binding: "projects/my-project" + operations: + - secretmanager.secrets.get + - secretmanager.secrets.list + targets: + - "projects/my-project/secrets/*" # This line is metadata only; GCP ignores it +``` + +The custom role created in GCP will only include `secretmanager.secrets.get` and `secretmanager.secrets.list`. The `targets` line provides documentation to users or tools about which resources these operations apply to, but it does not limit the role itself. + +### Avoid Relying on Targets for GCP Resource Scoping + +**Incorrect approach** (targets won't enforce scope): +```yaml +# DON'T do this—targets alone won't restrict scope +permissions: + allow: + - operations: + - storage.buckets.get + targets: + - "projects/my-project/buckets/my-bucket" # This is ignored! +``` + +**Correct approach** (use binding for scope control): +```yaml +# DO this—binding controls both role creation scope and IAM binding scope +permissions: + allow: + - binding: "projects/my-project" # Role created here; binding applied here + operations: + - storage.buckets.get + targets: + - "projects/my-project/buckets/my-bucket" # Optional: documentation +``` + +--- + ## Summary | Feature | Manual Action Required | Notes | @@ -302,6 +394,7 @@ permissions: | Conditions | Optional | New feature, AWS-only currently | | Composite field | No | System-managed, do not set | | Binding field | Recommended | Silences deprecation warning when tenant ≠ role creation scope | +| Statement ID | Optional | Stable per-statement custom role names for multi-statement roles | {: .note} While auto-migration ensures your existing configurations continue to work, we recommend updating your YAML files to the new format when convenient. The new format is more expressive, supports conditions and domain scopes, and makes the relationship between operations and their target resources explicit. diff --git a/internal/common/validator.go b/internal/common/validator.go index 863d0699..4bd2b06d 100644 --- a/internal/common/validator.go +++ b/internal/common/validator.go @@ -61,6 +61,17 @@ func GetValidator() *validator.Validate { // Log error but don't panic } + // Register custom validator for strict snake_case identifiers + // Used by Statement.Name field — must start with a lowercase letter, + // followed by lowercase alphanumeric characters and underscores only. + if err := validatorInstance.RegisterValidation("snake_case", func(fl validator.FieldLevel) bool { + value := fl.Field().String() + matched, _ := regexp.MatchString(`^[a-z][a-z0-9_]*$`, value) + return matched + }); err != nil { + // Log error but don't panic + } + // Call all registered custom validator functions validatorRegistrationsMu.Lock() registrations := validatorRegistrations diff --git a/internal/common/validator_test.go b/internal/common/validator_test.go index d1fdef2a..1c4ce854 100644 --- a/internal/common/validator_test.go +++ b/internal/common/validator_test.go @@ -167,8 +167,8 @@ func TestGetValidator(t *testing.T) { // Test that custom validators are registered type TestStruct struct { - SemverField string `validate:"semver_pattern"` - AlphanumHyphen string `validate:"alphanum_hyphen"` + SemverField string `validate:"semver_pattern"` + AlphanumHyphen string `validate:"alphanum_hyphen"` } tests := []struct { @@ -227,3 +227,38 @@ func TestGetValidator(t *testing.T) { }) } } + +func TestSnakeCaseValidator(t *testing.T) { + v := GetValidator() + + type TestStruct struct { + Name string `validate:"omitempty,snake_case"` + } + + tests := []struct { + name string + value string + wantErr bool + }{ + {"valid simple", "secrets_read", false}, + {"valid single word", "read", false}, + {"valid with numbers", "kms_v2", false}, + {"valid long", "gcp_secret_manager_folder", false}, + {"empty is valid (omitempty)", "", false}, + {"invalid uppercase", "Secrets_Read", true}, + {"invalid starts with number", "2secrets", true}, + {"invalid has hyphen", "secrets-read", true}, + {"invalid has dot", "secrets.read", true}, + {"invalid starts with underscore", "_secrets", true}, + {"invalid has space", "secrets read", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := v.Struct(TestStruct{Name: tt.value}) + if (err != nil) != tt.wantErr { + t.Errorf("snake_case(%q) error = %v, wantErr %v", tt.value, err, tt.wantErr) + } + }) + } +} diff --git a/internal/config/roles.go b/internal/config/roles.go index eb30f193..7ad8ed44 100644 --- a/internal/config/roles.go +++ b/internal/config/roles.go @@ -78,6 +78,31 @@ func validateRoleLimits(roleKey string, role *models.Role) error { return nil } +// isGcpRole checks if a role is configured for the GCP provider +func isGcpRole(role *models.Role) bool { + for _, provider := range role.Providers { + if strings.HasPrefix(provider, "gcp") { + return true + } + } + return false +} + +// hasTargetsInStatements checks if a role's permission statements contain targets +func hasTargetsInStatements(role *models.Role) bool { + for _, stmt := range role.Permissions.Allow { + if len(stmt.Targets) > 0 { + return true + } + } + for _, stmt := range role.Permissions.Deny { + if len(stmt.Targets) > 0 { + return true + } + } + return false +} + // LoadRoles loads roles from a file or URL func (c *Config) LoadRoles() (map[string]models.Role, error) { vaultData, err := c.loadRolesVaultData() @@ -168,6 +193,12 @@ func (c *Config) ApplyRoles(foundRoles []*models.RoleDefinitions) (map[string]mo logrus.WithError(err).Warnln("Role exceeds limits, skipping:", roleKey) continue } + + // Warn if GCP role has targets (targets are ignored by GCP provider) + if isGcpRole(&r) && hasTargetsInStatements(&r) { + logrus.Warnf("Role '%s' is configured for GCP provider but contains statement targets. GCP ignores targets in role definitions; use binding field to control IAM assignment scope.", roleKey) + } + defs[roleKey] = r } } @@ -1295,6 +1326,7 @@ func (c *Config) filterStatementsListByProvider(stmts models.RoleStatements, all filteredTargets := c.filterByProvider(stmt.Targets, allowedProviders) result = append(result, models.Statement{ + ID: stmt.ID, Operations: filteredOps, Targets: filteredTargets, Conditions: stmt.Conditions, diff --git a/internal/daemon/static/elevate_static.html b/internal/daemon/static/elevate_static.html index 54e76f61..b94b1b15 100644 --- a/internal/daemon/static/elevate_static.html +++ b/internal/daemon/static/elevate_static.html @@ -240,6 +240,13 @@

+
+ Binding: + +
+ diff --git a/internal/daemon/static/role.html b/internal/daemon/static/role.html index 8a71794b..e00d061c 100644 --- a/internal/daemon/static/role.html +++ b/internal/daemon/static/role.html @@ -134,14 +134,23 @@

+ ID Operations Targets + Binding Conditions {{range .CompositeRole.Permissions.Allow}} + + {{if .ID}} + {{.ID}} + {{else}} + — + {{end}} + {{if .Operations}} {{range $i, $op := .Operations}} @@ -160,6 +169,13 @@

All {{end}} + + {{if .Binding}} + {{.Binding}} + {{else}} + — + {{end}} + {{if .Conditions}} {{toJSON .Conditions}} @@ -192,14 +208,23 @@

+ ID Operations Targets + Binding Conditions {{range .CompositeRole.Permissions.Deny}} + + {{if .ID}} + {{.ID}} + {{else}} + — + {{end}} + {{if .Operations}} {{range $i, $op := .Operations}} @@ -218,6 +243,13 @@

All {{end}} + + {{if .Binding}} + {{.Binding}} + {{else}} + — + {{end}} + {{if .Conditions}} {{toJSON .Conditions}} diff --git a/internal/models/provider_rbac.go b/internal/models/provider_rbac.go index d23684e7..f6bcdfea 100644 --- a/internal/models/provider_rbac.go +++ b/internal/models/provider_rbac.go @@ -549,6 +549,7 @@ func validatePermissions(providerPermissions []SearchResult[ProviderPermission], // Create validated statement with expanded operations validatedStatements = append(validatedStatements, Statement{ + ID: stmt.ID, Operations: validatedOperations, Targets: stmt.Targets, Conditions: stmt.Conditions, @@ -639,6 +640,7 @@ func ExpandWildcardPermissionsForProvider(provider Provider, role *Role) { } if len(ops) > 0 { result = append(result, Statement{ + ID: stmt.ID, Operations: ops, Targets: stmt.Targets, Conditions: stmt.Conditions, diff --git a/internal/models/role.go b/internal/models/role.go index 6cf0ef90..652f5fcb 100644 --- a/internal/models/role.go +++ b/internal/models/role.go @@ -506,6 +506,13 @@ func (s *RoleStatements) UnmarshalJSON(data []byte) error { // This design allows passing through provider-native conditions without requiring // this system to understand every provider's condition syntax. type Statement struct { + // ID is an optional identifier for this statement, used to derive + // deterministic per-statement custom role IDs in providers like GCP. + // Must be strict snake_case (lowercase alphanumeric and underscores, + // starting with a letter). When omitted, the statement's index in the + // list is used as a fallback suffix. + ID string `json:"id,omitempty" validate:"omitempty,snake_case,min=1,max=64"` + // Operations contains provider-specific actions/permissions. // Examples: ["s3:GetObject", "s3:PutObject"] for AWS, ["storage.buckets.get"] for GCP Operations []string `json:"operations" validate:"max=500,dive,min=1,max=500"` diff --git a/internal/providers/gcp/provider_test.go b/internal/providers/gcp/provider_test.go index e68e5ecd..d7ef2022 100644 --- a/internal/providers/gcp/provider_test.go +++ b/internal/providers/gcp/provider_test.go @@ -466,3 +466,45 @@ func TestResolveCustomRoleTenant(t *testing.T) { assert.NotContains(t, warningText, "permissions.allow statements are missing 'binding'; inferring project from targets") }) } + +func TestStatementRoleID(t *testing.T) { + t.Run("single statement uses base name", func(t *testing.T) { + stmt := models.Statement{Operations: []string{"secretmanager.secrets.get"}} + id := statementRoleID("my_role", stmt, 0, 1) + assert.Equal(t, gcpRoleID("my_role"), id) + }) + + t.Run("multi-statement with name uses name suffix", func(t *testing.T) { + stmt := models.Statement{ + ID: "secrets_read", + Operations: []string{"secretmanager.secrets.get"}, + } + id := statementRoleID("my_role", stmt, 0, 2) + assert.Equal(t, gcpRoleID("my_role_secrets_read"), id) + }) + + t.Run("multi-statement without name uses index suffix", func(t *testing.T) { + stmt := models.Statement{Operations: []string{"secretmanager.secrets.get"}} + id0 := statementRoleID("my_role", stmt, 0, 3) + id1 := statementRoleID("my_role", stmt, 1, 3) + id2 := statementRoleID("my_role", stmt, 2, 3) + assert.Equal(t, gcpRoleID("my_role_s0"), id0) + assert.Equal(t, gcpRoleID("my_role_s1"), id1) + assert.Equal(t, gcpRoleID("my_role_s2"), id2) + }) + + t.Run("different names produce different IDs", func(t *testing.T) { + stmtA := models.Statement{ID: "read", Operations: []string{"storage.get"}} + stmtB := models.Statement{ID: "write", Operations: []string{"storage.put"}} + idA := statementRoleID("my_role", stmtA, 0, 2) + idB := statementRoleID("my_role", stmtB, 1, 2) + assert.NotEqual(t, idA, idB) + }) + + t.Run("deterministic across calls", func(t *testing.T) { + stmt := models.Statement{ID: "kms", Operations: []string{"cloudkms.get"}} + id1 := statementRoleID("my_role", stmt, 0, 2) + id2 := statementRoleID("my_role", stmt, 0, 2) + assert.Equal(t, id1, id2) + }) +} diff --git a/internal/providers/gcp/rbac.go b/internal/providers/gcp/rbac.go index 81179c85..750af84e 100644 --- a/internal/providers/gcp/rbac.go +++ b/internal/providers/gcp/rbac.go @@ -8,6 +8,7 @@ import ( "net/url" "slices" "sort" + "strconv" "strings" "time" @@ -51,6 +52,23 @@ func gcpRoleID(name string) string { return id } +// statementRoleID derives the GCP custom role ID for a single permission statement. +// +// When a role has exactly one allow statement, the base role name is used directly +// (backwards-compatible with existing single-role behaviour). +// +// When a role has multiple allow statements, each statement produces a distinct +// role. The suffix is the statement's ID field if set, otherwise "s{index}". +func statementRoleID(baseName string, stmt models.Statement, index, count int) string { + if count <= 1 { + return gcpRoleID(baseName) + } + if stmt.ID != "" { + return gcpRoleID(baseName + "_" + stmt.ID) + } + return gcpRoleID(baseName + "_s" + strconv.Itoa(index)) +} + // primitiveRoles are GCP basic/primitive roles that do not support IAM conditions. // See: https://cloud.google.com/iam/docs/conditions-overview#limitations var primitiveRoles = []string{"roles/owner", "roles/editor", "roles/viewer"} @@ -119,34 +137,16 @@ func (p *gcpProvider) AuthorizeRole( } stage := config.GetStringWithDefault("stage", "GA") - // Folder resources cannot host custom roles; custom roles are created at - // project scope (default) or organization scope when organization_id is set. - // When a folder tenant is used and permissions.allow is present, resolve the - // project from explicit statement Binding fields (preferred) or from Targets - // (legacy fallback). Non-folder tenants use the tenant directly. - customRoleTenant := tenant - customRoleResourceID := projectId - if isFolderResource(tenant) && len(role.Permissions.Allow) > 0 { - resolved, err := resolveCustomRoleTenant(role.Permissions.Allow, projectId) - if err != nil { - return nil, fmt.Errorf("custom roles (permissions.allow) cannot be created at folder scope (%s): %w", projectId, err) - } - customRoleTenant = resolved - customRoleResourceID = resolved.ID - } - var assignedRoles []string // If inherits is specified, validate and bind predefined GCP roles if len(role.Inherits) > 0 { for _, inheritedRole := range role.Inherits { - // Validate that the role is a valid GCP predefined role predefinedRole, err := p.GetRole(localCtx, inheritedRole) if err != nil { return nil, fmt.Errorf("invalid GCP role '%s': %w", inheritedRole, err) } - // Bind the user to the predefined role via IAM policy err = p.bindUserToPredefinedRole(localCtx, projectId, user, predefinedRole.Name, tenant) if err != nil { return nil, temporal.NewApplicationErrorWithOptions( @@ -169,15 +169,25 @@ func (p *gcpProvider) AuthorizeRole( } } - // If permissions are specified, create a custom role with those permissions. - // Composite roles get a unique name; non-composite roles share a base identifier. - if len(role.Permissions.Allow) > 0 { - customRoleName := gcpRoleID(role.GetName()) - roleParent := p.getCustomRoleParent(customRoleResourceID) + // Create a custom role per allow statement. Each statement independently + // resolves its binding tenant and gets its own role ID. + stmtCount := len(role.Permissions.Allow) + for stmtIdx, stmt := range role.Permissions.Allow { + stmtTenant, err := resolveStatementBindingTenant(stmt, tenant, projectId) + if err != nil { + return nil, fmt.Errorf("failed to resolve binding for statement %d: %w", stmtIdx, err) + } + stmtResourceID := stmtTenant.ID + + customRoleName := statementRoleID(role.GetName(), stmt, stmtIdx, stmtCount) + roleParent := p.getCustomRoleParent(stmtResourceID) + + stmtPermissions := models.RolePermissions{ + Allow: models.RoleStatements{stmt}, + } existingRole, err := p.getRole(localCtx, roleParent, customRoleName) if err != nil { - // If role doesn't exist, create it existingRole, err = p.createRole( localCtx, roleParent, @@ -185,7 +195,7 @@ func (p *gcpProvider) AuthorizeRole( role.Role.GetName(), role.GetDescription(), stage, - role.Permissions, + stmtPermissions, ) if err != nil { return nil, temporal.NewApplicationErrorWithOptions( @@ -198,22 +208,23 @@ func (p *gcpProvider) AuthorizeRole( ) } - // For project-scoped non-composite roles, record version in project labels. if !isComposite && !isOrganizationRoleParent(roleParent) { - p.setRoleVersionLabel(localCtx, customRoleResourceID, customRoleName, role.GetVersionString()) + p.setRoleVersionLabel(localCtx, stmtResourceID, customRoleName, role.GetVersionString()) } logrus.WithFields(logrus.Fields{ "role_name": customRoleName, "role_parent": roleParent, "is_composite": isComposite, - "permissions": role.Permissions.Allow, - }).Info("Created custom GCP role") + "stmt_index": stmtIdx, + "stmt_id": stmt.ID, + "binding": stmt.Binding, + "operations": stmt.Operations, + }).Info("Created custom GCP role for statement") } else { - // Role already exists — for non-composite roles, check version first. needsUpdate := true if !isComposite && !isOrganizationRoleParent(roleParent) { - storedVersion := p.getRoleVersionLabel(localCtx, customRoleResourceID, customRoleName) + storedVersion := p.getRoleVersionLabel(localCtx, stmtResourceID, customRoleName) requestedVersion := role.GetVersionString() if storedVersion == requestedVersion { needsUpdate = false @@ -225,19 +236,17 @@ func (p *gcpProvider) AuthorizeRole( } if needsUpdate { - existingRole, err = p.patchRoleIfStale(localCtx, existingRole, role.Permissions) + existingRole, err = p.patchRoleIfStale(localCtx, existingRole, stmtPermissions) if err != nil { return nil, fmt.Errorf("failed to update custom role %s: %w", customRoleName, err) } - // Update version label after patching (non-composite only). if !isComposite && !isOrganizationRoleParent(roleParent) { - p.setRoleVersionLabel(localCtx, customRoleResourceID, customRoleName, role.GetVersionString()) + p.setRoleVersionLabel(localCtx, stmtResourceID, customRoleName, role.GetVersionString()) } } } - // Bind the user to the custom role via IAM policy - err = p.bindUserToRole(localCtx, customRoleResourceID, user, existingRole, customRoleTenant) + err = p.bindUserToRole(localCtx, stmtResourceID, user, existingRole, stmtTenant) if err != nil { return nil, temporal.NewApplicationErrorWithOptions( fmt.Sprintf("failed to bind user to custom role %s: %v", existingRole.Name, err), @@ -252,7 +261,8 @@ func (p *gcpProvider) AuthorizeRole( logrus.WithFields(logrus.Fields{ "user_email": user.Email, "role": existingRole.Name, - "project_id": customRoleResourceID, + "project_id": stmtResourceID, + "stmt_index": stmtIdx, }).Info("Successfully bound user to custom GCP role") assignedRoles = append(assignedRoles, existingRole.Name) @@ -295,17 +305,6 @@ func (p *gcpProvider) RevokeRole( projectId = tenant.ID } - customRoleTenant := tenant - customRoleResourceID := projectId - if isFolderResource(tenant) && len(role.Permissions.Allow) > 0 { - resolved, err := resolveCustomRoleTenant(role.Permissions.Allow, projectId) - if err != nil { - return nil, fmt.Errorf("custom roles (permissions.allow) cannot be revoked at folder scope (%s): %w", projectId, err) - } - customRoleTenant = resolved - customRoleResourceID = resolved.ID - } - isComposite := role.IsComposite() logrus.WithFields(logrus.Fields{ @@ -365,8 +364,10 @@ func (p *gcpProvider) RevokeRole( ) } - customTenantForRole := customRoleTenant - customResourceForRole := customRoleResourceID + // Derive tenant from the role's resource path — each per-statement + // role encodes its binding project (e.g. projects/{project}/roles/{name}). + customTenantForRole := tenant + customResourceForRole := projectId if roleProjectID, ok := projectIDFromRoleParent(roleParent); ok { customTenantForRole = &models.ProviderTenant{ ID: roleProjectID, @@ -489,15 +490,6 @@ func (p *gcpProvider) authorizeRoleTemporal( } stage := p.GetConfig().GetStringWithDefault("stage", "GA") - customRoleTenant := tenant - if isFolderResource(tenant) && len(role.Permissions.Allow) > 0 { - resolved, err := resolveCustomRoleTenant(role.Permissions.Allow, projectID) - if err != nil { - return nil, fmt.Errorf("custom roles (permissions.allow) cannot be created at folder scope (%s): %w", projectID, err) - } - customRoleTenant = resolved - } - if len(role.Inherits) == 0 && len(role.Permissions.Allow) == 0 { return nil, fmt.Errorf("role %s has no inherits or permissions defined", role.Name) } @@ -520,9 +512,18 @@ func (p *gcpProvider) authorizeRoleTemporal( assignedRoles = append(assignedRoles, resp.RoleName) } - if len(role.Permissions.Allow) > 0 { - // Composite roles get a unique name; non-composite roles share a base identifier. - customRoleName := gcpRoleID(role.GetName()) + // Create a custom role per allow statement via separate activities. + stmtCount := len(role.Permissions.Allow) + for stmtIdx, stmt := range role.Permissions.Allow { + stmtTenant, err := resolveStatementBindingTenant(stmt, tenant, projectID) + if err != nil { + return nil, fmt.Errorf("failed to resolve binding for statement %d: %w", stmtIdx, err) + } + + customRoleName := statementRoleID(role.GetName(), stmt, stmtIdx, stmtCount) + stmtPermissions := models.RolePermissions{ + Allow: models.RoleStatements{stmt}, + } var resp GetOrCreateAndBindCustomRoleResponse if err := workflow.ExecuteActivity( @@ -534,13 +535,13 @@ func (p *gcpProvider) authorizeRoleTemporal( Title: role.Role.GetName(), Description: role.GetDescription(), Stage: stage, - Permissions: role.Permissions, + Permissions: stmtPermissions, IsComposite: isComposite, Version: role.GetVersionString(), - Tenant: customRoleTenant, + Tenant: stmtTenant, }, ).Get(wfCtx, &resp); err != nil { - return nil, fmt.Errorf("GetOrCreateAndBindCustomRole activity failed: %w", err) + return nil, fmt.Errorf("GetOrCreateAndBindCustomRole activity failed for statement %d: %w", stmtIdx, err) } assignedRoles = append(assignedRoles, resp.RoleName) } @@ -584,15 +585,6 @@ func (p *gcpProvider) revokeRoleTemporal( // Determine composite flag from the role. isComposite := role.IsComposite() - customRoleTenant := tenant - if isFolderResource(tenant) && len(role.Permissions.Allow) > 0 { - resolved, err := resolveCustomRoleTenant(role.Permissions.Allow, tenant.ID) - if err != nil { - return nil, fmt.Errorf("custom roles (permissions.allow) cannot be revoked at folder scope (%s): %w", tenant.ID, err) - } - customRoleTenant = resolved - } - if req.AuthorizeRoleResponse == nil || len(req.AuthorizeRoleResponse.Roles) == 0 { return nil, fmt.Errorf("no roles found in authorization response for revocation") } @@ -611,7 +603,7 @@ func (p *gcpProvider) revokeRoleTemporal( return nil, fmt.Errorf("UnbindUserFromPredefinedRole activity failed for %s: %w", roleName, err) } } else if isComposite { - tenantForRole := customRoleTenant + tenantForRole := tenant if roleParent, _, err := parseCustomRolePath(roleName); err == nil { if roleProjectID, ok := projectIDFromRoleParent(roleParent); ok { tenantForRole = &models.ProviderTenant{ID: roleProjectID, Type: "project", Name: roleProjectID} @@ -631,7 +623,7 @@ func (p *gcpProvider) revokeRoleTemporal( return nil, fmt.Errorf("UnbindAndDeleteCustomRole activity failed for %s: %w", roleName, err) } } else { - tenantForRole := customRoleTenant + tenantForRole := tenant if roleParent, _, err := parseCustomRolePath(roleName); err == nil { if roleProjectID, ok := projectIDFromRoleParent(roleParent); ok { tenantForRole = &models.ProviderTenant{ID: roleProjectID, Type: "project", Name: roleProjectID} @@ -954,6 +946,52 @@ func resolveCustomRoleTenant(statements models.RoleStatements, fallbackTenantID return &models.ProviderTenant{ID: projectID, Type: "project", Name: projectID}, nil } +// resolveStatementBindingTenant determines the project tenant for a single permission +// statement. Used by the per-statement authorization loop. +// +// Resolution order: +// 1. If the statement has an explicit Binding, parse the project from it. +// 2. If the request tenant is not a folder, use it directly. +// 3. Fall back to inferring the project from the statement's Targets (legacy, deprecated). +func resolveStatementBindingTenant(stmt models.Statement, requestTenant *models.ProviderTenant, fallbackProjectID string) (*models.ProviderTenant, error) { + if stmt.Binding != "" { + binding := strings.TrimSpace(stmt.Binding) + if strings.HasPrefix(binding, "folders/") { + return nil, fmt.Errorf("binding %q targets a folder; GCP custom roles must be created at project or organization scope", binding) + } + if strings.HasPrefix(binding, "organizations/") { + return nil, fmt.Errorf( + "binding %q targets an organization; organization-level role creation via the 'binding' field is not currently supported — "+ + "set 'organization_id' in the provider configuration to create custom roles at organization scope", + binding, + ) + } + projectID := strings.TrimPrefix(binding, "projects/") + if len(projectID) == 0 { + return nil, fmt.Errorf("binding %q does not resolve to a project", binding) + } + return &models.ProviderTenant{ID: projectID, Type: "project", Name: projectID}, nil + } + + if !isFolderResource(requestTenant) { + return requestTenant, nil + } + + logrus.WithFields(logrus.Fields{ + "operations": stmt.Operations, + "fallback_project": fallbackProjectID, + }).Warn( + "permission statement is missing 'binding' and request tenant is a folder; " + + "inferring project from targets. Set an explicit 'binding' to remove this warning.", + ) + singleStmt := models.RoleStatements{stmt} + projectID, err := inferProjectIDFromPermissionTargets(singleStmt) + if err != nil { + return nil, fmt.Errorf("cannot resolve binding for statement with operations %v: %w", stmt.Operations, err) + } + return &models.ProviderTenant{ID: projectID, Type: "project", Name: projectID}, nil +} + func inferProjectIDFromPermissionTargets(statements models.RoleStatements) (string, error) { projectIDs := make(map[string]struct{}) @@ -1399,6 +1437,11 @@ func permissionsToGcpPermissions(permissions models.RolePermissions) []string { // Process Allow statements for _, stmt := range permissions.Allow { gcpPermissions = append(gcpPermissions, stmt.Operations...) + + // Warn if targets are present (GCP ignores targets in custom role definitions) + if len(stmt.Targets) > 0 { + logrus.Warnf("GCP custom roles do not enforce statement targets; targets are metadata only. Use binding field to control IAM assignment scope. Targets: %v", stmt.Targets) + } } // Log warning for Deny statements (GCP doesn't support deny in custom roles) diff --git a/internal/workflows/tasks/providers/thand/approval_email_content.html b/internal/workflows/tasks/providers/thand/approval_email_content.html index e8b26cb0..fca832b9 100644 --- a/internal/workflows/tasks/providers/thand/approval_email_content.html +++ b/internal/workflows/tasks/providers/thand/approval_email_content.html @@ -85,7 +85,7 @@

Permi

Allowed:

    {{range .Permissions.Allow}} -
  • - Operations: {{range $i, $op := .Operations}}{{if $i}}, {{end}}{{$op}}{{end}}{{if .Targets}}
      Targets: {{range $i, $t := .Targets}}{{if $i}}, {{end}}{{$t}}{{end}}{{end}}
    • +
  • - Operations: {{range $i, $op := .Operations}}{{if $i}}, {{end}}{{$op}}{{end}}{{if .Binding}}
      Binding: {{.Binding}}{{end}}{{if .Targets}}
      Targets: {{range $i, $t := .Targets}}{{if $i}}, {{end}}{{$t}}{{end}}{{end}}
    • {{end}}
{{end}} @@ -93,7 +93,7 @@

Permi

Denied:

    {{range .Permissions.Deny}} -
  • - Operations: {{range $i, $op := .Operations}}{{if $i}}, {{end}}{{$op}}{{end}}{{if .Targets}}
      Targets: {{range $i, $t := .Targets}}{{if $i}}, {{end}}{{$t}}{{end}}{{end}}
    • +
  • - Operations: {{range $i, $op := .Operations}}{{if $i}}, {{end}}{{$op}}{{end}}{{if .Binding}}
      Binding: {{.Binding}}{{end}}{{if .Targets}}
      Targets: {{range $i, $t := .Targets}}{{if $i}}, {{end}}{{$t}}{{end}}{{end}}
    • {{end}}
{{end}} diff --git a/internal/workflows/tasks/providers/thand/approvals_email.go b/internal/workflows/tasks/providers/thand/approvals_email.go index d6e7e0f0..a39774bb 100644 --- a/internal/workflows/tasks/providers/thand/approvals_email.go +++ b/internal/workflows/tasks/providers/thand/approvals_email.go @@ -108,6 +108,9 @@ func (a *approvalsNotifier) createApprovalEmailBody() (string, string) { if len(stmt.Operations) > 0 { fmt.Fprintf(&plainText, "- Operations: %s\n", strings.Join(stmt.Operations, ", ")) } + if len(stmt.Binding) > 0 { + fmt.Fprintf(&plainText, " Binding: %s\n", stmt.Binding) + } if len(stmt.Targets) > 0 { fmt.Fprintf(&plainText, " Targets: %s\n", strings.Join(stmt.Targets, ", ")) } @@ -119,6 +122,9 @@ func (a *approvalsNotifier) createApprovalEmailBody() (string, string) { if len(stmt.Operations) > 0 { fmt.Fprintf(&plainText, "- Operations: %s\n", strings.Join(stmt.Operations, ", ")) } + if len(stmt.Binding) > 0 { + fmt.Fprintf(&plainText, " Binding: %s\n", stmt.Binding) + } if len(stmt.Targets) > 0 { fmt.Fprintf(&plainText, " Targets: %s\n", strings.Join(stmt.Targets, ", ")) } diff --git a/internal/workflows/tasks/providers/thand/approvals_slack.go b/internal/workflows/tasks/providers/thand/approvals_slack.go index b29ece9f..3f9e7c12 100644 --- a/internal/workflows/tasks/providers/thand/approvals_slack.go +++ b/internal/workflows/tasks/providers/thand/approvals_slack.go @@ -171,6 +171,9 @@ func (a *approvalsNotifier) addPermissionsSection(blocks *[]slack.Block, elevate if len(stmt.Operations) > 0 { fmt.Fprintf(&permissionsText, "- Operations: `%s`\n", strings.Join(stmt.Operations, "`, `")) } + if len(stmt.Binding) > 0 { + fmt.Fprintf(&permissionsText, " Binding: `%s`\n", stmt.Binding) + } if len(stmt.Targets) > 0 { fmt.Fprintf(&permissionsText, " Targets: `%s`\n", strings.Join(stmt.Targets, "`, `")) } @@ -183,6 +186,9 @@ func (a *approvalsNotifier) addPermissionsSection(blocks *[]slack.Block, elevate if len(stmt.Operations) > 0 { fmt.Fprintf(&permissionsText, "- Operations: `%s`\n", strings.Join(stmt.Operations, "`, `")) } + if len(stmt.Binding) > 0 { + fmt.Fprintf(&permissionsText, " Binding: `%s`\n", stmt.Binding) + } if len(stmt.Targets) > 0 { fmt.Fprintf(&permissionsText, " Targets: `%s`\n", strings.Join(stmt.Targets, "`, `")) } diff --git a/internal/workflows/tasks/providers/thand/authorize_email.go b/internal/workflows/tasks/providers/thand/authorize_email.go index 45fbae5a..38df321a 100644 --- a/internal/workflows/tasks/providers/thand/authorize_email.go +++ b/internal/workflows/tasks/providers/thand/authorize_email.go @@ -54,6 +54,21 @@ func (a *authorizerNotifier) createAuthorizeEmailBody() (string, string) { } } + if elevationReq.Role != nil && len(elevationReq.Role.Permissions.Allow) > 0 { + plainText.WriteString("\nGranted Permissions:\n") + for _, stmt := range sortedStatementsWithSortedFields(elevationReq.Role.Permissions.Allow) { + if len(stmt.Operations) > 0 { + plainText.WriteString(fmt.Sprintf("- Operations: %s\n", strings.Join(stmt.Operations, ", "))) + } + if len(stmt.Binding) > 0 { + plainText.WriteString(fmt.Sprintf(" Binding: %s\n", stmt.Binding)) + } + if len(stmt.Targets) > 0 { + plainText.WriteString(fmt.Sprintf(" Targets: %s\n", strings.Join(stmt.Targets, ", "))) + } + } + } + plainText.WriteString("\nYour access is now active. Please use it responsibly.") // Build data map for template @@ -78,7 +93,7 @@ func (a *authorizerNotifier) createAuthorizeEmailBody() (string, string) { // Add permissions if available if len(elevationReq.Role.Permissions.Allow) > 0 { - data["Permissions"] = elevationReq.Role.Permissions.Allow + data["Permissions"] = sortedStatementsWithSortedFields(elevationReq.Role.Permissions.Allow) } } diff --git a/internal/workflows/tasks/providers/thand/authorize_email_content.html b/internal/workflows/tasks/providers/thand/authorize_email_content.html index 65dad90d..d3224a57 100644 --- a/internal/workflows/tasks/providers/thand/authorize_email_content.html +++ b/internal/workflows/tasks/providers/thand/authorize_email_content.html @@ -40,7 +40,7 @@

Acces

Permissions

    {{range .Permissions}} -
  • ✓ Operations: {{range $i, $op := .Operations}}{{if $i}}, {{end}}{{$op}}{{end}}{{if .Targets}}
      Targets: {{range $i, $t := .Targets}}{{if $i}}, {{end}}{{$t}}{{end}}{{end}}
    • +
  • ✓ Operations: {{range $i, $op := .Operations}}{{if $i}}, {{end}}{{$op}}{{end}}{{if .Binding}}
      Binding: {{.Binding}}{{end}}{{if .Targets}}
      Targets: {{range $i, $t := .Targets}}{{if $i}}, {{end}}{{$t}}{{end}}{{end}}
    • {{end}}
diff --git a/internal/workflows/tasks/providers/thand/authorize_slack.go b/internal/workflows/tasks/providers/thand/authorize_slack.go index 0962b498..bde1e020 100644 --- a/internal/workflows/tasks/providers/thand/authorize_slack.go +++ b/internal/workflows/tasks/providers/thand/authorize_slack.go @@ -125,10 +125,13 @@ func (a *authorizerNotifier) addAuthorizePermissionsSection(blocks *[]slack.Bloc var permissionsText strings.Builder permissionsText.WriteString("*Granted Permissions:*\n") - for _, stmt := range elevateRequest.Role.Permissions.Allow { + for _, stmt := range sortedStatementsWithSortedFields(elevateRequest.Role.Permissions.Allow) { if len(stmt.Operations) > 0 { permissionsText.WriteString(fmt.Sprintf("✓ Operations: `%s`\n", strings.Join(stmt.Operations, "`, `"))) } + if len(stmt.Binding) > 0 { + permissionsText.WriteString(fmt.Sprintf(" Binding: `%s`\n", stmt.Binding)) + } if len(stmt.Targets) > 0 { permissionsText.WriteString(fmt.Sprintf(" Targets: `%s`\n", strings.Join(stmt.Targets, "`, `"))) } diff --git a/internal/workflows/tasks/providers/thand/helpers.go b/internal/workflows/tasks/providers/thand/helpers.go index 55833c9f..2cd2647a 100644 --- a/internal/workflows/tasks/providers/thand/helpers.go +++ b/internal/workflows/tasks/providers/thand/helpers.go @@ -21,6 +21,7 @@ func sortedStrings(s []string) []string { // sortedStatementsWithSortedFields returns a new slice of Statements derived // from stmts where: // 1. Operations and Targets within each Statement are sorted A-Z. +// 1a. Binding is preserved as-is. // 2. The statements themselves are sorted A-Z with a fully deterministic // comparator: Operations are compared element-by-element (shorter slice // sorts first on a tie), then Targets are used as a further tie-breaker @@ -35,9 +36,11 @@ func sortedStatementsWithSortedFields(stmts models.RoleStatements) models.RoleSt out := make(models.RoleStatements, len(stmts)) for i, stmt := range stmts { out[i] = models.Statement{ + ID: stmt.ID, Operations: sortedStrings(stmt.Operations), Targets: sortedStrings(stmt.Targets), Conditions: stmt.Conditions, + Binding: stmt.Binding, } } sort.SliceStable(out, func(i, j int) bool { diff --git a/internal/workflows/tasks/providers/thand/helpers_test.go b/internal/workflows/tasks/providers/thand/helpers_test.go index 41634e47..44e0361f 100644 --- a/internal/workflows/tasks/providers/thand/helpers_test.go +++ b/internal/workflows/tasks/providers/thand/helpers_test.go @@ -168,6 +168,18 @@ func TestSortedStatementsWithSortedFields(t *testing.T) { assert.Equal(t, conditions, result[0].Conditions) }) + t.Run("binding is preserved on each statement", func(t *testing.T) { + stmts := models.RoleStatements{ + { + Operations: []string{"secretmanager.secrets.get"}, + Targets: []string{"projects/sourcegraph-secrets/secrets/*"}, + Binding: "projects/sourcegraph-secrets", + }, + } + result := sortedStatementsWithSortedFields(stmts) + assert.Equal(t, "projects/sourcegraph-secrets", result[0].Binding) + }) + t.Run("original slice and its elements are not mutated", func(t *testing.T) { original := models.RoleStatements{ { diff --git a/internal/workflows/tasks/providers/thand/revoke_email.go b/internal/workflows/tasks/providers/thand/revoke_email.go index 8702c3d9..d65d6986 100644 --- a/internal/workflows/tasks/providers/thand/revoke_email.go +++ b/internal/workflows/tasks/providers/thand/revoke_email.go @@ -31,6 +31,21 @@ func (r *revokeNotifier) createRevokeEmailBody() (string, string) { fmt.Fprintf(&plainText, "Duration: %s\n", elevationReq.Duration) } + if elevationReq.Role != nil && len(elevationReq.Role.Permissions.Allow) > 0 { + plainText.WriteString("\nRevoked Permissions:\n") + for _, stmt := range sortedStatementsWithSortedFields(elevationReq.Role.Permissions.Allow) { + if len(stmt.Operations) > 0 { + fmt.Fprintf(&plainText, "- Operations: %s\n", strings.Join(stmt.Operations, ", ")) + } + if len(stmt.Binding) > 0 { + fmt.Fprintf(&plainText, " Binding: %s\n", stmt.Binding) + } + if len(stmt.Targets) > 0 { + fmt.Fprintf(&plainText, " Targets: %s\n", strings.Join(stmt.Targets, ", ")) + } + } + } + plainText.WriteString("\nYour access has been successfully revoked. If you need access again, please submit a new request.") // Build data map for template diff --git a/internal/workflows/tasks/providers/thand/revoke_email_content.html b/internal/workflows/tasks/providers/thand/revoke_email_content.html index 1e5142c7..1bedab07 100644 --- a/internal/workflows/tasks/providers/thand/revoke_email_content.html +++ b/internal/workflows/tasks/providers/thand/revoke_email_content.html @@ -37,7 +37,7 @@

Revoc

Revoked Permissions

    {{range .Permissions}} -
  • ✗ Operations: {{range $i, $op := .Operations}}{{if $i}}, {{end}}{{$op}}{{end}}{{if .Targets}}
      Targets: {{range $i, $t := .Targets}}{{if $i}}, {{end}}{{$t}}{{end}}{{end}}
    • +
  • ✗ Operations: {{range $i, $op := .Operations}}{{if $i}}, {{end}}{{$op}}{{end}}{{if .Binding}}
      Binding: {{.Binding}}{{end}}{{if .Targets}}
      Targets: {{range $i, $t := .Targets}}{{if $i}}, {{end}}{{$t}}{{end}}{{end}}
    • {{end}}
From 6002b5a2bd13970a3401293a52877db56d2a4dfd Mon Sep 17 00:00:00 2001 From: hughneale Date: Tue, 17 Mar 2026 12:35:28 +0000 Subject: [PATCH 4/4] role name bindings --- internal/common/validator.go | 21 +++ internal/common/validator_test.go | 33 +++++ internal/config/roles.go | 18 ++- .../config/roles_permission_merging_test.go | 99 +++++++++++++ internal/models/role.go | 2 +- internal/providers/gcp/provider_test.go | 136 ------------------ internal/providers/gcp/rbac.go | 115 +++------------ 7 files changed, 189 insertions(+), 235 deletions(-) diff --git a/internal/common/validator.go b/internal/common/validator.go index 4bd2b06d..05f14db3 100644 --- a/internal/common/validator.go +++ b/internal/common/validator.go @@ -72,6 +72,27 @@ func GetValidator() *validator.Validate { // Log error but don't panic } + // Register custom validator for CSP binding resource identifiers. + // Ensures the value starts with a known cloud provider resource prefix + // so typos are caught at config-load time rather than at authorization time. + if err := validatorInstance.RegisterValidation("csp_binding", func(fl validator.FieldLevel) bool { + value := fl.Field().String() + for _, prefix := range []string{ + "projects/", // GCP + "organizations/", // GCP + "folders/", // GCP + "/subscriptions/", // Azure + "arn:aws:", // AWS + } { + if strings.HasPrefix(value, prefix) { + return true + } + } + return false + }); err != nil { + // Log error but don't panic + } + // Call all registered custom validator functions validatorRegistrationsMu.Lock() registrations := validatorRegistrations diff --git a/internal/common/validator_test.go b/internal/common/validator_test.go index 1c4ce854..5ef97075 100644 --- a/internal/common/validator_test.go +++ b/internal/common/validator_test.go @@ -262,3 +262,36 @@ func TestSnakeCaseValidator(t *testing.T) { }) } } + +func TestCSPBindingValidator(t *testing.T) { + v := GetValidator() + + type TestStruct struct { + Binding string `validate:"omitempty,csp_binding"` + } + + tests := []struct { + name string + value string + wantErr bool + }{ + {"empty is valid (omitempty)", "", false}, + {"GCP project", "projects/my-project", false}, + {"GCP organization", "organizations/123456789", false}, + {"GCP folder", "folders/205090528354", false}, + {"Azure subscription", "/subscriptions/sub-123", false}, + {"AWS ARN", "arn:aws:iam::123456789:root", false}, + {"invalid bare project ID", "my-project", true}, + {"invalid random string", "foobar", true}, + {"invalid empty prefix", "/projects/foo", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := v.Struct(TestStruct{Binding: tt.value}) + if (err != nil) != tt.wantErr { + t.Errorf("csp_binding(%q) error = %v, wantErr %v", tt.value, err, tt.wantErr) + } + }) + } +} diff --git a/internal/config/roles.go b/internal/config/roles.go index 7ad8ed44..fad209b9 100644 --- a/internal/config/roles.go +++ b/internal/config/roles.go @@ -1008,16 +1008,18 @@ func (c *Config) mergeRolePermissions(composite *models.Role, inherited *models. // normalizeStatements expands all statement operations and creates a map by operation. // Returns a map where key is operation and value is the set of targets associated with that operation. // normalizeStatements separates statements into two groups: -// 1. Normalized map (for statements WITHOUT conditions) - can be merged and deduplicated -// 2. Preserved statements (for statements WITH conditions) - kept as complete units -// This ensures conditions are not lost during merge operations. +// 1. Normalized map (for plain statements) - can be merged and deduplicated +// 2. Preserved statements (those with conditions, ID, or binding) - kept as complete units +// This ensures metadata that cannot survive the normalize/rebuild cycle is not lost. func normalizeStatements(stmts models.RoleStatements) (map[string]map[string]bool, models.RoleStatements) { result := make(map[string]map[string]bool) preservedStmts := make(models.RoleStatements, 0) for _, stmt := range stmts { - // Statements WITH conditions are preserved as-is - if len(stmt.Conditions) > 0 { + // Statements with conditions, ID, or binding are preserved as complete + // units — they carry metadata that cannot survive the normalize/rebuild + // cycle (which reduces statements to operation→targets maps). + if len(stmt.Conditions) > 0 || stmt.ID != "" || stmt.Binding != "" { preservedStmts = append(preservedStmts, stmt) continue } @@ -1076,6 +1078,11 @@ func deduplicatePreservedStatements(allow, deny models.RoleStatements) (models.R // statementsEqual checks if two statements are equal by comparing their operations, targets, and conditions. // String slices are compared in sorted order to ensure consistent comparison. func statementsEqual(a, b models.Statement) bool { + // Compare ID and Binding + if a.ID != b.ID || a.Binding != b.Binding { + return false + } + // Compare operations (sorted) if !stringSlicesEqual(a.Operations, b.Operations) { return false @@ -1330,6 +1337,7 @@ func (c *Config) filterStatementsListByProvider(stmts models.RoleStatements, all Operations: filteredOps, Targets: filteredTargets, Conditions: stmt.Conditions, + Binding: stmt.Binding, }) } return result diff --git a/internal/config/roles_permission_merging_test.go b/internal/config/roles_permission_merging_test.go index 7ec965c0..d51abca2 100644 --- a/internal/config/roles_permission_merging_test.go +++ b/internal/config/roles_permission_merging_test.go @@ -734,5 +734,104 @@ func TestGCPStylePermissionHandling(t *testing.T) { }) } +func TestStatementIDAndBindingPreservation(t *testing.T) { + t.Run("ID and binding survive inheritance merging", func(t *testing.T) { + roles := map[string]models.Role{ + "base": { + Name: "base", + Permissions: models.RolePermissions{ + Allow: models.RoleStatements{ + {Operations: []string{"secretmanager.secrets.list"}}, + }, + }, + Enabled: true, + }, + "child": { + Name: "child", + Inherits: []string{"base"}, + Permissions: models.RolePermissions{ + Allow: models.RoleStatements{ + { + ID: "stmt_one", + Binding: "projects/proj-a", + Operations: []string{"secretmanager.secrets.get"}, + }, + { + ID: "stmt_two", + Binding: "projects/proj-b", + Operations: []string{"secretmanager.secrets.get"}, + }, + }, + }, + Enabled: true, + }, + } + + config := &Config{Roles: RoleConfig{Definitions: roles}} + identity := &models.Identity{ID: "user", User: &models.User{Username: "u", Email: "u@e.com"}} + + result, err := config.GetCompositeRoleByName(identity, "child") + require.NoError(t, err) + + // Find statements by ID + var foundOne, foundTwo bool + for _, stmt := range result.Permissions.Allow { + switch stmt.ID { + case "stmt_one": + foundOne = true + assert.Equal(t, "projects/proj-a", stmt.Binding) + assert.Contains(t, stmt.Operations, "secretmanager.secrets.get") + case "stmt_two": + foundTwo = true + assert.Equal(t, "projects/proj-b", stmt.Binding) + assert.Contains(t, stmt.Operations, "secretmanager.secrets.get") + } + } + + assert.True(t, foundOne, "statement stmt_one must survive merge, got: %v", result.Permissions.Allow) + assert.True(t, foundTwo, "statement stmt_two must survive merge, got: %v", result.Permissions.Allow) + }) + + t.Run("ID and binding survive conflict resolution", func(t *testing.T) { + roles := map[string]models.Role{ + "role": { + Name: "role", + Permissions: models.RolePermissions{ + Allow: models.RoleStatements{ + { + ID: "keep_me", + Binding: "projects/proj-x", + Operations: []string{"compute.instances.get"}, + }, + {Operations: []string{"compute.instances.list"}}, + }, + Deny: models.RoleStatements{ + {Operations: []string{"compute.instances.list"}}, + }, + }, + Enabled: true, + }, + } + + config := &Config{Roles: RoleConfig{Definitions: roles}} + identity := &models.Identity{ID: "user", User: &models.User{Username: "u", Email: "u@e.com"}} + + result, err := config.GetCompositeRoleByName(identity, "role") + require.NoError(t, err) + + // The allow/deny conflict on compute.instances.list should be resolved, + // but the ID/Binding statement should survive. + var found bool + for _, stmt := range result.Permissions.Allow { + if stmt.ID == "keep_me" { + found = true + assert.Equal(t, "projects/proj-x", stmt.Binding) + assert.Contains(t, stmt.Operations, "compute.instances.get") + } + } + assert.True(t, found, "statement keep_me must survive conflict resolution, got: %v", result.Permissions.Allow) + }) +} + // Note: Helper functions expandCondensedActions and condenseActions // are now implemented in roles.go diff --git a/internal/models/role.go b/internal/models/role.go index 652f5fcb..9e7593ba 100644 --- a/internal/models/role.go +++ b/internal/models/role.go @@ -545,7 +545,7 @@ type Statement struct { // When omitted, the provider falls back to the request tenant for binding scope. // Providers may additionally attempt to infer a binding resource from Targets // for backwards compatibility. - Binding string `json:"binding,omitempty" validate:"max=500"` + Binding string `json:"binding,omitempty" validate:"omitempty,csp_binding,max=500"` } // ScopeIdentities defines identity-based restrictions for users, groups, and domains. diff --git a/internal/providers/gcp/provider_test.go b/internal/providers/gcp/provider_test.go index d7ef2022..33faa2fa 100644 --- a/internal/providers/gcp/provider_test.go +++ b/internal/providers/gcp/provider_test.go @@ -1,12 +1,9 @@ package gcp import ( - "bytes" "context" - "io" "testing" - "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -334,139 +331,6 @@ func TestInferProjectIDFromPermissionTargets(t *testing.T) { }) } -func TestResolveCustomRoleTenant(t *testing.T) { - captureWarnings := func(t *testing.T) *bytes.Buffer { - t.Helper() - - logger := logrus.StandardLogger() - originalOut := logger.Out - originalLevel := logger.Level - - buffer := &bytes.Buffer{} - logger.SetOutput(buffer) - logger.SetLevel(logrus.WarnLevel) - - t.Cleanup(func() { - logger.SetOutput(originalOut) - logger.SetLevel(originalLevel) - }) - - return buffer - } - - t.Run("explicit binding on all statements — single project", func(t *testing.T) { - tenant, err := resolveCustomRoleTenant(models.RoleStatements{ - { - Operations: []string{"secretmanager.secrets.get"}, - Binding: "projects/thand-secrets", - }, - { - Operations: []string{"secretmanager.versions.access"}, - Binding: "projects/thand-secrets", - }, - }, "folders/205090528354") - require.NoError(t, err) - assert.Equal(t, "thand-secrets", tenant.ID) - assert.Equal(t, "project", tenant.Type) - }) - - t.Run("explicit binding without projects/ prefix accepted", func(t *testing.T) { - tenant, err := resolveCustomRoleTenant(models.RoleStatements{ - { - Operations: []string{"secretmanager.secrets.get"}, - Binding: "thand-secrets", - }, - }, "folders/205090528354") - require.NoError(t, err) - assert.Equal(t, "thand-secrets", tenant.ID) - }) - - t.Run("folder binding rejected", func(t *testing.T) { - _, err := resolveCustomRoleTenant(models.RoleStatements{ - { - Operations: []string{"secretmanager.secrets.get"}, - Binding: "folders/205090528354", - }, - }, "folders/205090528354") - require.Error(t, err) - assert.Contains(t, err.Error(), "folder") - }) - - t.Run("conflicting bindings across statements error", func(t *testing.T) { - _, err := resolveCustomRoleTenant(models.RoleStatements{ - { - Operations: []string{"secretmanager.secrets.get"}, - Binding: "projects/proj-a", - }, - { - Operations: []string{"secretmanager.versions.access"}, - Binding: "projects/proj-b", - }, - }, "folders/205090528354") - require.Error(t, err) - assert.Contains(t, err.Error(), "conflicting bindings") - }) - - t.Run("missing binding falls back to target inference", func(t *testing.T) { - buffer := captureWarnings(t) - - // One statement has no Binding — triggers legacy target-inference path - tenant, err := resolveCustomRoleTenant(models.RoleStatements{ - { - Operations: []string{"secretmanager.secrets.get"}, - Targets: []string{"projects/thand-secrets/*"}, - // No Binding - }, - }, "folders/205090528354") - require.NoError(t, err) - assert.Equal(t, "thand-secrets", tenant.ID) - - warningOutput, err := io.ReadAll(buffer) - require.NoError(t, err) - assert.Contains(t, string(warningOutput), "permissions.allow statements are missing 'binding'; inferring project from targets") - }) - - t.Run("organization binding rejected", func(t *testing.T) { - _, err := resolveCustomRoleTenant(models.RoleStatements{ - { - Operations: []string{"secretmanager.secrets.get"}, - Binding: "organizations/123456789", - }, - }, "folders/205090528354") - require.Error(t, err) - assert.Contains(t, err.Error(), "organization") - assert.Contains(t, err.Error(), "organization_id") - }) - - t.Run("partial binding falls back to target inference with specific warning", func(t *testing.T) { - buffer := captureWarnings(t) - - // First statement has binding, second does not — explicit binding should be - // ignored and inference used instead (with a warning logged). - tenant, err := resolveCustomRoleTenant(models.RoleStatements{ - { - Operations: []string{"secretmanager.secrets.get"}, - Targets: []string{"projects/thand-secrets/*"}, - Binding: "projects/thand-secrets", // set - }, - { - Operations: []string{"secretmanager.versions.access"}, - Targets: []string{"projects/thand-secrets/*"}, - // Binding intentionally absent - }, - }, "folders/205090528354") - require.NoError(t, err) - // Despite the explicit binding on the first statement, inference wins - assert.Equal(t, "thand-secrets", tenant.ID) - - warningOutput, err := io.ReadAll(buffer) - require.NoError(t, err) - warningText := string(warningOutput) - assert.Contains(t, warningText, "some permissions.allow statements have 'binding' set but not all") - assert.NotContains(t, warningText, "permissions.allow statements are missing 'binding'; inferring project from targets") - }) -} - func TestStatementRoleID(t *testing.T) { t.Run("single statement uses base name", func(t *testing.T) { stmt := models.Statement{Operations: []string{"secretmanager.secrets.get"}} diff --git a/internal/providers/gcp/rbac.go b/internal/providers/gcp/rbac.go index 750af84e..4e9d7e9b 100644 --- a/internal/providers/gcp/rbac.go +++ b/internal/providers/gcp/rbac.go @@ -41,7 +41,12 @@ func gcpRoleID(name string) string { // Truncate to GCP maximum of 64 characters if len(id) > 64 { - id = strings.TrimRight(id[:64], "_") + trimmed := strings.TrimRight(id[:64], "_") + if len(trimmed) > 0 { + id = trimmed + } else { + id = id[:3] // degenerate case: first 64 chars were all underscores + } } // Pad if too short (minimum 3 characters) @@ -69,6 +74,14 @@ func statementRoleID(baseName string, stmt models.Statement, index, count int) s return gcpRoleID(baseName + "_s" + strconv.Itoa(index)) } +// stmtLabel returns a human-readable label for a statement in error/log messages. +func stmtLabel(stmt models.Statement, index int) string { + if stmt.ID != "" { + return stmt.ID + } + return "s" + strconv.Itoa(index) +} + // primitiveRoles are GCP basic/primitive roles that do not support IAM conditions. // See: https://cloud.google.com/iam/docs/conditions-overview#limitations var primitiveRoles = []string{"roles/owner", "roles/editor", "roles/viewer"} @@ -175,7 +188,7 @@ func (p *gcpProvider) AuthorizeRole( for stmtIdx, stmt := range role.Permissions.Allow { stmtTenant, err := resolveStatementBindingTenant(stmt, tenant, projectId) if err != nil { - return nil, fmt.Errorf("failed to resolve binding for statement %d: %w", stmtIdx, err) + return nil, fmt.Errorf("failed to resolve binding for statement %s (index %d): %w", stmtLabel(stmt, stmtIdx), stmtIdx, err) } stmtResourceID := stmtTenant.ID @@ -218,9 +231,12 @@ func (p *gcpProvider) AuthorizeRole( "is_composite": isComposite, "stmt_index": stmtIdx, "stmt_id": stmt.ID, - "binding": stmt.Binding, - "operations": stmt.Operations, }).Info("Created custom GCP role for statement") + logrus.WithFields(logrus.Fields{ + "role_name": customRoleName, + "binding": stmt.Binding, + "operations": stmt.Operations, + }).Debug("Custom GCP role statement details") } else { needsUpdate := true if !isComposite && !isOrganizationRoleParent(roleParent) { @@ -517,7 +533,7 @@ func (p *gcpProvider) authorizeRoleTemporal( for stmtIdx, stmt := range role.Permissions.Allow { stmtTenant, err := resolveStatementBindingTenant(stmt, tenant, projectID) if err != nil { - return nil, fmt.Errorf("failed to resolve binding for statement %d: %w", stmtIdx, err) + return nil, fmt.Errorf("failed to resolve binding for statement %s (index %d): %w", stmtLabel(stmt, stmtIdx), stmtIdx, err) } customRoleName := statementRoleID(role.GetName(), stmt, stmtIdx, stmtCount) @@ -541,7 +557,7 @@ func (p *gcpProvider) authorizeRoleTemporal( Tenant: stmtTenant, }, ).Get(wfCtx, &resp); err != nil { - return nil, fmt.Errorf("GetOrCreateAndBindCustomRole activity failed for statement %d: %w", stmtIdx, err) + return nil, fmt.Errorf("GetOrCreateAndBindCustomRole activity failed for statement %s (index %d): %w", stmtLabel(stmt, stmtIdx), stmtIdx, err) } assignedRoles = append(assignedRoles, resp.RoleName) } @@ -859,93 +875,6 @@ func projectIDFromRoleParent(roleParent string) (string, bool) { return projectID, true } -// resolveCustomRoleTenant determines the project tenant to use when creating and -// binding a custom GCP role for a set of permission statements. -// -// Resolution order: -// 1. If every statement has an explicit Binding set, and all bindings resolve to -// the same project, use that project. A folders/ binding is rejected because -// GCP does not support custom roles at folder scope. -// 2. Otherwise fall back to inferring the project from Targets (legacy behaviour, -// emits a deprecation warning so operators can migrate to explicit Binding). -// 3. If neither succeeds the error from inference is returned. -func resolveCustomRoleTenant(statements models.RoleStatements, fallbackTenantID string) (*models.ProviderTenant, error) { - // ── Step 1: explicit Binding on all statements ──────────────────────────── - anyHaveBinding := false - allHaveBinding := len(statements) > 0 - for _, stmt := range statements { - if stmt.Binding != "" { - anyHaveBinding = true - } else { - allHaveBinding = false - } - } - - // Warn loudly when a user sets binding on some statements but not all — the - // explicit binding values will be silently ignored in the fallback path, - // which is almost certainly not what was intended. - if anyHaveBinding && !allHaveBinding { - logrus.WithField("fallback_tenant", fallbackTenantID).Warn( - "some permissions.allow statements have 'binding' set but not all; " + - "the explicit binding values will be ignored and the project will be inferred from targets instead. " + - "Set 'binding' on every statement or remove it from all statements.", - ) - } - - if allHaveBinding { - projectIDs := make(map[string]struct{}) - for _, stmt := range statements { - binding := strings.TrimSpace(stmt.Binding) - if strings.HasPrefix(binding, "folders/") { - return nil, fmt.Errorf("binding %q targets a folder; GCP custom roles must be created at project or organization scope", binding) - } - // Organization-level role creation via the binding field is not currently - // implemented: the org IAM API path is not wired up in bindUserToRoleByName. - // Use the provider-level 'organization_id' config to create roles at org scope. - if strings.HasPrefix(binding, "organizations/") { - return nil, fmt.Errorf( - "binding %q targets an organization; organization-level role creation via the 'binding' field is not currently supported — "+ - "set 'organization_id' in the provider configuration to create custom roles at organization scope", - binding, - ) - } - // For GCP we need a project ID, not a full resource string. - // Accept either "projects/{id}" or bare "{id}". - projectID := strings.TrimPrefix(binding, "projects/") - if len(projectID) == 0 { - return nil, fmt.Errorf("binding %q does not resolve to a project", binding) - } - projectIDs[projectID] = struct{}{} - } - - if len(projectIDs) > 1 { - projects := make([]string, 0, len(projectIDs)) - for p := range projectIDs { - projects = append(projects, p) - } - sort.Strings(projects) - return nil, fmt.Errorf("permission statements have conflicting bindings %v; all statements in a role must bind to the same project", projects) - } - - for projectID := range projectIDs { - return &models.ProviderTenant{ID: projectID, Type: "project", Name: projectID}, nil - } - } - - // ── Step 2: legacy fallback – infer from targets ────────────────────────── - if !anyHaveBinding { - logrus.WithField("fallback_tenant", fallbackTenantID).Warn( - "permissions.allow statements are missing 'binding'; inferring project from targets. " + - "Set an explicit 'binding' on each statement to remove this warning.", - ) - } - projectID, err := inferProjectIDFromPermissionTargets(statements) - if err != nil { - return nil, err - } - return &models.ProviderTenant{ID: projectID, Type: "project", Name: projectID}, nil -} - // resolveStatementBindingTenant determines the project tenant for a single permission // statement. Used by the per-statement authorization loop. //