ROSAENG-64874: fix: Cedar schema validation and ARN normalization for non-privileged accounts - #297
Conversation
…accounts - Remove invalid additionalAttributes from Cedar schema (AVP PutSchema rejection) - Add adminArn field to account provisioning to bootstrap initial admin - Require adminArn for non-privileged accounts to prevent admin deadlock - Export and use NormalizeAssumedRoleARN to match STS assumed-role ARNs against stored IAM role ARNs in both IsAdmin and Cedar buildAVPRequest - Add unit tests for ARN normalization and accounts handler validation - Add Cedar policy management docs and cedar.sh helper script Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ixes - Fix FIPS + custom endpoint conflict in DynamoDB client - Fix Cedar template resolution for `principal == ?principal` format - Fix ARN normalization for group membership lookups - Add AdminStore.DeleteAll to clean up stale admins on account deletion - Include Cedar policy text in policy API responses - Add LocalStack-based e2e compose with postgres, cedar-agent, and Keycloak - Add Keycloak OIDC realm (rosa-e2e) with supervisor, admin, and user test accounts - Add init scripts for LocalStack IAM/STS and Keycloak OIDC token provisioning - Add full e2e authz test suite covering account lifecycle, Cedar policy CRUD, group/attachment management, authorization evaluation, and ARN normalization Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cdoan1
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@cdoan1: This pull request references ROSAENG-64874 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoan1 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughChangesThe pull request adds a local authorization E2E environment using LocalStack, Postgres, Cedar Agent, and Keycloak. It adds account administrator provisioning, assumed-role ARN normalization, policy response data, cleanup behavior, and comprehensive authorization lifecycle tests. Authorization and local E2E flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Makefile
participant Compose
participant LocalStack
participant Keycloak
participant run-e2e-authz-local.sh
participant PlatformAPI
participant Ginkgo
Makefile->>Compose: start authorization services
run-e2e-authz-local.sh->>LocalStack: initialize IAM, STS, and DynamoDB
run-e2e-authz-local.sh->>Keycloak: initialize OIDC realm and tokens
run-e2e-authz-local.sh->>PlatformAPI: build and start API
Ginkgo->>PlatformAPI: execute local authorization tests
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
platform-api/pkg/handlers/accounts_test.go (1)
155-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for an
AddAdminfailure.
addAdminFnalways returnsnilin every test. No test drives the failure branch atplatform-api/pkg/handlers/accounts.goLines 97-98. That branch currently only logs and still returns201. Add a test that returns an error fromaddAdminFnand asserts the expected status. The test then pins the chosen behavior for a partially provisioned account.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/pkg/handlers/accounts_test.go` around lines 155 - 179, Add a test alongside TestAccounts_Create_NonPrivilegedWithAdminArn that makes mockAuthzService.addAdminFn return an error, invokes h.Create with the same non-privileged admin request, and asserts the status returned by the AddAdmin failure branch. Keep the test focused on pinning the handler’s current partial-provisioning behavior.test/e2e-authz-local/authz_local_test.go (1)
88-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe suite is not repeatable against a persistent environment.
This test provisions account
111111111111and expects201 Created. TheAccount Re-provisioningcontext deletes account333333333333at Lines 491-499, so that account is repeatable. Account111111111111is never deleted. A second run against the same LocalStack instance receives409 account-existshere, and every later context that depends onnonPrivilegedAccountIDthen fails.Add an
AfterAllthat deletesnonPrivilegedAccountID, or make this step tolerate an existing account.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-authz-local/authz_local_test.go` around lines 88 - 105, Make the Account Provisioning context repeatable by adding an AfterAll cleanup that deletes nonPrivilegedAccountID through the existing client and verifies the cleanup succeeds. Keep the provisioning assertions unchanged and use the same deletion pattern as the Account Re-provisioning context.scripts/e2e-init-localstack.sh (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
CUSTOMER_ACCOUNTis unused.The script sets
CUSTOMER_ACCOUNTat Line 14 and never reads it.CustomerAdminRoleis created with a literal role name at Line 75, and the ARN at Line 85 usesSUPERVISOR_ACCOUNTonly. Remove the variable, or use it where the customer account ID is needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e-init-localstack.sh` around lines 13 - 14, Remove the unused CUSTOMER_ACCOUNT assignment from the script, unless the customer account ID is required by a later resource; in that case, replace the relevant customer-account literal with CUSTOMER_ACCOUNT. Leave SUPERVISOR_ACCOUNT and its existing ARN usage unchanged.Source: Linters/SAST tools
platform-api/pkg/handlers/accounts.go (1)
70-74: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate the format of
adminArn.The check only rejects an empty value. A caller can send any string, for example
not-an-arn. That value is stored as the administrator principal.store.NormalizeAssumedRoleARNthen leaves it unchanged, so the administrator check can never match it, and the account has no usable administrator. Validate thatadminArnis an IAM role or user ARN before you create the account.♻️ Proposed validation
if !req.Privileged && req.AdminArn == "" { h.writeError(w, http.StatusBadRequest, "missing-admin-arn", "adminArn is required for non-privileged accounts") return } + + if req.AdminArn != "" && !adminARNPattern.MatchString(req.AdminArn) { + h.writeError(w, http.StatusBadRequest, "invalid-admin-arn", "adminArn must be an IAM role or user ARN") + return + }Add the anchored pattern at package scope:
var adminARNPattern = regexp.MustCompile(`^arn:aws[a-zA-Z-]*:iam::\d{12}:(role|user)/[\w+=,.@/-]+$`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/pkg/handlers/accounts.go` around lines 70 - 74, Validate req.AdminArn against an anchored IAM role/user ARN pattern at package scope before account creation, while retaining the existing required-value check for non-privileged accounts. Reject malformed values with the handler’s bad-request error path, using the adminARNPattern symbol and ensuring both role and user ARNs with valid 12-digit account IDs are accepted.platform-api/pkg/handlers/authz.go (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
policyis absent from list responses.
CreatePolicy,GetPolicy, andUpdatePolicypopulateCedarPolicy.ListPoliciesat Lines 189-198 does not.omitemptyhides the field, so a client cannot tell whether the policy text is empty or simply not returned by the list endpoint. Add a comment on the field, or populate it inListPoliciesas well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/pkg/handlers/authz.go` at line 43, Document the `CedarPolicy` field’s endpoint-specific behavior in its struct comment: `CreatePolicy`, `GetPolicy`, and `UpdatePolicy` populate it, while `ListPolicies` intentionally omits it and `omitempty` hides the field. Do not change list response population unless that behavior is intended to change.hack/podman-compose.e2e-authz.yaml (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the container image tags.
localstack/localstack-pro:latestanddocker.io/permitio/cedar-agent:latestfloat. A new upstream release can break the local suite with no repository change, and failures are then hard to reproduce. Pin both to an explicit version, as the file already does forpostgres:16-alpineandkeycloak:26.0.Also applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/podman-compose.e2e-authz.yaml` at line 26, Replace the floating latest tags for the LocalStack and Cedar Agent images in the compose configuration with explicit, fixed version tags, matching the existing pinned-image style used by postgres and keycloak.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api/cedar-policies.md`:
- Line 22: Update the flow-diagram fenced code block in the Cedar policies
documentation to specify the `text` language identifier, leaving the diagram
content unchanged.
- Line 117: Correct the Step 2 heading by replacing the misspelled “CedaÏter”
with “Cedar,” leaving the rest of the heading unchanged.
In `@platform-api/pkg/authz/authz.go`:
- Around line 403-408: Update the account deletion flow around
adminStore.DeleteAll to return the cleanup error immediately when it fails,
preventing deletion of the account record and allowing the caller to retry.
Preserve the existing warning log, and only continue to delete the account after
DeleteAll succeeds.
- Around line 197-205: Update the authorization context construction to use the
already normalized principalARN for context.principalArn, alongside the
principal EntityIdentifier, so assumed-role callers consistently evaluate
against IAM role ARNs. Do not use req.CallerARN for this context field unless a
separate raw-ARN attribute is explicitly documented.
In `@platform-api/pkg/authz/store/admins.go`:
- Around line 165-174: Update AdminStore.DeleteAll to iterate through every page
returned by the underlying administrator query, continuing with LastEvaluatedKey
or the established SDK paginator until no pages remain, and remove each admin
from every page. Add a test covering multiple pages and verify all administrator
records are deleted.
- Around line 119-135: Update NormalizeAssumedRoleARN in
platform-api/pkg/authz/store/admins.go: require both a non-empty role name and
session name in the assumed-role resource before converting it to an IAM role
ARN; otherwise return the original ARN unchanged. Update
platform-api/pkg/authz/store/admins_test.go lines 42-45 to retain and verify the
no-session ARN remains unnormalized.
In `@platform-api/pkg/handlers/accounts.go`:
- Around line 96-103: Update the account-creation handler around
authorizer.AddAdmin so an AddAdmin failure is returned to the caller instead of
merely logged and proceeding with 201 Created. On failure, invoke
h.authorizer.DisableAccount for req.AccountID to roll back the newly created
account, then return the appropriate error response; preserve the successful
logging and response path when AddAdmin succeeds.
In `@scripts/e2e-init-keycloak.sh`:
- Around line 82-87: Both init scripts create secret-bearing credential files
with permissive default modes. In scripts/e2e-init-keycloak.sh at lines 82-87,
touch $CREDENTIALS_FILE and chmod it to 600 before the heredoc writes the OIDC
tokens; apply the same touch-and-chmod-600 preparation in
scripts/e2e-init-localstack.sh at lines 125-129 before writing the STS
credentials.
- Around line 54-72: Update fetch_token so its informational echo messages,
including the fetching notice and token-length diagnostic, are written to
stderr; keep only the raw access token on stdout for command substitution into
SUPERVISOR_TOKEN and other callers.
In `@scripts/run-e2e-authz-local.sh`:
- Around line 19-38: The local runner scripts must stop embedding or exposing
PostgreSQL credentials. In scripts/run-e2e-authz-local.sh lines 19-38, require
the secret-backed POSTGRES_DSN environment variable without a credential-bearing
default and replace the full DSN output with non-sensitive connection metadata.
In scripts/run-e2e-authz.sh line 29, remove the credential-bearing command-line
DSN and configure the API using POSTGRES_DSN from the environment.
In `@test/e2e-authz-local/authz_local_test.go`:
- Around line 80-85: Update the policies-list handler used by the
privilegedAccountID request to return a defined, non-error response when no
policy store exists, then revise the test to assert that intended status and
body/code instead of expecting http.StatusInternalServerError with
“internal-error”.
---
Nitpick comments:
In `@hack/podman-compose.e2e-authz.yaml`:
- Line 26: Replace the floating latest tags for the LocalStack and Cedar Agent
images in the compose configuration with explicit, fixed version tags, matching
the existing pinned-image style used by postgres and keycloak.
In `@platform-api/pkg/handlers/accounts_test.go`:
- Around line 155-179: Add a test alongside
TestAccounts_Create_NonPrivilegedWithAdminArn that makes
mockAuthzService.addAdminFn return an error, invokes h.Create with the same
non-privileged admin request, and asserts the status returned by the AddAdmin
failure branch. Keep the test focused on pinning the handler’s current
partial-provisioning behavior.
In `@platform-api/pkg/handlers/accounts.go`:
- Around line 70-74: Validate req.AdminArn against an anchored IAM role/user ARN
pattern at package scope before account creation, while retaining the existing
required-value check for non-privileged accounts. Reject malformed values with
the handler’s bad-request error path, using the adminARNPattern symbol and
ensuring both role and user ARNs with valid 12-digit account IDs are accepted.
In `@platform-api/pkg/handlers/authz.go`:
- Line 43: Document the `CedarPolicy` field’s endpoint-specific behavior in its
struct comment: `CreatePolicy`, `GetPolicy`, and `UpdatePolicy` populate it,
while `ListPolicies` intentionally omits it and `omitempty` hides the field. Do
not change list response population unless that behavior is intended to change.
In `@scripts/e2e-init-localstack.sh`:
- Around line 13-14: Remove the unused CUSTOMER_ACCOUNT assignment from the
script, unless the customer account ID is required by a later resource; in that
case, replace the relevant customer-account literal with CUSTOMER_ACCOUNT. Leave
SUPERVISOR_ACCOUNT and its existing ARN usage unchanged.
In `@test/e2e-authz-local/authz_local_test.go`:
- Around line 88-105: Make the Account Provisioning context repeatable by adding
an AfterAll cleanup that deletes nonPrivilegedAccountID through the existing
client and verifies the cleanup succeeds. Keep the provisioning assertions
unchanged and use the same deletion pattern as the Account Re-provisioning
context.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 28f11a72-b3cf-47a5-aba7-aec4d5d4ac50
📒 Files selected for processing (20)
.gitignoreMakefiledocs/api/cedar-policies.mdhack/keycloak/rosa-e2e-realm.jsonhack/podman-compose.e2e-authz.yamlplatform-api/pkg/authz/authz.goplatform-api/pkg/authz/client/dynamodb.goplatform-api/pkg/authz/client/mock_avp.goplatform-api/pkg/authz/schema/rosa.cedarschema.jsonplatform-api/pkg/authz/store/admins.goplatform-api/pkg/authz/store/admins_test.goplatform-api/pkg/handlers/accounts.goplatform-api/pkg/handlers/accounts_test.goplatform-api/pkg/handlers/authz.goscripts/e2e-init-keycloak.shscripts/e2e-init-localstack.shscripts/run-e2e-authz-local.shscripts/run-e2e-authz.shtest/e2e-authz-local/authz_local_test.gotest/e2e-authz-local/suite_test.go
|
|
||
| ## Authorization Flow | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced block.
Set the flow-diagram fence language to text. This resolves markdownlint MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/api/cedar-policies.md` at line 22, Update the flow-diagram fenced code
block in the Cedar policies documentation to specify the `text` language
identifier, leaving the diagram content unchanged.
Source: Linters/SAST tools
|
|
||
|
|
||
|
|
||
| ### Step 2: Create a CedaÏter policy (admin caller) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the Step 2 heading.
Replace CedaÏter with Cedar.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/api/cedar-policies.md` at line 117, Correct the Step 2 heading by
replacing the misspelled “CedaÏter” with “Cedar,” leaving the rest of the
heading unchanged.
Source: Linters/SAST tools
| // Build principal — normalize STS assumed-role ARNs to IAM role ARNs | ||
| // so they match the IAM ARN used when attaching policies. | ||
| principalARN := store.NormalizeAssumedRoleARN(req.CallerARN) | ||
| if principalARN != req.CallerARN { | ||
| a.logger.Info("normalized assumed-role ARN for Cedar evaluation", "original", req.CallerARN, "normalized", principalARN) | ||
| } | ||
| principal := &avptypes.EntityIdentifier{ | ||
| EntityType: aws.String("ROSA::Principal"), | ||
| EntityId: aws.String(req.CallerARN), | ||
| EntityId: aws.String(principalARN), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how principalArn context attribute is used in Cedar policies, schema, and docs.
fd -t f -e md -e json -e cedar . | xargs rg -n 'principalArn|principalAccount' -C 3
rg -n 'principalArn' --type=go -C 3Repository: openshift-online/rosa-hyperfleet-api
Length of output: 7351
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- authz.go relevant sections ---'
sed -n '140,240p' platform-api/pkg/authz/authz.go
sed -n '380,420p' platform-api/pkg/authz/authz.go
printf '%s\n' '--- normalization implementation and callers ---'
rg -n 'NormalizeAssumedRoleARN|principalArn' platform-api/pkg --type go -C 4
printf '%s\n' '--- Cedar policy documentation and condition-key tests ---'
sed -n '420,445p' docs/authz.md
cat -n platform-api/pkg/authz/testdata/policies/07-condition-keys/principal-arn-condition.cedar
cat -n platform-api/pkg/authz/testdata/policies/07-condition-keys/principal-arn-condition.jsonRepository: openshift-online/rosa-hyperfleet-api
Length of output: 19593
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- normalization implementation ---'
sed -n '84,145p' platform-api/pkg/authz/store/admins.go
printf '%s\n' '--- buildAVPRequest tests and CallerARN fixtures ---'
rg -n 'buildAVPRequest|CallerARN|principalArn|assumed-role' platform-api/pkg/authz --type go -C 5
printf '%s\n' '--- all policy references to principal entity and context ARN ---'
rg -n 'ROSA::Principal|context\.principalArn|principalArn' --glob '*.cedar' --glob '*.md' --glob '*.json' .Repository: openshift-online/rosa-hyperfleet-api
Length of output: 20058
Normalize context.principalArn for assumed-role callers
principal uses NormalizeAssumedRoleARN(req.CallerARN), but context.principalArn uses the raw ARN. Policies that match IAM role ARNs will not match STS assumed-role callers. Set context.principalArn to principalARN, or document a separate raw ARN attribute.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/pkg/authz/authz.go` around lines 197 - 205, Update the
authorization context construction to use the already normalized principalARN
for context.principalArn, alongside the principal EntityIdentifier, so
assumed-role callers consistently evaluate against IAM role ARNs. Do not use
req.CallerARN for this context field unless a separate raw-ARN attribute is
explicitly documented.
| // Clean up all admins for this account so stale entries don't carry over | ||
| // if the account is re-provisioned with a different adminArn. | ||
| if err := a.adminStore.DeleteAll(ctx, accountID); err != nil { | ||
| a.logger.Warn("failed to clean up admins during account deletion", "error", err, "account_id", accountID) | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not delete the account when admin cleanup fails.
DeleteAll removes admin records one by one and returns on the first failure (see platform-api/pkg/authz/store/admins.go:165-177). A partial failure therefore leaves admin records for accountID in the admins table. The account row is then deleted anyway. If the account is later re-provisioned with a different adminArn, the stale admins keep admin access, which is the exact case this cleanup is meant to prevent. The local suite asserts this guarantee in test/e2e-authz-local/authz_local_test.go Lines 522-528.
Return the error so the caller can retry, and delete the account record only after cleanup succeeds.
🔒 Proposed fix
// Clean up all admins for this account so stale entries don't carry over
// if the account is re-provisioned with a different adminArn.
if err := a.adminStore.DeleteAll(ctx, accountID); err != nil {
- a.logger.Warn("failed to clean up admins during account deletion", "error", err, "account_id", accountID)
+ a.logger.Error("failed to clean up admins during account deletion", "error", err, "account_id", accountID)
+ return fmt.Errorf("failed to clean up admins for account %s: %w", accountID, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Clean up all admins for this account so stale entries don't carry over | |
| // if the account is re-provisioned with a different adminArn. | |
| if err := a.adminStore.DeleteAll(ctx, accountID); err != nil { | |
| a.logger.Warn("failed to clean up admins during account deletion", "error", err, "account_id", accountID) | |
| } | |
| // Clean up all admins for this account so stale entries don't carry over | |
| // if the account is re-provisioned with a different adminArn. | |
| if err := a.adminStore.DeleteAll(ctx, accountID); err != nil { | |
| a.logger.Error("failed to clean up admins during account deletion", "error", err, "account_id", accountID) | |
| return fmt.Errorf("failed to clean up admins for account %s: %w", accountID, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/pkg/authz/authz.go` around lines 403 - 408, Update the account
deletion flow around adminStore.DeleteAll to return the cleanup error
immediately when it fails, preventing deletion of the account record and
allowing the caller to retry. Preserve the existing warning log, and only
continue to delete the account after DeleteAll succeeds.
| func NormalizeAssumedRoleARN(arn string) string { | ||
| if !strings.Contains(arn, ":assumed-role/") { | ||
| return arn | ||
| } | ||
| parts := strings.SplitN(arn, ":", 6) | ||
| if len(parts) < 6 { | ||
| return arn | ||
| } | ||
| resource := parts[5] | ||
| if !strings.HasPrefix(resource, "assumed-role/") { | ||
| return arn | ||
| } | ||
| segments := strings.SplitN(strings.TrimPrefix(resource, "assumed-role/"), "/", 2) | ||
| roleName := segments[0] | ||
| parts[2] = "iam" | ||
| parts[5] = "role/" + roleName | ||
| return strings.Join(parts, ":") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 --type go 'type AuthzRequest|CallerARN|Authorize\(' platform-apiRepository: openshift-online/rosa-hyperfleet-api
Length of output: 50392
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- authz call paths ---'
rg -n -C 8 'func \(.*\) IsAdmin|func IsAdmin|NormalizeAssumedRoleARN|CallerARN|HeaderCallerARN|Principal' \
platform-api/pkg/authz platform-api/pkg/middleware platform-api/pkg/handlers/authz.go
printf '%s\n' '--- relevant source sections ---'
sed -n '1,230p' platform-api/pkg/authz/store/admins.go
sed -n '1,90p' platform-api/pkg/middleware/identity.go
sed -n '620,680p' platform-api/pkg/handlers/authz.go
sed -n '1,90p' platform-api/pkg/authz/authz.go
printf '%s\n' '--- tests for admin and normalization behavior ---'
sed -n '1,180p' platform-api/pkg/authz/store/admins_test.go
rg -n -C 8 'IsAdmin|Admin|Authorize' platform-api/pkg/authz/store/*_test.go platform-api/pkg/authz/*_test.goRepository: openshift-online/rosa-hyperfleet-api
Length of output: 50393
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- admin store implementation ---'
cat -n platform-api/pkg/authz/store/admins.go | sed -n '1,180p'
printf '%s\n' '--- normalization and admin tests ---'
cat -n platform-api/pkg/authz/store/admins_test.go | sed -n '1,130p'
rg -n -C 4 'IsAdmin\(|adminStore|NormalizeAssumedRoleARN' platform-api/pkg/authz --glob '*_test.go'
printf '%s\n' '--- routes and middleware ordering ---'
rg -n -C 5 'Identity\(|NewAuthz|Authz\{|RequireAdmin|HeaderCallerARN|CheckAuthorization' platform-api/pkg/server platform-api/pkg/middleware platform-api/pkg/handlers --glob '*.go' --glob '!**/*_test.go'
printf '%s\n' '--- direct callers of Authorize and IsAdmin ---'
rg -n '(\.Authorize|\.IsAdmin)\(' platform-api --glob '*.go' --glob '!**/*_test.go'Repository: openshift-online/rosa-hyperfleet-api
Length of output: 22792
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- privileged middleware behavior ---'
cat -n platform-api/pkg/middleware/privileged.go | sed -n '1,150p'
printf '%s\n' '--- authorization route setup ---'
cat -n platform-api/pkg/server/server.go | sed -n '140,165p'
cat -n platform-api/pkg/handlers/authz.go | sed -n '628,682p'
printf '%s\n' '--- deterministic normalization probe ---'
python3 - <<'PY'
def normalize(arn):
if ":assumed-role/" not in arn:
return arn
parts = arn.split(":", 5)
if len(parts) < 6:
return arn
resource = parts[5]
if not resource.startswith("assumed-role/"):
return arn
segments = resource.removeprefix("assumed-role/").split("/", 1)
role_name = segments[0]
parts[2] = "iam"
parts[5] = "role/" + role_name
return ":".join(parts)
cases = [
"arn:aws:sts::123456789012:assumed-role/MyRole",
"arn:aws:sts::123456789012:assumed-role/MyRole/session",
"arn:aws:sts::123456789012:assumed-role//session",
"arn:aws:sts::123456789012:assumed-role/MyRole/",
]
for value in cases:
print(f"{value!r} -> {normalize(value)!r}")
PYRepository: openshift-online/rosa-hyperfleet-api
Length of output: 8063
Reject assumed-role ARNs without a session name.
Authorize checks the supplied principal against the admin store before Cedar authorization. The /api/v0/authz/check endpoint accepts that principal from the request body, so assumed-role/MyRole can normalize to a stored IAM role ARN and trigger the admin bypass. Require non-empty role and session components before normalization, and update platform-api/pkg/authz/store/admins_test.go#L42-L45 to retain the no-session ARN.
📍 Affects 2 files
platform-api/pkg/authz/store/admins.go#L119-L135(this comment)platform-api/pkg/authz/store/admins_test.go#L42-L45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/pkg/authz/store/admins.go` around lines 119 - 135, Update
NormalizeAssumedRoleARN in platform-api/pkg/authz/store/admins.go: require both
a non-empty role name and session name in the assumed-role resource before
converting it to an IAM role ARN; otherwise return the original ARN unchanged.
Update platform-api/pkg/authz/store/admins_test.go lines 42-45 to retain and
verify the no-session ARN remains unnormalized.
| if req.AdminArn != "" && !req.Privileged { | ||
| if err := h.authorizer.AddAdmin(ctx, req.AccountID, req.AdminArn, callerARN); err != nil { | ||
| h.logger.Error("failed to add initial admin", "error", err, "account_id", req.AccountID, "admin_arn", req.AdminArn) | ||
| } else { | ||
| h.logger.Info("initial admin added", "account_id", req.AccountID, "admin_arn", req.AdminArn) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not return 201 when AddAdmin fails.
The handler rejects a non-privileged request without adminArn at Lines 70-73. That rule makes the initial administrator mandatory. Here, an AddAdmin failure is only logged, and the handler still returns 201 Created. The result is a non-privileged account with no administrator. No principal can then manage policies, groups, or admins for that account, and the caller receives no error.
Fail the request when AddAdmin fails. Roll back the account with DisableAccount so the caller can retry.
🐛 Proposed fix
if req.AdminArn != "" && !req.Privileged {
if err := h.authorizer.AddAdmin(ctx, req.AccountID, req.AdminArn, callerARN); err != nil {
h.logger.Error("failed to add initial admin", "error", err, "account_id", req.AccountID, "admin_arn", req.AdminArn)
- } else {
- h.logger.Info("initial admin added", "account_id", req.AccountID, "admin_arn", req.AdminArn)
+ if derr := h.authorizer.DisableAccount(ctx, req.AccountID); derr != nil {
+ h.logger.Error("failed to roll back account after admin bootstrap failure", "error", derr, "account_id", req.AccountID)
+ }
+ h.writeError(w, http.StatusInternalServerError, "admin-bootstrap-failed", "Failed to add the initial administrator")
+ return
}
+ h.logger.Info("initial admin added", "account_id", req.AccountID, "admin_arn", req.AdminArn)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if req.AdminArn != "" && !req.Privileged { | |
| if err := h.authorizer.AddAdmin(ctx, req.AccountID, req.AdminArn, callerARN); err != nil { | |
| h.logger.Error("failed to add initial admin", "error", err, "account_id", req.AccountID, "admin_arn", req.AdminArn) | |
| } else { | |
| h.logger.Info("initial admin added", "account_id", req.AccountID, "admin_arn", req.AdminArn) | |
| } | |
| } | |
| if req.AdminArn != "" && !req.Privileged { | |
| if err := h.authorizer.AddAdmin(ctx, req.AccountID, req.AdminArn, callerARN); err != nil { | |
| h.logger.Error("failed to add initial admin", "error", err, "account_id", req.AccountID, "admin_arn", req.AdminArn) | |
| if derr := h.authorizer.DisableAccount(ctx, req.AccountID); derr != nil { | |
| h.logger.Error("failed to roll back account after admin bootstrap failure", "error", derr, "account_id", req.AccountID) | |
| } | |
| h.writeError(w, http.StatusInternalServerError, "admin-bootstrap-failed", "Failed to add the initial administrator") | |
| return | |
| } | |
| h.logger.Info("initial admin added", "account_id", req.AccountID, "admin_arn", req.AdminArn) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/pkg/handlers/accounts.go` around lines 96 - 103, Update the
account-creation handler around authorizer.AddAdmin so an AddAdmin failure is
returned to the caller instead of merely logged and proceeding with 201 Created.
On failure, invoke h.authorizer.DisableAccount for req.AccountID to roll back
the newly created account, then return the appropriate error response; preserve
the successful logging and response path when AddAdmin succeeds.
| fetch_token() { | ||
| local username="$1" | ||
| local password="$2" | ||
| local label="$3" | ||
|
|
||
| echo " Fetching token for $label ($username)..." | ||
| TOKEN_RESPONSE=$(curl -sf -X POST "$TOKEN_URL" \ | ||
| -H "Content-Type: application/x-www-form-urlencoded" \ | ||
| -d "grant_type=password" \ | ||
| -d "client_id=$CLIENT_ID" \ | ||
| -d "client_secret=$CLIENT_SECRET" \ | ||
| -d "username=$username" \ | ||
| -d "password=$password" \ | ||
| -d "scope=openid") | ||
|
|
||
| ACCESS_TOKEN=$(echo "$TOKEN_RESPONSE" | python3 -c "import sys,json; print(json.load(sys.stdin)['access_token'])") | ||
| echo " token length: ${#ACCESS_TOKEN} chars" | ||
| echo "$ACCESS_TOKEN" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
fetch_token writes log text to stdout, so the captured token is corrupted.
Line 77 captures the whole stdout of fetch_token into SUPERVISOR_TOKEN. fetch_token writes three things to stdout: the message at Line 59, the token length at Line 70, and the token at Line 71. The captured value is therefore all three lines joined, not the access token. Lines 84-86 then write that multi-line value into $CREDENTIALS_FILE. Every consumer that builds an Authorization: Bearer header from these variables sends an invalid token.
Send the informational messages to stderr.
🐛 Proposed fix
fetch_token() {
local username="$1"
local password="$2"
local label="$3"
+ local TOKEN_RESPONSE ACCESS_TOKEN
- echo " Fetching token for $label ($username)..."
+ echo " Fetching token for $label ($username)..." >&2
TOKEN_RESPONSE=$(curl -sf -X POST "$TOKEN_URL" \
-H "Content-Type: application/x-www-form-urlencoded" \
-d "grant_type=password" \
-d "client_id=$CLIENT_ID" \
-d "client_secret=$CLIENT_SECRET" \
-d "username=$username" \
-d "password=$password" \
-d "scope=openid")
ACCESS_TOKEN=$(echo "$TOKEN_RESPONSE" | python3 -c "import sys,json; print(json.load(sys.stdin)['access_token'])")
- echo " token length: ${`#ACCESS_TOKEN`} chars"
+ echo " token length: ${`#ACCESS_TOKEN`} chars" >&2
echo "$ACCESS_TOKEN"
}Also applies to: 77-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/e2e-init-keycloak.sh` around lines 54 - 72, Update fetch_token so its
informational echo messages, including the fetching notice and token-length
diagnostic, are written to stderr; keep only the raw access token on stdout for
command substitution into SUPERVISOR_TOKEN and other callers.
| cat > "$CREDENTIALS_FILE" <<EOF | ||
| export KEYCLOAK_ISSUER_URL="$ISSUER_URL" | ||
| export SUPERVISOR_OIDC_TOKEN="$SUPERVISOR_TOKEN" | ||
| export CUSTOMER_ADMIN_OIDC_TOKEN="$ADMIN_TOKEN" | ||
| export CUSTOMER_USER_OIDC_TOKEN="$USER_TOKEN" | ||
| EOF |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Both init scripts write secrets to /tmp with default permissions. Each script uses cat > "$CREDENTIALS_FILE", which creates the file with the process umask. The default path is under /tmp, which every local user can read. The shared root cause is the missing mode restriction before the redirection.
scripts/e2e-init-keycloak.sh#L82-L87: create$CREDENTIALS_FILEwithtouchandchmod 600before the heredoc writes the three OIDC bearer tokens.scripts/e2e-init-localstack.sh#L125-L129: create$CREDENTIALS_FILEwithtouchandchmod 600before the heredoc writes the STS secret access key and session token.
📍 Affects 2 files
scripts/e2e-init-keycloak.sh#L82-L87(this comment)scripts/e2e-init-localstack.sh#L125-L129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/e2e-init-keycloak.sh` around lines 82 - 87, Both init scripts create
secret-bearing credential files with permissive default modes. In
scripts/e2e-init-keycloak.sh at lines 82-87, touch $CREDENTIALS_FILE and chmod
it to 600 before the heredoc writes the OIDC tokens; apply the same
touch-and-chmod-600 preparation in scripts/e2e-init-localstack.sh at lines
125-129 before writing the STS credentials.
| POSTGRES_DSN="${POSTGRES_DSN:-postgres://rosa:rosa@localhost:5432/hyperfleet?sslmode=disable}" | ||
| CREDENTIALS_FILE="/tmp/e2e-localstack-credentials.env" | ||
| KEYCLOAK_CREDENTIALS_FILE="/tmp/e2e-keycloak-credentials.env" | ||
|
|
||
| cleanup() { | ||
| echo "Cleaning up..." | ||
| if [[ -f "$PIDFILE" ]] && kill -0 "$(cat "$PIDFILE")" 2>/dev/null; then | ||
| kill "$(cat "$PIDFILE")" 2>/dev/null || true | ||
| wait "$(cat "$PIDFILE")" 2>/dev/null || true | ||
| fi | ||
| rm -f "$PIDFILE" "$CREDENTIALS_FILE" "$KEYCLOAK_CREDENTIALS_FILE" | ||
| } | ||
| trap cleanup EXIT INT TERM | ||
|
|
||
| echo "=== Local Authz E2E Tests ===" | ||
| echo "LocalStack: $LOCALSTACK_ENDPOINT" | ||
| echo "DynamoDB: $DYNAMODB_ENDPOINT" | ||
| echo "Cedar Agent: $CEDAR_AGENT_ENDPOINT" | ||
| echo "Keycloak: $KEYCLOAK_URL" | ||
| echo "Postgres: $POSTGRES_DSN" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not embed or print PostgreSQL credentials.
The local runner prints the full POSTGRES_DSN, including any password supplied by an operator. Both runners also commit DSNs with embedded passwords. Use a required secret-backed environment variable and print only non-sensitive connection metadata.
scripts/run-e2e-authz-local.sh#L19-L38: remove the credential-bearing default and do not printPOSTGRES_DSN.scripts/run-e2e-authz.sh#L29-L29: remove the credential-bearing command-line DSN and use the APIPOSTGRES_DSNenvironment configuration.
As per coding guidelines, flag hardcoded passwords, URLs with embedded credentials, and logging that may expose passwords.
📍 Affects 2 files
scripts/run-e2e-authz-local.sh#L19-L38(this comment)scripts/run-e2e-authz.sh#L29-L29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/run-e2e-authz-local.sh` around lines 19 - 38, The local runner
scripts must stop embedding or exposing PostgreSQL credentials. In
scripts/run-e2e-authz-local.sh lines 19-38, require the secret-backed
POSTGRES_DSN environment variable without a credential-bearing default and
replace the full DSN output with non-sensitive connection metadata. In
scripts/run-e2e-authz.sh line 29, remove the credential-bearing command-line DSN
and configure the API using POSTGRES_DSN from the environment.
Source: Coding guidelines
| It("should not have policy store (privileged accounts bypass Cedar)", func() { | ||
| resp, err := client.Get("/api/v0/authz/policies", privilegedAccountID) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(resp.StatusCode).To(Equal(http.StatusInternalServerError)) | ||
| Expect(string(resp.Body)).To(ContainSubstring("internal-error")) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not assert a 500 response as expected behavior.
The test requires 500 internal-error when a privileged account lists policies. A privileged account has no policy store, which is a known and expected state, not a server fault. Encoding 500 as the contract prevents a later fix from returning 200 with an empty list or a 4xx with a clear code, because this test would then fail.
Change the handler to return a defined response for the no-policy-store case, and assert that response here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e-authz-local/authz_local_test.go` around lines 80 - 85, Update the
policies-list handler used by the privilegedAccountID request to return a
defined, non-error response when no policy store exists, then revise the test to
assert that intended status and body/code instead of expecting
http.StatusInternalServerError with “internal-error”.
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
Bot
added
the
needs-rebase
2 participants
Description
format doesn't support it, causing validation failures
so that admin checks, group lookups, and Cedar policy evaluation match the IAM ARN stored at provisioning time
accounts provide an initial admin ARN and auto-provisions it
(LocalStack), since the AWS SDK rejects the combination
text
a comprehensive Ginkgo test suite covering the full account lifecycle
Type of Change
Testing
make test)Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation