Introduce GPG operations

[tapeinosyne]

A Cask::GPG class which covers the basics of gpg verification, relying on system gpg. Refer to #5971.

[rolandwalker]

+1

[rolandwalker]

This is not compatible with version :latest Casks.

The check against that case could go in Cask::DSL, but it is probably more robust to put it here. It should be sufficient to check that version is not a symbol. The current general practice is for string values to be proper version numbers, which will be enforced in the future.

[tapeinosyne]

I agree that GPG-related checks would be best implemented here.

Incompatibility with :latest arises on the tangent of GPG verification, i.e. on metadata management. Disabling signature caching for :latest casks should be sufficient to prevent failures.

[rolandwalker]

My reading is that we fully agree re: :latest Casks.

The following issues came up on further review:

Namespace clash with Cask::DSL::Gpg.

Since class Cask extends class Cask::DSL, Cask::Gpg.new ends up invoking the initialize method from Cask::DSL::Gpg instead of here. Later, I'd prefer to resolve that by making Cask not extend Cask::DSL. For expedience, we should probably relocate this class to Cask::GpgCheck or similar.

@cask.gpg can return nil

If there is no gpg stanza, @cask.gpg will return nil, so eg @cask.gpg.key_id will raise an exception. If verify is the only typical entry point, it is easy to test @cask.gpg there.

Exit codes from Cask::SystemCommand

You had to use open3 directly because Cask::SystemCommand doesn't give you the exit value. That should be easy to address, and I will do so.

Nit: full paths preferred

Since we only support one OS, /usr/bin/type is more robust than type. We might also do the same for the gpg executable, as installed? discovers the full path.

Cask::DSL::Gpg currently permits invalid :key_id values

That can be assigned to me.

Return value from verify

The current implementation prints the output from the gpg command but does not return success/fail. So we are depending on run! to raise an exception on failure? Those exceptions will be cryptic. The same goes for failures within import_keys. Dispatching verify on Cask cryptol.rb gave me a printed WARNING, but did not raise an error. Will the user see that as success or failure?

$ brew cask fetch cryptol
==> Fetching resources for Cask cryptol
==> Downloading https://github.com/GaloisInc/cryptol/releases/download/v2.0.0/cryptol-2.0.0-MacOSX-64.tar.gz
Already downloaded: /Library/Caches/Homebrew/cryptol-2.0.0.tar.gz
==> Verifying GPG signature for cryptol
==> gpg: Signature made Thu Apr 24 14:20:54 2014 EDT using RSA key ID 47E6F9A6
==> gpg: Good signature from "Galois, Inc. (Galois corporate key for code signing.) <[email protected]>"
==> gpg:                 aka "[jpeg image of size 10642]"
==> gpg: WARNING: This key is not certified with a trusted signature!
==> gpg:          There is no indication that the signature belongs to the owner.
==> Primary key fingerprint: 9C05 6E42 F9E6 D236 13C6  3E86 EAB0 F620 47E6 F9A6

[tapeinosyne]

Namespace clash with Cask::DSL::Gpg

As a non-rubyist, that catches me unprepared. I seem unable to reproduce the collision.

@cask.gpg can return nil

The nil check (which I failed to mention in the commit message) is currently delegated to the caller, i.e. gpg.verify(cask) if gpg.available?. Moving the check to ::Gpg is sensible, but we lose some clarity at the call site.

Return value from verify
[…] Dispatching verify on Cask cryptol.rb gave me a printed WARNING, but did not raise an error. Will the user see that as success or failure?

The warning you mention is very common, and I expect gpg users to be familiar with it. Nonetheless, it is desirable to keep an internal notion of success and failure for each gpg event.

[rolandwalker]

#6329 addresses the ability to retrieve the exit status when using Cask::SystemCommand.

import_key will have to explicitly invoke the stdout method to get a string value:

@command.run!('gpg', :args => args).stdout

In addition, I hope that preserving the exit status will be helpful for tracking success of the verification.

[tapeinosyne]

Addressed some of the concerns raised by review.

• :latest casks always download signatures.
• A patch for DSL-level :key_id validation is pending has been merged.
• Use refactored Cask::SystemCommand.
• Use absolute path for type.
• Amended the class name. (I was unable to reproduce the namespace collision, but @rolandwalker observes it consistently.)