Repository navigation
fix(auth): auto-detect GitHub Actions WIF with proper audience, host validation, and lazy GSM init - #2109
Merged
Andriy Knysh (aknysh) merged 14 commits intoFeb 27, 2026
Conversation
…mic pair Defer Google Secret Manager client initialization to first use via ensureClient() instead of eagerly creating it in the constructor. This fixes a chicken-and-egg problem where config loading creates stores before auth credentials (e.g., GOOGLE_OAUTH_ACCESS_TOKEN from WIF) are established, causing "failed to create client: could not find default credentials" errors in CI. Also treat ADC client_id/client_secret as an atomic pair: a custom client_id without a matching secret falls back to the full default pair rather than producing a mismatched combination that fails OAuth refresh. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ADC client-credentials resolution with env/ADC-file/default fallbacks; switches GSM store to lazy client initialization, adds secret helper methods and Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/auth/cloud/gcp/setup_test.go (1)
157-158: Drop manualos.Unsetenvin this test and keep env state fully test-scoped.
resolveADCClientCredentials()usesGetenv, sot.Setenv("ATMOS_GCP_ADC_CLIENT_SECRET", "")is sufficient here; the explicit unset is redundant.Proposed cleanup
t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(tmp, "missing.json")) t.Setenv("ATMOS_GCP_ADC_CLIENT_SECRET", "") - os.Unsetenv("ATMOS_GCP_ADC_CLIENT_SECRET")Based on learnings: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/gcp/setup_test.go` around lines 157 - 158, The test currently calls t.Setenv("ATMOS_GCP_ADC_CLIENT_SECRET", "") and then immediately calls os.Unsetenv("ATMOS_GCP_ADC_CLIENT_SECRET"); remove the manual os.Unsetenv call so the test relies solely on testing.T.Setenv for scope-managed environment changes; update the test in pkg/auth/cloud/gcp/setup_test.go where resolveADCClientCredentials() is exercised to only use t.Setenv(...) and not os.Unsetenv(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/auth/cloud/gcp/setup_test.go`:
- Around line 157-158: The test currently calls
t.Setenv("ATMOS_GCP_ADC_CLIENT_SECRET", "") and then immediately calls
os.Unsetenv("ATMOS_GCP_ADC_CLIENT_SECRET"); remove the manual os.Unsetenv call
so the test relies solely on testing.T.Setenv for scope-managed environment
changes; update the test in pkg/auth/cloud/gcp/setup_test.go where
resolveADCClientCredentials() is exercised to only use t.Setenv(...) and not
os.Unsetenv(...).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
pkg/auth/cloud/gcp/setup.gopkg/auth/cloud/gcp/setup_test.gopkg/store/google_secret_manager_store.gopkg/store/google_secret_manager_store_test.gopkg/store/identity_test.go
When token_source is not configured in the WIF provider spec, auto-detect GitHub Actions by checking ACTIONS_ID_TOKEN_REQUEST_URL and default to URL-based OIDC token fetch. This eliminates the need for explicit token_source config in CI. Also adds docstrings to all functions in changed files to satisfy the 80% docstring coverage threshold. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pkg/auth/cloud/gcp/setup.go (1)
176-180:⚠️ Potential issue | 🟡 MinorSupport Windows gcloud default credentials path.
On Windows, gcloud stores ADC at
%APPDATA%\gcloud\application_default_credentials.json(e.g.,C:\Users\<USERNAME>\AppData\Roaming\gcloud\...), but the current code only checks the Unix path. This will fail to find credentials on Windows systems.The fix should check the
APPDATAenvironment variable before falling back to the Unix home directory:Proposed Windows compatibility fix
func adcCredentialsPath() string { if path := strings.TrimSpace(os.Getenv("GOOGLE_APPLICATION_CREDENTIALS")); path != "" { return path } if configDir := strings.TrimSpace(os.Getenv("CLOUDSDK_CONFIG")); configDir != "" { return filepath.Join(configDir, "application_default_credentials.json") } + // Windows: %APPDATA%\gcloud; Unix: ~/.config/gcloud + if appData := os.Getenv("APPDATA"); appData != "" { + return filepath.Join(appData, "gcloud", "application_default_credentials.json") + } home, err := os.UserHomeDir() if err != nil { return "" } return filepath.Join(home, ".config", "gcloud", "application_default_credentials.json") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/gcp/setup.go` around lines 176 - 180, The code that builds the gcloud ADC path only uses os.UserHomeDir (Unix-style ~/.config/gcloud/...), which misses Windows' %APPDATA%\gcloud\application_default_credentials.json; update the function that currently calls os.UserHomeDir to first check os.Getenv("APPDATA") and if non-empty return filepath.Join(appData, "gcloud", "application_default_credentials.json"), otherwise fall back to using os.UserHomeDir and filepath.Join(home, ".config", "gcloud", "application_default_credentials.json"); keep the existing failure behavior of returning "" when neither APPDATA nor UserHomeDir is available and reference the os.Getenv("APPDATA") check and the filepath.Join calls to locate where to change the logic.pkg/store/google_secret_manager_store.go (2)
413-418:⚠️ Potential issue | 🟠 MajorHandle
PermissionDeniedexplicitly inGetKey.Right now
PermissionDeniedfalls intoErrAccessSecret, unlikeGet, which loses error specificity and consistency for callers using typed error handling.Suggested fix
if err != nil { - if status.Code(err) == codes.NotFound { - return nil, fmt.Errorf(errWrapFormatWithID, ErrResourceNotFound, secretName, err) - } - return nil, fmt.Errorf(errWrapFormat, ErrAccessSecret, err) + if st, ok := status.FromError(err); ok { + switch st.Code() { + case codes.NotFound: + return nil, fmt.Errorf(errWrapFormatWithID, ErrResourceNotFound, secretName, err) + case codes.PermissionDenied: + return nil, fmt.Errorf(errWrapFormatWithID, ErrPermissionDenied, fmt.Sprintf("secret %s", secretName), err) + } + } + return nil, fmt.Errorf(errWrapFormat, ErrAccessSecret, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/store/google_secret_manager_store.go` around lines 413 - 418, In GetKey's error handling (inside function GetKey), explicitly check for status.Code(err) == codes.PermissionDenied and return a wrapped error using the ErrPermissionDenied sentinel (use the same errWrapFormatWithID pattern with secretName and err) instead of letting it fall through to ErrAccessSecret; keep the existing NotFound handling and fallback to ErrAccessSecret for other codes.
386-431: 🛠️ Refactor suggestion | 🟠 MajorAdd the required perf tracker in public
GetKey.
GetKeyis a non-trivial public method and should include the standarddefer perf.Track(...)()hook at function entry (with the required blank line after it).As per coding guidelines "Add
defer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions, usenilif no atmosConfig param."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/store/google_secret_manager_store.go` around lines 386 - 431, Add the standard perf tracker at the top of the public GetKey method: insert a defer perf.Track(nil, "pkg.GetKey")() immediately after the function entry (as the first executable statement) and include a blank line after that defer; also add the perf import if missing so GSMStore.GetKey compiles. Ensure the tracker uses nil for atmosConfig per guidelines and references the GetKey method name.pkg/store/google_secret_manager_store_test.go (1)
24-60:⚠️ Potential issue | 🟠 MajorSwitch to
mockgenforGSMClientmock generation.Add a
//go:generatedirective togoogle_secret_manager_store.goabove theGSMClientinterface and generate the mock, just like the other interfaces in thepkg/storepackage. This keeps the mock in sync with interface changes and aligns with the codebase pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/store/google_secret_manager_store_test.go` around lines 24 - 60, Add a //go:generate directive above the GSMClient interface in google_secret_manager_store.go and remove the hand-written MockGSMClient type in google_secret_manager_store_test.go; then run the same mock generation command used elsewhere in pkg/store to produce an up-to-date mock for GSMClient (so generated mock matches the GSMClient interface and follows the repo pattern), ensuring references to GSMClient, CreateSecret, AddSecretVersion, AccessSecretVersion, and Close are present in the generated mock.pkg/auth/providers/gcp_wif/provider_test.go (1)
410-439:⚠️ Potential issue | 🟠 MajorMock the HTTP request in
TestGetOIDCToken_NilTokenSource_AutoDetectsGitHubActionsto eliminate external dependency and strengthen assertions.The test currently attempts a real HTTP request to
token.actions.githubusercontent.comand only asserts that an error occurs without verifying the intended behavior—it checks that a specific substring is absent rather than confirming the code path works correctly. This can pass on unrelated failures and is non-deterministic.Use a custom
RoundTripperto intercept the request, verify the Authorization header, and return a deterministic response. Then assert the actual token value is extracted correctly. Consider applying the same pattern to both nil-token_sourcetest cases as a table-driven structure to cover multiple scenarios (no GitHub Actions env, GitHub Actions with success, GitHub Actions with fetch failure).The suggested refactor pattern aligns with how other tests in this file (e.g.,
TestGetTokenFromURL) usehttptestto mock services deterministically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/providers/gcp_wif/provider_test.go` around lines 410 - 439, Modify TestGetOIDCToken_NilTokenSource_AutoDetectsGitHubActions to mock the HTTP call instead of hitting the external URL: create a custom RoundTripper or httptest server and assign it to p.httpClient, intercept requests to verify the Authorization header contains the ACTIONS_ID_TOKEN_REQUEST_TOKEN value, return a deterministic JSON token response, then call p.getOIDCToken(ctx) and assert the returned token matches the mocked response (and still assert it does not return errUtils.ErrInvalidProviderConfig). Use the same mocking pattern for the other nil token_source case if helpful to convert tests into a small table-driven set covering no-GHA, GHA-success, and GHA-failure scenarios so behavior is deterministic.
🧹 Nitpick comments (3)
pkg/auth/providers/gcp_wif/provider.go (2)
305-308: Consider limiting error body read size.When the token request fails, the entire response body is read without a size limit. A malicious or misconfigured server could return a very large body, consuming memory.
Suggested fix
if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) + body, _ := io.ReadAll(io.LimitReader(resp.Body, 4096)) return "", fmt.Errorf("%w: token request failed: %s: %s", errUtils.ErrAuthenticationFailed, resp.Status, string(body)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/providers/gcp_wif/provider.go` around lines 305 - 308, The error handling in the token request checks resp.StatusCode but reads the entire resp.Body via io.ReadAll which can OOM on large responses; update the failure branch in the token fetch logic to read only a bounded amount (e.g., wrap resp.Body with io.LimitReader and read up to a safe max like 4KB) and handle read errors, then include that truncated body in the fmt.Errorf return (still using errUtils.ErrAuthenticationFailed and resp.Status) so you preserve context while preventing unbounded memory usage.
376-379: Same unbounded body read pattern here.Similar to the token URL request, the STS error response body is read without a size limit.
Suggested fix
if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) + body, _ := io.ReadAll(io.LimitReader(resp.Body, 4096)) return nil, fmt.Errorf("%w: STS error: %s: %s", errUtils.ErrAuthenticationFailed, resp.Status, string(body)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/providers/gcp_wif/provider.go` around lines 376 - 379, The STS error handling reads resp.Body without a size limit; change the read to use a bounded reader (e.g. io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize))) and introduce a sensible constant like maxErrorBodySize (e.g. 4–16 KB) to avoid unbounded memory use; update the error message to include the possibly truncated body and ensure resp.Body is closed as before; apply this change around the STS response check that constructs the fmt.Errorf with errUtils.ErrAuthenticationFailed and resp.Status.pkg/auth/cloud/gcp/setup_test.go (1)
158-159: Redundant unset after setting to empty.
t.Setenvalready sets the variable and handles cleanup. The subsequentos.Unsetenvis redundant and may conflict witht.Setenv's cleanup logic.Simplify by removing the redundant unset
- t.Setenv("ATMOS_GCP_ADC_CLIENT_SECRET", "") - os.Unsetenv("ATMOS_GCP_ADC_CLIENT_SECRET") + os.Unsetenv("ATMOS_GCP_ADC_CLIENT_SECRET")Or if you want to ensure it's truly unset (not empty string), just use
os.Unsetenvalone sincet.Setenvwill capture the original value for restoration anyway.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/gcp/setup_test.go` around lines 158 - 159, Remove the redundant os.Unsetenv call after using t.Setenv for the ATMOS_GCP_ADC_CLIENT_SECRET variable: keep the t.Setenv("ATMOS_GCP_ADC_CLIENT_SECRET", "") line (which records original value and restores it after the test) and delete the subsequent os.Unsetenv("ATMOS_GCP_ADC_CLIENT_SECRET") to avoid conflicting cleanup logic; alternatively, if you prefer not to use t.Setenv, replace the t.Setenv call with a single os.Unsetenv("ATMOS_GCP_ADC_CLIENT_SECRET") instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/store/google_secret_manager_store_test.go`:
- Around line 89-90: The helper gsmClientSecretCreationMock currently hardcodes
"test-project" for the parent string; update it to use the function parameter
projectID (e.g., build parent with fmt.Sprintf("projects/%s", projectID)) so the
mock respects the passed projectID; verify any other occurrences inside
gsmClientSecretCreationMock that reference the project ID are replaced to use
the projectID parameter and keep the rest of the mock (MockGSMClient
expectations) unchanged.
---
Outside diff comments:
In `@pkg/auth/cloud/gcp/setup.go`:
- Around line 176-180: The code that builds the gcloud ADC path only uses
os.UserHomeDir (Unix-style ~/.config/gcloud/...), which misses Windows'
%APPDATA%\gcloud\application_default_credentials.json; update the function that
currently calls os.UserHomeDir to first check os.Getenv("APPDATA") and if
non-empty return filepath.Join(appData, "gcloud",
"application_default_credentials.json"), otherwise fall back to using
os.UserHomeDir and filepath.Join(home, ".config", "gcloud",
"application_default_credentials.json"); keep the existing failure behavior of
returning "" when neither APPDATA nor UserHomeDir is available and reference the
os.Getenv("APPDATA") check and the filepath.Join calls to locate where to change
the logic.
In `@pkg/auth/providers/gcp_wif/provider_test.go`:
- Around line 410-439: Modify
TestGetOIDCToken_NilTokenSource_AutoDetectsGitHubActions to mock the HTTP call
instead of hitting the external URL: create a custom RoundTripper or httptest
server and assign it to p.httpClient, intercept requests to verify the
Authorization header contains the ACTIONS_ID_TOKEN_REQUEST_TOKEN value, return a
deterministic JSON token response, then call p.getOIDCToken(ctx) and assert the
returned token matches the mocked response (and still assert it does not return
errUtils.ErrInvalidProviderConfig). Use the same mocking pattern for the other
nil token_source case if helpful to convert tests into a small table-driven set
covering no-GHA, GHA-success, and GHA-failure scenarios so behavior is
deterministic.
In `@pkg/store/google_secret_manager_store_test.go`:
- Around line 24-60: Add a //go:generate directive above the GSMClient interface
in google_secret_manager_store.go and remove the hand-written MockGSMClient type
in google_secret_manager_store_test.go; then run the same mock generation
command used elsewhere in pkg/store to produce an up-to-date mock for GSMClient
(so generated mock matches the GSMClient interface and follows the repo
pattern), ensuring references to GSMClient, CreateSecret, AddSecretVersion,
AccessSecretVersion, and Close are present in the generated mock.
In `@pkg/store/google_secret_manager_store.go`:
- Around line 413-418: In GetKey's error handling (inside function GetKey),
explicitly check for status.Code(err) == codes.PermissionDenied and return a
wrapped error using the ErrPermissionDenied sentinel (use the same
errWrapFormatWithID pattern with secretName and err) instead of letting it fall
through to ErrAccessSecret; keep the existing NotFound handling and fallback to
ErrAccessSecret for other codes.
- Around line 386-431: Add the standard perf tracker at the top of the public
GetKey method: insert a defer perf.Track(nil, "pkg.GetKey")() immediately after
the function entry (as the first executable statement) and include a blank line
after that defer; also add the perf import if missing so GSMStore.GetKey
compiles. Ensure the tracker uses nil for atmosConfig per guidelines and
references the GetKey method name.
---
Nitpick comments:
In `@pkg/auth/cloud/gcp/setup_test.go`:
- Around line 158-159: Remove the redundant os.Unsetenv call after using
t.Setenv for the ATMOS_GCP_ADC_CLIENT_SECRET variable: keep the
t.Setenv("ATMOS_GCP_ADC_CLIENT_SECRET", "") line (which records original value
and restores it after the test) and delete the subsequent
os.Unsetenv("ATMOS_GCP_ADC_CLIENT_SECRET") to avoid conflicting cleanup logic;
alternatively, if you prefer not to use t.Setenv, replace the t.Setenv call with
a single os.Unsetenv("ATMOS_GCP_ADC_CLIENT_SECRET") instead.
In `@pkg/auth/providers/gcp_wif/provider.go`:
- Around line 305-308: The error handling in the token request checks
resp.StatusCode but reads the entire resp.Body via io.ReadAll which can OOM on
large responses; update the failure branch in the token fetch logic to read only
a bounded amount (e.g., wrap resp.Body with io.LimitReader and read up to a safe
max like 4KB) and handle read errors, then include that truncated body in the
fmt.Errorf return (still using errUtils.ErrAuthenticationFailed and resp.Status)
so you preserve context while preventing unbounded memory usage.
- Around line 376-379: The STS error handling reads resp.Body without a size
limit; change the read to use a bounded reader (e.g.
io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize))) and introduce a
sensible constant like maxErrorBodySize (e.g. 4–16 KB) to avoid unbounded memory
use; update the error message to include the possibly truncated body and ensure
resp.Body is closed as before; apply this change around the STS response check
that constructs the fmt.Errorf with errUtils.ErrAuthenticationFailed and
resp.Status.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
pkg/auth/cloud/gcp/setup.gopkg/auth/cloud/gcp/setup_test.gopkg/auth/providers/gcp_wif/provider.gopkg/auth/providers/gcp_wif/provider_test.gopkg/store/google_secret_manager_store.gopkg/store/google_secret_manager_store_test.go
GitHub Actions uses dynamic subdomains for ACTIONS_ID_TOKEN_REQUEST_URL (e.g., run-actions-1-azure-eastus.actions.githubusercontent.com) which don't match the static allowlist. When the URL comes from the env var, skip host validation entirely — GitHub Actions controls that URL. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/auth/providers/gcp_wif/provider_test.go`:
- Around line 426-438: The test uses a real GitHub URL causing flakiness; make
it deterministic by replacing the outbound call with an httptest server and
injecting its URL via the ACTIONS_ID_TOKEN_REQUEST_URL env var (keep
ACTIONS_ID_TOKEN_REQUEST_TOKEN for the bearer token), then call
Provider.getOIDCToken with the Provider instance (use the existing http.Client)
and assert the exact expected outcome: either a successful token string when the
test server returns a valid token payload or a specific authentication error
type/message when the server returns an auth failure; ensure the test server
validates the incoming Authorization header and body to exercise the same code
paths as getOIDCToken.
In `@pkg/auth/providers/gcp_wif/provider.go`:
- Around line 269-274: The current check in provider.go skips host validation
when fromEnv is true, allowing any HTTPS URL from ACTIONS_ID_TOKEN_REQUEST_URL;
change the logic in the token URL validation (the block referencing fromEnv,
AllowedHosts, and hostAllowed(u, allowedHosts)) so that host validation is not
bypassed: if token_source.allowed_hosts is set, use hostAllowed(u, allowedHosts)
as now; if allowed_hosts is empty and the URL came from the environment, only
accept HTTPS hosts that match the known GitHub Actions domains (e.g.,
actions.githubusercontent.com and its
run-actions-*.actions.githubusercontent.com subdomains) and reject all others;
when rejecting, return the same errUtils.ErrInvalidProviderConfig error with a
clear message. Ensure the scheme check (https) remains enforced and keep
references to fromEnv, AllowedHosts, hostAllowed, and
errUtils.ErrInvalidProviderConfig so reviewers can locate the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/auth/providers/gcp_wif/provider.gopkg/auth/providers/gcp_wif/provider_test.go
…ctions When fetching OIDC token from GitHub Actions without an explicit audience, auto-construct the WIF provider resource name as the audience parameter. Without this, GitHub defaults the aud claim to the repo owner URL (e.g., https://github.com/org) which GCP STS rejects as audience mismatch. Also refactors exchangeToken to reuse the new wifAudience() helper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/auth/providers/gcp_wif/provider.go (2)
428-440: Consider returning an error instead of empty string.If
wifAudience()returns empty due to missing fields,exchangeTokenwill send an empty audience to STS, yielding a confusing GCP error. WhileValidate()guards against this inAuthenticate(), a defensive check here could help catch misuse in other call paths.🛡️ Optional defensive approach
-func (p *Provider) wifAudience() string { +func (p *Provider) wifAudience() (string, error) { pool := p.poolID() provider := p.providerID() if p.spec.ProjectNumber == "" || pool == "" || provider == "" { - return "" + return "", fmt.Errorf("%w: cannot construct WIF audience: missing project_number, pool, or provider", errUtils.ErrInvalidProviderConfig) } return fmt.Sprintf( "//iam.googleapis.com/projects/%s/locations/global/workloadIdentityPools/%s/providers/%s", p.spec.ProjectNumber, pool, provider, - ) + ), nil }Then update callers to handle the error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/providers/gcp_wif/provider.go` around lines 428 - 440, wifAudience currently returns an empty string when required fields are missing which lets callers like exchangeToken send an invalid audience to STS; change wifAudience() to return (string, error) and return a descriptive error if p.spec.ProjectNumber, pool or provider are empty, then update callers (notably exchangeToken and any other usages) to handle and propagate that error (or return it up through Authenticate/Validate) so no empty audience is ever sent; keep Validate() as-is but treat wifAudience as a defensive check used by wifAudience(), exchangeToken(), and Authenticate().
331-347: Simplify case handling in hostAllowed.Both
allowedandhostare already lowercased before the comparison. Usingstrings.EqualFoldafterwards is redundant forhostand only needed foru.Host. You could streamline this.♻️ Suggested simplification
func hostAllowed(u *url.URL, allowedHosts []string) bool { host := strings.ToLower(u.Hostname()) + hostWithPort := strings.ToLower(u.Host) if host == "" { return false } for _, allowed := range allowedHosts { allowed = strings.ToLower(strings.TrimSpace(allowed)) if allowed == "" { continue } - if strings.EqualFold(allowed, u.Host) || strings.EqualFold(allowed, host) { + if allowed == hostWithPort || allowed == host { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/providers/gcp_wif/provider.go` around lines 331 - 347, hostAllowed currently lowercases host and allowed but then uses strings.EqualFold redundantly; simplify by lowercasing and trimming allowed once, then compare using simple equality against the lowercased u.Hostname() and the lowercased u.Host (if needed). Update the hostAllowed function: compute host := strings.ToLower(u.Hostname()) and normAllowed := strings.ToLower(strings.TrimSpace(allowed)), skip empty normAllowed, and use direct equality checks (normAllowed == host or normAllowed == strings.ToLower(u.Host)) instead of strings.EqualFold to remove redundancy while preserving correct comparisons for u.Host and host.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/auth/providers/gcp_wif/provider.go`:
- Around line 428-440: wifAudience currently returns an empty string when
required fields are missing which lets callers like exchangeToken send an
invalid audience to STS; change wifAudience() to return (string, error) and
return a descriptive error if p.spec.ProjectNumber, pool or provider are empty,
then update callers (notably exchangeToken and any other usages) to handle and
propagate that error (or return it up through Authenticate/Validate) so no empty
audience is ever sent; keep Validate() as-is but treat wifAudience as a
defensive check used by wifAudience(), exchangeToken(), and Authenticate().
- Around line 331-347: hostAllowed currently lowercases host and allowed but
then uses strings.EqualFold redundantly; simplify by lowercasing and trimming
allowed once, then compare using simple equality against the lowercased
u.Hostname() and the lowercased u.Host (if needed). Update the hostAllowed
function: compute host := strings.ToLower(u.Hostname()) and normAllowed :=
strings.ToLower(strings.TrimSpace(allowed)), skip empty normAllowed, and use
direct equality checks (normAllowed == host or normAllowed ==
strings.ToLower(u.Host)) instead of strings.EqualFold to remove redundancy while
preserving correct comparisons for u.Host and host.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/auth/providers/gcp_wif/provider.go
Code review cleanup: add doc comment for getTokenFromURL and adcClientCredentials, add perf.Track to Validate() and Environment(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/auth/providers/gcp_wif/provider.go (1)
275-281:⚠️ Potential issue | 🟠 MajorHost validation bypass for env-sourced URLs still needs hardening.
The past review flagged this: skipping host validation entirely when
fromEnv=truecreates a token exfiltration risk ifACTIONS_ID_TOKEN_REQUEST_URLis poisoned. The bearer token would be sent to an attacker-controlled server.Consider validating against known GitHub Actions OIDC hosts even for environment-sourced URLs:
🔐 Suggested hardening
allowedHosts := ts.AllowedHosts - if !fromEnv && len(allowedHosts) > 0 && !hostAllowed(u, allowedHosts) { + if fromEnv { + host := strings.ToLower(u.Hostname()) + if host != "token.actions.githubusercontent.com" && !strings.HasSuffix(host, ".actions.githubusercontent.com") { + return "", fmt.Errorf("%w: token URL host %q is not a trusted GitHub Actions OIDC host", errUtils.ErrInvalidProviderConfig, u.Hostname()) + } + } else if len(allowedHosts) > 0 && !hostAllowed(u, allowedHosts) { return "", fmt.Errorf("%w: token URL host %q is not allowed; set token_source.allowed_hosts to override", errUtils.ErrInvalidProviderConfig, u.Hostname()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/providers/gcp_wif/provider.go` around lines 275 - 281, The current logic in provider.go skips host validation when fromEnv is true, which allows env-sourced ACTIONS_ID_TOKEN_REQUEST_URL to point at attacker-controlled hosts; instead, keep a whitelist check by augmenting the existing allowedHosts check: define a small constant list of canonical GitHub Actions OIDC hosts (e.g., "actions.githubusercontent.com" and the run-actions subdomain pattern) and when fromEnv==true validate u (the parsed token URL) against either ts.AllowedHosts or this known-GHA whitelist using hostAllowed; if neither matches return the same ErrInvalidProviderConfig error. Update the validation around allowedHosts/hostAllowed and ensure the error message references u.Hostname() as before.
🧹 Nitpick comments (1)
pkg/auth/cloud/gcp/setup.go (1)
116-117: Consider using Viper for ATMOS_ prefixed environment variables.The guideline states that ATMOS_ environment variables should use
viper.BindEnv. Currently, these are read directly viaos.Getenv. This works but diverges from the Atmos pattern where config flows through Viper →atmosConfig.Settings.If this function had access to
atmosConfig, you'd bind these incmd/root.goand read from settings. Since this is a lower-level utility, the directos.Getenvis pragmatic for now, but worth noting for future consistency.As per coding guidelines: "Environment variables MUST use
viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")- ATMOS_ prefix required."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/gcp/setup.go` around lines 116 - 117, The code in setup.go reads ATMOS_GCP_ADC_CLIENT_ID and ATMOS_GCP_ADC_CLIENT_SECRET directly via os.Getenv into customClientID and customClientSecret, which violates the project's guideline to use viper.BindEnv for ATMOS_ variables; fix this by binding those env vars (e.g., viper.BindEnv("ATMOS_GCP_ADC_CLIENT_ID", "ATMOS_GCP_ADC_CLIENT_ID") and similarly for CLIENT_SECRET) — ideally in cmd/root.go where atmosConfig is initialized — and then read them via atmosConfig.Settings or viper.GetString (replace the os.Getenv usages that populate customClientID and customClientSecret) so the values flow through Viper/atmosConfig.Settings per project convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/auth/providers/gcp_wif/provider.go`:
- Around line 275-281: The current logic in provider.go skips host validation
when fromEnv is true, which allows env-sourced ACTIONS_ID_TOKEN_REQUEST_URL to
point at attacker-controlled hosts; instead, keep a whitelist check by
augmenting the existing allowedHosts check: define a small constant list of
canonical GitHub Actions OIDC hosts (e.g., "actions.githubusercontent.com" and
the run-actions subdomain pattern) and when fromEnv==true validate u (the parsed
token URL) against either ts.AllowedHosts or this known-GHA whitelist using
hostAllowed; if neither matches return the same ErrInvalidProviderConfig error.
Update the validation around allowedHosts/hostAllowed and ensure the error
message references u.Hostname() as before.
---
Nitpick comments:
In `@pkg/auth/cloud/gcp/setup.go`:
- Around line 116-117: The code in setup.go reads ATMOS_GCP_ADC_CLIENT_ID and
ATMOS_GCP_ADC_CLIENT_SECRET directly via os.Getenv into customClientID and
customClientSecret, which violates the project's guideline to use viper.BindEnv
for ATMOS_ variables; fix this by binding those env vars (e.g.,
viper.BindEnv("ATMOS_GCP_ADC_CLIENT_ID", "ATMOS_GCP_ADC_CLIENT_ID") and
similarly for CLIENT_SECRET) — ideally in cmd/root.go where atmosConfig is
initialized — and then read them via atmosConfig.Settings or viper.GetString
(replace the os.Getenv usages that populate customClientID and
customClientSecret) so the values flow through Viper/atmosConfig.Settings per
project convention.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/auth/cloud/gcp/setup.gopkg/auth/providers/gcp_wif/provider.go
… whitelist Previously, when the OIDC token URL came from ACTIONS_ID_TOKEN_REQUEST_URL, host validation was skipped entirely. This could allow an attacker who controls that env var to redirect token fetches to arbitrary hosts. Now env-sourced URLs are validated against known GitHub Actions OIDC hosts (*.actions.githubusercontent.com). Explicitly configured URLs continue to use the user-supplied allowed_hosts list. Also adds //nolint:forbidigo to ATMOS_GCP_ADC_CLIENT_* env reads in setup.go, matching the established auth package pattern for env vars read during runtime auth (before Viper/flags are wired). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
CodeRabbit (@coderabbitai) review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/auth/providers/gcp_wif/provider_test.go (1)
423-439:⚠️ Potential issue | 🟠 MajorTest still depends on network; use httptest server instead.
This test sets
ACTIONS_ID_TOKEN_REQUEST_URLto a real GitHub domain and relies on the request failing. It'll be flaky in CI and the assertion (NotContains) is weak. Spin up anhttptest.NewTLSServer, inject its URL, and assert the exact outcome.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/providers/gcp_wif/provider_test.go` around lines 423 - 439, The test TestGetOIDCToken_NilTokenSource_AutoDetectsGitHubActions is flaky because it hits a real GitHub URL instead of a local test server; replace the external dependency by creating an httptest.NewTLSServer that returns a deterministic error/response, set ACTIONS_ID_TOKEN_REQUEST_URL to the test server URL and ACTIONS_ID_TOKEN_REQUEST_TOKEN accordingly, then call p.getOIDCToken and assert the exact expected error/HTTP status/response text from the httptest server (not just that the error doesn't contain "token_source not configured") to make the test hermetic and deterministic while keeping the test focused on Provider.getOIDCToken behavior.
🧹 Nitpick comments (2)
pkg/auth/providers/gcp_wif/provider_test.go (1)
173-175: Minor:t.Setenv("")+os.Unsetenvis redundant.
t.Setenv("")sets the var to empty (still exists), thenos.Unsetenvremoves it. Since you want "not set," just callos.Unsetenvdirectly. Thet.Setenvcall here doesn't add value.This same pattern appears at lines 521-522 and 679-680.
🔧 Suggested simplification
// Test missing env var - ensure it's unset for this test. - t.Setenv("TEST_OIDC_TOKEN", "") os.Unsetenv("TEST_OIDC_TOKEN")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/providers/gcp_wif/provider_test.go` around lines 173 - 175, Remove the redundant t.Setenv("") calls and use only os.Unsetenv to ensure the environment variable is truly unset when testing getTokenFromEnv; specifically, in the test code around the call to p.getTokenFromEnv(p.spec.TokenSource) (and the equivalent spots that repeat this pattern), delete the t.Setenv("TEST_OIDC_TOKEN", "") lines and keep the os.Unsetenv("TEST_OIDC_TOKEN") so the tests reflect an unset variable when calling p.getTokenFromEnv.pkg/auth/providers/gcp_wif/provider.go (1)
295-304: Consider a defensive check when audience cannot be computed.If
wifAudience()returns empty (due to missing ProjectNumber/pool/provider), noaudienceparam gets added. The STS exchange will later fail with a confusing audience mismatch error.Since
Authenticate()validates first, this shouldn't happen in normal flow. But ifgetTokenFromURLis ever called in isolation (tests, future refactors), an explicit check here could surface the real issue earlier.🔧 Optional: fail fast on missing audience
audience := ts.Audience if audience == "" && fromEnv { audience = p.wifAudience() + if audience == "" { + return "", fmt.Errorf("%w: cannot compute WIF audience; ensure project_number, pool_id, and provider_id are configured", errUtils.ErrInvalidProviderConfig) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/providers/gcp_wif/provider.go` around lines 295 - 304, Add a defensive "fail fast" check when computing the audience in the token URL construction: after resolving audience (audience := ts.Audience / audience = p.wifAudience()), if audience is still empty then return an explicit error indicating missing WIF audience instead of proceeding; update getTokenFromURL (and callers if needed) to propagate that error so callers like Authenticate() or isolated tests surface the real configuration problem early. Ensure the error message references the missing ProjectNumber/pool/provider context to make debugging easier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/auth/cloud/gcp/setup.go`:
- Line 110: The hardcoded public OAuth secret assigned to defaultClientSecret is
triggering gitleaks; add an explicit gitleaks allow marker to the same line (in
addition to the existing //nolint:gosec) so the scanner recognizes this
intentional public value; update the declaration of defaultClientSecret in
pkg/auth/cloud/gcp/setup.go to include a gitleaks suppression comment (for
example adding "gitleaks:allow" or the repo's accepted allow marker) preserving
the existing explanatory comment.
---
Duplicate comments:
In `@pkg/auth/providers/gcp_wif/provider_test.go`:
- Around line 423-439: The test
TestGetOIDCToken_NilTokenSource_AutoDetectsGitHubActions is flaky because it
hits a real GitHub URL instead of a local test server; replace the external
dependency by creating an httptest.NewTLSServer that returns a deterministic
error/response, set ACTIONS_ID_TOKEN_REQUEST_URL to the test server URL and
ACTIONS_ID_TOKEN_REQUEST_TOKEN accordingly, then call p.getOIDCToken and assert
the exact expected error/HTTP status/response text from the httptest server (not
just that the error doesn't contain "token_source not configured") to make the
test hermetic and deterministic while keeping the test focused on
Provider.getOIDCToken behavior.
---
Nitpick comments:
In `@pkg/auth/providers/gcp_wif/provider_test.go`:
- Around line 173-175: Remove the redundant t.Setenv("") calls and use only
os.Unsetenv to ensure the environment variable is truly unset when testing
getTokenFromEnv; specifically, in the test code around the call to
p.getTokenFromEnv(p.spec.TokenSource) (and the equivalent spots that repeat this
pattern), delete the t.Setenv("TEST_OIDC_TOKEN", "") lines and keep the
os.Unsetenv("TEST_OIDC_TOKEN") so the tests reflect an unset variable when
calling p.getTokenFromEnv.
In `@pkg/auth/providers/gcp_wif/provider.go`:
- Around line 295-304: Add a defensive "fail fast" check when computing the
audience in the token URL construction: after resolving audience (audience :=
ts.Audience / audience = p.wifAudience()), if audience is still empty then
return an explicit error indicating missing WIF audience instead of proceeding;
update getTokenFromURL (and callers if needed) to propagate that error so
callers like Authenticate() or isolated tests surface the real configuration
problem early. Ensure the error message references the missing
ProjectNumber/pool/provider context to make debugging easier.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
pkg/auth/cloud/gcp/setup.gopkg/auth/providers/gcp_wif/provider.gopkg/auth/providers/gcp_wif/provider_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2109 +/- ##
==========================================
+ Coverage 76.50% 76.54% +0.04%
==========================================
Files 832 832
Lines 79391 79407 +16
==========================================
+ Hits 60736 60783 +47
+ Misses 14860 14834 -26
+ Partials 3795 3790 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
- Add gitleaks:allow marker to public gcloud OAuth secret (setup.go) - Fail fast when env-sourced OIDC URL cannot construct WIF audience (missing project_number/pool/provider), instead of hitting a cryptic STS audience mismatch downstream - Make TestGetOIDCToken_NilTokenSource_AutoDetectsGitHubActions hermetic: replace real GitHub URL with local httptest server - Remove redundant os.Unsetenv calls (t.Setenv provides cleanup) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
CodeRabbit (@coderabbitai) review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/auth/cloud/gcp/setup.go (2)
172-176: Addnolint:forbidigofor consistency.Lines 116-119 annotate
os.Getenvcalls with//nolint:forbidigoexplaining they're called before Viper is wired. The same rationale applies here forGOOGLE_APPLICATION_CREDENTIALSandCLOUDSDK_CONFIG.♻️ Proposed fix
func adcCredentialsPath() string { + //nolint:forbidigo // Direct os.Getenv required: called during auth before Viper/flags are wired. if path := strings.TrimSpace(os.Getenv("GOOGLE_APPLICATION_CREDENTIALS")); path != "" { return path } + //nolint:forbidigo // Direct os.Getenv required: called during auth before Viper/flags are wired. if configDir := strings.TrimSpace(os.Getenv("CLOUDSDK_CONFIG")); configDir != "" { return filepath.Join(configDir, "application_default_credentials.json") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/gcp/setup.go` around lines 172 - 176, In adcCredentialsPath(), add the same `//nolint:forbidigo` annotations used earlier for os.Getenv to the two getenv calls that read "GOOGLE_APPLICATION_CREDENTIALS" and "CLOUDSDK_CONFIG" so the linter knows these environment reads occur before Viper is wired; update the lines that call os.Getenv in the adcCredentialsPath function to include `//nolint:forbidigo` immediately after the call sites to maintain consistency with the other getenv annotations.
101-148: Unused error return value.The function signature includes an error return but all paths return
nil. Consider either simplifying to(string, string)or adding a brief comment indicating the error is reserved for future validation.Not blocking—current code is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/gcp/setup.go` around lines 101 - 148, The resolveADCClientCredentials function currently declares an error return but never returns a non-nil error; remove the unused error from the signature and return only (string, string) from resolveADCClientCredentials (update the function declaration and all call sites to accept two return values), or alternatively keep the error in the signature but add a brief comment above resolveADCClientCredentials explaining the error slot is reserved for future validation; prefer the first option (drop the error) for clarity and adjust callers accordingly (e.g., any code currently doing "id, secret, err := resolveADCClientCredentials()" must be changed to "id, secret := resolveADCClientCredentials()").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/auth/cloud/gcp/setup.go`:
- Around line 172-176: In adcCredentialsPath(), add the same
`//nolint:forbidigo` annotations used earlier for os.Getenv to the two getenv
calls that read "GOOGLE_APPLICATION_CREDENTIALS" and "CLOUDSDK_CONFIG" so the
linter knows these environment reads occur before Viper is wired; update the
lines that call os.Getenv in the adcCredentialsPath function to include
`//nolint:forbidigo` immediately after the call sites to maintain consistency
with the other getenv annotations.
- Around line 101-148: The resolveADCClientCredentials function currently
declares an error return but never returns a non-nil error; remove the unused
error from the signature and return only (string, string) from
resolveADCClientCredentials (update the function declaration and all call sites
to accept two return values), or alternatively keep the error in the signature
but add a brief comment above resolveADCClientCredentials explaining the error
slot is reserved for future validation; prefer the first option (drop the error)
for clarity and adjust callers accordingly (e.g., any code currently doing "id,
secret, err := resolveADCClientCredentials()" must be changed to "id, secret :=
resolveADCClientCredentials()").
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
pkg/auth/cloud/gcp/setup.gopkg/auth/providers/gcp_wif/provider.gopkg/auth/providers/gcp_wif/provider_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/auth/providers/gcp_wif/provider.go
…ials The function always returns a valid credential pair (falling back to public gcloud defaults), so the error return was dead code. Removing it simplifies the caller in SetupFiles and the three test call sites. Also adds //nolint:forbidigo to os.Getenv calls in adcCredentialsPath() for consistency with the ATMOS_GCP_ADC_* env reads above. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
CodeRabbit (@coderabbitai) review |
Contributor
✅ Actions performedReview triggered.
|
Mikhail Shirkov (shirkevich)
changed the title
…t mock gsmClientSecretCreationMock was ignoring its projectID parameter and hardcoding "test-project". All callers happened to pass "test-project" so this was latent, but the mock should respect its contract. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
In GitHub Actions, token_source is now auto-detected from ACTIONS_ID_TOKEN_REQUEST_URL. The audience is auto-constructed from project_number/pool_id/provider_id, and the token URL is validated against known GitHub Actions OIDC hosts. Updated providers.mdx to show the simplified config (no token_source block needed) and documented all auto-detection behavior. Updated blog post to split ADC and WIF examples and explain the zero-config GitHub Actions experience. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
Feb 27, 2026
|
These changes were released in v1.208.0. |
Igor Rodionov (goruha)
added a commit
that referenced
this pull request
Mar 9, 2026
…raform-plan * osterman/native-ci-terraform: (28 commits) feat: Add source cache TTL for JIT-vendored components (#2138) feat: Per-target version overrides in vendor manifests (#2141) docs: Add PRD for browser-based auth in aws/user identity (#1887) docs: Add EKS kubeconfig authentication integration PRD (#1884) fix: correct marketplace.json schema and update docs with install/uninstall commands (#2142) fix: propagate auth to all YAML functions in multi-component execution (#2140) fix: Use atmos_component for source provisioner workdir paths (#2137) Fix identity prompts to respect --interactive flag (#2130) Increase PR size thresholds to accommodate AI-assisted development (#2136) docs: Add Azure authentication provider documentation (#2132) fix: propagate component-type level dependencies through stack processor (#2127) fix: Add retry and missing workflow step properties to all schema copies (#2113) Exclude unsupported windows/arm from goreleaser build matrix (#2133) Add AI Agent Skills for LLM-Powered Infrastructure Development (#2121) Fix: Convert toolchain paths to absolute in PATH to resolve exec.LookPath failures (#2095) Fix workdir collision for component instances sharing base component (#2093) fix(auth): propagate TTY state to subprocesses for SSO device flow in workflows (#2126) fix(security): prevent SSRF in GitHub OIDC token URL handling (CWE-918) (#2106) Fix #2112: add workflow_retry definition and retry property to workflow step schema (#2114) fix(auth): auto-detect GitHub Actions WIF with proper audience, host validation, and lazy GSM init (#2109) ...
|
These changes were released in v1.208.1-test.9. |
|
These changes were released in v1.208.1-test.10. |
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
3 participants
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.
What
Fix GCP Workload Identity Federation (WIF) for GitHub Actions CI environments by auto-detecting the OIDC token source, constructing the correct audience, and validating token URL hosts.
Core fixes (GitHub Actions WIF)
token_sourceis not explicitly configured — detectsACTIONS_ID_TOKEN_REQUEST_URLand defaults to URL-based token fetch.project_number,workload_identity_pool_id, andworkload_identity_provider_idso the OIDC token'saudclaim matches what GCP STS expects. Without this, GitHub defaults to the repo owner URL (e.g.,https://github.com/org), which STS rejects.*.actions.githubusercontent.com) instead of skipping host validation entirely.Supporting fixes
ensureClient()instead of eagerly creating it in the constructor — solves the chicken-and-egg problem where store init during config loading happens before auth sets credentials.client_id/client_secretas an atomic pair — custom ID without matching secret falls back to the full default pair to preventinvalid_clienterrors.resolveADCClientCredentials()(always returns a valid pair).gitleaks:allowmarker to public gcloud OAuth secret.//nolint:forbidigotoos.Getenvcalls in auth code that runs before Viper/flags are wired.perf.Trackcalls per project conventions.httptestservers; remove redundantos.Unsetenvcalls.Why
GCP WIF authentication was failing in GitHub Actions CI with three sequential errors:
token_source not configured— nil token source with no auto-detectiontoken URL host not allowed— GitHub Actions uses dynamic subdomainsaudience mismatch— OIDCaudclaim defaulted to repo owner URL instead of WIF provider resource nameReferences
shirkevich/atmos