Use aws-lc-rs instead of ring for TLS #4734
Conversation
kcon-stackav
changed the title
|
Thanks for the pull request! |
|
This Windows failure also looks somewhat problematic (see aws/aws-lc#1477 and example fix) |
kcon-stackav
force-pushed
the
kcon-stackav/use-aws-lc-instead-of-ring
branch
from
7f091b3 to
34852fe
Compare
kcon-stackav
force-pushed
the
kcon-stackav/use-aws-lc-instead-of-ring
branch
from
34852fe to
7fdb31c
Compare
| - name: "Install nasm" | ||
| uses: ilammy/setup-nasm@v1 |
There was a problem hiding this comment.
@zanieb I'm not very familiar with maturin, do you think installing NASM is needed for the build-binaries Windows job too?
uv/.github/workflows/build-binaries.yml
Lines 153 to 162 in 66a4b8e
There was a problem hiding this comment.
maturin calls cargo build, so they same rules should apply whether it's maturin or not.
| @@ -216,6 +218,9 @@ jobs: | |||
| run: | | |||
| Copy-Item -Path "${{ github.workspace }}" -Destination "${{ env.DEV_DRIVE }}/uv" -Recurse | |||
|
|
|||
| - name: "Install nasm" | |||
| uses: ilammy/setup-nasm@v1 | |||
There was a problem hiding this comment.
Note to self we should audit this action and consider just implementing it ourself if the install is straightforward
There was a problem hiding this comment.
ilammy also known for https://github.com/ilammy/msvc-dev-cmd
| if aws_lc_rs::default_provider().install_default().is_err() { | ||
| warn_user_once!("Failed to install aws_lc_rs as the default TLS provider."); | ||
| } |
There was a problem hiding this comment.
It looks like the error attached to the result is just Self and has no information, right?
There was a problem hiding this comment.
Yes I understand the error holds the CryptoProvider that was previously installed as the default in that case.
There was a problem hiding this comment.
I guess we could capture it to say what we're using instead but it doesn't seem critical.
There was a problem hiding this comment.
Ah yeah I thought about doing that but I wasn't sure the CryptoProvider had a nice human-readable name field we could use: https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html
|
Sorry this is lingering, I'm just not sure of the trade-offs here since it changes our release pipeline and hasn't been requested by many people. |
No worries, and feel free to close this PR if not enough people are wanting it since I learned that switching from |
|
I think I'll close for now since this changes our build dependencies and there isn't a compelling reason to switch over at this time. Happy to reconsider in the future. |
|
@zanieb As a part of Rustup's plan for the v1.28.0 release cycle, I've migrated Rustup to |
|
Thanks @rami3l, I appreciate the heads up and am definitely interested. |
Summary
This PR switches the TLS backend used by
reqwestfromringtoaws-lcto support more SSL certificate signature algorithms (especially P521 algorithms which aren't yet supported byring: briansmith/ring#1631).Fixes #4534
Test Plan
I used
uv pip installto try installing a package from a private PyPI server whose SSL certificate was signed using the ECDSA SHA-512 certificate signature algorithm and, using the Rust debugger, observed thatuvdid not fail to install the package due to not supporting the ECDSA SHA-512 certificate signature algorithm.