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

Migrate x25519 to use rust-openssl #7933

Merged
merged 1 commit into from Mar 24, 2023
Merged

Migrate x25519 to use rust-openssl #7933

merged 1 commit into from Mar 24, 2023

Conversation

alex
Copy link
Member

@alex alex commented Dec 25, 2022

No description provided.

@alex alex mentioned this pull request Dec 25, 2022
11 tasks
@alex alex force-pushed the rust-x25519 branch 5 times, most recently from 83771e6 to 567a974 Compare December 25, 2022 15:44
@alex
Copy link
Member Author

alex commented Dec 25, 2022

Ok, BoringSSL test failures are quite a yak shave:

  • rust-openssl's usual binding approach is hand-written, this includes the values of constants (e.g. EVP_PKEY_X25519)
  • The values it has are from OpenSSL. BoringSSL doesn't use the same ones.
  • This causes EVP_PKEY_CTX_new_id to fail since its looking for something by the wrong ID.
  • sfackler/rust-openssl@e610c0f describes how linking BoringSSL into rust-openssl requires special handling.
  • Right now we simply treat BoringSSL the same as OpenSSL, so more thought is required here.

@alex
Copy link
Member Author

alex commented Mar 16, 2023

Self-notes:

  • Hang is in memleak test_x25519_pubkey_from_private_key
  • Stack at the point of the hang: x25519 generate -> calls into OpenSSL -> calls into something else in OpenSSL (probably to allocate) -> calls into an ffi callback (probably our memleak malloc) -> tries to acquire the GIL and hangs
  • This obviously all works on CPython
  • The proximate change here is that with this PR we're not releasing the GIL on generating a key (or anything else for that matter)
    • Unclear if we should release it for key gen; generating 32 random bytes should be very fast
    • Even if we did release the GIL here, it'd just kick the can down the road
  • Unclear if having a cffi callback invoked with the GIL held is expected to work or not (though it clearly does on CPython)

@alex alex mentioned this pull request Mar 17, 2023
@alex alex force-pushed the rust-x25519 branch 3 times, most recently from 3dc1c2b to b259544 Compare March 20, 2023 00:57
@alex alex marked this pull request as ready for review March 20, 2023 00:57
@alex alex force-pushed the rust-x25519 branch 2 times, most recently from 0cca511 to bdff655 Compare March 20, 2023 01:42
@reaperhulk reaperhulk merged commit c8328c0 into pyca:main Mar 24, 2023
64 checks passed
@alex alex deleted the rust-x25519 branch March 24, 2023 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants