[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
|
There was a problem hiding this comment.
🧪 Test Quality Sentinel Report
✅ Test Quality Score: 100/100 — Excellent
Analyzed 2 test(s): 2 design, 0 implementation, 0 violation(s).
📊 Metrics (2 tests)
| Metric | Value |
|---|---|
| Analyzed | 2 (Go: 2, JS: 0) |
| ✅ Design | 2 (100%) |
| 0 (0%) | |
| Edge/error coverage | 2 (100%) |
| Duplicate clusters | 0 |
| Inflation | No (1.1:1 test:prod ratio) |
| 🚨 Violations | 0 |
| Test | File | Classification | Coverage |
|---|---|---|---|
Added to TestHasOTLPGitHubOIDCAuth (raw map) |
observability_otlp_test.go:283 |
design_test / high_value | Google workload-identity configuration parsing |
TestGenerateSetupStepExchangesGoogleOTLPWorkloadIdentityToken |
setup_step_version_test.go:359 |
design_test / high_value | STS exchange + token passing validation |
Verdict
✅ Passed. 0% implementation tests (threshold: 30%). All tests verify external user-visible behavior with comprehensive assertions and no forbidden patterns detected. Test inflation ratio 1.1:1 (well below 2:1 threshold).
Key strengths:
- All 2 tests are design tests verifying configuration parsing and code generation
- Comprehensive error assertions with descriptive context messages
- Tests edge cases: raw frontmatter map parsing and Google-specific STS endpoints
- No mocking libraries, proper build tags, clean assertions
- Excellent test:production ratio (1.1:1)
|
@copilot run pr-finisher skill (local skill, no agent finder) |
There was a problem hiding this comment.
Verdict: Request changes
The WIF token-exchange flow is a reasonable design, but the implementation has a correctness bug (domain duplication per endpoint) and a maintainability/correctness inconsistency (hardcoded step-id reference next to a step-id abstraction that exists specifically to avoid this).
Themes
- Domain duplication:
sts.googleapis.com,iamcredentials.googleapis.com, andoauth2.googleapis.comare appended inside the per-endpoint loop, so a workflow with N OTLP endpoints gets these three domains added N times to the firewall allowlist. - Inconsistent step-id reference:
getOTLPAuthTokenStepIDwas introduced to abstract which step produced the OIDC/WIF token, but the workload-identity exchange step itself still referencessteps.mint-otlp-oidc-tokendirectly instead of using the helper (or being colocated with it), which is fragile if the mint step is ever renamed or conditionally skipped. - Missing dedicated unit test for
validateOTLPWorkloadIdentitycovering provider mismatch, missing audience, and github-app conflict cases. - Error handling in the generated STS/IAM exchange script discards the response body, making CI failures hard to diagnose.
🔎 Code quality review by PR Code Quality Reviewer · auto · 101.4 AIC · ⌖ 4.53 AIC · ⊞ 7.9K
Comment /review to run again
| if workloadIdentity := getOTLPWorkloadIdentity(workflowData.ParsedFrontmatter, workflowData.RawFrontmatter); workloadIdentity != nil && | ||
| strings.EqualFold(strings.TrimSpace(workloadIdentity.Provider), "google") { | ||
| if workflowData.NetworkPermissions == nil { | ||
| workflowData.NetworkPermissions = &NetworkPermissions{} | ||
| } | ||
| workflowData.NetworkPermissions.Allowed = append(workflowData.NetworkPermissions.Allowed, |
There was a problem hiding this comment.
Google WIF domains are appended inside the endpoint loop, so they get duplicated once per configured OTLP endpoint entry.
| if workloadIdentity != nil { | ||
| compilerYamlStepGenerationLog.Print("Generating Google OTLP workload identity token exchange step before setup") | ||
| lines = append(lines, | ||
| " - name: Exchange OTLP workload identity token\n", | ||
| " id: exchange-otlp-workload-identity-token\n", | ||
| fmt.Sprintf(" uses: %s\n", getCachedActionPin("actions/github-script", data)), | ||
| " env:\n", | ||
| " GH_AW_OTLP_OIDC_TOKEN: ${{ steps.mint-otlp-oidc-token.outputs.token }}\n", |
There was a problem hiding this comment.
This step references the mint step output by hardcoded id steps.mint-otlp-oidc-token, bypassing the getOTLPAuthTokenStepID abstraction added right below — if the mint step id ever changes this silently breaks.
There was a problem hiding this comment.
The exchange step reads the mint step output via fmt.Sprintf(..., otlpOIDCMintStepID), not a hardcoded literal — getOTLPAuthTokenStepID is a separate concern (which step downstream consumers read the final token from).
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
No unit test exercises validateOTLPWorkloadIdentity directly (missing provider, missing audience, or combined with github-app) — only the OIDC-auth detection test was extended.
There was a problem hiding this comment.
Added TestValidateOTLPWorkloadIdentity covering non-google provider, missing audience, the github-app combination, and the valid/absent cases. Commit 34ff26b.
| " 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.
Error paths swallow the STS/IAM response body, so failures only say "exchange failed" with no diagnostic detail — this will be painful to debug in CI logs.
|
@copilot run pr-finisher skill (local skill, not agent finder) |
… firewall hosts, error detail Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection results could not be parsed. Review the workflow run logs for details. Triage update: feature (OTel) / high risk
|
…vice-account env, add validator and no-service-account tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@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, |
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