From 1ae21d27319b7959838b0d5974fe40ae3cdfeb57 Mon Sep 17 00:00:00 2001 From: Nanda H Krishna Date: Sun, 14 Jul 2024 12:06:21 -0400 Subject: [PATCH 1/2] attestation: improve error message when `gh` is too old --- Library/Homebrew/attestation.rb | 14 +++++++-- Library/Homebrew/test/attestation_spec.rb | 35 +++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/attestation.rb b/Library/Homebrew/attestation.rb index 0ae04cdf5e7ab..a27124f5413fc 100644 --- a/Library/Homebrew/attestation.rb +++ b/Library/Homebrew/attestation.rb @@ -14,6 +14,9 @@ module Attestation # @api private HOMEBREW_CORE_REPO = "Homebrew/homebrew-core" + # @api private + GH_ATTESTATION_MIN_VERSION = T.let(Version.new("2.49.0").freeze, Version) + # @api private BACKFILL_REPO = "trailofbits/homebrew-brew-verify" @@ -59,10 +62,10 @@ def self.enabled? # @api private sig { returns(Pathname) } def self.gh_executable - # NOTE: We disable HOMEBREW_VERIFY_ATTESTATIONS when installing `gh` itself, + # NOTE: We disable HOMEBREW_NO_VERIFY_ATTESTATIONS when installing `gh` itself, # to prevent a cycle during bootstrapping. This can eventually be resolved # by vendoring a pure-Ruby Sigstore verifier client. - @gh_executable ||= T.let(with_env("HOMEBREW_VERIFY_ATTESTATIONS" => nil) do + @gh_executable ||= T.let(with_env(HOMEBREW_NO_VERIFY_ATTESTATIONS: "1") do ensure_executable!("gh") end, T.nilable(Pathname)) end @@ -104,6 +107,13 @@ def self.check_attestation(bottle, signing_repo, signing_workflow = nil, subject # Even if we have credentials, they may be invalid or malformed. raise GhAuthNeeded, "invalid credentials" if e.status.exitstatus == 4 + gh_version = Version.new(system_command!(gh_executable, args: ["--version"], print_stderr: false) + .stdout.match(/\d+(?:\.\d+)+/i).to_s) + if gh_version < GH_ATTESTATION_MIN_VERSION + raise e, + "#{gh_executable} is too old, you must upgrade it to continue." + end + raise InvalidAttestationError, "attestation verification failed: #{e}" end diff --git a/Library/Homebrew/test/attestation_spec.rb b/Library/Homebrew/test/attestation_spec.rb index bd92aea4f914c..bde80468cabf0 100644 --- a/Library/Homebrew/test/attestation_spec.rb +++ b/Library/Homebrew/test/attestation_spec.rb @@ -4,7 +4,10 @@ RSpec.describe Homebrew::Attestation do let(:fake_gh) { Pathname.new("/extremely/fake/gh") } + let(:fake_old_gh) { Pathname.new("/extremely/fake/old/gh") } let(:fake_gh_creds) { "fake-gh-api-token" } + let(:fake_gh_version) { instance_double(SystemCommand::Result, stdout: "2.49.0") } + let(:fake_old_gh_version) { instance_double(SystemCommand::Result, stdout: "2.48.0") } let(:fake_error_status) { instance_double(Process::Status, exitstatus: 1, termsig: nil) } let(:fake_auth_status) { instance_double(Process::Status, exitstatus: 4, termsig: nil) } let(:cached_download) { "/fake/cached/download" } @@ -74,10 +77,42 @@ end end + describe "::check_attestation (with old gh)" do + before do + allow(described_class).to receive(:gh_executable) + .and_return(fake_old_gh) + + allow(described_class).to receive(:system_command!) + .with(fake_old_gh, args: ["--version"], print_stderr: false) + .and_return(fake_old_gh_version) + end + + it "raises when gh is too old" do + expect(GitHub::API).to receive(:credentials) + .and_return(fake_gh_creds) + + expect(described_class).to receive(:system_command!) + .with(fake_old_gh, args: ["attestation", "verify", cached_download, "--repo", + described_class::HOMEBREW_CORE_REPO, "--format", "json"], + env: { "GH_TOKEN" => fake_gh_creds }, secrets: [fake_gh_creds], + print_stderr: false) + .and_raise(ErrorDuringExecution.new(["foo"], status: fake_error_status)) + + expect do + described_class.check_attestation fake_bottle, + described_class::HOMEBREW_CORE_REPO + end.to raise_error(ErrorDuringExecution) + end + end + describe "::check_attestation" do before do allow(described_class).to receive(:gh_executable) .and_return(fake_gh) + + allow(described_class).to receive(:system_command!) + .with(fake_gh, args: ["--version"], print_stderr: false) + .and_return(fake_gh_version) end it "raises without any gh credentials" do From ad1500ad6012a454b4d3e635cdf20288820eb6bf Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 14 Jul 2024 16:30:12 -0400 Subject: [PATCH 2/2] Apply suggestions from code review --- Library/Homebrew/attestation.rb | 4 ++-- Library/Homebrew/test/attestation_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/attestation.rb b/Library/Homebrew/attestation.rb index a27124f5413fc..912f531a748b6 100644 --- a/Library/Homebrew/attestation.rb +++ b/Library/Homebrew/attestation.rb @@ -62,7 +62,7 @@ def self.enabled? # @api private sig { returns(Pathname) } def self.gh_executable - # NOTE: We disable HOMEBREW_NO_VERIFY_ATTESTATIONS when installing `gh` itself, + # NOTE: We set HOMEBREW_NO_VERIFY_ATTESTATIONS when installing `gh` itself, # to prevent a cycle during bootstrapping. This can eventually be resolved # by vendoring a pure-Ruby Sigstore verifier client. @gh_executable ||= T.let(with_env(HOMEBREW_NO_VERIFY_ATTESTATIONS: "1") do @@ -111,7 +111,7 @@ def self.check_attestation(bottle, signing_repo, signing_workflow = nil, subject .stdout.match(/\d+(?:\.\d+)+/i).to_s) if gh_version < GH_ATTESTATION_MIN_VERSION raise e, - "#{gh_executable} is too old, you must upgrade it to continue." + "#{gh_executable} is too old, you must upgrade it to continue" end raise InvalidAttestationError, "attestation verification failed: #{e}" diff --git a/Library/Homebrew/test/attestation_spec.rb b/Library/Homebrew/test/attestation_spec.rb index bde80468cabf0..64240d33d31b2 100644 --- a/Library/Homebrew/test/attestation_spec.rb +++ b/Library/Homebrew/test/attestation_spec.rb @@ -77,7 +77,7 @@ end end - describe "::check_attestation (with old gh)" do + describe "::check_attestation fails with old gh" do before do allow(described_class).to receive(:gh_executable) .and_return(fake_old_gh)