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

Create API to get curve parameters on a group #425

Open
rickmark opened this issue Mar 30, 2021 · 7 comments · May be fixed by #432
Open

Create API to get curve parameters on a group #425

rickmark opened this issue Mar 30, 2021 · 7 comments · May be fixed by #432

Comments

@rickmark
Copy link
Contributor

You are able to call new(:GFp, p, a, b) but are not able to pull those values later.

This becomes a problem when you want to for instance open a curve by name and the the value for field for modular operations. I think we should support a method like OpenSSL::PKey::EC::Group#curve_params => [ :GFp, p, a, b ]

This will require doing a little more work because that getter will first need to query the EC_GROUP_method_of for the group, then if it is GFp or GF2m pull the values of the field, a, b, etc as needed for the curve type. Since creating a new curve this is supported it can be a get only attribute.

@rickmark
Copy link
Contributor Author

Upon thought perhaps this should be a few different calls:

OpenSSL::PKey::EC::Group#method => Symbol where symbol is in [ :GFp_simple, :GFp_mont, :GFp_nist, :GF2m_simple ]

OpenSSL::PKey::EC::Group#field_type => Symbol where symbol in [ :GFp, GF2m ]

OpenSSL::PKey::EC::Group#field => aBN where result is the field

OpenSSL::PKey::EC::Group#curve_params => [ ... ] where the array returns the values as stated originally so that they can be used directly into a call to #new

@rhenium
Copy link
Member

rhenium commented Mar 31, 2021

OpenSSL::PKey::EC::Group#method => Symbol where symbol is in [ :GFp_simple, :GFp_mont, :GFp_nist, :GF2m_simple ]

This sounds like just OpenSSL's implementation detail. #432 doesn't include this and I feel this could be skipped.

OpenSSL::PKey::EC::Group#field_type => Symbol where symbol in [ :GFp, GF2m ]

Should it simply return the text representation of the NID (in other words, "prime-field" or "characteristic-two-field" - as returned by OBJ_nid2ln())?

In the upcoming OpenSSL 3.0, EC_METHOD_get_field_type() is going to be replaced by EVP_PKEY_get_params() with OSSL_PKEY_PARAM_EC_FIELD_TYPE argument. OpenSSL 3.0 is still in development and the API will likely change, but the general trend is to use strings to specify algorithm (rather than NID - exposing them out of internals has caused issues for OpenSSL).

It's not ideal if we have to maintain code like:

if (!strcmp(ft, "prime-field"))
    return ID2SYM(rb_intern("GFp"));
if (!strcmp(ft, "characteristic-two-field"))
    return ID2SYM(rb_intern("GF2m"));

OpenSSL::PKey::EC::Group#field => aBN where result is the field

OpenSSL::PKey::EC::Group#curve_params => [ ... ] where the array returns the values as stated originally so that they can be used directly into a call to #new

Why should it be an Array? Hash seems to be a better fit. If a new Group object with the same parameters is required, one can simply use #dup.

Also perhaps it could be named #params - "curve" is obvious anyway.

@rickmark
Copy link
Contributor Author

Should it simply return the text representation of the NID (in other words, "prime-field" or "characteristic-two-field" - as returned by OBJ_nid2ln())?

I did it this way to match Group#initialize where it takes those symbols.

Why should it be an Array? Hash seems to be a better fit. If a new Group object with the same parameters is required, one can simply use #dup.

Again, done to match #initialize which is a arg list not a hash (I agree hashes are a better idea though)

Also perhaps it could be named #params - "curve" is obvious anyway.

Wanted to avoid conflict with other params concepts like rails

@rhenium
Copy link
Member

rhenium commented Apr 2, 2021

Should it simply return the text representation of the NID (in other words, "prime-field" or "characteristic-two-field" - as returned by OBJ_nid2ln())?

I did it this way to match Group#initialize where it takes those symbols.

I understand. I think we should rather have Group#initialize take sn/ln ("prime-field" or "characteristic-two-field"), too, for consistency.

The :GFp and :GF2m are ruby/openssl-specific keywords and those have increased maintenance burden, when OpenSSL is a moving target. It seems about the right timing to switch to OpenSSL's vocabulary.

Why should it be an Array? Hash seems to be a better fit. If a new Group object with the same parameters is required, one can simply use #dup.

Again, done to match #initialize which is a arg list not a hash (I agree hashes are a better idea though)

I don't get why one would want to pass the returned value to Group#initialize as-is. If it's not needed, can we use Hash instead?

Group#initialize could also take keyword arguments, but that would be a different topic.

Also perhaps it could be named #params - "curve" is obvious anyway.

Wanted to avoid conflict with other params concepts like rails

OpenSSL::PKey::{RSA,DSA,DH} have #params method to dump the content as a Hash, so I'm not worried about that.

@rickmark
Copy link
Contributor Author

rickmark commented Apr 2, 2021

I understand. I think we should rather have Group#initialize take sn/ln ("prime-field" or "characteristic-two-field"), too, for consistency.

What about taking an Int and defining const values for OpenSSL::PKey::EC::NID_X9_62_prime_field and OpenSSL::PKey::EC:: NID_X9_62_characteristic_two_field instead of strings? (I too hate magic strings).

At least that way if there is a third value some time if the feature clients can just have their own const.

I don't get why one would want to pass the returned value to Group#initialize as-is. If it's not needed, can we use Hash instead?

Yeah - a hash is a better call here, will make changes. Not like it cant be called with Group.new(NID_X9_62_prime_field, params[:p], params[:a], params[:b]) right?

@rickmark
Copy link
Contributor Author

rickmark commented Apr 2, 2021

Also looks like EC_GROUP_get_field_type was added in OpenSSL 3

For prior versions will have to check using EC_GROUP_method_of

Will implement with a have_func ifdef

@rhenium
Copy link
Member

rhenium commented Apr 4, 2021

What about taking an Int and defining const values for OpenSSL::PKey::EC::NID_X9_62_prime_field and OpenSSL::PKey::EC:: NID_X9_62_characteristic_two_field instead of strings? (I too hate magic strings).

group.field_type returning Integer 406 (== NID_X9_62_prime_field) wouldn't be very useful.

Actually, OpenSSL looks like going towards getting rid of NID in public API and has started using const char * as the canonical way to represent identifiers; EVP_PKEY_get_params() is a (somewhat extreme) example. So I don't think we have a choice here...

I don't think OpenSSL::PKey::EC::NID_X9_62_prime_field = "prime-field" would contribute to making code more readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants