CORS-4423: update GCP auth functions & validations#10694
Conversation
Change the deprecated WithCredentialsJSON func to WithAuthCredentialsJSON which takes a "type" argument. That parameter is used to indicate the intended credentials type to accept, which can be useful to limit choice if accepting credentials from 3rd party users. As the installer considers its users as first parties, we want to accept all credentials transparently for GCP. Any layered products utililzing the installer can limit provided credentials if needed.
Google's docs indicate that WIF credentials should be validated by the caller. See: https://docs.cloud.google.com/docs/authentication/client-libraries#external-credentials The validation requires that token_url equal https://sts.googleapis.com/v1/token In a non-default universe domain, the token_url would utilize that domain instead of googleapis.com. But universe domain is an arbitrary string that cannot be validated; therefore we cannot accept WIF credentials for the installer with custom universe domains.
Bring in the most up-to-date authentication packages.
go mod tidy && go mod vendor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@patrickdillon: This pull request references CORS-4423 which is a valid jira issue. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughChangesGCP credential handling
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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: 2
🤖 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 `@pkg/asset/installconfig/gcp/session_test.go`:
- Line 28: Update the fixture serialization calls in the affected session tests
to handle the error returned by json.Marshal instead of discarding it. Require
marshaling to succeed before using the serialized credentials, including the
occurrences at the referenced lines, using the test’s existing assertion or
failure mechanism.
In `@pkg/asset/installconfig/gcp/session.go`:
- Around line 302-309: Update validateCredSourceURL to allow HTTP only for
recognized trusted metadata/local credential endpoints, using a metadata-host
allowlist; continue requiring HTTPS for all other hosts and preserve the
existing URL parsing and error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| "url": credSourceURL, | ||
| } | ||
| } | ||
| b, _ := json.Marshal(creds) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not discard fixture serialization errors.
Require successful marshaling so future fixture changes cannot silently produce invalid credentials.
Proposed fix
- b, _ := json.Marshal(creds)
+ b, err := json.Marshal(creds)
+ assert.NoError(t, err)As per path instructions, “Never ignore error returns.”
Also applies to: 61-61, 74-74
🤖 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 `@pkg/asset/installconfig/gcp/session_test.go` at line 28, Update the fixture
serialization calls in the affected session tests to handle the error returned
by json.Marshal instead of discarding it. Require marshaling to succeed before
using the serialized credentials, including the occurrences at the referenced
lines, using the test’s existing assertion or failure mechanism.
Source: Path instructions
| func validateCredSourceURL(rawURL string) error { | ||
| parsed, err := url.Parse(rawURL) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid URL %q: %v", rawURL, err) | ||
| } | ||
| if parsed.Scheme != "https" { | ||
| return fmt.Errorf("%q must use HTTPS", rawURL) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby symbols
ast-grep outline pkg/asset/installconfig/gcp/session.go --view expanded || true
# Read the relevant section with line numbers
sed -n '250,360p' pkg/asset/installconfig/gcp/session.go | cat -n
# Find all uses of validateCredSourceURL and related URL validation helpers
rg -n "validateCredSourceURL|cred[- ]source|credential source|metadata|localhost|IMDS|WIF|url.Parse|Scheme != \"https\"" pkg/asset/installconfig -S
# Find provider docs/tests that mention supported credential-source URL schemes/hosts
rg -n "http://|https://|localhost|metadata server|IMDS|workload identity|credential source" -S .Repository: openshift/installer
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- session.go excerpt ---'
sed -n '280,340p' pkg/asset/installconfig/gcp/session.go | cat -n
echo '--- search for credential-source URL handling in GCP install config ---'
rg -n "validateCredSourceURL|credSource|credential source|metadata|localhost|http://|https://" pkg/asset/installconfig/gcp -S
echo '--- search for tests around this validator ---'
rg -n "validateCredSourceURL|credential source|metadata" pkg/asset/installconfig/gcp -g '*_test.go' -S
echo '--- file list in gcp installconfig ---'
git ls-files pkg/asset/installconfig/gcpRepository: openshift/installer
Length of output: 10659
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- session.go around the validator ---'
sed -n '220,320p' pkg/asset/installconfig/gcp/session.go | cat -n
echo '--- session_test.go relevant cases ---'
sed -n '1,140p' pkg/asset/installconfig/gcp/session_test.go | cat -n
echo '--- any docs describing external_account support in this package ---'
rg -n "Workload Identity Federation|external_account|credential_source.url|IMDS|localhost|Azure" pkg/asset/installconfig/gcp -SRepository: openshift/installer
Length of output: 10140
🌐 Web query:
Google external_account credential_source.url HTTP localhost IMDS supported AIP 4117
💡 Result:
The AIP-4117 standard defines the technical specification for External Account Credentials used in Google Cloud Workload Identity Federation [1][2]. The credential_source.url field is specifically designed to facilitate the retrieval of external credentials (such as OIDC tokens) from local metadata servers, such as the Azure Instance Metadata Service (IMDS) [1][2]. Key technical aspects include: 1. Purpose: When an application is running in an environment that provides credentials via a local metadata endpoint, the credential_source.url field instructs Google Cloud authentication libraries to perform an HTTP GET request to that URL to retrieve the subject token required for the Security Token Service (STS) exchange [1][3]. 2. Supported Operations: By design, URL-sourced credentials in this context are intended for simple retrieval using HTTP GET [4]. They are not generally intended for complex flows requiring POST requests or custom bearer tokens, as these are typically handled by "Executable Sourced Credentials" [4]. 3. Configuration Details: - url (Required): The URL of the local metadata server [1][2]. - headers (Optional): Allows appending custom headers to the HTTP GET request [1][2]. - format.type (Optional): Specifies the response format (text or json; defaults to text) [1][2]. - format.subject_token_field_name (Required if format.type is json): Identifies the specific JSON field containing the subject token [1][2]. 4. Practical Implementation: While standard AIP-4117 URL-sourced credentials are often used for platforms like Azure, developers working with AWS environments (such as EC2 or ECS) typically use specialized credential providers within the Google Cloud SDKs [5][6][7]. These providers natively handle the specific metadata endpoints (like IMDSv2) and token exchange processes required for those environments [8][6][7]. Because these URL-sourced credentials rely on a straightforward HTTP GET request to a local, trusted endpoint, they are primarily suited for environments where the metadata service is reachable at localhost (e.g., 169.254.169.254) and does not require complex authentication headers [1][7][4].
Citations:
- 1: https://google.aip.dev/auth/4117
- 2: https://github.com/aip-dev/google.aip.dev/blob/master/aip/auth/4117.md
- 3: https://docs.cloud.google.com/iam/docs/workload-identity-federation-with-other-providers
- 4: Support POST verb for URL-sourced credentials to enable Azure Pipelines WIF googleapis/google-auth-library-python#1508
- 5: auth: support AWS ECS default credentials detection googleapis/google-cloud-go#13479
- 6: https://docs.rs/gcloud-sdk/latest/src/gcloud_sdk/token_source/ext_creds_source/mod.rs.html
- 7: Workload Identity Federation with AWS Fargate (ECS) googleapis/google-api-dotnet-client#2963
- 8: https://apache.googlesource.com/opendal-reqsign/+/3b749afa8ed2ee249e126c41fb6432f48dbcbb3c%5E%21/
Allow HTTP for recognized credential_source.url endpoints. validateCredSourceURL rejects all non-HTTPS sources, but AIP-4117 allows URL-sourced external_account creds to read from trusted local/metadata endpoints (for example Azure IMDS or localhost). This blocks valid WIF configs before auth starts; use a metadata-host allowlist instead of forcing HTTPS 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 `@pkg/asset/installconfig/gcp/session.go` around lines 302 - 309, Update
validateCredSourceURL to allow HTTP only for recognized trusted metadata/local
credential endpoints, using a metadata-host allowlist; continue requiring HTTPS
for all other hosts and preserve the existing URL parsing and error handling.
|
/cc |
|
@patrickdillon: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| var f struct { | ||
| Type string `json:"type"` | ||
| } | ||
| if err := json.Unmarshal(ssn.Credentials.JSON, &f); err != nil { | ||
| return nil, fmt.Errorf("failed to parse credentials JSON: %w", err) | ||
| } |
There was a problem hiding this comment.
nit: This unmarshalling block also appears in other places. Maybe, we can define it in a common helper?
| var t struct { | ||
| Type string `json:"type"` | ||
| } | ||
| if err := json.Unmarshal(credsJSON, &t); err != nil { | ||
| return nil | ||
| } | ||
| if t.Type != "external_account" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I guess we can skip this block since we parse the creds JSON in the next lines? The struct just needs type defining. Any missing fields will be zero values 👀
var creds struct {
Type string `json:"type"`
TokenURL string `json:"token_url"`
ServiceAccountImpersonationURL string `json:"service_account_impersonation_url"`
UniverseDomain string `json:"universe_domain"`
CredentialSource struct {
URL string `json:"url"`
} `json:"credential_source"`
}
if err := json.Unmarshal(credsJSON, &creds); err != nil {
return fmt.Errorf("failed to parse credentials JSON: %w", err)
}
if creds.Type != string(googleoauth.ExternalAccount) {
return nil
}| if err := json.Unmarshal(credsJSON, &t); err != nil { | ||
| return nil | ||
| } | ||
| if t.Type != "external_account" { |
There was a problem hiding this comment.
We should validate the case "external_account_authorized_user", which also uses token_url. Though, this method applies to interactive browser sign-in flow, but we can be thorough? WDYT?
|
I find https://google.aip.dev/auth/4117#expected-behavior quite helpful in defining the expected values for creds fields. I think we don't need to apply all the rules in installer, but maybe managed services can do that. |
| return nil | ||
| } | ||
|
|
||
| func validateCredSourceURL(rawURL string) error { |
There was a problem hiding this comment.
It looks like code rabbit identified some of this already. There is a possibility of whitelisting some known (and allowed) metadata servers if we intend to support IMDS. Right now I don't know that this is supported, but we could put in a note for future requirements when OIDC-Based WIF.
| Type string `json:"type"` | ||
| } | ||
| if err := json.Unmarshal(credsJSON, &t); err != nil { | ||
| return nil |
There was a problem hiding this comment.
Should we atleast warn when the credtial failed to be loaded from JSON here? There is no indication of any failure.
openshift-ci
Bot
added
the
needs-rebase
|
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. |
4 participants
Updates from
WithCredentialsJSON->WithAuthCredentialsJSON&FromCredentialsJSON->FromCredentialsJSONWithTypeto avoid using the deprecated function. This change is a no-op, only intended to avoid deprecation.These functions take a credential type parameter, which can be used to limit accepted credentials types, which is useful for validating credentials provided by external third parties. For OpenShift, cluster credentials are coming from first party users running clusters in their project, so we do not need to limit which credentials are accepted; therefore we just pass the type through from the credential to the function.
Also, adds credential validation for WIF authentication as recommended by Google's docs.
Summary by CodeRabbit
Security
Bug Fixes
Chores
Tests