From 793d68b16160c96367c33ff7df4448c66aff8a97 Mon Sep 17 00:00:00 2001 From: Sev7eNup <79143581+Sev7eNup@users.noreply.github.com> Date: Wed, 12 Aug 2026 00:54:42 +0200 Subject: [PATCH] Verify the data directory the way the service will, before starting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 1.2.3 install failed at the first service start with "JWT signing-key file security validation failed: parent directory 'C:\ProgramData\NodePilot' grants mutation rights to an untrusted principal", and rolled back. The check is not new — it has existed since 1.0.0 — and the installer already applied the right ACL. What it never did was verify the result. The trusted set the service evaluates against includes the identity it is RUNNING as, so an ACE for a service account is only harmless while the service runs as that account. Install once as A and again as B and A's leftover ACE is a stranger with write access next to the signing key. Nothing between the ACL call and Start-Service noticed, so the API discovered it — after the binaries had been replaced, which puts the failure in the rollback path the catch block already documents as able to fail on its own. The installer now asks the same question itself. Test-ServiceDirectoryAclTrust mirrors BuildTrustedSids (as SIDs, not names — a localised Windows calls these groups something else), and Assert-ServiceDirectoryAclUsable verifies, repairs once through the same Set-DirectoryAclForService, verifies again, and otherwise gives up naming the principal and the icacls command that removes it. It runs before the artifact is extracted, so giving up costs nothing. The security check itself is unchanged. Widening the trusted set to bless any configured service account would defeat an audit finding whose whole point is a foreign account with write access beside the JWT key; producing a clean directory achieves the same outcome without giving that up. RestrictedFileWriter now names the offending principal instead of reporting an anonymous "untrusted principal" — account name and SID where it resolves, the bare SID where the account is gone, which is the common case here and exactly what icacls needs. --- deploy/Install-NodePilot.ps1 | 144 ++++++++++++++++++ deploy/README.md | 2 +- deploy/Test-DeploymentTemplates.ps1 | 31 ++++ docs/deployment-guide.md | 1 + .../Security/RestrictedFileWriter.cs | 32 +++- .../Security/RestrictedFileWriterTests.cs | 44 ++++++ 6 files changed, 251 insertions(+), 3 deletions(-) diff --git a/deploy/Install-NodePilot.ps1 b/deploy/Install-NodePilot.ps1 index 080b8e2..24f9dbf 100644 --- a/deploy/Install-NodePilot.ps1 +++ b/deploy/Install-NodePilot.ps1 @@ -375,6 +375,143 @@ function Set-DirectoryAclForService { Set-Acl -Path $Path -AclObject $acl } +function Test-ServiceDirectoryAclTrust { + <# + Answers the one question the service will ask itself seconds later, and answers it the same + way: RestrictedFileWriter.ValidateWindowsDirectoryAcl refuses to read the JWT signing key + when the directory holding it has an owner it does not trust, or grants mutation rights to a + principal outside a deliberately tiny set - SYSTEM, Administrators, TrustedInstaller, + CreatorOwner, OwnerRights, and the identity the service is RUNNING as. + + That last one is the trap this exists for. An ACE for a service account is only harmless + while the service runs as that account; install once as A and again as B, and A's leftover + ACE is a stranger with write access next to the signing key. The service then refuses to + start with "grants mutation rights to an untrusted principal", after the installer has + already replaced the binaries - so the failure lands in the rollback path instead of in a + check. Same rule, evaluated here, turns that into something the installer can fix. + + Returns @{ IsSecure = bool; Reason = string } - never throws, so a directory this account + cannot read its ACL from is reported rather than crashing the install. + #> + param( + [Parameter(Mandatory)][string]$Path, + [Parameter(Mandatory)][string]$ServiceAccount, + [switch]$SkipServiceRule + ) + + try { + $acl = Get-Acl -LiteralPath $Path -ErrorAction Stop + } catch { + return @{ IsSecure = $false; Reason = "the ACL of '$Path' could not be read: $($_.Exception.Message)" } + } + + # Mirrors BuildTrustedSids() on the API side. Kept as SIDs, not names, because a localised + # Windows calls these groups something else and a domain account resolves differently. + $trusted = [System.Collections.Generic.HashSet[string]]::new( + [string[]]@( + 'S-1-5-18', # LocalSystem + 'S-1-5-32-544', # Administrators + 'S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464', # TrustedInstaller + 'S-1-3-0', # CreatorOwner + 'S-1-3-4' # OwnerRights + ), + [System.StringComparer]::OrdinalIgnoreCase) + + # The account the service will run as. LocalSystem is already in the set above; for anything + # else the SID has to be resolved, and a name that no longer resolves is itself a finding. + if (-not $SkipServiceRule) { + try { + $svcSid = (New-Object System.Security.Principal.NTAccount($ServiceAccount)).Translate( + [System.Security.Principal.SecurityIdentifier]).Value + $null = $trusted.Add($svcSid) + } catch { + return @{ IsSecure = $false; Reason = "the service account '$ServiceAccount' could not be resolved to a SID: $($_.Exception.Message)" } + } + } + + $ownerSid = $null + try { + $ownerSid = $acl.GetOwner([System.Security.Principal.SecurityIdentifier]).Value + } catch { + return @{ IsSecure = $false; Reason = "the owner of '$Path' could not be read: $($_.Exception.Message)" } + } + if (-not $trusted.Contains($ownerSid)) { + return @{ IsSecure = $false; Reason = "'$Path' is owned by $(Resolve-SidLabel $ownerSid), which the service does not trust" } + } + + # Same right mask the API applies to the immediate parent of a secret. + $dangerous = [System.Security.AccessControl.FileSystemRights]::Delete ` + -bor [System.Security.AccessControl.FileSystemRights]::DeleteSubdirectoriesAndFiles ` + -bor [System.Security.AccessControl.FileSystemRights]::ChangePermissions ` + -bor [System.Security.AccessControl.FileSystemRights]::TakeOwnership ` + -bor [System.Security.AccessControl.FileSystemRights]::CreateFiles ` + -bor [System.Security.AccessControl.FileSystemRights]::CreateDirectories ` + -bor [System.Security.AccessControl.FileSystemRights]::WriteAttributes ` + -bor [System.Security.AccessControl.FileSystemRights]::WriteExtendedAttributes + + foreach ($rule in $acl.GetAccessRules($true, $true, [System.Security.Principal.SecurityIdentifier])) { + if ($rule.AccessControlType -ne [System.Security.AccessControl.AccessControlType]::Allow) { continue } + if ($rule.PropagationFlags.HasFlag([System.Security.AccessControl.PropagationFlags]::InheritOnly)) { continue } + $sid = $rule.IdentityReference.Value + if ($trusted.Contains($sid)) { continue } + if (($rule.FileSystemRights -band $dangerous) -ne 0) { + return @{ IsSecure = $false; Reason = "'$Path' grants write access to $(Resolve-SidLabel $sid), which the service does not trust" } + } + } + + return @{ IsSecure = $true; Reason = '' } +} + +function Resolve-SidLabel { + <# + "DOMAIN\account (S-1-5-21-...)" where the SID still resolves, the bare SID otherwise. An + orphaned SID is the common case here - a decommissioned service account keeps its ACE - and + it is precisely the value icacls needs to remove it, so translation failure is an answer, + not an error. + #> + param([Parameter(Mandatory)][string]$Sid) + try { + $name = (New-Object System.Security.Principal.SecurityIdentifier($Sid)).Translate( + [System.Security.Principal.NTAccount]).Value + return "$name ($Sid)" + } catch { + return "$Sid (account no longer exists)" + } +} + +function Assert-ServiceDirectoryAclUsable { + <# + Verify, repair once, verify again - then give up loudly instead of handing the service a + directory it will refuse. The repair is not a second mechanism: it is the same + Set-DirectoryAclForService the install already ran, which drops inheritance, wipes every + explicit ACE and forces the owner back to Administrators. Running it again is what clears a + stranger's ACE that predates this installation. + #> + param( + [Parameter(Mandatory)][string]$Path, + [Parameter(Mandatory)][string]$ServiceAccount, + [switch]$SkipServiceRule, + [Parameter(Mandatory)][string]$Label + ) + + $verdict = Test-ServiceDirectoryAclTrust -Path $Path -ServiceAccount $ServiceAccount -SkipServiceRule:$SkipServiceRule + if ($verdict.IsSecure) { return } + + Write-Warn " $Label is not usable by the service yet: $($verdict.Reason)" + Write-Info ' Repairing it (owner, inheritance and ACEs) and re-checking.' + Set-DirectoryAclForService -Path $Path -ServiceAccount $ServiceAccount -SkipServiceRule:$SkipServiceRule + + $verdict = Test-ServiceDirectoryAclTrust -Path $Path -ServiceAccount $ServiceAccount -SkipServiceRule:$SkipServiceRule + if ($verdict.IsSecure) { + Write-Info ' Repaired.' + return + } + + throw ("$Label cannot be made usable by the service: $($verdict.Reason). " + + 'The service would refuse to start with "JWT signing-key file security validation failed". ' + + "Remove that entry (icacls '$Path' /remove:g ''), then run the installer again.") +} + function Assert-SafeInstallRoot { <# H-18. The install directory holds the service binaries and is registered as the image path @@ -1092,6 +1229,13 @@ foreach ($identityBoundSecret in @('jwt-secret.key', 'admin-setup.token')) { } } +# Applying an ACL and assuming it landed is what let a leftover ACE from an earlier installation +# survive all the way to the first service start, where the API - not the installer - discovered +# it and refused to read the JWT key. Ask the same question here, while the only thing that has +# happened is that two directories exist: the binaries are not extracted until below. +Assert-ServiceDirectoryAclUsable -Path $DataPath -ServiceAccount $AclIdentity ` + -SkipServiceRule:$isLocalSystem -Label "The data directory '$DataPath'" + # Provisioning seed: copied in rather than referenced where the operator left it, because the API # reads it as the service account at first start and a deployment share is not reachable then. # It carries credentials, so it lands with the same restricted ACL as the configuration itself and diff --git a/deploy/README.md b/deploy/README.md index 6ad1856..29db7ba 100644 --- a/deploy/README.md +++ b/deploy/README.md @@ -348,7 +348,7 @@ Der Installer macht alles Weitere: 5. `appsettings.Production.json` aus Template erzeugen 6. ACLs setzen (Service-Identität = gMSA bzw. `NT AUTHORITY\SYSTEM` bei LocalSystem): - InstallPath: Service = **ReadAndExecute**, Admins/SYSTEM = Full, Vererbung aus. Der Dienst führt die Binaries aus, er überschreibt sie nie — Schreibrecht dort wäre Code-Ausführung als Dienstkonto (H-18). Der Pfad wird vorher validiert (lokal, NTFS/ReFS, keine Reparse Points) und nach dem Kopieren erneut geprüft. - - DataPath: Service = Modify, Admins/SYSTEM = Full, sonst nichts. Bei LocalSystem deckt die SYSTEM-Full-ACE den Dienst bereits ab — keine zusätzliche ACE. + - DataPath: Service = Modify, Admins/SYSTEM = Full, sonst nichts. Bei LocalSystem deckt die SYSTEM-Full-ACE den Dienst bereits ab — keine zusätzliche ACE. **Direkt danach wird gegengeprüft**, und zwar mit derselben Regel, die der Dienst gleich anlegt (`Test-ServiceDirectoryAclTrust` spiegelt `RestrictedFileWriter.BuildTrustedSids`): Owner vertrauenswürdig, kein fremder Allow-ACE mit Mutationsrechten. Schlägt das an, repariert der Installer einmal und prüft erneut; erst dann geht es weiter. Grund: Eine ACE für ein Dienstkonto ist nur so lange harmlos, wie der Dienst **genau unter diesem Konto** läuft — ein Rest aus einer Installation mit anderer Identität überlebte sonst bis zum ersten Dienststart und kippte ihn dort mit „grants mutation rights to an untrusted principal", also erst im Rollback-Pfad. Die Prüfung liegt bewusst **vor** dem Entpacken des Artefakts, damit ein Aufgeben nichts kostet. - `appsettings.Production.json`: Service = Read, Admins/SYSTEM = Full (bei LocalSystem analog von SYSTEM-Full abgedeckt) - Cert Private Key: gMSA = Read; bei LocalSystem übersprungen (SYSTEM hat Read auf MachineKeys per Default) - PostgreSQL: `ConnectionStrings:Postgres` bleibt in JSON leer; der vollständig gequotete diff --git a/deploy/Test-DeploymentTemplates.ps1 b/deploy/Test-DeploymentTemplates.ps1 index d8a7890..43b0acb 100644 --- a/deploy/Test-DeploymentTemplates.ps1 +++ b/deploy/Test-DeploymentTemplates.ps1 @@ -582,6 +582,37 @@ Assert-TextMatches -Name 'the data directory gets a trusted owner, not just trus -Text $installerScript.Substring($aclFunctionStart, $aclFunctionEnd - $aclFunctionStart) ` -Pattern 'SetOwner\(' +# Applying the ACL and assuming it landed is not the same as the service being able to use it. A +# leftover ACE from an installation that ran under a different service identity survives into the +# new install, and the API - not the installer - discovers it, at the first service start, with +# "grants mutation rights to an untrusted principal" and a rollback that the comment at the catch +# block records as capable of failing on its own. Reported from the field on 1.2.3. The installer +# therefore asks the same question itself, and it has to ask it BEFORE the artifact is extracted, +# so a give-up costs nothing. +Assert-TextMatches -Name 'the installer verifies the data directory the way the service will' ` + -Text $installerScript ` + -Pattern '(?s)Set-DirectoryAclForService -Path \$DataPath[\s\S]{0,2000}Assert-ServiceDirectoryAclUsable -Path \$DataPath' +Assert-TextMatches -Name 'that verification runs before the artifact is extracted' ` + -Text $installerScript ` + -Pattern '(?s)Assert-ServiceDirectoryAclUsable -Path \$DataPath[\s\S]{0,4000}Write-Step "Extracting artifact"' +# Repair-then-recheck, not repair-and-hope: the second verdict is what decides. +$assertFunctionStart = $installerScript.IndexOf('function Assert-ServiceDirectoryAclUsable') +$assertFunctionEnd = $installerScript.IndexOf('function Assert-SafeInstallRoot', $assertFunctionStart) +if ($assertFunctionStart -lt 0 -or $assertFunctionEnd -le $assertFunctionStart) { + throw 'Deployment template check failed: could not delimit Assert-ServiceDirectoryAclUsable in the installer.' +} +$assertFunction = $installerScript.Substring($assertFunctionStart, $assertFunctionEnd - $assertFunctionStart) +Assert-TextMatches -Name 'the ACL repair is re-verified and gives up loudly' ` + -Text $assertFunction ` + -Pattern '(?s)Set-DirectoryAclForService[\s\S]{0,600}Test-ServiceDirectoryAclTrust[\s\S]{0,600}throw' +# The trusted set has to match BuildTrustedSids() in RestrictedFileWriter.cs, or the installer +# blesses a directory the service then rejects - the exact failure this whole check exists for. +foreach ($trustedSid in @('S-1-5-18', 'S-1-5-32-544', 'S-1-3-0', 'S-1-3-4')) { + Assert-TextMatches -Name "the installer's trusted set carries $trustedSid, like the API's" ` + -Text $installerScript ` + -Pattern ([regex]::Escape($trustedSid)) +} + # H-18. The install directory is the image path of a service running as LocalSystem or a gMSA, so # write access to it is code execution as that account. Only DataPath used to be hardened; # InstallPath was created with a plain New-Item -Force and inherited whatever the parent allowed - diff --git a/docs/deployment-guide.md b/docs/deployment-guide.md index cba21c9..f42aa25 100644 --- a/docs/deployment-guide.md +++ b/docs/deployment-guide.md @@ -365,6 +365,7 @@ Logs: `C:\ProgramData\NodePilot\logs\` (CMTrace-formatted). Firewall rule: | Boot log repeats `Waiting for the database to accept connections (n/120s)` | the database is not answering yet — a remote SQL Server still recovering, a DC not yet reachable for Kerberos, or a wrong host | let it finish; it proceeds either way and then reports the real connection error. Raise `Database:StartupWaitSeconds` (max 600) if the database routinely needs longer | | Event log 7000 *the service did not start due to a logon failure*, gMSA identity, only on boot | the service tried to log on before Netlogon could fetch the gMSA password from a DC | current builds set `depend= Netlogon` for gMSA services; on older ones `sc.exe config NodePilot depend= Netlogon` fixes it in place | | `admin-setup.token` → *Access to the path is denied* | intentional owner-only ACL for the service account | read via `robocopy /B` as shown in [Step 4](#step-4--first-login) instead of editing the ACL | +| Install fails with `JWT signing-key file security validation failed: parent directory 'C:\ProgramData\NodePilot' grants mutation rights to an untrusted principal`, then rolls back | an ACE on the data directory belongs to a principal the service does not trust — in practice the service account of an **earlier** installation, because an ACE is only trusted while the service actually runs as that account. Not a version problem; the check has existed since 1.0.0 | installers from 2026-08-12 on verify the directory with the service's own rule after applying the ACL, repair it, and only then start the service — so this no longer reaches the service. On older builds: `icacls C:\ProgramData\NodePilot` names the stranger, `icacls C:\ProgramData\NodePilot /remove:g ""` removes it. **`Jwt:RotateInsecureKeyFile=true` does not help here** — it replaces the key file, and the directory is what was rejected | | Every `runScript` step fails with `The term 'Write-Output' is not recognized` | artifact built with a pre-2026-08 `Build-Artifact.ps1` that did not stage the PowerShell built-in modules — `$PSHOME\Modules` is missing in the install dir | rebuild with the current build script; or hot-fix in place: `Copy-Item 'C:\Program Files\NodePilot\runtimes\win\lib\net10.0\Modules' 'C:\Program Files\NodePilot\Modules' -Recurse` and restart the service | | Installer prints `FAILED: ... Restoring the previous installation` | any error after mutation began rolls back to the previous state | fix the reported cause and re-run; note the diagnostics tail the shared log file, so lines from the *previous* installation can appear — check timestamps | | Browser shows *Not secure* / `Invoke-RestMethod` trust error | self-signed Kestrel certificate not trusted on the client | import it into `LocalMachine\Root` on that machine | diff --git a/src/NodePilot.Api/Security/RestrictedFileWriter.cs b/src/NodePilot.Api/Security/RestrictedFileWriter.cs index 23bfbf3..b8a54ee 100644 --- a/src/NodePilot.Api/Security/RestrictedFileWriter.cs +++ b/src/NodePilot.Api/Security/RestrictedFileWriter.cs @@ -309,7 +309,7 @@ private static ExistingSecretFileSecurity ValidateWindowsDirectoryAcl( var owner = acl.GetOwner(typeof(SecurityIdentifier)) as SecurityIdentifier; if (owner is null || !trustedSids.Contains(owner)) return ExistingSecretFileSecurity.Invalid( - $"parent directory '{directory.FullName}' has an untrusted owner"); + $"parent directory '{directory.FullName}' has an untrusted owner ({Describe(owner)})"); var dangerous = FileSystemRights.Delete | FileSystemRights.DeleteSubdirectoriesAndFiles @@ -339,12 +339,40 @@ private static ExistingSecretFileSecurity ValidateWindowsDirectoryAcl( if ((rule.FileSystemRights & dangerous) != 0) return ExistingSecretFileSecurity.Invalid( - $"parent directory '{directory.FullName}' grants mutation rights to an untrusted principal"); + $"parent directory '{directory.FullName}' grants mutation rights to an untrusted principal " + + $"({Describe(sid)})"); } return ExistingSecretFileSecurity.Valid(); } + /// + /// Renders a SID for an operator: the account name where it still resolves, the raw SID + /// otherwise — and both when they differ, because the SID is what icacls needs. + /// + /// Naming the principal is the whole point. "grants mutation rights to an untrusted + /// principal" is true and useless: the usual cause is a leftover ACE from an earlier + /// installation that ran under a different service identity, and without the name there is + /// nothing to search for. An orphaned SID — the account was deleted, which is exactly what a + /// decommissioned service account looks like — cannot be translated, and that failure is + /// itself the answer, so it must never turn into an exception on a boot path. + /// + private static string Describe(SecurityIdentifier? sid) + { + if (sid is null) return "no owner could be read"; + try + { + var name = ((NTAccount)sid.Translate(typeof(NTAccount))).Value; + return $"{name}, {sid.Value}"; + } + catch (Exception ex) when (ex is IdentityNotMappedException or SystemException) + { + // Unresolvable SIDs are the interesting case, not an error: a deleted account still + // holds its ACE, and the raw SID is what removes it. + return $"{sid.Value}, unresolvable — the account no longer exists"; + } + } + private static HashSet BuildTrustedSids() { var currentSid = WindowsIdentity.GetCurrent().User diff --git a/tests/NodePilot.Api.Tests/Security/RestrictedFileWriterTests.cs b/tests/NodePilot.Api.Tests/Security/RestrictedFileWriterTests.cs index 213d7b5..79a9408 100644 --- a/tests/NodePilot.Api.Tests/Security/RestrictedFileWriterTests.cs +++ b/tests/NodePilot.Api.Tests/Security/RestrictedFileWriterTests.cs @@ -137,4 +137,48 @@ public void WriteText_DoesNotLeavePartialFile_OnFailure() "the writer must delete its partial file when anything in the create-then-acl-then-write " + "sequence fails, so a retry never reuses a half-secured artifact"); } + + [Fact] + public void ValidateParentDirectory_ForeignMutationAce_NamesTheOffendingPrincipal() + { + if (!OperatingSystem.IsWindows()) return; + + // The rejection used to read "grants mutation rights to an untrusted principal" and stop + // there. True, and unusable: the usual cause is a leftover ACE from an earlier install + // that ran under a different service identity, and without the principal there is nothing + // to search for or hand to icacls. A server operator hit exactly this on 1.2.3 and the + // message pointed at the JWT key file instead of at the directory's ACL. + var dir = new DirectoryInfo(_tempDir); + var acl = dir.GetAccessControl(); + acl.AddAccessRule(new FileSystemAccessRule( + new SecurityIdentifier(WellKnownSidType.WorldSid, null), + FileSystemRights.Modify, + InheritanceFlags.ContainerInherit | InheritanceFlags.ObjectInherit, + PropagationFlags.None, + AccessControlType.Allow)); + dir.SetAccessControl(acl); + + var reason = ValidateParentDirectoryReason(Path.Combine(_tempDir, "jwt.key")); + + reason.Should().Contain("mutation rights"); + // The SID, not the display name: "Everyone" is localised (German Windows says "Jeder"), + // and the SID is the form icacls takes to remove the entry. + reason.Should().Contain("S-1-1-0", + "the operator needs to know WHICH principal to remove, and the SID is what does it"); + } + + /// + /// Calls the internal ValidateParentDirectory and returns its Reason. Same + /// reflection boundary as — the type is internal on purpose. + /// + private static string ValidateParentDirectoryReason(string path) + { + var writerType = typeof(NodePilot.Api.Security.JwtKeyResolver).Assembly + .GetType("NodePilot.Api.Security.RestrictedFileWriter", throwOnError: true)!; + var validate = writerType.GetMethod( + "ValidateParentDirectory", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static)!; + var result = validate.Invoke(null, new object[] { path })!; + var reason = result.GetType().GetProperty("Reason")!.GetValue(result) as string; + return reason ?? ""; + } }