Skip to content

Conversation

@bavshin-f5
Copy link
Member

Taking suggestions for directive syntax or documentation. I want to keep the --preferred-profile/--required-profile distinction, but maybe that's not the best way.

Verified working with:

  • pebble
  • Let's Encrypt staging with shortlived and IP identifiers

Curious testing artifact:

[warn] 1#1: urn:ietf:params:acme:error:unauthorized: Error creating new order :: account ID 1234567890 is not permitted to use certificate profile "shortlived" while updating certificate "letsencrypt/example.com"

Which doesn't quite look like urn:ietf:params:acme:error:invalidProfile. Neither is malformed from Pebble.

@bavshin-f5 bavshin-f5 requested a review from Copilot October 21, 2025 17:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for the ACME Profiles Extension (draft-ietf-acme-profiles-00), allowing users to request specific certificate profiles from ACME servers. The implementation provides two modes: preferred profiles (gracefully degrade if unsupported) and required profiles (fail if unsupported).

Key changes:

  • Added profile directive to acme_issuer configuration with optional require parameter
  • Implemented profile validation during account registration and certificate ordering
  • Added comprehensive test coverage for both default and short-lived certificate profiles

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
t/acme_profiles.t Adds test suite verifying profile functionality with default and short-lived certificates
src/conf/issuer.rs Defines Profile enum and adds profile field to Issuer struct
src/conf.rs Implements profile directive configuration parser supporting 1-2 arguments
src/acme/types.rs Extends ACME types to include profiles in directory metadata, order requests, and error handling
src/acme/error.rs Adds profile-related error variant to NewAccountError
src/acme.rs Implements profile validation logic and includes profile in order requests
README.md Documents the new profile directive and references the ACME Profiles Extension specification

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@bavshin-f5 bavshin-f5 force-pushed the draft-ietf-acme-profiles branch 2 times, most recently from b0a3303 to 72ef19b Compare October 22, 2025 17:31
Copy link
Contributor

@ensh63 ensh63 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTreeMap looks slightly overwhelmed solution for such a small collection as profile list, but it has predefined de-serialization support. So, this is a reasonable choice.
Looks good overall.

@bavshin-f5 bavshin-f5 force-pushed the draft-ietf-acme-profiles branch from 72ef19b to 7a5e27a Compare October 28, 2025 00:43
@bavshin-f5
Copy link
Member Author

The latest fixup updates handling of required profiles and invalidProfile errors:

  • urn:ietf:params:acme:invalidProfile can be returned if the order configuration is not compatible with the requested profile (e.g. unsupported identifier types). Reduced scope of the error to the current order.
  • Certbot sends --required-profile to the ACME server instead of validating locally against the advertised profile list. The mailing list discussions suggest that it is acceptable to have profiles that are not publicly advertised through the directory metadata.

@bavshin-f5 bavshin-f5 force-pushed the draft-ietf-acme-profiles branch 2 times, most recently from ff993dd to 1dbe801 Compare November 4, 2025 20:23
@bavshin-f5 bavshin-f5 requested review from Copilot and xeioex November 4, 2025 20:24
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bavshin-f5 bavshin-f5 force-pushed the draft-ietf-acme-profiles branch from 1dbe801 to a6abe56 Compare November 4, 2025 20:33
Copy link
Contributor

@xeioex xeioex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Merging all 3 patches into 1 seems more natural to me, but YMMV.

@bavshin-f5 bavshin-f5 merged commit e5dad32 into nginx:main Nov 5, 2025
20 checks passed
@bavshin-f5 bavshin-f5 deleted the draft-ietf-acme-profiles branch November 5, 2025 00:21
@bavshin-f5 bavshin-f5 added this to the 0.3.0 milestone Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants