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

add support for EC_POINT_bn2point #1645

Merged
merged 3 commits into from
Jul 3, 2024
Merged

Conversation

samuel40791765
Copy link
Contributor

Issues:

Addresses CryptoAlg-1715

Description of changes:

Ruby happens to consume this. This is deprecated in OpenSSL 3.0 and I've marked it likewise in AWS-LC as well. Although Ruby has generally moved off of deprecated 3.0 APIs, they still depend on this and later versions like 3.2 and 3.3 have not removed support for this either.

Call-outs:

N/A

Testing:

Tests built upon original tests for EC_POINT_point2bn.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@samuel40791765 samuel40791765 requested a review from a team as a code owner June 19, 2024 23:41
// On success, it returns the BIGNUM pointer supplied or, if |ret| is NULL,
// allocates and returns a fresh |BIGNUM|. On error, it returns NULL. The |ctx|
// argument may be used if not NULL.
OPENSSL_EXPORT OPENSSL_DEPRECATED BIGNUM *EC_POINT_point2bn(
const EC_GROUP *group, const EC_POINT *point, point_conversion_form_t form,
BIGNUM *ret, BN_CTX *ctx);

// EC_POINT_bn2point is like |EC_POINT_point2bn|, but converts a |BIGNUM| to an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't make sense (nor does the EC_POINT_point2bn documentation :)). An EC point consists of (at least) two coordinates x and y, which are usually of type BIGNUM. So what's the relation between bn and point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you're saying now, OpenSSL also deprecated these functions for this reason.

I've reworded the function documentation to describe the actual relation (there's not really much of one, it's just another conversion of the serialized output).

crypto/ec_extra/ec_asn1.c Show resolved Hide resolved
crypto/ec_extra/ec_asn1.c Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 78.19%. Comparing base (3da2c4a) to head (e5cc14e).

Files Patch % Lines
crypto/ec_extra/ec_asn1.c 66.66% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1645      +/-   ##
==========================================
- Coverage   78.21%   78.19%   -0.02%     
==========================================
  Files         571      571              
  Lines       95405    95458      +53     
  Branches    13705    13705              
==========================================
+ Hits        74621    74647      +26     
- Misses      20173    20201      +28     
+ Partials      611      610       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crypto/ec_extra/ec_asn1.c Show resolved Hide resolved
crypto/ec_extra/ec_asn1.c Show resolved Hide resolved
include/openssl/ec.h Outdated Show resolved Hide resolved
include/openssl/ec.h Outdated Show resolved Hide resolved
@samuel40791765 samuel40791765 merged commit bf992e7 into aws:main Jul 3, 2024
99 checks passed
@samuel40791765 samuel40791765 deleted the bn2point branch July 3, 2024 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants