Use machine-scope DPAPI for acme-dns credentials#8
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the acme-dns credential storage format so scheduled renewals running as SYSTEM can decrypt credentials by switching DPAPI protection from CurrentUser to LocalMachine scope.
Changes:
Register-AcmeDns.ps1now storesEncryptedPasswordas base64 of DPAPI(LocalMachine) protected bytes and writesStorageMethod = "DPAPI-LocalMachine".Get-AcmeDnsCredential.ps1adds aDPAPI-LocalMachinedecrypt path while keeping legacyDPAPI(CurrentUser) support and improves the legacy failure message to point to a migration tool.- Documentation updates explain the symptom/cause and clarify the new DPAPI scope.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| scripts/AcmeDns/Register-AcmeDns.ps1 | Encrypts new acme-dns passwords under DPAPI LocalMachine and persists DPAPI-LocalMachine storage method. |
| scripts/AcmeDns/Get-AcmeDnsCredential.ps1 | Adds decrypt logic for DPAPI-LocalMachine and updates legacy decrypt failure messaging. |
| docs/troubleshooting.md | Documents the DPAPI scope mismatch symptom and a migration path via repair script. |
| docs/security-review.md | Updates security review text to reflect LocalMachine DPAPI scope and backwards compatibility. |
Comments suppressed due to low confidence (1)
scripts/AcmeDns/Register-AcmeDns.ps1:235
- DPAPI LocalMachine means any local user who can read the credential JSON can also decrypt it. Since the credential files are stored under ProgramData and
Initialize-WinCertManagerdoesn't set restrictive ACLs, this change can weaken confidentiality compared to the previous CurrentUser-scoped DPAPI. Consider explicitly setting file/directory ACLs (e.g., SYSTEM + Administrators only, and the account that needs access) when writing the credential file.
$storageData = [PSCustomObject]@{
Domain = $credentialData.Domain
AcmeDnsServer = $credentialData.AcmeDnsServer
Subdomain = $credentialData.Subdomain
FullDomain = $credentialData.FullDomain
Username = $credentialData.Username
EncryptedPassword = $encryptedPassword
AllowFrom = $credentialData.AllowFrom
RegisteredAt = $credentialData.RegisteredAt
StorageMethod = 'DPAPI-LocalMachine'
}
$storageData | ConvertTo-Json -Depth 5 | Set-Content -Path $credentialFile -Force
Write-Log "Credentials stored at: $credentialFile" -Level Info
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Add-Type -AssemblyName System.Security -ErrorAction SilentlyContinue | ||
|
|
There was a problem hiding this comment.
Add-Type -AssemblyName System.Security -ErrorAction SilentlyContinue will hide genuine load failures and can lead to confusing downstream errors when calling [System.Security.Cryptography.ProtectedData]. Prefer -ErrorAction Stop and a targeted error message (or a pre-flight check that the ProtectedData type is available) so decrypt failures are clearly distinguished from missing dependencies.
| Add-Type -AssemblyName System.Security -ErrorAction SilentlyContinue | |
| try { | |
| Add-Type -AssemblyName System.Security -ErrorAction Stop | |
| if (-not ('System.Security.Cryptography.ProtectedData' -as [type])) { | |
| throw 'The System.Security.Cryptography.ProtectedData type is not available.' | |
| } | |
| } | |
| catch { | |
| Write-Error "Unable to initialize DPAPI support required to decrypt stored acme-dns credentials. Ensure System.Security is available. $($_.Exception.Message)" | |
| exit 1 | |
| } |
| ## 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. |
There was a problem hiding this comment.
The write-up implies LocalMachine-scoped DPAPI is safe as long as an attacker isn't "without local access", but with DataProtectionScope.LocalMachine any local account can decrypt if it can read the blob. It would be worth explicitly calling out that confidentiality now depends heavily on NTFS ACLs on the credential files/directories (ideally restricted to SYSTEM/Administrators) to avoid leakage to unprivileged local users.
| 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. |
| 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 |
There was a problem hiding this comment.
This error message (and the docs) points users at scripts\Recovery\Repair-AcmeDnsCredential.ps1, but that path/script isn't present in this PR/repo state. Unless PR #7 is guaranteed to merge first, this guidance will be a dead-end; consider either including the recovery script in this PR, adding a check that it exists and tailoring the message, or ensuring the docs/message reference whatever path will exist at merge time.
| 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 |
There was a problem hiding this comment.
The troubleshooting steps reference .\scripts\Recovery\Repair-AcmeDnsCredential.ps1, but scripts/Recovery doesn't exist in the repo unless PR #7 lands first. To avoid broken guidance if merge order changes (or users read docs from this commit), consider updating the text to explicitly note the dependency/availability or include the recovery script in this PR.
| 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 | |
| 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). | |
| **Note:** `Repair-AcmeDnsCredential.ps1` is only available in toolkit revisions/releases that include the `scripts/Recovery` folder. If `.\scripts\Recovery\Repair-AcmeDnsCredential.ps1` is not present in your checkout, skip to the re-registration steps below instead. | |
| ```powershell | |
| # Only run this if the recovery script exists in .\scripts\Recovery\ |
| } | ||
|
|
||
| # DPAPI LocalMachine scope requires System.Security.dll | ||
| Add-Type -AssemblyName System.Security |
There was a problem hiding this comment.
Add-Type -AssemblyName System.Security can emit a non-terminating error and the script will continue until it fails later on [ProtectedData] usage. Recommend using -ErrorAction Stop with a short try/catch that throws a clear, actionable error if the assembly can't be loaded (and keep behavior consistent with Get-AcmeDnsCredential).
| Add-Type -AssemblyName System.Security | |
| try { | |
| Add-Type -AssemblyName System.Security -ErrorAction Stop | |
| } | |
| catch { | |
| throw "Failed to load required assembly 'System.Security'. DPAPI-based credential protection is unavailable. Ensure the PowerShell/.NET environment includes System.Security and then retry." | |
| } |
| $bstr = [System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($securePassword) | ||
| $plainText = [System.Runtime.InteropServices.Marshal]::PtrToStringBSTR($bstr) | ||
| $plainBytes = [System.Text.Encoding]::UTF8.GetBytes($plainText) | ||
|
|
There was a problem hiding this comment.
This code converts the SecureString back into a managed plaintext string (PtrToStringBSTR) before encrypting. Managed strings can't be reliably zeroed, so this keeps an extra plaintext copy in memory longer than necessary. If you want to minimize exposure, consider copying bytes directly from the BSTR (or building a SecureString/byte[] without creating an intermediate managed string) and avoid storing $plainText at all.
andrewyager
force-pushed
the
fix/acmedns-localmachine-dpapi-default
branch
2 times, most recently
from
e2e32fa to
a7daf44
Compare
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) <noreply@anthropic.com>
…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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
andrewyager
force-pushed
the
fix/acmedns-localmachine-dpapi-default
branch
from
9a0bf5b to
a37cb29
Compare
Summary
Register-AcmeDns.ps1now encrypts the acme-dns password underDataProtectionScope.LocalMachine(via[ProtectedData]::Protect, base64) and writesStorageMethod = "DPAPI-LocalMachine"to the credential file.Get-AcmeDnsCredential.ps1gains aDPAPI-LocalMachineswitch case and keeps the legacyDPAPIcase so older credential files still load when retrieved by the original user. The legacy decrypt-failure error now points at the recovery tool.docs/troubleshooting.mddocuments the symptom, the cause, and the migration path viaRepair-AcmeDnsCredential.ps1.docs/security-review.mdis updated to clarify that DPAPI scope is nowLocalMachine.Why
The renewal scheduled task created by
Install-Prerequisites.ps1runs as SYSTEM. Encrypting credentials under DPAPICurrentUserscope from the operator's interactive session means SYSTEM cannot decrypt them, so every automatic renewal fails silently until the certificate expires. This is a structural mismatch in the toolkit, not an operator error — anyone following the runbook hits it. Production confirmation onsyd03-ad01.internal.thecore.net.au: first renewal cycle never succeeded; cert expired before it was caught.LocalMachinescope still ties the secret to the specific host (an attacker without local administrator access cannot decrypt) but allows any local account, including SYSTEM, to read it — which is what the toolkit needs.Backwards compatibility
Existing credential files with
StorageMethod = "DPAPI"continue to work for the user that registered them. They will not work for SYSTEM-context renewals; affected hosts should runscripts/Recovery/Repair-AcmeDnsCredential.ps1once (PR #7) to migrate, then the new code path takes over.Test plan
[Parser]::ParseFileclean on both modified scriptssyd03-ad01: staged the newGet-AcmeDnsCredential.ps1against the in-placeDPAPI-LocalMachinecredential file (migrated by PR Add Repair-AcmeDnsCredential recovery tool for DPAPI scope mismatch #7's repair tool) and confirmed the password decrypts end-to-end (length 40, matches expected)Repair-AcmeDnsCredential.ps1) merged first so the troubleshooting doc reference is validCo-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com