Skip to content

need a way to pass rustls::ServerConfig for TLS configuration#676

Merged
davepacheco merged 2 commits intomainfrom
dap/raw-tls
May 25, 2023
Merged

need a way to pass rustls::ServerConfig for TLS configuration#676
davepacheco merged 2 commits intomainfrom
dap/raw-tls

Conversation

@davepacheco
Copy link
Copy Markdown
Collaborator

Note: This PR has a breaking change to the TLS API. I believe it'll be easy for consumers to move past. If you're using Dropshot with TLS and this looks painful, I'd like to hear from you -- soon.

Today, Dropshot supports TLS by providing a ConfigDropshotTls in the ConfigDropshot that you use to construct a Dropshot server. ConfigDropshotTls has variants for providing a TLS certificate via either files or raw bytes. This is a pretty static mechanism. If consumers want to change the certificate at runtime (e.g., to rotate it without downtime), they can use refresh_tls() on the server.

oxidecomputer/omicron#3164 wants to use TLS SNI to be able to select the certificate based on what server the client says they're connecting to. To do this, the certificate selection needs to be more dynamic than refresh_tls() allows. Fortunately, under the covers, when you start Dropshot with TLS, it always just converts the ConfigDropshotTls into a rustls::ServerConfig and uses that. The rustls interface lets you use SNI (or just change certificates at runtime) by providing your own certificate resolver. For a consumer like Omicron whose certificates are truly dynamic, it's much cleaner to just pass a rustls::ServerConfig directly into Dropshot and have it use that. Omicron will no longer need to bother with refresh_tls() after that.

This PR makes the following changes:

  • adds a variant ConfigDropshotTls::Dynamic that wraps a rustls::ServerConfig
  • removes ConfigDropshotTls from ConfigDropshot. This is a breaking change, but it's easy to move past (see below).
  • adds a new constructor called HttpServerStart::new_tls() that's the same as HttpServerStart::new() but also accepts an Option<ConfigDropshotTls>. If that argument is None, the server does not use TLS.

The motivation for the breaking change is that ConfigDropshot is intended to be dropped directly into a consumer's configuration file type (e.g., some type that's deserialized with serde from toml). But when using ConfigTls::Dynamic, there's nothing you can put in a config file to describe the TLS configuration. The conclusion: separate the static config from the TLS config. It's up to the consumer to decide how to get the TLS config and provide it separately to Dropshot (in the a new argument).

For consumers not using TLS, there's no breaking change.

For consumers using TLS:

  • If they were previously putting a ConfigDropshot in an actual configuration file, if no action is taken, the existing TLS-related properties will be ignored. If you want to accept TLS configuration the same as before this PR, you can include a ConfigDropshotTls property in the config file alongside the ConfigDropshot one. I believe (but have not tested) that you could do this without breaking your own consumers by defining a new type ConfigDropshotWithTls with a ConfigDropshot and a ConfigDropshotTls plus #[serde(flatten)] so that all the properties show up in the same place they used to.
  • Regardless of that, instead of calling HttpServerStarter::new(), they must call HttpServerStarter::new_tls() and provide Some(ConfigDropshotTls) for some ConfigDropshotTls value. The same values that were supported before are still supported.

Some other thoughts:

  • We could avoid a breaking change at the cost of making consumers like Omicron more confusing (because they'll have a dummy ConfigDropshotTls in their config file that they ignore) or creating extra compatibility stuff (e.g., a separate ConfigDropshotV2 that doesn't have the TLS stuff). At our stage, I generally prefer to make breaking changes in Dropshot rather than keep compatibility cruft.
  • We could probably do even better with a builder pattern (e.g., HttpServerStarter::builder() with a fn with_tls<T: Into<rustls::ServerConfig>>(t: T) and then define conversions from ConfigDropshotTls into rustls::ServerConfig.
  • I think it probably makes sense to deprecate refresh_tls() and say that if you want to change things at runtime, provide your own rustls::ServerConfig.
  • Urgency is important here. I'd like to punt on all of these things if folks don't object.

I'm aware of two Dropshot TLS consumers:

  • Omicron, which I'm doing all this for
  • @jclulow's consumer (buildomat?) -- I've sync'd with him on this already

@davepacheco davepacheco requested a review from luqmana May 22, 2023 02:35
@davepacheco davepacheco merged commit 62ee51d into main May 25, 2023
@davepacheco davepacheco deleted the dap/raw-tls branch May 25, 2023 21:42
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.

2 participants