fix(fs): make Windows admin ACE inheritable

[DavSanchez]

Windows: files left inaccessible after an upgrade

What was broken

On Windows, upgrading Agent Control could leave files under its managed data root with permissions it couldn't use. Two symptoms seen in the field:

• the infra sub-agent crash-looped — it couldn't manipulate its own log ("Access is denied"), despite running as the highest-privilege system account;
• decommissioning a sub-agent failed — Agent Control couldn't delete the sub-agent's stored remote config ("Access is denied", os error 5).

Root cause

Every Windows file carries a DACL — the list of "who may touch this." Agent Control protects its folders so only Administrators (which includes the LocalSystem account it runs as) may access them, and a file created inside normally inherits that entry.

The bug: Agent Control stamped the folder's Administrators entry as non-inheritable. When Windows recomputes an inherited-only child's DACL and finds nothing to inherit, it leaves the child with an empty DACL — which denies everyone, including SYSTEM.

This only bites on upgrade, and exactly once:

flowchart TD
    subgraph FRESH["Fresh install — safe"]
        direction TB
        A1["folder protected FIRST<br/>(non-inheritable entry)"] --> A2["THEN sub-agent creates log"]
        A2 --> A3["log gets its OWN explicit entry<br/>✅ survives later re-protect"]
    end
    subgraph UPGRADE["Upgrade to a protecting version — breaks"]
        direction TB
        B1["old version: folder not protected<br/>log INHERITS access from parent"] --> B2["upgrade protects folder<br/>(non-inheritable, first time)"]
        B2 --> B3["Windows recomputes child:<br/>nothing to inherit → EMPTY DACL"]
        B3 --> B4["❌ nobody allowed, incl. SYSTEM<br/>(crash loop / can't delete)"]
    end

Loading

Restarting the same version changes nothing — a healthy file stays healthy, an emptied one stays empty. The damage happens only when a protecting version first meets files created before the folder was ever protected.

The fix — two parts

1. Make the entry inheritable. The folder's Administrators entry is now marked inheritable, so files created inside inherit access instead of ending up with an empty DACL. This prevents the bug on fresh installs and future upgrades.

2. Repair machines an upgrade already broke. Part 1 only affects newly-created files — it can't heal files an upgrade already emptied, and Agent Control never re-protects an existing folder. So on startup, Agent Control now walks its entire managed data root (sub-agent filesystem, stored remote configs under fleet-data, local data) and re-stamps any entry that doesn't already grant the access it needs — empty/unreadable, or missing a required right (e.g. an older Administrators:(R,W) with no delete, or a non-inheritable directory). Conforming entries are left untouched, so a healthy install isn't rewritten on every boot. Agent Control owns these files, so the rewrite succeeds even on an empty DACL, and grants read/write/execute and delete — fixing both the crash loop and the decommission failure. If any managed entry can't be repaired, Agent Control aborts startup rather than run on a data tree it can't fully access.

flowchart LR
    S["Agent Control start"] --> R["Recursive repair over whole data root:<br/>re-stamp entries lacking needed access;<br/>skip conforming ones"]
    R --> L["sub-agent log → openable ✅"]
    R --> F["stored remote configs → deletable ✅<br/>(decommission works)"]
    R --> D["data / local-data → accessible ✅"]

Loading

Repair decisions are logged at debug (each root, each re-stamp) and trace (per-entry reason: empty / NULL / unreadable / missing right), so an affected machine records exactly what it healed.

Security posture — unchanged

Files stay Administrators-only with a protected DACL; no new principal gains access (SYSTEM qualifies because it is an Administrator). The fix only restores the delete/execute/inheritance bits SYSTEM needs to manage files it already owns.

I checked Confluence for the rationale behind the original restriction: there is no DACI or design doc reasoning about the Windows ACL specifics. The explicit-filesystem spike that introduced this layout is written purely in POSIX mode terms and never addressed the Windows ACL model — which is the root of this bug. The documented intent everywhere it appears (the installer grants SYSTEM Full Control; Linux configs are 0600) is "owner/admin-only, deny others," which this change preserves.

Verification

• Reproduced on a clean VM: 1.17.0 (works) → 1.18.0 (breaks); the resulting permissions matched a real affected machine byte-for-byte. Detail in NR-601065.
• Field-confirmed: a Part-1-only binary healed the log but not the stored remote configs, so decommission still failed with os error 5. That is why repair is scoped to the whole data root, and why its predicate is "grants the required access" rather than merely "non-empty" — one fleet-data config had a healthy-looking Administrators:(R,W) with no delete, written by a much older version and never rewritten.
• End-to-end validated on a clean Windows VM with the fixed build, across the version bisection and a full fleet lifecycle (ACL reports + logs attached to NR-601065):
  • 1.17.0 (before the explicit-filesystem work, feat: new explicit filesystem #2615 / Feat/filesystem persistence #2618): healthy, empty-DACL count 0 — baseline.
  • 1.18.0 (ships the explicit filesystem feat: new explicit filesystem #2615 and its persistence follow-up Feat/filesystem persistence #2618): breaks — the per-agent dir flips to protected + non-inheritable (PAI(A;;0x13019f;;;BA)), collapsing the log / data / user_data to empty DACLs (empty-count 3). The infra sub-agent crash-loops (Can't open log file … Access is denied) and ephemeral cleanup fails (deleting …\nr-infra\config: Access is denied (os error 5)).
    • Which PR surfaced what: feat: new explicit filesystem #2615 added the per-agent explicit-filesystem layout whose dirs get the protected, non-inheritable Administrators ACE — that's what surfaces the crash loop (inherited-only children collapse to empty DACLs). Feat/filesystem persistence #2618 then (a) added DELETE to that ACE (0x12019f→0x13019f, still non-inheritable/protected — the exact mask above) and (b) introduced the on-startup reconciliation + decommission cleanup, the deletion path that surfaces the os error 5 on empty/no-delete DACLs. The protected + non-inheritable hardening itself predates both.
  • Fixed build: startup repair flags and re-stamps each insufficient entry (repairing managed permissions on …\newrelic-infra, its integration/logging subdirs, and legacy fleet-data\agent-control\*) and leaves conforming ones (managed permissions intact, skipping). Empty-DACL count → 0; the per-agent dir is now PAI(A;OICI;0x1301bf;;;BA) → icacls (OI)(CI)(M); the infra sub-agent starts and reports healthy. Zero Access is denied / os error 5 in the entire log.
  • Decommission (remote config removing the infra agent): AC deletes fleet-data\nr-infra\{remote_config,instance_id}.yaml and the filesystem\nr-infra + packages\nr-infra trees with no os error 5 — the exact operation that failed pre-fix. Post-state: those paths gone, empty-count 0.
  • Recommission (remote config re-adding it): the trees are recreated with (OI)(CI)(M) (children inheriting Modify) and the infra sub-agent returns and sub_agent_became_healthy.

Tests

fs-crate unit tests on the windows-latest leg of unit-docs-onhost-integration-tests, run as admin (required to set the ACLs they assert on):

• child_created_in_managed_directory_inherits_admin_access — Part 1; fails on unfixed code, which the regression throwaway PR [DO NOT MERGE] Verify Windows ACL regression test fails on pre-fix code #2763 demonstrates.
• recursive_repair_heals_existing_empty_dacl_child_from_older_version / ..._nested_empty_dacl_directory_and_file — repair restores access + openability on emptied files and directories.
• repair_restores_delete_so_managed_tree_is_removable — repair restores DELETE so remove_dir_all succeeds (decommission).
• repair_reinstates_delete_on_read_write_only_file_from_older_version — the Administrators:(R,W)-with-no-delete field case.
• permissions_need_repair_flags_only_broken_entries — detection gate leaves conforming entries alone, so healthy trees aren't re-stamped.
• repair_makes_a_non_inheritable_managed_directory_inheritable — a non-inheritable managed directory is flagged and re-stamped inheritable, so future runtime children inherit access.

Also in this change (internal, no behavior change)

win_permissions is migrated from the raw windows-sys bindings to the higher-level windows crate (already a workspace dependency, used by agent-control and self-replacer). Win32 calls now return Result, so error handling is ? / .ok()? instead of manual == 0 + GetLastError, and FFI failures are logged with the OS error message. The DACL check is now a pure predicate (grants_managed_admin_access) driven by .any() rather than an imperative scan loop, and the whole ACL story — stamping, the check, the recursive repair, and the multi-root startup entry point (ensure_managed_permissions) — lives in fs::win_permissions (the on-host runner just calls it and maps the error). fs no longer declares windows-sys directly.

Follow-up tracked in NR-601920: unify the repo's mixed target_os/target_family cfg gating (deliberately out of scope here).

[paologallinaharbur]

I would not exit in case of error with permissions, there is the risk of causing a deadlock of the application in which we are not able to update because of this, no?

[DavSanchez]

I see your point! I initially erred in the side of caution (just quit if we cannot ensure proper operation, but it's true that the self-upgrade can work as a healing path. I'm updating it to not stop AC but log any failures as warnings.

[DavSanchez]

This has been changed to not bail out on failure. This now logs a report of the operation outcome and continues the startup.

[gsanchezgavier]

Looks good to me , i just left a question. On the other hand i think we took a complex path by setting this permissions in AC and we should leverage the permissions inheritance of root folders that are set at installation time, and we are hitting that complexity with things like this bug.

[gsanchezgavier]

is this matching to what we set in the installation ps1?

[DavSanchez]

Matches to a sub-set of it. The PowerShell script sets Full Control (F), here we use a least privilege approach and use only Modify (M).

[gsanchezgavier]

i think it would be ideal if we have a FF to control this kind of behaviour. We could activated just for exiting customers for instance. Out of scope of this fix for sure

[sigilioso]

I've left some nits that can be perfectly ignored. On top of that, I'd take a look at the logs/Errors style conventions (there are some lowercase/uppercase inconsistencies).

The fix looks good to me but I'd address @paologallinaharbur comment regarding existing on error before merging.

[sigilioso]

Nit: I'd try to place helpers together.

[DavSanchez]

Done.

[sigilioso]

Nit: I'd try to make the Changelog message shorter

[DavSanchez]

Done.

[sigilioso]

Nit: do we need this to be public? (same for ensure_permissions_recursive)

[DavSanchez]

Done.

[sigilioso]

Nit: Considering this, would it make sense to use io::Error::other directly?

[DavSanchez]

Done.

[DavSanchez]

[...] I'd take a look at the logs/Errors style conventions (there are some lowercase/uppercase inconsistencies).

[...] I'd address @paologallinaharbur comment regarding existing on error before merging.

Both done!

[DavSanchez]

Fixed conflicts

[github-actions]

📦 Binary size

Binary	main	PR	Δ
newrelic-agent-control	21.92 MB	21.93 MB	⬆️ +0.00 MB (0.01%)
newrelic-agent-control-cli	13.66 MB	13.66 MB	⬇️ -0.00 MB (-0.00%)

[DavSanchez]

📦 Binary size

Binary main PR Δ
newrelic-agent-control 21.92 MB 21.93 MB ⬆️ +0.00 MB (0.01%)
newrelic-agent-control-cli 13.66 MB 13.66 MB ⬇️ -0.00 MB (-0.00%)

Now that I see this @vjripoll maybe it could be useful to have more rows to specify per OS/arch, because I added code that is Windows-only it won't be reflected here.

[sigilioso]

I think logs should begin with a capital letter according to the style guide.

I've left a comment as example, but I'd take a look at all logs.

Everything else looks good!