Add inference provider credential provisioning (Vertex AI) - #215
Conversation
waynesun09
left a comment
There was a problem hiding this comment.
Flagging a security concern around raw GCP API error body leakage — see inline comment on gcp.go. The same pattern appears at lines 76, 98, and 116.
waynesun09
left a comment
There was a problem hiding this comment.
GOOGLE_APPLICATION_CREDENTIALS secret naming creates a consumption trap
Severity: Medium — not broken today, predictable bug later
The Vertex provider stores raw SA key JSON in a GitHub secret named GOOGLE_APPLICATION_CREDENTIALS:
// internal/inference/vertex/vertex.go
SecretCredentials = "GOOGLE_APPLICATION_CREDENTIALS"This name collides with the well-known GCP SDK environment variable of the same name, but they expect different value types:
| Namespace | What GOOGLE_APPLICATION_CREDENTIALS holds |
|---|---|
| GitHub Actions secret (what this PR creates) | Raw JSON content ({"type":"service_account",...}) |
| GCP SDK env var (what the runtime needs) | A file path like /tmp/creds.json |
Why this matters
The agent dispatch workflow is currently a stub, so nothing breaks today. But when someone writes the real consumption step, the identical name in both namespaces invites a natural one-liner:
env:
GOOGLE_APPLICATION_CREDENTIALS: ${{ secrets.GOOGLE_APPLICATION_CREDENTIALS }}The GCP SDK will try to open a file at path {"type":"service_account",...} → silent auth failure.
The correct pattern requires an intermediate write-to-file step:
- run: |
echo "$GCP_CREDS" > /tmp/gcp_credentials.json
echo "GOOGLE_APPLICATION_CREDENTIALS=/tmp/gcp_credentials.json" >> "$GITHUB_ENV"
env:
GCP_CREDS: ${{ secrets.GOOGLE_APPLICATION_CREDENTIALS }}Existing repo precedent
The experiments directory already uses a clearer name that avoids this confusion:
# experiments/agent-scoped-tools-triage/README.md
echo "$GCP_SA_KEY_JSON" > /tmp/gcp_credentials.json
echo "GOOGLE_APPLICATION_CREDENTIALS=/tmp/gcp_credentials.json" >> "$GITHUB_ENV"Recommendation
Rename the secret constant to something that signals "this is JSON content, not a file path":
SecretCredentials = "FULLSEND_GCP_SA_KEY_JSON"This follows the FULLSEND_ prefix convention already used for other secrets (FULLSEND_DISPATCH_TOKEN, FULLSEND_<ROLE>_APP_PRIVATE_KEY) and makes the write-to-file step obvious to the workflow author.
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review: 5 High-severity issues requiring changes
Five high-severity issues identified across three independent review agents (security, architecture, Gemini-style). All five are in new code introduced by this PR.
Summary of findings:
| # | File | Issue |
|---|---|---|
| 1 | gcp.go:21-33 |
PATH-hijackable gcloud subprocess — use GCP Go SDK instead |
| 2 | gcp.go:46-62 |
No HTTP client timeout — http.DefaultClient blocks indefinitely |
| 3 | gcp.go:66-79 |
URL injection — unescaped projectID/saName in API URL paths |
| 4 | vertex.go:78-100 |
SA key accumulation — every Provision() creates a new key (10 key limit), violating ADR 0006 idempotency |
| 5 | admin.go:118-137 |
--gcp-service-account / --gcp-credentials-file silently ignored without --gcp-project |
See also prior review comments on this PR regarding GCP API error body leaks and GOOGLE_APPLICATION_CREDENTIALS secret naming.
Full review identified 38 total issues (5 High, 16 Medium, 17 Low) across security, correctness, architecture, and code quality. The inline comments above cover the 5 High items that should be addressed before merge.
waynesun09
left a comment
There was a problem hiding this comment.
Follow-up from Gemini 3.1 Pro review: config overwrite on re-install is a data loss bug. See inline comment.
waynesun09
left a comment
There was a problem hiding this comment.
Verified the implementation against all 7 high-severity issues from the multi-agent review. All are resolved:
-
Raw GCP API error body leakage (gcp.go:114) —
extractGCPErrorMessage()now parses onlyerror.messagefrom GCP responses across all three methods. Raw body no longer leaked. -
PATH-hijackable
gcloudsubprocess (gcp.go:46) —accessToken()now usesgolang.org/x/oauth2/google.FindDefaultCredentials(). No subprocess, no$PATHdependency. -
No HTTP client timeout /
http.DefaultClient(gcp.go:92) —NewLiveGCPClient()creates a dedicated&http.Client{Timeout: 30 * time.Second}. Global client no longer used. -
URL injection via unescaped path parameters (gcp.go) — Input validated with
gcpIDPatternandsaEmailPatternregexes, and all path parameters go throughurl.PathEscape(). -
SA key accumulation / idempotency contract (vertex.go:106) —
InferenceLayer.Install()checks whether all expected secrets already exist in GitHub before callingProvision(). If present, provisioning is skipped and no new GCP SA key is created. -
--gcp-service-accountsilently ignored without--gcp-project(admin.go:145) — Explicit early error when--gcp-service-accountor--gcp-credentials-fileare set without--gcp-project. -
Re-install without
--gcp-projectsilently erases inference config (admin.go:141) —loadExistingInferenceProvider()reads the existinginference.providerfromconfig.yamlin.fullsendbefore building the layer stack, preserving the existing config when no GCP flags are provided.
Per ADR convention, leaving resolution of the inline threads to the PR owner.
waynesun09
left a comment
There was a problem hiding this comment.
Review: 5 Medium-Severity Findings
All 7 previously reported high-severity issues have been resolved — nice work. Five medium-severity items remain across naming convention, input validation, code safety, test coverage, and credential handling. See inline comments.
waynesun09
left a comment
There was a problem hiding this comment.
Additional: 5 Low-Severity Findings (non-blocking)
waynesun09
left a comment
There was a problem hiding this comment.
All 5 medium-severity and 5 low-severity findings from the previous review round are resolved (or explicitly declined by the PR owner for the type naming item).
Two independent review agents (Gemini code review + security-focused review) found no critical or high-severity issues in the latest commit. Remaining observations are non-blocking:
- TOCTOU race in idempotency check (Medium) — acceptable for a manual admin CLI tool
- Credential string copies survive zeroing (Medium) — Go language limitation, CLI exits after use
- Project ID not validated in Mode 3 (Medium) — could cause confusing runtime failures but not a security issue; consider adding
gcpIDPatternvalidation before the Mode 3 early return in a follow-up
Security posture is solid: ADC replaces gcloud subprocess, URL path parameters escaped and validated, GCP error messages sanitized, io.LimitReader bounded, credential file validated and symlink-protected, memory zeroed where possible. Supply chain additions (golang.org/x/oauth2, cloud.google.com/go/compute/metadata) are official Google/Go libraries.
Test coverage is thorough: 28 unit tests + e2e across all 3 provisioning modes, error cases, nil client, idempotency, and 409 Conflict handling.
Adds an InferenceLayer to the install stack that provisions and stores inference provider credentials as repo secrets on .fullsend. Supports three modes: create SA + key, verify existing SA + key, or use a pre-made key directly. Coded behind an abstract inference.Provider interface to support future providers. Closes #152 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace gcloud subprocess with Go SDK (golang.org/x/oauth2/google) to eliminate PATH-hijack risk - Add dedicated HTTP client with 30s timeout instead of http.DefaultClient - Escape user-supplied URL path parameters and validate GCP naming rules - Extract only error.message from GCP API responses to prevent metadata leakage - Rename secret from GOOGLE_APPLICATION_CREDENTIALS to FULLSEND_GCP_SA_KEY_JSON to avoid collision with the GCP SDK env var (which expects a file path) - Add idempotency check: skip provisioning if secrets already exist, preventing SA key accumulation against GCP's 10-key limit - Error if --gcp-service-account or --gcp-credentials-file set without --gcp-project instead of silently ignoring them - Preserve existing inference config on re-install when GCP flags omitted Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Guard against nil gcpAPI in Provision() for modes 1/2, returning a clear error instead of panicking if called without a GCP client - Add saEmailPattern validation in CreateServiceAccountKey to prevent URL injection via crafted email parameters - Add tests for nil GCP client in all three modes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rename GCP_PROJECT_ID to FULLSEND_GCP_PROJECT_ID for prefix consistency - Validate credential file JSON structure before upload - Add NewAnalyzeOnly() constructor to replace fragile sentinel - Add 409 Conflict idempotency test for SA creation - Use []byte for credential JSON and zero after use - Add symlink protection on credential file path - Use json.Marshal instead of fmt.Sprintf for SA payload - Deduplicate runAnalyze by reusing loadExistingInferenceProvider - Bound HTTP response body reads with io.LimitReader Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ec3d4e1 to
f30c25e
Compare
Site previewPreview: https://7ba5f381-site.fullsend-ai.workers.dev Commit: |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
InferenceLayerto the install stack (position 4: after secrets, before dispatch-token) that provisions inference provider credentials as repo-level secrets on.fullsendinference.Providerinterface ininternal/inference/for future provider supportinference: { provider: vertex }config section toconfig.yaml--gcp-project,--gcp-service-account,--gcp-credentials-fileTest plan
internal/inference/vertex/(all 3 modes + error cases)internal/layers/inference.go(install, analyze, nil provider, errors)internal/config/(inference validation, parse, marshal)E2E_HALFSEND_VERTEX_KEYenv var (mode 3) when available, skip gracefully otherwiseE2E_HALFSEND_VERTEX_KEYinto CI workflow to enable e2e inference testingCloses #152
🤖 Generated with Claude Code