feat: OIDC token mint dispatch — deprecate PAT, migrate to OIDC#503
Conversation
Site previewPreview: https://d995ab16-site.fullsend-ai.workers.dev Commit: |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: #503
Head SHA: 7d62d54
Timestamp: 2026-04-28T00:00:00Z
Outcome: request-changes
Summary
This PR adds a well-architected OIDC dispatch mode as an alternative to PAT-based dispatch, directly addressing #308. The design follows existing patterns (mode-switched layer, interface-based GCP client), the test coverage is thorough, and the shim workflow correctly uses environment variables to prevent script injection. However, there are two high-severity issues that must be resolved before merge: (1) the Cloud Function source code is never uploaded to GCS, so deployment will fail at runtime, and (2) the workflow_file request parameter is used unsanitized in the dispatch URL path, creating a path traversal risk in the security-critical dispatch proxy.
Findings
High
-
[Correctness]
internal/dispatch/gcf/gcf.go:149-162— Cloud Function source never uploaded to GCS. TheProvisionmethod callsCreateFunctionwhich references a storage source ({projectID}-gcf-source/{functionName}/source.zip), but no step in the provisioning flow uploads the Go source zip to that bucket. TheFunctionConfig.Sourcefield exists but is never populated. Deployment will fail at runtime with a "source not found" error.
Remediation: Add a step between secret provisioning and function deployment that (a) creates the GCS bucket if needed, (b) zips the embedded Cloud Function source, and (c) uploads it to the expected storage path. The implementation plan mentions//go:embedfor the source — implement this. -
[Platform security]
internal/dispatch/gcf/function/main.go:175-178—workflow_fileinput is not validated before being interpolated into the GitHub API URL path. A caller that passes OIDC validation (any repo in the org) could send aworkflow_filevalue containing path traversal characters (e.g.,../../other-org/other-repo/actions/workflows/evil.yml) to construct a dispatch URL targeting a different repository. While the GitHub API would likely reject malformed paths, defense-in-depth requires validating the input at the proxy layer.
Remediation: Validatereq.WorkflowFileagainst a strict pattern like^[a-zA-Z0-9_.-]+\.ya?ml$before using it in URL construction. Reject requests that don't match. -
[Platform security]
internal/dispatch/gcf/function/main.go:175—source_repoin the request body is trusted without verification against the OIDC token claims. After OIDC validation, the Cloud Function knows the caller is from the configured org, butsource_repois taken from the JSON body, not from the OIDC token'srepositoryclaim. A workflow in org repo A could dispatch withsource_reposet to org repo B, potentially causing the downstream triage/code/review workflow to operate on the wrong repository context.
Remediation: After STS token exchange, decode the original OIDC token'srepositoryclaim and validate thatreq.SourceRepomatches it. Alternatively, extractsource_repofrom the token itself rather than the request body.
Medium
-
[Correctness]
internal/dispatch/gcf/gcf.go:155-162— Cloud Function URL is a hardcoded format string placeholder, not the actual deployed URL. When a new function is created, the code constructshttps://{region}-{project}.cloudfunctions.net/{name}and discards the LRO operation name (_ = opName). Cloud Functions v2 use Cloud Run URLs (https://{name}-{hash}-{region}.a.run.app), not the v1 format. The storedFULLSEND_DISPATCH_URLwill be wrong.
Remediation: AfterCreateFunctionreturns the operation name, poll the operation until complete, then callGetFunctionto retrieve the actualserviceConfig.uri. -
[Platform security]
internal/dispatch/gcf/gcp.go:279-294— No concurrency or instance limits set on the Cloud Function. The implementation plan explicitly called out DoS mitigation via "Cloud Function concurrency caps," but theCreateFunctionpayload doesn't setmaxInstanceCountormaxInstanceRequestConcurrencyin the service config.
Remediation: Add"maxInstanceCount": 10(or a configurable value) to theserviceConfigin the function creation payload. -
[Correctness]
internal/dispatch/gcf/gcp.go:189-211—SetIAMBindingappends a new binding on every call without checking for duplicates. Repeatedfullsend admin installruns will accumulate duplicate IAM bindings on the secret resource.
Remediation: Before appending, check whether the role+member combination already exists in the current policy bindings.
Low
-
[Style/conventions]
internal/dispatch/gcf/function/main.go:395-409—GenerateTestRSAKeyis exported in production code (function/main.go) rather than in a test file. It uses a 1024-bit key explicitly marked "DO NOT use in production" but is available to any importer.
Remediation: Move this function to a_test.gofile or atestutilpackage. -
[Platform security]
internal/dispatch/gcf/function/main.go:131— Error responses from the Cloud Function include internal details (e.g.,"OIDC validation failed: STS returned 403: {GCP error body}"). Since the function is internet-accessible to org members, these details could leak GCP infrastructure information.
Remediation: Return generic error messages to callers and log detailed errors server-side. -
[Correctness]
internal/cli/admin.go:218-233— WhendispatchMode == "oidc-gcf", the code searchesagentCredsfor a credential withRole == "fullsend"but silently leavesappPEMandappIDas zero values if none is found. The provisioner will reject empty PEM, but the error message ("GitHub App PEM is required") won't indicate that the root cause is a missing fullsend role credential.
Remediation: Add an explicit check: if no fullsend role credential is found, return a descriptive error like"--dispatch-mode=oidc-gcf requires a 'fullsend' role agent credential".
Info
-
[Style/conventions] The
DispatchTokenLayerstruct name is now a misnomer in OIDC mode since it doesn't manage a token. Consider renaming toDispatchLayerin a follow-up to match the conceptual shift. -
[Correctness] The shim workflow OIDC template (
shim-workflow-oidc.yaml) correctly usespull_request_target(notpull_request) and passes event data via environment variables, following security best practices for untrusted PR workflows. Thecore.setSecret(token)call properly masks the OIDC token in logs.
Footer
Outcome: request-changes
This review applies to SHA 7d62d547b18b8cc5e8416f6a2e9afff7a91c9b96. Any push to the PR head clears this review and requires a new evaluation.
There was a problem hiding this comment.
Review: #503
Head SHA: 04bc260
Timestamp: 2026-04-28T22:00:00Z
Outcome: request-changes
Summary
This PR adds a well-architected OIDC dispatch mode via GCP Cloud Functions, with strong security design (WIF attribute conditions, OIDC pre-validation, input sanitization, PEM zeroing). However, the Cloud Function deployment path is incomplete — the source upload to GCS is missing and the function module lacks GCF v2 entry point registration, which means fullsend admin install --dispatch-mode=oidc-gcf will fail at the deploy step. The installation token requested by the Cloud Function is also unnecessarily broad (full app permissions instead of scoped-down actions:write on .fullsend). These issues must be resolved before merging.
Findings
High
-
[correctness]
internal/dispatch/gcf/gcp.go:431-480— Cloud Function deployment is incomplete.CreateFunctionreferences a GCS storage source (projectID-gcf-sourcebucket,functionName/source.zipobject) but no code creates the bucket or uploads the source zip. TheFunctionConfig.Sourcefield (line 35) holds[]bytefor zipped source but is never used inCreateFunction. Furthermore,Provision()ingcf.go:184passes an emptySourcefield. The PR description mentions source "embedded via//go:embed" but no//go:embeddirective exists anywhere in the diff. Deployment will fail with a missing-source error.
Remediation: Add a GCS bucket creation step and a source upload step before callingCreateFunction. Either embed the function source via//go:embedin the provisioner binary or use the Cloud Functions v2 inline upload API (generateUploadUrl+ PUT). -
[correctness]
internal/dispatch/gcf/function/main.go— The Cloud Function module lacks GCF v2 entry point registration. Cloud Functions v2 (Go) requires callingfunctions.HTTP("functionName", handler)in aninit()function (fromgithub.com/GoogleCloudPlatform/functions-framework-go). The module has noinit(), nomain(), and no functions-framework dependency ingo.mod. Even if the source were uploaded, the Cloud Function would not start.
Remediation: Addgithub.com/GoogleCloudPlatform/functions-framework-gotofunction/go.mod, add aninit()that registers the handler viafunctions.HTTP(), and ensureEntryPointinFunctionConfigmatches the registered name.
Medium
-
[platform-security]
internal/dispatch/gcf/function/main.go:390-419—createInstallationTokensends an empty POST body, so GitHub returns a token with ALL permissions the app has. The Cloud Function only needsactions:writeon the.fullsendrepo. An over-scoped token increases blast radius if the function is compromised.
Remediation: Scope the installation token request by including{"permissions": {"actions": "write"}, "repositories": [".fullsend"]}in the POST body. This follows the principle of least privilege. -
[correctness]
internal/dispatch/gcf/gcp.go:483-517—WaitForOperationpolls every 5 seconds with no maximum iteration count or built-in timeout. If the operation never completes and the caller's context has no deadline (CLI commands typically don't), this will block indefinitely.
Remediation: Add a maximum timeout (e.g., 10 minutes) or iteration cap. Considercontext.WithTimeoutwrapping the poll loop. -
[correctness]
internal/forge/github/github.go:1480-1498—OrgVariableExistsdoesn't handle HTTP 403 like the existingOrgSecretExistsdoes (which returns a clear "insufficient permissions" error). When the token lacksadmin:orgscope, the user gets a generic error instead of actionable guidance.
Remediation: Add acase http.StatusForbiddenbranch mirroringOrgSecretExists(line 1394-1395).
Low
- [correctness]
internal/dispatch/gcf/gcf.go:143-153— When the Secret Manager secret already exists (secretExists == true),AddSecretVersionis skipped. If the GitHub App PEM has been rotated since the last install, re-runninginstallwill leave the stale PEM in Secret Manager, causing dispatch failures with the new key.
Remediation: Consider always adding a new secret version (Secret Manager retains version history), or add a--force-update-pemflag.
Info
- [style]
internal/dispatch/gcf/function/main.go:41—event_typeis validated as non-empty but not restricted to known values (issues,issue_comment,pull_request_target). An allowlist would provide defense-in-depth against unexpected event routing.
Footer
Outcome: request-changes
This review applies to SHA 04bc260d747ac3c1df05c4b0746acca46da27d2d. Any push to the PR head clears this review and requires a new evaluation.
- Add GCF v2 entry point registration with functions-framework-go - Add SecretAccessor implementation using metadata server auth - Add source upload via generateUploadUrl API before CreateFunction - Scope installation token to actions:write only (least privilege) - Add 10-minute timeout to WaitForOperation polling - Add 403 handling to OrgVariableExists matching OrgSecretExists Signed-off-by: Wayne Sun <gsun@redhat.com>
Review: #503Head SHA: 86acd66 SummaryThis PR is a substantial, well-structured migration from PAT-based dispatch to OIDC token minting. The security architecture is sound: OIDC tokens are the only credential, PEM keys are stored in Secret Manager (never in GitHub), FindingsMedium
Low
Info
PR-Specific ChecksPR body injection defense: The PR body contains no non-rendering Unicode, no prompt injection patterns, and no directives to skip checks or approve unconditionally. The body is a thorough technical description of the change. Clean. Scope authorization: No linked issue. The PR is labeled FooterOutcome: comment-only Previous runReview: #503Head SHA: 3676a72 SummaryThis is a well-architected migration from PAT-based dispatch to OIDC token minting with comprehensive security controls. The token mint handler implements defense-in-depth with two-layer org validation (prevalidation + STS), FindingsMedium
Low
Info
FooterOutcome: comment-only Previous run (2)Review: #503Head SHA: cf995d2 SummaryThis is a substantial and well-engineered migration from PAT-based dispatch to OIDC token minting via a GCP Cloud Function. The security architecture is strong: multi-layer OIDC validation (pre-validation + STS exchange), role-based permission scoping, PEM zeroing, defense-in-depth input validation throughout workflow templates, and systematic script-injection prevention via FindingsMedium
Low
Info
FooterOutcome: comment-only Previous run (3)Review: #503Head SHA: 75be54d SummaryThis is a well-structured migration from PAT-based dispatch to OIDC token minting. The security posture improves significantly: long-lived PATs are eliminated, tokens are short-lived and role-scoped, and the mint validates OIDC JWTs via WIF with defense-in-depth (pre-validation + STS exchange). The implementation demonstrates strong security practices throughout — PEM zeroing, bounded response body drains, token masking, script injection prevention via FindingsMedium
Low
Info
FooterOutcome: comment-only Previous run (4)Review: #503Head SHA: 309c491 SummaryThe OIDC token mint implementation is well-engineered with strong security properties — defense-in-depth JWT validation (pre-validation + STS), strict input validation via regex, PEM zeroing after use, token masking in workflows, and safe workflow template patterns (all attacker-controlled data via FindingsCritical(none) High(none) Medium
Low
Info
FooterOutcome: comment-only Previous run (5)Review: #503Head SHA: fdf155e SummaryThis is a well-architected migration from PAT-based dispatch to OIDC token minting via GCP Cloud Functions. The security model is sound: defense-in-depth with pre-validation + STS exchange, job_workflow_ref pinning, role-based permission downscoping, PEM zeroing, and proper token masking. The code demonstrates thorough input validation, bounded response body drains, and safe handling of secrets throughout. The migration path preserves backward compatibility via BothModesDispatchLayer. Findings are limited to medium and low severity observations — no critical or high issues were identified. FindingsMedium
Low
Info
FooterOutcome: comment-only Previous run (6)Review: #503Head SHA: 55f39ea SummaryThis is a large, well-structured migration from PAT-based dispatch to OIDC-based dispatch via a centralized token mint Cloud Function. The architecture is sound: OIDC tokens replace long-lived PATs, role-based permission downscoping enforces least privilege per agent role, WIF CEL conditions enforce org isolation, and the migration path correctly handles stale PAT cleanup. The code demonstrates strong security practices including bounded error-path response reads, PEM zeroing, token masking, and script injection prevention via However, three high-severity findings require resolution: (1) the collapse of separate read-only/write tokens into a single role-scoped token removes defense-in-depth within agent execution, (2) the Cloud Run invoker is set to FindingsHigh
Medium
Low
Info
FooterOutcome: request-changes Previous run (7)Review: #503Head SHA: 37aa6dc SummaryThis is a well-executed migration from PAT-based dispatch to OIDC token minting with a centralized Cloud Function. The security architecture is sound: defense-in-depth with pre-validation + STS exchange, proper input validation, bounded reads, PEM zeroing, and consistent script-injection prevention in workflows. No critical findings. Several medium findings warrant attention — notably incomplete key material zeroing, the FindingsCriticalNone. HighNone. Medium
Low
Info
FooterOutcome: comment-only Previous run (8)Review: automated reviewOutcome: failure The review agent reviewed commit Previous run (9)Review: #503Head SHA: c8562f7 SummaryThis is a well-executed migration from PAT-based dispatch to OIDC token minting via GCP Cloud Functions. The security architecture is sound: defense-in-depth OIDC validation (pre-validation + STS), role-based permission downscoping, PEM zeroing, script injection prevention in workflows, and proper credential masking. PAT deprecation is cleanly enforced in config validation, and the BothModes uninstall layer correctly handles migration cleanup. No critical or high findings were identified. The medium findings below are defense-in-depth improvements — none represent exploitable vulnerabilities in the current deployment context. No prompt injection patterns or suspicious non-rendering Unicode detected in the PR body or commit messages. FindingsMedium
Low
Info
FooterOutcome: comment-only Previous run (10)Review: #503Head SHA: d825fd6 SummaryThis is a substantial architectural change that deprecates PAT-based dispatch in favor of OIDC token minting via a GCP Cloud Function. The security design is sound overall: OIDC validation is layered (pre-validation + authoritative STS), attacker-controlled values in workflow YAML are consistently routed through FindingsHighNo findings. CriticalNo findings. Medium
Low
Info
FooterOutcome: comment-only |
- Add GCF v2 entry point registration with functions-framework-go - Add SecretAccessor implementation using metadata server auth - Add source upload via generateUploadUrl API before CreateFunction - Scope installation token to actions:write only (least privilege) - Add 10-minute timeout to WaitForOperation polling - Add 403 handling to OrgVariableExists matching OrgSecretExists Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
force-pushed
the
oidc-gcf-dispatch
branch
from
984e516 to
7f92e1c
Compare
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
| @@ -0,0 +1,612 @@ | |||
| // Package gcf implements the dispatch.Dispatcher interface using a GCP | |||
There was a problem hiding this comment.
perhaps this file should be renamed
There was a problem hiding this comment.
Renamed gcf.go → provisioner.go (and gcf_test.go → provisioner_test.go). The file contains the Provisioner type and mint infrastructure setup — the name should describe what it does, not the GCP product.
There was a problem hiding this comment.
Done in baf935a — renamed gcf.go → provisioner.go and gcf_test.go → provisioner_test.go.
| --mint-project "$GCP_PROJECT" | ||
| ``` | ||
|
|
||
| `--mint-project` specifies the GCP project where the OIDC token mint Cloud Function is deployed. It can be the same project as `--gcp-project` or a separate project. The installer automatically provisions a Cloud Function, WIF pool (`fullsend-pool`), WIF provider (`github-oidc`), service account (`fullsend-dispatch`), and Secret Manager secrets in the mint project. |
There was a problem hiding this comment.
do we still need a service account for dispatching when using the mint?
There was a problem hiding this comment.
No — the admin does not need to create a service account for dispatching. The fullsend-dispatch SA mentioned on this line is auto-provisioned by the installer (step 2 in provisionSelfManaged). It serves as the Cloud Function's runtime identity so it can read GitHub App PEM keys from Secret Manager.
The manual SA setup in Section 1 (steps 1a–1c) is for Vertex AI inference, not dispatching.
I'll clarify the wording here to make it obvious that the SA is auto-provisioned infrastructure, not something the admin creates.
| gcpProjectNum: os.Getenv("GCP_PROJECT_NUMBER"), | ||
| wifPoolName: os.Getenv("WIF_POOL_NAME"), | ||
| wifProviderName: os.Getenv("WIF_PROVIDER_NAME"), | ||
| }, |
There was a problem hiding this comment.
Might as well pass the rest of the Env-var based values here rather then have additional Getenv calls in NewHandler.
That way all the configuration inputs are in one place and we can easily decide to add e.g. commad line parameters
There was a problem hiding this comment.
Agreed — NewHandler currently reads OIDC_AUDIENCE, ROLE_APP_IDS, ALLOWED_ORGS, ALLOWED_ROLES, and ALLOWED_WORKFLOW_FILES via os.Getenv internally, while init() reads the WIF/STS values and passes them explicitly. Moving all env var reads into init() and passing them as explicit parameters (or a config struct) to NewHandler would make the configuration flow visible in one place and simplify testing.
Deferring to a follow-up to keep this PR focused — the current code is functionally correct and tests use t.Setenv + TestMain defaults.
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
Add GCP client abstraction, Cloud Function token mint, GCF provisioner with WIF/STS integration, dispatch interface, and forge extensions for org variable and repo variable management. The mint validates GitHub OIDC JWTs via Workload Identity Federation and issues scoped GitHub App installation tokens per agent role. Signed-off-by: Wayne Sun <gsun@redhat.com>
Wire the GCF provisioner into the admin install/uninstall flow, add --mint-url and --mint-project CLI flags, update config to store OIDC dispatch settings, and migrate layers to manage org/repo variables instead of PAT secrets. Remove legacy PAT e2e helpers. Signed-off-by: Wayne Sun <gsun@redhat.com>
Replace PAT-based dispatch in all agent workflows with OIDC token minting via the shared mint-token action. Convert enrolled-repo shim templates from workflow_dispatch to workflow_call. Add dispatch-prioritize job, port auto-triage on issue open/edit, and add lint-workflow-size caps. Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
Update architecture overview and installation guide to reflect the OIDC mint dispatch model replacing PAT-based dispatch. Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
Review: #503
Head SHA: 75be54df839f7ccdac0f2fa05c530e8d297ce034
Strategic Assessment
This PR is a strong security improvement — replacing long-lived PATs with OIDC-minted, role-scoped, short-lived installation tokens directly addresses the project's threat model. The architecture is sound (defense-in-depth with pre-validation + STS, role-based permission downscoping, proper workflow injection prevention), and the ~1800 lines of PAT code removal meaningfully reduces attack surface. The implementation has been through 3+ review rounds with 70+ inline comments, most resolved. Approve with deferred notes below.
Deferred Notes
Six moderate/minor findings noted inline for future consideration — none blocking merge. Several overlap with items already tracked in #741.
| startsWith(github.event.comment.body, '/review ') || | ||
| startsWith(github.event.comment.body, format('{0}{1}', '/review', fromJSON('"\n"'))) | ||
| )) || | ||
| (github.event_name == 'pull_request_target' |
There was a problem hiding this comment.
[moderate — note, defer] dispatch-review and dispatch-retro fire on pull_request_target without the fork guard that dispatch-fix-bot correctly uses (github.event.pull_request.head.repo.full_name == github.event.pull_request.base.repo.full_name). This means fork PRs trigger review/retro agents, which mint OIDC tokens scoped to the target repo for untrusted fork contributions. While pull_request_target runs the base branch workflow (so the shim itself is safe), the downstream dispatch still mints tokens.
If reviewing fork PRs is intentional, add a comment explaining why. Otherwise, add the fork guard to both jobs.
There was a problem hiding this comment.
Tracked in #741 — fork guard for dispatch-review/dispatch-retro.
|
|
||
| // generateAppJWT creates a signed JWT for GitHub App authentication. | ||
| func generateAppJWT(appID string, pemData []byte) (string, error) { | ||
| block, _ := pem.Decode(pemData) |
There was a problem hiding this comment.
[moderate — note, defer] pemData is zeroed by the caller's defer, but pem.Decode(pemData) creates block.Bytes (sub-slice or copy) and x509.ParsePKCS1PrivateKey creates an *rsa.PrivateKey with internal big.Int values — neither is zeroed. Consider zeroing block.Bytes after parsing. Go's GC makes full memory scrubbing of the parsed RSA key impractical, but documenting this limitation would help.
There was a problem hiding this comment.
Tracked in #741 — PEM memory zeroing for block.Bytes and RSA key internals.
|
|
||
|
|
||
| // rolePattern restricts role to safe lowercase identifiers. | ||
| var rolePattern = regexp.MustCompile(`^[a-z][a-z0-9_-]*$`) |
There was a problem hiding this comment.
[minor — note, defer] rolePattern (^[a-z][a-z0-9_-]*$) is defined independently here and in internal/dispatch/gcf/provisioner.go. Divergence risk if one is updated without the other. Consider extracting to a shared package.
There was a problem hiding this comment.
Tracked in #741 — extract shared rolePattern to avoid divergence.
| // rolePermissions defines the minimum GitHub App permissions per agent role. | ||
| // Tokens are always downscoped to these permissions regardless of what the | ||
| // App itself has configured. | ||
| var rolePermissions = map[string]map[string]string{ |
There was a problem hiding this comment.
[minor — note, defer] retro and prioritize roles are defined in rolePermissions but excluded from test defaults in testmain_test.go (ALLOWED_ROLES=triage,coder,review,fix,fullsend). These permission maps have zero test coverage — typos or wrong access levels would not be caught.
There was a problem hiding this comment.
Tracked in #741 — add retro and prioritize to test defaults for permission map coverage.
|
|
||
| // adcToken obtains a token via Application Default Credentials. | ||
| func (c *Client) adcToken(ctx context.Context) (string, error) { | ||
| creds, err := google.FindDefaultCredentials(ctx, "https://www.googleapis.com/auth/cloud-platform") |
There was a problem hiding this comment.
[minor — note, defer] adcToken calls google.FindDefaultCredentials on every API request. This re-reads environment variables, files, and the metadata server each time. The TokenSource from FindDefaultCredentials already handles token refresh internally — caching the creds at construction time or on first use would avoid repeated credential discovery.
There was a problem hiding this comment.
Tracked in #741 — cache FindDefaultCredentials result at construction time.
| // audience handles the OIDC aud claim which can be a string or array of strings. | ||
| type audience []string | ||
|
|
||
| func (a *audience) UnmarshalJSON(data []byte) error { |
There was a problem hiding this comment.
[minor — note, defer] audience.UnmarshalJSON accepts an empty string "" silently, producing audience{""}. The empty-array check only applies to the array branch. Consider adding if s == "" { return fmt.Errorf("aud must not be empty") } in the single-string branch for defense-in-depth.
There was a problem hiding this comment.
Tracked in #741 — validate against empty string in audience.UnmarshalJSON single-string branch.
…isioning
The GCF provisioner and mint function now support multiple orgs sharing
a single GCP project. Secrets use org-scoped names (fullsend-{org}--{role}-app-pem),
ROLE_APP_IDS uses org-scoped keys ({org}/{role}), and WIF attribute conditions
are merged additively. ALLOWED_ROLES is derived from ROLE_APP_IDS keys.
Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
google-github-actions/auth@v3 uses the IAM-format audience URI
for STS token exchange, not just the custom "fullsend-mint" audience.
Without both audiences registered on the WIF provider, token exchange
fails at runtime.
Register both audiences on create and update: the custom OIDC audience
("fullsend-mint") and the IAM audience
("https://iam.googleapis.com/projects/{num}/.../providers/{id}").
UpdateWIFProvider now patches oidc.allowedAudiences alongside
attributeCondition. GetWIFProvider now returns AllowedAudiences.
Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
- Preserve installing orgs before WIF merge to prevent PEM/env-var
overwrites of existing orgs' secrets during additive provisioning
- Pass e2eDispatcher to BothModesDispatchLayer in E2E uninstall so
OIDC variables are actually cleaned up (nil dispatcher was a no-op)
- Add FULLSEND_MINT_URL assertion in verifyNotInstalled
- Add fullsend-{role} slug convention to E2E cleanup
- Enable WithOIDCMode on E2E install SecretsLayer
- Add githubOrgPattern validation in smPEMAccessor.AccessPEM
- Fix stale PEM naming in docs and comments to match org-scoped format
Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
… cap
Defense-in-depth: the "--" separator in secret names
(fullsend-{org}--{role}-app-pem) requires that neither org nor role
contain "--". Add explicit strings.Contains checks in both mint
AccessPEM and provisioner validation. Align provisioner githubOrgPattern
with mint's 39-char length cap. Add non-empty assertion to multi-org
merge test. Remove dead isStale condition in E2E cleanup.
Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
4 participants
Summary
dispatch.mode: "pat"is now rejected; OIDC mint is the sole dispatch mechanismfullsend admin install— staleFULLSEND_DISPATCH_TOKENsecret is auto-cleaned, enrolled repo shims are updated viareconcile-repos.shadmin installfullsend-{org}--{role}-app-pemwith double-hyphen separator (GitHub org names cannot contain--). Each org gets its own PEM per role, enabling both shared public apps and per-org private apps.fullsend-mint(custom OIDC audience) and the IAM-format audience (https://iam.googleapis.com/projects/{num}/.../providers/{id}) required bygoogle-github-actions/auth@v3for STS token exchange--publicflag:admin install --publiccreates public unlisted GitHub Apps installable by additional orgs without re-creating the app.fullsendis always created as a public repo. Cross-repoworkflow_callonly works reliably when the called repo is public, across all GitHub plan tiers (free, team, enterprise). If an admin later makes it private, only private repos can trigger workflows; public and internal repos fail silently. The installer warns on both creation and detection of a private config repo.--dispatch-*flags renamed to--mint-*(--mint-provider,--mint-project,--mint-region,--mint-source-dir) to reflect what they configureMulti-org: one GCP project, many GitHub orgs
A single GCP project hosts the mint infrastructure for multiple GitHub orgs. Each
admin installmerges its org into the existing deployment:fullsend-pool)github-oidc)attributeConditionmerged additively (single-org==→ multi-orgin [...]).oidc.allowedAudiencesupdated to include bothfullsend-mintand the IAM audience. On 409, falls through to PATCH both fields.fullsend-dispatch)fullsend-{org}--{role}-app-pem)secretAccessoron each secret individually.fullsend-mint)ALLOWED_ORGSunioned,ROLE_APP_IDSmerged (org-scoped keys:{org}/{role}→ app ID).ALLOWED_ROLESderived from merged keys. Redeployed only when env vars or source hash change.What happens when org B installs after org A?
GetWIFProviderreads existing condition (== 'orgA') and audiences['orgA', 'orgB']CreateWIFProvider→ 409 →UpdateWIFProviderPATCHes both condition (in ['orgA', 'orgB']) and audiencesfullsend-orgB--{role}-app-pem(orgA's PEMs untouched)GetFunctionreads existing env vars →mergeAllowedOrgsunions org lists →mergeRoleAppIDsmerges app ID maps → deploy if changedKey separator:
--(double hyphen)GitHub org names allow single hyphens but not consecutive hyphens (
--). Role names are fullsend-defined (triage,coder, etc.) and don't use--. This makesfullsend-{org}--{role}-app-pemunambiguously parseable.Deployment profiles
Three profiles from an org admin's perspective:
fullsend-{org}--{role}-app-pem)admin installper org — PEMs stored org-scoped, WIF/env merged additivelyfullsend-{org}--{role}-app-pem)admin install --mint-url— stores org-scoped PEMs in shared project, skips infra deployadmin install --mint-url— no GCP project neededBundled onboarding with public apps still requires PEM storage (one per org×role), since each org's installation has a unique PEM even when sharing the same app.
PAT → OIDC migration
What happens to existing PAT orgs?
fullsend admin installoidc-mint, deletes staleFULLSEND_DISPATCH_TOKENsecret, provisions OIDC mint infra. Logs "migrating to OIDC mint" when stale PAT detected.fullsend admin uninstallBothModesDispatchLayercleans both PAT secrets and OIDC variables regardless of config.fullsend admin analyzereconcile-repos.shdetects stale PAT shim content, opens update PRs to replace withworkflow_callshim. Same filename (.github/workflows/fullsend.yaml), no orphaned files.What was removed
templates/shim-workflow.yaml(469-line PAT shim) — deletedNewDispatchTokenLayer,installPAT,createSecret,PromptTokenFunc,promptDispatchToken(~700 lines)workflow_dispatchtrigger fromdispatch.yml— onlyworkflow_callremains--dispatch-mode=patacceptance — replaced by--mint-provider(defaultgcf)createDispatchPAT,deleteDispatchPAT,deleteAllDispatchPATs(~590 lines)What was kept (for migration cleanup)
uninstallPAT/analyzePAT— used byNewBothModesDispatchLayerto detect and clean stale PAT secrets during uninstallinstallOIDC— auto-deletesFULLSEND_DISPATCH_TOKENwhen migrating, logs warning if check failsArchitecture
Token flow (OIDC only)
--repotargetjob.workflow_repository=.fullsend$DISPATCH_REPO=.fullsendvars.FULLSEND_MINT_URL(org variable) stores the mint endpoint. Both the OIDC JWT and minted installation token are masked with::add-mask::.Secret naming convention
fullsend-{org}--{role}-app-pemfullsend-acme--coder-app-pem{org}/{role}acme/coderROLE_APP_IDSenv var (JSON map → app ID)Org-scoped naming means each org gets its own PEM per role. The mint's
AccessPEM(ctx, org, role)derives the org from the validated JWT'srepository_ownerclaim — never from user input.What's new
Token mint (
internal/mint/)triage,coder,review,fix,fullsend)job_workflow_refclaim pins requests to.fullsendworkflow filesrepository_ownerclaim (validated againstALLOWED_ORGSenv var), not hardcodedPEMAccessor.AccessPEM(ctx, org, role)— org from validated JWT, secrets stored asfullsend-{org}--{role}-app-pemio.Copy(io.Discard, resp.Body)calls useio.LimitReader(resp.Body, 4096)GCP provisioner (
internal/dispatch/gcf/)gcf.go: Provisions WIF pool/provider, service account, Secret Manager secrets, Cloud Function deploymentgcp.go: GCP REST API client for all infrastructure operations--mint-url): stores PEMs in existing mint's Secret Manager, skips deploymentConfig.GitHubOrgs []stringwith per-org validation, lowercase normalization, and deduplicationmergeAllowedOrgsunions org lists,mergeRoleAppIDsmerges org-scoped app ID maps,deriveAllowedRolesextracts unique roles from merged keysGetWIFProvider, merges org lists, creates or updates with merged condition + dual audiencesfullsend-mintandhttps://iam.googleapis.com/projects/{num}/.../providers/{id}(required bygoogle-github-actions/auth@v3for STS exchange)gcpProjectIDPatternfor project IDs,gcpRegionPatternfor regionsSetSecretIAMBindingretries on 409 Conflict with escalating backoff (read-modify-write race protection)bundleFunctionSourcecalled once during validation and reused at deploy time (eliminates TOCTOU window and redundant I/O)App setup (
internal/appsetup/)--publicflag:AppManifestsupportspublic: truefor creating public unlisted GitHub AppsAppSlug(role)returnsfullsend-{role}(no org prefix)Dispatch workflow (
dispatch.yml)workflow_callonly:secrets,vars, andgithub.repositoryresolve from the caller — dispatch.yml uses OIDC mint for its own tokensecrets: {}in shims is correct by designcurl --retry 3 --retry-delay 2 --retry-all-errorson both OIDC token and mint requestsShim workflow (
templates/shim-workflow-call.yaml)workflow_callshim — enrolled repos call dispatch.yml directly withsecrets: {}id-token: writefor OIDC flowInfrastructure layers
internal/layers/configrepo.go: Config repo always created as public; warns when existing repo is private (workflow_call will fail for non-private callers)internal/layers/dispatch.go: OIDC mode wiring, stale PAT migration logging with error visibilityinternal/config/config.go:oidc-mintis the only accepted dispatch modeinternal/cli/admin.go:--mint-provider(defaultgcf),--mint-project,--mint-region,--mint-source-dir,--mint-url,--publicflagsinternal/forge/: Org-level variable CRUD supportinternal/gcp/: Shared GCP ADC + HTTP clientSecurity model
.fullsend— PEMs stored in Secret Manager, tokens minted at runtimejob_workflow_refauthorization — only.fullsendworkflow files can request tokensrepository_ownercondition — cross-org token requests rejected at the GCP levelALLOWED_ORGS, (2) WIF STS exchange with CEL attribute condition onrepository_ownerrepository_owneris cryptographically signed, not user-suppliedfullsend-{org}--{role}-app-pem. The org inAccessPEMis derived from the validated JWT, not from user input. Even if an attacker could influence the org parameter, Secret Manager path resolution would reject traversal attempts.env:+jq --arghead.repo == base.repobefore dispatchingdeferafter use::add-mask::Test plan
go test ./internal/...— all unit tests pass (19 packages)go vet ./internal/...— cleango vet -tags e2e ./e2e/...— e2e tests compile cleanfullsend-mintand IAM audience registered on create and updatedispatch.mode: "pat"FULLSEND_DISPATCH_TOKEN, logs migration messagesecretID(org, role)returnsfullsend-{org}--{role}-app-pem,AccessPEM(ctx, org, role)signature verifiedAppSlug(role)returnsfullsend-{role}without org prefixfullsend admin install --mint-project=... nonflux— 5 existingnonflux-*apps reused vialoadKnownSlugs, PEMs stored with org-scoped naming, mint redeployed, enrollment succeeded. Found and fixed 2 bugs: Secret Managerreplication.auto→replication.automatic(API 400), coder/review manifestissues:read→issues:write(permission mismatch with installed apps).fullsend admin install --public --mint-project=... myorgfor multi-org