Add support for raw private/public keys by sylph01 · Pull Request #646 · ruby/openssl

sylph01 · 2023-07-01T07:50:23Z

Based off of #373, applied review comments and fixed tests for older versions.

I removed the private? / public? methods and marshalling support that relies on these methods, as these methods are causing old versions of OpenSSL to not work. I tried fixing these methods, but wasn't able to come up with a consistent fix that works on all versions (out of scope for this PR, but the errors were mostly segfaults on the buffer used in EVP_PKEY_get_raw_private_key). I am thinking on working on this later, so any advice/support would be appreciated.

I would like to first add support for raw private/public keys independently, as these methods are needed to implement protocols that rely on X25519/X448 raw private/public key support (such as Hybrid Public Key Encryption and QUIC).

sorah · 2023-07-01T09:28:44Z

ext/openssl/ossl_pkey.c

+    int pkey_id;
+    size_t keylen;
+
+#ifndef HAVE_EVP_PKEY_NEW_RAW_PRIVATE_KEY


This doesn't cover the call of EVP_PKEY_new_raw_private_key below so this doesn't prevent a compile error when !HAVE_EVP_PKEY_NEW_RAW_PRIVATE_KEY.

[I believe] This kind of error should result into NoMethodError by excluding whole method implementation and declaration.

Thanks for the comment!
The approach in 2. looks good so I went with that in 80e9ef1.

unasuke · 2023-07-01T11:59:28Z

as these methods are needed to implement protocols that rely on X25519/X448 raw private/public key support (such as Hybrid Public Key Encryption and QUIC).

Yes, QUIC (or rather TLS1.3) SHOULD support key exchange with X25519.

https://www.rfc-editor.org/rfc/rfc8446.html#section-9.1

This patch helps people who want to implement TLS 1.3. This is what I wanted.

rhenium · 2023-07-01T12:13:34Z

Thank you for working on this!

I removed the private? / public? methods and marshalling support that relies on these methods, as these methods are causing old versions of OpenSSL to not work.

Sounds reasonable. It is a difficult task to support marshaling OpenSSL::PKey::PKey because it can be wrapping a wide variety of EVP_PKEYs. There is no convenient way to tell whether an EVP_PKEY represents a private key, a public key, or just key parameters, either. #527 is my abandoned effort to define #private? and #public? on OpenSSL::PKey::PKey.

rhenium · 2023-07-01T12:18:11Z

I find the method naming confusing. In my understanding, "raw {private,public} key" is an OpenSSL term to mean the keys specifically used by X25519/Ed25519 family (at least currently), and these new methods are not supposed to work for other PKey types, such as RSA. I want to see the method names reflect it.

IMHO, using OpenSSL's function names (with "EVP_PKEY_" and "EVP_PKEY_get_" stripped) just fits here:

OpenSSL::PKey.new_raw_private_key
OpenSSL::PKey.new_raw_public_key
OpenSSL::PKey::PKey#raw_private_key
OpenSSL::PKey::PKey#raw_public_key

What do you think?

rhenium · 2023-07-01T12:18:39Z

/cc @bdewater

rhenium · 2023-07-01T12:20:24Z

ext/openssl/ossl_pkey.c

+ * See the OpenSSL documentation for EVP_PKEY_new_raw_private_key()
+ */
+
+#ifdef HAVE_EVP_PKEY_NEW_RAW_PRIVATE_KEY


I think #ifdef needs to be moved above the doc comment so that rdoc can parse it correctly.

Fixed @ 202cac7 .
Maybe the first and second #ifdef blocks can be merged into one, but I thought having it separated would make it easier to understand that each method needs this condition to be met.

rhenium · 2023-07-01T12:22:09Z

ext/openssl/ossl_pkey.c

+
+    pkey = EVP_PKEY_new_raw_private_key(pkey_id, NULL, (unsigned char *)RSTRING_PTR(key), keylen);
+    if (!pkey)
+        ossl_raise(ePKeyError, "Could not parse PKey");


The error message should be adjusted because parsing isn't happening here (perhaps just "EVP_PKEY_new_raw_private_key" would work).

Fixed @ e9ccf58

ext/openssl/ossl_pkey.c

rhenium · 2023-07-01T12:23:55Z

ext/openssl/ossl_pkey.c

+    size_t len;
+
+    GetPKey(self, pkey);
+    EVP_PKEY_get_raw_private_key(pkey, NULL, &len);


This probably needs an error check.

Fixed @ 6628df1.
Maybe we could have a different error message for this call and the next call, but it could be difficult to tell what exactly caused the error on each call so I think leaving as it is now would be reasonable.

rhenium · 2023-07-01T12:24:23Z

ext/openssl/ossl_pkey.c

+ */
+
+#ifdef HAVE_EVP_PKEY_NEW_RAW_PRIVATE_KEY
+static VALUE ossl_pkey_private_to_raw(VALUE self)


Style nit: please insert a newline after VALUE.

Fixed @ 26bd41d

rhenium · 2023-07-01T12:25:06Z

lib/openssl/pkey.rb

+      OpenSSL::PKey.read(string)
+    end
+  end
+


Should this be removed too, as this would be unreachable now?

Fixed @ 4d67841

sylph01 · 2023-07-07T08:25:53Z

OpenSSL::PKey.new_raw_private_key
OpenSSL::PKey.new_raw_public_key
OpenSSL::PKey::PKey#raw_private_key
OpenSSL::PKey::PKey#raw_public_key

Yes, I think these names would convey the intention that these aren't supposed to be used except for algorithms that support "raw public/private keys".

The fixes on the code are as below:

The renaming is done @ edadf2f
Documentation fix wasn't complete on the last commit so did that on f721fd0
Then renamed C functions to match its method name @ 18623dd

rhenium

I added some minor comments, but otherwise it looks good to me!

rhenium · 2023-07-08T06:56:11Z

ext/openssl/ossl_pkey.c

+static VALUE
+ossl_pkey_new_raw_private_key(VALUE self, VALUE type, VALUE key)
+{
+


Please remove this empty line.

rhenium · 2023-07-08T06:56:23Z

ext/openssl/ossl_pkey.c

+    pkey = EVP_PKEY_new_raw_private_key(pkey_id, NULL, (unsigned char *)RSTRING_PTR(key), keylen);
+    if (!pkey)
+        ossl_raise(ePKeyError, "EVP_PKEY_new_raw_private_key");
+


This line has trailing spaces.

rhenium · 2023-07-08T07:02:42Z

ext/openssl/ossl_pkey.c

+
+/*
+ *  call-seq:
+ *      pkey.sign(digest, data) -> String


This line looks like a copy-and-paste error.

rhenium · 2023-07-08T07:04:44Z

ext/openssl/ossl_pkey.c

    rb_define_method(cPKey, "public_to_pem", ossl_pkey_public_to_pem, 0);
+#ifdef HAVE_EVP_PKEY_NEW_RAW_PRIVATE_KEY
+    rb_define_method(cPKey, "raw_public_key", ossl_pkey_raw_public_key, 0);
+#endif


Can we group the #raw_*_key methods together?

rhenium · 2023-07-08T07:06:49Z

ext/openssl/ossl_pkey.c

+ *  See the OpenSSL documentation for EVP_PKEY_get_raw_public_key()
+ */
+
+static VALUE ossl_pkey_raw_public_key(VALUE self)


Please insert a newline before the function name.

- Remove trailing whitespaces - Remove extra line break - Styling of function definition - Remove wrong documentation - Group raw_*_key methods together

sylph01 · 2023-07-08T08:06:30Z

Applied styling fixes @ c40d4fe
Minor documentation fix @ 23262c2, where I fixed the documentation to say pkey instead of key as receiver object to match other occurences.

rhenium · 2023-07-12T13:12:51Z

I've merged this to master now. Thank you!

(ruby/openssl#646) Add OpenSSL::PKey.new_raw_private_key, #raw_private_key and public equivalents. These methods are useful for importing and exporting keys that support "raw private/public key". Currently, OpenSSL implements X25519/X448 and Ed25519/Ed448 keys. [rhe: rewrote commit message] ruby/openssl@3f29525618 Co-authored-by: Bart de Water <bartdewater@gmail.com>

bdewater · 2023-07-12T18:40:05Z

Thank you both for getting this over the finish line!

Add OpenSSL::PKey.new_raw_private_key, #raw_private_key and public equivalents. These methods are useful for importing and exporting keys that support "raw private/public key". Currently, OpenSSL implements X25519/X448 and Ed25519/Ed448 keys. [rhe: rewrote commit message] Co-authored-by: Bart de Water <bartdewater@gmail.com>

bdewater and others added 7 commits May 21, 2020 00:05

Add private?/public? methods and Marshal support to OpenSSL::PKey::PKey

717fe3f

Add OpenSSL::PKey.private_new, #private_to_raw and public equivalents

8f7467f

Merge branch 'master' into pkey-convenience-methods

9cf27ac

Temporarily remove private?/public? methods

7b599ca

Temporarily remove dumping

4e1da3d

Rewrite OBJ_sn2nid() using EVP_PKEY_asn1_find_str()

8e89a27

Check if running OpenSSL's EVP has raw private key support

46d4fc7

sorah reviewed Jul 1, 2023

View reviewed changes

Remove method definition when OpenSSL does not support raw pkeys

80e9ef1

rhenium reviewed Jul 1, 2023

View reviewed changes

sylph01 added 9 commits July 7, 2023 14:33

Move #ifdev above doc comment

202cac7

Ensure key is a String

b48fcab

Fix error message to match actual operation

e9ccf58

Styling fix

26bd41d

Error check on EVP_PKEY_get_raw_private/public_key

6628df1

Remove loading support as this might result in unreachable code

4d67841

Rename methods to match OpenSSL function names

edadf2f

Fix documentation

f721fd0

Rename C function names

18623dd

rhenium reviewed Jul 8, 2023

View reviewed changes

sylph01 added 2 commits July 8, 2023 17:01

Styling fixes

c40d4fe

- Remove trailing whitespaces - Remove extra line break - Styling of function definition - Remove wrong documentation - Group raw_*_key methods together

Doc: key -> pkey

23262c2

rhenium approved these changes Jul 12, 2023

View reviewed changes

rhenium merged commit 3f29525 into ruby:master Jul 12, 2023

rhenium mentioned this pull request Jul 12, 2023

OpenSSL::PKey convenience methods #373

Closed

Conversation

sylph01 commented Jul 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unasuke commented Jul 1, 2023

Uh oh!

rhenium commented Jul 1, 2023

Uh oh!

rhenium commented Jul 1, 2023

Uh oh!

rhenium commented Jul 1, 2023

Uh oh!

rhenium Jul 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylph01 commented Jul 7, 2023

Uh oh!

rhenium left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylph01 commented Jul 8, 2023

Uh oh!

rhenium commented Jul 12, 2023

Uh oh!

bdewater commented Jul 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rhenium Jul 1, 2023 •

edited

Loading