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

[tls] Allow refreshing TLS configuration information at runtime #502

Merged
merged 4 commits into from
Dec 14, 2022

Conversation

smklein
Copy link
Contributor

@smklein smklein commented Dec 6, 2022

Adds a refresh_tls method to HttpServer, which allows TLS information to be updated for a running server.

Fixes #491

@smklein smklein requested review from ahl and jclulow December 6, 2022 20:07
@smklein smklein marked this pull request as ready for review December 6, 2022 20:07
@jclulow
Copy link
Collaborator

jclulow commented Dec 6, 2022

How does the mutex on the acceptor work if there are no inbound connections being made?

@smklein
Copy link
Contributor Author

smklein commented Dec 6, 2022

How does the mutex on the acceptor work if there are no inbound connections being made?

The acceptor exists independently of inbound connections. My addition of an Arc<Mutex<...>> is basically just a way of indicating: "within the HttpsAcceptor, which accept-s new connections and negotiates TLS, allow the acceptor to get replaced by a different value". If no inbound connections are being made, we simply aren't accepting anything -- this is the same with or without the mutex.

Note that the TlsAcceptor is constructed using the certificate chain + private key pair.

So, basically, refresh_tls keeps this HttpsAcceptor running, but just uses an acceptor from a new "cert chain / private key" pair. This doesn't impact any clients that have previously connected, but it does impact all new connections - which is behavior I tested.

Does that answer your question?

@jclulow
Copy link
Collaborator

jclulow commented Dec 6, 2022

Yes that makes sense thanks. I had missed that we are not blocking at that point waiting on a connection, because of the select loop it's inside that actually deals directly with the system-level sockets.

Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ship it

CHANGELOG.adoc Outdated
@@ -15,6 +15,7 @@

https://github.com/oxidecomputer/dropshot/compare/v0.8.0\...HEAD[Full list of commits]

* https://github.com/oxidecomputer/dropshot/pull/502[#502] Dropshot exposes a `refresh_tls` method to update the TLS certificates being used by a running server.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be mistaken, but I think this is the first breaking change in this list. If that's the case, can you please create two sections as below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, updated in 219ff7a

@smklein
Copy link
Contributor Author

smklein commented Dec 14, 2022

FYI @davepacheco , since this is a breaking change

@smklein smklein merged commit 94967b8 into main Dec 14, 2022
@smklein smklein deleted the refresh_tls branch December 14, 2022 20:23
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.

TLS certificate and key should be able to be replaced at runtime
3 participants