security: an acquisition source got this host's ambient database credentials by fas89 · Pull Request #750 · Agenticstiger/forge-cli

fas89 · 2026-10-26T17:24:48Z

First of two PRs for the roadmap board's "[P1][Security][forge-cli] Acquisition secretRef env:// sends any environment variable to a host the contract names" (Trello 1025). You chose to ship this half on its own because no allowlist over secretRef can close it — the credential never passes through secretRef at all.

The defect

A contract names the host its postgres or mysql source lives on. libpq and mysql_scanner then read credentials from the process environment for whatever host they are handed — so a contract naming a host of its own choosing could be authenticated with this machine's PGPASSWORD, PGPASSFILE, PGSERVICE or MYSQL_PWD: a credential the contract was never given, belonging to a different server.

fluid init --discover has suppressed these since its JDBC introspection landed — 14 variables, plus a no-client-certificate path so libpq never presents this host's ~/.postgresql/postgresql.crt to a server that asks for one. The acquisition runners had not, so the same contract fields reached the same client libraries with the environment intact.

The duckdb runner's ATTACH now runs inside the same window. The declared DSN still carries its own credentials, so a legitimate source is unaffected and no contract change is needed.

Why the implementation moved

The obvious import was forbidden: import-linter enforces "build_runners must not depend on cli", so build_runners/duckdb/runner.py cannot import the helper from cli/discover/_jdbc_introspect.py.

It therefore moved to credentials/host_db_env.py and both sides import it. That beats a second copy — a duplicated credential-hiding window is exactly the kind of thing that drifts apart, and this one carries a carefully-worded docstring about what its lock does and does not guarantee.

cli/discover/_jdbc_introspect keeps _HOST_CREDENTIAL_ENV, _NO_CLIENT_CERT, _HOST_CREDENTIAL_ENV_LOCK and _host_credential_env_suppressed as aliases, so its own tests and the patch points around them are untouched. Verified they are the same objects, not duplicates — one RLock, which the serialisation actually depends on:

lock is the same object: True
env tuple is the same  : True  (14 vars)
ctx manager is the same: True

The lock's documented limits are inherited as they are, and the docstring stating them moved with it: it serialises windows opened through that function and nothing else, so a concurrent os.environ.copy() or an engine._run_sync_stage(env_snapshot=True) overlapping a window is still outside its guarantee.

Validation

202 existing jdbc attach-safety tests pass through the aliases, unchanged.
lint-imports: 6 contracts kept, 0 broken — run exactly as CI does, since the layering is the whole reason for the new module's location.
The new test observes the environment from inside the attach, which is the only place that proves the window wraps the call reaching libpq. Reverting the with fails it — mutation-checked.
A sqlite source is pinned as not taking this path, so the change did not quietly widen to a branch whose confinement is a different control.
ruff and CI-pinned black==24.10.0 clean whole-tree. Startup budget 6/6. ripwire --quality-delta gating 18 against base 16; the two extra are a self-churn artifact and one 26-token assert-shape collision.
Full suites on duckdb 1.5.6 and 2.0.0 were still running when this was pushed, with 0 failures at that point. CI runs the matrix properly; I will report if anything turns up.

What is NOT here

The env:// allowlist policy itself — option B (allowlist required, loopback exempt), the operator-only FLUID_ACQUISITION_SECRET_REF_ALLOWLIST, redactor registration for resolved values (#884), per-runner tests and the upgrade note — is the second PR. All six acquisition runners call resolve_connection_secrets, not the four the card lists: debezium and kafka_connect are also callers.

Documentation

No docs needed — this change does not affect user-facing behaviour

No new flag, config key, environment variable or CLI surface. A legitimate source, whose DSN carries its own credentials, behaves identically. The operator-facing note is in CHANGELOG.md under ## [Unreleased].

…entials A contract names the host its postgres or mysql source lives on. libpq and mysql_scanner read credentials from the PROCESS environment for whatever host they are handed, so a contract naming a host of its own choosing could be authenticated with this machine's PGPASSWORD, PGPASSFILE, PGSERVICE or MYSQL_PWD — a credential the contract was never given, belonging to a different server. `fluid init --discover` has suppressed these since its JDBC introspection landed (14 variables, plus a no-client-certificate path so libpq never presents this host's ~/.postgresql/postgresql.crt). The acquisition runners had not, so the same contract fields reached the same client libraries with the environment intact. The duckdb runner's ATTACH now runs inside the same window. This route is INDEPENDENT of the contract's own `connection.secretRef`, and no allowlist over `secretRef` would close it, because the credential never passes through `secretRef` at all. That is why it ships on its own rather than waiting for the acquisition secretRef policy (founder decision, 2026-10-26). The implementation is shared, not copied: it moved to `credentials/host_db_env.py` because `build_runners` may not import `cli` — the import-linter contract "build_runners must not depend on cli" would have refused the obvious import, and a second copy of a credential-hiding window is exactly the kind of thing that drifts. `cli/discover/_jdbc_introspect` keeps `_HOST_CREDENTIAL_ENV`, `_NO_CLIENT_CERT`, `_HOST_CREDENTIAL_ENV_LOCK` and `_host_credential_env_suppressed` as aliases of the shared names, so its own tests and the patch points around them are untouched. Verified they are the SAME objects, not duplicates — one RLock, as the serialisation requires. The lock's documented limits are inherited as they are, and the docstring that states them moved with it: it serialises windows opened through this function and nothing else, so a concurrent `os.environ.copy()` or an `engine._run_sync_stage(env_snapshot=True)` overlapping a window is still outside its guarantee. Verified: 202 existing jdbc attach-safety tests pass through the aliases; `lint-imports` reports 6 contracts kept, 0 broken; the new test observes the environment from INSIDE the attach, which is the only place that proves the window wraps the call reaching libpq, and reverting the `with` fails it.

Two review findings on #750, both in the new test file. The Lint & Format failure was its `Secret scan (detect-secrets on changed files)` step, not ruff or black: `assert seen["password"] == "from-the-contract"` read as a Secret Keyword. The dict entry it compares against carried a pragma but the assertion did not. Naming the value once as CONTRACT_PASSWORD removes the literal from both places, which beats sprinkling pragmas — and it says what the value IS: what the contract declared, as opposed to what the host environment holds, which is the distinction the whole test is about. CodeQL read the `raise` inside nested `pytest.raises` as making the rest of the block unreachable. Rewritten as try/except/else, which states the same intention without the warning. Not done: the two comments asking to delete the `_HOST_CREDENTIAL_ENV` and `_HOST_CREDENTIAL_ENV_LOCK` aliases as unused. They are the compatibility shim this PR exists to provide, and the first is used directly by tests/cli/discover/test_jdbc_attach_safety.py:480 and :487 — deleting it breaks 202 passing tests. The bot sees one file at a time and cannot see that use. Verified after the change: detect-secrets flags 0 in the file and the hook exits 0 over the changed-file list; 9 tests pass; reverting the `with` in the runner still fails the regression test, so the guard did not become tautological.

CodeQL was right about `_HOST_CREDENTIAL_ENV_LOCK`: checked each alias individually and it has 0 references, while `_HOST_CREDENTIAL_ENV` has 2 (its own tests) and `_NO_CLIENT_CERT` has 2 (build_attach_dsn in the same module). The lock stays reachable as `host_db_env.HOST_CREDENTIAL_ENV_LOCK` if anything ever needs it, and the comment now says which names are kept and why.

CodeQL flagged the `else: pytest.fail(...)` as unreachable, and statically it is -- the `raise` is unconditional. But it is reachable in the one case the test exists for: a context manager whose `__exit__` returns true SWALLOWS the exception, and CodeQL does not model that. Dropping the branch as suggested would leave a swallowing window passing this test with nothing asserted. Replaced with a `propagated` flag asserted after the `try`, which keeps the guard and gives the analyser nothing unreachable. Verified against a deliberately swallowing context manager: the flag stays false, so the assertion fails -- the guard still works.

… named (#751) * security: a contract could send any environment variable to a host it named `connection.secretRef: env://NAME` let the contract choose BOTH halves — the variable to read and the host the resolved value is sent to as a password — and nothing checked either. A run whose environment holds secrets (CI, an operator's shell) could therefore have any variable exfiltrated to a host the contract picked. All six acquisition runners go through `resolve_connection_secrets`, so all six were affected: airbyte, debezium, dlt, duckdb, kafka_connect and meltano. The card named four; debezium and kafka_connect are callers too. Option B, founder-chosen on 2026-10-26 after the three options were laid out: `env://` resolves only for a LOOPBACK destination, or when the operator listed the variable in FLUID_ACQUISITION_SECRET_REF_ALLOWLIST as `NAME` or `NAME@host`. The check sits in `resolve_connection_secrets`, the one place that sees both halves, so every runner inherits it without touching any runner. It runs BEFORE resolution, so a refused send never reads the secret, and the refusal names the variable and host but never the value — asserted over the whole rendered error, not one field. Only `env://` is policed. `vault://`, `aws://`, `gcp://`, `azure://` and `file://` name a secret the operator provisioned and granted this process access to, which is already an operator decision; `env://` turns whatever happens to be in the environment into a credential for a host the contract chose. The allowlist is operator-only, registered in `credentials/protected_env.py` beside the federation one: `fluid apply` hydrates `.env`, `.env.<env>` and FLUID_SECRETS_FILE, and those files are committed beside the contract, so without this a contract repository could grant itself the allowlist meant to constrain it. That existing mechanism was reused rather than reimplemented. TWO THINGS THE WORK ITSELF TURNED UP, both fixed here: 1. `TESTCONTAINERS_HOST_OVERRIDE` was NOT operator-only, while FLUID_RUNNER_HOST_OVERRIDE was. Both rewrite a contract-declared loopback host to a reachable one AFTER this policy clears the send as local-only, so a project dotenv file could have redirected a credential allowed only because it was staying on this machine — defeating the single allowance option B grants. Now registered. 2. I had written a second loopback check. `_acquisition_common._is_loopback_host` already existed and is what `apply_loopback_host_override` rewrites on, and the exemption is sound ONLY while the two cover the same hosts: wider here would exempt a send the override leaves pointing at a real host. It now delegates to that one definition, and a test asserts they agree over nine cases. The narrowing is deliberate — `db.localhost` is remote and needs an entry, because the override would not rewrite it either. Resolved values are registered with the exact-value redactor (#884): a literal a client library echoes in its own error is now masked by value, not by hoping a pattern matches its shape. Seven existing tests refused correctly — they use `env://` against `db.test` and `db.example`, which are remote. They now set the allowlist, which is the same one-line change an operator makes on upgrade, rather than the policy being weakened to accommodate them. Three per-class copies of that fixture were hoisted to one module-level fixture. CHANGELOG carries the upgrade note: which contracts stop working, the single variable that restores each, and that local development is unaffected. Verified: 36 new policy tests and 1735 build_runners tests green; import-linter 6 contracts kept, 0 broken; startup budget 6/6; ruff and CI-pinned black clean whole-tree. * test: the debezium credential-hygiene suite allows its own fixture host The full-suite run caught an eighth refusal the targeted runs had not: tests/security/test_credentials_stay_out_of_files_and_diagnostics.py drives the DEBEZIUM runner — one of the two callers the card never listed — with `env://PG_PASSWORD_T211` against the bare host `db`, which is not loopback. A correct refusal, so the test sets the allowlist rather than the policy being loosened. That suite is about the secret not reaching a file or a diagnostic once it is legitimately in use, which is a different control from whether the send is permitted at all. Swept every test file mentioning env:// afterwards; none still refuses. 299 security tests green. * security: close the bypasses the adversarial review found in this policy The 4-lens review of the policy returned bypass=FAIL, correctness=FAIL and 8 must-fix. It was right, and two of the findings were my own errors stated as fact in the previous commit message. THE BYPASS (high). `connection_host` trusted the first destination field that parsed, so a contract set `host: localhost` as a decoy beside a remote `connection_string` (debezium's mongodb shape) and the send was cleared on the decoy while the credential went to the remote host. `connection_hosts` now returns EVERY destination and the policy clears all of them; the field set also gained `connection_string`, `server` and `endpoint`. THE CLAIM I GOT WRONG. The previous commit said the airbyte `deployment.auth.secretRef` path was policed. It was not: that edit had failed on a bad anchor, I noted "nothing written" at the time and never redid it, then asserted it was done. It is policed now, and a test reads the module source for the call site rather than trusting a claim. FAIL-OPEN PARSING. An allowlist entry of the form `NAME@` — a name with an empty host — parsed to "any host", the opposite of fail-closed, and my own "malformed entry grants nothing" test did not cover it. `NAME@` now grants nothing. federation fails closed here too, which is where I should have looked. Also fixed, each with a test: - A `secretRef` whose target field already holds an inline credential is no longer policed: the reference is never read and nothing is sent, which is what the upgrade note already promised. - An object-store URI (`s3://`, `gs://`, `az://`, `file://`) is no longer read as a host. Its authority is a bucket, so refusing on it broke working contracts and suggested an allowlist entry naming a bucket. Now explicitly out of the policy's model, and recorded as a known limitation. - `localhost.`, the FQDN form, is loopback. - `AcquisitionSecretPolicyError` was missing `@dataclass`, so its machine-readable `code` reported `FluidUserError`. - The duckdb runner flattened the refusal through `except Exception`, dropping the `fix` the operator needs; it now propagates so the CLI renders the panel. - The refusal's `fix` warns that a host written with a port never matches — the trip-up federation already warns about — and a test pins that it fails closed. - `describe()` was dead code. THE CHANGELOG NOW SAYS WHAT IS TRUE. The review's sharpest point was not a code defect: the previous text asserted "the set of contracts this breaks is exactly the set that was vulnerable" and that loopback "needs no configuration and is unaffected". Both are false for kafka_connect and the `bring-your-own` / `managed` modes of debezium and airbyte — the DEFAULT modes — where forge-cli does not connect to the database at all but hands the whole connector config, credential included, to the orchestrator at a contract-chosen `deployment.server_url` that nothing allowlists. A loopback `connection.host` there is a zero-configuration exfil, unchanged from before this release. Publishing a guarantee that does not hold is worse than publishing none, so there is now a `### Known limitations` section naming that gap, the object-store gap, and `{{ env.NAME }}` interpolation, which bypasses `secretRef` entirely. Rebased onto 98e51fb (#750), resolving the CHANGELOG conflict and consolidating `## [Unreleased]` back to one section per heading. 2830 tests green across build_runners, cli/discover and security; 50 in the policy file itself. * chore(ci): the secret gate reads the new tests and the upgrade note as clean `Lint & Format` failed on four detect-secrets findings in this branch, all false positives, and my pre-push run had missed them: it was not the pinned 1.5.0 over the PR's own changed-file list. Reproduced faithfully first (detect-secrets==1.5.0, the same `detect-secrets-hook --baseline` call CI makes over `git diff --name-only BASE HEAD`), then fixed: * The upgrade note's example wrote the allowlist as `NAME="VALUE"`, which is the keyword detector's exact shape. Reworded to name the variable and give the value as prose, so the note needs no pragma wedged into rendered text. * Four lines in the policy tests carry `password`/userinfo literals that ARE the shape under test. Each gets an inline `pragma: allowlist secret`. Two traps worth recording. The detector reports one finding per value per file, so allowlisting line 183 surfaced the identical literal at 322 — a second round was needed, not a sign the first fix failed. And black then exploded both literals and carried the trailing pragma onto the closing bracket, where detect-secrets does not look: each pragma had to move back onto the line that actually holds the value. Verified with CI's pinned black==24.10.0, not the newer black in this machine's venvs, which disagrees with it on 30 unrelated files. `.secrets.baseline` carries refreshed line numbers for the files this branch moved. The diff is line numbers and `generated_at` only — no hashed secret is added, so nothing new is being recorded or suppressed. Gates: detect-secrets-hook rc=0 over the changed-file list; ruff clean; black==24.10.0 clean whole-tree; 383 passed, 2 skipped across the policy suite and the three existing suites this branch touches. * chore: two imports this branch added and never used The code-quality bot found `typing.Dict` in `_secret_policy.py` and `os` in the policy test; both are leftovers from earlier drafts and neither is referenced. Removed. Repo-wide lint cannot catch these: `F401` is in the `ignore` list in pyproject's `[tool.ruff.lint]` ("many are re-exports"), so `ruff check fluid_build/ tests/` passes over a dead import and only the bot flags it. The one job that does select `F401` scopes itself to `fluid_build/copilot` and `fluid_build/forge_datamodel`. Worth running `ruff check --select F401,F841` over a branch's own files before pushing, since nothing else will. Doing that here found five more, all **identical on the base commit** `98e51fb6` and so none of this branch's doing: four function-local `import duckdb` statements in `build_runners/duckdb/runner.py` (lines 1023, 1128, 1525, 1583) and an unused `ctx` local in `test_acquisition_common.py:801`. Left alone deliberately — a security PR is the wrong place to touch four unrelated call sites in the duckdb runner, and the `ctx` fix is one ruff itself marks unsafe. Gates after the change: ruff clean whole-tree, black==24.10.0 clean whole-tree, `ruff --select F401` clean on both edited files, detect-secrets-hook rc=0 with the baseline unchanged, import-linter 6 kept / 0 broken, startup budget 6 passed, 383 passed / 2 skipped across the policy suite and the three existing suites this branch touches.

Cuts 0.23.0 from the unreleased changes. A minor bump, not a patch: it adds seven optional 0.7.6 preview fields, and it changes the shape of exported ODCS and ODPS artifacts — the per-port contract name, the input-port contractId, the Kafka server format and column physicalType, quality rule ids, and the import pass-through bucket's address. Two of its commits are marked breaking, since a contract whose pinned contract id collides with a sibling's now fails validate and every export path; that export used to succeed and silently drop one output port's entire schema. The release also carries three commits that were on main in no release yet: the .tmp symlink sandbox fix (#743), the DuckDB 2.0 drift fixes (#749), and the acquisition ambient-credential fix (#750).

github-actions Bot added security Security-related changes or reports cli Changes to the CLI surface or implementation tests Test coverage or test infrastructure changes labels Oct 26, 2026

github-code-quality Bot found potential problems Oct 26, 2026

View reviewed changes

Comment thread tests/build_runners/test_duckdb_host_credential_env.py Fixed

Comment thread fluid_build/cli/discover/_jdbc_introspect.py Fixed

Comment thread fluid_build/cli/discover/_jdbc_introspect.py Fixed

fas89 added 2 commits October 26, 2026 18:41

github-code-quality Bot found potential problems Oct 26, 2026

View reviewed changes

Comment thread tests/build_runners/test_duckdb_host_credential_env.py Fixed

Comment thread fluid_build/cli/discover/_jdbc_introspect.py Resolved

fas89 merged commit 98e51fb into main Oct 26, 2026
37 checks passed

fas89 deleted the fix/host-credential-env-suppression branch October 26, 2026 18:10

fas89 mentioned this pull request Oct 26, 2026

security: a contract could send any environment variable to a host it named #751

Merged

1 task done

fas89 mentioned this pull request Oct 27, 2026

chore(release): 0.23.0 #755

Merged

1 of 3 tasks

Navigation Menu

Repository navigation

security: an acquisition source got this host's ambient database credentials - #750

security: an acquisition source got this host's ambient database credentials#750
fas89 merged 4 commits into
mainfrom
fix/host-credential-env-suppression

fas89 commented Oct 26, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

fas89 commented Oct 26, 2026

The defect

Why the implementation moved

Validation

What is NOT here

Documentation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant