Repository navigation
Conversation
…ed CredentialsFromJSON loadStoredGCPTokenSource parsed stored GCP JSON with the deprecated google.CredentialsFromJSON (SA1019, nolint'd), which accepts any Google credential type: authorized_user, impersonated_service_account, external_account_authorized_user, an OAuth client config with no type, and external_account in the service-account slot. Use google.CredentialsFromJSONWithType, already in golang.org/x/oauth2 v0.36.0 (no go.mod change), and pin each slot to the one type the API accepts into it: gcp_service_account -> service_account, gcp_workload_identity_config -> external_account. A slot with no pinned type fails closed before the store is read. Closes LeanerCloud#80 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
loadStoredGCPTokenSource(internal/credentials/resolver.go) turned stored GCP credential JSON into a token source with the deprecatedgoogle.CredentialsFromJSON. It was//nolint:staticcheck(SA1019) on the grounds that the replacement needed a GCP SDK upgrade and that the bytes are operator-controlled.Neither reason holds:
google.CredentialsFromJSONWithTypeis already in the pinnedgolang.org/x/oauth2 v0.36.0.authorized_user,impersonated_service_account,external_account_authorized_user,external_accountin the service-account slot, and an OAuth client config ({"installed": …}, which has notype). The client config became an interactive auth-code token source. The API'svalidateCredentialPayloadonly letsservice_accountintogcp_service_accountandexternal_accountintogcp_workload_identity_config, but the read path never checked that.Fix
gcpStoredJSONTypes, ties each stored slot to the one JSON type the API accepts for it:gcp_service_account→google.ServiceAccountgcp_workload_identity_config→google.ExternalAccount(the legacy stored-JSON WIF path needsexternal_account, so that slot has to keep accepting it)google.CredentialsFromJSONWithTypeparses with that pinned type, and the//nolintis gone. Any other type fails withparse gcp credentials …: google: expected credential type "…", found "…".go.mod/go.sumchange.Accepted types are only ever narrowed, never widened. Nothing is accepted that wasn't before. The only new rejections are types the API never let into those slots.
Closes #80
Merge-order and overlap notes
go.modalone, so it doesn't interact with feat(ri-exchange): pass offered instance types to reshape analysis #1246 / fix(onboarding): flag aws-cn accounts and refuse their Savings Plans purchases #1248 (they bump thecloud-commitments-gopins), with branchfix/issue-204(it addshcl/v2and indirect deps), or with each other. Any merge order works.fix/issue-313removescredentials.ResolveGCPCredentialsand editsresolver.go/resolver_extra_test.goelsewhere. This PR doesn't touch or useResolveGCPCredentials.git merge-treeagainst that branch is clean.fix/issue-559editshost_identity.go. This PR doesn't touch it, andgit merge-treeis clean.git merge-treeagainst the heads of feat(ri-exchange): pass offered instance types to reshape analysis #1246 and fix(onboarding): flag aws-cn accounts and refuse their Savings Plans purchases #1248 is also clean.When it takes effect
This is a code-only change to the GCP credential resolver used by the scheduler and the purchase manager. It needs no deploy to merge. It takes effect in a deployment once that deployment runs a build that includes it. GCP deploys are paused right now, so GCP-hosted instances keep the old behavior until the pause lifts and they are redeployed. AWS/Azure-hosted deployments that manage GCP accounts pick it up on their next deploy.
Verification
All evidence below is unit tests with mock-store fixtures. No GCP call was made: building a token source is lazy, and no test calls
Token().New tests are in
internal/credentials/resolver_gcp_type_test.go:TestResolveGCPTokenSource_ServiceAccountSlot_AcceptsServiceAccountJSON: service-account JSON is accepted (auth mode""andservice_account_key).TestResolveGCPTokenSource_ServiceAccountSlot_RejectsOtherTypes:external_account,authorized_user,impersonated_service_account,external_account_authorized_userand an untypedinstalledclient config are rejected.TestResolveGCPTokenSource_WIFSlot_AcceptsExternalAccountJSON:external_accountJSON is accepted on the legacy stored-WIF path.TestResolveGCPTokenSource_WIFSlot_RejectsOtherTypes:service_accountplus the four types above are rejected.TestResolveGCPTokenSource_StoredSlots_RejectMalformedJSON: both slots reject non-JSON, truncated JSON, a JSON array, a non-stringtypeand{}. This guards behavior that was already correct.TestLoadStoredGCPTokenSource_RejectsUnknownSlot: a slot with no pinned type fails closed.Fail-before, on
origin/main(a14dc8f2) with only the new test file added:Pass-after, at
63405f9a: the same command passes all 6 tests and all 20 subtests.Other checks run at
63405f9a, withAWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true:Not verified: a real GCP token exchange end to end. GCP calls are off-limits here, and GCP deploys are paused.
Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@b56afb2b= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-324andfix/issue-204both edit.github/workflows/ci.ymlnext to theazure-kv-access-policyjob. The tree kept #324's new "Run Function URL reader self-tests" step and #204's updated job comment; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran oned0c7174, which differs fromb56afb2bonly by one.gitallowedline on fix/issue-324:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....--build-tags integration.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout):internal/api,internal/purchase,internal/schedulerandinternal/serverall pass..gitallowedline. Re-run atb56afb2b: git-secrets, using the repo'sscripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope (follow-ups worth filing)
POST /api/register(submitRegistration→encryptRegistrationCredential) storescredential_typeandcredential_payloadwith novalidCredentialTypesorvalidateCredentialPayloadcheck. On admin approval,storeRegistrationCredentialswrites them straight intoaccount_credentials. So the "operator-controlled bytes" assumption in the old//nolintcomment only holds as far as an admin reviews the payload. This PR closes the type-confusion part on the read side (e.g. anauthorized_userorexternal_accountJSON in the service-account slot). The write side should still validate registrations the same wayPUT /api/accounts/:id/credentialsdoes.external_accountcontent isn't validated. The WIF slot still accepts anyexternal_accountconfig, which is what the API and the legacy path allow today. Google's guidance is to validate such configs from external sources:token_url,service_account_impersonation_url, andcredential_source(file/url), because a hostile config can make the server read a local file or call an arbitrary URL and send the result to an attacker-chosentoken_url.validateGCPWIFPayloadonly checks that keys are present. Restricting those URLs (e.g. tohttps://sts.googleapis.comandhttps://iamcredentials.googleapis.com) is a behavior and policy decision for the owner, so it isn't done here.Maintainer: please mirror the issue's triage labels onto this PR:
impact/internal,type/chore,triaged,priority/p3,severity/low,urgency/eventually,effort/m.🤖 Generated with Claude Code