Implement ACME Automated Renewal Information (ARI) #85

cpu · 2025-02-22T22:38:14Z

Note to reviewers: opened as a draft while we sort through some details. Extends the commits found in #86.

This branch implements ACME Renewal Information based on draft-07. While not yet a proposed standard RFC this draft is nearly at that stage, and has been deployed by Let's Encrypt (and I believe Google as well).

Support brings a few new bits of API surface:

a new EncodedCertificateIdentifier type that uniquely identifies a certificate for ARI compatible servers. This requires parsing specific certificate fields to construct, and so without taking a dependency users are expected to provide the relevant pki_types::Der data pre-extracted from a certificate.
for users willing to take an optional x509-parser dependency we offer a way to go from pki_types::CertificateDer<_> to a EncodedCertificateIdentifier when the x509-parser dependency feature is activated.
The DirectoryUrls type gets a new renewal_info field to track the optional ARI endpoint used to signal the ACME server supports ARI.
When the optional chrono dependency feature is activated, the Account struct gains a new renewal_info() function that accepts a EncodedCertificateIdentifier and returns RenewalInfo that the caller can use to determine when to replace the identified certificate. This function returns an error if the CA doesn't support ARI.
The NewOrder struct gains a new optional replaces(cert_id: EncodedCertificateIdentifier) function that can be used to indicate an order is a replacement for a previous order. CAs might use this information to give more generous rate limits, or to know when it's safe to revoke a previously issued certificate due to imposed compliance reasons. Like Account.renewal_info() the Account.new_order() function will error if provided a NewOrder with a replacement value if the ACME CA doesn't support ARI.

I haven't tested this with the live Let's Encrypt staging or production ACME servers, but it does work with the ARI support built into Pebble (and is now included in the integration test coverage).

src/types.rs

cpu · 2025-02-22T22:42:42Z

I think there might be some kind of a flaky race condition with the Pebble tests that also needs investigating. I'm seeing the occasional spurious error from an order's identifiers not matching the CSR identifiers 🤔

djc · 2025-02-23T06:24:11Z

Why is it useful for the identifier type to live in pki-types? What would the downside be if it living in this crate?

djc

Looking pretty good!

djc · 2025-02-23T09:14:00Z

src/types.rs

+/// See <https://www.ietf.org/archive/id/draft-ietf-acme-ari-07.html#section-4.1> for
+/// more information.
+#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
+pub struct EncodedCertificateIdentifier(pub(crate) String);


Should we make this contain a Cow<'a, str> instead?

I think since we're always format!()ing a concatenation of base64 inputs to construct this there's not an opportunity to use a &str. Am I overlooking something?

Not really -- just wondering if people might have pre-encoded certificate identifiers that they might want to splice in? Like, if you're storing a bunch of certs in your database (like we used to do at my old startup), it wouldn't be crazy to store these directly to make querying ARI easier, and in that case it feels like low-effort future-proofing to enable borrowing this down the line.

Also, how important is the Encoded prefix here? In my quest to strive for minimal entropy in type/variable names, is there another kind/encoding of certificate identifier that we need to disambiguate from?

Digging into this a little bit more, I noticed that the spec also doesn't really define the concept of a certificate identifier (maybe good spec feedback?). I think my ideal representation might be more like this:

struct CertificateId { authority_key_identifier: Cow<'a, str>, serial: Cow<'a, str>, }

(Perhaps even with pub fields?) WDYT?

src/types.rs

cpu · 2025-02-23T17:13:11Z

Why is it useful for the identifier type to live in pki-types? What would the downside be if it living in this crate?

I flip flopped a bit. Initially I was thinking there was a reason to involve rcgen, and so it seemed like a helpful bridge between instant-acme and rcgen. In practice I think it made more sense to do the heavy lifting here. I closed the pki-types PR and will rebase with a similar type defined here instead.

I think there might be some kind of a flaky race condition with the Pebble tests that also needs investigating. I'm seeing the occasional spurious error from an order's identifiers not matching the CSR identifiers 🤔

I figured this out locally. It's authz reuse & a logic bug in the test finalization logic carried forward from the provision.rs example. I'll fix shortly.

Previously both the provision example and the Pebble integration test logic created a new order for a set of input identifiers, iterated the authorizations for the resulting order, and then pulled out the identifier/challenge for each **pending** status authz. After POSTing each authz's challenge URL the associated challenge identifiers were used for the CSR sent to finalize the order. Since the logic above only processes pending authz's it would skip any that are already **valid** (e.g. from previous orders). This can end up producing a CSR that has fewer names than the initial order, which provokes an error during finalization when the CA rejects the CSR. The solution is simple: don't tie the CSR identifiers to the challenges at all, use the same list of identifiers as was used to create the order. Everything else remains the same.

It was mistakenly issuing for `dns01.example.com` like the `dns_01()` test.

By default the Pebble server reuses valid authorizations 50% of the time. To shake out authz related bugs in the issuance flow it's helpful to crank this to 100% per-test. Unfortunately since it's controlled by an env var and not part of the Pebble JSON config this requires some extra plumbing.

This commit adds an integration test that uses Pebble w/ 100% valid authorization reuse to confirm the issuance logic works correctly when only one of three identifiers in a subsequent order need challenge validation.

Also add more detail to the pre-reqs for running the Pebble tests.

See https://www.ietf.org/archive/id/draft-ietf-acme-ari-07.html#section-3

This type represents the BASE64 URL-safe encoding of a certificate's DER encoded authority key information (AKI) ext's keyIdentifier field, and the BASE64 URL-safe encoding of a certificate's raw DER encoded serial number. Both encodings are then separated by a "." character for use in ARI related requests. For now this can only be constructed directly from a raw DER and the caller is responsible for extracting the right values from a certificate. Subsequent commits will offer a way to construct one from certificate DER using an optional x509-parser dependency.

This error type can be used when a feature is requested that we know the ACME server doesn't support (e.g. based on the absence of a URL from the directory resource).

Instead of constructing a `NewOrder` instance by populating the `pub identifiers` field directly, use `NewOrder::new()` and make the internal fields private. This will let us switch to a builder-model for future customization.

By optionally specifying a `EncodedCertificateIdentifier` when constructing a `NewOrder` it's possible to indicate to the ACME CA that the order is a replacement for a pre-existing certificate that was previously ordered.

Add an optional, non-default, feature for `x509-parser` that allows constructing `EncodedCertificateIdentifier` instances from certificate DER.

This will allow using the issued certificate for further testing (e.g. revocation, replacement order).

In the near future it may contain IP address identifiers.

This will support making it easier for each test to construct its own identifiers up-front.

This will make it ergonomic to test things like order replacement by calling `replaces()` on the `NewOrder` before providing it to `test()`.

Also activate the optional `x509-parser` feature for the CI invocation.

Adds a new `Account.renewal_info()` function that takes an `EncodedCertificateIdentifier` and returns the `RenewalInfo` the CA suggests for that certificate.

Adds a new `Account.renewal_info()` function that takes an `EncodedCertificateIdentifier` and returns the `RenewalInfo` the CA suggests for that certificate. All of the above is gated by an optional, non-default, `chrono` feature, using that crate to parse the RFC3339 formatted date/time stamps in the renewal information responses.

Updates the ARI integration test to revoke the initial certificate, and then verifying the ARI info suggests immediate replacement before replacing it.

cpu · 2025-02-23T20:01:40Z

I rebased this on top of #86 and implemented the review feedback so far.

cpu · 2025-02-23T20:07:50Z

src/types.rs

@@ -1,18 +1,24 @@
 use std::fmt;

+use crate::BytesResponse;
+use crate::crypto::{self, KeyPair};


oops, biffed the import order in my rebase. I'll fix this.

cpu · 2025-02-23T20:08:24Z

Cargo.toml

@@ -23,6 +23,7 @@ async-trait = "0.1"
 aws-lc-rs = { version = "1.8.0", optional = true }
 base64 = "0.22"
 bytes = "1"
+chrono = { version = "0.4", features = ["serde"], optional = true }


ah, this commit was meant to be squashed into its parent. Will fix.

Hmm, also on consideration, rcgen and yasna depend on time instead of chrono today. I wonder if that mean we should use time instead of chrono here?

Also, make sure that we have doc_auto_cfg set up so renewal_info() will be discoverable in the docs even when cfged out by default.

cpu · 2025-02-23T21:56:45Z

This branch has conflicts that must be resolved

Will fix this & the other nits from self-review shortly. Out of juice for today

djc · 2025-02-23T22:25:15Z

Yup, won't take a look at it until tomorrow either.

djc · 2025-02-24T13:15:06Z

src/types.rs

+/// See <https://www.ietf.org/archive/id/draft-ietf-acme-ari-07.html#section-4.1> for
+/// more information.
+#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
+pub struct EncodedCertificateIdentifier(pub(crate) String);


Not really -- just wondering if people might have pre-encoded certificate identifiers that they might want to splice in? Like, if you're storing a bunch of certs in your database (like we used to do at my old startup), it wouldn't be crazy to store these directly to make querying ARI easier, and in that case it feels like low-effort future-proofing to enable borrowing this down the line.

Also, how important is the Encoded prefix here? In my quest to strive for minimal entropy in type/variable names, is there another kind/encoding of certificate identifier that we need to disambiguate from?

Digging into this a little bit more, I noticed that the spec also doesn't really define the concept of a certificate identifier (maybe good spec feedback?). I think my ideal representation might be more like this:

struct CertificateId { authority_key_identifier: Cow<'a, str>, serial: Cow<'a, str>, }

(Perhaps even with pub fields?) WDYT?

djc · 2025-02-24T13:38:44Z

tests/pebble.rs

@@ -184,6 +184,17 @@ fn try_tracing_init() {
        .try_init();
 }

+fn dns_identifiers<I, T>(dns_names: I) -> Vec<Identifier>


Nits: move this down below its user? Also use impl Trait to remove the indirection, I think we could write names: impl IntoIterator<Item = impl ToString>?

djc · 2025-02-24T13:45:18Z

Cargo.toml

@@ -23,6 +23,7 @@ async-trait = "0.1"
 aws-lc-rs = { version = "1.8.0", optional = true }
 base64 = "0.22"
 bytes = "1"
+chrono = { version = "0.4", features = ["serde"], optional = true }


Hmm, also on consideration, rcgen and yasna depend on time instead of chrono today. I wonder if that mean we should use time instead of chrono here?

cpu mentioned this pull request Feb 22, 2025

v0.12.0, add AcmeRenewalInfoCertIdentifier rustls/pki-types#75

Closed

cpu commented Feb 22, 2025

View reviewed changes

src/types.rs Show resolved Hide resolved

djc reviewed Feb 23, 2025

View reviewed changes

cpu added 4 commits February 23, 2025 12:33

pebble: fix tls_alpn_01 test hostname

eb96e09

It was mistakenly issuing for `dns01.example.com` like the `dns_01()` test.

pebble: test authorization reuse

7ad1513

This commit adds an integration test that uses Pebble w/ 100% valid authorization reuse to confirm the issuance logic works correctly when only one of three identifiers in a subsequent order need challenge validation.

cpu mentioned this pull request Feb 23, 2025

Fix authorization reuse in provision.rs and the Pebble integration tests #86

Merged

cpu added 17 commits February 23, 2025 13:11

pebble: fix stale PebbleEnvironment ref

8ae8181

Also add more detail to the pre-reqs for running the Pebble tests.

types: add renewalInfo endpoint to DirectoryUrls

bac8313

See https://www.ietf.org/archive/id/draft-ietf-acme-ari-07.html#section-3

types: make Error non-exhaustive

11e23f2

types: add Error::Unsupported

4ceef6b

This error type can be used when a feature is requested that we know the ACME server doesn't support (e.g. based on the absence of a URL from the directory resource).

types: make OrderState non-exhaustive

c9714af

types: make NewOrder internals private

bc0a4fc

Instead of constructing a `NewOrder` instance by populating the `pub identifiers` field directly, use `NewOrder::new()` and make the internal fields private. This will let us switch to a builder-model for future customization.

implement replacement orders

4e9f507

By optionally specifying a `EncodedCertificateIdentifier` when constructing a `NewOrder` it's possible to indicate to the ACME CA that the order is a replacement for a pre-existing certificate that was previously ordered.

support EncodedCertificateIdentifier from cert DER

c391018

Add an optional, non-default, feature for `x509-parser` that allows constructing `EncodedCertificateIdentifier` instances from certificate DER.

pebble: return issued EE cert DER from test

7578424

This will allow using the issued certificate for further testing (e.g. revocation, replacement order).

types: make Identifier non-exhaustive

09bed7e

In the near future it may contain IP address identifiers.

pebble: add helper for constructing dns idents

2434d8d

This will support making it easier for each test to construct its own identifiers up-front.

pebble: pass &NewOrder from individual tests

e95644a

This will make it ergonomic to test things like order replacement by calling `replaces()` on the `NewOrder` before providing it to `test()`.

pebble: add ARI replacement order test

34eac61

Also activate the optional `x509-parser` feature for the CI invocation.

implement fetching ARI RenewalInfo

79f6ad6

Adds a new `Account.renewal_info()` function that takes an `EncodedCertificateIdentifier` and returns the `RenewalInfo` the CA suggests for that certificate.

pebble: add ARI renewal info testing

615c319

Updates the ARI integration test to revoke the initial certificate, and then verifying the ARI info suggests immediate replacement before replacing it.

cpu force-pushed the cpu-ari-ci branch from 18577ca to 615c319 Compare February 23, 2025 19:57

cpu commented Feb 23, 2025

View reviewed changes

djc reviewed Feb 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ACME Automated Renewal Information (ARI) #85

Implement ACME Automated Renewal Information (ARI) #85

cpu commented Feb 22, 2025 •

edited

Loading

cpu commented Feb 22, 2025

djc commented Feb 23, 2025

djc left a comment

djc Feb 23, 2025

cpu Feb 23, 2025

djc Feb 24, 2025

cpu commented Feb 23, 2025 •

edited

Loading

cpu commented Feb 23, 2025

cpu Feb 23, 2025

cpu Feb 23, 2025

djc Feb 24, 2025

djc Feb 24, 2025

cpu commented Feb 23, 2025

djc commented Feb 23, 2025

djc Feb 24, 2025

djc Feb 24, 2025

djc Feb 24, 2025

Implement ACME Automated Renewal Information (ARI) #85

Are you sure you want to change the base?

Implement ACME Automated Renewal Information (ARI) #85

Conversation

cpu commented Feb 22, 2025 • edited Loading

cpu commented Feb 22, 2025

djc commented Feb 23, 2025

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Feb 23, 2025 • edited Loading

cpu commented Feb 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Feb 23, 2025

djc commented Feb 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Feb 22, 2025 •

edited

Loading

cpu commented Feb 23, 2025 •

edited

Loading