The pre-lock check trusts the install's own service SID and an existing install gets exact steps (#5627)

[erikdarlingdata]

Claude posting for Erik Darling.

Fixes #5627.
Fixes #5629.

Why

An upgrade over a 3.7.1 install was refused with a long list. The same happens on a re-run over a 3.9.0 install. The list has one line for the folder itself. It then has about 1,740 lines like <path> (owned by NT SERVICE\PerformanceMonitor Darling). Those lines are two different findings.

• Real, kept. The folder sits directly under C:\. It inherits a write grant from the volume root. That is Authenticated Users on the runner images and BUILTIN\Users in the field report. It is 1 line.
• False, fixed. The product's own service account owns the pg-runtime files its service extracted. That is by design. It was listed 1,742 times on the repro runs. The check listed it whenever the trusted set at step 1a lacked the service's SID. That happens when the service was deleted since 3.7.1 (shape D1). It also happens when the logon was changed to another account (D2). The trusted set only ever held the service's current logon account, and only while the service was registered.

Repro numbers come from windows-2022 and windows-2025, runs 37922364817 and 37922938743:

• Service registered on its default account: 1 finding, the root grant.
• Service deleted or on another logon: 1,745 findings. That is the root grant, 1,742 service-owned objects and darling.json.
• With this fix, every one of those shapes lists only the root line.

The old advice also did not say what happens to the service, darling.json or the store.

What changes

In Darling/tools/install-darling.ps1 and Darling/tools/upgrade-darling.ps1. The shared functions stay byte-identical, and the existing identity test covers them.

• Get-DarlingServiceSids takes the service name and the current logon account. It returns the SID of NT SERVICE\<serviceName> and the logon account's SID. Get-DarlingPreLockTrustedSids is the base set and names no service account.
• Get-DarlingPreLockTrustedSidsForRerun passes the name on both paths, whether or not the service is registered. The L3 and L4 refusals (logon cannot be read or resolved) are unchanged.
• The install folder checks (install 1a and the upgrade's install-dir check) go through the re-run function and pass the service SIDs to Get-UntrustedWriteGrantees -ServiceSids. The upgrade's -Source folder check and its SHA256SUMS folder check use the base set only (see Review round 1).
• Only this product's service name is derived. No other NT SERVICE\* SID is trusted, and neither is the authority as a whole. A tree owned by a different service's SID is still listed.
• One deliberate difference from the ruling. The SID is computed from the name. It is the SHA-1 of the upper-cased name in UTF-16LE. The first 20 bytes are read as five little-endian integers under S-1-5-80. That is what sc.exe showsid prints. The ruling used NTAccount.Translate instead.
• Why: on a machine with no service of that name, Translate of NT SERVICE\<name> fails with "Some or all identity references could not be translated". That is exactly the deleted-service shape. The runner repro translated only because the service had existed minutes earlier.
• The computed value equals the Translate result for registered services. I checked Winmgmt and EventLog, and a test pins it. It also equals the SID the repro recorded for this product's name (...-944500918). If the computation throws, the SID is left out and the walk names what it finds, as before.
• The refusal for an existing install now gives numbered steps. An existing install is one whose registered service runs from the refused folder. The steps come from Get-DarlingExistingInstallSteps, with the same text in both scripts.
  1. Stop the service.
  2. Extract the new zip into a new, empty C:\Program Files\PerformanceMonitorDarling.
  3. Check that every setting in darling.json is yours, then copy it first, or the sample config is copied in. If it names a certificate or key file in this folder, make a new one. Keep it outside the install folder and point darling.json at it. Do not copy a darling-keys folder.
  4. Run install-darling.ps1 from the new folder. It re-points the existing service. Only the program path changes. The logon account, credentials and the store under C:\ProgramData\PerformanceMonitorDarling are untouched.
  5. Start the service and check that it collects.
  6. Delete the old folder. The step lists any paths darling.json names inside the old folder. It says to keep that folder until they are moved.
• The upgrade script's install-dir refusal uses the same choice: the steps when the registered service runs from the refused folder, the fresh-folder wording otherwise.
• Darling/README.md gets one sentence group naming the steps.
• I ran the rendered refusal text through the plain-English checker and fixed the hits in my lines. The hits left are in sentences that were already there.

There is no bypass, no new switch and no automatic permission repair.

The two findings

Finding	Class	Before	After
Root grant inherited from C:\	real	1 line	1 line, unchanged
<path> (owned by NT SERVICE\PerformanceMonitor Darling)	false	1,742 lines with the service deleted or on another logon	0
NT SERVICE\PerformanceMonitor Darling ACE on darling.json (3.7.1 step 4b)	false	1 line, same cause	0

Repairing the folder in place: options

This PR does not decide it. It keeps option (a).

• (a) Keep refusing, as now.
  • Proves: the folder was not safe to keep when the check ran. The user lands on a folder whose contents came from the zip.
  • Cannot: spare the user a move for a folder that is fine. It also does nothing for someone who cannot move the folder.
• (b) An opt-in switch that locks the tree and then verifies every shipped file against hashes from the zip.
  • Proves: each file the zip ships matches what the zip shipped, after the lock.
  • Cannot: say anything about files the zip does not ship, such as darling.json or files the service writes. It also needs the hash list in the zip to be trusted.
• (c) Lock and continue with a warning.
  • Proves: only that writes stop from now on.
  • Cannot: say anything about what is already in the folder. It is the weakest option and is close to what -AcceptWritableExtraction already does.

Upgrade-shape check

This PR also adds a permanent check, .github/workflows/darling-upgrade-shapes.yml, so the install and upgrade scripts are run over installs that real releases made. Until now nothing in CI ran either script over an older install.

What runs: each leg downloads a released zip and runs that release's own install-darling.ps1. It lets the service extract pg-runtime (it waits for pg-runtime.stamp), stops the service, and then runs this commit's script. Every leg runs on windows-2022 and windows-2025. A leg fails the job with a one-line reason when its expectation is not met.

Shape	Setup	Script	Expected
U1	3.9.0 in C:\Program Files\PerformanceMonitorDarling	install, and upgrade (separate legs)	passes
U2	as U1, with the service logon changed to a non-default local account, and started once on it	install, and upgrade	passes
U3	as U1, with the service deleted	install	passes, and the service is registered afterwards
U3	same	upgrade	exits non-zero, says the service is not installed (without -InstallRoot) and not registered (with it), and lists no service-owned files
U4	3.7.1 in C:\PerformanceMonitorDarling	install	refuses, lists exactly one entry (the root folder's), lists no service-owned file, names C:\Program Files and darling.json

When it runs:

• On a pull request that touches Darling/tools/*.ps1, the workflow file or its script (pull_request, never pull_request_target, with contents: read).
• In the nightly, as one job (uses: ./.github/workflows/darling-upgrade-shapes.yml) gated like the other nightly jobs. It is not in publish's needs.
• On workflow_dispatch.

The release versions (3.9.0 and 3.7.1) are named once, in the workflow's top-level env. Every checkout uses github.sha. Each job has a 30-minute timeout, and concurrency cancels a superseded run on the same ref.

Time, measured on this PR's own run (run 37927343702, head 0cc327f81): all 14 legs passed. The run took 2.2 minutes wall, and the longest leg took 2.2 minutes (windows-2022 / U2-install). The shortest took 1.6 minutes. The 30-minute timeout is for a hung service, not for a slow leg.

What the first run found: the first run (head 22ad9a466) failed 8 legs for two reasons, both fixed here.

• upgrade-darling.ps1 run from a staging folder stopped with "No -Source given". Windows PowerShell 5.1 leaves $PSScriptRoot empty inside the param() default of a [CmdletBinding()] script started with -File (reproduced locally with a four-line script). The script now applies the default in its body. A pin in DarlingUpgradeShapesWorkflowTests holds both lines.
• A passing leg returned the exit code of its last native command. The script now ends with exit 0 after every expectation passes.

Guards: DarlingUpgradeShapesWorkflowTests is new, with 6 tests. It pins the trigger paths and pull_request (not pull_request_target). It pins the four shapes and both OS labels, and the leg list matching the script's ValidateSet.

It pins read-only permissions, timeouts, concurrency, github.sha checkouts, action refs on a release line and the versions named once. It also pins the nightly job, its gate and its absence from publish's needs. DarlingPathFilterGateTests gained a pin that the Darling gate lists the workflow and its script, and the gate file lists both. RepoFileAdoptionTests lists the new class as an LF reader.

Red-plant: I edited the workflow four ways. Each time DarlingUpgradeShapesWorkflowTests failed 1 test. The edits were dropping windows-2025, dropping the Darling/tools/*.ps1 trigger path, dropping U4-install, and using pull_request_target. I restored the file after each.

Release checklist

The release checklist (.claude/skills/release-checklist/SKILL.md) had no Darling service install or upgrade step, which is how this shipped. A new section, 8a, now requires four things for every release:

• The upgrade-shape CI job. All 14 legs of darling-upgrade-shapes.yml must be green at the release commit's sha, by name. The two pinned release versions in the workflow are checked too. If there is no run at that sha, start one by dispatch.
• upgrade-darling.ps1 over a real install, run once from the last release and once from the oldest release still supported. The oldest supported release is the oldest migration-ladder-v<version>.sql fixture in Darling/Darling.Tests/Fixtures, so the section names the rule and no version. Pass means a clean script run and a service that is collecting. The store must be at the top rung with the earlier rows still there. The five store checks the script prints must hold. The section says what the *RungTests classes, MigrationUpgradeLadderLiveTests and DarlingStoreUpgradeTests already cover, and what only a live run adds.
• A fresh install-darling.ps1 run on a clean machine, then uninstall-darling.ps1.
• The Linux tarball and the container image. Linux has no install script, no upgrade script and no unit file in the tarball. There is no scripted upgrade path. The section says so. It lists the by-hand fresh-install and upgrade checks for the compose stack and for the systemd shape from the README.

The scope note no longer reads as if no service install testing happens. This part is docs only. No test reads the file except the repo-wide identifier scan, and I checked the new text against that scan's two patterns by hand: no match.

Review round 1

• The -Source folder check and the SHA256SUMS folder check in upgrade-darling.ps1 use $sourceTrusted, the base set with no service account. A service-owned file or write grant in a source folder is a finding.
• One list of the service-written places, Get-DarlingServiceWrittenNames (pg-runtime, pg-runtime-prev, darling-keys, darling.json and darling.json.bak-*). Lock-DarlingInstallTree and the pre-lock walk both read it. The functions are byte-identical in both scripts and pinned.
• The service's own account is trusted as owner and as write grantee only at or under that list, using Test-PathIsAtOrUnder. A file it owns at the root, beside the exe or in any other folder is named.
• Step 3 asks the admin to check every setting in darling.json before copying it, then to go straight on to step 4.
• Before printing the steps the script reads darling.json. It names every configured path under the old folder: postgres.dataDirectory, file: references and the certificate and key paths. When there are any, the delete step says to keep the old folder until they are moved. Step 4 says where the store is, including the postgres.dataDirectory case.
• Inside C:\Program Files the steps and the fresh-folder wording ask for a new, empty folder with a different name under C:\Program Files.
• When the service SID cannot be worked out, a one-line warning says so. The computation covers plain ASCII service names only.
• Every refusal text this PR changes (steps and fresh-folder versions, both scripts) went through the plain-English checker. The rendered texts are clean. That includes the older sentences in those texts, which are reworded.
• Runner baseline from the earlier proof: 3 write ACEs (pg-runtime, pg-runtime-prev, darling.json) and 1,752 owner findings, all the service's. The proof's file counts put 1,653 files in pg-runtime plus darling.json outside the zip, and the service writes nothing else in the root. I did not get a per-path list of the 1,752 here. The upgrade-shape run on this head is the check that they all sit under the list. Result: 14 of 14 legs passed (https://github.com/erikdarlingdata/PerformanceMonitor/actions/runs/37930205158).

Review round 2

• The pre-lock check names every write grant the service's own account holds on the install folder. It does so in all three service states: virtual account, other logon and deleted. Only the folders and files on the shared list trust that account.
• The by-hand fix the script prints grants the service read and execute on the install folder (/grant:r ... (OI)(CI)RX). The step 4b2 header comment in install-darling.ps1 says the same.
• The refusal gives the numbered steps only when the registered service's folder is the refused folder. The folder is the service's ImagePath, read by Get-DarlingInstallRootFromService. Full paths are compared without regard to case or a trailing separator. A service registered elsewhere, or none, gets the fresh-folder wording. upgrade-darling.ps1 applies the same rule to an explicit -InstallRoot. Test-DarlingSamePath, Get-DarlingInstallRootFromService and the new Get-DarlingRefusalAdvice are byte-identical in both scripts and pinned.
• Get-DarlingConfiguredPathsUnder reads each value on its own. A value holding a control character, a line break or any of < > | " is left out, and so is one over 260 characters. At most 10 are listed, each in quotes. The last step keeps the old folder when a value is left out, the list is cut or darling.json is unreadable. In those cases it asks for darling.json to be checked for paths inside the folder.
• Step 3 says to make a new certificate or key file when darling.json names one in this folder. The new file goes outside the install folder, and darling.json is pointed at it.
• The service-written list matches darling.json by exact name. Its backups match only darling.json.bak-yyyyMMdd-HHmmss with an optional -N counter. The first name below the install folder is compared exactly. The listed files count only in the install folder itself, in the lock and in the check. darling-keys counts only when Get-DarlingExtraServiceWriteDirectories returns it, and the check is handed the same list as the lock.
• A grantee SID that Windows cannot translate is named by its SID in the findings. It used to print blank.
• The upgrade-shape script checks each release zip's SHA-256 against the release's own SHA256SUMS.txt before extracting. A missing file, a missing or repeated listing or a different hash fails the leg and deletes the zip. Both pinned releases ship that file, so no hash is pinned in the script. A guard test pins the check.
• Every refusal text this round changed (existing-install and fresh-folder, root and C:\Program Files, both scripts) went through the plain-English checker. All 12 renders are clean.

Compose image tag

The compose quickstart pulled the nightly image, even on a release. Darling/compose/docker-compose.yml named performancemonitor-darling:nightly, but since 3.4.0 each release pushes :<version> and :latest, so a release user who followed the README ran the newest dev build.

• The darling service now names :latest. It is not a version pin. The next version's image does not exist until the release ships, so a pin makes the quickstart fail on dev. The comment above the line says the same and still tells people to substitute a version tag.
• The README's "Run on Linux" section says the compose file pulls :latest, how to pin a release, and that :nightly is for testing unreleased changes.
• ComposeImageTagTests reads the real compose file and fails unless the darling service's image is ghcr.io/erikdarlingdata/performancemonitor-darling:latest. A second test runs the same parse over a copy that names :nightly, so a parse that matches nothing cannot pass.
• The release checklist's section 8a, item (d), now says to run the compose check with the release's own image tag (:<version>). Set it in the compose file or an override file. :latest still points at the previous release until the new one publishes.
• Nothing else reads the tag. The only :nightly readers are nightly.yml (it builds and pushes that tag) and a comment in build.yml. The CI jobs that start the stack use the locally built performancemonitor-darling:pr image. The other Compose* tests read other keys of the same file.

Test plan

• DarlingInstallLocationTests: 33 passed, 0 failed. There are 6 new tests.

  • The trust set derives only the product's own service SID, and the steps text is identical in both scripts.
  • The steps text is pinned, and both scripts use it.
  • The 3.7.1 shapes (deleted, other logon, virtual account) are clean. A root write grant is the only line left.
  • A locked 3.9.0 tree passes on the virtual account, on another logon and with the service deleted.
  • A tree owned by NT SERVICE\Winmgmt still refuses, by that name.
  • The derived SID equals Windows' own for a registered service.
  • The new tests shadow Get-Acl, which is the class's existing idiom, so they need no elevated token.

• Red-plant. I disabled the derived-SID block in install-darling.ps1. Both behavior tests above failed, and so did the identity pin because the scripts then differed. I restored it and the class is green.

• The test classes that read either script, plus DocCommentHygiene*, *Census* and *Ratchet*: 816 run, 0 failed, 1 skipped.

• The Stage=Guard set: 1,474 run, 0 failed.

• Build: 0 warnings, 0 errors.

• A runner run of the fix against the D1 and D2 shapes. The repro workflow lives on a separate branch. Expect only the root line.

• The numbered steps end to end on a real 3.7.1 install. Another lane is proving them. I wrote the wording from the code.

• The BUILTIN\Users root grant of the older Server image was not measured. The check's write mask covers both grants.

• Upgrade-shape check, this PR's own run (Darling upgrade shapes, run 37927343702, head 0cc327f81): 14 of 14 legs passed, 2.2 minutes wall, longest leg 2.2 minutes. The first run (head 22ad9a466) failed 8 legs. The cause is described above.

• DarlingUpgradeShapesWorkflowTests (6), DarlingPathFilterGateTests (10), RepoFileAdoptionTests (2): 0 failed. Red-plant: 4 plants, 4 reds.

• After merging this branch with origin/dev, none of these failed. Darling Stage=Guard ran 1,480. Lite Stage=Guard ran 469. *Census* ran 500 (1 skipped), *Ratchet* 15, DocCommentHygiene* 78 and DarlingInstallLocationTests 33. Darling.Tests and Lite.Tests build with 0 warnings. darling-upgrade-shapes.yml and nightly.yml parse with PyYAML.

• The new check was not run against the old scripts (before this fix). The U2 and U3 expectations come from the repro that found the false positives. I did not re-run them on the unfixed scripts.

• Review round 1. DarlingInstallLocationTests: 38 passed, 0 failed. New tests cover six things. The service account is trusted only where the service writes. Listed paths pass. A file at the root or beside the exe is named. The source-build checks trust no service account. A folder holding darling.json with no registered service gets the fresh-folder wording. Configured paths under the old folder are named. The Program Files wording and the SID warning are covered too. The identity pin covers every shared function including the new list.

• Red-plant. Trusting the service as owner everywhere in Get-UntrustedWriteGrantees made the new scope test and the identity pin fail. I restored it and the class is green.

• The test classes that read either script (including the upgrade-shape and path-filter classes): 246 run, 0 failed. *Census*, *Ratchet* and DocCommentHygiene*: 593 run, 1 failed (a displaced doc comment in my test file), fixed and re-run green with DarlingInstallLocationTests (116 run). Stage=Guard: 1,480 run, with the same single failure before the fix. I did not re-run it in full after the fix. Build: 0 warnings, 0 errors.

• A runner run of the numbered steps on a real 3.7.1 install. Not done.

• ComposeImageTagTests (2), ComposeDarlingHealthcheckTests and CiComposeWorkerSizingTests: 9 run, 0 failed.

• Red-plant. I put :nightly back in the compose file. TheDarlingService_NamesTheLatestImage failed. I restored :latest.

• After merging this branch and origin/dev: build 0 warnings, 0 errors. Darling Stage=Guard ran 1,480, 0 failed. *Census*, *Ratchet* and DocCommentHygiene* ran 593, 0 failed, 1 skipped.

• A real docker compose up against the published :latest image. Not run: no Docker on the machine that wrote this.

• Review round 2. DarlingInstallLocationTests: 42 passed, 0 failed. New tests cover the service's grant on the folder in three service states and the advice gate. Others cover odd and over-long values, the list's exact names and the key folder. DarlingUpgradeShapesWorkflowTests: 7 passed (1 new, for the checksum check). The script-reading classes: 208 run, 0 failed.

• Red-plant. Putting the folder-grantee branch back failed the new folder-grant test and the identity pin. Dropping the folder comparison from the advice failed the new advice test. I restored both and the class is green.

• Final run after the last code change: Stage=Guard 1,481 run, 0 failed. *Census*, *Ratchet* and DocCommentHygiene* 593 run, 0 failed, 1 skipped. Build: 0 warnings, 0 errors.

• Upgrade-shape run on this head: run 37936483027 on head 7022cfd: 14 of 14 legs passed (https://github.com/erikdarlingdata/PerformanceMonitor/actions/runs/37936483027).

• CI fix for run 37937037134. Four DarlingInstallLocationTests failed on the runner and passed here. The cause is the runner's short 8.3 temp folder (C:\Users\RUNNER~1\AppData\Local\Temp). Windows PowerShell 5.1 returns the long form for every file it walks. So the test's stand-in Get-Acl never matched a walked path, and every count came back 0. It was not a privilege problem, and the product scripts are unchanged. The probe now names its tree in the long form. Every probe that sets an owner or an ACE also prints a setup.* line. The test asserts that line, so a setup that did not take fails by name. Pointing TEMP at the short form here reproduced the same four failures. With the fix, 42 of 42 pass under both forms. Dropping the long-form line fails 6 tests with the setup message. On a throwaway branch (now deleted), DarlingInstallLocationTests passed 42 of 42 on windows-2022 and windows-2025. Those are run 37940768905 (long-form fix) and run 37941620495 (plus the setup checks). Here: Stage=Guard 1,481 run, 0 failed. *Census*, *Ratchet*, DocCommentHygiene* and DarlingInstallLocationTests: 635 run, 0 failed, 1 skipped. Build: 0 warnings.