-
Notifications
You must be signed in to change notification settings - Fork 1.5k
CORS-4423: update GCP auth functions & validations #10694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
adaa500
552d5e3
23d6cb0
9fc89e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,9 @@ package gcp | |
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "net/url" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
@@ -44,6 +46,12 @@ func GetSession(ctx context.Context) (*Session, error) { | |
| return nil, errors.Wrap(err, "failed to load credentials") | ||
| } | ||
|
|
||
| if creds.JSON != nil { | ||
| if err := validateCredentialURLs(creds.JSON); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| return &Session{ | ||
| Credentials: creds, | ||
| Path: path, | ||
|
|
@@ -182,7 +190,14 @@ type contentLoader struct { | |
| } | ||
|
|
||
| func (f *contentLoader) Load(ctx context.Context) (*googleoauth.Credentials, error) { | ||
| return googleoauth.CredentialsFromJSON(ctx, []byte(f.content), compute.CloudPlatformScope) | ||
| jsonData := []byte(f.content) | ||
| var t struct { | ||
| Type string `json:"type"` | ||
| } | ||
| if err := json.Unmarshal(jsonData, &t); err != nil { | ||
| return nil, fmt.Errorf("failed to parse credentials JSON: %w", err) | ||
| } | ||
| return googleoauth.CredentialsFromJSONWithType(ctx, jsonData, googleoauth.CredentialsType(t.Type), compute.CloudPlatformScope) | ||
| } | ||
|
|
||
| func (f *contentLoader) String() string { | ||
|
|
@@ -230,3 +245,67 @@ func (u *userLoader) Load(ctx context.Context) (*googleoauth.Credentials, error) | |
| func (u *userLoader) Content() string { | ||
| return defaultAuthFilePath | ||
| } | ||
|
|
||
| // validateCredentialURLs validates external_account (Workload Identity Federation) | ||
| // credential URLs per Google's guidance that callers must validate these fields: | ||
| // https://google.aip.dev/auth/4117 | ||
| // https://cloud.google.com/docs/authentication/client-libraries#external-credentials | ||
| // | ||
| // WIF with custom universe domains is not supported in the installer — all | ||
| // Google endpoint URLs are restricted to googleapis.com. | ||
| func validateCredentialURLs(credsJSON []byte) error { | ||
| var t struct { | ||
| Type string `json:"type"` | ||
| } | ||
| if err := json.Unmarshal(credsJSON, &t); err != nil { | ||
| return nil | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we atleast warn when the credtial failed to be loaded from JSON here? There is no indication of any failure. |
||
| } | ||
| if t.Type != "external_account" { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should validate the case |
||
| return nil | ||
| } | ||
|
Comment on lines
+257
to
+265
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we can skip this block since we parse the creds JSON in the next lines? The struct just needs 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
} |
||
|
|
||
| var creds struct { | ||
| 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 external account credentials: %v", err) | ||
| } | ||
|
|
||
| if creds.UniverseDomain != "" && creds.UniverseDomain != "googleapis.com" { | ||
| return fmt.Errorf("Workload Identity Federation (external_account) with custom universe domain %q is not supported. "+ | ||
| "If you need this capability, please open an RFE with Red Hat or a GitHub issue at https://github.com/openshift/installer/issues", creds.UniverseDomain) | ||
| } | ||
|
|
||
| const expectedTokenURL = "https://sts.googleapis.com/v1/token" | ||
| const iamPrefix = "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/" | ||
| const wifNote = "Workload Identity Federation (external_account) credentials are only supported with googleapis.com endpoints" | ||
|
|
||
| if creds.TokenURL != "" && creds.TokenURL != expectedTokenURL { | ||
| return fmt.Errorf("token_url %q must equal %s. %s", creds.TokenURL, expectedTokenURL, wifNote) | ||
| } | ||
| if creds.ServiceAccountImpersonationURL != "" && !strings.HasPrefix(creds.ServiceAccountImpersonationURL, iamPrefix) { | ||
| return fmt.Errorf("service_account_impersonation_url %q must begin with %s. %s", creds.ServiceAccountImpersonationURL, iamPrefix, wifNote) | ||
| } | ||
| if creds.CredentialSource.URL != "" { | ||
| if err := validateCredSourceURL(creds.CredentialSource.URL); err != nil { | ||
| return fmt.Errorf("credential_source.url: %v", err) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func validateCredSourceURL(rawURL string) error { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| 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) | ||
| } | ||
|
Comment on lines
+302
to
+309
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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:
💡 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:
Allow HTTP for recognized credential_source.url endpoints. 🤖 Prompt for AI Agents |
||
| return nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| package gcp | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestValidateCredentialURLs(t *testing.T) { | ||
| buildCreds := func(tokenURL, saImpURL, credSourceURL string) []byte { | ||
| creds := map[string]interface{}{ | ||
| "type": "external_account", | ||
| "audience": "//iam.googleapis.com/projects/123/locations/global/workloadIdentityPools/pool/providers/provider", | ||
| "subject_token_type": "urn:ietf:params:oauth:token-type:jwt", | ||
| } | ||
| if tokenURL != "" { | ||
| creds["token_url"] = tokenURL | ||
| } | ||
| if saImpURL != "" { | ||
| creds["service_account_impersonation_url"] = saImpURL | ||
| } | ||
| if credSourceURL != "" { | ||
| creds["credential_source"] = map[string]interface{}{ | ||
| "url": credSourceURL, | ||
| } | ||
| } | ||
| b, _ := json.Marshal(creds) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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 AgentsSource: Path instructions |
||
| return b | ||
| } | ||
|
|
||
| cases := []struct { | ||
| name string | ||
| creds []byte | ||
| err string | ||
| }{ | ||
| { | ||
| name: "valid standard WIF credentials", | ||
| creds: buildCreds("https://sts.googleapis.com/v1/token", "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/sa@proj.iam.gserviceaccount.com:generateAccessToken", "https://my-oidc-provider.example.com/token"), | ||
| }, | ||
| { | ||
| name: "all URL fields empty uses safe defaults", | ||
| creds: buildCreds("", "", ""), | ||
| }, | ||
| { | ||
| name: "valid credential_source.url to external OIDC provider", | ||
| creds: buildCreds("https://sts.googleapis.com/v1/token", "", "https://login.microsoftonline.com/tenant/oauth2/v2.0/token"), | ||
| }, | ||
| { | ||
| name: "non-external_account type is not validated", | ||
| creds: []byte(`{"type": "service_account", "token_url": "https://evil.com/steal"}`), | ||
| }, | ||
| { | ||
| name: "custom universe domain with WIF is not supported", | ||
| creds: func() []byte { | ||
| c := map[string]interface{}{ | ||
| "type": "external_account", | ||
| "universe_domain": "custom.example.com", | ||
| "token_url": "https://sts.custom.example.com/v1/token", | ||
| } | ||
| b, _ := json.Marshal(c) | ||
| return b | ||
| }(), | ||
| err: `custom universe domain.*not supported.*open an RFE with Red Hat or a GitHub issue`, | ||
| }, | ||
| { | ||
| name: "googleapis.com universe domain with WIF is allowed", | ||
| creds: func() []byte { | ||
| c := map[string]interface{}{ | ||
| "type": "external_account", | ||
| "universe_domain": "googleapis.com", | ||
| "token_url": "https://sts.googleapis.com/v1/token", | ||
| } | ||
| b, _ := json.Marshal(c) | ||
| return b | ||
| }(), | ||
| }, | ||
| { | ||
| name: "invalid token_url pointing to unrelated host", | ||
| creds: buildCreds("https://bad-host.example.com/v1/token", "", ""), | ||
| err: `token_url.*must equal https://sts\.googleapis\.com/v1/token`, | ||
| }, | ||
| { | ||
| name: "invalid token_url with extra path", | ||
| creds: buildCreds("https://sts.googleapis.com/v1/token/extra", "", ""), | ||
| err: `token_url.*must equal https://sts\.googleapis\.com/v1/token`, | ||
| }, | ||
| { | ||
| name: "invalid token_url with custom universe domain", | ||
| creds: buildCreds("https://sts.custom-domain.example.com/v1/token", "", ""), | ||
| err: `token_url.*must equal https://sts\.googleapis\.com/v1/token`, | ||
| }, | ||
| { | ||
| name: "invalid service_account_impersonation_url pointing to unrelated host", | ||
| creds: buildCreds("https://sts.googleapis.com/v1/token", "https://bad-host.example.com/capture", ""), | ||
| err: `service_account_impersonation_url.*must begin with https://iamcredentials\.googleapis\.com/`, | ||
| }, | ||
| { | ||
| name: "invalid credential_source.url using HTTP", | ||
| creds: buildCreds("https://sts.googleapis.com/v1/token", "", "http://my-oidc-provider.example.com/token"), | ||
| err: `credential_source\.url.*must use HTTPS`, | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range cases { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| err := validateCredentialURLs(test.creds) | ||
| if test.err == "" { | ||
| assert.NoError(t, err) | ||
| } else { | ||
| assert.Error(t, err) | ||
| assert.Regexp(t, test.err, err.Error()) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This unmarshalling block also appears in other places. Maybe, we can define it in a common helper?