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.

Sign up for GitHub

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

Jump to bottom

Support signing requests and CRLs using ED25519 #804

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support signing requests and CRLs using ED25519 #804

wants to merge 3 commits into from

Conversation

joshcooper
Copy link

@joshcooper joshcooper commented Oct 8, 2024 •
edited
Loading

Allow requests and CRLs to be signed using Ed25519 private keys by passing a nil digest. This is similar to commit b0fc100 when signing certs.

Note Ed25519 keys do not implement the same public_key method, so the test must special case RSA and DSA.

@joshcooper joshcooper mentioned this pull request Oct 8, 2024
Closed
rhenium
rhenium reviewed Oct 29, 2024
View reviewed changes
Copy link
Member

@rhenium rhenium left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

I think the same change can be applied to OpenSSL::X509::CRL. Could you update it as well?

test/openssl/test_x509req.rb Outdated Show resolved Hide resolved
@rhenium
Copy link
Member

rhenium commented Oct 29, 2024 

This is similar to commit f463f5620583a927653772ae7cee95736a963a55 when signing certs.

This commit doesn't belong to ruby/openssl. I think you meant b0fc100091207d7eab20a349433ccbd8260c6ddd.

@joshcooper joshcooper changed the title Support signing requests using ED25519 Support signing requests and CRLs using ED25519 Oct 29, 2024
@joshcooper joshcooper force-pushed the ed25519_req branch 2 times, most recently from 21fead2 to 3103d90 Compare October 30, 2024 18:43
@joshcooper
Copy link
Author

Th pkey oid for Ed25519 has different cases depending on the ssl library, so I switched to casecmp? instead

openssl:

OpenSSL::PKey::generate_key("ED25519").public_key
(irb):2:in `<main>': undefined method `public_key' for #<OpenSSL::PKey::PKey:0x00007f553184da90 oid=ED25519>

libressl:

NoMethodError: undefined method `public_key' for #<OpenSSL::PKey::PKey:0x000055ec67641d48 oid=Ed25519>

rhenium
rhenium reviewed Oct 31, 2024
View reviewed changes
test/openssl/utils.rb Outdated Show resolved Hide resolved
rhenium
rhenium reviewed Nov 6, 2024
View reviewed changes
test/openssl/test_pkey.rb Outdated
@@ -90,6 +90,7 @@ def test_hmac_sign_verify
def test_ed25519
# Ed25519 is not FIPS-approved.
omit_on_fips
omit "Ed25519 not supported" unless openssl?(1, 1, 1)
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
omit "Ed25519 not supported" unless openssl?(1, 1, 1)
omit "Ed25519 not supported" unless openssl?(1, 1, 1) || libressl?(3, 7, 0)

Non-ASN1_item_sign() Ed25519 usage is supported in LibreSSL >= 3.7.0.

Copy link
Author

@joshcooper joshcooper Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I had a question about that. The original code didn't guard against that, but it makes sense for it to be consistent across test_x509* tests. I'll update it.

Copy link
Author

Choose a reason for hiding this comment

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

Looking at https://github.com/libressl/portable/blob/1a3e756a757da607621949b43f97fb79b6fb31ae/ChangeLog#L352 did you mean libressl?(3, 8, 1)?

Copy link
Member

Choose a reason for hiding this comment

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

That changelog entry is for ASN1_item_sign(). Ed25519 in EVP_PKEY_sign() was added in 3.7.0. https://github.com/libressl/portable/blob/1a3e756a757da607621949b43f97fb79b6fb31ae/ChangeLog#L591-L592

@joshcooper
Check for compatible openssl versions earlier
b4f8aab 
test_pkey wasn't checking for libressl as is done elsewhere.
@joshcooper
Support signing requests using Ed25519
ade53f2 
Allow requests to be signed using Ed25519 private keys by passing a nil digest.
This is similar to commit b0fc100 when signing certs.

Calling PKey#public_key is deprecated and does not work for Ed25519. The same
can be accomplished by passing the private key.
@joshcooper
Support signing CRLs using Ed25519
79100f5 
Allow CRLs to be signed using Ed25519 private keys by passing a nil digest.
@joshcooper
Copy link
Author

joshcooper commented Nov 7, 2024 •
edited
Loading

It seems libressl behaves differently when calling csr.public_key = key and then retrieving the public key:

https://github.com/ruby/openssl/actions/runs/11711737721/job/32645324677?pr=804#step:10:694

And openssl 1.0.2u:

https://github.com/ruby/openssl/actions/runs/11711737721/job/32645317144?pr=804#step:10:828

@rhenium
Copy link
Member

rhenium commented Nov 12, 2024

It seems libressl behaves differently when calling csr.public_key = key and then retrieving the public key:

https://github.com/ruby/openssl/actions/runs/11711737721/job/32645324677?pr=804#step:10:694

And openssl 1.0.2u:

https://github.com/ruby/openssl/actions/runs/11711737721/job/32645317144?pr=804#step:10:828

This commit that went to OpenSSL 1.1.0 seems relevant: openssl/openssl@fa0a9d7. I guess my new assertion in test_public_key was too much into the implementation detail. Does something like this work?

assert_equal(@rsa1024.public_to_der, req.public_key.public_to_der)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhenium rhenium rhenium left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joshcooper @rhenium