Per-type challenge state + handles#283
Conversation
|
Want to rebase this now that #280 has been merged? |
49973d4 to
e4e5c47
Compare
Done! |
djc
left a comment
djc
left a comment
There was a problem hiding this comment.
Here's some early feedback. Need to think more about all the different challenge handling stuff...
| pub struct Http01Challenge { | ||
| /// A token for constructing a key authorization to complete this challenge | ||
| pub token: String, | ||
| } | ||
|
|
||
| /// Challenge state for an RFC 8555 dns-01 challenge | ||
| /// | ||
| /// See <https://www.rfc-editor.org/rfc/rfc8555#section-8.4> | ||
| #[derive(Clone, Debug, Deserialize, Eq, PartialEq)] | ||
| #[non_exhaustive] | ||
| pub struct Dns01Challenge { | ||
| /// A token for constructing a key authorization to complete this challenge | ||
| pub token: String, | ||
| } | ||
|
|
||
| /// Challenge state for an RFC 8737 tls-alpn-01 challenge | ||
| /// | ||
| /// See <https://www.rfc-editor.org/rfc/rfc8737#section-3> | ||
| #[derive(Clone, Debug, Deserialize, Eq, PartialEq)] | ||
| #[non_exhaustive] | ||
| pub struct TlsAlpn01Challenge { | ||
| /// A token for constructing a key authorization to complete this challenge |
There was a problem hiding this comment.
Not totally convinced it's worth having 3 different types for these.
There was a problem hiding this comment.
I tried to motivate this in the PR description but I think it's a worthwhile trade-off to make the API prevent misuse. I think it's a mistake to encourage thinking about all challenges as being equivalent. By specification they aren't ever satisfied in a uniform way and I think the API should reflect that.
As one example from the current API send_device_attestation() exists on Order, and at runtime produces an error for challenges other than ChallengeType::DeviceAttest01. It seems preferable to me that there never be a code path that compiles for trying to use the device attest challenge response submission for an HTTP-01/DNS-01/TLS-ALPN-01/etc challenge.
Similarly, the current API lets you get the BASE64 encoding of the SHA-256 digest of the key authorization of an HTTP-01 challenge token, but that's the incorrect format to use for provisioning a HTTP-01 challenge response. That format only make sense for DNS-01, not HTTP-01 or TLS-ALPN-01. Similar for getting the raw SHA-256 digest of the token without the BASE64 encoding, that's specific to TLS-ALPN-01.
Without separate types we have to offer each of these presentation options and rely on documentation to point out which should be used where, and I think it'd be better to let the type system enforce that.
Does that help convince you? I'm open to other solutions if you think there's a middle ground that can offer the same benefits with fewer moving pieces.
There was a problem hiding this comment.
Pff, sorry for the long delay.
I'm open to the idea of more strongly anchoring the challenges and their associated state/actions in the type system. I think the per-challenge state types look okay but it feels like there is too much boilerplate in the current iteration of the type-specific challenge handle. I'd also suggest leaving out some of the extra stuff like the PREFIX and webroot_path() stuff at least for now -- I'm inclined to think users probably don't need help solving that part?
Maybe there should also be a separate challenges.rs module with inline modules per challenge type, to make things easier to navigate?
It's more of a data class and I think fits better alongside the challenge types (vs the challenge handles).
It's more and more common for the challenge types to be divergent in state, and authorization mechanism. Let's start to reflect this better in the type system.
Handling authorization challenges requires per-type specific knowledge, e.g. whether the challenge should get an empty POST-as-GET to mark it ready (not applicable for device-attest-01), or whether the challenge response is provisioned with a key authorization (not applicable to device-attest-01, dns-persist-01) and which format (e.g. for a DNS-01 TXT record, or a TLS-ALPN-01 challenge certificate extension).
How to provision a challenge response to complete an authz depends on the challenge type. E.g. when provisioning an HTTP-01 response, you shouldn't use the SHA256 digest of the key auth (that's specific to TLS-ALPN-01) or the base64 encoding of that digest (that's specific to DNS-01). Similarly, there's extra context that's useful to provide to callers that depends on the challenge type context (e.g. the hostname for a DNS-01 TXT record, or the file path for an HTTP-01 webroot response file). By removing the common `KeyAuthorization` type in favour of per-challenge type authorization types that the challenge type handles produce we can use the type system to guide callers to the right process/data for their challenge.
In theory an ACME CA won't produce challenges that don't support the authorization's identifier type, but just in case we should make sure that we don't produce a challenge handle for an identifier type it isn't spec'd to support.
e4e5c47 to
de24537
Compare
|
|
||
| impl ChallengeState { | ||
| /// Get the challenge type associated with this challenge state | ||
| pub fn r#type(&self) -> ChallengeType { |
There was a problem hiding this comment.
Nit: I think this should be below token().
| pub struct Http01Challenge { | ||
| /// A token for constructing a key authorization to complete this challenge | ||
| pub token: String, | ||
| } | ||
|
|
||
| /// Challenge state for an RFC 8555 dns-01 challenge | ||
| /// | ||
| /// See <https://www.rfc-editor.org/rfc/rfc8555#section-8.4> | ||
| #[derive(Clone, Debug, Deserialize, Eq, PartialEq)] | ||
| #[non_exhaustive] | ||
| pub struct Dns01Challenge { | ||
| /// A token for constructing a key authorization to complete this challenge | ||
| pub token: String, | ||
| } | ||
|
|
||
| /// Challenge state for an RFC 8737 tls-alpn-01 challenge | ||
| /// | ||
| /// See <https://www.rfc-editor.org/rfc/rfc8737#section-3> | ||
| #[derive(Clone, Debug, Deserialize, Eq, PartialEq)] | ||
| #[non_exhaustive] | ||
| pub struct TlsAlpn01Challenge { | ||
| /// A token for constructing a key authorization to complete this challenge |
There was a problem hiding this comment.
Pff, sorry for the long delay.
I'm open to the idea of more strongly anchoring the challenges and their associated state/actions in the type system. I think the per-challenge state types look okay but it feels like there is too much boilerplate in the current iteration of the type-specific challenge handle. I'd also suggest leaving out some of the extra stuff like the PREFIX and webroot_path() stuff at least for now -- I'm inclined to think users probably don't need help solving that part?
Maybe there should also be a separate challenges.rs module with inline modules per challenge type, to make things easier to navigate?
2 participants
Extracts the portions of #279 that are not specific to
dns-persist-01.The key motivation of the design I landed on is that while it's convenient to try and abstract away authorization challenge response across multiple challenge types, in reality there are a lot of challenge type specific details one has to contend with even when using challenges that all derive the key auth from the token (e.g. the format of the key authorization: should it be the raw string? a SHA256 digest? the base64 of that digest?).
Further, the new challenge types (
dns-persist-01,device-attest-01) don't have as much in common with the "legacy" challenge types and so we should be using the type system more to differentiate the challenge contexts. This also lets us tighten up what challenges are used for which identifier types (e.g. to avoid a situation where you try to usedevice-attest-01with a DNS identifier, ordns-01/dns-persist-01with an IP address identifier).