fix(extensions): prime LIBREFANG_VAULT_KEY from dotenv before vault unlock by f-liva · Pull Request #5065 · librefang/librefang

f-liva · 2026-05-15T13:52:15Z

Problem

Container deployments that ship the credential-vault key in secrets.env (rather than the host process env) hit a chicken-and-egg inside load_dotenv:

load_vault() runs first and tries to unlock vault.enc. The resolver reads LIBREFANG_VAULT_KEY from the process env, finds nothing (the orchestrator/container runtime doesn't set it), and returns VaultLocked.
load_env_file(secrets.env) runs immediately after and populates LIBREFANG_VAULT_KEY — too late.

Every subsequent caller of CredentialVault::unlock() (the MCP OAuth provider in particular) sees a stale "Vault not initialized" or decryption-fails error and silently returns None. The user-visible symptom is: OAuth flow completes in the dashboard, tokens never persist, the agent reports no MCP access on the next turn.

Concrete reproducer: Lazycat NAS container that ignores Docker ENV lines because the orchestrator launches its own binary with a fresh env. The only way operators can ship the vault key on that platform is secrets.env, which today is loaded after vault unlock.

Change

Add a tightly-scoped bootstrap pass — prime_vault_key_from_dotenv — that scans .env and secrets.env for LIBREFANG_VAULT_KEY only and seeds it into the process env if it isn't already present. The regular load_env_file pass continues to run after the vault is consulted, so the documented priority order (system env > vault > .env > secrets.env) stays intact for every other variable. VAULT_KEY_ENV is promoted from a private const to pub(crate) so the bootstrap doesn't have to duplicate the env var name string.

The new helper extract_env_value(path, key) reads a single key out of a dotenv file without polluting the process env with everything else in it; it correctly handles base64 padding (= in the value), quoted values, and # comments.

Verification

cargo test -p librefang-extensions --lib → 89 passed, 0 failed.
cargo clippy --workspace --all-targets -- -D warnings clean.
Three new unit tests cover the failure modes that motivated this PR:
- extract_env_value_finds_key_with_base64_padding — guards against parsers that mishandle the trailing = on a 32-byte base64 vault key (the exact symptom that surfaced on secrets.env in the field).
- extract_env_value_handles_quoted_and_commented_lines — guards against the two common dotenv hand-edits (quoted value, comment line that shadows the real one).
- extract_env_value_returns_none_for_missing_and_empty — confirms the silent-no-op branches.
End-to-end on a Lazycat-managed container: dropping LIBREFANG_VAULT_KEY=… into /data/secrets.env is now sufficient to unlock the vault on next restart, so OAuth token persistence — and therefore MCP refresh — works without a code change to the orchestrator side.

Backwards compatibility

No public API changes.
The bootstrap is a no-op when LIBREFANG_VAULT_KEY is already in the system env (existing host-env deployments behave unchanged byte-for-byte).
The bootstrap is also a no-op when .env / secrets.env don't contain the key.
Other dotenv keys keep their existing precedence: regular load_env_file still runs after the vault, so vault-stored secrets still win over .env / secrets.env values.

…nlock Container deployments that ship the credential-vault key in `secrets.env` (rather than the host process env) hit a chicken-and-egg inside `load_dotenv`: 1. `load_vault()` runs first and tries to unlock `vault.enc`. The resolver reads `LIBREFANG_VAULT_KEY` from the process env, finds nothing (the orchestrator/container runtime doesn't set it), and returns `VaultLocked`. 2. `load_env_file(secrets.env)` runs immediately after and populates `LIBREFANG_VAULT_KEY` — too late: every subsequent caller of `CredentialVault::unlock()` (e.g. the MCP OAuth provider) sees a stale "Vault not initialized" / decryption-fails error and silently returns `None`. The user-visible symptom is "OAuth flow completes, tokens never persist, agent reports no MCP access on the next turn". Add a tightly-scoped bootstrap pass — `prime_vault_key_from_dotenv` — that scans `.env` and `secrets.env` for `LIBREFANG_VAULT_KEY` *only* and seeds it into the process env if it isn't already present. The documented priority order (system env > vault > .env > secrets.env) stays intact for every other secret, because the regular `load_env_file` pass runs after the vault is consulted and continues to skip keys the vault already populated. `VAULT_KEY_ENV` is promoted from private `const` to `pub(crate)` so the bootstrap doesn't have to duplicate the env var name string. Tests: - `extract_env_value_finds_key_with_base64_padding` — guards against parsers that mishandle the trailing `=` on a 32-byte base64 vault key. - `extract_env_value_handles_quoted_and_commented_lines` — guards against the two common dotenv hand-edits (quoted value, comment line that shadows the real one). - `extract_env_value_returns_none_for_missing_and_empty` — confirms the silent-no-op branches. `cargo test -p librefang-extensions --lib` → 89 passed, 0 failed. `cargo clippy --workspace --all-targets -- -D warnings` clean. Verified end-to-end on a Lazycat-managed container where the orchestrator binds the manifest but doesn't expose `environment:` edits post-install: dropping `LIBREFANG_VAULT_KEY=…` into `/data/secrets.env` is now sufficient to unlock the vault on next restart, so OAuth token persistence — and therefore MCP refresh — works without a code change to the orchestrator side.

f-liva · 2026-05-15T14:01:00Z

Why this isn't an ad-hoc fix

The chicken-and-egg in load_dotenv only surfaces on certain deployment topologies, so it's worth spelling out who benefits and why this is a general-purpose fix rather than a workaround for one user's setup.

The latent bug

CredentialVault::unlock() resolves the master key in this priority order:

System env — LIBREFANG_VAULT_KEY set in the parent process before librefang starts.
OS keyring — SecretService / Keychain / Windows Credential Manager.
Error.

load_dotenv runs:

load_vault() — attempts vault unlock, falls through silently if neither source is available.
load_env_file(.env) — sets process env vars.
load_env_file(secrets.env) — sets process env vars.

The vault unlock attempt in step 1 is final — it doesn't retry after steps 2-3 populate the env. So if a user puts LIBREFANG_VAULT_KEY=… in secrets.env (the natural place to put credentials), the vault is permanently unreachable for the rest of the process lifetime. Every downstream caller — KernelOAuthProvider::load_token, try_refresh, store_tokens, the mcp_oauth:* lookups in the MCP runtime — sees Vault not initialized or Decryption failed: aead::Error and degrades to "no auth", with vault_get_or_warn returning None.

The OAuth flow then completes successfully at the HTTP layer (browser sees "Authorization Successful"), the kernel asks the provider to persist tokens, the provider silently no-ops on the vault write, and the next refresh / tool call comes back unauthenticated. That's exactly the user-facing failure mode in this PR's body: "OAuth flow completes, tokens never persist, agent reports no MCP access on the next turn."

Who gets the bug on stock librefang

Anyone whose deployment can't easily set arbitrary env vars in the parent process:

Topology	Why VAULT_KEY ends up in `secrets.env`
Docker Compose without per-host `.env` file	`environment:` requires a rebuild/redeploy to change; ops drop credentials into a mounted file instead
Kubernetes with `Secret` mounted as a file	`volumeMounts` for `Secret` is the documented k8s pattern; `env.valueFrom.secretKeyRef` is one extra layer of YAML per key and many teams skip it
Lazycat NAS, Yunohost, Umbrel, CasaOS, and similar self-hosted app stores	The orchestrator binds a generated manifest read-only and there's no first-class env-edit UI post-install
systemd unit using `EnvironmentFile=`	Some operators prefer keeping all secrets in a single mode 0600 file under `/etc/librefang/` rather than splattering them across `Environment=` lines in the unit
Render / Fly.io / Railway tier where env edits trigger a full redeploy	Operators bake the secret into the persistent volume's `secrets.env` to avoid the deploy roundtrip

Operators on those platforms today are dropping LIBREFANG_VAULT_KEY=… into secrets.env, then opening an issue when the vault doesn't unlock. The fix turns secrets.env into a first-class location for the vault key, matching what every dotenv-shaped consumer already expects.

Who doesn't get the bug

Anyone whose VAULT_KEY is already in the parent process env by the time librefang exec's:

Linux desktop with D-Bus SecretService (vault uses the OS keyring path — load_vault resolves the key via keyring, dotenv files are irrelevant).
Docker ENV LIBREFANG_VAULT_KEY=… in the image (rare for secrets, but works).
A wrapper script that sources /etc/librefang.env before exec librefang.
CI / dev shells where LIBREFANG_VAULT_KEY is already exported.

For all of those, the bootstrap is a no-op (line 42-ish of the new helper, std::env::var(VAULT_KEY_ENV).is_ok() short-circuits before any file read).

Why this is the right shape

Scope is the smallest fix that resolves the chicken-and-egg. Only LIBREFANG_VAULT_KEY is bootstrap-read from dotenv. The rest of secrets.env continues to load after the vault, so the documented priority order (system env > vault > .env > secrets.env) is preserved for every other variable — vault-stored secrets still win over dotenv values, as the comment on load_vault() promises.
Read-only, side-effect-bounded: extract_env_value() reads one file once per dotenv path, returns one Option<String>, doesn't mutate. The only std::env::set_var lives behind the same UB-safety comment the existing code uses, and only runs from synchronous main before tokio starts.
Prior art: the same chicken-and-egg shows up in sops, git-crypt, the HashiCorp Vault client, and a handful of dotenv-aware secret stores — all of them pre-bootstrap the unlock key from a known file before the main load. This PR follows the same pattern; nothing exotic.

Operator-facing impact

Once this lands, the operator documentation can simplify from "set LIBREFANG_VAULT_KEY in your process environment via one of these N platform-specific tricks" to "put it in secrets.env like every other credential." That's a real reduction in support burden, particularly for the self-hosted-NAS / homelab crowd that's currently hitting this as a silent failure.

Happy to factor the bootstrap into a separate module / add a test for the [no .env, no secrets.env, no system env] path / split into two commits if any of that would help review. Just say the word.

houko

Thanks for the careful write-up — the priority-order preservation argument is correct and the motivation is real. One blocking parser bug and a few smaller items.

Blocking

extract_env_value aborts the entire scan on the first malformed line — crates/librefang-extensions/src/dotenv.rs:98:

let (k, v) = line.split_once('=')?;

The ? propagates None out of the whole function, not the loop iteration. So if .env / secrets.env contains any non-empty, non-#-prefixed line that lacks = (a stray token, a hand-edited typo, a leftover marker), the scanner returns None before it ever reaches LIBREFANG_VAULT_KEY on a later line. The existing load_env_file (crates/librefang-extensions/src/dotenv.rs:90-110) handles this correctly via if let Some(...) = parse_env_line(...) + implicit continue — the new function regresses that robustness.

Repro (verified locally):

# header
SOME_FLAG
LIBREFANG_VAULT_KEY=correct_value

returns None instead of Some(\"correct_value\").

Fix: replace ? with explicit continue:

let Some((k, v)) = line.split_once('=') else { continue; };

Since this is the exact production path that motivates the PR, please add a unit test covering a malformed line that precedes the vault-key line.

Non-blocking

Silent IO error on bootstrap path — crates/librefang-extensions/src/dotenv.rs:92:
```
let content = std::fs::read_to_string(path).ok()?;
```
A permission-denied or transient IO failure on secrets.env here produces the same opaque VaultLocked downstream that this PR is trying to fix. load_vault (line 72) uses eprintln! for its analogous failure mode — it'd be consistent to log a one-line warning on Err(e) from read_to_string when the path exists but isn't readable (skip the log if e.kind() == NotFound, since absent files are expected). Same eprintln! channel as load_vault for the same reason (no tracing subscriber yet).
No end-to-end test for the boot-ordering fix itself. The three new unit tests exercise extract_env_value in isolation; the actual claim — "load_dotenv now unlocks the vault when only secrets.env carries the key" — has no regression test. Acknowledged it's awkward because of DOTENV_LOADED: Once + process-global env, but a serial_test::serial test driving LIBREFANG_HOME to a tempdir and asserting prime_vault_key_from_dotenv sets the var would lock the bug closed. Same pattern as test_load_env_file_does_not_override_existing_var at crates/librefang-extensions/src/dotenv.rs:424.
Minor parse divergence from parse_env_line. extract_env_value doesn't unescape \\n / \\\\ / \\\" inside double-quoted values, while parse_env_line (line 152, unescape_env_value) does. For a base64 vault key this is irrelevant in practice, but if an operator copies a quoted value from a template with literal \\n for visual wrapping, the two parsers will diverge. Probably fine to leave; flagging only because the divergence isn't documented in the helper doc-comment.

Question

Is there a reason prime_vault_key_from_dotenv doesn't just call parse_env_line (line-filtered for the matching key) rather than reimplementing a smaller parser? Reusing parse_env_line would (a) close the divergence above, (b) inherit the existing test coverage, and (c) eliminate the split_once('=')? bug structurally. The new helper would be ~10 lines instead of ~25.

Addresses PR librefang#5065 review feedback (houko, 2026-05-15): **Blocking** — `extract_env_value` propagated `?` out of `line.split_once('=')`, so a single malformed line (no `=`, stray marker, hand-edit typo) anywhere in `.env` / `secrets.env` aborted the entire scan before the real vault-key line was reached. Repro: # header SOME_FLAG LIBREFANG_VAULT_KEY=correct_value returned `None` instead of `Some("correct_value")`. Fix by delegating per-line parsing to `parse_env_line`. Same approach the regular `load_env_file` loop uses — a `None` from `parse_env_line` is an implicit `continue`, never an early-exit. Bonus: closes the quote / escape-sequence divergence flagged in the non-blocking review point #3, because the helper now inherits `parse_env_line`'s `unescape_env_value` handling for double-quoted values. **Non-blocking** — opaque IO failure (review point #1): a non-NotFound read error on `secrets.env` now logs a one-line `eprintln!` warning (same channel `load_vault` uses, since `load_dotenv` runs before any tracing subscriber is installed) instead of silently returning `None`. Absent files stay silent — that's the expected case. **Tests**: - `extract_env_value_skips_malformed_lines_before_vault_key` — regression for the `?`-propagation blocker. Writes a file with two malformed lines preceding the vault-key line and asserts the scan still recovers the value. - `prime_vault_key_from_dotenv_seeds_env_from_secrets_env` — the end-to-end test the review (point #2) asked for: drives `LIBREFANG_HOME` at a tempdir, populates `secrets.env`, asserts `prime_vault_key_from_dotenv` seeds `LIBREFANG_VAULT_KEY` into the process env. Guarded with `#[serial_test::serial]` like `test_load_env_file_does_not_override_existing_var` — env-mutating tests must not run concurrently or `std::env::{set,remove}_var` becomes UB. `cargo check --workspace --lib` clean. `cargo test -p librefang-extensions --lib dotenv` → 19 passed, 0 failed. `cargo clippy -p librefang-extensions --all-targets -- -D warnings` clean.

f-liva · 2026-05-16T11:38:10Z

Addressed @houko's feedback in 931921c:

Blocking — extract_env_value now delegates to parse_env_line, so a malformed line earlier in the file never aborts the scan. Bonus: closes the quote / escape-sequence divergence from non-blocking point #3 — the helper inherits parse_env_line's unescape_env_value handling for double-quoted values for free.

Non-blocking #1 (silent IO error) — a non-NotFound read error on secrets.env now emits a one-line eprintln! warning on the same channel load_vault uses (since load_dotenv runs before any tracing subscriber is installed). Absent files stay silent.

Non-blocking #2 (end-to-end test for the boot-ordering fix) — added prime_vault_key_from_dotenv_seeds_env_from_secrets_env, guarded with #[serial_test::serial] so std::env::{set,remove}_var doesn't race sibling tests.

Non-blocking #3 — closed structurally by the refactor (see above).

Also added a focused regression test for the blocker: extract_env_value_skips_malformed_lines_before_vault_key writes two malformed lines before the vault-key line and asserts the scan still recovers the value.

Local verification:

cargo check --workspace --lib clean
cargo test -p librefang-extensions --lib dotenv → 19 passed
cargo clippy -p librefang-extensions --all-targets -- -D warnings clean

houko

LGTM — all four review points addressed substantively.

Blocking (malformed-line scan abort): closed by the refactor at crates/librefang-extensions/src/dotenv.rs:107-118. extract_env_value now delegates per-line parsing to parse_env_line via let Some((k, v)) = parse_env_line(trimmed) else { continue; }; — same robustness shape load_env_file uses. Regression locked by extract_env_value_skips_malformed_lines_before_vault_key (writes STRAY_MARKER_NO_EQUALS + ANOTHER_BAD_LINE before the vault-key line and asserts Some("after_garbage")).

Non-blocking #1 (silent IO error): addressed at dotenv.rs:96-108. NotFound stays silent (expected for absent dotenv files), every other io::Error emits a one-line eprintln! on the same channel load_vault already uses.

Non-blocking #2 (e2e ordering test): prime_vault_key_from_dotenv_seeds_env_from_secrets_env drives LIBREFANG_HOME at a tempdir, populates secrets.env, runs prime_vault_key_from_dotenv(), asserts VAULT_KEY_ENV is seeded. Guarded with #[serial_test::serial], with prev-state save/restore before the assertion so a failure doesn't leak env state to sibling tests — matches the pattern of test_load_env_file_does_not_override_existing_var.

Non-blocking #3 (parse divergence): closed structurally — the helper now inherits unescape_env_value handling for double-quoted values from parse_env_line, so the two parsers can't diverge.

Boot-ordering also verified end-to-end: load_dotenv (dotenv.rs:41-62) now runs prime_vault_key_from_dotenv() → load_vault() → load_env_file(.env) → load_env_file(secrets.env). The bootstrap is scoped strictly to VAULT_KEY_ENV, so the documented priority order (system env > vault > .env > secrets.env) for every other variable stays intact — load_env_file still skips keys already populated by the vault. No-op when LIBREFANG_VAULT_KEY is already in system env.

All CI lanes green (unit-fast, integration shards, macOS, Windows, clippy, secrets scan, OpenAPI drift).

houko

All three previously-flagged blockers are resolved in the latest revision:

Malformed line scan no longer aborts early — extract_env_value delegates to parse_env_line and continues on parse failure (extensions/src/dotenv.rs ~107-118). Regression test extract_env_value_skips_malformed_lines_before_vault_key pins it.
Non-NotFound IO errors on secrets.env now emit eprintln! warnings (~96-108) instead of silent swallow.
Parse divergence from parse_env_line closed structurally — extract_env_value inherits unescape_env_value for double-quoted values via the shared helper.

Verified the ordering invariant: prime_vault_key_from_dotenv() runs before load_vault() (dotenv.rs ~43-60), std::env check at line 50 prevents override of an operator-set system var, and decode_master_key() (vault.rs ~707-709) runs after the prime so an invalid base64 fails loudly. Priority documented as system env > vault > .env > secrets.env (only vault-key pre-seeds; other keys load after vault).

Test coverage spans positive (key in secrets.env unlocks), negative (missing key, no spurious error), edge (system env wins). .env location respects LIBREFANG_HOME, falls back to ~/.librefang per existing convention.

houko · 2026-05-20T12:45:48Z

Closing this as superseded rather than just textually conflicting — the equivalent fix already shipped on main.

crates/librefang-extensions/src/dotenv.rs on main already pre-seeds LIBREFANG_VAULT_KEY before load_vault() via preseed_vault_key_from(path), called once on env_file_path() and once on secrets_file_path() from load_dotenv(), with the same env-wins ordering this PR proposes. It landed in 403acdc1 via #5179 (porting #5139). The in-tree comment reads:

#5139: the vault master key may itself live in ~/.librefang/.env (or secrets.env). Vault::resolve_master_key() reads LIBREFANG_VAULT_KEY directly from std::env, so if the dotenv files aren't parsed before load_vault() the key isn't present yet and vault unlock fails silently.

The merge conflict shown in the PR view is the two implementations colliding on the same call site, not a mergeable textual diff.

Functional comparison:

	main (`preseed_vault_key_from`)	this PR (`prime_vault_key_from_dotenv`)
Env-wins short-circuit	yes	yes
Scans `.env` + `secrets.env`	yes (two calls from `load_dotenv`)	yes (internal loop)
Uses `parse_env_line`	yes	yes
Skips malformed lines	yes	yes
Logs path on non-NotFound read failure	silent	`eprintln!` with path + io error

Two deltas where this branch is arguably nicer:

extract_env_value emits a path-level eprintln! on non-NotFound read failure, which main's preseed_vault_key_from silently swallows.
References crate::vault::VAULT_KEY_ENV directly rather than re-declaring a local const VAULT_KEY_ENV = "LIBREFANG_VAULT_KEY".

If those two are worth landing, a much smaller follow-up against today's main (add the eprintln! to preseed_vault_key_from, promote VAULT_KEY_ENV to pub(crate)) would be cleaner than re-porting this branch.

The user-visible bug this PR was filed against — "MCP OAuth tokens silently never persist when LIBREFANG_VAULT_KEY is only in secrets.env" — is fixed on main today.

Thanks for the work @f-liva, and apologies for the duplicate path — the supersession wasn't obvious until the merge view was inspected.

github-actions Bot added size/M 50-249 lines changed area/security Security systems and auditing labels May 15, 2026

f-liva mentioned this pull request May 15, 2026

vault: init() + subsequent unlock() in daemon process fail with aead::Error despite identical LIBREFANG_VAULT_KEY env #5069

Closed

Merge branch 'main' into feat/dotenv-vault-key-bootstrap

89e9c09

houko requested changes May 16, 2026

View reviewed changes

github-actions Bot added the needs-changes Changes requested by reviewer label May 16, 2026

f-liva mentioned this pull request May 16, 2026

/mcp tools/list bypasses per-agent tool_allowlist/blocklist — claude-code driver silently dies on large MCP catalogues #5101

Closed

Merge branch 'main' into feat/dotenv-vault-key-bootstrap

75e7b7f

houko mentioned this pull request May 16, 2026

[audit] Auth / ACL hardening gaps #5139

Closed

3 tasks

Merge branch 'main' into feat/dotenv-vault-key-bootstrap

cd06146

houko approved these changes May 18, 2026

View reviewed changes

houko enabled auto-merge (squash) May 18, 2026 00:44

github-actions Bot added the has-conflicts PR has merge conflicts that need resolution label May 18, 2026

houko approved these changes May 20, 2026

View reviewed changes

houko closed this May 20, 2026

auto-merge was automatically disabled May 20, 2026 12:45
Pull request was closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(extensions): prime LIBREFANG_VAULT_KEY from dotenv before vault unlock#5065

fix(extensions): prime LIBREFANG_VAULT_KEY from dotenv before vault unlock#5065
f-liva wants to merge 5 commits into
librefang:mainfrom
f-liva:feat/dotenv-vault-key-bootstrap

f-liva commented May 15, 2026

Uh oh!

f-liva commented May 15, 2026

Uh oh!

houko left a comment

Uh oh!

f-liva commented May 16, 2026

Uh oh!

houko left a comment

Uh oh!

houko left a comment

Uh oh!

houko commented May 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

f-liva commented May 15, 2026

Problem

Change

Verification

Backwards compatibility

Related

Uh oh!

f-liva commented May 15, 2026

Why this isn't an ad-hoc fix

The latent bug

Who gets the bug on stock librefang

Who doesn't get the bug

Why this is the right shape

Operator-facing impact

Uh oh!

houko left a comment

Choose a reason for hiding this comment

Blocking

Non-blocking

Question

Uh oh!

f-liva commented May 16, 2026

Uh oh!

houko left a comment

Choose a reason for hiding this comment

Uh oh!

houko left a comment

Choose a reason for hiding this comment

Uh oh!

houko commented May 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants