sec(credentials): validate gcp_client_email + aws_web_identity_token_file + aws_role_arn at API boundary (closes #405, #403, #413) - #504
Conversation
…n_file at API boundary Add format validation for three credential fields at the handler boundary so invalid values are rejected with HTTP 400 before reaching the database or credential resolver. - gcp_client_email: must match the GCP service-account email regex (name@project.iam.gserviceaccount.com); blocks URL path injection into the SA impersonation URL in BuildGCPFederatedCredential (closes #405) - aws_web_identity_token_file: restricted to /var/run/secrets/eks... and /var/run/secrets/kubernetes.io/... prefixes; path traversal sequences rejected; blocks arbitrary host file reads via STS (closes #403) - aws_role_arn: must match the IAM role ARN format for aws/aws-cn/aws-us-gov partitions; blocks malformed strings reaching stscreds (closes #413) All three helpers live in validation.go alongside the existing validators. 108 new test cases cover the attack vectors from each issue plus happy paths.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds three credential field validators to the API layer before account persistence: GCP service account email format validation, AWS IAM role ARN format validation, and AWS web identity token file path validation with prefix allowlisting and path-traversal blocking. Handler integration calls these validators during account creation and update. ChangesCredential Field Validation for Cloud Accounts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/api/validation_test.go (1)
361-363: ⚡ Quick winAssert client-error type explicitly in negative-path cases.
The current conditional check can pass even if the error is not a
ClientError. Make the type assertion mandatory before checking code400, so these tests enforce the API contract.Proposed hardening pattern (apply in all 3 tests)
- if ce, ok := IsClientError(err); ok { - assert.Equal(t, 400, ce.code) - } + ce, ok := IsClientError(err) + if assert.True(t, ok, "expected ClientError") { + assert.Equal(t, 400, ce.code) + }Also applies to: 397-399, 433-435
🤖 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 `@internal/api/validation_test.go` around lines 361 - 363, The tests currently use a conditional type-check pattern "if ce, ok := IsClientError(err); ok { assert.Equal(t, 400, ce.code) }" which allows a non-failing path when the error is not a ClientError; change each of these (the occurrences around IsClientError in internal/api/validation_test.go) to make the type-assertion mandatory: first assert or require that IsClientError(err) returns true (e.g., require.True(t, ok) or assert.True(t, ok)), then use the asserted value (ce) to assert the code equals 400; update the three places noted (around lines 361-363, 397-399, 433-435) to follow this pattern so the test fails if err is not a ClientError.
🤖 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 `@internal/api/validation.go`:
- Line 38: Replace the permissive awsRoleARNRegex with a stricter pattern that
only allows valid IAM role path/name characters: update the awsRoleARNRegex
variable to use a regex like
^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/(?:[A-Za-z0-9+=,.@_-]+\/)*[A-Za-z0-9+=,.@_-]+$
so that the part after "role/" accepts only IAM-safe characters (alphanumerics
and +=,.@_-) and proper path segments, preventing whitespace or other invalid
characters from passing validation (update the awsRoleARNRegex declaration in
internal/api/validation.go accordingly).
---
Nitpick comments:
In `@internal/api/validation_test.go`:
- Around line 361-363: The tests currently use a conditional type-check pattern
"if ce, ok := IsClientError(err); ok { assert.Equal(t, 400, ce.code) }" which
allows a non-failing path when the error is not a ClientError; change each of
these (the occurrences around IsClientError in internal/api/validation_test.go)
to make the type-assertion mandatory: first assert or require that
IsClientError(err) returns true (e.g., require.True(t, ok) or assert.True(t,
ok)), then use the asserted value (ce) to assert the code equals 400; update the
three places noted (around lines 361-363, 397-399, 433-435) to follow this
pattern so the test fails if err is not a ClientError.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 020c2690-2485-44e1-b702-b0c1af72a17f
📒 Files selected for processing (3)
internal/api/handler_accounts.gointernal/api/validation.gointernal/api/validation_test.go
Resolves CodeRabbit feedback on PR #504. The previous awsRoleARNRegex accepted any non-empty suffix after `:role/`, which let whitespace, newlines, and HTML metachars pass validation and reach downstream consumers (stscreds.NewAssumeRoleProvider and any caller that logs or echoes the value). The role name and path segments are now restricted to the IAM-permitted character class `[A-Za-z0-9+=,.@_-]`, with optional path segments separated by `/`. A trailing slash (empty role name) is also rejected. Also tighten the validation_test.go assertions on negative paths so they fail when the returned error is not a ClientError, rather than silently passing the type guard. No behaviour change for valid ARNs.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
validateGCPClientEmailinvalidation.go: rejects anygcp_client_emailthat does not match^[a-zA-Z0-9._-]+@[a-zA-Z0-9.-]+\.iam\.gserviceaccount\.com$, blocking URL path injection into the SA impersonation URL (closes sec(credentials): gcp_client_email not validated — attacker-controlled string injected into SA impersonation URL #405)validateAWSWebIdentityTokenFileinvalidation.go: restrictsaws_web_identity_token_fileto/var/run/secrets/eks.amazonaws.com/serviceaccount/and/var/run/secrets/kubernetes.io/serviceaccount/prefixes, and blocks..segments, preventing arbitrary host file reads via STS (closes sec(credentials): aws_web_identity_token_file not validated at API layer — arbitrary host file read on workload_identity_federation accounts #403)validateAWSRoleARNinvalidation.go: requires non-empty values to match^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.+$, catching malformed ARNs at the API boundary (closes sec(credentials): aws_role_arn not validated as ARN format — any string including attacker-account ARNs accepted #413)validateAuthMode/validateAWSAuthModeinhandler_accounts.goso they fire on both create and updatevalidation_test.gocover the exact attack vectors from each issue plus happy pathsTest plan
go test ./internal/api/...passes (1087 tests)POST /api/accountswithgcp_client_email: "foo@bar.com/../../v1/projects/-/serviceAccounts/attacker@evil.com"returns 400POST /api/accountswithaws_web_identity_token_file: "/proc/self/environ"returns 400POST /api/accountswithaws_role_arn: "not-an-arn"returns 400Summary by CodeRabbit
New Features
Tests