Pluggable Crypto / Update reqwest 0.13#707
Conversation
|
Ping? Would be nice to avoid having duplicate reqwest versions in downstream projects. |
|
I've given this another rebase, I think it's more blocked on review/maintainer capacity though? |
|
Maintainers, could someone take a look? Would be nice to avoid duplicate dependencies in downstream applications that are up to date with the rest of the ecosystem (and it's been 5 months since reqwest 0.13 was released). |
Tpt
left a comment
Tpt
left a comment
There was a problem hiding this comment.
Not a maintainer but I would love to see this MR get merged.
I allowed myself 3 suggestions.
| - name: Check no crypto crates | ||
| run: | ||
| cargo tree --features gcp-no-crypto,aws-no-crypto,azure-no-crypto,http-no-crypto \ | ||
| | grep -qE '\b(ring|openssl)\b' && { echo "❌ disallowed crate found"; exit 1; } || echo "✅ no disallowed crates" |
There was a problem hiding this comment.
I guess this test should also add aws-lc-rs?
| | grep -qE '\b(ring|openssl)\b' && { echo "❌ disallowed crate found"; exit 1; } || echo "✅ no disallowed crates" | |
| | grep -qE '\b(ring|openssl|aws-lc-rs)\b' && { echo "❌ disallowed crate found"; exit 1; } || echo "✅ no disallowed crates" |
| "user_agent" => Ok(Self::UserAgent), | ||
| _ => Err(super::Error::UnknownConfigurationKey { | ||
| store: "HTTP", | ||
| store: "http-no-crypto", |
There was a problem hiding this comment.
find/replace error?
| store: "http-no-crypto", | |
| store: "HTTP", |
| Ok(_) => Err(Error::MissingKey), | ||
| Err(source) => Err(Error::ReadPem { source }), | ||
| } | ||
| #[cfg(feature = "aws-lc-rs")] |
There was a problem hiding this comment.
hyper-nit (please ignore): moving the cfg switch inside of the function might help ensuring the signature does not diverge between aws-lc and rustls (same for the other functions)
|
Apologies this is still on my list, things have just been extremely busy at work and maintaining this and arrow-rs is no longer my day job 😅. I'll try to take a look this evening. |
| quick-xml = { version = "0.39.0", features = ["serialize", "overlapped-lists"], optional = true } | ||
| rand = { version = "0.10", default-features = false, features = ["std", "std_rng", "thread_rng"], optional = true } | ||
| reqwest = { version = "0.12", default-features = false, features = ["rustls-tls-native-roots", "http2"], optional = true } | ||
| reqwest = { version = "0.13", default-features = false, features = ["http2"], optional = true } |
There was a problem hiding this comment.
Can I request we make a separate PR to upgrade the version of reqwest to 0.13
That way we won't tie the fate of that upgrade to the pluggable crypto stuff (and it will maek the PR easier / faster to review)
There was a problem hiding this comment.
Unfortunately the reqwest upgrade ends up tied to this, because there were changes to the way reqwest handles encryption
|
Given it looks like we will make a new major version anyways I think it would be good to try and update reqwest as well |
6 participants
NOTE: this is a rebased version of #585
Which issue does this PR close?
Closes #413.
Rationale for this change
What changes are included in this PR?
Adds the ability to compile object_store without a dependency on ring, and instead use aws-lc-rs or a user-provided crypto implementation. Also changes the default to aws-lc-rs, which the ecosystem seems to be moving toward as a default.
Also upgrades
reqwestto 0.13, and makes it possible to configure reqwest with any TLS backend by selecting a-no-cryptofeature.Are there any user-facing changes?