Skip to content

fix(api): validate registration credentials and GCP WIF endpoint URLs - #1414

Draft
IAmJSD wants to merge 3 commits into
LeanerCloud:mainfrom
IAmJSD:fix/credential-validation
Draft

IAmJSD wants to merge 3 commits into
LeanerCloud:mainfrom
IAmJSD:fix/credential-validation

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

Credentials can be submitted two ways: with a registration (POST /api/register, saved when an admin approves the registration) and through POST /api/accounts/:id/credentials. The second path checks the credential type and the payload shape. The first path didn't, so a registration's credential was saved exactly as sent. This PR runs the same checks on both paths. It also adds an endpoint check for GCP workload identity federation (WIF) configs, which runs when a config is saved and again before a stored config is used.

This complements #1308, which pins the JSON type each stored GCP credential slot may hold. The two branches merge cleanly, and the combined tree passes go test -race ./internal/credentials/ ./internal/api/.

What is checked now

Registration credentials (internal/api/handler_registrations_credentials.go)

  • credential_type and credential_payload must be sent together or not at all. Registrations without credentials are still accepted.
  • credential_type must be one of the four known types, and it must belong to the registration's provider (aws_access_keys→aws, azure_client_secret→azure, gcp_*→gcp).
  • credential_payload must be a JSON object that passes validateCredentialPayload, the same check POST /api/accounts/:id/credentials runs.
  • The payload is re-encoded from the checked object, so the stored bytes are exactly the ones that were checked.
  • On approval, the stored credential is decrypted and checked again against the provider of the account being created. This happens before the registration leaves pending. A credential that fails gets a 400 telling the admin to reject the registration. Nothing is transitioned, created or saved. Before this change, a decrypt failure or a payload with no type was logged and skipped, and the account was still created and enabled.
  • Without a credential store, the payload was already dropped at submit time. The type is now dropped with it.

GCP WIF endpoints (internal/credentials/gcp_endpoints.go, used by validateGCPWIFPayload)

  • token_url is required and must be https://sts.googleapis.com/....
  • service_account_impersonation_url is optional. When set, it must be a non-empty string of the form https://iamcredentials.googleapis.com/....
  • Neither URL may have user info or an explicit port, and the host must match exactly. That rejects look-alike hosts such as sts.googleapis.com.example.net and my-sts.googleapis.com, http:// URLs, and IP literals.
  • loadStoredGCPTokenSource runs the same check on any stored external_account JSON before building a token source. It decodes the JSON the same way golang.org/x/oauth2/google does (case-insensitive keys, last duplicate wins), so it checks the value that library would use. A failing config makes the load return a clear error that names the field but not the stored value. The load does not panic or fall back. A stored WIF config still takes priority over the secret-free federated path, and the check covers that case.

Compatibility

  • Who could be affected: stored or submitted GCP WIF configs whose token_url or service_account_impersonation_url points anywhere other than Google's public STS / IAM Credentials endpoints, for example a private or regional STS endpoint, a proxy, or a URL with an explicit :443. gcloud-generated configs and the gcp-target IaC (iac/federation/gcp-target/terraform/registration.tf) use exactly https://sts.googleapis.com/v1/token and https://iamcredentials.googleapis.com/..., and both pass (tested). An affected account's GCP calls fail with "stored gcp workload identity config for account … is not usable: …; save the config again" until the config is saved again with Google endpoints.
  • Registrations that send a credential which POST /api/accounts/:id/credentials would also reject now get a 400 at submit. Pending registrations that already hold such a credential can't be approved. The admin gets a 400 and can reject the registration and ask for a new one.
  • Secret-free federated GCP accounts, application-default, Azure and AWS paths are unchanged, apart from the registration checks above.

Verification

All commands were run from the worktree on Linux, with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true. Nothing calls a real cloud API: building a token source makes no network calls, and no test calls Token().

  • go build ./... && go vet ./... && go vet -tags integration ./... && (cd tests/e2e && go vet -tags e2e ./...): clean
  • golangci-lint run v2.10.1 on ./internal/api/... ./internal/credentials/..., with and without --build-tags integration: 0 issues
  • gocyclo -over 10 on the changed non-test files: none
  • go test -race -short -count=1 ./internal/api/ ./internal/credentials/: ok
  • go test -race -tags integration -count=1 -timeout 45m ./internal/api/ ./internal/credentials/: credentials ok. api: every test passed except TestIntegration_HistorySummary_TotalsCoverAllInScopeRows, whose Postgres test container did not become ready within the timeout. The same test fails the same way on unmodified origin/main on this VM (Docker was heavily loaded at the time), and it does not touch the code changed here. CI's Integration Tests job should confirm it.
  • Fail-before / pass-after: I put the new tests on a tree where the non-test part of the fix was reverted. handler_registrations.go, validation.go and resolver.go were restored from origin/main, the new handler file was removed, and ValidateGCPWIFEndpoints was made a no-op. On that tree, every new rejection test failed (35 failing api subtests, 76 failing credentials subtests). The acceptance and compatibility tests passed before and after: no credentials, gcloud-style configs, and the empty / malformed load errors. With the fix, all new tests pass.
  • Cases covered: look-alike hosts (sts.googleapis.com.example.net, my-sts.googleapis.com, trailing dot, other Google host), http:// and other schemes, user info (sts.googleapis.com@host, user:pass@), explicit ports (:443, :8443, empty), IP literals, case-variant and duplicate keys, a valid gcloud-generated WIF config (AWS aws1 source and file source), registrations with a mismatched provider/type, junk/array/null/string JSON, type-only or payload-only submissions, credentials omitted (still accepted), and approval of a pending registration whose stored credential fails (400, nothing created).
  • secrets-scan.sh on the changed files: "clean; planted key flagged". scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.

🤖 Generated with Claude Code

IAmJSD and others added 3 commits October 25, 2026 16:18
A stored external_account config names the endpoints the resolver talks
to when it exchanges and impersonates tokens. Only Google's STS and IAM
Credentials endpoints are ever expected there (gcloud and the gcp-target
IaC write exactly those, and the secret-free federated path hard-codes
them), so the resolver now checks them before it builds a token source:
token_url must be https://sts.googleapis.com/..., and
service_account_impersonation_url, when set, must be
https://iamcredentials.googleapis.com/..., with no user info or port.

The check decodes the JSON the same way golang.org/x/oauth2/google does,
so it sees the values that library would use. A config that fails it
makes the load return a clear error naming the field, without the stored
value, instead of a token source.

ValidateGCPWIFEndpoints is exported so the API can apply the same rule
when a config is submitted.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
validateGCPWIFPayload checked the shape of a gcp_workload_identity_config
but not its URLs. It now runs credentials.ValidateGCPWIFEndpoints on
token_url and service_account_impersonation_url, the same rule the
resolver applies before using a stored config, so a config that could
never be used is refused with a 400 when it is saved. The optional
impersonation URL must also be a non-empty string when present.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
POST /api/register accepts an optional credential_type and
credential_payload, which approval saves into account_credentials. They
were stored as sent, while POST /api/accounts/:id/credentials checks the
type and the payload shape. Registrations now get the same checks:

- credential_type and credential_payload come together or not at all;
- the type must be a known one and belong to the registration's
  provider;
- the payload must be a JSON object that passes
  validateCredentialPayload (including the GCP WIF endpoint check).

The stored payload is re-encoded from the checked object, so the saved
bytes are the ones that were checked. Registrations without credentials
are accepted as before.

Approval checks the stored credential again, against the provider of the
account being created, before the registration leaves "pending". A
credential that fails stops the approval with a 400 telling the admin to
reject the registration; nothing is created or saved. Previously a
decrypt failure or a type-less payload was skipped with a log line while
the account was still created and enabled.

Without a credential store the payload was already dropped at submit;
the type is now dropped with it so the record stays consistent.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added priority/p1 Next up; this sprint type/security Security finding labels Oct 25, 2026
@coderabbitai

coderabbitai Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Opened at head 7d1fb80. There are three commits: the endpoint check in the credentials resolver (238633a), the same check in the WIF payload validation (3f818a3), and the registration credential checks on submit and approval (7d1fb80). Local verification is in the PR body.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority/p1 Next up; this sprint type/security Security finding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant