Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

make verify command work in bundler#50

Merged
rochlefebvre merged 1 commit into
mainfrom
bundler_verify_command
Dec 17, 2021
Merged

make verify command work in bundler#50
rochlefebvre merged 1 commit into
mainfrom
bundler_verify_command

Conversation

@doodzik
Copy link
Copy Markdown

@doodzik doodzik commented Dec 16, 2021

This PR fixes #36 and enables the usage of gem signature verifying with the usage of bundler.

I'm mainly copied the existing verify command and had to adjust the code a bit because I don't have the command class available. I also introduced a concept of signing-policy, which will enable us to provide different signing stories.

Running the gem command with the signing-policy env variable
Screen Shot 2021-12-16 at 11 01 51 AM

Running bundler with the --verbose flag and the signing-policy env variable
Screen Shot 2021-12-16 at 5 46 07 PM

I added a raise statement to the command to see if it would fail bundle install and it did.
Screen Shot 2021-12-16 at 5 48 51 PM

I tested the commands with the gem and bundler command and it works for both.

Feel free to merge this PR or make changes to it.

@doodzik doodzik force-pushed the bundler_verify_command branch 2 times, most recently from dcfc548 to d7ac873 Compare December 16, 2021 18:22
Comment thread lib/rubygems/commands/verify_extend.rb Outdated

class Gem::Commands::InstallCommand
alias_method :original_execute, :execute
def execute
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Any particular reasons why we're not leveraging the pre_install hook for plugins? I would only reopen the command class and redefine its entry method if necessary.

Copy link
Copy Markdown
Author

@doodzik doodzik Dec 17, 2021

Choose a reason for hiding this comment

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

I changed it here so I had fewer things to check when I tested the gem. I didn't intend for this change to be permanent.

@@ -0,0 +1,29 @@
class Gem::SecurityPolicy
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Security policy sounds very broad. How about we start with Signing Policy?

@doodzik doodzik force-pushed the bundler_verify_command branch 5 times, most recently from 8950d32 to 94f00dd Compare December 17, 2021 01:52
end
end
rescue Gem::SigstoreException => ex
rescue StandardError => ex
Copy link
Copy Markdown
Author

@doodzik doodzik Dec 17, 2021

Choose a reason for hiding this comment

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

Gem::SigstoreException doesn't exist at the moment. We can implement proper error handling in a followup PR since it is outside of the scope of this PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yup, I have hit this same problem. I will create an issue to track the introduction of a proper exception hierarchy.

@doodzik doodzik marked this pull request as ready for review December 17, 2021 05:40
@doodzik doodzik requested review from a team and rochlefebvre December 17, 2021 05:40
@doodzik
Copy link
Copy Markdown
Author

doodzik commented Dec 17, 2021

As a followup to this PR and #51 we can consolidate the logic for the verify command into one spot.

Comment thread lib/rubygems/commands/verify_extend.rb Outdated
if (package = installer.package)
gem_path = package.gem.path

pp "Verifying #{gem_path}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Let's use rubygems' own UI printing commands, like say.

say "Verifying #{gem_path}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Given that @doodzik is out for the year, maybe we could do this in a followup PR?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have pulled down his branch to replace pp with say. Otherwise the PR looks good to me.


pp "Verifying #{gem_path}"

raise Gem::CommandLineError, "#{gem_path} is not a file" unless File.file?(gem_path)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Decent placeholder until we address #37.

@rochlefebvre rochlefebvre force-pushed the bundler_verify_command branch from 94f00dd to f9684af Compare December 17, 2021 14:37
@rochlefebvre rochlefebvre merged commit 1ea850b into main Dec 17, 2021
Copy link
Copy Markdown

@jchestershopify jchestershopify left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Control whether gem install verifies signatures with an environment variable

4 participants