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

yup-oauth2 10 #89

Merged
merged 1 commit into from
Jun 16, 2024
Merged

yup-oauth2 10 #89

merged 1 commit into from
Jun 16, 2024

Conversation

serprex
Copy link
Contributor

@serprex serprex commented Jun 11, 2024

hyper v1 removed HttpConnector, now exists in hyper-utils

hyper v1 removed HttpConnector, now exists in hyper-utils
Copy link
Owner

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

LGTM

@lquerel
Copy link
Owner

lquerel commented Jun 13, 2024

I will merge and publish this weekend. Thanks for contributing.

@lquerel lquerel merged commit 7a20ff0 into lquerel:main Jun 16, 2024
1 of 3 checks passed
@lquerel
Copy link
Owner

lquerel commented Jun 17, 2024

I had to define a default crypto provider. There might be a better way to set the default provider for cryptography, but for now, without this call, the following message is produced by rustls:

no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point

If you have a better approach, please let me know. I plan to release a new version of the crate this Monday or Tuesday. Thanks!

See cf3cd5c

@serprex
Copy link
Contributor Author

serprex commented Jun 17, 2024

@lquerel https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html#using-the-per-process-default-cryptoprovider libraries shouldn't set the default provider. yup-oauth2 is using ring crypto provider, but better is for libraries to have ring/aws-lc-rs as features like in pgwire: sunng87/pgwire#179

I'll look to adjust yup-oauth2 to have ring/aws-lc-rs features, & then gcp-bigquery-client can apply that same feature to its yup-oauth2 dependency. But that'll be yup-oauth 11, so for yup-oauth 10 we can explicitly use ring provider

PR on yup-oauth2 to not set default provider either: dermesser/yup-oauth2#234
I think they added that default logic after releasing 10.0.1, so probably their next release will just work with this crate not concerning itself with default providers

@serprex
Copy link
Contributor Author

serprex commented Jun 17, 2024

found dermesser/yup-oauth2#232 which does what I'd like with ring/aws-lc-rs features

@lquerel
Copy link
Owner

lquerel commented Jun 17, 2024

@serprex I find that asking applications to call CryptoProvider::install_default() because one of their dependencies uses rustls is not ideal. It imposes a form of API leak caused by rustls. Is there any documentation somewhere that explains precisely how to set up this new version of yup-oauth2 using features in such a way that:

  1. It provides the ability to define the preference of the crypto provider via features.
  2. In the absence of a preference, a default feature initializes the crypto provider without requiring the application to do so.

@serprex
Copy link
Contributor Author

serprex commented Jun 17, 2024

I agree. yup-oauth2 will stop requiring setting default provider in next release

There are 3 versions we're dealing with:

  1. current yup-oauth2 release, which errors out when no default set
  2. current head of yup-oauth2 master branch, which sets default to ring when no default already set
  3. do not set rustls default provider dermesser/yup-oauth2#234 which ignores defaults, explicitly constructing httpsconnector to use ring

@serprex
Copy link
Contributor Author

serprex commented Jun 17, 2024

oh, re question, I'll review pr, for pgwire ring is in default features, it someone wants aws-lc-rs they disable defaults

edit: same for yup-oauth2 pr, has default = ["hyper-rustls", "service-account", "ring"]

probably easiest to wait on next yup-oauth2 release before releasing new gcp-bigquery-client

@lquerel
Copy link
Owner

lquerel commented Jun 17, 2024

@serprex I agree, I will wait for the next release of yup-oauth2 to deliver a new version of gcp-bigquery-client. It will be simpler and cleaner.

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.

None yet

2 participants