sslcertificate::from_pem - add switch to remove duplicates immediately

[andrew-davison]

sslcertificate::from_pem currently leaves expired certificates around for 30 days, before it removes them.
This causes issues with NPS still sometimes trying to use the old certificate and breaking WiFi auth.

This PR adds a switch of remove_immediately that we can pass from puppet when using the module on a server.
The existing logic gets certificates from the local cert store that match the certificate name, looks at the expiry date, checks if it's more than 30 days ago and removes them.
The new logic if the switch is used should get certificates from the local store, matches the name, then checks the expiry date again the current one being stored in Vault, and removes them if they don't match. which should get rid of dupes on the first puppet run after there are multiples.

This Puppet resources already notifies the IAS service, so should restart it and confirm it's using the current cert.

Is this a sensible plan?
Do we still want to keep expired certs around for 30 days? I assume this was a thing for when we weren't positive the cert renewal process worked?
Would we prefer to just compare each certs notafter date to todays date and remove them the first puppet run after they expire?

[simonlye]

Is the if $remove_expired_certs section now duplicated when it shouldn't be?
I presume this will only be applied to servers where having the current 30 day past expiry removal causes issues, such as NPS?

[andrew-davison]

@simonlye spot on, that duplicate wasn't supposed to be there.
Yes, it's designed to be an option for places that expired certificates can cause issues, e.g. NPS.

[rmc47]

If the intent of this is to always have exactly one cert with a given name, then I might suggest comparing thumbprints rather than ExpiresAfter dates - these are guaranteed to always be unique.

If the intent is to have no expired certificates, then I'd compare the ExpiresAfter to the current timestamp (but there's still a window of one puppet-run's duration).

[andrew-davison]

Have re-worked to just compare the current date to NotAfter.
Also switched the parameter to an optional Integer, so we can just set the number of days to remove the expired cert after.
We've never specifically set the old parameter to False anywhere so the change in name for clarity won't break anything, but allowing Undef does leave that as an option still.

[rmc47]

Sounds good. If I were being picky, I'd suggest multiplying by -1 instead of using the infix - operator: currently, you can't pass a negative integer to remove_expired_certs_after, as it'd result in .AddDays(--1))), which isn't valid Powershell, whereas * -1 would result in .AddDays(-1 * -1), which is .AddDays(1).

With remove_expired_certs_after set to zero, if the cert expires at 10am, Puppet won't remove it until (perhaps) 10:20am. Maybe that's a short enough window not to care, but if you had the option of setting it to -1, then Puppet'd remove them a day before they expired, which should be well after the new cert was installed.

[njhowell]

Make it so