-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add rustls-platform-verifier
binding
#419
Conversation
src/cipher.rs
Outdated
pub extern "C" fn rustls_platform_server_cert_verifier( | ||
verifier_out: *mut *mut rustls_server_cert_verifier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also take an (optional) rustls_root_cert_store
to use for Verifier::new_with_extra_roots
; however, it doesn't exist on e.g. macOS and Windows, so one would either need a platform-conditional API or at least new error codes for these cases. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an open issue (rustls/rustls-platform-verifier#58) on rustls-platform-verifier to add support for extra roots for the other platforms.
I think we should keep this simple and avoid implementing it in -ffi until the feature is available more broadly. There's not very much platform-conditional code in this repo and it'd be great to avoid adding more if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is looking good. I had a few suggestions for documentation related fixes but the actual implementation is great 🌠
Have not yet taken a closer look at the Windows failures
I did some fiddling of my own and I believe we need crypt32.lib
added to the Windows CMakeLists.txt
in the target_link_libraries
list.
Looking into that was worthwhile because (as mentioned in a review comment) it uncovered that I accidentally removed our test coverage for the expected Windows link libraries changing 😓
Applying this commit 575c80d fixes the Windows build in my testing. It tidies the overall list to match what I get using RUSTFLAGS=--print native-static-libs cargo build
in CI while also adding crypt32.lib
.
81e77da
to
a16b456
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The latest revisions look great. I left a comment suggesting we separate the new test but that's a minor detail.
This is to be expected from the MSRV update. We'll admin merge and fixup the branch protection rules afterwards. |
a16b456
to
396923d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks great!
396923d
to
819ba4b
Compare
819ba4b
to
4d65d5a
Compare
Thanks @amesgen ! |
I've retroactively fixed the branch protection rules for the new MSRV. |
Closes #417
Have not yet taken a closer look at the Windows failuresNow resolved, see the review comments below.