Skip to content

feat: deliver secrets as env vars instead of inlining in YAML#25

Merged
TerrifiedBug merged 7 commits intomainfrom
feat/env-var-secret-delivery
Mar 7, 2026
Merged

feat: deliver secrets as env vars instead of inlining in YAML#25
TerrifiedBug merged 7 commits intomainfrom
feat/env-var-secret-delivery

Conversation

@TerrifiedBug
Copy link
Copy Markdown
Owner

Summary

  • Replace resolveSecretRefs (which inlined decrypted credentials into pipeline YAML) with convertSecretRefsToEnvVars that converts SECRET[name]"${VF_SECRET_NAME}" env var placeholders
  • Vector interpolates credentials from environment variables set by the Go agent — no plaintext in YAML configs on disk or in transit
  • Secret names are normalized via secretNameToEnvVar() (e.g. db-passwordVF_SECRET_DB_PASSWORD) on both placeholder and delivery sides
  • Secrets included in config checksum so rotation triggers agent restart
  • Collision detection warns when different secret names normalize to the same env var key
  • Secrets query hoisted outside pipeline loop (single DB call per request)
  • Deterministic ordering (orderBy: name asc) for consistent collision resolution
  • YAML strings force-quoted with double quotes to prevent Vector's parser from choking on ${...} flow indicators

Test plan

  • Deploy a pipeline with SECRET[name] references in sensitive fields
  • Verify YAML contains "${VF_SECRET_NAME}" placeholders, not real values
  • Verify secrets dict in API response contains decrypted values with normalized keys
  • Verify Vector starts successfully and interpolates env vars
  • Rotate a secret and confirm agent detects the checksum change

TerrifiedBug and others added 7 commits March 6, 2026 17:35
Secret names with hyphens or dots (e.g. "db-password") produced
invalid env var names like ${VF_SECRET_db-password}. Now normalized
to uppercase with underscores: ${VF_SECRET_DB_PASSWORD}.

Both the YAML placeholder and secrets dict key use the same
secretNameToEnvVar function to ensure they always match.
- Include secrets dict in checksum computation so secret rotation
  triggers agent restart (previously, YAML-only checksum wouldn't
  change when env var placeholders stayed the same)
- Log warning when secret name normalization produces a collision
  (e.g. "db-password" and "db_password" both → VF_SECRET_DB_PASSWORD)
…ordering

Move the secrets findMany + decrypt loop above the pipeline iteration so
it runs once per request instead of once per pipeline. Add orderBy name
asc to ensure deterministic collision resolution when multiple secret
names normalize to the same env var key.
Vector's YAML parser (yaml-rust) rejects unquoted ${VF_SECRET_*}
placeholders because { is a flow indicator character. Force double-
quoting all string values in the YAML dump so the output produces
password: "${VF_SECRET_NAME}" instead of password: ${VF_SECRET_NAME}.
Vector interpolates env vars from parsed string values, so quoting
does not affect interpolation.
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 7, 2026

Greptile Summary

This PR implements the secret-delivery redesign for the agent config endpoint. The key change is adding forceQuotes: true, quotingType: '"' to yaml.dump() at line 103 so that ${VF_SECRET_NAME} placeholders survive Vector's YAML parser.

Key findings:

  • Decrypted secrets in API response (lines 120–122): The endpoint includes all environment secrets in plaintext in every config response. While this is intentional and required for env-var injection, it is flagged by the security review policy. The response body must be treated as sensitive — HTTPS-only transport and response logging suppression are critical mitigations. Consider documenting this design decision inline to prevent accidental removal in future reviews.

  • All-environment secrets in every pipeline's checksum (lines 108–110): The checksum includes ALL environment secrets regardless of whether a pipeline references any of them. This creates a broad restart blast radius — rotating any secret triggers all pipelines on all nodes to update their checksums, even unrelated pipelines. A tighter design would include only the secret keys each pipeline actually uses.

The broader secret-delivery infrastructure (pre-resolving secrets, collision detection, env-var placeholders, normalized naming) appears solid and is already landed.

Confidence Score: 3/5

  • Safe to merge functionally, but the intentional inclusion of decrypted secrets in HTTP responses requires explicit security controls (HTTPS-only, response logging suppression) and the all-environment checksum design should be reviewed for intentionality.
  • The yaml.dump change is correct and the broader secret-delivery system is solid. However, two design decisions warrant explicit sign-off: (1) decrypted secrets appearing in API response bodies, which is flagged by the security policy despite being intentional, and (2) all environment secrets bundled into every pipeline's checksum, causing a broader restart scope than strictly necessary. Neither is a runtime bug, but both represent security and operational trade-offs that should be consciously accepted.
  • src/app/api/agent/config/route.ts (lines 108–122: checksum scope and secrets response payload)

Comments Outside Diff (2)

  1. src/app/api/agent/config/route.ts, line 120-122 (link)

    Decrypted secret values are transmitted in plaintext in the API response body (lines 120–122). While this is intentional by design—the agent needs plaintext values to inject them as env vars for Vector—the security review policy flags this pattern. Ensure these risks are understood and accepted:

    1. Transport security: This endpoint must always be served over HTTPS. If an agent connects over plain HTTP, all decrypted secrets leak in the response body.
    2. Response logging: Any middleware, proxy, or Next.js request logger capturing response bodies will log all decrypted secret values.
    3. Secrets scoped to environment, not pipeline: Every response includes ALL environment secrets, not just those referenced by pipelines on this node. A compromised node receives secrets it has no functional need for.

    Consider adding an inline comment documenting this intentional design decision so future reviewers understand why decrypted secrets appear in the response and don't accidentally remove the secrets field.

    Rule Used: ## Security & Cryptography Review Rules

    When revi... (source)

  2. src/app/api/agent/config/route.ts, line 108-110 (link)

    The checksum at lines 108–110 includes ALL environment secrets—every secret in the environment is bundled into every pipeline's checksum, regardless of whether that specific pipeline references any of those secrets.

    This design has a broad blast radius: rotating any secret causes every pipeline on every node in the environment to restart. For example, rotating an unrelated database credential would trigger all pipelines to restart, even those that don't use that credential.

    A tighter approach would only include secret keys actually referenced by each pipeline's config YAML in its checksum. Since convertSecretRefsToEnvVars already knows which secrets were substituted, this would be straightforward to implement and would isolate secret rotation impact to only affected pipelines.

Last reviewed commit: 3337c08

@TerrifiedBug TerrifiedBug merged commit d231dbf into main Mar 7, 2026
10 checks passed
@TerrifiedBug TerrifiedBug deleted the feat/env-var-secret-delivery branch March 7, 2026 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant