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

Fix SSL curve identifiers #239

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Conversation

jrouviere
Copy link
Contributor

While testing I noticed that SslRef::curve() was returning 29 (SSL_CURVE_X25519) instead of 948 (NID_X25519).
I assume most users rely on SslCurve::name() which is correct because it's passing the id back to the ffi layer.

@inikulin inikulin requested review from nox, bwesterb and ghedo June 3, 2024 13:01
@bwesterb bwesterb self-requested a review June 3, 2024 13:15
@bwesterb
Copy link
Member

bwesterb commented Jun 3, 2024

This breaks set_curves, which expects NIDs.

Some functions use the NID_* constants, and some use the SSL_CURVE_* ones.
Extract from the documentation:
> Where NIDs are unstable constants specific to OpenSSL and BoringSSL, group IDs are defined by the TLS protocol. Prefer the group ID representation if storing persistently, or exporting to another process or library.
@rushilmehra
Copy link
Collaborator

rushilmehra commented Jun 18, 2024

CI is borked, I'm trying to fix it in #240. Let's hold off on merging this until CI is fixed

@rushilmehra rushilmehra self-requested a review June 18, 2024 08:22
@rushilmehra rushilmehra merged commit c7fd324 into cloudflare:master Jul 30, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants