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 ?? ""; + } }