[WIP] Add support for OTLP export auth via Workload Identity Federation - #50184
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey 👋 — thanks for kicking off the Workload Identity Federation support for OTLP export auth via the coding agent! This is a solid feature area (OTLP/OIDC auth) and fits well within the project's scope. A few notes since this is still early/WIP:
Since this is marked Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage: feature (OTel) / high risk
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds Google Workload Identity Federation authentication for OTLP exports.
Changes:
- Adds WIF configuration, schema, and validation.
- Generates GitHub OIDC-to-Google access-token exchange steps.
- Adds firewall entries, tests, and documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/setup_step_version_test.go |
Tests token-exchange setup generation. |
pkg/workflow/permissions_compiler_validator.go |
Validates WIF OIDC permissions. |
pkg/workflow/observability_otlp.go |
Parses WIF and adds network domains. |
pkg/workflow/observability_otlp_test.go |
Tests WIF OIDC detection. |
pkg/workflow/frontmatter_types.go |
Defines WIF configuration types. |
pkg/workflow/compiler_yaml_step_generation.go |
Generates Google token exchange steps. |
pkg/workflow/compiler_validators.go |
Validates WIF fields and combinations. |
pkg/parser/schemas/main_workflow_schema.json |
Adds the WIF frontmatter schema. |
docs/src/content/docs/guides/open-telemetry.mdx |
Documents Google WIF configuration. |
.github/workflows/smoke-goose.lock.yml |
Regenerates step capitalization. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
pkg/workflow/compiler_yaml_step_generation.go:212
- This error hides the IAM Credentials HTTP status, making an invalid service-account value indistinguishable from missing impersonation permissions. Include the status and the likely corrective action.
" if (!impersonationResponse.ok) throw new Error('Google service account impersonation failed');\n",
- Files reviewed: 10/10 changed files
- Comments generated: 5
- Review effort level: Balanced
| var audience string | ||
| if workloadIdentity != nil { | ||
| audience = strings.TrimSpace(workloadIdentity.Audience) | ||
| } else { | ||
| audience = strings.TrimSpace(githubApp.Audience) |
| lines = append(lines, formatYAMLEnv(" ", "GH_AW_OTLP_OIDC_AUDIENCE", audience)) | ||
| } | ||
|
|
||
| if workloadIdentity != nil { |
| if workloadIdentity := getOTLPWorkloadIdentity(workflowData.ParsedFrontmatter, workflowData.RawFrontmatter); workloadIdentity != nil && | ||
| strings.EqualFold(strings.TrimSpace(workloadIdentity.Provider), "google") { |
| if !requiresIDTokenWrite && hasOTLPGitHubOIDCAuth(workflowData.ParsedFrontmatter, workflowData.RawFrontmatter) { | ||
| requiresIDTokenWrite = true | ||
| errorPrefix = "observability.otlp.github-app" | ||
| if getOTLPWorkloadIdentity(workflowData.ParsedFrontmatter, workflowData.RawFrontmatter) != nil { | ||
| errorPrefix = "observability.otlp.workload-identity" |
| " scope: 'https://www.googleapis.com/auth/cloud-platform',\n", | ||
| " }),\n", | ||
| " });\n", | ||
| " if (!response.ok) throw new Error('Google workload identity token exchange failed');\n", |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 36.5 AIC · ⌖ 12.3 AIC · ⊞ 5.4K
| if workflowData.NetworkPermissions == nil { | ||
| workflowData.NetworkPermissions = &NetworkPermissions{} | ||
| } | ||
| workflowData.NetworkPermissions.Allowed = append(workflowData.NetworkPermissions.Allowed, |
There was a problem hiding this comment.
Bug: WIF network domains appended once per OTLP endpoint (duplicates)
The sts.googleapis.com, iamcredentials.googleapis.com, and oauth2.googleapis.com domains are appended inside the for _, e := range entries loop. With multiple OTLP endpoints configured, these three domains are added to the allowlist N times — once per endpoint — producing duplicate entries in the network allowlist.
Move the WIF guard block outside (and after) the loop so it runs at most once.
@copilot please address this.
There was a problem hiding this comment.
The WIF host injection was hoisted out of the endpoint loop in 81768e1, so the hosts are added at most once regardless of endpoint count (and now also when the endpoint is an expression with no extractable domain).
| workflowData.NetworkPermissions.Allowed = append(workflowData.NetworkPermissions.Allowed, | ||
| "sts.googleapis.com", | ||
| "iamcredentials.googleapis.com", | ||
| "oauth2.googleapis.com", |
There was a problem hiding this comment.
Unused domain: oauth2.googleapis.com is never called in generated code
The generated JavaScript in generateOTLPOIDCMintStep only calls (sts.googleapis.com/redacted) and (iamcredentials.googleapis.com/redacted) There is no call to oauth2.googleapis.com anywhere in the generated code. Adding it to the network allowlist widens the firewall surface unnecessarily.
Remove the oauth2.googleapis.com entry, or add a comment explaining why it is needed.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /diagnosing-bugs, and /tdd — requesting changes on correctness and debuggability issues.
📋 Key Themes & Highlights
Issues Found
- Duplicate network-domain injection (
sts/iamcredentialsappended N times when N endpoints are configured) — correctness bug - Dead redundant nil-check inside the endpoint loop — dead code left from merging two code paths
- Unreferenced allowlist entry
oauth2.googleapis.com— never used by the generated JS steps - Opaque error messages in the JS token-exchange steps — HTTP status and body are not surfaced, making IAM failures hard to diagnose
- Always-emitted empty env var
GH_AW_OTLP_WIF_SERVICE_ACCOUNTwhenservice-accountis unset - Missing test for service-account-omitted path — the optional impersonation branch has no negative assertion
Positive Highlights
- ✅ Clean two-step design: mint OIDC token → exchange for GCP access token — follows the existing
github-apppattern well - ✅
core.setSecretcalled on both the OIDC and access tokens — good hygiene - ✅ Validation correctly blocks combining
workload-identitywithgithub-appcredentials - ✅
id-token: writepermission error message updated to use the correct config path
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 49.1 AIC · ⌖ 8.56 AIC · ⊞ 7.1K
Comment /matt to run again
| } | ||
| if workloadIdentity := getOTLPWorkloadIdentity(workflowData.ParsedFrontmatter, workflowData.RawFrontmatter); workloadIdentity != nil && | ||
| strings.EqualFold(strings.TrimSpace(workloadIdentity.Provider), "google") { | ||
| if workflowData.NetworkPermissions == nil { |
There was a problem hiding this comment.
[/codebase-design] Dead code: NetworkPermissions is already guaranteed non-nil at line 745 (inside the same if domain != "" block), so this inner nil-check at line 750 can never execute.
💡 Suggested fix
Remove lines 750–752 — the outer guard at line 745 covers it.
// before setup — just append directly:
workflowData.NetworkPermissions.Allowed = append(...)@copilot please address this.
There was a problem hiding this comment.
Resolved by hoisting the WIF block out of the endpoint loop; the remaining nil-check is now the only guard for that path. Commit 81768e1.
| workflowData.NetworkPermissions.Allowed = append(workflowData.NetworkPermissions.Allowed, | ||
| "sts.googleapis.com", | ||
| "iamcredentials.googleapis.com", | ||
| "oauth2.googleapis.com", |
There was a problem hiding this comment.
[/diagnosing-bugs] oauth2.googleapis.com is added to the network allowlist but never contacted by the generated exchange steps — only sts.googleapis.com and iamcredentials.googleapis.com are used. This either allows unnecessary network egress or is a dead entry left from an earlier design.
💡 Suggested fix
Remove "oauth2.googleapis.com" from the Allowed list unless a concrete future use is planned and documented. Unnecessary firewall holes widen the attack surface.
@copilot please address this.
There was a problem hiding this comment.
Removed oauth2.googleapis.com from the allowlist — only sts.googleapis.com and iamcredentials.googleapis.com are contacted. Commit 34ff26b.
| if workflowData.NetworkPermissions == nil { | ||
| workflowData.NetworkPermissions = &NetworkPermissions{} | ||
| } | ||
| if workloadIdentity := getOTLPWorkloadIdentity(workflowData.ParsedFrontmatter, workflowData.RawFrontmatter); workloadIdentity != nil && |
There was a problem hiding this comment.
[/codebase-design] The Google WIF network-domain injection is nested inside the for _, e := range entries loop, so sts.googleapis.com and iamcredentials.googleapis.com are appended once per OTLP endpoint. If multiple endpoints are configured the allowlist will contain duplicates.
💡 Suggested fix
Hoist the WIF domain injection above the for loop — it only depends on workload-identity config, not on each individual endpoint:
if wi := getOTLPWorkloadIdentity(...); wi != nil && strings.EqualFold(...) {
if workflowData.NetworkPermissions == nil { ... }
workflowData.NetworkPermissions.Allowed = append(..., "sts.googleapis.com", "iamcredentials.googleapis.com")
}
for _, e := range entries { ... }@copilot please address this.
| " scope: 'https://www.googleapis.com/auth/cloud-platform',\n", | ||
| " }),\n", | ||
| " });\n", | ||
| " if (!response.ok) throw new Error('Google workload identity token exchange failed');\n", |
There was a problem hiding this comment.
[/diagnosing-bugs] When the STS token exchange fails, the error message "Google workload identity token exchange failed" does not include the HTTP status code or response body, making it hard to diagnose IAM misconfiguration in CI logs.
💡 Suggested fix
if (!response.ok) {
const body = await response.text().catch(() => "");
throw new Error(`Google WIF token exchange failed: ${response.status} ${body}`);
}Apply the same pattern to the impersonation response at line 212.
@copilot please address this.
There was a problem hiding this comment.
Both the STS and impersonation error paths now include the HTTP status/statusText plus configuration guidance. Commit 81768e1.
| } | ||
| if !strings.Contains(combined, "https://sts.googleapis.com/v1/token") || !strings.Contains(combined, "iamcredentials.googleapis.com") { | ||
| t.Fatalf("expected setup step to include Google STS and service account exchange, got:\n%s", combined) | ||
| } |
There was a problem hiding this comment.
[/tdd] The new test checks happy-path output strings but has no coverage for the WIF-without-service-account case (i.e. service-account is omitted). The impersonation branch at compiler line 206 would silently be skipped and a regression there would go undetected.
💡 Suggested additional test
Add a second test TestGenerateSetupStepExchangesGoogleOTLPWorkloadIdentityTokenWithoutServiceAccount that omits service-account and asserts the iamcredentials.googleapis.com call is NOT present in the generated output, confirming the optional-impersonation path is correct.
@copilot please address this.
There was a problem hiding this comment.
Added TestGenerateSetupStepExchangesGoogleOTLPWorkloadIdentityTokenWithoutServiceAccount, which omits service-account and asserts the impersonation env var is not emitted. Commit 34ff26b.
| " env:\n", | ||
| " GH_AW_OTLP_OIDC_TOKEN: ${{ steps.mint-otlp-oidc-token.outputs.token }}\n", | ||
| ) | ||
| lines = append(lines, formatYAMLEnv(" ", "GH_AW_OTLP_WIF_AUDIENCE", workloadIdentity.Audience)) |
There was a problem hiding this comment.
[/codebase-design] GH_AW_OTLP_WIF_SERVICE_ACCOUNT is always emitted as an env var even when service-account is empty, injecting a blank environment variable into every WIF exchange step. Use formatYAMLEnv conditionally.
💡 Suggested fix
if sa := strings.TrimSpace(workloadIdentity.ServiceAccount); sa != "" {
lines = append(lines, formatYAMLEnv(" ", "GH_AW_OTLP_WIF_SERVICE_ACCOUNT", sa))
}@copilot please address this.
There was a problem hiding this comment.
GH_AW_OTLP_WIF_SERVICE_ACCOUNT is now emitted only when service-account is non-empty. Commit 34ff26b.
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (203 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot This PR needs a quick maintainer-ready refresh.
Run: https://github.com/github/gh-aw/actions/runs/30912825871
|
All four items are now done:
Branch is up to date with Remaining blockers: CI on the current HEAD is stale — my pushes don't trigger workflows, so a maintainer needs to re-trigger it (close/reopen, |
|
@copilot This PR is close to maintainer-ready.
Run: https://github.com/github/gh-aw/actions/runs/30917238977
|
Final Review state
Branch — up to date with Local validation — Remaining blockers
|
| lines = append(lines, | ||
| " with:\n", | ||
| " script: |\n", | ||
| " const oidcToken = process.env.GH_AW_OTLP_OIDC_TOKEN;\n", |
There was a problem hiding this comment.
@copilot move to javascript file in actions/setup/js and add tests
There was a problem hiding this comment.
Moved the exchange logic to actions/setup/js/exchange_otlp_workload_identity.cjs with 8 vitest cases in exchange_otlp_workload_identity.test.cjs (STS success, impersonation, no service account, STS/IAM HTTP-status errors, missing access token, missing OIDC token).
Since this step runs before Setup Scripts copies the .cjs files onto the runner, the compiler can't require() it from ${{ runner.temp }}/gh-aw/actions. So the file is mirrored into pkg/workflow/js/ and //go:embeded, then inlined via FormatJavaScriptForYAML — same pattern as safe_outputs_tools.json, with TestExchangeOTLPWorkloadIdentityScriptInSync enforcing the two copies stay byte-identical.
…overage Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The failure was |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed properly this time. My previous fix used inline Replaced them with prettier-stable typed intermediate variables: /** @type {any} */
const stsPayload = await response.json();
let accessToken = stsPayload.access_token;Verified |
|
🎉 This pull request is included in a new release. Release: |
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/30909334614
safeoutputs add_comment --pr_number 50184 --body $'
@copilotThis PR needs a quick maintainer-ready refresh.Run: 'https://github.com/github/gh-aw/actions/runs/30909334614
PR 50183 combined nudge
safeoutputs update_pull_request --pr_number 50183 --update_branch true --operation append --body $'
Run: 'https://github.com/github/gh-aw/actions/runs/30909334614
Run: https://github.com/github/gh-aw/actions/runs/30912825871