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

Add support for Ed25519 #3

Merged
merged 1 commit into from
Feb 11, 2021
Merged

Add support for Ed25519 #3

merged 1 commit into from
Feb 11, 2021

Conversation

santiagorodriguez96
Copy link
Contributor

@santiagorodriguez96 santiagorodriguez96 commented Aug 10, 2020

What

This PR attempts to close #2.

How

Used the gem RubyCrypto/ed25519 as we still require ruby/openssl to expose the required OpenSSL APIs in Ruby, expected to be released as 3.0 – See ruby/openssl#329.
Once OpenSSL Ruby v3.0 is released, we could change the implementation in a way that users that use this gem alongside that version of OpenSSL will use the API provided by OpenSSL, but users with an older version of OpenSSL will still be provided with Ed25519 support with the use of RubyCrypto/ed25519.

@santiagorodriguez96 santiagorodriguez96 changed the title Add support for Ed25519 credentials Add support for Ed25519 Aug 10, 2020
@santiagorodriguez96 santiagorodriguez96 force-pushed the add-ed25519-support branch 2 times, most recently from a098690 to 8a21069 Compare August 10, 2020 18:05
@santiagorodriguez96 santiagorodriguez96 marked this pull request as ready for review August 10, 2020 18:16
Copy link
Contributor

@grzuy grzuy left a comment

Choose a reason for hiding this comment

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

Nice work!


FYI, Travis build is red.

@@ -0,0 +1,57 @@
# frozen_string_literal: true

require "ed25519"
Copy link
Contributor

Choose a reason for hiding this comment

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

Given we are not including this as a runtime dependency in the gemspec file (which I think is fine), users need to explicitly include this in their Gemfile for EdDSA to work.

We can do something a tiny bit more sophisticated to give the user a more helpful error message here if the gem not present. A good example is what activesupport does for the cache backends. See e.g. RedisCacheStore https://github.com/rails/rails/blob/fbe2433be6e052a1acac63c7faf287c52ed3c5ba/activesupport/lib/active_support/cache/redis_cache_store.rb#L3-L10.

module OpenSSL
module SignatureAlgorithm
class EdDSA < Base
class Ed25519
Copy link
Contributor

Choose a reason for hiding this comment

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

In the same vein as the curve argument comment, we can remove this class from the namespace for now.

@@ -9,7 +9,7 @@ Gem::Specification.new do |spec|
spec.email = ["[email protected]"]
spec.license = "Apache-2.0"

spec.summary = "ECDSA, RSA-PSS and RSA-PKCS#1 algorithms for ruby"
spec.summary = "ECDSA, EdDSA, RSA-PSS and RSA-PKCS#1 algorithms for ruby"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

# frozen_string_literal: true

begin
gem "ed25519", ">= 1.0.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grzuy grzuy force-pushed the add-ed25519-support branch 3 times, most recently from d88f9a8 to 341c965 Compare February 10, 2021 21:57
This commit attempts to close #2.

Used the gem RubyCrypto/ed25519 as we still require ruby/openssl to
expose the required OpenSSL APIs in Ruby, expected to be released as 3.0
– See ruby/openssl#329.
Once OpenSSL Ruby v3.0 is released, we could change the implementation
in a way that users that use this gem alongside that version of OpenSSL
will use the API provided by OpenSSL, but users with an older version of
OpenSSL will still be provided with Ed25519 support with the use of
RubyCrypto/ed25519
@grzuy grzuy force-pushed the add-ed25519-support branch from 341c965 to 2338eaf Compare February 10, 2021 21:59
@grzuy grzuy merged commit 15879ed into master Feb 11, 2021
@grzuy grzuy deleted the add-ed25519-support branch February 11, 2021 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide EdDSA
2 participants