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/security-review.md b/docs/security-review.md index d82c430..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, providing machine/user-bound encryption +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/docs/troubleshooting.md b/docs/troubleshooting.md index d15c592..0c3e180 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.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. **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.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): + ```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..228f1e4 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,17 @@ else { exit 1 } +# 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 if ($ListAll) { @@ -126,8 +139,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 +184,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..c0cd95e 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,17 @@ else { exit 1 } +# 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 Write-Log "Registering domain '$Domain' with acme-dns server: $AcmeDnsServer" -Level Info @@ -191,8 +203,44 @@ $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. + # + # 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) + # 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, + [System.Security.Cryptography.DataProtectionScope]::LocalMachine + ) + $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) + } + } $storageData = [PSCustomObject]@{ Domain = $credentialData.Domain @@ -203,7 +251,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