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

Proposal for creating pkeys from raw parameters #555

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anakinj
Copy link

@anakinj anakinj commented Oct 19, 2022

Hi.

I'm Joakim and have never done anything else than used and read the code for this gem.

I'm involved in the ruby-jwt gem and as @bellebaum pointed out in #551 there is the need to create RSA (and other keys) from raw parameters. Due to the changes in mutability of keys in OpenSSL 3.0 it is no longer possible that easily.

This PR is a suggestion on how the interface for the OpenSSL gem could look like and a very rough implementation that probably does not work for all the key types (yet).

It's a while ago I did C so apologies for the potential messiness.

Do you think this direction and way solving the problem is feasible? Im open for all feedback.

@anakinj
Copy link
Author

anakinj commented Oct 25, 2022

Got the RSA generation pretty much working, with user-friendly parameter names for the most common parameters.

Started to look into how to generate EC keys from params, turned out to be a little more complicate than to just throw bignums at it so still a WIP :)

Does this approach make sense:

  • Hash with the different parameters to build the key
  • Choose the hash iterator function based on the desired key type. Raise if there is no support.
  • Translate the hash keys into the OpenSSL constants.
  • Push the key and value into the parameter builder with correct types.
  • Let the builder generate the key

Also would it make sense to incrementally build this and maybe first only support RSA keys and then expanding support for other key types later?

@anakinj
Copy link
Author

anakinj commented Oct 28, 2022

Continued a bit on the EC pkey generation. Seems that OpenSSL only allows passing the pub parameter as an octet string to the builder. It's a little cumbersome to use in this way instead of passing the point as-is or the x y coordinates.

Not sure if we want to go into making it convenient by:

  • Converting x and y into a octet string
  • Some custom parameter for this gem that takes the bignum that represents the point and extract x and y from that depending on the curve type.

Also unsure what other parameters do we want to support on creating, the EC object is pretty simple in Ruby. There is a bunch of different parameters that can be set for the EC pkey, some are inherited from the curve name I guess.

Im not an expert on this so all feedback is appreciated :)

For the use-case Im interested in this implementation would be good enough.

Allow EC keys to be generated based on:

  • curve name
  • private point (octet string representing x and y)
  • private key (unsigned integer)

@anakinj
Copy link
Author

anakinj commented Oct 29, 2022

Continuing my monologue :)

This draft now supports all the PKey types in this gem (DH, DSA, RSA, EC). The EC support is only supporting the most common parameters as mentioned in some previous comment.

Now I'm wondering about backwards compatibility. Would be nice to be able to support the same interface for OpenSSL 1.1 and OpenSSL 3.0. The implementation would probably be very different for OpenSSL 1.1.

Could someone allow the CI to run on this one? Would be interesting to see all the issues. Also still open for feedback on the code part of things.

@anakinj
Copy link
Author

anakinj commented Dec 22, 2022

Feel a little bad for leaving this open like this, but a little clueless on how to continue or if to continue at all.

Any pointers on the direction to take this? Try to use the mutable pkeys in openssl 1.x to handle the parameters or just support openssl 3.0 for now?

Maybe @rhenium could have some ideas?

@rhenium
Copy link
Member

rhenium commented Dec 22, 2022

Thank you for working on this! Sorry for taking so long time to review.

Yes, OpenSSL::PKey.from_parameters(type, param1=>value1, ...) matches what I hope to have in this library!

As for the implementation, I want generic methods directly defined in OpenSSL::PKey to be minimum and independent of key types as much as possible. I wonder if the parameter name → value type mappings could be done without hard-coding them; OpenSSL's docs mention EVP_PKEY_fromdata_settable() and it appears to be provided for this purpose.

Try to use the mutable pkeys in openssl 1.x to handle the parameters or just support openssl 3.0 for now?

The compatibility is nice to have, but I don't find it a requirement. Once we have OpenSSL 3.0 code in C, I expect this can be implemented in Ruby with existing methods (probably in lib/openssl/pkey.rb).

@anakinj
Copy link
Author

anakinj commented Dec 28, 2022

Thanks @rhenium for pointing to EVP_PKEY_fromdata_settable, now the implementation is pretty type agnostic. Left the parameter types OSSL_PARAM_OCTET_PTR and OSSL_PARAM_UTF8_PTR unsupported, did not find any key types that would support these kind of parameter types.

Still not happy with the way the aliases are handled, as said earlier my C skills are a little rusty so passing static arrays to the callback was not that straightforward so for now passing both size and pointer was the only option I could figure out, so probably going return to them soonish.

@collimarco
Copy link

+1 It would be extremely useful to have this method and it would make it easier to migrate some code from OpenSSL v1.1 to OpenSSL v3. Is there any chance to see this merged?

@wilsonsilva
Copy link

Excellent work! Meanwhile, is there a Ruby workaround?

@collimarco
Copy link

@wilsonsilva Probably you can find this commit useful as a start... take a look to the set_keys! method in particular.

@wilsonsilva
Copy link

@wilsonsilva Probably you can find this commit useful as a start... take a look to the set_keys! method in particular.

Thank you. The morning you posted this, I had already been on that repo! Thanks for writing that code.

https://github.com/wilsonsilva/nostr/blob/main/lib/nostr/crypto.rb#L111-L129

@anakinj
Copy link
Author

anakinj commented Dec 9, 2023

I refreshed (and rebased) the branch a bit, curious if the tests pass on CI.

@bdewater
Copy link
Contributor

@rhenium could you approve the CI workflow please?

@junaruga
Copy link
Member

I approved the CI workflow.
@anakinj I think you can also run the GitHub Actions CI workflow on your forked repository without waiting for approval, https://github.com/anakinj/openssl/actions. For example, here is my GitHub Actions page on my forked repository.

@junaruga
Copy link
Member

junaruga commented Mar 23, 2024

I see the failures on the openssl-head and openssl 3 fips cases on CI. For the failure of the openssl-head case, the 2e826d5 fixes it. So, you can pass the case by rebasing this PR on the latest master branch.

@junaruga
Copy link
Member

junaruga commented Mar 23, 2024

For example below are the failures on openssl-head fips.
https://github.com/ruby/openssl/actions/runs/8085746974/job/23008578186?pr=555#step:13:110

I would like you to consider using FIPS-approved crypto algorithms or bits in your new tests as much as possible because I want to see that this new feature is supported in the FIPS cases.

As a reference, I don't think that the following code using the OpenSSL.fips_mode to skip the tests in the FIPS cases is a good practice. I am passionate about rewriting the part in the future.

if defined?(OpenSSL) && defined?(OpenSSL::Provider) && !OpenSSL.fips_mode

Using the method omit_on_fips is possible if there is no alternative way to pass a test in the FIPS case. But I think that it is a last resort.

@anakinj
Copy link
Author

anakinj commented Mar 26, 2024

Thanks for all of the pointers. I'll look into refreshing the branch again and looking into making the tests FIPS compatible.

test/openssl/test_pkey.rb Outdated Show resolved Hide resolved
@anakinj
Copy link
Author

anakinj commented Mar 27, 2024

Thanks for the great pointers again @junaruga, the changes seem to pass CI now. Is there any adjustments that you would wish to see still?

dmp1: source.dmp1,
dmq1: source.dmq1,
iqmp: source.iqmp
)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the position of the ) can be adjusted. As a reference, below is what Rubocop style guide suggests.
https://github.com/rubocop/ruby-style-guide?tab=readme-ov-file#method-arguments-alignment

@junaruga
Copy link
Member

Thanks for the great pointers again @junaruga, the changes seem to pass CI now. Is there any adjustments that you would wish to see still?

@anakinj Thank you for working for applying the FIPS-approved crypto algorithms. I think the logic for the FIPS part is good to me. I think you can squash the 6 commits to 1 commit in this PR now, because eventually I think we want to see just one commit for this PR.

Then perhaps you may need small adjustments about the coding style. While the maximum line length in the current existing code is sometimes more than 80 bytes, I may try to keep the maximum 80 bytes for the code I implement newly if I were you. @rhenium may have his opinion about the style.
https://github.com/rubocop/ruby-style-guide?tab=readme-ov-file#maximum-line-length


key = OpenSSL::PKey.from_parameters("ED25519", pub: "\xD0\x8E\xA8\x96\xB6Fbi{$k\xAC\xB8\xA2V\xF4n\xC3\xD06}R\x8A\xE6I\xA7r\xF6D{W\x84")
assert_instance_of OpenSSL::PKey::PKey, key
assert_equal "-----BEGIN PUBLIC KEY-----\nMCowBQYDK2VwAyEA0I6olrZGYml7JGusuKJW9G7D0DZ9UormSady9kR7V4Q=\n-----END PUBLIC KEY-----\n", key.public_to_pem
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use Ruby's heredoc syntax for a better readability. You can refer to other parts using the heredoc in the test/openssl/test_pkey.rb.

Copy link
Author

Choose a reason for hiding this comment

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

Adjusted to have the expected public presented in heredoc. Looks way prettier

def test_s_from_parameters_rsa_with_n_e_and_d_given_as_integers
new_key = OpenSSL::PKey.from_parameters("RSA", n: 3161751493,
e: 65537,
d: 2064855961)
Copy link
Member

@junaruga junaruga Mar 27, 2024

Choose a reason for hiding this comment

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

Is it intentional that you are using mixture of the symbol keys such as n: in some tests and String keys such as "n" => in other tests?

Copy link
Author

Choose a reason for hiding this comment

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

The intention was to test that keys could also be passed as strings. Maybe it would be clearer to have a dedicated test for that instead of testing it "on the side"?

Copy link
Member

@junaruga junaruga Mar 27, 2024

Choose a reason for hiding this comment

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

Yes, I think the dedicated test for testing the symbol keys is a good idea to show the intention clearly. Checking the existing test/openssl/test_pkey.rb, there are only the cases using String keys for a hash in the file. So, in my opinion, maybe it's better to use the String keys as a default, and a dedicated test for testing the symbol keys.

Copy link
Author

Choose a reason for hiding this comment

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

Now tests are using string keys, except one dedicated test with symbol keys

"rsa-exponent1" => source.dmp1,
"rsa-exponent2" => source.dmq1,
"rsa-coefficient1" => source.iqmp
)
Copy link
Member

Choose a reason for hiding this comment

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

The thing for the ) is same with the one I mentioned in another part.

Copy link
Author

Choose a reason for hiding this comment

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

Adjusted the alignment of everything

ext/openssl/ossl_pkey.c Show resolved Hide resolved
else
from_params_args.aliases = fcc_aliases;

rb_hash_foreach(options, &add_parameter_to_builder, (VALUE) &from_params_args);
Copy link
Member

Choose a reason for hiding this comment

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

ctx will leak if rb_hash_foreach() raises an exception. Please wrap it with rb_protect() so that they can freed before escaping from this function.

Copy link
Author

Choose a reason for hiding this comment

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

Adjusted this one. There were more leaks than just this one but now I think it might be freeing everything when exceptions happen. This approach also removes the need to free things inside the iterator function.

static VALUE
ossl_pkey_s_from_parameters(int argc, VALUE *argv, VALUE self)
{
#if OSSL_OPENSSL_PREREQ(3, 0, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: please remove spaces before #

Comment on lines 250 to 253
e = assert_raise(OpenSSL::PKey::PKeyError) {
OpenSSL::PKey.from_parameters("ASR", {})
}
assert_match(/^EVP_PKEY_CTX_new_from_name: unsupported/, e.message)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
e = assert_raise(OpenSSL::PKey::PKeyError) {
OpenSSL::PKey.from_parameters("ASR", {})
}
assert_match(/^EVP_PKEY_CTX_new_from_name: unsupported/, e.message)
assert_raise_with_message(OpenSSL::PKey::PKeyError, /^EVP_PKEY_CTX_new_from_name: unsupported/) {
OpenSSL::PKey.from_parameters("ASR", {})
}

We can use assert_raise_with_message for this kind of test.

end

def test_s_from_parameters_rsa_with_openssl_internal_names
source = OpenSSL::PKey::RSA.generate(2048)
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid generating a new RSA/DSA/DH key because it can take a long time. For this feature, pre-generated keys (Fixtures.pkey("rsa2048"), for example, for RSA) should work.

Copy link
Author

Choose a reason for hiding this comment

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

I change the key generations to use fixtures. For the EC test I added 3 new fixtures for each EC curve used in these tests. The existing p256.pem and ec-prime256v1.pem serve the same purpose and could probably be unified, but think that could happen in separate.

@junaruga
Copy link
Member

junaruga commented Mar 27, 2024

Thanks for the great pointers again @junaruga, the changes seem to pass CI now. Is there any adjustments that you would wish to see still?

@anakinj Thank you for working for applying the FIPS-approved crypto algorithms. I think the logic for the FIPS part is good to me. I think you can squash the 6 commits to 1 commit in this PR now, because eventually I think we want to see just one commit for this PR.

Then perhaps you may need small adjustments about the coding style. While the maximum line length in the current existing code is sometimes more than 80 bytes, I may try to keep the maximum 80 bytes for the code I implement newly if I were you. @rhenium may have his opinion about the style. https://github.com/rubocop/ruby-style-guide?tab=readme-ov-file#maximum-line-length

Below is the ruby/ruby's coding style. And there is no comment about the maximum line length. So, I think the style of the maximum length 80 bytes is optional, and nice to have.

https://github.com/ruby/ruby/wiki/Developer-How-To#coding-style

@rhenium
Copy link
Member

rhenium commented Mar 27, 2024

I'm sorry for the slow response.

As for the method name, can we maybe have a different one than from_parameters? We currently have a similarly named method OpenSSL::PKey.generate_parameters which generates a new pkey object containing only key parameters (though I'm starting to regret that I didn't name it generate_key_parameters instead). It sounds too similar while having a different function.

This is based on EVP_PKEY_fromdata(), so from_data is my suggestion. I'm also hoping to add the counterpart to the method in future, which should correspond to EVP_PKEY_todata().

The implementatation, I'm not sure what value the alias names for RSA/DSA/DH give us. Since this is a completely new method, I think we can simply forget about the old names and only accept what EVP_PKEY_fromdata() supports.

@anakinj
Copy link
Author

anakinj commented Mar 27, 2024

Thanks for all the feedback. The suggested from_data sounds great.

About the aliases, it's mainly because of convenience and keeping the consistency on how this gem has named the parameters and how the average user is subject to the parameters. I have a few arguments for and against the aliases :)

Being able to pass back the same values that are referred to for the PKey types. Think it just would be a bit confusing that sometimes the RSA parameter is p and sometimes rsa-factor1. Im a bit biased in this example but when mapping a JWK RSA key to a PKey object the transformation from p to rsa-factor1 needs to happen somewhere, so why not in the place that actually knows the relation between the different names used for different parameters.

Think the biggest challenge with all this is the need to know a bit of the internals of openssl to be able to use this method. Maybe all this could be documented for the method. With comprehensive descriptions of what these parameters also sometimes are called. Maybe there could even be some new methods on the PKey objects that use the names used internally. OpenSSL::PKey::RSA#p would be an alias for OpenSSL::PKey::RSA#rsa-factor1.

Also the aliases will cause a bit of confusion if a potential to_data or OpenSSL::PKey#data method will return different keys than passed into from_data. So for the future it would be better not to have them.

I'm fine either way, I would say you decide if aliases are valuable or not. I will comply :)

EDIT:
Also rsa-factor1 cannot be represented as a Ruby symbol that nicely :)

@anakinj anakinj force-pushed the pkey-from-parameters branch 2 times, most recently from 762bdb1 to 2b73110 Compare March 28, 2024 05:44
@anakinj anakinj requested a review from rhenium April 2, 2024 13:23
@anakinj
Copy link
Author

anakinj commented Jun 17, 2024

@rhenium I've adjusted based on comments, what do you think about the changes now? Still open to drop the aliases if that is desired.

@rhenium
Copy link
Member

rhenium commented Jul 3, 2024

Sorry for the slow response again.

I prefer not having algorithm-specific code in ossl_pkey.c.

I'm not necessarily against adding an alias if it makes it easier to migrate existing user code, but it should belong to OpenSSL::PKey::RSA. Such examples exist today: for instance, OpenSSL::PKey::RSA.{new,generate}(2048) which wraps the generic OpenSSL::PKey.generate_key("RSA", rsa_keygen_bits: 2048). Perhaps it can be a separate patch?


Think the biggest challenge with all this is the need to know a bit of the internals of openssl to be able to use this method. Maybe all this could be documented for the method. With comprehensive descriptions of what these parameters also sometimes are called.

These current names on OpenSSL::PKey::RSA were directly taken from OpenSSL's API. It seems that all OpenSSL functions using the current names (p, q, dmp1, dmq1, and iqmp) are deprecated now. As an OpenSSL binding library, I think we have to follow the change.

While p and q for the primes are commonly used, crypto libraries differ a lot in the CRT parameter names. So users would have to consult OpenSSL's man page anyway...

Maybe there could even be some new methods on the PKey objects that use the names used internally. OpenSSL::PKey::RSA#p would be an alias for OpenSSL::PKey::RSA#rsa-factor1.

We should add these for the new names.

};

static int
add_data_to_builder(VALUE key, VALUE value, VALUE arg) {
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: please insert new line before {.


if (EVP_PKEY_fromdata(ctx, &pkey, EVP_PKEY_KEYPAIR, params) <= 0) {
EVP_PKEY_CTX_free(ctx);
EVP_PKEY_free(pkey);
Copy link
Member

Choose a reason for hiding this comment

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

This line should be unnecessary. We can assume pkey is not updated when EVP_PKEY_fromdata() fails.

end

def test_s_from_data_ec_pub_given_as_string
source = Fixtures.pkey("ec-prime256v1")
Copy link
Member

Choose a reason for hiding this comment

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

Since these test cases for PKey::EC don't depend on the actual key data, we can generate a key inside the test cases to simplify it. EC key generation should be fast unlike RSA/DSA/DH.

(text/openssl/fixtures/p256.pem already exists for this prime256v1 curve.)

* == Example
* pkey = OpenSSL::PKey.from_data("RSA", n: 3161751493, e: 65537, d: 2064855961)
* pkey.private? #=> true
* pkey.public_key #=> #<OpenSSL::PKey::RSA...
Copy link
Member

Choose a reason for hiding this comment

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

RSA#public_key doesn't seem to be a useful method for this example.

Suggested change
* pkey.public_key #=> #<OpenSSL::PKey::RSA...
* pkey.n #=> #<OpenSSL::BN 3161751493>

case OSSL_PARAM_OCTET_PTR:
ossl_raise(ePKeyError, "Unsupported parameter \"%s\", handling of OSSL_PARAM_UTF8_PTR and OSSL_PARAM_OCTET_PTR not implemented", key_ptr);
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

Please handle default too. I believe it would not reachable today, but just in case a future OpenSSL version adds new data types.

#else
rb_raise(ePKeyError, "OpenSSL::PKey.from_data requires OpenSSL 3.0 or later");
#endif
}
Copy link
Member

Choose a reason for hiding this comment

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

We haven't been very consistent on this, but I think it might be a better design to not define this method for unsupported OpenSSL versions.

I guess a user code that must work with both OpenSSL 1.1 and 3.x will have to check the availability of this method, so it would be easier that way.

@anakinj anakinj force-pushed the pkey-from-parameters branch 2 times, most recently from b05603b to a28fb0b Compare September 7, 2024 18:34
@anakinj
Copy link
Author

anakinj commented Sep 7, 2024

@rhenium Finally got around looking into and adjusting based on your comments, thanks for those.

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

Successfully merging this pull request may close these issues.

6 participants