simpler impl of dynamically swapping tls creds

[hcldan]

closes #2363

reference: #2683

[hcldan]

This was done against v0.5.
It needs to be ported to master. There were too many differences for me to do that today.

[GentBinaku]

How does it know how to listen to the USR1 signal when we want to reload the certificate?

[hcldan]

@GentBinaku This default implementation doesn't reload the certificate. However, it allows you to provide a certificate loading implementation that will be used instead of the default by having the rocket builder .manage(Arc::new(YourResolverImpl))

[hcldan]

@SergioBenitez It looks like in the master branch that the ability to specify a resolver is gone?

[SergioBenitez]

@hcldan Nothing's changed with respect to that. The QUIC implementation uses an older rustls, but Rocket itself continues to use the same rustls. What issue are you running into?

[hcldan]

@SergioBenitez When I looked at master the TLSConfig seems to have changed. And the listener has moved files.

I am not sure where the with_cert_resolver function was coming from before, it was off of ServerBuilder. I'm just trying to get my bearings in the new code and find breadcrumbs for where this stuff has to go now.

Are you in favor of this simpler implementation? If so, I will try again, but I don't want to devote much more time if you're going to want it done another way.

[SergioBenitez]

I don't think this will work quite as-is, largely because the resolver can't be async, but I think the idea is the right one in that we want to expose a thin layer on-top of rustls' existing ResolvesServerCert. Effectively, an async version of that trait. This would also resolve #2730. I'll try to tweaking this PR into one that allows async resolvers.

[SergioBenitez]

Check out the tls-resolver branch, and specifically f08bd39, where I've implementing the beginning of this.

Can you push this past the finish line? What's left is some way for the user to register a resolver and for Rocket to pick it up. And docs and examples, of course. And later, an implementation of resolver that monitors for changes to the configured certs and provides an updated config if one is available.

[SergioBenitez]

Copying from Matrix for posterity:

@hcldan Great idea exposing the resolver as a general mechanism.
Here are the docs for that branch: https://api.rocket.rs/tls-resolver
I think the ideal would probably be to add a method to Resolver, fairing(),
that returns a fairing that adds something to managed state.

Something like:

struct OpaqueResolverName(Box<dyn Resolver>);

/// A dynamic TLS configuration resolver.
#[crate::async_trait]
pub trait Resolver: Send + Sync {
    fn fairing(self) -> fairing::AdHoc {
        AdHoc::on_ignite("TLS Resolver", |rocket| async move {
            rocket.attach(OpaqueResolverName(Box::new(self)))
        });
    }

    async fn resolve(&self, hello: ClientHello<'_>) -> Option<Arc<ServerConfig>>;
}

Then someone can just do: rocket.attach(resolver.fairing()).
And we can look it up via rocket.state::().
We want some opaque name to avoid type name conflicts.

[hcldan]

@SergioBenitez
Back from vacation and I was thinking about your comment:

I don't think this will work quite as-is, largely because the resolver can't be async,

I just wanted to clarify your meaning here. In my example the resolver is not async, no.
It doesn't need to be. That is, the rusttls definition for it just says it must have a function that returns the tls creds, and that's all we really need.

Any other implementation (granted, I did not provide one that monitors the FS for changes) can poll, register for updates, whatever it wants and update some cached local current copy of those pieces of information.

The one I was using periodically checks a secure store for updates and when found, will replace the values in the custom struct that will be used for the trait implementation.

I thought it would be nice to keep the trait specific to the tls impl we are using and not try to wrap it in this project. And because of that, it was very unlikely that we would collide with another attempt at putting Arc<dyn ResolvesServerCert> in state.

I think I like the fairing idea, because it really does belong mostly in the config of the server and not some ad-hoc state that we slap on. But I think I would prefer to see it referred to by the trait definition.

I just took a look at your branch and I think while it might work, it is a bit overkill. The supplied resolver is handed over to the underlying tls impl and is used on every session negotiation. I'm not sure why the config would need to be "lazy" (unless there's some part of Rocket I'm not familiar with yet). When the server is launched, it should have all the info it needs via config. The resolver really shouldn't need to do anything special, be async, or be acquired in an async fashion (again, unless there's something that makes it necessary in Rocket).

[hcldan]

I see why it needs to be async. You've made the entire tls config dynamic.
Are you thinking the entire tls config will need to change dynamically? I've only really seen this happen with cert rollovers... I was really hoping for something simple so I could just provide the server certs for new connections.

rusttls gave us a trait for doing that, why not just use that?

[SergioBenitez]

I just wanted to clarify your meaning here. In my example the resolver is not async, no.

It doesn't need to be. That is, the rusttls definition for it just says it must have a function that returns the tls creds, and that's all we really need.

I understand that the TLS library doesn't require it to be async, but the concern is that you might need to perform some kind of async operation, even a minimal one like taking a lock or reading from a channel, and be unable to do so. It's possible that this wouldn't be required for any implementation, but there's no evidence to suggest this one way or another as it stands. In this sense, making it async is the "safe" choice. Perhaps we need to implement a few "obvious" use cases to get some experience here.

The one I was using periodically checks a secure store for updates and when found, will replace the values in the custom struct that will be used for the trait implementation.

How does it perform the replacement atomically?

I thought it would be nice to keep the trait specific to the tls impl we are using and not try to wrap it in this project. And because of that, it was very unlikely that we would collide with another attempt at putting Arc<dyn ResolvesServerCert> in state.

I think I like the fairing idea, because it really does belong mostly in the config of the server and not some ad-hoc state that we slap on. But I think I would prefer to see it referred to by the trait definition.

The amount of confusion that can be caused by conflicting managed state types is considerable, while what we need to ensure that such a thing doesn't happen is roughly 0 lines of code: one private type in managed state versus another. There's no disadvantage to doing this, as far as I can tell.

I just took a look at your branch and I think while it might work, it is a bit overkill. The supplied resolver is handed over to the underlying tls impl and is used on every session negotiation. I'm not sure why the config would need to be "lazy" (unless there's some part of Rocket I'm not familiar with yet). When the server is launched, it should have all the info it needs via config.

Configuration needs to be "lazy," in the sense that rustls calls it lazy, to be able to dynamically select configuration based on the client. This is critical for being able to do something like SNI based certificate negotiation. This happens during the TLS handshake.