From 4e057a555b6d0ecea3d30eade06f21012a730678 Mon Sep 17 00:00:00 2001 From: Andrew Yager Date: Thu, 30 Apr 2026 10:51:09 +1000 Subject: [PATCH 1/3] Use machine-scope DPAPI for acme-dns credentials Register-AcmeDns.ps1 previously called ConvertFrom-SecureString without arguments, which encrypts under DPAPI CurrentUser scope. The win-acme renewal scheduled task is created by Install-Prerequisites.ps1 to run as SYSTEM, which cannot decrypt CurrentUser-scoped DPAPI blobs. Result: operators following the runbook produce credentials that the renewal task cannot read, so renewals fail silently until certificates expire. Switch the JsonFile storage path to encrypt with DataProtectionScope.LocalMachine via [ProtectedData]::Protect, base64, and tag the credential with StorageMethod = "DPAPI-LocalMachine". Get-AcmeDnsCredential.ps1 gains a "DPAPI-LocalMachine" switch case and keeps the legacy "DPAPI" case so existing CurrentUser-scoped credentials still load when retrieved by the same user that registered them. The legacy error message points at scripts\Recovery\Repair-AcmeDnsCredential.ps1 for migration. Validated end-to-end on a Domain Controller: the updated Get-AcmeDnsCredential.ps1 decrypts an in-place DPAPI-LocalMachine credential (already migrated by the recovery tool). Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/security-review.md | 2 +- docs/troubleshooting.md | 29 +++++++++++++--- scripts/AcmeDns/Get-AcmeDnsCredential.ps1 | 40 +++++++++++++++++++++-- scripts/AcmeDns/Register-AcmeDns.ps1 | 33 ++++++++++++++++--- 4 files changed, 93 insertions(+), 11 deletions(-) diff --git a/docs/security-review.md b/docs/security-review.md index d82c430..f039109 100644 --- a/docs/security-review.md +++ b/docs/security-review.md @@ -188,7 +188,7 @@ if (Test-Path $commonPath) { ## Positive Security Practices Observed -1. **DPAPI Encryption** - Credentials are encrypted using Windows DPAPI, providing machine/user-bound encryption +1. **DPAPI Encryption** - Credentials are encrypted using Windows DPAPI in `LocalMachine` scope, so any account on the host (including the SYSTEM-context renewal task) can decrypt while attackers without local access cannot. Legacy `CurrentUser` scope blobs are still decrypted for backwards compatibility. 2. **TLS 1.2 Enforcement** - All network communications explicitly set TLS 1.2 minimum 3. **Administrator Requirements** - Scripts that require elevated privileges declare `#Requires -RunAsAdministrator` 4. **Parameter Validation** - Proper use of `ValidateSet`, `ValidateNotNullOrEmpty` attributes diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index d15c592..91643d3 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -180,17 +180,38 @@ nslookup -type=CNAME _acme-challenge.example.com 8.8.8.8 **Symptoms:** ``` Failed to decrypt password +ConvertTo-SecureString : ... CryptographicException Cannot authenticate to acme-dns ``` +The renewal log will show the error inside `Get-AcmeDnsCredential.ps1` while running under the SYSTEM account that owns the win-acme scheduled task. + **Causes:** -- Credentials created by different user -- Credentials created on different machine -- Corrupted credential file + +1. **Legacy DPAPI scope mismatch** — credentials were registered with toolkit ≤ v1.0.1 (which used DPAPI CurrentUser scope) by an interactive operator, but the renewal task runs as SYSTEM and cannot decrypt them. The `StorageMethod` in the credential JSON file will be `DPAPI`. **This is the most common cause** and certificates will silently fail to renew until they expire. +2. Credentials created on a different machine (DPAPI keys are host-bound). +3. Corrupted credential file. **Solutions:** + +For cause 1 (DPAPI scope mismatch), use the recovery tool to re-encrypt under machine scope without losing the existing acme-dns subdomain registration (no DNS changes needed): + +```powershell +.\scripts\Recovery\Repair-AcmeDnsCredential.ps1 -Domain "example.com" +# Prompts for the original acme-dns password from /register +``` + +After the repair, force a renewal to confirm: + +```powershell +C:\Tools\win-acme\wacs.exe --renew --force --verbose +``` + +Toolkit ≥ v1.0.2 registers new credentials with `StorageMethod = DPAPI-LocalMachine` by default, so this issue does not affect fresh installs. Existing legacy installs only need the repair script run once. + +For other causes, re-register from scratch (this generates a new acme-dns subdomain UUID and requires updating the external CNAME): + ```powershell -# Re-register domain .\scripts\AcmeDns\Register-AcmeDns.ps1 -Domain "example.com" -Force ``` diff --git a/scripts/AcmeDns/Get-AcmeDnsCredential.ps1 b/scripts/AcmeDns/Get-AcmeDnsCredential.ps1 index 8c009db..fa75b4f 100644 --- a/scripts/AcmeDns/Get-AcmeDnsCredential.ps1 +++ b/scripts/AcmeDns/Get-AcmeDnsCredential.ps1 @@ -29,6 +29,8 @@ #> [CmdletBinding(DefaultParameterSetName = 'Single')] +[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSAvoidUsingConvertToSecureStringWithPlainText', '', + Justification = 'Wraps a password decrypted from DPAPI back into a SecureString for the non-AsPlainText return path.')] param( [Parameter(Mandatory = $true, ParameterSetName = 'Single')] [ValidateNotNullOrEmpty()] @@ -51,6 +53,9 @@ else { exit 1 } +# DPAPI LocalMachine scope requires System.Security.dll +Add-Type -AssemblyName System.Security -ErrorAction SilentlyContinue + Initialize-WinCertManager if ($ListAll) { @@ -126,8 +131,39 @@ catch { $password = $null switch ($storedData.StorageMethod) { + 'DPAPI-LocalMachine' { + # Machine-scoped DPAPI: any account on this host (including SYSTEM) + # can decrypt. This is the default for credentials registered by + # current toolkit versions. + $plainBytes = $null + try { + $protectedBytes = [Convert]::FromBase64String($storedData.EncryptedPassword) + $plainBytes = [System.Security.Cryptography.ProtectedData]::Unprotect( + $protectedBytes, $null, + [System.Security.Cryptography.DataProtectionScope]::LocalMachine + ) + $plainText = [System.Text.Encoding]::UTF8.GetString($plainBytes) + if ($AsPlainText) { + $password = $plainText + } + else { + $password = ConvertTo-SecureString -String $plainText -AsPlainText -Force + } + } + catch { + Write-Error "Failed to decrypt password (LocalMachine DPAPI): $($_.Exception.Message)" + return $null + } + finally { + if ($null -ne $plainBytes) { [Array]::Clear($plainBytes, 0, $plainBytes.Length) } + } + } + 'DPAPI' { - # Decrypt password from DPAPI + # Legacy CurrentUser-scoped DPAPI. Only the user that registered the + # domain can decrypt. If the renewal task runs as SYSTEM, decryption + # will fail here; use scripts\Recovery\Repair-AcmeDnsCredential.ps1 + # to migrate to DPAPI-LocalMachine. try { $securePassword = ConvertTo-SecureString -String $storedData.EncryptedPassword if ($AsPlainText) { @@ -140,7 +176,7 @@ switch ($storedData.StorageMethod) { } } catch { - Write-Error "Failed to decrypt password. This usually means the credential was stored by a different user or on a different machine." + Write-Error "Failed to decrypt password. This usually means the credential was stored by a different user or on a different machine. Run scripts\Recovery\Repair-AcmeDnsCredential.ps1 to migrate to machine-scope DPAPI." return $null } } diff --git a/scripts/AcmeDns/Register-AcmeDns.ps1 b/scripts/AcmeDns/Register-AcmeDns.ps1 index f9dedb4..f4778a1 100644 --- a/scripts/AcmeDns/Register-AcmeDns.ps1 +++ b/scripts/AcmeDns/Register-AcmeDns.ps1 @@ -21,7 +21,8 @@ .PARAMETER StorageMethod How to store the credentials: CredentialManager or JsonFile. - Default: JsonFile (DPAPI encrypted) + Default: JsonFile (DPAPI encrypted, machine scope so SYSTEM-context + renewal tasks can decrypt). .PARAMETER ApiKey API key for authenticated acme-dns servers. Required for RWTS acme-dns. @@ -89,6 +90,9 @@ else { exit 1 } +# DPAPI LocalMachine scope requires System.Security.dll +Add-Type -AssemblyName System.Security + # Initialize Initialize-WinCertManager Write-Log "Registering domain '$Domain' with acme-dns server: $AcmeDnsServer" -Level Info @@ -191,8 +195,29 @@ $credentialData = [PSCustomObject]@{ # Store credentials switch ($StorageMethod) { 'JsonFile' { - # Use DPAPI encryption for the password - $encryptedPassword = ConvertFrom-SecureString -SecureString $securePassword + # DPAPI machine-scope encryption: any account on this host (including + # SYSTEM, which runs the win-acme renewal task) can decrypt. Without + # this, a renewal triggered by SYSTEM cannot read credentials that + # were registered by an interactive operator. + $bstr = [IntPtr]::Zero + $plainBytes = $null + try { + $bstr = [System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($securePassword) + $plainText = [System.Runtime.InteropServices.Marshal]::PtrToStringBSTR($bstr) + $plainBytes = [System.Text.Encoding]::UTF8.GetBytes($plainText) + + $protected = [System.Security.Cryptography.ProtectedData]::Protect( + $plainBytes, $null, + [System.Security.Cryptography.DataProtectionScope]::LocalMachine + ) + $encryptedPassword = [Convert]::ToBase64String($protected) + } + finally { + if ($null -ne $plainBytes) { [Array]::Clear($plainBytes, 0, $plainBytes.Length) } + if ($bstr -ne [IntPtr]::Zero) { + [System.Runtime.InteropServices.Marshal]::ZeroFreeBSTR($bstr) + } + } $storageData = [PSCustomObject]@{ Domain = $credentialData.Domain @@ -203,7 +228,7 @@ switch ($StorageMethod) { EncryptedPassword = $encryptedPassword AllowFrom = $credentialData.AllowFrom RegisteredAt = $credentialData.RegisteredAt - StorageMethod = 'DPAPI' + StorageMethod = 'DPAPI-LocalMachine' } $storageData | ConvertTo-Json -Depth 5 | Set-Content -Path $credentialFile -Force From f1478b5757acf9a101889a33c0a492d42b26eb65 Mon Sep 17 00:00:00 2001 From: Andrew Yager Date: Thu, 30 Apr 2026 12:22:14 +1000 Subject: [PATCH 2/3] Address PR review: tighten Add-Type, drop plaintext string copy, ACL caveat Three independent improvements from Copilot review feedback: 1. docs/security-review.md: rewrite the DPAPI bullet to be explicit that LocalMachine scope shifts the confidentiality boundary onto NTFS ACLs. Any local account that can read the credential blob can also decrypt it, so the credential file/directory should be locked down to SYSTEM and Administrators. ACL hardening in Initialize-WinCertManager is left as a separate change. 2. Register-AcmeDns.ps1 and Get-AcmeDnsCredential.ps1: replace silent Add-Type with a try/catch + -ErrorAction Stop, so a missing System.Security assembly fails immediately with an actionable error instead of letting the later [ProtectedData] reference throw a confusing type-not-found exception. 3. Register-AcmeDns.ps1: stop materialising the plaintext password as a managed System.String during encryption. Copy the BSTR contents directly into a byte buffer and transcode UTF-16 -> UTF-8 via Encoding.Convert. The previous PtrToStringBSTR/UTF8.GetBytes path left an immutable plaintext copy on the GC heap that could not be zeroed; the new path keeps the plaintext only in byte buffers that are explicitly cleared in the finally block. Functionally verified with a Unicode round-trip locally. No CI rule or behavioural regression. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/security-review.md | 2 +- scripts/AcmeDns/Get-AcmeDnsCredential.ps1 | 12 +++++++-- scripts/AcmeDns/Register-AcmeDns.ps1 | 31 ++++++++++++++++++++--- 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/docs/security-review.md b/docs/security-review.md index f039109..4c2e65b 100644 --- a/docs/security-review.md +++ b/docs/security-review.md @@ -188,7 +188,7 @@ if (Test-Path $commonPath) { ## Positive Security Practices Observed -1. **DPAPI Encryption** - Credentials are encrypted using Windows DPAPI in `LocalMachine` scope, so any account on the host (including the SYSTEM-context renewal task) can decrypt while attackers without local access cannot. Legacy `CurrentUser` scope blobs are still decrypted for backwards compatibility. +1. **DPAPI Encryption** - Credentials are encrypted using Windows DPAPI in `LocalMachine` scope, which allows any local account on the host (including the SYSTEM-context renewal task) to decrypt a blob if it can read it. As a result, confidentiality depends heavily on NTFS ACLs on the credential files/directories being locked down appropriately (ideally to `SYSTEM` and `Administrators`) so unprivileged local users cannot access the encrypted blobs. Legacy `CurrentUser` scope blobs are still decrypted for backwards compatibility. 2. **TLS 1.2 Enforcement** - All network communications explicitly set TLS 1.2 minimum 3. **Administrator Requirements** - Scripts that require elevated privileges declare `#Requires -RunAsAdministrator` 4. **Parameter Validation** - Proper use of `ValidateSet`, `ValidateNotNullOrEmpty` attributes diff --git a/scripts/AcmeDns/Get-AcmeDnsCredential.ps1 b/scripts/AcmeDns/Get-AcmeDnsCredential.ps1 index fa75b4f..228f1e4 100644 --- a/scripts/AcmeDns/Get-AcmeDnsCredential.ps1 +++ b/scripts/AcmeDns/Get-AcmeDnsCredential.ps1 @@ -53,8 +53,16 @@ else { exit 1 } -# DPAPI LocalMachine scope requires System.Security.dll -Add-Type -AssemblyName System.Security -ErrorAction SilentlyContinue +# DPAPI LocalMachine decrypt path needs [ProtectedData] from System.Security.dll. +# Failure to load is fatal; surface it loudly rather than letting the type +# lookup fail with a confusing error later. +try { + Add-Type -AssemblyName System.Security -ErrorAction Stop +} +catch { + Write-Error "Failed to load required assembly 'System.Security'. DPAPI-based credential decryption is unavailable: $($_.Exception.Message)" + exit 1 +} Initialize-WinCertManager diff --git a/scripts/AcmeDns/Register-AcmeDns.ps1 b/scripts/AcmeDns/Register-AcmeDns.ps1 index f4778a1..c0cd95e 100644 --- a/scripts/AcmeDns/Register-AcmeDns.ps1 +++ b/scripts/AcmeDns/Register-AcmeDns.ps1 @@ -90,8 +90,16 @@ else { exit 1 } -# DPAPI LocalMachine scope requires System.Security.dll -Add-Type -AssemblyName System.Security +# DPAPI LocalMachine encrypt path needs [ProtectedData] from System.Security.dll. +# Failure to load is fatal; surface it loudly so the operator gets a clear +# error rather than a missing-type exception further down. +try { + Add-Type -AssemblyName System.Security -ErrorAction Stop +} +catch { + Write-Error "Failed to load required assembly 'System.Security'. DPAPI-based credential protection is unavailable: $($_.Exception.Message)" + exit 1 +} # Initialize Initialize-WinCertManager @@ -199,12 +207,26 @@ switch ($StorageMethod) { # SYSTEM, which runs the win-acme renewal task) can decrypt. Without # this, a renewal triggered by SYSTEM cannot read credentials that # were registered by an interactive operator. + # + # The plaintext password is copied directly out of the BSTR as raw + # UTF-16 bytes and transcoded to UTF-8 via Encoding.Convert. We avoid + # creating a managed System.String (e.g., via PtrToStringBSTR) so + # there is no garbage-collected immutable plaintext copy lingering + # in memory after this block exits. $bstr = [IntPtr]::Zero + $utf16Bytes = $null $plainBytes = $null try { $bstr = [System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($securePassword) - $plainText = [System.Runtime.InteropServices.Marshal]::PtrToStringBSTR($bstr) - $plainBytes = [System.Text.Encoding]::UTF8.GetBytes($plainText) + # BSTR length-in-bytes prefix lives 4 bytes before the pointer. + $byteCount = [System.Runtime.InteropServices.Marshal]::ReadInt32($bstr, -4) + $utf16Bytes = New-Object byte[] $byteCount + [System.Runtime.InteropServices.Marshal]::Copy($bstr, $utf16Bytes, 0, $byteCount) + $plainBytes = [System.Text.Encoding]::Convert( + [System.Text.Encoding]::Unicode, + [System.Text.Encoding]::UTF8, + $utf16Bytes + ) $protected = [System.Security.Cryptography.ProtectedData]::Protect( $plainBytes, $null, @@ -213,6 +235,7 @@ switch ($StorageMethod) { $encryptedPassword = [Convert]::ToBase64String($protected) } finally { + if ($null -ne $utf16Bytes) { [Array]::Clear($utf16Bytes, 0, $utf16Bytes.Length) } if ($null -ne $plainBytes) { [Array]::Clear($plainBytes, 0, $plainBytes.Length) } if ($bstr -ne [IntPtr]::Zero) { [System.Runtime.InteropServices.Marshal]::ZeroFreeBSTR($bstr) From a37cb29d6a08aa2b446297f1707ef71d2cf72fb3 Mon Sep 17 00:00:00 2001 From: Andrew Yager Date: Thu, 30 Apr 2026 12:29:31 +1000 Subject: [PATCH 3/3] Bump version refs to v1.0.4 for the LocalMachine DPAPI fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fix lands in the next release, which is v1.0.4 (current main is v1.0.3, not v1.0.2 as the docs assumed). Affected versions extend to v1.0.3 inclusive. - README.md: "≤ v1.0.1" -> "≤ v1.0.3", "≥ v1.0.2" -> "≥ v1.0.4" - docs/troubleshooting.md (new section): same bumps The unrelated v1.0.2+ reference at troubleshooting.md:303 (prereqs signature handling, fixed in v1.0.2) is correct and unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 4 ++-- docs/troubleshooting.md | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index b862085..f785368 100644 --- a/README.md +++ b/README.md @@ -250,14 +250,14 @@ This tells win-acme to use Google/Cloudflare DNS for checking TXT records instea ### Renewals Silently Failing (Legacy DPAPI Scope) -Toolkit versions ≤ v1.0.1 stored acme-dns credentials using DPAPI in `CurrentUser` scope, but the win-acme renewal scheduled task runs as `SYSTEM`. SYSTEM cannot decrypt CurrentUser-scoped DPAPI blobs, so every automatic renewal fails inside `Get-AcmeDnsCredential.ps1` and the certificate eventually expires without anyone being paged. +Toolkit versions ≤ v1.0.3 stored acme-dns credentials using DPAPI in `CurrentUser` scope, but the win-acme renewal scheduled task runs as `SYSTEM`. SYSTEM cannot decrypt CurrentUser-scoped DPAPI blobs, so every automatic renewal fails inside `Get-AcmeDnsCredential.ps1` and the certificate eventually expires without anyone being paged. **Symptoms:** - `Get-ScheduledTask -TaskName 'win-acme*' | Get-ScheduledTaskInfo` shows `LastTaskResult` of `4294967295` (`0xFFFFFFFF`) - `%ProgramData%\win-acme\acme-v02.api.letsencrypt.org\Log\log-*.txt` contains `ConvertTo-SecureString : ... CryptographicException` and `Failed to decrypt password. This usually means the credential was stored by a different user or on a different machine.` - The credential JSON in `%ProgramData%\WinCertManager\Config\acme-dns\` reports `"StorageMethod": "DPAPI"` -**Fix:** `scripts/Recovery/Repair-AcmeDnsCredential.ps1` re-encrypts the credential under DPAPI `LocalMachine` scope (so SYSTEM can decrypt it) without changing the acme-dns subdomain registration — no DNS changes required. Run it once per affected host. Toolkit ≥ v1.0.2 stores new credentials in `LocalMachine` scope by default, so fresh installs are unaffected. +**Fix:** `scripts/Recovery/Repair-AcmeDnsCredential.ps1` re-encrypts the credential under DPAPI `LocalMachine` scope (so SYSTEM can decrypt it) without changing the acme-dns subdomain registration — no DNS changes required. Run it once per affected host. Toolkit ≥ v1.0.4 stores new credentials in `LocalMachine` scope by default, so fresh installs are unaffected. > **Verify the script before running it.** Open it in a text editor or run `Get-AuthenticodeSignature` against a copy from a signed release ZIP. The script will prompt for the original acme-dns password (the `password` field returned by `/register`, retrievable from your password manager). diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 91643d3..0c3e180 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -188,7 +188,7 @@ The renewal log will show the error inside `Get-AcmeDnsCredential.ps1` while run **Causes:** -1. **Legacy DPAPI scope mismatch** — credentials were registered with toolkit ≤ v1.0.1 (which used DPAPI CurrentUser scope) by an interactive operator, but the renewal task runs as SYSTEM and cannot decrypt them. The `StorageMethod` in the credential JSON file will be `DPAPI`. **This is the most common cause** and certificates will silently fail to renew until they expire. +1. **Legacy DPAPI scope mismatch** — credentials were registered with toolkit ≤ v1.0.3 (which used DPAPI CurrentUser scope) by an interactive operator, but the renewal task runs as SYSTEM and cannot decrypt them. The `StorageMethod` in the credential JSON file will be `DPAPI`. **This is the most common cause** and certificates will silently fail to renew until they expire. 2. Credentials created on a different machine (DPAPI keys are host-bound). 3. Corrupted credential file. @@ -207,7 +207,7 @@ After the repair, force a renewal to confirm: C:\Tools\win-acme\wacs.exe --renew --force --verbose ``` -Toolkit ≥ v1.0.2 registers new credentials with `StorageMethod = DPAPI-LocalMachine` by default, so this issue does not affect fresh installs. Existing legacy installs only need the repair script run once. +Toolkit ≥ v1.0.4 registers new credentials with `StorageMethod = DPAPI-LocalMachine` by default, so this issue does not affect fresh installs. Existing legacy installs only need the repair script run once. For other causes, re-register from scratch (this generates a new acme-dns subdomain UUID and requires updating the external CNAME):