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

Support Ed25519 keys/certificates #8869

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

tambry
Copy link
Contributor

@tambry tambry commented Jan 30, 2022

The generic interface usage was added by 78712fe, which improved key format support.
ruby-openssl 3.0, shipped in Ruby 3.0, supports Ed25519 keys using the generic interface and returns a OpenSSL::PKey::PKey.
The only thing preventing these from working is a simple type check. Remove it and update various type annotations to refer to OpenSSL::PKey::PKey, though the old types will still work.

@tambry tambry requested a review from a team as a code owner January 30, 2022 18:01
@tambry tambry requested a review from a team January 30, 2022 18:01
@joshcooper
Copy link
Contributor

Hi @tambry thanks for you contribution! I think we'll need a few more changes to merge this:

  1. Can you update the rake task for generating cert & key fixtures to generate an ED25519 private key and signed cert? It seems fine to reuse the test vectors from ruby or generate new private/pub keys, create a CSR and issue the cert using the intermediate CA's private key.
  2. Run bundle exec rake gen_cert_fixtures to regenerate all of the fixtures, including the new key and cert. Best to create one commit for just those changes.
  3. Next add unit tests for the new key, like we do for RSA and EC keys. You will probably need to do something like context 'using EC25519', if: RUBY_VERSION.to_f >= 3 && OpenSSL::OPENSSL_VERSION_NUMBER > 0x10101000 do so that the tests only run on Ruby and openssl versions >= 1.1.1.
  4. This PR will now accept a bogus object as the private key, since we're now just checking that it's not an instance of DSA https://github.com/puppetlabs/puppet/pull/8869/files#diff-267828e88cef126d517addbead183553921e62648a51c9befa85dec6587357dbR129

I think it'd be better to only allow key types that we know work. The trouble is the OpenSSL::PKey::PKey#oid method doesn't exist in older ruby versions, so it'll need to be conditional. Maybe something like:

if !private_key.is_a?(OpenSSL::PKey::RSA) && \
   !private_key.is_a?(OpenSSL::PKey::EC) && \
   !(private_key.is_a?(OpenSSL::PKey::PKey) && private_key.respond_to?(:oid) && private_key.oid == 'ED25519')
  raise ...

@CLAassistant
Copy link

CLAassistant commented Apr 19, 2023

CLA assistant check
All committers have signed the CLA.

@joshcooper joshcooper added the enhancement New feature or request label Nov 15, 2023
@joshcooper
Copy link
Contributor

Hi @tambry changes look great! Could you rebase on main and resolve the ssl_provider conflict?

@tambry
Copy link
Contributor Author

tambry commented Nov 29, 2023

@joshcooper Sorry for the delay. I'll try to have a look at this within a few weeks.
We've been using this downstream for close to 2 years by now so it'd be lovely to get this upstreamed.

Last time I got hung up on updating the tests. I began, but getting a Ruby development environment setup proved a bit annoying as I couldn't simply grab the dependencies from my system package manager. I was also worried about breaking my whole Debian install so I began creating a separate rootfs, investigating how to use schroot with it and... then I dropped it. 😆

@joshcooper
Copy link
Contributor

@tambry looks like there are some conflicts now. Could you rebase on main?

The generic interface usage was added by 78712fe, which improved key format support.
ruby-openssl 3.0, shipped in Ruby 3.0, supports Ed25519 keys using the generic interface and returns a OpenSSL::PKey::PKey.
The only thing preventing these from working is a simple type check. Update it to only check key types that aren't supported.
@tambry
Copy link
Contributor Author

tambry commented Mar 13, 2024

@joshcooper Thanks for the reminder, I've rebased. Note that the tests remain unfinished and are probably insufficient.

@joshcooper
Copy link
Contributor

joshcooper commented Apr 11, 2024

@tambry The ed25519 fixtures are missing, so tests are failing. Could you run bundle exec rake gen_cert_fixtures and add all of the updated cert/key fixtures? Also please include those update fixtures in this commit so it doesn't confuse git bisect later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants