-
Notifications
You must be signed in to change notification settings - Fork 593
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
Feature request: a way to set/get default ticketer dynamically #1876
Comments
I'm not sure that we want to link the There's not really a "default" ticketer from my perspective, just two convenient implementations of So, what about something inspired by impl ServerConfig {
...
#[cfg(feature = "std")]
pub fn ticketer_from_crate_features() -> Option<Result<Arc<dyn ProducesTickets>, Error>> {
#[cfg(all(feature = "ring", not(feature = "aws_lc_rs")))]
{
return Some(rustls::crypto::ring::Ticketer::new());
}
#[cfg(all(feature = "aws_lc_rs", not(feature = "ring")))]
{
return Some(rustls::crypto::aws_lc_rs::Ticketer::new());
}
#[allow(unreachable_code)]
None
}
} For other cases, where the features aren't unambigious, or an external-to-rustls |
That would be nice what about something like: impl ServerConfig {
...
#[cfg(feature = "std")]
pub fn ticketer_from_crate_features_or(use_ring_if_both_enabled: bool) -> Result<Arc<dyn ProducesTickets>, Error> {
#[cfg(all(feature = "ring", not(feature = "aws_lc_rs")))]
{
return rustls::crypto::ring::Ticketer::new();
}
#[cfg(all(feature = "aws_lc_rs", not(feature = "ring")))]
{
return rustls::crypto::aws_lc_rs::Ticketer::new();
}
#[cfg(all(feature = "aws_lc_rs", feature = "ring"))]
if use_ring_if_both_enabled {
return rustls::crypto::ring::Ticketer::new();
} else {
return rustls::crypto::aws_lc_rs::Ticketer::new();
}
}
} Similarly, if the same could be applied to CryptoProvider::from_crate_features, as I feel its current API can be too footguny. I had too many panics due to enabling the two features together. With this additional option, the library author at least can decide what to do when both are there. |
Right, if the two options are enabled together, and no explicit default has been set, and someone uses a builder fn that uses the default it will panic rather than try to guess which of the two to use. I don't think we should try to do something different here by offering a parameter. If the user has a working configuration with both features enabled irrespective of ticketer config it's because something is constructing the ring or aws provider, installing it as default, and then using the builders. In this case whatever is doing so has the necessary knowledge to also construct a choice of the aws or ring ticketer and install it to the server config field, no? IMO it's only when the default is implied by the use of unambiguous crate features that there's not enough knowledge to know which of the ticketers to construct and that's what the helper I proposed would offer. |
I think changes to how default crypto providers are constructed is a separate topic that should be its own issue/discussion (but I would also caution that the current design is the result of a fair amount of iteration balancing many tradeoffs and I don't think we're especially keen to revisit core aspects without strong motivation for why something else is a better choice). |
Sorry for going off-topic. I know this design is probably well discussed but I wanted to add one data point for reference. I won't open new issue about this as it is probably will discussed. Yes but one problem are features are additive, if some crate enable ring by default (which is what may be done for Rocket, but I think due to this maybe we should not do so) and other crate enable aws-lc-rs by default (rustls, etc..) and I use them both. Then, suddenly my codebase which was working before will end with too many panics and tests failing. This mistake is not obvious. It occurred to me a lot of times in a short period (while doing the PR for Rocket). Beginners to Rust may even be more frustrated at this issue, but I am not sure how common it would be. As an application developer, if I don't want to accidentally use them both at the same time, I could do this or anything of sort:
or setup the default crypto provider. But if the user enabled both. I feel that using any one of them is actually valid choice. And it doesn't have to be ambiguous, simply documenting a priority for the features could be enough. Or documenting that it is ambiguous and decision may change and users who want determinism should setup the default crypto provider manually. Many users won't care about these details but would care that their code is brittle and would panic due to adding a feature or a crate. |
(Going to ignore the #1877 discussion in my comments here, thanks @Alvenix for splitting that off.) So I guess it would be cool if we could reformulate the In the absence of that, maybe we should come to consensus in #1877 before we resolve the details of the interface needed to expose a ticketer? |
Checklist
Is your feature request related to a problem? Please describe.
Currently I am trying to make Rocket use the default CryptoProvider if it is set. This is the only blocking issue.
Describe the solution you'd like
Either ticketer is added as a field on CryptoProvider or add a way to set/get default ticketer separately.
Describe alternatives you've considered
I considered simply using ring Ticketer but this may be unexpected to the users who change CryptoProvider.
The text was updated successfully, but these errors were encountered: