FlorianUekermann / rustls-acme

Apache License 2.0
136 stars 27 forks source link

solve tide-acme update block #46

Closed FlorianUekermann closed 10 months ago

FlorianUekermann commented 1 year ago

tide-acme can't update to the current version of rustls-acme, because it requires async-rustls streams, instead of the futures-rustls streams we use nowadays.

Upstreaming the meat of tide-acme could be desirable as well, to prevent similar problems going forward.

From #28:

Yes, that's unfortunate. I'm a little fuzzy on the details atm, because I switched such a long time ago. I see three ways forward:

  1. Figure out if we can convert the types. I suspect that's not that hard. I think async-rustls is just missing a From impl we can use.
  2. I could switch back. This is only an option if async-rustls has caught up in term of features (not entirely sure anymore what exactly was missing). futures-rustls is a lot more popular though.
  3. Given how much more popular futures-rustls has become and how redundant async-rustls and futures-rustls are (is that correct?), maybe it is time to explore consolidating by switching over on the tide-rustls side.
joshtriplett commented 1 year ago

I think the most likely fix would be to make tide-acme independent of tide-rustls, and make it a standalone TLS implementation for tide built on rustls-acme.

FlorianUekermann commented 1 year ago

Solution 1 seems both easier and benefitial for others who face this problem. I think the TlsStream structs of the two crates are identical and futures-rustls gives us access to all fields already. All that's missing is a TlsStream<IO>::from(io: IO, session: ServerConnection, state: TlsState) on the async-rustls side. I'm on vacation for a few days atm, but I can try a PR for async-rustsls next weekend.

FlorianUekermann commented 1 year ago

Issue #47 gave me an idea.

Since tide-acme is using the low-level API, maybe there is a way we can remove the AcmeAcceptor from rustls-acme and let the user deal with that stuff. I think that would make the low-level API independent of futures-rustls.

All the Acceptor does is:

  1. Poll a futures_rustls::LazyConfigAcceptor until it resolves to a futures_rustls::StartHandshake.
  2. Check if the futures_rustls::StartHandshake is a validation attempt and: a. (is validation attempt) complete the handshake with a special rustls::ServerConfig and drop the connection b. (normal connection) hand the futures_rustls::StartHandshake back to the user

I think this is easy enough for users to do on their own with any library they like (including async-rustls). Everything you need should already be there if you get the rustls_acme::ResolvesServerCertAcme from rustls::AcmeState::resolver(). I'm tempted to try to make this a bit more accessible in any way I can (couple of helper methods on state maybe) and then remove the AcmeAcceptor concept altogether.

@joshtriplett : It would be great if you could give this approach a go on your side. That would allow us to judge what kind of helpers make your life easy. src/acceptor.rs contains all the info you need. Ideally examples/low_level.rs becomes shorter with the right helpers added to rustls-acme.

joshtriplett commented 1 year ago

On Tue, Jun 06, 2023 at 07:31:12AM -0700, Florian Uekermann wrote:

Issue #47 gave me an idea.

Since tide-acme is using the low-level API, maybe there is a way we can remove the AcmeAcceptor from rustls-acme and put that on the user.

Honestly, I'm not attached to the Acceptor API, I just used it because it seemed like the only way to fit within tide-rustls.

I think it'd be possible to use the high-level API. That'll take some experimenting.

FlorianUekermann commented 1 year ago

The high-level API is quite convenient if you want to ditch tide-rustls and would likely allow tide-acme to drop the runtime dependency (I'm working on that for rustls-acme as well).

I'm also really warming up to having the low-level API entirely independent of what futures-rustls. I'll provide an example later today.

I'm also considering adding a tide_rustls::CustomTlsAcceptor implementation behind a feature flag. I already do this for axum.

FlorianUekermann commented 1 year ago

Here is an example that only uses async-rustls: https://github.com/FlorianUekermann/rustls-acme/blob/main/examples/low_level_async_rustls.rs

It's a bit too verbose for my taste, so any ideas for helpers that make this more ergonomic are very welcome.

FlorianUekermann commented 10 months ago

You should be able to update tide-acme with the approach used in the example above, so I'm closing this. futures-rustls will just become an unused dependency. When I deprecate AcmeAcceptor, I'll make the futures-rustls dependency optional (gated by whether the high-level API is available or not).

Let me know if you would like me to send a PR or something.