Add --re-sign flag to cert command #2391

bronzdoc · 2018-08-31T04:55:20Z

Description:

Follow up of: #1720

(i'll squash the commits when i'm done)

I will abide by the code of conduct.

djberg96 · 2018-08-31T15:38:57Z

👍 Thanks!

djberg96 · 2018-09-04T16:54:16Z

@bronzdoc The failures look like a bundler (?) issue. Maybe restart the job?

bronzdoc · 2018-09-04T17:03:28Z

@djberg96 nope not a bundler issue, is just i'm asserting a linux path and in windows is not the same 😁 already fixed that locally, just finishing up some things, it will be ready today :)

djberg96 · 2018-09-05T19:36:46Z

@bronzdoc Looks green, thanks!

@segiddins Ok to merge?

colby-swandale · 2018-09-07T05:58:06Z

lib/rubygems/commands/cert_command.rb

@@ -89,6 +93,11 @@ def initialize
               'Days before the certificate expires') do |days, options|
      options[:expiration_length_days] = days.to_i
    end
+
+    add_option('-R', '--re-sign',
+               'Re-sign the certificate from -C with the key from -K') do |resign, options|


It would be good to make it clear to the user that -C and -K are options.

"Re-sign the certificate specified by the --certificate option with the key specified by the --private-key option"

How does that sound?

This is the description of the --sign flag, i just wanted to make it familiar with what users already have.

add_option('-s', '--sign CERT', 'Signs CERT with the key from -K', 'and the certificate from -C') do |cert_file, options|

Ok, so let's use the same message, just changing "Signs" to "Re-signs". I guess '--re-sign' should be changed to '--re-sign CERT' then, too.

The command will be used like:

gem cert --certificate ~/cert.pem --private-key ~/private_key.pem --re-sign

So the --re-sign flag does not take any CERT argument

Ah, nevermind that comment then. 😄

colby-swandale · 2018-09-07T05:58:46Z

lib/rubygems/commands/cert_command.rb

@@ -290,6 +303,13 @@ def sign_certificates # :nodoc:
    end
  end

+  def re_sign_cert(cert, private_key)
+    Gem::Security::Signer.re_sign_cert(cert, private_key) do |cert_path, expired_cert_path|
+      alert("Your cert #{cert_path} has been re-signed")


I would use certificate instead of cert

Is there possible confusion with a method name? Otherwise I'm ok with it.

I think there's no confusion with a method name, but maybe certificate is more clear, thanks

lib/rubygems/security/signer.rb

drbrain · 2018-09-07T20:51:00Z

lib/rubygems/commands/cert_command.rb

@@ -15,6 +15,8 @@ def initialize
          :add => [], :remove => [], :list => [], :build => [], :sign => []

    OptionParser.accept OpenSSL::X509::Certificate do |certificate|
+      @certificate_file = certificate


OptionParser::accept is used to verify an argument is valid and to convert it to the correct type, not to set values for the input. If we end up needing to validate and convert arguments with these types and pull this code out it will cause breakage for this command.

Instead OptionParser#on (which add_option calls in RubyGems) must be used to record options given to a command, -C/--certificate below for this option.

… to preserve the file this acceptor can return both the certificate and the certificate location.

This is something the acceptor can do or is something that needs to be implemented?

Line 17 can become … do |certificate_file|

Line 19 (21 new) can become:

certificate = OpenSSL::X509::Certificate.new File.read certificate_file [certificate, certificate_file] # return both converted certificate and location of certificate

And line 70–71 (74–75 new) can become:

… do |(cert, file), options| options[:issuer_cert] = cert options[:issuer_cert_file] = cert_file

So the acceptor still only validates, but returns both the loaded Certificate and certificate path and opt.on '-C' stores both in options

drbrain · 2018-09-07T20:51:54Z

lib/rubygems/commands/cert_command.rb

@@ -26,6 +28,8 @@ def initialize
    end

    OptionParser.accept OpenSSL::PKey::RSA do |key_file|
+      @key_file = key_file


Ditto on OptionParser.accept, -K/--private-key is used to record arguments

drbrain · 2018-09-07T21:01:08Z

lib/rubygems/commands/cert_command.rb

@@ -114,6 +123,10 @@ def execute
      build email
    end

+    if options[:resign]
+      re_sign_cert(@certificate_file, @key_file)


This should use values from options after removing the instance variables

drbrain · 2018-09-07T21:02:19Z

lib/rubygems/commands/cert_command.rb

@@ -290,6 +303,13 @@ def sign_certificates # :nodoc:
    end
  end

+  def re_sign_cert(cert, private_key)
+    Gem::Security::Signer.re_sign_cert(cert, private_key) do |cert_path, expired_cert_path|


Since this needs the certificate location to store the expired certificate when we generate a new one that should be passed in as well, then have Gem::Security::Signer::re_sign_cert accept three arguments.

bronzdoc · 2018-09-11T03:56:41Z

@drbrain changes are done, build failing is not related to this PR... something weird with rubocop not being installed is happening in specific builds...

djberg96 · 2018-09-11T14:38:17Z

I tried restarting a couple of the builds to no avail. Not sure what's happening, but I thought I saw some curl failures in there.

drbrain

My name is Eric Hodel and I approve this pull request

bronzdoc · 2018-09-12T01:34:12Z

@bundlerbot r+

bundlerbot · 2018-09-12T01:34:12Z

📌 Commit 54ede15 has been approved by bronzdoc

bundlerbot · 2018-09-12T01:34:17Z

⌛ Testing commit 54ede15 with merge 7a49f40...

…nzdoc Add --re-sign flag to cert command # Description: Follow up of: #1720 (i'll squash the commits when i'm done) I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).

bundlerbot · 2018-09-12T01:59:36Z

☀️ Test successful - status-travis
Approved by: bronzdoc
Pushing 7a49f40 to master...

djberg96 · 2018-09-12T02:07:46Z

Woohoo, thanks!

This commits includes tiny bugfix and new features listed here: * Add --re-sign flag to cert command by bronzdoc: rubygems/rubygems#2391 * Download gems with threads. by indirect: rubygems/rubygems#1898 git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64769 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

bronzdoc added the status: triage label Aug 31, 2018

bronzdoc force-pushed the add_resign_flag_to_cert_command branch from b58a4af to 4895554 Compare September 5, 2018 03:50

bronzdoc changed the title ~~[WIP] Add resign flag to cert command~~ Add re-sign flag to cert command Sep 5, 2018

bronzdoc changed the title ~~Add re-sign flag to cert command~~ Add --re-sign flag to cert command Sep 5, 2018

bronzdoc force-pushed the add_resign_flag_to_cert_command branch from 6c605a6 to b558711 Compare September 5, 2018 14:19

colby-swandale reviewed Sep 7, 2018

View reviewed changes

bronzdoc force-pushed the add_resign_flag_to_cert_command branch 2 times, most recently from 23b4a6f to 847aaf3 Compare September 7, 2018 17:09

drbrain requested changes Sep 7, 2018

View reviewed changes

bronzdoc force-pushed the add_resign_flag_to_cert_command branch 4 times, most recently from e476898 to 6a4adbc Compare September 11, 2018 03:02

drbrain approved these changes Sep 11, 2018

View reviewed changes

Add --re-sign flag to cert

54ede15

bronzdoc force-pushed the add_resign_flag_to_cert_command branch from 6a4adbc to 54ede15 Compare September 12, 2018 01:34

bundlerbot merged commit 54ede15 into master Sep 12, 2018

bronzdoc removed the status: triage label Sep 12, 2018

bronzdoc deleted the add_resign_flag_to_cert_command branch September 12, 2018 16:13

colby-swandale added this to the 2.8.0 milestone Sep 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --re-sign flag to cert command #2391

Add --re-sign flag to cert command #2391

bronzdoc commented Aug 31, 2018 •

edited

djberg96 commented Aug 31, 2018

djberg96 commented Sep 4, 2018

bronzdoc commented Sep 4, 2018

djberg96 commented Sep 5, 2018

colby-swandale Sep 7, 2018

djberg96 Sep 7, 2018

bronzdoc Sep 7, 2018

djberg96 Sep 7, 2018

bronzdoc Sep 7, 2018

djberg96 Sep 7, 2018

colby-swandale Sep 7, 2018

djberg96 Sep 7, 2018

bronzdoc Sep 7, 2018

drbrain Sep 7, 2018

drbrain Sep 7, 2018

bronzdoc Sep 7, 2018

drbrain Sep 9, 2018

drbrain Sep 7, 2018

drbrain Sep 7, 2018

drbrain Sep 7, 2018

bronzdoc Sep 7, 2018

bronzdoc commented Sep 11, 2018

djberg96 commented Sep 11, 2018

drbrain left a comment

bronzdoc commented Sep 12, 2018

bundlerbot commented Sep 12, 2018

bundlerbot commented Sep 12, 2018

bundlerbot commented Sep 12, 2018

djberg96 commented Sep 12, 2018

Add --re-sign flag to cert command #2391

Add --re-sign flag to cert command #2391

Conversation

bronzdoc commented Aug 31, 2018 • edited

Description:

djberg96 commented Aug 31, 2018

djberg96 commented Sep 4, 2018

bronzdoc commented Sep 4, 2018

djberg96 commented Sep 5, 2018

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

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

bronzdoc commented Sep 11, 2018

djberg96 commented Sep 11, 2018

drbrain left a comment

Choose a reason for hiding this comment

bronzdoc commented Sep 12, 2018

bundlerbot commented Sep 12, 2018

bundlerbot commented Sep 12, 2018

bundlerbot commented Sep 12, 2018

djberg96 commented Sep 12, 2018

bronzdoc commented Aug 31, 2018 •

edited