search

FlorianUekermann / rustls-acme

Apache License 2.0
136 stars 27 forks source link

No way to configure ALPN settings #37

Closed hdevalence closed 1 year ago

hdevalence commented 1 year ago

Hi, while trying to use rustls-acme with tonic, we ran into problems that I think are related to not having the ability to configure rustls ALPN settings. (Full context is here: https://github.com/penumbra-zone/penumbra/pull/1666#issuecomment-1504875450)

Currently, the rustls-acme API doesn't allow setting rustls ServerConfig at all.

Would it be preferable to expose the entire ServerConfig as part of the AcmeConfig builder API, or just the ALPN settings?

FlorianUekermann commented 1 year ago

Thanks for the opening the issue. Note that AcmeConfig is used for both the low level and high level API in rustls-acme. That's why it only offers configuration of acme related things. With the low level API you should be able to do what you want already (have a look at the low_level*.rs examples).

But I do think it would be nice to offer at least basic alpn control via the simple API (on Incoming).

  1. The most we can do here is allow the user to provide a rustls_config and simply overwrite the resolver. That would also cover other rustls config changes the user wants to make.
  2. Or we could let the user specify the alpn parameters at construction (AcmeState::incoming_with_alpn(self, cb: Fn(rustls::ClientHello) -> ...))

Maybe both. I'll see what I can come up with today. If you try the low level api and run into problems or have opinions/suggestions regarding the above, let me know.

FlorianUekermann commented 1 year ago

I added an alpn_protocols argument to all functions that construct Incoming, which makes this available in the high-level api. rustls-acme = { git = "https://github.com/FlorianUekermann/rustls-acme" }

In the long run I'm considering merging the low- and high-level APIs, by making the stream emit something similar to rustls::server::Accepted, which the user can turn into a stream using their own config. That would side-step the whole issue. But it is good to get this out the door first, so there's something to compare the new API to during development.

Please let me know if this works for you and if you have any usability concerns, so I can tag a release asap.

hdevalence commented 1 year ago

Amazing, thanks so much! We tested with a fork where we just hardcoded in ALPN settings and got things working, we'll try the patch now and let you know if it works.

hdevalence commented 1 year ago

Please let me know if this works for you and if you have any usability concerns, so I can tag a release asap.

We've tested and this works for us! If you'd be interested in #38, we could also submit a PR for that so you'd only have to do one release, but we'd also be happy to do that later if you'd prefer more deliberation. Whatever works.

FlorianUekermann commented 1 year ago

published as 0.6.0