move `order`, `authorize` and `duration_until_renewal_attempt` to `acme`

[User65k]

Hi,

I wanted to use this library for a webserver. My server needs a custom implementation of rustls ResolvesServerCert and thus left me with pretty much copying order, authorize and duration_until_renewal_attempt from resolver.rs. I figure that might happen to others a well and would like to propose to move the functions out of the resolver and make them pub. I also added the callback set_auth_key to order as the way to store the challenge might be different as well.

What do you think?

[FlorianUekermann]

Could you provide some context why your server needs a custom ResolvesServerCert?

[User65k]

My Server adds support for virtual hosts(via SNI) and support to use multiple key types at the same time (rsa, ec, ed)

I suppose Rustls made resolvesservercert a trait and keept the default one simple with the intention that others might build complex use cases while some can keep it simple

[FlorianUekermann]

I see, picking a different way to resolve than what this crate offers based on SNI makes sense in general to me. So we should definitely make sure that is well supported. It does look like there is a little more going on here though. Looking at the functions you are making public, you don't want to call run either. That also makes sense to me, as run() makes the lifetime of the resolver infinite, which may not suit your situation.

Regarding the first problem. I guess you can just wrap ResolvesServerCertUsingAcme (and probably already do that). Maybe we should return None if the SNI doesn't match the cert in the resolve method. Or you check if the SNI matches the returned cert yourself.

Regarding the second problem. Maybe run() should downgrade the reference to the resolver and return when the resolver is dropped, so we don't leak resources. The assumption that run() should run forever always felt dirty, but it is easy to fix.

I feel like I'm missing something.

Btw, if you are just looking for an implementation of the acme API. Maybe the contents of the rustls-acme::acme module are more suitable for your usecase.

[User65k]

Yeah, I use a Weak to avoid running forever. Btw I also use a different async runtime, so choice there might also be nice.

I am not really interested in a low level API. I want one function that gives me a new Cert. 😁 But yeah, I will use a order function as in this PR, so in the end the acme calls

[FlorianUekermann]

I see. I don't mind adding a public order method on the resolver, which returns a certificate. That is a useful subset of the current private order method (which would have to be renamed).

    pub async fn new_public_order(
        &self,
        directory_url: impl AsRef<str>,
        domains: &Vec<String>,
    ) -> Result<CertifiedKey, OrderError> {

We can use it in the existing private order function by breaking the loop after retrieving the cert and writing to cache after the loop. With another method to get the cert out, you would be free from dealing with this crate after you got the cert out. In practice getting a new cert would boil down to:

let resolver = Arc::new(ResolvesServerCertUsingAcme::new());
// Register resolver so your own resolver can use it (maybe as Weak ref).
let cert = resolver.order(...)?;
drop(resolver);
// Deregister resolver or trigger collection of Weak refs to dropped resolvers.

How does that sound?

I don't quite see the benefit of removing the resolver entirely in favor of a set_auth_key function argument. If you want to remove the resolver in this crate from the whole game, I would rather split it up into two functions. The first one would set up the authorization and return the BTreeMap<String, CertifiedKey>, so you can do with it whatever you want to make it available to your own resolver. And then another that does the rest and returns the cert.

Those could be implemented as convenience methods for Order and be used inside the resolvers order method.

In any case: Pull request welcome. I'm just not a a fan of the callback approach that is in there atm.

[FlorianUekermann]

Closing in favor of #13