openssl icon indicating copy to clipboard operation
openssl copied to clipboard

Create API to get curve parameters on a group

Open rickmark opened this issue 3 years ago • 7 comments

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 avatar Mar 30 '21 20:03 rickmark

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

rickmark avatar Mar 30 '21 22:03 rickmark

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.

rhenium avatar Mar 31 '21 05:03 rhenium

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

rickmark avatar Mar 31 '21 17:03 rickmark

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.

rhenium avatar Apr 02 '21 12:04 rhenium

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 avatar Apr 02 '21 20:04 rickmark

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

rickmark avatar Apr 02 '21 20:04 rickmark

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.

rhenium avatar Apr 04 '21 13:04 rhenium