Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hot Reloading TLS Certificates #2683

Closed
wants to merge 7 commits into from

Conversation

martynp
Copy link

@martynp martynp commented Dec 28, 2023

Alternative method with TLS server test

#2363

@martynp
Copy link
Author

martynp commented Dec 31, 2023

Verifying certificates looks like it will be difficult.

We can assume that the pair are good when the pair is first loaded, but after that there is a potential for a sequencing error to occur as updated cert_chain/private_key pair are written. If we read before both have been changed it could be some seconds before the issue is resolved - assuming the OS reports a modified time to enough precision to detect the file change...

(Maybe move to using a hash of the certs rather than rely on OS reported file modified time - but that would mean reading the whole file every update interval).

Importantly a bad cert_chain/private_key does not generate any errors from Rocket, I am not sure rustls even knows what is happening. Clients will generate something along the lines for "DecryptError".

A slightly more complex but ever so slightly more resilient approach would be to wait a minimum time before applying new certificates - so detect the change then wait until the next loop update to actually apply. The user would then have that loop time to load in the new files correctly.

Users should check their keys are valid before using them - the approach is different depending on the key type.

@GentBinaku
Copy link

Is this done, I started working on it, but it's proving hard to test it properly. I am sorry I didn't do it in time, my skills aren't up to par like others.

@GentBinaku
Copy link

GentBinaku commented Jan 5, 2024

I tested on Chrome, even if I reload the page the certificate doesn't change and I can't open it. Same thing happens to me, I even used notify-rs to watch for file changes so the code is smaller. However still same behaviour as my code.

@GentBinaku
Copy link

GentBinaku commented Jan 5, 2024

My suggestion is either we go with a different Acceptor, or we change the add another configuration in which Rocket<Reload> which would restart the server and then do the certificate change, this way doing it in a seperate thread seems counterintiutive. Seems like on par with what FastAPI offers.

@martynp
Copy link
Author

martynp commented Jan 6, 2024

I tested on Chrome, even if I reload the page the certificate doesn't change and I can't open it. Same thing happens to me, I even used notify-rs to watch for file changes so the code is smaller. However still same behaviour as my code.

Interesting, it works using reqwest - but you do have to create a new client because a client will keep a TCP connection using the old certificate going for some time. I was hoping rustls would handle that side of things. I will have a play.

@martynp
Copy link
Author

martynp commented Jan 6, 2024

My suggestion is either we go with a different Acceptor, or we change the add another configuration in which Rocket which would restart the server and then do the certificate change, this way doing it in a seperate thread seems counterintiutive. Seems like on par with what FastAPI offers.

It's only a small task in the async runtime, pretty much no overhead. My intention was to add it as a feature flag option in any case.

Notify-rs uses a system thread to watch files - the only other way I can think is to trigger checks from some other event, but that will get more messy.

Restarting the server is an option I suppose, but not very hot-loading.

If you have connections open they should continue to work until the original certificates expire.

@martynp
Copy link
Author

martynp commented Jan 6, 2024

I tested on Chrome, even if I reload the page the certificate doesn't change and I can't open it. Same thing happens to me, I even used notify-rs to watch for file changes so the code is smaller. However still same behaviour as my code.

Interesting, it works using reqwest - but you do have to create a new client because a client will keep a TCP connection using the old certificate going for some time. I was hoping rustls would handle that side of things. I will have a play.

So I have tried using chrome and I cannot see any issue, chrome does keep the connection alive - so refreshing wont always get to the new cert. Opening a private window or different profile will start a new TLS session - you can see the TLS setup in the debug window.

@hcldan
Copy link

hcldan commented Mar 11, 2024

I would prefer a simpler approach, similar to the rocket shutdown signal.
That way the application author can decide where the certs are coming from easily, and outside the context of a rocket request.

let signaler = rocket.tls_config_change();

// somewhere else
signaler.notify(TlsConfig { ... });

@hcldan
Copy link

hcldan commented Mar 12, 2024

I thought about it some more, and it made sense to me to have rocket manage an impl of the resolver. #2748

SergioBenitez added a commit that referenced this pull request Apr 17, 2024
This commit introduces the ability to dynamically select a TLS
configuration based on the client's TLS hello.

Added `Authority::set_port()`.
Various `Config` structures for listeners removed.
`UdsListener` is now `UnixListener`.
`Bindable` removed in favor of new `Bind`.
`Connection` requires `AsyncRead + AsyncWrite` again
The `Debug` impl for `Endpoint` displays the underlying address in plaintext.
`Listener` must be `Sized`.
`tls` listener moved to `tls::TlsListener`
The preview `quic` listener no longer implements `Listener`.

All built-in listeners now implement `Bind<&Rocket>`.

Clarified docs for `mtls::Certificate` guard.

No reexporitng rustls from `tls`.
Added `TlsConfig::server_config()`.

Added some future helpers: `race()` and `race_io()`.

Fix an issue where the logger wouldn't respect a configuration during error
printing.

Added Rocket::launch_with(), launch_on(), bind_launch().

Added a default client.pem to the TLS example.

Revamped the testbench.

Added tests for TLS resolvers, MTLS, listener failure output.

TODO: clippy.
TODO: UDS testing.

Resolves #2730.
Resolves #2363.
Closes #2748.
Closes #2683.
Closes #2577.
SergioBenitez added a commit that referenced this pull request Apr 17, 2024
This commit introduces the ability to dynamically select a TLS
configuration based on the client's TLS hello.

Added `Authority::set_port()`.
Various `Config` structures for listeners removed.
`UdsListener` is now `UnixListener`.
`Bindable` removed in favor of new `Bind`.
`Connection` requires `AsyncRead + AsyncWrite` again
The `Debug` impl for `Endpoint` displays the underlying address in plaintext.
`Listener` must be `Sized`.
`tls` listener moved to `tls::TlsListener`
The preview `quic` listener no longer implements `Listener`.

All built-in listeners now implement `Bind<&Rocket>`.

Clarified docs for `mtls::Certificate` guard.

No reexporitng rustls from `tls`.
Added `TlsConfig::server_config()`.

Added some future helpers: `race()` and `race_io()`.

Fix an issue where the logger wouldn't respect a configuration during error
printing.

Added Rocket::launch_with(), launch_on(), bind_launch().

Added a default client.pem to the TLS example.

Revamped the testbench.

Added tests for TLS resolvers, MTLS, listener failure output.

TODO: clippy.
TODO: UDS testing.

Resolves #2730.
Resolves #2363.
Closes #2748.
Closes #2683.
Closes #2577.
SergioBenitez added a commit that referenced this pull request Apr 17, 2024
This commit introduces the ability to dynamically select a TLS
configuration based on the client's TLS hello.

Added `Authority::set_port()`.
Various `Config` structures for listeners removed.
`UdsListener` is now `UnixListener`.
`Bindable` removed in favor of new `Bind`.
`Connection` requires `AsyncRead + AsyncWrite` again
The `Debug` impl for `Endpoint` displays the underlying address in plaintext.
`Listener` must be `Sized`.
`tls` listener moved to `tls::TlsListener`
The preview `quic` listener no longer implements `Listener`.

All built-in listeners now implement `Bind<&Rocket>`.

Clarified docs for `mtls::Certificate` guard.

No reexporitng rustls from `tls`.
Added `TlsConfig::server_config()`.

Added some future helpers: `race()` and `race_io()`.

Fix an issue where the logger wouldn't respect a configuration during error
printing.

Added Rocket::launch_with(), launch_on(), bind_launch().

Added a default client.pem to the TLS example.

Revamped the testbench.

Added tests for TLS resolvers, MTLS, listener failure output.

TODO: clippy.
TODO: UDS testing.

Resolves #2730.
Resolves #2363.
Closes #2748.
Closes #2683.
Closes #2577.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants