ACME: alternative chains support. #82
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for selecting alternative certificate chains from ACME servers. When multiple chains are available, users can specify a preferred chain by matching the issuer's Common Name of the topmost certificate.
Key changes:
- Added
chaindirective to specify preferred certificate chain by issuer name - Implemented RFC8288-compliant Link header parser to discover alternate chains
- Modified certificate retrieval to parse and compare X.509 certificates against the matcher
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| t/lib/Test/Nginx/ACME.pm | Added test support for alternate roots and updated certificate retrieval method |
| t/acme_alternative_chains.t | New test file validating chain selection with multiple alternate roots |
| src/lib.rs | Updated to use parsed X.509 certificates and new certificate bytes field |
| src/conf/issuer.rs | Added CertificateChainMatcher struct to match chains by issuer CN |
| src/conf.rs | Added command handler for the new chain directive |
| src/acme/headers.rs | New RFC8288-compliant Link header parser for discovering alternate chains |
| src/acme/error.rs | Updated error variants for certificate handling |
| src/acme.rs | Implemented chain discovery and selection logic using Link headers |
| README.md | Documented the new chain directive |
| Cargo.toml | Added iri-string dependency for URI resolution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bavshin-f5
force-pushed
the
alternative-chains
branch
2 times, most recently
from
6d1117c to
edbe668
Compare
xeioex
left a comment
xeioex
left a comment
There was a problem hiding this comment.
preferred_chain ... looks much better than chain "issuers=..." syntax to me.
Overall I find it a good name for the feature.
The code looks very good.
The new directive, "preferred_chain", allows selecting one of the alternative certificate chains (RFC8555 § 7.4.2) by a Subject Common Name that issued the topmost certificate in the chain. If no suitable chains are found, the default one will be used. To fetch the alternative chains, we need a rather complicated parser for RFC8288 Link header. After considering the correctness, maintenance status and dependency weight of the available implementations, I decided to add our own instead, simplified as much as possible.
bavshin-f5
force-pushed
the
alternative-chains
branch
from
a3e44f8 to
cd3baf0
Compare
laurelmay
left a comment
laurelmay
left a comment
There was a problem hiding this comment.
The preferred_chain "ISSUER_NAME" syntax seems straightforward and matches the most common terminology; trying to support even further options for matching might make it harder to parse and document the user's choice. This definitely meets was I was hoping for when #35 was written and I look forward to being able to use it!
| let default = | ||
| X509::stack_from_pem(cert.body()).map_err(NewCertificateError::InvalidCertificate)?; |
There was a problem hiding this comment.
In a properly formatted response, it shouldn't happen (though, could it given some data newer than our OpenSSL version?), but would it make sense to delay the map_err until the time where default becomes the chosen chain (a bit like is_empty)? By delaying that check, it may be possible to allow working around errors where parsing the default certificate fails for some reason, allowing administrators to still specifically configure a preferred chain. That probably requires further extracting the actual search logic within the if though, since it requires treating the Err(_) and Some(stack) if !matcher.test(stack) cases the same.
There was a problem hiding this comment.
I don't believe this degree of resilience is necessary. Parsing errors are actually unlikely here, because the certificate format haven't changed for a few decades and we are in control of the key algorithm. In addition, the case when the whole response is junk (PEM_R_NO_START_LINE) is already translated to an Ok() with an empty Vec.
What we can encounter is either a memory allocation error, or a corrupted data from the CA, and neither should be recoverable.
3 participants
Tested with Peble, LE staging/prod.
Tested for regressions with Dogtag, Step CA, Vault etc.
Linkheader parser should be mostly compliant with RFC8288, to the extent necessary to find all the links withrel=absoluteand resolve the URIs. Feel free to tell me if it needs more comments or a separate commit.The syntax (
chainissuer=name) is a bit raw. I'm still deciding if I should:requireparameter, similar to the profile.alternative_chain?preferred_chain?)issuer=toissuer_name=or drop itSuggestions and feedback are welcome.
Fixes #35