Merge pull request #13627 from SMillerDev/feature/cask/signing_check

cask: add audit for incorrect signing

Library/Homebrew/cask/audit.rb

@@ -20,28 +20,31 @@ module Cask
 
     attr_reader :cask, :download
 
-    attr_predicate :appcast?, :new_cask?, :strict?, :online?, :token_conflicts?
+    attr_predicate :appcast?, :new_cask?, :strict?, :signing?, :online?, :token_conflicts?
 
     def initialize(cask, appcast: nil, download: nil, quarantine: nil,
-                   token_conflicts: nil, online: nil, strict: nil,
+                   token_conflicts: nil, online: nil, strict: nil, signing: nil,
                    new_cask: nil)
 
-      # `new_cask` implies `online` and `strict`
+      # `new_cask` implies `online`, `token_conflicts`, `strict` and `signing`
       online = new_cask if online.nil?
       strict = new_cask if strict.nil?
+      signing = new_cask if signing.nil?
+      token_conflicts = new_cask if token_conflicts.nil?
 
       # `online` implies `appcast` and `download`
       appcast = online if appcast.nil?
       download = online if download.nil?
 
-      # `new_cask` implies `token_conflicts`
+      # `signing` implies `download`
-      token_conflicts = new_cask if token_conflicts.nil?
+      download = signing if download.nil?
 
       @cask = cask
       @appcast = appcast
       @download = Download.new(cask, quarantine: quarantine) if download
       @online = online
       @strict = strict
+      @signing = signing
       @new_cask = new_cask
       @token_conflicts = token_conflicts
     end
@@ -81,6 +84,7 @@ module Cask
       check_github_repository_archived
       check_github_prerelease_version
       check_bitbucket_repository
+      check_signing
       self
     rescue => e
       odebug e, e.backtrace
@@ -550,6 +554,33 @@ module Cask
       add_error "download not possible: #{e}"
     end
 
+    def check_signing
+      return if !signing? || download.blank? || cask.url.blank?
+
+      odebug "Auditing signing"
+      odebug cask.artifacts
+      artifacts = cask.artifacts.select { |k| k.is_a?(Artifact::Pkg) || k.is_a?(Artifact::App) }
+
+      return if artifacts.empty?
+
+      downloaded_path = download.fetch
+      primary_container = UnpackStrategy.detect(downloaded_path, type: @cask.container&.type, merge_xattrs: true)
+
+      return if primary_container.nil?
+
+      Dir.mktmpdir do |tmpdir|
+        tmpdir = Pathname(tmpdir)
+        primary_container.extract_nestedly(to: tmpdir, basename: downloaded_path.basename, verbose: false)
+        cask.artifacts.each do |artifact|
+          result = system_command("codesign", args: [
+            "--verify",
+            tmpdir/artifact.source.basename,
+          ], print_stderr: false)
+          add_warning result.merged_output unless result.success?
+        end
+      end
+    end
+
     def check_livecheck_version
       return unless appcast?
 

Library/Homebrew/cask/auditor.rb

@@ -15,6 +15,7 @@ module Cask
       audit_online: nil,
       audit_new_cask: nil,
       audit_strict: nil,
+      audit_signing: nil,
       audit_token_conflicts: nil,
       quarantine: nil,
       any_named_args: nil,
@@ -29,6 +30,7 @@ module Cask
         audit_online:          audit_online,
         audit_new_cask:        audit_new_cask,
         audit_strict:          audit_strict,
+        audit_signing:         audit_signing,
         audit_token_conflicts: audit_token_conflicts,
         quarantine:            quarantine,
         any_named_args:        any_named_args,
@@ -46,6 +48,7 @@ module Cask
       audit_appcast: nil,
       audit_online: nil,
       audit_strict: nil,
+      audit_signing: nil,
       audit_token_conflicts: nil,
       audit_new_cask: nil,
       quarantine: nil,
@@ -60,6 +63,7 @@ module Cask
       @audit_online = audit_online
       @audit_new_cask = audit_new_cask
       @audit_strict = audit_strict
+      @audit_signing = audit_signing
       @quarantine = quarantine
       @audit_token_conflicts = audit_token_conflicts
       @any_named_args = any_named_args
@@ -133,6 +137,7 @@ module Cask
         appcast:         @audit_appcast,
         online:          @audit_online,
         strict:          @audit_strict,
+        signing:         @audit_signing,
         new_cask:        @audit_new_cask,
         token_conflicts: @audit_token_conflicts,
         download:        @audit_download,

Library/Homebrew/cask/cmd/audit.rb

@@ -19,6 +19,8 @@ module Cask
                  description: "Audit the appcast"
           switch "--[no-]token-conflicts",
                  description: "Audit for token conflicts"
+          switch "--[no-]signing",
+                 description: "Audit for signed apps, which is required on ARM"
           switch "--[no-]strict",
                  description: "Run additional, stricter style checks"
           switch "--[no-]online",
@@ -50,6 +52,7 @@ module Cask
           appcast:               args.appcast?,
           online:                args.online?,
           strict:                args.strict?,
+          signing:               args.signing?,
           new_cask:              args.new_cask?,
           token_conflicts:       args.token_conflicts?,
           quarantine:            args.quarantine?,
@@ -71,6 +74,7 @@ module Cask
         appcast: nil,
         online: nil,
         strict: nil,
+        signing: nil,
         new_cask: nil,
         token_conflicts: nil,
         quarantine: nil,
@@ -84,6 +88,7 @@ module Cask
           audit_appcast:         appcast,
           audit_online:          online,
           audit_strict:          strict,
+          audit_signing:         signing,
           audit_new_cask:        new_cask,
           audit_token_conflicts: token_conflicts,
           quarantine:            quarantine,

Library/Homebrew/test/cask/audit_spec.rb

@@ -411,6 +411,53 @@ describe Cask::Audit, :cask do
       end
     end
 
+    describe "signing checks" do
+      let(:download_double) { instance_double(Cask::Download) }
+      let(:unpack_double) { instance_double(UnpackStrategy::Zip) }
+
+      before do
+        allow(audit).to receive(:download).and_return(download_double)
+        allow(audit).to receive(:signing?).and_return(true)
+        allow(audit).to receive(:check_https_availability)
+      end
+
+      context "when cask is not using a signed artifact" do
+        let(:cask) do
+          tmp_cask "signing-cask-test", <<~RUBY
+            cask 'signing-cask-test' do
+              version '1.0'
+              url "https://brew.sh/index.html"
+              binary 'Audit.app'
+            end
+          RUBY
+        end
+
+        it "does not fail" do
+          expect(download_double).not_to receive(:fetch)
+          expect(UnpackStrategy).not_to receive(:detect)
+          expect(run).not_to warn_with(/Audit\.app/)
+        end
+      end
+
+      context "when cask is using a signed artifact" do
+        let(:cask) do
+          tmp_cask "signing-cask-test", <<~RUBY
+            cask 'signing-cask-test' do
+              version '1.0'
+              url "https://brew.sh/"
+              pkg 'Audit.app'
+            end
+          RUBY
+        end
+
+        it "does not fail since no extract" do
+          allow(download_double).to receive(:fetch).and_return(Pathname.new("/tmp/test.zip"))
+          allow(UnpackStrategy).to receive(:detect).and_return(nil)
+          expect(run).not_to warn_with(/Audit\.app/)
+        end
+      end
+    end
+
     describe "livecheck should be skipped" do
       let(:online) { true }
       let(:message) { /Version '[^']*' differs from '[^']*' retrieved by livecheck\./ }
@@ -888,7 +935,7 @@ describe Cask::Audit, :cask do
     end
 
     describe "audit of downloads" do
-      let(:cask_token) { "with-binary" }
+      let(:cask_token) { "basic-cask" }
       let(:cask) { Cask::CaskLoader.load(cask_token) }
       let(:download_double) { instance_double(Cask::Download) }
       let(:message) { "Download Failed" }
@@ -896,10 +943,11 @@ describe Cask::Audit, :cask do
       before do
         allow(audit).to receive(:download).and_return(download_double)
         allow(audit).to receive(:check_https_availability)
+        allow(UnpackStrategy).to receive(:detect).and_return(nil)
       end
 
       it "when download and verification succeed it does not fail" do
-        expect(download_double).to receive(:fetch)
+        expect(download_double).to receive(:fetch).and_return(Pathname.new("/tmp/test.zip"))
         expect(run).to pass
       end