feat(config): make Windows cache-dir DACL hardening configurable (env var + CLI config key)

[roosteer]

Summary

Closes #1624 (fix option #2 from #1620).

Makes the Windows cache-directory DACL hardening configurable via two opt-out surfaces:

1. Env kill switch CBM_SKIP_DACL_HARDENING=1 — effective from the very first run, before any config store exists.
2. Persisted config key windows-dacl-hardening (default true) — settable via config set/reset, visible via config list. Effective from the second run (the store _config.db lives inside the cache directory, so it does not exist when the first run creates that directory).

Precedence: env > config. When hardening is disabled, owner validation of the cache directory stays active in every path; the default behavior is unchanged.

What changes when disabled

Site	Behavior
cbm_windows_stamp_dir_owner (creation stamp)	Owner stamp kept, DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION dropped → OS-default inherited DACL
win_runtime_directory_secure (per-start re-application)	Re-protection skipped; a directory hardened by a previous run has inheritance restored (SE_DACL_PROTECTED cleared, parent-inherited DACL applied) — self-healing, no manual icacls
win_file_acl_secure (validator)	Accepts the OS-default inherited DACL instead of requiring the one-ACE owner-only form (both write and validate sides gated together to avoid fail-closed startup)

Decisions and tradeoffs

D1 — Flag threading: process-global accessor, not a signature change. The stamp site lives in foundation/ (reached from cbm_mkdir_p, ~25 call sites) while the harden site lives in daemon/; threading a bool through both would touch two definitions plus ~9 callers and still could not reach the stamp without going through cbm_mkdir_p. Instead a static atomic_int g_dacl_hardening = -1 + accessor, mirroring the existing cbm_mem_profile_enabled idiom.

D2 — Config flows via an explicit setter, not the accessor. The accessor resolves the env var only, so it stays dependency-free inside the stamp path (which runs before any config store exists). A lazy config read inside the accessor would recurse: cbm_config_open → mkdirp → stamp → accessor. main_build_identity reads the config once (env unset) and stores it via the setter.

D3 — Skip + unprotect (self-healing), not skip-only. The issue's literal proposal (skip the protection) leaves an already-hardened directory broken — the user would still need a manual icacls /inheritance:e, contradicting "config set false → index → indexed". The opt-out therefore also restores inheritance. Important implementation detail: SetSecurityInfo with DACL_SECURITY_INFORMATION and a NULL pDacl creates a NULL DACL (full access to everyone) and would also make the relaxed validator fail closed. The unprotect therefore copies the parent directory's DACL (the ACE set a freshly created directory inherits) and clears SE_DACL_PROTECTED — the same end state as icacls /inheritance:e.

D4 — Global stamp skip, runtime-dir-only unprotect. The stamp skip is global (all dirs created via cbm_mkdir_p); other directories do not use rename-replace, so skipping their DACL protection has no functional effect. The unprotect is naturally runtime-dir-only.

D5 — Two-phase activation timing (documented, not worked around). Env from run 1; config from run 2. A single-run config opt-out is impossible by construction (the store lives inside the boundary being protected) and is documented in docs/CONFIGURATION.md.

D6 — Verification strategy. Wine (the only local Windows runtime) does not implement real ACL semantics: SE_DACL_PROTECTED is not honored, so the unprotect unit test asserts meaningfully only on real Windows (fork CI msys2 legs). Local gates: clang-format-20 + cppcheck, linux/amd64 test+build, mingw cross-compile, Wine test-runner and version check.

D7 — Unit tests. Three Windows-only tests in tests/test_daemon_ipc.c: env accessor contract, stamp-skip (inherited DACL + exact owner), and relaxed-validator + unprotect (previously hardened directory accepted, SE_DACL_PROTECTED cleared, resulting DACL non-NULL and valid).

Security tradeoff. The protected DACL defends the cache/IPC boundary against other local accounts. Disabling it is intended for single-user hosts where the protected DACL breaks MoveFileExW rename-replace (EDR/minifilter conflicts); it is not recommended on multi-user/terminal-server hosts. Default remains true; owner validation is never disabled.

Verification

• ./test-infrastructure/run.sh lint — pass (clang-format-20 + cppcheck 2.20.0)
• ./test-infrastructure/run.sh amd64 — build pass; 5 test failures are pre-existing and base-identical (container runs as root vs /src owned by uid 1000 → activation-transaction ancestor check; vendored-integrity drift) — verified by running the same leg on the base commit
• docker compose -f test-infrastructure/docker-compose.yml run --rm test-windows — mingw cross-compile pass; Wine daemon_ipc suite shows no regressions vs base (2/3 new tests pass under Wine; the third requires real ACL semantics, per D6)
• ./test-infrastructure/run.sh windows — production mingw build + Wine version check pass
• Real-Windows DACL proof: fork CI msys2 Windows legs on the final SHA

Test plan for reviewers

• config set windows-dacl-hardening false → index any repo → status:"indexed" (on an affected host; regression-check on a normal host that default behavior is unchanged)
• CBM_SKIP_DACL_HARDENING=1 overrides true in config
• With the option off, daemon startup accepts the inherited DACL and no re-protection is applied

[DeusData]

Thank you for this, and especially for the two details that show you understood the constraint rather than working around it: the env kill switch working from the very first run (the config store lives inside the directory being hardened, so a persisted key alone cannot help someone who is locked out on run one), and keeping owner validation active in every path when hardening is off. Those are exactly the two things that would have made a naive version of this unsafe.

Your #1620 report is also the best-evidenced thing in the tracker this week — the background icacls /inheritance:e loop that made the identical index succeed is a controlled proof, not a guess.

I need to be straight with you about how this will be handled, because it is a policy question rather than a code review.

Making security hardening opt-out is a maintainer decision, not something I will merge on technical merit alone. The protected-DACL behaviour was a deliberate choice, and an escape hatch changes what a default install guarantees — including for people who never read the flag. That belongs to the project owner, and I will put it in front of them with your evidence attached rather than quietly deciding it.

There is also a live alternative worth weighing against yours: the re-stamp currently fires on every process start, whether or not anything is wrong. That is what produces the rewrite window your MoveFileEx publish loses to. Making it conditional would remove the churn without introducing an opt-out at all. I had a change doing exactly that, and CI caught it breaking Windows daemon startup — I had conflated "the DACL has no untrusted grants" with "the DACL is protected against inheritance", which are not the same property. So that path is real but not free, and your PR may well be the better answer; I do not want to present it as obviously preferable when my own attempt at it just failed.

Two things I can do now: your work is queued behind a v0.10.5 that fixes a batch of install and startup blockers (CI is badly backed up tonight), and #1628 — which makes an atomic-publish failure report the actual Win32 error instead of Pipeline failed. Check repo_path exists — is in that release. That will not fix your host, but it stops the next person being told their repository is the problem.

I will come back with a decision rather than leaving this open indefinitely.

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

[junk151516]

Third-party data point, not a position on the policy question. In #1620 the maintainer wrote that it would "genuinely help to know whether [CBM_RUNTIME_DIR] unblocks your host", since the argument for this flag rests on whether a normal Windows machine has any reachable location that legitimately passes. I measured that on a different host today (Windows 11 Pro, v0.10.6, windows-amd64), so here it is.

Short answer: on this host, yes — and the failure it replaces is now legible.

A directory under the user profile passes cleanly:

CBM_RUNTIME_DIR=C:\Users\<user>\.cbm106rt   -> daemon starts, all CLI tools work

A world-writable ancestor is refused, and the refusal names the exact ACE:

CBM_RUNTIME_DIR=C:\tmp
codebase-memory-mcp: secure CLI coordination could not be created (endpoint):
  C:\tmp: DACL entry 3 grants mutation rights 0x00010112 to untrusted identity
  (Authenticated Users S-1-5-11)

Worth saying plainly since #1620 was closed on the diagnosis half: that message is a large improvement over what sent that reporter chasing "check repo_path exists". It names the entry index, the rights mask and the identity, so the operator can fix it or relocate without guessing.

What I would not conclude from this: that #1649 is unnecessary. My host is a personal machine where I control the profile ACLs. A domain-managed machine whose profile tree carries an inherited grant to some administrative or backup group would fail the same check with no operator-reachable alternative, and that is precisely the case the flag exists for. One passing host does not establish that every host has a passing location — it only kills the strongest version of the objection, which was that maybe none do.

A use case for CBM_RUNTIME_DIR that may not have been in the design intent, offered because it made the difference between testing a release and not testing it: it is what finally makes version evaluation safe. We pin a production binary and keep ten indexed projects; when we trialled v0.9.1-rc.1 with CBM_CACHE_DIR alone, the isolation leaked — a daemon landed in %LOCALAPPDATA% and touched the default store, and we found all eleven .db files with empty DACLs, unreadable by their own owner (same class as #1351; list_projects returned an empty list rather than an error, which reads as "nothing is indexed" and invites a pointless full reindex).

Today, with CBM_RUNTIME_DIR + CBM_CACHE_DIR both set, I indexed a project on v0.10.6 and then diffed the production store: 33 files, byte-identical sizes and mtimes, ACLs untouched, %LOCALAPPDATA% unchanged, and the temporary daemon created inside the isolated runtime directory where it belonged. That is the first time we have been able to evaluate a candidate build without risking the graphs we use daily — which, whatever is decided about the opt-out, is worth documenting as a supported pattern.

[DeusData]

I owe you the policy decision I promised, and I am going to hold it back deliberately — because two findings block this regardless of how it goes, and a third may make the question moot. Let me take them in order of consequence.

First, what is right here. When hardening is off you restore inheritance by copying a real parent ACL rather than passing a NULL pDacl — which would have created a NULL DACL granting Everyone full access, the classic way this exact refactor goes wrong. The test asserts dacl_valid_after for precisely that. The env switch working on run one is also correct, since _config.db lives inside the directory being protected and a config-only knob could never take effect the first time. Those are the details that told me this was written carefully.

---

Blocking finding 1: the opt-out is much wider than the PR describes.

The description says the opt-out relaxes the owner-only protected DACL to "the OS-default inherited DACL". The code does more than that. The early return you added to win_file_acl_secure is:

if (!cbm_windows_dacl_hardening_enabled()) {
    ...
    return secure;
}

and at that point secure means only "GetSecurityInfo succeeded, a DACL is present, IsValidAcl passed, and GetAclInformation succeeded." The return happens before the ACE scan. So with hardening off we do not accept "the inherited DACL" — we accept any structurally valid DACL, including one whose ACEs grant Authenticated Users or Everyone write access. And because win_file_security_secure runs this validator on ancestors too (ipc.c:4477 passes ancestor=true), that applies up the tree.

Concretely: the C:\tmp refusal that @junk151516 praised in this thread — the one naming the ACE index, rights mask and identity — would no longer fire. That refusal is the ACE scan.

The PR repeats that "owner validation stays active", and that is true. It is also not a substitute: ownership constrains who owns the object, not what a third principal holding a write ACE may do to it. A world-writable cache directory owned by you is still world-writable.

If the intent really is "accept the inherited DACL but still refuse untrusted mutation grants", then the early return needs to sit after the ACE scan and relax only the one-ACE shape requirement. That is a much narrower and much more defensible change, and I would look at it very differently.

Blocking finding 2: this breaks lock adoption.

private_win_owner_only_dacl in src/foundation/private_file_lock.c:892 requires (control & SE_DACL_PROTECTED) != 0, and it gates lock adoption at three call sites (lines 1054, 1072, 1159). win_file_dacl_is_owner_only in ipc.c:4199 demands the same. Your opt-out actively clears SE_DACL_PROTECTED on the runtime directory — and touches neither predicate.

This failure mode is already documented in the tree, at ipc.c:4190:

...would skip the re-stamp and then strand every subsequent lock adoption (observed as 59 daemon-suite failures on a fresh runtime dir whose inherited DACL carried SYSTEM+Administrators).

That is the same conflation I described hitting from the other direction when my own attempt broke Windows daemon startup: "no untrusted grants" and "protected against inheritance" are different properties, and the lock layer needs the second one. Your CI could not catch it — the Windows legs are cancelled, and the three new tests are #ifndef _WIN32 PASS();, so on macOS and Linux they are vacuous. Our own Windows VM is arm64, and Wine does not honour SE_DACL_PROTECTED at all. So the RED-without-fix evidence for this change is currently unverified anywhere, which is worth knowing before either of us trusts a green.

---

Third — and this is why I am not answering the policy question yet: the motivation may have shrunk.

The alternative I floated on 14 August has since landed. ipc.c:4430 now has an already_correct fast path: when the owner is exact and the DACL is already owner-only, the re-stamp is skipped entirely. Its comment cites #1601 (eleven no-op "Security change" USN records against one _config.db in a day) and your #1620 by name.

The unconditional per-start rewrite that this PR exists to escape should therefore already be much rarer on current main than it was when you measured. Before I decide whether a security default may become opt-out at all, I want to know what the problem looks like now. Could you re-measure on the affected host against current main? If the fast path has taken the rewrite window down to nothing, the honest answer is that neither of us needs to make this decision.

@junk151516 — thank you for measuring on your own machine and, more importantly, for arguing against over-reading your own result. Pointing out that one passing host only kills the strongest objection, and that a domain-managed profile tree with an inherited administrative grant would still fail with no operator-reachable alternative, is exactly the right way to offer evidence. I have not treated it as settled, but it is on the record and it is the scenario I will test against.

Two housekeeping items whichever way this goes: DCO is red because the single commit carries no Signed-off-by (git commit --amend -s), and the commit author is rv <raffaele.verde2@gmail.com>, which does not obviously map to your GitHub account — worth confirming that is you. The branch also conflicts with main, partly because a get_security_descriptor_control member you add to win_security_t now already exists there.

[junk151516]

Two small corrections on the CI evidence, both of which sharpen rather than weaken your point.

"The Windows legs are cancelled" isn't quite it — three Windows jobs ran and passed on this head (2d23632):

job	conclusion	duration
pr-smoke (windows-latest)	success	21m45s
test / test-package-wrappers (windows-latest)	success	1m30s
test / test-windows-guards	success	10m2s
test / test-windows	cancelled	0s

Only test-windows was cancelled — and that is the one leg that would have executed the bodies of the three new tests. So your conclusion holds exactly as stated, but putting it precisely also closes the reading that "Windows CI is green here", which is what someone skimming the checks page will otherwise take away from a security change.

Second: that run is from 14 Aug and main last moved on 20 Aug. Whatever those three greens meant, they do not describe the current tree — which is also why the branch conflicts.

On the substance I have nothing to add. I checked both blocking findings against the tree and they reproduce:

• the ACE scan starts at ipc.c:4121 and the early return lands right after the secure assignment at 4113, so it does return before the scan — and win_file_security_secure(..., ancestor=true) at 4477 carries that up the tree;
• private_win_owner_only_dacl (private_file_lock.c:892, SE_DACL_PROTECTED checked at 907) gates adoption at 1054, 1072 and 1159, and this PR does not touch that file at all.

The comment already in the tree at ipc.c:4190 states the same distinction the finding rests on, including the 59-failure observation, so the tree is not silent about it either.

[junk151516]

Pointer, since the maintainer asked here for a re-measurement on current main: the #1620 reporter posted it on the closed issue on 3 Sep — v0.10.8, no daemon, fresh cache, still deterministic, so not the re-stamp race #1685 removed. I added a control from an unmanaged host under the identical protected owner-only DACL where the same MoveFileExW probe passes in all variants (#1620 (comment)). Relevant to the policy question: on that host the failure is a managed-host layer, not the DACL shape, and the silent branch is the writer's first rename in sqlite_writer.c:1787, which discards the errno cbm_rename_replace already translates.

[DeusData]

Thank you for connecting the requested re-measurement back to this PR.

The new evidence materially changes the diagnosis: the v0.10.8 failure persists with no daemon and a fresh cache, while the same protected owner-only DACL permits the native rename on an unmanaged control host. Current main also still has the first writer publication rename return ERR_WRITE_FAILED without logging the translated errno, so the observability gap is real.

This PR still changes a local security boundary and the earlier lock-adoption concerns remain unresolved, so we need more time to review the policy and implementation. I am treating the fresh #1620 evidence as a reason to reconsider that closed issue separately, not as proof that this opt-out is safe as written. Thank you both for the controlled comparison.