From 3bb91601fbde74c3926147b319c55e02e110a564 Mon Sep 17 00:00:00 2001 From: Nanda H Krishna Date: Mon, 15 Jul 2024 11:39:22 -0400 Subject: [PATCH 1/8] Ensure early installation of `gh` for attestations --- Library/Homebrew/attestation.rb | 27 ++++++++++++++++++++++++--- Library/Homebrew/cmd/install.rb | 8 ++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/attestation.rb b/Library/Homebrew/attestation.rb index 0ab000e583c6b..acf1c5c20a9df 100644 --- a/Library/Homebrew/attestation.rb +++ b/Library/Homebrew/attestation.rb @@ -57,6 +57,16 @@ def self.enabled? Homebrew::EnvConfig.developer? || Homebrew::EnvConfig.devcmdrun? end + # Ensures the availability of a suitable `gh` executable for attestation verification. + # + # @api private + sig { returns(Pathname) } + def self.ensure_gh_installed! + return @gh_executable if @gh_executable.present? + + gh_executable + end + # Returns a path to a suitable `gh` executable for attestation verification. # # @api private @@ -65,9 +75,20 @@ def self.gh_executable # 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 - ensure_executable!("gh") - end, T.nilable(Pathname)) + return @gh_executable if @gh_executable.present? + + with_env(HOMEBREW_NO_VERIFY_ATTESTATIONS: "1") do + @gh_executable = ensure_executable!("gh") + + 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 + @gh_executable = ensure_formula_installed!("gh", latest: true, + reason: "verifying attestations").opt_bin/"gh" + end + end + + @gh_executable end # Verifies the given bottle against a cryptographic attestation of build provenance. diff --git a/Library/Homebrew/cmd/install.rb b/Library/Homebrew/cmd/install.rb index 763a933606198..372326c74f842 100644 --- a/Library/Homebrew/cmd/install.rb +++ b/Library/Homebrew/cmd/install.rb @@ -263,6 +263,14 @@ def run end end + if Homebrew::Attestation.enabled? + if formulae.include?(Formula["gh"]) + formulae.unshift(formulae.delete(Formula["gh"])) + else + Homebrew::Attestation.ensure_gh_installed! + end + end + # if the user's flags will prevent bottle only-installations when no # developer tools are available, we need to stop them early on build_flags = [] From 4d387d285e0cc48f680be2a17c3a1d302008658a Mon Sep 17 00:00:00 2001 From: Nanda H Krishna Date: Mon, 15 Jul 2024 11:44:53 -0400 Subject: [PATCH 2/8] Fix type and style errors --- Library/Homebrew/attestation.rb | 3 ++- Library/Homebrew/cmd/install.rb | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/attestation.rb b/Library/Homebrew/attestation.rb index acf1c5c20a9df..76b1c27cc02af 100644 --- a/Library/Homebrew/attestation.rb +++ b/Library/Homebrew/attestation.rb @@ -75,6 +75,7 @@ def self.gh_executable # 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(nil, T.nilable(Pathname)) return @gh_executable if @gh_executable.present? with_env(HOMEBREW_NO_VERIFY_ATTESTATIONS: "1") do @@ -88,7 +89,7 @@ def self.gh_executable end end - @gh_executable + T.must(@gh_executable) end # Verifies the given bottle against a cryptographic attestation of build provenance. diff --git a/Library/Homebrew/cmd/install.rb b/Library/Homebrew/cmd/install.rb index 372326c74f842..bab0b2e90375b 100644 --- a/Library/Homebrew/cmd/install.rb +++ b/Library/Homebrew/cmd/install.rb @@ -265,7 +265,7 @@ def run if Homebrew::Attestation.enabled? if formulae.include?(Formula["gh"]) - formulae.unshift(formulae.delete(Formula["gh"])) + formulae.unshift(T.must(formulae.delete(Formula["gh"]))) else Homebrew::Attestation.ensure_gh_installed! end From 802eb54e87495c7cf7675b264a92876e5a9a41f6 Mon Sep 17 00:00:00 2001 From: Nanda H Krishna Date: Mon, 15 Jul 2024 12:23:10 -0400 Subject: [PATCH 3/8] Fix tests for attestations --- Library/Homebrew/attestation.rb | 7 ---- Library/Homebrew/test/attestation_spec.rb | 39 ++++++----------------- 2 files changed, 9 insertions(+), 37 deletions(-) diff --git a/Library/Homebrew/attestation.rb b/Library/Homebrew/attestation.rb index 76b1c27cc02af..19c281db960b8 100644 --- a/Library/Homebrew/attestation.rb +++ b/Library/Homebrew/attestation.rb @@ -129,13 +129,6 @@ 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 9011b25579588..1df9afd6e384d 100644 --- a/Library/Homebrew/test/attestation_spec.rb +++ b/Library/Homebrew/test/attestation_spec.rb @@ -6,6 +6,7 @@ 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_formula) { instance_double(Formula, "gh", opt_bin: Pathname.new("/extremely/fake")) } 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) } @@ -68,40 +69,22 @@ end describe "::gh_executable" do - it "calls ensure_executable" do - expect(described_class).to receive(:ensure_executable!) - .with("gh") - .and_return(fake_gh) - - described_class.gh_executable - end - end - - describe "::check_attestation fails 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) + it "calls ensure_executable and ensure_formula_installed" do + expect(described_class).to receive(:ensure_executable!) + .with("gh") + .and_return(fake_old_gh) - 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, chdir: HOMEBREW_TEMP) - .and_raise(ErrorDuringExecution.new(["foo"], status: fake_error_status)) + expect(described_class).to receive(:ensure_formula_installed!) + .with("gh", latest: true, reason: "verifying attestations") + .and_return(fake_gh_formula) - expect do - described_class.check_attestation fake_bottle, - described_class::HOMEBREW_CORE_REPO - end.to raise_error(ErrorDuringExecution) + described_class.gh_executable end end @@ -109,10 +92,6 @@ 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 51ec743d6f6116a2996eb225d9e64251329428ec Mon Sep 17 00:00:00 2001 From: Nanda H Krishna Date: Mon, 15 Jul 2024 12:28:07 -0400 Subject: [PATCH 4/8] Remove `ensure_gh_installed!` Co-authored-by: William Woodruff --- Library/Homebrew/attestation.rb | 10 ---------- Library/Homebrew/cmd/install.rb | 8 -------- 2 files changed, 18 deletions(-) diff --git a/Library/Homebrew/attestation.rb b/Library/Homebrew/attestation.rb index 19c281db960b8..d65ee36e015c5 100644 --- a/Library/Homebrew/attestation.rb +++ b/Library/Homebrew/attestation.rb @@ -57,16 +57,6 @@ def self.enabled? Homebrew::EnvConfig.developer? || Homebrew::EnvConfig.devcmdrun? end - # Ensures the availability of a suitable `gh` executable for attestation verification. - # - # @api private - sig { returns(Pathname) } - def self.ensure_gh_installed! - return @gh_executable if @gh_executable.present? - - gh_executable - end - # Returns a path to a suitable `gh` executable for attestation verification. # # @api private diff --git a/Library/Homebrew/cmd/install.rb b/Library/Homebrew/cmd/install.rb index bab0b2e90375b..763a933606198 100644 --- a/Library/Homebrew/cmd/install.rb +++ b/Library/Homebrew/cmd/install.rb @@ -263,14 +263,6 @@ def run end end - if Homebrew::Attestation.enabled? - if formulae.include?(Formula["gh"]) - formulae.unshift(T.must(formulae.delete(Formula["gh"]))) - else - Homebrew::Attestation.ensure_gh_installed! - end - end - # if the user's flags will prevent bottle only-installations when no # developer tools are available, we need to stop them early on build_flags = [] From d2d814414eb987ce41bb8621399a7292b1913cf5 Mon Sep 17 00:00:00 2001 From: Nanda H Krishna Date: Mon, 15 Jul 2024 13:30:47 -0400 Subject: [PATCH 5/8] cmd/{install,reinstall,upgrade}: ensure that `gh` is installed --- Library/Homebrew/attestation.rb | 1 + Library/Homebrew/cmd/install.rb | 8 ++++++++ Library/Homebrew/cmd/reinstall.rb | 8 ++++++++ Library/Homebrew/cmd/upgrade.rb | 8 ++++++++ 4 files changed, 25 insertions(+) diff --git a/Library/Homebrew/attestation.rb b/Library/Homebrew/attestation.rb index d65ee36e015c5..e62564394ea7e 100644 --- a/Library/Homebrew/attestation.rb +++ b/Library/Homebrew/attestation.rb @@ -4,6 +4,7 @@ require "date" require "json" require "utils/popen" +require "utils/github/api" require "exceptions" require "system_command" diff --git a/Library/Homebrew/cmd/install.rb b/Library/Homebrew/cmd/install.rb index 763a933606198..bab0b2e90375b 100644 --- a/Library/Homebrew/cmd/install.rb +++ b/Library/Homebrew/cmd/install.rb @@ -263,6 +263,14 @@ def run end end + if Homebrew::Attestation.enabled? + if formulae.include?(Formula["gh"]) + formulae.unshift(T.must(formulae.delete(Formula["gh"]))) + else + Homebrew::Attestation.ensure_gh_installed! + end + end + # if the user's flags will prevent bottle only-installations when no # developer tools are available, we need to stop them early on build_flags = [] diff --git a/Library/Homebrew/cmd/reinstall.rb b/Library/Homebrew/cmd/reinstall.rb index 20f87915f937b..37046832f0834 100644 --- a/Library/Homebrew/cmd/reinstall.rb +++ b/Library/Homebrew/cmd/reinstall.rb @@ -124,6 +124,14 @@ def run end end + if Homebrew::Attestation.enabled? + if formulae.include?(Formula["gh"]) + formulae.unshift(T.must(formulae.delete(Formula["gh"]))) + else + Homebrew::Attestation.ensure_gh_installed! + end + end + Install.perform_preinstall_checks formulae.each do |formula| diff --git a/Library/Homebrew/cmd/upgrade.rb b/Library/Homebrew/cmd/upgrade.rb index aab7e7714da04..ff898c08a771b 100644 --- a/Library/Homebrew/cmd/upgrade.rb +++ b/Library/Homebrew/cmd/upgrade.rb @@ -134,6 +134,14 @@ def run only_upgrade_formulae = formulae.present? && casks.blank? only_upgrade_casks = casks.present? && formulae.blank? + if Homebrew::Attestation.enabled? + if formulae.include?(Formula["gh"]) + formulae.unshift(T.must(formulae.delete(Formula["gh"]))) + else + Homebrew::Attestation.ensure_gh_installed! + end + end + upgrade_outdated_formulae(formulae) unless only_upgrade_casks upgrade_outdated_casks(casks) unless only_upgrade_formulae From 81e606007fe134e6f0cc54089746e9f111f9dc6e Mon Sep 17 00:00:00 2001 From: Nanda H Krishna Date: Mon, 15 Jul 2024 13:32:59 -0400 Subject: [PATCH 6/8] Fix type errors --- Library/Homebrew/cmd/upgrade.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/cmd/upgrade.rb b/Library/Homebrew/cmd/upgrade.rb index ff898c08a771b..75645c94f25fe 100644 --- a/Library/Homebrew/cmd/upgrade.rb +++ b/Library/Homebrew/cmd/upgrade.rb @@ -136,7 +136,7 @@ def run if Homebrew::Attestation.enabled? if formulae.include?(Formula["gh"]) - formulae.unshift(T.must(formulae.delete(Formula["gh"]))) + formulae.unshift(formulae.delete(Formula["gh"])) else Homebrew::Attestation.ensure_gh_installed! end From 8839ccfe72286f5e90a16897839b5f4016e3964f Mon Sep 17 00:00:00 2001 From: Nanda H Krishna Date: Mon, 15 Jul 2024 16:42:43 -0400 Subject: [PATCH 7/8] Fix tests for attestations --- Library/Homebrew/attestation.rb | 3 +-- Library/Homebrew/cmd/install.rb | 2 +- Library/Homebrew/cmd/reinstall.rb | 2 +- Library/Homebrew/cmd/upgrade.rb | 2 +- Library/Homebrew/test/attestation_spec.rb | 2 +- 5 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Library/Homebrew/attestation.rb b/Library/Homebrew/attestation.rb index e62564394ea7e..0e4b305ac919c 100644 --- a/Library/Homebrew/attestation.rb +++ b/Library/Homebrew/attestation.rb @@ -53,7 +53,6 @@ def self.enabled? return true if Homebrew::EnvConfig.verify_attestations? return false if GitHub::API.credentials.blank? return false if ENV.fetch("CI", false) - return false unless Formula["gh"].any_version_installed? Homebrew::EnvConfig.developer? || Homebrew::EnvConfig.devcmdrun? end @@ -70,7 +69,7 @@ def self.gh_executable return @gh_executable if @gh_executable.present? with_env(HOMEBREW_NO_VERIFY_ATTESTATIONS: "1") do - @gh_executable = ensure_executable!("gh") + @gh_executable = ensure_executable!("gh", reason: "verifying attestations") gh_version = Version.new(system_command!(@gh_executable, args: ["--version"], print_stderr: false) .stdout.match(/\d+(?:\.\d+)+/i).to_s) diff --git a/Library/Homebrew/cmd/install.rb b/Library/Homebrew/cmd/install.rb index bab0b2e90375b..9b8b9d59e4c45 100644 --- a/Library/Homebrew/cmd/install.rb +++ b/Library/Homebrew/cmd/install.rb @@ -267,7 +267,7 @@ def run if formulae.include?(Formula["gh"]) formulae.unshift(T.must(formulae.delete(Formula["gh"]))) else - Homebrew::Attestation.ensure_gh_installed! + Homebrew::Attestation.gh_executable end end diff --git a/Library/Homebrew/cmd/reinstall.rb b/Library/Homebrew/cmd/reinstall.rb index 37046832f0834..5f8a75144d55e 100644 --- a/Library/Homebrew/cmd/reinstall.rb +++ b/Library/Homebrew/cmd/reinstall.rb @@ -128,7 +128,7 @@ def run if formulae.include?(Formula["gh"]) formulae.unshift(T.must(formulae.delete(Formula["gh"]))) else - Homebrew::Attestation.ensure_gh_installed! + Homebrew::Attestation.gh_executable end end diff --git a/Library/Homebrew/cmd/upgrade.rb b/Library/Homebrew/cmd/upgrade.rb index 75645c94f25fe..cab4f27556c2e 100644 --- a/Library/Homebrew/cmd/upgrade.rb +++ b/Library/Homebrew/cmd/upgrade.rb @@ -138,7 +138,7 @@ def run if formulae.include?(Formula["gh"]) formulae.unshift(formulae.delete(Formula["gh"])) else - Homebrew::Attestation.ensure_gh_installed! + Homebrew::Attestation.gh_executable end end diff --git a/Library/Homebrew/test/attestation_spec.rb b/Library/Homebrew/test/attestation_spec.rb index 1df9afd6e384d..850b4508922bb 100644 --- a/Library/Homebrew/test/attestation_spec.rb +++ b/Library/Homebrew/test/attestation_spec.rb @@ -77,7 +77,7 @@ it "calls ensure_executable and ensure_formula_installed" do expect(described_class).to receive(:ensure_executable!) - .with("gh") + .with("gh", reason: "verifying attestations") .and_return(fake_old_gh) expect(described_class).to receive(:ensure_formula_installed!) From 6db608f43f894ddd9c157706aa76483af6d30e18 Mon Sep 17 00:00:00 2001 From: Nanda H Krishna Date: Wed, 17 Jul 2024 14:45:59 -0400 Subject: [PATCH 8/8] Ensure that the `gh` formula is sufficiently new --- Library/Homebrew/attestation.rb | 4 ++++ Library/Homebrew/test/attestation_spec.rb | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/Library/Homebrew/attestation.rb b/Library/Homebrew/attestation.rb index 0e4b305ac919c..7dd23bdad4fb1 100644 --- a/Library/Homebrew/attestation.rb +++ b/Library/Homebrew/attestation.rb @@ -74,6 +74,10 @@ def self.gh_executable 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 + if Formula["gh"].version < GH_ATTESTATION_MIN_VERSION + raise "#{@gh_executable} is too old, you must upgrade it to >=#{GH_ATTESTATION_MIN_VERSION} to continue" + end + @gh_executable = ensure_formula_installed!("gh", latest: true, reason: "verifying attestations").opt_bin/"gh" end diff --git a/Library/Homebrew/test/attestation_spec.rb b/Library/Homebrew/test/attestation_spec.rb index 850b4508922bb..b82dc1156563a 100644 --- a/Library/Homebrew/test/attestation_spec.rb +++ b/Library/Homebrew/test/attestation_spec.rb @@ -70,6 +70,10 @@ describe "::gh_executable" do before do + allow(Formulary).to receive(:factory) + .with("gh") + .and_return(instance_double(Formula, version: Version.new("2.49.0"))) + allow(described_class).to receive(:system_command!) .with(fake_old_gh, args: ["--version"], print_stderr: false) .and_return(fake_old_gh_version)