Add support to build and sign certificates with multiple key algorithms #4991

doodzik · 2021-10-16T00:21:56Z

What was the end-user or developer problem that led to this PR?

The RubyGems client is currently hard-coded to build and sign certificates with private keys as OpenSSL::PKey::RSA instances, which prevents the usage from non-rsa certifcates. Adding support for different key algorithms has been on the TODO list of Rubygems' Security module for a long time.

What is your fix for the problem implemented in this PR?

This PR approaches the problem by swapping out the hardcoded OpenSSL::PKey::RSA constructor for the OpenSSL::PKey factory method, which can take an RSA/DSA/EC a key and instantiate the correct object.

The user can choose which algorithm to use to build a new certificate through a --key-algorithm ALGORITHM flag.

We ran into issues when using different OpenSSL::PKey subclasses because their interface wasn't consistent. Specifically how a private key gets retrieved and the lack of the to_s method for EC keys.

Make sure the following tasks are checked

Describe the problem / feature
Write tests for features and bug fixes
Write code to solve the problem
Make sure you follow the current code style and write meaningful commit messages without tags

@jennyshen has been the co-author of this PR.

doodzik · 2021-10-16T00:27:45Z

lib/rubygems/security/policy.rb


-    pkey_str = root.public_key.to_s
+    pkey_str = root.public_key.to_pem


EC keys don't have to_s method like RSA.

doodzik · 2021-10-16T00:30:02Z

lib/rubygems/security.rb


-  KEY_LENGTH = 3072
+  EC_NAME = 'secp384r1'


We choose this particular curve as the default because it is reasonably secure and widely adopted.

rochlefebvre · 2021-10-18T13:26:42Z

lib/rubygems/security.rb

+  def self.get_public_key(key)
+    return key.public_key unless key.is_a?(OpenSSL::PKey::EC)
+
+    ec_key = OpenSSL::PKey::EC.new(EC_NAME)


I generated my own EC key file with a different curve, then tried to build a certificate on the constant_resolver gem using my own key:

➜ constant_resolver git:(master) ✗ ruby -v ruby 2.6.2p47 (2019-03-13 revision 67232) [x86_64-darwin20] ➜ constant_resolver git:(master) ✗ ls ~/.gem ➜ constant_resolver git:(master) ✗ openssl ecparam -name prime256v1 -genkey -out ~/.gem/ec_prime256v1_key.pem ➜ constant_resolver git:(master) ✗ gem cert --build roch.lefebvre@shopify.com -K ~/.gem/ec_prime256v1_key.pem ERROR: While executing gem ... (OptionParser::InvalidArgument) invalid argument: -K /Users/rochlefebvre/.gem/ec_prime256v1_key.pem: invalid RSA key

So far, so good. My stock rubygems install doesn't accept EC keys. I'll try the same with this PR's branch:

➜ constant_resolver git:(master) ✗ ruby -I../../jenshenny/rubygems/lib ../../jenshenny/rubygems/bin/gem cert --build roch.lefebvre@shopify.com -K ~/.gem/ec_prime256v1_key.pem ERROR: While executing gem ... (OpenSSL::PKey::ECError) EC_KEY_set_public_key: incompatible objects ➜ constant_resolver git:(master) ✗ ruby -help Usage: ruby [switches] [--] [programfile] [arguments] -0[octal] specify record separator (\0, if no argument) -a autosplit mode with -n or -p (splits $_ into $F) -c check syntax only -Cdirectory cd to directory before executing your script -d set debugging flags (set $DEBUG to true) [...] ➜ constant_resolver git:(master) ✗ ruby -d -I../../jenshenny/rubygems/lib ../../jenshenny/rubygems/bin/gem cert --build roch.lefebvre@shopify.com -K ~/.gem/ec_prime256v1_key.pem [...] Exception `OpenSSL::PKey::ECError' at /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/security.rb:425 - EC_KEY_set_public_key: incompatible objects ERROR: While executing gem ... (OpenSSL::PKey::ECError) EC_KEY_set_public_key: incompatible objects /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/security.rb:425:in `public_key=' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/security.rb:425:in `get_public_key' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/security.rb:400:in `create_cert' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/security.rb:448:in `create_cert_self_signed' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/security.rb:439:in `create_cert_email' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/commands/cert_command.rb:157:in `build_cert' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/commands/cert_command.rb:143:in `build' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/commands/cert_command.rb:123:in `block in execute' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/commands/cert_command.rb:122:in `each' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/commands/cert_command.rb:122:in `execute' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/command.rb:323:in `invoke_with_build_args' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/command_manager.rb:178:in `process_args' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/command_manager.rb:147:in `run' /Users/rochlefebvre/src/github.com/jenshenny/rubygems/lib/rubygems/gem_runner.rb:53:in `run' ../../jenshenny/rubygems/bin/gem:13:in `<main>'

If I go through the same test while using a self-generated secp384r1 key, then everything works.

I think we may be able to instantiate an EC key instance with the same curve name as the given private key. After changing this line, my test worked:

ec_key = OpenSSL::PKey::EC.new(key.group.curve_name)

Good catch!

rochlefebvre · 2021-10-18T13:29:55Z

lib/rubygems/security.rb

@@ -462,8 +470,21 @@ def self.create_digest(algorithm = DIGEST_NAME)
  # Creates a new key pair of the specified +length+ and +algorithm+.  The


If we decide to rework this method, then we'll need new documentation.

rochlefebvre · 2021-10-18T13:36:38Z

lib/rubygems/security.rb

@@ -462,8 +470,21 @@ def self.create_digest(algorithm = DIGEST_NAME)
  # Creates a new key pair of the specified +length+ and +algorithm+.  The
  # default is a 3072 bit RSA key.

-  def self.create_key(length = KEY_LENGTH, algorithm = KEY_ALGORITHM)
-    algorithm.new length
+  def self.create_key(algorithm)


We're changing a public helper method's signature, which may be a no-no.

I did a quick search on GitHub (not exhaustive, since not everything is indexed). Virtually all of the ~1500 hits are from cert_command.rb in vendored snapshots of Rubygems or Ruby. If I exclude cert_command.rb from my search, I get an insignificant set of hits.

If an incompatible change is unacceptable, consider creating a new method, e.g. create_private_key.

jchestershopify

A few nits. One question I still have: I see we added tests for EC certs, do we exercise RSA and DSA certs as well?

jchestershopify · 2021-10-18T15:14:06Z

lib/rubygems/commands/cert_command.rb

-  rescue OpenSSL::PKey::RSAError
-    raise OptionParser::InvalidArgument, "#{key_file}: invalid RSA key"
+  rescue OpenSSL::PKey::PKeyError
+    raise OptionParser::InvalidArgument, "#{key_file}: invalid RSA/DSA/EC key"


Nit: I'd suggest the text be "invalid RSA, DSA or EC key".

jchestershopify · 2021-10-18T15:15:31Z

lib/rubygems/commands/cert_command.rb

@@ -170,7 +175,8 @@ def build_key # :nodoc:
    raise Gem::CommandLineError,
          "Passphrase and passphrase confirmation don't match" unless passphrase == passphrase_confirmation

-    key      = Gem::Security.create_key
+    algorithm = options[:key_algorithm] || 'RSA'


Nit: 'RSA' could be a constant.

jchestershopify · 2021-10-18T15:18:34Z

lib/rubygems/security.rb

@@ -152,6 +152,7 @@
 #                                      certificate for EMAIL_ADDR
 #     -C, --certificate CERT           Signing certificate for --sign
 #     -K, --private-key KEY            Key for --sign or --build
+#     --key-algorithm ALGORITHM        Select which key algorithm to use for --build


Seems to be missing the -A option which was added via add_option in cert_command.rb.

I'd also suggest that you give the alternatives and default in the help text. So:

Select key algorithm for --build from RSA, DSA or EC. Defaults to RSA.

jchestershopify · 2021-10-18T15:20:35Z

lib/rubygems/security.rb

@@ -337,17 +337,14 @@ class Exception < Gem::Exception; end
  DIGEST_NAME = 'SHA256' # :nodoc:

  ##
-  # Algorithm for creating the key pair used to sign gems
+  # Length of keys created by KEY_ALGORITHM


Does KEY_ALGORITHM still exist after this PR is applied? I think it refers to RSA, so the comment might read
Length of RSA keys. The constant might be renamed from KEY_LENGTH to RSA_KEY_LENGTH.

Though I see below we use KEY_LENGTH for DSA as well. So perhaps RSA_DSA_KEY_LENGTH. Mostly I'm concerned that folks can see it's clearly for one set of options and is unrelated to the EC curve.

jchestershopify · 2021-10-18T15:22:56Z

lib/rubygems/security.rb

+        domain_key
+      else
+        raise Gem::Security::Exception,
+        "#{algorithm} algorithm not found. RSA/DSA/EC algorithms are supported."


Nit: suggest "RSA, DSA and EC algorithms are supported".

doodzik · 2021-10-18T20:37:48Z

lib/rubygems/commands/cert_command.rb

    key
+  rescue ArgumentError
+    raise OptionParser::InvalidArgument,
+      "#{key_file}: private key not found"


Ruby 2.3.8 raises an argument error when encountering an empty file for PKey.read, whereas newer ruby versions don't.

We also had to add a custom private key check, because ruby 2.3 doesn't implement PKey::EC#private?.

It might be worthwhile dropping support for ruby 2.3 since it adds some complexity in this PR and has been discussed in the past.

jchestershopify · 2021-10-19T16:57:49Z

lib/rubygems/security.rb

+
+  ##
+  # Checks if the provided key is private.
+  # In Ruby 2.3 EC doesn't implments the private_key? but not the private? method


Nit: "doesn't implements" -> "doesn't implement".

Co-Authored-By: Frederik Dudzik <frederik.dudzik@shopify.com>

jchestershopify · 2021-10-20T18:56:05Z

lib/rubygems/security.rb

@@ -317,7 +318,6 @@
 # * Honor extension restrictions
 # * Might be better to store the certificate chain as a PKCS#7 or PKCS#12
 #   file, instead of an array embedded in the metadata.
-# * Flexible signature and key algorithms, not hard-coded to RSA and SHA1.


jchestershopify · 2021-10-21T14:55:51Z

/ping @simi @deivid-rodriguez

Can we help to move this in some way?

deivid-rodriguez

Hey! I don't really know this area very well, and I've heard other maintainers feel skeptical about the security guarantees our current gem signing system provides. For example, see this answer by @indirect: #4682 (comment). That said, you seem the closest to security experts we can get right now, and I don't see how this PR could made anything worse, so I'm not blocking this, and I'm ready to approve it!

jchestershopify · 2021-10-21T15:53:07Z

For context, our team (@rochlefebvre, @jenshenny and @doodzik + myself currently) are interested in improving or even renovating the gem signing system. This is one low hanging fruit we could identify to help get ourselves up to speed with the current codebase.

deivid-rodriguez · 2021-10-21T15:59:42Z

I will merge this in a couple of days if there is no opposition.

Add support to build and sign certificates with multiple key algorithms (cherry picked from commit 9892d1e)

bundlerbot added the RubyGems label Oct 16, 2021

doodzik commented Oct 16, 2021

View reviewed changes

jenshenny force-pushed the multiple-pkey-algorithms branch from e3d46d8 to 79bebfd Compare October 18, 2021 12:55

rochlefebvre reviewed Oct 18, 2021

View reviewed changes

jenshenny force-pushed the multiple-pkey-algorithms branch 3 times, most recently from 0c7eb55 to 85928f7 Compare October 18, 2021 14:55

jchestershopify reviewed Oct 18, 2021

View reviewed changes

jenshenny force-pushed the multiple-pkey-algorithms branch 2 times, most recently from 2d96121 to bf48b1a Compare October 18, 2021 17:18

doodzik force-pushed the multiple-pkey-algorithms branch from bf48b1a to 2e89bcc Compare October 18, 2021 20:18

doodzik commented Oct 18, 2021

View reviewed changes

jchestershopify reviewed Oct 19, 2021

View reviewed changes

doodzik force-pushed the multiple-pkey-algorithms branch from 2e89bcc to 4f4c617 Compare October 19, 2021 19:10

Add support to build and sign certificates with multiple key algorithms

967876f

Co-Authored-By: Frederik Dudzik <frederik.dudzik@shopify.com>

doodzik force-pushed the multiple-pkey-algorithms branch from 4f4c617 to 967876f Compare October 19, 2021 19:15

jchestershopify reviewed Oct 20, 2021

View reviewed changes

deivid-rodriguez approved these changes Oct 21, 2021

View reviewed changes

deivid-rodriguez added the rubygems: enhancement label Oct 25, 2021

deivid-rodriguez merged commit 9892d1e into rubygems:master Oct 25, 2021

deivid-rodriguez added a commit that referenced this pull request Oct 25, 2021

Merge pull request #4991 from jenshenny/multiple-pkey-algorithms

ffcc625

Add support to build and sign certificates with multiple key algorithms (cherry picked from commit 9892d1e)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to build and sign certificates with multiple key algorithms #4991

Add support to build and sign certificates with multiple key algorithms #4991

doodzik commented Oct 16, 2021 •

edited

doodzik Oct 16, 2021

doodzik Oct 16, 2021

rochlefebvre Oct 18, 2021 •

edited

jenshenny Oct 18, 2021

rochlefebvre Oct 18, 2021

rochlefebvre Oct 18, 2021

jchestershopify left a comment

jchestershopify Oct 18, 2021

jchestershopify Oct 18, 2021

jchestershopify Oct 18, 2021

jchestershopify Oct 18, 2021

jchestershopify Oct 18, 2021

jchestershopify Oct 18, 2021

doodzik Oct 18, 2021

jchestershopify Oct 19, 2021

jchestershopify Oct 20, 2021

jchestershopify commented Oct 21, 2021

deivid-rodriguez left a comment •

edited

jchestershopify commented Oct 21, 2021

deivid-rodriguez commented Oct 21, 2021


		pkey_str = root.public_key.to_s
		pkey_str = root.public_key.to_pem

		@@ -462,8 +470,21 @@ def self.create_digest(algorithm = DIGEST_NAME)
		# Creates a new key pair of the specified +length+ and +algorithm+. The

Add support to build and sign certificates with multiple key algorithms #4991

Add support to build and sign certificates with multiple key algorithms #4991

Conversation

doodzik commented Oct 16, 2021 • edited

What was the end-user or developer problem that led to this PR?

What is your fix for the problem implemented in this PR?

Make sure the following tasks are checked

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rochlefebvre Oct 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchestershopify left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchestershopify commented Oct 21, 2021

deivid-rodriguez left a comment • edited

Choose a reason for hiding this comment

jchestershopify commented Oct 21, 2021

deivid-rodriguez commented Oct 21, 2021

doodzik commented Oct 16, 2021 •

edited

rochlefebvre Oct 18, 2021 •

edited

deivid-rodriguez left a comment •

edited