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

Make rustls provider option configurable #516

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

LecrisUT
Copy link

This should allow downstream projects to configure the rustls provider that they need to be consistent across all their dependencies. This should address an issue in atuin where the rustls tls backend needs to be moved back to ring due to other workflows breaking. @tessus does this look good for that?

Closes #515

@tobz tobz added C-exporter Component: exporters such as Prometheus, TCP, etc. E-simple Effort: simple. T-ergonomics Type: ergonomics. labels Sep 21, 2024
@tessus
Copy link
Contributor

tessus commented Sep 21, 2024

I think that should do it.

@tobz
Copy link
Member

tobz commented Sep 21, 2024

It looks like this needs to be a little more fleshed out, unfortunately.

With default features turned off, and the push-gateway feature enabled, the package fails to build because we try to use hyper_rustls::HttpsConnectorBuilder::with_native_roots, which needs the rustls-native-certs feature and either the ring or aws-lc-rs feature. Since we have no provider feature enabled, it fails to compile. Since exporters are meant to be featureful by default, I want push-gateway to do the right thing, even if default features have been disabled.

Thinking out loud, I think we need to do three things:

  • change the push-gateway feature to actually enable hyper-rustls/aws-lc-rs
  • add a new feature -- push-gateway-no-tls-provider? -- that does what push-gateway does now so you can opt out of push-gateway and the default choice of AWS-LC
  • update the actual code in the exporter to use rustls::crypto::CryptoProvider::get_default and then switch to calling HttpsConnectorBuilder::with_provider_and_native_roots using the provider we get

Doing number 3 would mean we also need to essentially copy the error message that rustls emits when there's no provider feature flags and no default provider set, since that would still be a possibility for folks using push-gateway-no-tls-provider.

How does that sound?

@LecrisUT
Copy link
Author

push-gateway-no-tls-provider should work. Just fearful if this would eventually propagate to other variations, but probably it's not relevant for a while.

Let me get a second pair of eyes from Fedora rust packaging to check if we don't need to do other tweaks.

@tessus
Copy link
Contributor

tessus commented Sep 22, 2024

Thinking out loud, I think we need to do three things:

You mean all of them or one of them? The first one is probably the simplest, as long as when one wanted to use ring they wouldn't have to list too many deps after having to specify --no-default-features.

@tobz
Copy link
Member

tobz commented Sep 22, 2024

You mean all of them or one of them? The first one is probably the simplest, as long as when one wanted to use ring they wouldn't have to list too many deps after having to specify --no-default-features.

All of them.

Essentially, if we only had a feature flag called push-gateway-no-tls-provider, and a user wanted TLS support, then they would need to manually import rustls in their application, pick a provider, and install it as a default. That's unnecessary friction that I'm not willing to force on them.

This means we need at least two feature flags -- one that includes proper defaults, and one that includes no default -- which are represented by the first and second work items in the list. The third work item is the necessary code changes to support both of those variants, since the current code depends on hyper-rustls having a specific provider enabled, which doesn't work for the "no TLS provider" scenario.

@tessus
Copy link
Contributor

tessus commented Sep 22, 2024

Thanks for the explanation.

While I think that at one point in the future (and I want to stress that this point might not be as soon as we'd hope) the defaults will be ok across the board, I also think that there's considerable work to be done to support aws-lc-rs in the current ecosystem. e.g. as soon as aws-lc-rs is used, pipelines on illumos fail. Not that I will ever understand why someone would want to run a pipeline on illumos. But this shows that the transition from ring was not as seemless as people hoped.

I can only suggest to make it work with ring as well. At least for the time being. As soon as the transitional phase is over, ring might be deprecated anyway. But right now it is certainly part of many projects and thus I think it makes sense to support it in metrics as well.

@tobz
Copy link
Member

tobz commented Sep 22, 2024

Yeah, I mean, I think what I'd want to do, if anything, is something like:

  • push-gateway: enables Push Gateway support using crypto provider A in rustls/hyper-rustls
  • push-gateway-tls-provider-<name>: enables Push Gateway support using crypto provider <name> in rustls/hyper-rustls
  • push-gateway-no-tls-provider: enables Push Gateway support without any default cryptography provider configured for rustls/hyper-rustls

In this scenario, crypto provider A would be whatever we decide the default should be, and <name> would really just be the other one, since rustls only has Ring and AWS-LC anyways. :) MY personal feeling is that I'd probably want AWS-LC to be the default in this scenario.

(As a side note: I tend to want to standardize on AWS-LC to help drive adoption, and fixing of bugs, so that by being forced to deal with these sharp edges when they occur, people -- myself included -- are incentivized to fix them. Over time, being responsible for supporting contributions that I personally don't use or depend on, it ends up being a large amount of time that has to be spent dealing with this stuff... so I'd rather have the experience be a little worse for some users but much much better for the majority, by focusing my time on pushing the majority of users towards the things I know are well-supported.

This is all to say that I currently use AWS-LC at work, and for a bunch of personal projects, so I know if there's a build issue on Linux, Windows, or macOS, I will likely notice it very quickly and be able to help diagnose or contribute to fixing the problem.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-exporter Component: exporters such as Prometheus, TCP, etc. E-simple Effort: simple. T-ergonomics Type: ergonomics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forwarding the rustls provider settings
3 participants