return logical and compliant ARI windows for expiring certs by jvanasco · Pull Request #484 · letsencrypt/pebble

jvanasco · 2025-02-15T22:03:53Z

While testing ARI functionality with extremely short lived certs, Pebble returned illogical data - such as the windowEnd appearing in the future for already expired certs, or after the cert's notAfter.

This PR adjusts RenewalInfoSimple to do the following:

if the cert has expired, just return RenewalInfoImmediate
if the cert will expire before the windowEnd, set the windowEnd to the cert's notAfter
cleanup: RFC requires the windowStart to be before windowEnd, so remove a second from the windowStart if needed.

I also made the RenewalInfoImmediate window change from -60:-30 minutes to -60:-59.59 minutes; the sole reason for this change is that it is visually easier to see the change (and not need to do mental math) and recognize this is a RenewalInfoImmediate from logs.

There are likely better ways to accomplish this, and I may be overlooking some elements from the RFC.

With these changes, Pebble can be configured for certificate validity periods as low as 5-10seconds, enabling quick testing of real (unmocked) flows such as:

issue cert
query ari
wait for expiry or query ari
renew with replaces

Also: The linting errors existed on creating my fork; there may be some linting errors in this PR - but there is a library issue.

If the windowEnd is after the cert's expiry, adjust it to the window end.

aarongable · 2025-02-19T01:19:34Z

+    if expires.Before(now) {
+        // If the cert has expired, return `RenewalInfoImmediate` instead.
+        return RenewalInfoImmediate(now)
+    }
+
+    if expires.Before(windowEnd) {
+        // If the windowEnd is after the cert's expiry, adjust it to the window end.
+        windowStart = expires.Add(-1 * time.Second)
+        windowEnd = expires
+    }
+
+    if !windowEnd.After(windowStart) {
+        // Ensure windowStart is before windowEnd
+        windowEnd = expires.Add(-1 * time.Second)
+    }


There is a lint error here: these lines all have incorrect indentation.

More importantly, I think there's a much simpler version of this logic that accomplishes the same goal:

If windowEnd is after expires, make it equal to expires

If windowStart is before issued, make it equal to issued

True, this doesn't explicitly protect against a certificate which has a zero-second validity period, but I'm not worried about that case.

I did the first element and changed initial spacing. I don't know why my editor was auto-expanding tabs here. I added the windowStart change to issued as well.

I left the last element, however, ensuring windowStart is BEFORE windowEnd - and included a reference to the RFC. The RFC draft explicitly requires windowStart to precede windowEnd, and the payload is invalid otherwise.

Striving to simplify logic has caveats. I prioritized ensuring an ARI compliant payload over edge cases of future notBefore values or zero second validity.

jvanasco

Some requested changes were made; some alternatives were presented.

jvanasco · 2025-02-19T20:45:59Z

+    if expires.Before(now) {
+        // If the cert has expired, return `RenewalInfoImmediate` instead.
+        return RenewalInfoImmediate(now)
+    }
+
+    if expires.Before(windowEnd) {
+        // If the windowEnd is after the cert's expiry, adjust it to the window end.
+        windowStart = expires.Add(-1 * time.Second)
+        windowEnd = expires
+    }
+
+    if !windowEnd.After(windowStart) {
+        // Ensure windowStart is before windowEnd
+        windowEnd = expires.Add(-1 * time.Second)
+    }


I did the first element and changed initial spacing. I don't know why my editor was auto-expanding tabs here. I added the windowStart change to issued as well.

I left the last element, however, ensuring windowStart is BEFORE windowEnd - and included a reference to the RFC. The RFC draft explicitly requires windowStart to precede windowEnd, and the payload is invalid otherwise.

Striving to simplify logic has caveats. I prioritized ensuring an ARI compliant payload over edge cases of future notBefore values or zero second validity.

aarongable · 2025-02-19T20:49:09Z

+	}
+
+	// Ensure RenewalWindow is not after the expiry
+	if windowEnd.After(expires){


You still have formatting issues here, and below. You should be running gofmt automatically.

Suggested change

if windowEnd.After(expires){

if windowEnd.After(expires) {

Ah! It's not performing on directories, just specific files. Fixed.

aarongable · 2025-02-19T20:52:12Z

+	if expires.Before(now) {
+		// If the cert has expired, return `RenewalInfoImmediate` instead.
+		return RenewalInfoImmediate(now)
+	}


This is redundant: if we're clamping windowEnd to expires, then an expired cert will always be given a window that is wholly in the past. No need to special case it here. This is why I suggested the removal of this stanza in my previous review.

If you want to keep it anyway, move it above the computation of the ideal window, since none of that computation is necessary for this stanza.

Oh, that makes sense. I just removed it.

aarongable · 2025-02-19T20:57:32Z

+	// Ensure correct start for future issueds
+	if windowStart.After(issued){


I didn't suggest this to handle issued timestamps that are in the future. I suggested this to handle the case where the certificate has a validity period of less than 48 hours, so that subtracting 24 hours from a point in the middle of the validity period results in a windowStart that precedes the issued timestamp.

Also, you've reversed the logic from what I suggested. This should be if windowStart.Before(issued) {.

Sorry, I was trying to do this in one window while debugging another upstream project in the cloud.

jvanasco · 2025-02-19T21:19:50Z

This should have all the requested changes and formatting. Apologies. I've also tested it against with a client as I couldn't find any ARI tests here.

* 'main' of https://github.com/letsencrypt/pebble: (35 commits) add overriding of ARI response (letsencrypt#501) wfe: fix a race in `orderForDisplay` (letsencrypt#500) Bump golang.org/x/ dependencies (letsencrypt#499) currectly triggers BadSignatureAlgorithmProblem at JWS parse time (letsencrypt#492) use newer validation subdomain for dns-account-01 (fix CI eggsampler/acme error) (letsencrypt#498) Orders don't have a "deactivated" status. (letsencrypt#301) Update golangci-lint (letsencrypt#488) build(deps): bump github.com/go-jose/go-jose/v4 from 4.0.4 to 4.0.5 (letsencrypt#487) Truncate ARI timestamps to millisecond resolution (letsencrypt#485) return logical and compliant ARI windows for expiring certs (letsencrypt#484) Update dependencies (letsencrypt#481) docs: rm mention of subproblems being unimpl'd (letsencrypt#479) Fix(NOISSUE): Fix docker compose file example in README.md (letsencrypt#475) Add support for ACME Profiles (letsencrypt#473) Simplify KU, EKU, and SKID fields of issued certs (letsencrypt#472) Update golangci-lint to 1.60.2 (letsencrypt#474) Update /x/net for compatibility with go1.23 (letsencrypt#470) Reject extra command line args and fix README invocation (letsencrypt#467) Document exposing API and management ports when not using docker-compose.yaml (letsencrypt#465) Implement latest draft-ietf-acme-ari spec (letsencrypt#461) ...

jvanasco added 5 commits February 15, 2025 16:45

If the cert has expired, return RenewalInfoImmediate instead.

0eab646

If the windowEnd is after the cert's expiry, adjust it to the window end.

RFC compliance

df3e455

move comments inline

393df53

fixed caps that had reverted

36973ba

reduce expired past to 1s, simply because it's easier for a visual check

c5d928a

aarongable reviewed Feb 19, 2025

View reviewed changes

jvanasco added 2 commits February 19, 2025 15:32

response to requested changes

110bbdd

issued concern

b609fb5

jvanasco commented Feb 19, 2025

View reviewed changes

aarongable requested changes Feb 19, 2025

View reviewed changes

requested changes

99c8bb6

jvanasco mentioned this pull request Feb 21, 2025

Add support for draft-ieft-ari-08 certbot/certbot#9945

Closed

4 tasks

aarongable approved these changes Feb 21, 2025

View reviewed changes

aarongable merged commit bc21177 into letsencrypt:main Feb 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

return logical and compliant ARI windows for expiring certs#484

return logical and compliant ARI windows for expiring certs#484
aarongable merged 8 commits into
letsencrypt:mainfrom
jvanasco:main

jvanasco commented Feb 15, 2025 •

edited

Loading

Uh oh!

Uh oh!

aarongable Feb 19, 2025

Uh oh!

jvanasco Feb 19, 2025

Uh oh!

jvanasco left a comment

Uh oh!

jvanasco Feb 19, 2025

Uh oh!

aarongable Feb 19, 2025

Uh oh!

jvanasco Feb 19, 2025

Uh oh!

aarongable Feb 19, 2025

Uh oh!

jvanasco Feb 19, 2025

Uh oh!

aarongable Feb 19, 2025

Uh oh!

jvanasco Feb 19, 2025

Uh oh!

jvanasco commented Feb 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// Ensure correct start for future issueds
		if windowStart.After(issued){

Conversation

jvanasco commented Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvanasco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvanasco commented Feb 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jvanasco commented Feb 15, 2025 •

edited

Loading