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

[crypto/ec] deprecate Jprojective_coordinates_GFp functions #11527

Closed
wants to merge 4 commits into from

Conversation

@bbbrumley
Copy link
Contributor

@bbbrumley bbbrumley commented Apr 12, 2020

Reiterating from #8332:

Deprecate the following two functions:

  1. EC_POINT_set_Jprojective_coordinates_GFp
  2. EC_POINT_get_Jprojective_coordinates_GFp

Whether a point has 3, 4, 5, ... coordinates, and what coordinate system the EC_METHOD uses, should not be public facing.

Before I sat down to write this PR, I used my Google-fu to search for linking applications using these functions. I only found one: ec_test.

Disclaimer: This is my first time marking something deprecated. I don't really know what I'm doing. Please tell me what I did wrong. (Including documenting the deprecation.)

Tagging @mattcaswell who seemed to like the suggestion. I reckon you have to vote on it.

crypto/ec/ec_lib.c Outdated
}

point->Z_is_one = 0;
return 1;

This comment has been minimized.

@bbbrumley

bbbrumley Apr 12, 2020
Author Contributor

This was early on in the change, and now seems a bit heavy handed. Should I just call ec_GFp_simple_set_Jprojective_coordinates_GFp directly here?

Edit: Yes it was heavy handed. Plus a few other mistakes. Force push coming ...

@bbbrumley bbbrumley force-pushed the bbbrumley:bbb_no_proj branch to b35e6ec Apr 12, 2020
@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Apr 13, 2020

Travis red cross is not relevant.

Copy link
Member

@mattcaswell mattcaswell left a comment

From a documentation perspective we should include a CHANGES entry which summarises the change and gives pointers (perhaps to the man pages) about what users of these APIs should do instead.

The relevant man page should also be updated to clearly indicate in the SYNOPSIS that the functions are deprecated (see DH_generate_parameters.pod for an example). The description should also say that the functions are deprecated and what users should do instead. Finally the HISTORY section should be updated to say when the deprecation occurred.

crypto/ec/ec2_smpl.c Show resolved Hide resolved
* Tests a point known to cause an incorrect underflow in an old version of
* ecp_nist521.c
*/
static int underflow_test(void)

This comment has been minimized.

@mattcaswell

mattcaswell Apr 13, 2020
Member

Moving this test to the internal test is fine. An acceptable alternative is to include this at the top of the file to suppress issues with deprecated warnings (and enclosing the relevant tests in guards for OPENSSL_NO_DEPRECATED_3_0):

/*
 * We need access to the deprecated EC_POINT_get_Jprojective_coordinates_GFp
 * and EC_POINT_set_Jprojective_coordinates_GFp APIs for testing purposes
 * when the deprecated calls are not hidden
 */
#ifndef OPENSSL_NO_DEPRECATED_3_0
# define OPENSSL_SUPPRESS_DEPRECATED
#endif

This comment has been minimized.

@bbbrumley

bbbrumley Apr 14, 2020
Author Contributor

TIL -- thanks for the tip :) I this case, I'd prefer it internal because (as I'm sure you know!) it's pretty common for developers to look at the tests to understand what they're supposed to do. And I wouldn't want to mislead anyone ;)

Copy link
Contributor

@slontis slontis left a comment

Apart form the changes needed in EC_POINT_new.pod it looks good.

@bbbrumley
Copy link
Contributor Author

@bbbrumley bbbrumley commented Apr 14, 2020

Thanks @mattcaswell for the tips and @slontis for the review.

I'll make the doc changes Matt pointed me at in the next week or so.

@bbbrumley
Copy link
Contributor Author

@bbbrumley bbbrumley commented Apr 18, 2020

From a documentation perspective we should include a CHANGES entry which summarises the change and gives pointers (perhaps to the man pages) about what users of these APIs should do instead.

The relevant man page should also be updated to clearly indicate in the SYNOPSIS that the functions are deprecated (see DH_generate_parameters.pod for an example). The description should also say that the functions are deprecated and what users should do instead. Finally the HISTORY section should be updated to say when the deprecation occurred.

This should be done now in fb5df41. I apologize in advance -- I don't know how to test the pod changes. I haven't used perl since the 90s 🙈

@romen
romen approved these changes Apr 18, 2020
Copy link
Member

@romen romen left a comment

LGTM

Copy link
Member

@mattcaswell mattcaswell left a comment

Approved

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Apr 21, 2020

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Apr 21, 2020
Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11527)
@romen
Copy link
Member

@romen romen commented Apr 21, 2020

Merged with 07caec8.

Thanks!

@romen romen closed this Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants