Repository navigation
Add --harden-files, the actor that can apply the ACLs the service only knows (#2352) - #2358
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (!string.IsNullOrEmpty(storeRoot)) | ||
| { | ||
| targets.Add(new(storeRoot, AllowInteractive: false, IsDirectory: true, "the store directory")); | ||
| targets.Add(new(Path.Combine(storeRoot, "pg-credential.dpapi"), false, false, "the store credential")); | ||
| targets.Add(new(Path.Combine(storeRoot, "pg-admin-credential.dpapi"), false, false, "the admin credential")); | ||
| } |
There was a problem hiding this comment.
--harden-files skips two of the four DPAPI credential files it exists to fix.
DarlingManagedRoles.EnsureProvisionedAsync → EnsureRoleCredential (DarlingManagedRoles.cs) provisions and re-hardens three per-role credentials beside the data directory, not two:
AdminCredentialPathFor(dataDirectory) -> pg-admin-credential.dpapi
ViewerCredentialPathFor(dataDirectory) -> pg-viewer-credential.dpapi
McpCredentialPathFor(dataDirectory) -> pg-mcp-credential.dpapi
and TryHardenRoleCredential raises the exact same LogCritical("... is READABLE by ordinary local users ...") for any of the three if the ACL can't be applied — the same class of message this verb exists to answer for the owner credential (pg-credential.dpapi).
This target list only includes pg-credential.dpapi and pg-admin-credential.dpapi. pg-viewer-credential.dpapi and pg-mcp-credential.dpapi are never added, so an operator who runs --harden-files in response to a CRITICAL log about the viewer or mcp credential gets All N item(s) secured. / exit code 0 without that file ever being touched — touched doesn't count it, so the "verifies rather than claims" guarantee this verb is built around (and the non-zero-exit contract the PR description says makes it "usable in a provisioning script") silently doesn't cover it.
Hardening storeRoot as a directory doesn't fix this either — HardenDirectory protects the directory's own DACL for future children, it doesn't recurse into already-existing files' explicit ACEs (which is exactly why pg-credential.dpapi/pg-admin-credential.dpapi need their own explicit target entries here).
Worth adding the two missing targets — ideally via DarlingManagedPostgres.ViewerCredentialPathFor(dataDirectory) / McpCredentialPathFor(dataDirectory) rather than new hardcoded string literals, which would also have avoided this gap in the first place (the existing two targets hardcode "pg-credential.dpapi" / "pg-admin-credential.dpapi" instead of using DarlingManagedPostgres.CredentialFileName / AdminCredentialFileName).
| /// <summary> | ||
| /// Creates or removes every scoped Darling firewall rule so the live firewall matches darling.json. | ||
| /// Requires elevation (that is the entire point of the verb) and is idempotent — safe to re-run on every | ||
| /// upgrade, which is exactly how install-darling.ps1 uses it. Returns 0 when the firewall ends up matching | ||
| /// the config, 1 when it could not be made to. | ||
| /// </summary> | ||
| [SupportedOSPlatform("windows")] | ||
| /// <summary> | ||
| /// One target of <see cref="HardenFiles"/>: a path, whether the interactive operator legitimately reads it, | ||
| /// and what it is called in the report. Kept as data so the list is readable as a policy rather than as | ||
| /// control flow — which file gets INTERACTIVE read is the only judgement in this verb, and it should be | ||
| /// visible at a glance. | ||
| /// </summary> | ||
| private readonly record struct HardenTarget(string Path, bool AllowInteractive, bool IsDirectory, string What); |
There was a problem hiding this comment.
Copy/paste artifact: the /// <summary>Creates or removes every scoped Darling firewall rule...</summary> doc comment (belonging to ConfigureFirewallAsync, which still correctly carries its own copy of this exact text further down at what is now line ~2922) got duplicated here, sitting above [SupportedOSPlatform("windows")] and the new HardenTarget struct's own doc comment. As written, [SupportedOSPlatform("windows")] on line 2758 attaches to HardenTarget (attributes bind to the next declaration, and the firewall-rule doc comment is just orphaned text above it, describing the wrong thing.
Also worth a second look: does a plain data-carrier readonly record struct (four fields, no OS-specific API calls) actually need [SupportedOSPlatform("windows")]? It's harmless either way, but if it's not required for a platform-compat-analyzer reason, it reads like it was pulled in by the same copy/paste rather than intentional.
Suggest deleting the stray lines 2752-2758 entirely, leaving just the HardenTarget's own summary at 2759-2764 above the struct.
|
Reviewed. Solid, well-tested addition overall (verb dispatch mirrors its siblings, help text,
No Lite/Darling parity concern — Lite has no installed service or DPAPI credential store, so there's no counterpart surface for this feature. No SQL in this PR, so the T-SQL style conventions don't apply here. |
f62ee6c to
83ded83
Compare
| { | ||
| targets.Add(new(storeRoot, AllowInteractive: false, IsDirectory: true, "the store directory")); | ||
| targets.Add(new(Path.Combine(storeRoot, "pg-credential.dpapi"), false, false, "the store credential")); | ||
| targets.Add(new(Path.Combine(storeRoot, "pg-admin-credential.dpapi"), false, false, "the admin credential")); |
There was a problem hiding this comment.
Wrong AllowInteractive value for the admin credential — this breaks the Viewer on a healthy box.
pg-admin-credential.dpapi is hardened here with AllowInteractive: false, but everywhere else in the codebase it's hardened with allowInteractiveRead: true:
DarlingManagedRoles.EnsureProvisionedAsync:/* admin/viewer credentials are read by the interactive Viewer -> INTERACTIVE-readable ACL. */ ... allowInteractiveRead: trueViewerSettings.csreadspg-admin-credential.dpapidirectly off disk for the admin connection.
So running --harden-files on a correctly-configured install strips the Viewer's read access to its own admin credential — the opposite of the PR description's "Idempotent, so it is safe on a healthy box and can simply live in a runbook." Should be AllowInteractive: true, matching pg-viewer-credential.dpapi's posture (which is missing from this list entirely — see the other comment).
| var targets = new List<HardenTarget> | ||
| { | ||
| /* INTERACTIVE read, alone in this list: the Viewer (ViewerSettings.ResolveConfigPath) and the CLI | ||
| verbs run as the operator and must still read the live config. Nothing reads a backup (#1769). */ | ||
| new(resolvedConfig, AllowInteractive: true, IsDirectory: false, "the live config"), | ||
| }; | ||
|
|
||
| foreach (var backup in SafeEnumerate(Path.GetDirectoryName(resolvedConfig), "darling.json.bak-*")) | ||
| { | ||
| targets.Add(new(backup, AllowInteractive: false, IsDirectory: false, "a config backup")); | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(storeRoot)) | ||
| { | ||
| targets.Add(new(storeRoot, AllowInteractive: false, IsDirectory: true, "the store directory")); | ||
| targets.Add(new(Path.Combine(storeRoot, "pg-credential.dpapi"), false, false, "the store credential")); | ||
| targets.Add(new(Path.Combine(storeRoot, "pg-admin-credential.dpapi"), false, false, "the admin credential")); | ||
| } |
There was a problem hiding this comment.
Target list is missing two of the four DPAPI credentials, plus the TLS private key.
The service itself hardens these files, but none of them appear here:
pg-viewer-credential.dpapi(DarlingManagedRoles.cs,allowInteractiveRead: true— read by the Viewer)pg-mcp-credential.dpapi(DarlingManagedRoles.cs,allowInteractiveRead: false— read by the in-service MCP host)server.key, the store TLS private key when network exposure is configured (DarlingManagedPostgres.EnsureServerCertificate→TryHardenCredentialFile(keyPath, allowInteractiveRead: false))
HardenFiles reports "All {touched} item(s) secured" without ever touching or verifying these, and the CHANGELOG entry claims the verb "Covers darling.json ... the store directory and the DPAPI credentials" — but 2 of 4 DPAPI credential files (and the key) are silently out of scope. An operator following the tool's own success message would have no way to know a misconfigured viewer/mcp credential or exposed TLS key is still readable.
| /// <summary> | ||
| /// Creates or removes every scoped Darling firewall rule so the live firewall matches darling.json. | ||
| /// Requires elevation (that is the entire point of the verb) and is idempotent — safe to re-run on every | ||
| /// upgrade, which is exactly how install-darling.ps1 uses it. Returns 0 when the firewall ends up matching | ||
| /// the config, 1 when it could not be made to. | ||
| /// </summary> | ||
| [SupportedOSPlatform("windows")] | ||
| /// <summary> | ||
| /// One target of <see cref="HardenFiles"/>: a path, whether the interactive operator legitimately reads it, | ||
| /// and what it is called in the report. Kept as data so the list is readable as a policy rather than as | ||
| /// control flow — which file gets INTERACTIVE read is the only judgement in this verb, and it should be | ||
| /// visible at a glance. | ||
| /// </summary> | ||
| private readonly record struct HardenTarget(string Path, bool AllowInteractive, bool IsDirectory, string What); |
There was a problem hiding this comment.
Leftover/duplicated doc comment + attribute: this /// <summary>Creates or removes every scoped Darling firewall rule...</summary> [SupportedOSPlatform("windows")] block describes ConfigureFirewallAsync (whose real, correct copy of this same doc comment still exists further down at ~line 2922-2928). Here it's misattached to the new HardenTarget record struct — looks like a copy/paste artifact from when this block was inserted above the firewall method. Misleading if anyone reads it in place, and [SupportedOSPlatform("windows")] on a plain data-carrying record struct is meaningless. Worth deleting these 7 lines.
|
Reviewed the diff. This is C#-only (no T-SQL), and the feature has no Lite counterpart — file-ACL hardening for a Windows service account is a Darling-only concept, so no parity gap there. Found two correctness/security bugs in
Nothing else jumped out — the rest of the target-discovery/verify/exit-code logic, the |
83ded83 to
302e537
Compare
| var targets = new List<HardenTarget> | ||
| { | ||
| /* INTERACTIVE read, alone in this list: the Viewer (ViewerSettings.ResolveConfigPath) and the CLI | ||
| verbs run as the operator and must still read the live config. Nothing reads a backup (#1769). */ | ||
| new(resolvedConfig, AllowInteractive: true, IsDirectory: false, "the live config"), | ||
| }; | ||
|
|
||
| foreach (var backup in SafeEnumerate(Path.GetDirectoryName(resolvedConfig), "darling.json.bak-*")) | ||
| { | ||
| targets.Add(new(backup, AllowInteractive: false, IsDirectory: false, "a config backup")); | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(storeRoot)) | ||
| { | ||
| targets.Add(new(storeRoot, AllowInteractive: false, IsDirectory: true, "the store directory")); | ||
| targets.Add(new(Path.Combine(storeRoot, "pg-credential.dpapi"), false, false, "the store credential")); | ||
| targets.Add(new(Path.Combine(storeRoot, "pg-admin-credential.dpapi"), false, false, "the admin credential")); | ||
| } |
There was a problem hiding this comment.
Two issues in this block:
1. Won't compile (CS1738). Every new(...) call here except the last two writes a positional argument after named arguments — new(resolvedConfig, AllowInteractive: true, IsDirectory: false, "the live config") has "the live config" as a trailing positional arg following two named ones. C# forbids a positional argument after any named argument in the same call ("valid as long as they're not followed by any positional arguments" per the language spec). This hits three call sites: the live-config target, the backup-loop target, and the store-directory target. The PR description says "Builds clean, 0 warnings, matching dev" — this method as written won't build at all.
2. Backup glob is hardcoded to "darling.json.bak-*". resolvedConfig can be an arbitrary path/filename via the --config positional arg or DARLING_CONFIG (see DarlingConfig.ResolveConfigPath), same as --configure-firewall/--configure-network. The existing analog in DarlingWorker.TryHardenConfigBackups builds the pattern from the actual filename (Path.GetFileName(configPath) + ".bak-*") for exactly this reason. As written, a custom-named config's backups are silently skipped by a tool whose entire point is to close exposure gaps.
| var targets = new List<HardenTarget> | |
| { | |
| /* INTERACTIVE read, alone in this list: the Viewer (ViewerSettings.ResolveConfigPath) and the CLI | |
| verbs run as the operator and must still read the live config. Nothing reads a backup (#1769). */ | |
| new(resolvedConfig, AllowInteractive: true, IsDirectory: false, "the live config"), | |
| }; | |
| foreach (var backup in SafeEnumerate(Path.GetDirectoryName(resolvedConfig), "darling.json.bak-*")) | |
| { | |
| targets.Add(new(backup, AllowInteractive: false, IsDirectory: false, "a config backup")); | |
| } | |
| if (!string.IsNullOrEmpty(storeRoot)) | |
| { | |
| targets.Add(new(storeRoot, AllowInteractive: false, IsDirectory: true, "the store directory")); | |
| targets.Add(new(Path.Combine(storeRoot, "pg-credential.dpapi"), false, false, "the store credential")); | |
| targets.Add(new(Path.Combine(storeRoot, "pg-admin-credential.dpapi"), false, false, "the admin credential")); | |
| } | |
| var targets = new List<HardenTarget> | |
| { | |
| /* INTERACTIVE read, alone in this list: the Viewer (ViewerSettings.ResolveConfigPath) and the CLI | |
| verbs run as the operator and must still read the live config. Nothing reads a backup (#1769). */ | |
| new(resolvedConfig, AllowInteractive: true, IsDirectory: false, What: "the live config"), | |
| }; | |
| foreach (var backup in SafeEnumerate(Path.GetDirectoryName(resolvedConfig), Path.GetFileName(resolvedConfig) + ".bak-*")) | |
| { | |
| targets.Add(new(backup, AllowInteractive: false, IsDirectory: false, What: "a config backup")); | |
| } | |
| if (!string.IsNullOrEmpty(storeRoot)) | |
| { | |
| targets.Add(new(storeRoot, AllowInteractive: false, IsDirectory: true, What: "the store directory")); | |
| targets.Add(new(Path.Combine(storeRoot, "pg-credential.dpapi"), false, false, "the store credential")); | |
| targets.Add(new(Path.Combine(storeRoot, "pg-admin-credential.dpapi"), false, false, "the admin credential")); | |
| } |
| /// <summary> | ||
| /// Creates or removes every scoped Darling firewall rule so the live firewall matches darling.json. | ||
| /// Requires elevation (that is the entire point of the verb) and is idempotent — safe to re-run on every | ||
| /// upgrade, which is exactly how install-darling.ps1 uses it. Returns 0 when the firewall ends up matching | ||
| /// the config, 1 when it could not be made to. | ||
| /// </summary> | ||
| [SupportedOSPlatform("windows")] | ||
| /// <summary> | ||
| /// One target of <see cref="HardenFiles"/>: a path, whether the interactive operator legitimately reads it, | ||
| /// and what it is called in the report. Kept as data so the list is readable as a policy rather than as | ||
| /// control flow — which file gets INTERACTIVE read is the only judgement in this verb, and it should be | ||
| /// visible at a glance. | ||
| /// </summary> | ||
| private readonly record struct HardenTarget(string Path, bool AllowInteractive, bool IsDirectory, string What); |
There was a problem hiding this comment.
Looks like the new HardenTarget/HardenFiles code got inserted in the middle of ConfigureFirewallAsync's existing doc comment + [SupportedOSPlatform("windows")] attribute instead of above/below it. The result: the firewall-rules <summary> and its [SupportedOSPlatform("windows")] attribute now dangle over the unrelated HardenTarget struct (a stray attribute on a plain data record), and the same doc comment reappears correctly further down at the real ConfigureFirewallAsync (around line 2922). Doesn't break the build, but it's dead/misleading documentation — a doc tool would show "Creates or removes every scoped Darling firewall rule..." as the summary for HardenTarget.
| /// <summary> | |
| /// Creates or removes every scoped Darling firewall rule so the live firewall matches darling.json. | |
| /// Requires elevation (that is the entire point of the verb) and is idempotent — safe to re-run on every | |
| /// upgrade, which is exactly how install-darling.ps1 uses it. Returns 0 when the firewall ends up matching | |
| /// the config, 1 when it could not be made to. | |
| /// </summary> | |
| [SupportedOSPlatform("windows")] | |
| /// <summary> | |
| /// One target of <see cref="HardenFiles"/>: a path, whether the interactive operator legitimately reads it, | |
| /// and what it is called in the report. Kept as data so the list is readable as a policy rather than as | |
| /// control flow — which file gets INTERACTIVE read is the only judgement in this verb, and it should be | |
| /// visible at a glance. | |
| /// </summary> | |
| private readonly record struct HardenTarget(string Path, bool AllowInteractive, bool IsDirectory, string What); | |
| /// <summary> | |
| /// One target of <see cref="HardenFiles"/>: a path, whether the interactive operator legitimately reads it, | |
| /// and what it is called in the report. Kept as data so the list is readable as a policy rather than as | |
| /// control flow — which file gets INTERACTIVE read is the only judgement in this verb, and it should be | |
| /// visible at a glance. | |
| /// </summary> | |
| private readonly record struct HardenTarget(string Path, bool AllowInteractive, bool IsDirectory, string What); |
ReviewReviewed Blocking: the Also in that block: the config-backup glob is hardcoded to Minor: the new code appears to have been inserted in the middle of Everything else — verb dispatch/help-text wiring, the Windows guard in |
…y knows The service computes the correct DACL and detects when the real one is wrong. What it lacks is authority: re-ACLing a file it does not own needs WRITE_DAC, and taking ownership needs a privilege a virtual service account is not granted. So it logs the remedy and carries on -- correctly, because a monitoring service must not refuse to monitor over a permissions problem -- and the only thing that ever APPLIED the rule to an existing install was install-darling.ps1. A box registered through the README's own sc create path left the operator hand-typing three icacls lines out of a CRITICAL log line (#2352). Seen on a real box today: darling.json and lp.secret both carried BUILTIN\Users:(RX) with inheritance unprotected, and every service start had been saying so, precisely and with the right commands, since the box was built. It VERIFIES rather than claims. Every target is re-read after the attempt and reported SECURED or STILL READABLE, and the exit code is 1 if anything is still exposed, so it can gate a provisioning script. "We tried" is not the same statement as "the secret is not readable" -- the distinction that let a permissions call which silently did nothing hide in the field for months. darling.json is the only target the interactive operator keeps read on: the Viewer and the CLI verbs run as that operator and must still read it. Nothing reads a backup (#1769), so the .bak-* siblings, the store directory and the DPAPI credentials get no interactive ACE. A config that will not load is a warning rather than a stop -- an unreadable darling.json is the very failure this repairs, so refusing to run without it would be refusing exactly when it is needed. Missing targets are skipped quietly: a BYO-Postgres install has no managed credential files and their absence is not a fault. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
302e537 to
e50d3b3
Compare
| /* The claim is the re-read, not the call that returned without throwing. */ | ||
| if (DarlingFileSecurity.IsReadableByOrdinaryUsers(target.Path)) |
There was a problem hiding this comment.
Verification is a no-op for the store-directory target.
IsReadableByOrdinaryUsers (DarlingFileSecurity.cs:187) does new FileInfo(path).GetAccessControl(). On Windows, opening a directory path through FileInfo/FileSecurity fails (no FILE_FLAG_BACKUP_SEMANTICS), so this throws — typically UnauthorizedAccessException — for every directory target, including storeRoot here. That's inside the method's own catch list (IOException/UnauthorizedAccessException/InvalidOperationException/PlatformNotSupportedException), so the exception is swallowed and the method returns false.
The result: for the store-directory target this line will always report false → SECURED, regardless of whether HardenDirectory actually locked it down. That directly undercuts the PR's stated selling point ("verifies rather than claims... reports SECURED or STILL READABLE") for exactly the target most likely to inherit a wide-open DACL (BUILTIN\Users:(RX) from C:\, per the PR description).
Every other existing call site of IsReadableByOrdinaryUsers in the repo passes a file path — this is the first directory call, and pre-existing TryHardenDirectory (DarlingManagedPostgres.cs:1621) never re-verifies after HardenDirectory for the same reason.
Worth adding a directory-aware check (e.g. DirectorySecurity/new DirectoryInfo(path).GetAccessControl()) before trusting this branch for target.IsDirectory, or skip the verification claim for directory targets and say so explicitly.
| /// <summary> | ||
| /// Creates or removes every scoped Darling firewall rule so the live firewall matches darling.json. | ||
| /// Requires elevation (that is the entire point of the verb) and is idempotent — safe to re-run on every | ||
| /// upgrade, which is exactly how install-darling.ps1 uses it. Returns 0 when the firewall ends up matching | ||
| /// the config, 1 when it could not be made to. | ||
| /// </summary> | ||
| [SupportedOSPlatform("windows")] | ||
| /// <summary> | ||
| /// One target of <see cref="HardenFiles"/>: a path, whether the interactive operator legitimately reads it, | ||
| /// and what it is called in the report. Kept as data so the list is readable as a policy rather than as | ||
| /// control flow — which file gets INTERACTIVE read is the only judgement in this verb, and it should be | ||
| /// visible at a glance. | ||
| /// </summary> | ||
| private readonly record struct HardenTarget(string Path, bool AllowInteractive, bool IsDirectory, string What); |
There was a problem hiding this comment.
Stray/duplicated doc comment + attribute: the <summary> block at 2752-2757 ("Creates or removes every scoped Darling firewall rule...") plus the [SupportedOSPlatform("windows")] at 2758 describe ConfigureFirewallAsync, not HardenTarget. It looks like this was copy-pasted as a starting point for the new code and never cleaned up — the correct copy of this same doc comment still precedes the real ConfigureFirewallAsync further down. As written it's misleading documentation attached to an unrelated record struct, and a [SupportedOSPlatform] attribute on a data-only struct is meaningless. Recommend deleting lines 2752-2758 and keeping just the actual HardenTarget summary at 2759-2764.
|
Reviewed the Left two inline comments:
Everything else looks solid — the file/backup/directory target table is easy to audit, the config-load-failure fallback and quiet-skip-on-missing-target behavior match the PR description, the CLI verb wiring ( |
1 participant
Closes #2352.
The gap
The service already has both halves of this except one:
DarlingFileSecurity.HardenFilecomputes the correct DACL,IsReadableByOrdinaryUsersverifies the result, andDarlingWorker.TryHardenConfigFiledetects the bad state and prints the exact remediation. What it lacks is authority — re-ACLing a file it does not own needs WRITE_DAC, and taking ownership needs a privilege a virtual service account is not granted.So it logs and continues, which is right (a monitoring service must not refuse to monitor over a permissions problem), and the only thing that ever applied the rule to an existing install was
install-darling.ps1. Anyone who registered the exe by hand — the README's ownsc createpath — was left hand-typing threeicaclslines out of a log message.Seen on a real box today, which is what prompted it:
darling.jsonandlp.secretboth carriedBUILTIN\\Users:(RX)with inheritance unprotected, and every service start had been saying so, precisely and with the right commands, since that box was built.It verifies rather than claims
Every target is re-read after the attempt and reported
SECUREDorSTILL READABLE, and the exit code is 1 if anything is still exposed — so it can gate a provisioning script. "We tried" is not the same statement as "the secret is not readable"; that distinction is exactly what let a permissions call which silently did nothing hide in the field, andTryHardenExportedSecret's own contract already makes it.The one judgement call, made visible
darling.jsonis the only target that keeps an INTERACTIVE read ACE — the Viewer and the CLI verbs run as the operator and must still read it. The.bak-*siblings, the store directory and the DPAPI credentials get none, because nothing reads a backup (#1769). The target list is a table rather than control flow so that policy is readable at a glance.Two deliberate softnesses
darling.jsonis the failure this exists to repair; refusing to run without it would refuse exactly when it is needed. It falls back to the documented store location and hardens the config anyway.Tests
Verb recognition (case-insensitive, and rejecting near-misses like
--harden-file),IsKnownVerbreachability — the #1912 failure where a verb had a full dispatch block, appeared in the help text, and was still bounced as "Unknown option" — help-text listing, and structural pins that Program.cs dispatches it behind a Windows guard, that it re-reads to verify, and that only the live config carries the interactive ACE.The existing reflection pin
IsKnownVerb_ReachesEveryVerbClassifierOnTheClasspicks the new verb up automatically, which is the anti-drift mechanism working as designed.Verification
Builds clean, 0 warnings, matching
dev. The ACL work itself is Windows-only and needs a real filesystem, so CI is the first place it executes.