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

Merged
merged 3 commits into from
Nov 22, 2024
Merged

Support signing requests and CRLs using ED25519 #804

merged 3 commits into from
Nov 22, 2024

Conversation

joshcooper
Copy link
Contributor

@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
Contributor 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 Show resolved Hide resolved
@joshcooper joshcooper force-pushed the ed25519_req branch 3 times, most recently from 303477f to 79100f5 Compare November 6, 2024 20:56
@joshcooper
Copy link
Contributor 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)

@joshcooper
Check for compatible openssl versions earlier
f1db5c8 
test_pkey wasn't checking for libressl as is done elsewhere.

Note the libressl version check is different when testing pkey, because
PKey#sign relies on EVP_PKey_sign, whereas signing an X509 cert/request/crl
relies on ASN1_item_sign.
@joshcooper
Support signing requests using Ed25519
d960903 
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
b62375b 
Allow CRLs to be signed using Ed25519 private keys by passing a nil digest.
@joshcooper
Copy link
Contributor Author

Thanks for your help @rhenium, all tests are passing now.

rhenium
rhenium approved these changes Nov 22, 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.

Looks good to me. Thank you for the PR!

@rhenium rhenium merged commit 0759018 into ruby:master Nov 22, 2024
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhenium rhenium rhenium approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

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