Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renewal window not capped at 30 days #382

Closed
lookcloser opened this issue Sep 17, 2021 · 5 comments
Closed

Renewal window not capped at 30 days #382

lookcloser opened this issue Sep 17, 2021 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@lookcloser
Copy link

I've noticed that all of our orders for certificates with a 1 year lifespan have their RenewAfter date set to 4 months before the CertExpires date.

Based on the comments starting at line 59 of https://github.com/rmbolger/Posh-ACME/blob/main/Posh-ACME/Public/Complete-PAOrder.ps1, I believe that the RenewAfter date should never be earlier than 30 days before the CertExpires date.

I think this is happening because line 65 of this file should be calling [Math]::Min rather than [Math]::Max.

Thanks.

@rmbolger
Copy link
Owner

Well that's...rather embarrassing. You're absolutely right. Thanks a ton, @lookcloser. I'll get it fixed shortly.

@rmbolger rmbolger self-assigned this Sep 17, 2021
@rmbolger rmbolger added the bug Something isn't working label Sep 17, 2021
@rmbolger
Copy link
Owner

rmbolger commented Sep 17, 2021

Fix is now in main. I may not push a new release until next week though. I also wrote a little fix-it script you can use in the mean time that will adjust the renewal windows of your existing orders.

Throw this in a file called something like Update-RenewalWindows.ps1:

#Requires -Modules Posh-ACME

[CmdletBinding()]
param()

Get-PAOrder -List | ForEach-Object {
    # skip orders without a cert
    if (-not ($cert = $_ | Get-PACertificate)) { return }

    # calculate the appropriate RenewAfter value
    $notBefore = $cert.NotBefore.ToUniversalTime()
    $notAfter = $cert.NotAfter.ToUniversalTime()
    $lifetime = $notAfter - $notBefore
    $renewHours = [Math]::Min(720, ($lifetime.TotalHours / 3))
    $renewAfter = $notAfter.AddHours(-$renewHours).ToString('yyyy-MM-ddTHH:mm:ssZ', [Globalization.CultureInfo]::InvariantCulture)

    # grab the order JSON
    $orderFile = Join-Path $_.Folder 'order.json'
    $orderRaw = Get-Content $orderFile -Raw | ConvertFrom-Json

    # un-DateTime'ify the RenewAfter value if we're in PS Core
    if ($orderRaw.RenewAfter -is [DateTime]) {
        $orderRaw.RenewAfter = $orderRaw.RenewAfter.ToString('yyyy-MM-ddTHH:mm:ssZ', [Globalization.CultureInfo]::InvariantCulture)
    }

    # save the updated value if the existing value is different
    if ($orderRaw.RenewAfter -ne $renewAfter) {
        Write-Verbose "Fixing renewal window $($_.RenewAfter) -> $renewAfter for order '$($_.Name)'"
        $orderRaw.RenewAfter = $renewAfter
        $orderRaw | ConvertTo-Json -Depth 10 | Out-File $orderFile -Force
    }
}

Import-Module -Name Posh-ACME -Force

Then run it like this:

.\Update-RenewalWindows.ps1 -Verbose

@lookcloser
Copy link
Author

Thanks for fixing the bug and writing the script.

I think the script needs to have the following line added after the Write-Verbose line:

$orderRaw.RenewAfter = $renewAfter

Otherwise the $orderRaw object will never be updated with the corrected renewal date and the incorrect renewal date will just get written to the file again.

I added this line to my local copy of the script and it seems to be working fine after doing this.

Thanks.

@rmbolger
Copy link
Owner

Whoops. Yes, you're correct about the script tweak. Modified the comment. Thanks!

@rmbolger
Copy link
Owner

The fix is now live in v4.9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants