diff --git a/docs/configuration/roles/index.md b/docs/configuration/roles/index.md index 9b7dcfb0..8c237d16 100644 --- a/docs/configuration/roles/index.md +++ b/docs/configuration/roles/index.md @@ -216,14 +216,17 @@ 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 | +| `binding` | string | No | Explicit CSP resource where the role is created and bound (see [Binding](#binding)) | ```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 @@ -232,6 +235,7 @@ permissions: conditions: # Optional provider-specific conditions ConditionOperator: ConditionKey: ConditionValue + binding: "" # Optional: explicit scope for role creation / IAM binding deny: - operations: - action3 @@ -492,6 +496,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, 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. + +#### 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..9f534e75 100644 --- a/docs/configuration/roles/migration.md +++ b/docs/configuration/roles/migration.md @@ -237,6 +237,151 @@ 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: + +``` +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: + +```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. + +--- + +## 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 | @@ -248,6 +393,8 @@ 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 | +| 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..05f14db3 100644 --- a/internal/common/validator.go +++ b/internal/common/validator.go @@ -61,6 +61,38 @@ 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 + } + + // 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 d1fdef2a..5ef97075 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,71 @@ 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) + } + }) + } +} + +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 eb30f193..fad209b9 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 } } @@ -977,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 } @@ -1045,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 @@ -1295,9 +1333,11 @@ 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, + 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/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 e1fc02af..9e7593ba 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 @@ -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"` @@ -519,6 +526,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:"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 775a906d..33faa2fa 100644 --- a/internal/providers/gcp/provider_test.go +++ b/internal/providers/gcp/provider_test.go @@ -256,3 +256,119 @@ 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 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 4cf06479..4e9d7e9b 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" @@ -40,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) @@ -51,6 +57,31 @@ 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)) +} + +// 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"} @@ -119,24 +150,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. - 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) - } - 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( @@ -159,15 +182,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(projectId) + // 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 %s (index %d): %w", stmtLabel(stmt, stmtIdx), 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, @@ -175,7 +208,7 @@ func (p *gcpProvider) AuthorizeRole( role.Role.GetName(), role.GetDescription(), stage, - role.Permissions, + stmtPermissions, ) if err != nil { return nil, temporal.NewApplicationErrorWithOptions( @@ -188,22 +221,26 @@ 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, 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, + }).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 { - // 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, stmtResourceID, customRoleName) requestedVersion := role.GetVersionString() if storedVersion == requestedVersion { needsUpdate = false @@ -215,19 +252,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, projectId, customRoleName, role.GetVersionString()) + p.setRoleVersionLabel(localCtx, stmtResourceID, 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, 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), @@ -242,7 +277,8 @@ func (p *gcpProvider) AuthorizeRole( logrus.WithFields(logrus.Fields{ "user_email": user.Email, "role": existingRole.Name, - "project_id": projectId, + "project_id": stmtResourceID, + "stmt_index": stmtIdx, }).Info("Successfully bound user to custom GCP role") assignedRoles = append(assignedRoles, existingRole.Name) @@ -344,7 +380,20 @@ func (p *gcpProvider) RevokeRole( ) } - err = p.unbindUserFromRole(localCtx, projectId, user, existingRole, tenant) + // 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, + 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 +408,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. @@ -461,12 +510,6 @@ func (p *gcpProvider) authorizeRoleTemporal( return nil, fmt.Errorf("role %s has no inherits or permissions defined", role.Name) } - // 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) - } - var assignedRoles []string for _, inheritedRole := range role.Inherits { @@ -485,9 +528,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 %s (index %d): %w", stmtLabel(stmt, stmtIdx), stmtIdx, err) + } + + customRoleName := statementRoleID(role.GetName(), stmt, stmtIdx, stmtCount) + stmtPermissions := models.RolePermissions{ + Allow: models.RoleStatements{stmt}, + } var resp GetOrCreateAndBindCustomRoleResponse if err := workflow.ExecuteActivity( @@ -499,13 +551,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: tenant, + 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 %s (index %d): %w", stmtLabel(stmt, stmtIdx), stmtIdx, err) } assignedRoles = append(assignedRoles, resp.RoleName) } @@ -567,6 +619,13 @@ func (p *gcpProvider) revokeRoleTemporal( return nil, fmt.Errorf("UnbindUserFromPredefinedRole activity failed for %s: %w", roleName, err) } } else if isComposite { + tenantForRole := tenant + 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 +633,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 := tenant + 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 +653,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 +864,129 @@ 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 +} + +// 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{}) + + 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 // ───────────────────────────────────────────────────────────────────────────── @@ -1177,6 +1366,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:

{{end}} @@ -93,7 +93,7 @@

Permi

Denied:

{{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

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