Skip to content

Conversation

djc
Copy link
Owner

@djc djc commented Mar 17, 2025

Slicing off some more boilerplate. Doesn't feel like a big win, but might be a small one. What do you think?

@djc djc requested a review from cpu March 17, 2025 21:48
@djc djc force-pushed the finalize-names branch from 15fc227 to 0d3e283 Compare March 17, 2025 21:55
Copy link
Collaborator

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Generally looks good, but I think we can make the API a bit easier. WDYT? I'm also open to landing this as-is and then iterating.

@djc djc force-pushed the finalize-names branch from 0d3e283 to dacfeaa Compare March 18, 2025 12:30
@djc djc force-pushed the finalize-names branch 2 times, most recently from d4e9c2c to 5f9cb12 Compare March 18, 2025 12:38
@djc
Copy link
Owner Author

djc commented Mar 18, 2025

@cpu any clue what this might be about off the top of your head?

 ---- dns_01 stdout ----
Error: Api(Problem { type: Some("urn:ietf:params:acme:error:unauthorized"), detail: Some("CSR is missing Order domain \"*.wildcard.example.com\""), status: Some(403), subproblems: [] })

I guess the fact that it's about a wildcard is not a coincidence...

Copy link
Collaborator

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Nice!

@cpu
Copy link
Collaborator

cpu commented Mar 18, 2025

@cpu any clue what this might be about off the top of your head?

Oh! I have a hunch....

We're collecting up the identifiers from the order authorizations now, but wildcard identifiers are handled specially (ugh) in the authz representation. RFC 8555 7.1.4 says:

Wildcard domain names (with "*" as the first label) MUST NOT be included in authorization objects. If an authorization object conveys authorization for the base domain of a newOrder DNS identifier containing a wildcard domain name, then the optional authorizations "wildcard" field MUST be present with a value of true.

E.g. I think an order that includes *.example.com gets an authorization server-side that has the identifier "example.com", with "wildcard: true". When we process the stream for the purpose of making a CSR I think we need to respect that boolean & prepend the wildcard label ourselves.

I have to switch gears to other work this morning but can look closer if my hunch is wrong.

@djc djc force-pushed the finalize-names branch from 5f9cb12 to ec42596 Compare March 18, 2025 14:13
@djc djc changed the title Add Order::finalize_for() method to reduce boilerplate Add simplified Order::finalize() method Mar 18, 2025
@djc djc force-pushed the finalize-names branch from ec42596 to 4e7b898 Compare March 18, 2025 15:00
@djc
Copy link
Owner Author

djc commented Mar 18, 2025

We're collecting up the identifiers from the order authorizations now, but wildcard identifiers are handled specially (ugh) in the authz representation. RFC 8555 7.1.4 says:

Wildcard domain names (with "*" as the first label) MUST NOT be included in authorization objects. If an authorization object conveys authorization for the base domain of a newOrder DNS identifier containing a wildcard domain name, then the optional authorizations "wildcard" field MUST be present with a value of true.

E.g. I think an order that includes *.example.com gets an authorization server-side that has the identifier "example.com", with "wildcard: true". When we process the stream for the purpose of making a CSR I think we need to respect that boolean & prepend the wildcard label ourselves.

Oh wow, that's pretty interesting stuff... Reworked the code to try and encode this into the type system, please have a look when you have a moment.

@djc djc force-pushed the finalize-names branch from 4e7b898 to e16cc1f Compare March 18, 2025 15:22
Copy link
Collaborator

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Just a few doc nits from the latest updates 👍

@cpu
Copy link
Collaborator

cpu commented Mar 18, 2025

Oh wow, that's pretty interesting stuff...

I wish I remembered why it ended up this way 🤔 I think there was some consternation around keeping the authz identifiers as valid domain names and the wildcard stuff is a (poorly specified) property of X.509/TLS.

@djc djc force-pushed the finalize-names branch from e16cc1f to 75db8ee Compare March 18, 2025 18:04
@djc djc merged commit d4067ff into main Mar 18, 2025
9 checks passed
@djc djc deleted the finalize-names branch March 18, 2025 18:09
@cpu
Copy link
Collaborator

cpu commented Mar 18, 2025

🎉 Very cool.

It was also nice to see the Pebble integration tests (and cc92a3e) pulling their weight and catching a bug before it lands!!

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.

2 participants