lquerel / gcp-bigquery-client

GCP BigQuery Client (Rust)
Apache License 2.0
97 stars 67 forks source link

yup-oauth2 10 #89

Closed serprex closed 5 months ago

serprex commented 5 months ago

hyper v1 removed HttpConnector, now exists in hyper-utils

lquerel commented 5 months ago

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

lquerel commented 5 months ago

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 https://github.com/lquerel/gcp-bigquery-client/commit/cf3cd5c55bcd324ef24bf1f54af7f1a3da4aa729

serprex commented 5 months ago

@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: https://github.com/sunng87/pgwire/pull/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: https://github.com/dermesser/yup-oauth2/pull/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 commented 5 months ago

found https://github.com/dermesser/yup-oauth2/pull/232 which does what I'd like with ring/aws-lc-rs features

lquerel commented 5 months ago

@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 commented 5 months ago

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. https://github.com/dermesser/yup-oauth2/pull/234 which ignores defaults, explicitly constructing httpsconnector to use ring
serprex commented 5 months ago

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 commented 5 months ago

@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.