Skip to content

Linux add_servers accepts env:/file: secret references (#2087) - #2096

Merged
erikdarlingdata merged 1 commit into
devfrom
linux-addservers-2087
Aug 7, 2026
Merged

Linux add_servers accepts env:/file: secret references (#2087)#2096
erikdarlingdata merged 1 commit into
devfrom
linux-addservers-2087

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

Closes #2087 (found during the 3.4.0 Azure smoke).

add_servers refused every SQL-auth password off-Windows — dead-ending the designed onboarding path for compose deployments, since the store-authoritative control plane means darling.json edits don't add servers after first seed.

Fix

  • ProtectPasswordForStorage: an env:/file: reference passes through verbatim on every platform — a pointer is not a secret; the secret stays in the mounted file/env var (the Darling on Linux: Docker/compose distribution of the service + TimescaleDB store #1804 contract). Literals still DPAPI-encrypt (Windows-only), and the refusal message now names the cross-platform alternative.
  • DarlingSecrets.ResolvePassword: a reference in the encrypted-password slot resolves via DarlingSecretSource instead of being fed to DPAPI Unprotect (a reference can never be confused with a base64 blob). usedPlaintext stays false — a reference is not plaintext-in-config, so callers don't warn.

Tests

Reference-in-encrypted-slot resolves through an env var; references pass storage untouched on all platforms; literals still round-trip DPAPI on Windows; null/empty stay null (Windows auth).

🤖 Generated with Claude Code

The onboarding path refused every SQL-auth password off-Windows, which
dead-ends compose deployments: the store-authoritative control plane
means darling.json edits do not add servers after first seed, so
add_servers IS the designed path -- and it demanded DPAPI on a platform
that has none. A reference is a pointer, not a secret (the #1804
contract): store it verbatim in the encrypted-password slot and teach
ResolvePassword to resolve references there instead of feeding them to
DPAPI Unprotect (a reference can never be confused with a base64 blob).
Literal passwords still require Windows; the refusal now names the
cross-platform alternative.

Tests: reference-in-encrypted-slot resolves via env (usedPlaintext
false, the no-warn rule); references pass through storage untouched on
every platform; literals still round-trip DPAPI on Windows.

Closes #2087

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines +482 to +485
if (DarlingSecretSource.IsReference(plaintextPassword))
{
return plaintextPassword;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical: the reference this stores into encrypted_password can never be read back on the platform this PR targets.

add_servers now writes an env:/file: reference verbatim into the encrypted_password column (there is no plaintext password column in config_monitored_servers at all — see InsertServerSql, $7). On the next collection sweep, StoreConfigProvider.BuildServerFromRow loads that value back into MonitoredServer.EncryptedPassword, and the actual connect happens in DarlingServerConnector.ResolveConnectionString:

else
{
    // Non-Windows branch
    if (!string.IsNullOrWhiteSpace(config.EncryptedPassword))
    {
        throw new PlatformNotSupportedException(
            "encryptedPassword requires Windows (DPAPI); use password with an env:/file: reference on other platforms.");
    }
    ...
}

That file (DarlingServerConnector.cs) isn't touched by this PR. On non-Windows it throws unconditionally the moment EncryptedPassword is non-blank — it never checks DarlingSecretSource.IsReference before rejecting. So a server added via add_servers with a file:/env: password on Linux will probe successfully (the probe uses ProbeConfig.Password, which is reference-aware) and get inserted as "added" — but every subsequent collection cycle will throw PlatformNotSupportedException trying to reconnect, because the persisted value lives in EncryptedPassword, not Password.

The DarlingSecrets.ResolvePassword fix in this same PR (DarlingSecrets.cs) does handle a reference in the encrypted slot correctly — but it's only reachable when OperatingSystem.IsWindows() is true (see the call site in DarlingServerConnector.ResolveConnectionString, line ~79). For the actual bug this PR is fixing (#2087, Linux/compose onboarding), that code path never runs, so the fix there is effectively dead code for the scenario in the issue.

Net effect: this PR unblocks adding the server (add_servers returns "added" instead of refusing), but the server then fails to connect on every collection sweep afterward — the underlying #2087 symptom just moves from "can't add" to "added but never collects," which is worse because it's silent/deferred instead of an immediate, actionable error at onboarding time.

Suggest: in DarlingServerConnector.ResolveConnectionString's non-Windows branch, check DarlingSecretSource.IsReference(config.EncryptedPassword) before the PlatformNotSupportedException, and resolve it the same way the Windows branch (now) does. Would also be worth an integration-level test that round-trips add_servers → reload from store → ResolveConnectionString on a simulated non-Windows config, since the current tests only exercise ProtectPasswordForStorage and DarlingSecrets.ResolvePassword in isolation and never catch the mismatch between the write slot and the non-Windows read path.

@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review

Reviewed the change against correctness, Lite/Darling parity, security, and performance. This is Darling-only (there's no add_servers/MCP admin-tools equivalent in Lite, so no parity drift to flag here).

Critical: the fix doesn't actually unblock Linux collection (left an inline comment with details)

add_servers now stores an env:/file: reference verbatim into the encrypted_password column (there's no plaintext password column in config_monitored_servers). But DarlingServerConnector.ResolveConnectionString — the actual runtime connect path used by the collection loop (DarlingWorker.TryConnectAsyncConnectAsyncResolveConnectionString) — has its own inlined non-Windows branch that throws PlatformNotSupportedException unconditionally whenever config.EncryptedPassword is non-blank, without ever checking DarlingSecretSource.IsReference first. That file isn't part of this diff.

The DarlingSecrets.ResolvePassword fix in this PR does handle a reference in the encrypted slot correctly, but it's only reached when OperatingSystem.IsWindows() is true at the call site in DarlingServerConnector.cs — which is not the platform #2087 is about.

Net effect as written: add_servers on Linux will probe successfully (the probe connects via ProbeConfig.Password, which is reference-aware) and report "added" — but the server then throws on every subsequent collection sweep, because the persisted reference lives in EncryptedPassword, not Password. This changes the failure from an immediate, actionable refusal at onboarding time to a silent, deferred failure during collection — arguably worse than the bug being fixed. None of the new/existing tests exercise the full add → reload-from-store → reconnect path on a simulated non-Windows config, so nothing caught this.

Suggested fix: in DarlingServerConnector.ResolveConnectionString's non-Windows branch, check DarlingSecretSource.IsReference(config.EncryptedPassword) before throwing, and resolve it the same way the reference-in-Password case already does.

Minor

  • The CHANGELOG entry states references are "stored verbatim in the encrypted-password slot and resolved at connect time" — that resolution doesn't actually happen off-Windows given the above, so the changelog is asserting behavior that doesn't hold for the platform the fix targets.

No other correctness, security, or performance issues found in the diff — DarlingSecretSource.IsReference/Resolve (pre-existing, #1804) is used consistently, the DPAPI literal path is untouched, and the new tests for ProtectPasswordForStorage/ResolvePassword are well-targeted for the units they cover.

@erikdarlingdata
erikdarlingdata merged commit 86a6183 into dev Aug 7, 2026
5 checks passed
@erikdarlingdata
erikdarlingdata deleted the linux-addservers-2087 branch August 7, 2026 09:14
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