🎚 Use rustls for https directly, for more control #100
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The CLI performs backend health checks on startup. Previously it was setting up its own TLS connector, but this commit changes it to use the same connector we use in viceroy-lib (now called `BackendConnector`) to ensure consistency and centralize any connector changes.
Rather than using hyper_rustls as before, this approach gives us more direct control over how TLS is connected and configured, including things like SNI and ALPN.
cratelyn
previously approved these changes
Dec 1, 2021
lib/src/session.rs
Outdated
| /// The TLS configuration for this execution. | ||
| /// | ||
| /// Populated prior to guest execution, and never modified. | ||
| pub(crate) tls_config: Arc<rustls::ClientConfig>, |
There was a problem hiding this comment.
Suggested change
| pub(crate) tls_config: Arc<rustls::ClientConfig>, | |
| tls_config: Arc<rustls::ClientConfig>, |
because we mention that this is never modified in the comment, perhaps we can remove the pub(crate) visibility here, to help enforce that?
that leaves the tls_config accessor below as the sole interface through which other modules can acquire a reference to this 🙂
There was a problem hiding this comment.
(this is one of those "take it or leave it" comments)
There was a problem hiding this comment.
oh good catch, i was following the pattern of the other read-only items. they should all be using accessors rather than pub(crate), so I'll add a commit to do that 👍
|
@cratelyn pushed up a commit tidying up the privacy for read-only session fields. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previous work (#75) moved Viceroy to using rustls for TLS, with some good benefits. However, the hyper_rustls library bakes in a fair amount of TLS configuration that we need to control more directly.
Since hyper_rustls is a very thin library, and since we already need our own Hyper connector for other reasons, I opted to just use rustls directly within Viceroy, for maximum clarity/control. The main downside is that there's a bit of boilerplate around TCP/TLS streams.
Along the way, this PR also refactors the CLI backend health checks to use the Viceroy library, rather than connecting in its own way, which will help ensure we keep those in sync.
I've confirmed that this PR:
Fixes #91
Fixes #96