Http-01 Challenge Framework support

[jklamer]

Approach to the implementation of support challenge type https://datatracker.ietf.org/doc/html/rfc8555#section-8.3.

Approach:

• New option configuration value that allows the developer to decide which challenge type they wish to use. Defaults to TlsAlpn01
• Cache of fixed reasonable size and duration that can map the token that will come from the challenge to the key_authorization that is needed to be returned from the resource /.well-known/acme-challenge/{token} in format
  ASCII representation of the key authorization. The cache is intended to guard against a growing number of tokens accumulating over many recertification cycles
• Populate a map from the token to the key auth. only expose through key get enforce developer gets token from request. Clean up the map when authorization is successful or fails.
• populate a challenge data field in the resolver that is cleared up on valid or invalid auth.

Alternate approaches considered:

• Store key auth map outside of resolver. Unclear if semantically it belongs in same inner/mutex
• Don't use cache dependency, roll own, clean up inside main state future loop to guard against infinite accumulation

Open questions:

• Best way to test. Unclear of good boundaries. Can volunteer personal site and test via local build.
• Do we ever need to store more than one key auth at a time. Is it even possible to have multiple challenges and tokens running at once?

[jklamer]

While attempting to implement this part of the spec, I realized I don't believe I need the cache and can expect the tokens to be cleaned up in authorize:

The client SHOULD de-provision the resource provisioned for this
   challenge once the challenge is complete, i.e., once the "status"
   field of the challenge has the value "valid" or "invalid".

[FlorianUekermann]

As you suspect. Per RFC we only need to do one challenge (and that's what we do atm). So I think we can drop the b-trees in favor of something like:

enum ChallengeData {
  TlsAlpn01 {
    sni: String,
    cert: Arc<CertifiedKey>,
  },
  Http01 { ... }
}

struct Inner {
  cert: Option<Arc<CertifiedKey>>,
  challenge_data: Option<ChallengeData>
}

[jklamer]

Absolutely can do this style.

Is it possible that there are many autherizes going on at once (one for each domain). Will the try_join_all below execute the authorize code out of order? Which might mean each one overrides and deletes the others?

let auth_futures = order.authorizations.iter().map(|url| Self::authorize(&config, &resolver, &account, url));
try_join_all(auth_futures).await?;

I think the code I have now to clear the challenge data would be incorrect in this case.
Looking at the code I think for a number of authorizations less than 30 it will be unordered. I think if we force in order execution to allow the single challenge data implementation to work. I'll implement this and see what you think

[FlorianUekermann]

I see, I mistook lack of order concurrency for a lack of authorization concurrency. I think your solution is good though, this isn't very time critical and this simplifies cleanup (not that keeping these around would become an issue in practice, but doing this properly is nice).

[jklamer]

Yeah it's interesting. For the Tls apn, the data is bounded by number of domains so doesn't really need to be cleaned up in practice just overwritten. For HTTP-01 the token can become different every authorize, so after enough of them or bunch of failed attempts and a lot of time the accumulation would likely become noticeable

[FlorianUekermann]

• Store key auth map outside of resolver. Unclear if semantically it belongs in same inner/mutex

For tls-alpn-01 we need it in a resolver. But we are always working with an Arc<ResolvesServerCertAcme> anyway (returned by State::resolver()). We should add a method to that, which returns the data you need in the http handler. If we want we can wrap it later. Technically we could split the resolver into two (one for challenges and one for the final cert), but both would need a mutex for updating and challenges are very rare in practice, so there's no benefit.

Looking pretty promising overall. I can test locally and do that for every release. So no need to worry about it. All I need is a working example in the examples folder.

[jklamer]

Validating the token is the expected challenge token

[jklamer]

Put what I believe to be a working example in the examples folder. Haven't properly tested it yet.

[FlorianUekermann]

Was a bit busy. I'll take a closer look asap.

[jklamer]

Was able to test this using my website and fixed a pretty silly bug/simplified it. My website is now running using this! Marking as ready for review. Let me know how you would like me to organize the commits.

[FlorianUekermann]

Got around to testing it today. Works with a minor change. Don't worry about the commits, we'll squash this in the end.

I need to think about order concurrency a bit, it's been too long since I looked at the details. I'll do that today.

[FlorianUekermann]

Nice work. I added some notes on details and a suggestion for ergonomic axum integration via tower.

[FlorianUekermann]

I had to change this (removing the trailing / was necessary)

Suggested change

	.route("/.well-known/acme-challenge/{challenge_token}/", get(http01_challenge))
	.route("/.well-known/acme-challenge/:challenge_token", get(http01_challenge))

[FlorianUekermann]

nvm, that's because axum is outdated, let's update it in this PR

[FlorianUekermann]

let's take a &str as an argument here and maybe call it get_http_01_key_auth

[FlorianUekermann]

Now that we've validated everything, let's add nice helper.

I tried two options:

#[derive(Clone)]
struct AxumHttp01ChallengeHandler(Arc<ResolvesServerCertAcme>);

impl<S> axum::handler::Handler<(), S> for AxumHttp01ChallengeHandler {
    type Future = std::future::Ready<axum::response::Response>;

    fn call(self, req: Request, _: S) -> Self::Future {
        let Some((_, token)) = req.uri().path().rsplit_once('/') else {
            return std::future::ready((StatusCode::NOT_FOUND,).into_response())
        };
        let Some(body) = self.0.get_key_auth(&token) else {
            return std::future::ready((StatusCode::NOT_FOUND,).into_response())
        };
        std::future::ready((StatusCode::OK,[(header::CONTENT_TYPE, HeaderValue::from_static("application/octet-stream"))], body).into_response())
    }
}

#[derive(Clone)]
struct TowerHttp01ChallengeService(Arc<ResolvesServerCertAcme>);

impl<B> tower_service::Service<http::Request<B>> for TowerHttp01ChallengeService {
    type Response = http::Response<String>;
    type Error = std::convert::Infallible;
    type Future = std::future::Ready<Result<Self::Response, Self::Error>>;

    fn poll_ready(&mut self, _cx: &mut std::task::Context<'_>) -> std::task::Poll<Result<(), Self::Error>> {
        std::task::Poll::Ready(Ok(()))
    }

    fn call(&mut self, req: Request<B>) -> Self::Future {
        let mut response = http::Response::new(String::new());;
        *response.status_mut() = StatusCode::NOT_FOUND;
        let Some((_, token)) = req.uri().path().rsplit_once('/') else {
            return std::future::ready(Ok(response))
        };
        let Some(body) = self.0.get_key_auth(&token) else {
            return std::future::ready(Ok(response))
        };
        *response.status_mut() = StatusCode::OK;
        *response.body_mut() = body;
        response.headers_mut().append(header::CONTENT_TYPE, HeaderValue::from_static("application/octet-stream"));
        std::future::ready(Ok(response))
    }
}

I don't see a benefit in the axum handler if we have a tower service, so I would suggest only adding the tower service (which is more general).

Let's add a method to State that returns the tower service.

let tower_service:  TowerHttp01ChallengeService = state.tower_service();

This way we don't need the axum state and the route call is:

.route_service("/.well-known/acme-challenge/{token}", tower_service)

With the axum handler the code looks pretty much the same, but as mentioned above, I don't think we need that.

We need to introduce a tower feature which the axum feature depends on.

[FlorianUekermann]

leaving this general comment on the first file to enable threading

Let's Encrypt follows https redirects and doesn't check the cert on those. So it may be a good idea to change our behavior a bit and add a self-signed cert by default instead of starting with no cert to cover usecases where all http requests are redirected to https.
I could add this as a separate PR, but if you want to add that change in this one I would appreciate it.

[FlorianUekermann]

Don't worry about pushing broken code if testing is tricky on your side. I can test and debug on my dev machine.