Proposal for creating pkeys from raw parameters

[anakinj]

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]

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]

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]

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]

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]

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]

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]

+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]

Excellent work! Meanwhile, is there a Ruby workaround?

[collimarco]

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

[wilsonsilva]

@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]

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

[bdewater]

@rhenium could you approve the CI workflow please?

[junaruga]

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]

I see the failures on the openssl-head and openssl 3 fips cases on CI. For the failure of the openssl-head case, the https://github.com/ruby/openssl/commit/2e826d571546cdc3beaa884f9e522a102d531641 fixes it. So, you can pass the case by rebasing this PR on the latest master branch.

[junaruga]

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.

https://github.com/ruby/openssl/blob/a8caa63729e66f8ad5cb503f0199e099042faac5/test/openssl/test_provider.rb#L3

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]

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

[anakinj]

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?

[junaruga]

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

[junaruga]

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]

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]

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 :)