Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cache bottle attestations #18581

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Cache bottle attestations #18581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89645ad
Cache bottle attestations
carlocab Oct 14, 2024
67351de
Update Library/Homebrew/attestation.rb
woodruffw Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions Library/Homebrew/attestation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,13 @@ def self.check_attestation(bottle, signing_repo, signing_workflow = nil, subject
attestation
end

ATTESTATION_CACHE_DIRECTORY = T.let((HOMEBREW_CACHE/"attestation").freeze, Pathname)

sig { params(bottle: Bottle).returns(Pathname) }
def self.cached_attestation_path(bottle)
ATTESTATION_CACHE_DIRECTORY/bottle.filename.attestation_json(bottle.resource.checksum.hexdigest)
end

ATTESTATION_MAX_RETRIES = 5

# Verifies the given bottle against a cryptographic attestation of build provenance
Expand All @@ -203,6 +210,16 @@ def self.check_attestation(bottle, signing_repo, signing_workflow = nil, subject
# @api private
sig { params(bottle: Bottle).returns(T::Hash[T.untyped, T.untyped]) }
def self.check_core_attestation(bottle)
cached_attestation = cached_attestation_path(bottle)

if cached_attestation.exist?
begin
return JSON.parse(cached_attestation.read)
Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure I understand: the intended behavior here is to treat a cached attestation as implicitly previously verified, right?

I think we could go two ways with this: either assume cached == verified (which in turn means an attacker who can modify the cache/insert a dummy cache member can bypass verification, although such an attacker is already pretty powerful), or perform verification again on the cached attestation (so skip the download, but do the rest of the gh attestation verify).

The first seems fine to me, but the second is perhaps preferable.

Copy link
Member

Choose a reason for hiding this comment

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

The first seems fine to me, but the second is perhaps preferable.

Agreed 👍🏻

rescue JSON::ParserError
cached_attestation.unlink
end
end

begin
# Ideally, we would also constrain the signing workflow here, but homebrew-core
# currently uses multiple signing workflows to produce bottles
Expand All @@ -216,6 +233,8 @@ def self.check_core_attestation(bottle)
# workflow, which would then be our sole identity. However, GitHub's
# attestations currently do not include reusable workflow state by default.
attestation = check_attestation bottle, HOMEBREW_CORE_REPO
ATTESTATION_CACHE_DIRECTORY.mkpath
cached_attestation.atomic_write attestation.to_json
return attestation
rescue MissingAttestationError
odebug "falling back on backfilled attestation for #{bottle}"
Expand Down Expand Up @@ -257,6 +276,8 @@ def self.check_core_attestation(bottle)
end
end

ATTESTATION_CACHE_DIRECTORY.mkpath
cached_attestation.atomic_write backfill_attestation.to_json
backfill_attestation
rescue InvalidAttestationError
@attestation_retry_count ||= T.let(Hash.new(0), T.nilable(T::Hash[Bottle, Integer]))
Expand Down
13 changes: 12 additions & 1 deletion Library/Homebrew/cleanup.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ def stale?(entry, scrub: false)
stale_cask?(pathname, scrub)
when :gh_actions_artifact
stale_gh_actions_artifact?(pathname, scrub)
when :attestation
stale_attestation?(pathname, scrub)
else
stale_formula?(pathname, scrub)
end
Expand All @@ -76,6 +78,13 @@ def stale_gh_actions_artifact?(pathname, scrub)
scrub || prune?(pathname, GH_ACTIONS_ARTIFACT_CLEANUP_DAYS)
end

ATTESTATION_CLEANUP_DAYS = 3

sig { params(pathname: Pathname, scrub: T::Boolean).returns(T::Boolean) }
def stale_attestation?(pathname, scrub)
scrub || prune?(pathname, ATTESTATION_CLEANUP_DAYS)
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice if this could be cleaned up based on the state of the cached bottles? Not a blocker, very much nice-to-have.

end

sig { params(pathname: Pathname, scrub: T::Boolean).returns(T::Boolean) }
def stale_api_source?(pathname, scrub)
return true if scrub
Expand Down Expand Up @@ -381,11 +390,13 @@ def cache_files
cask_files = (cache/"Cask").directory? ? (cache/"Cask").children : []
api_source_files = (cache/"api-source").glob("*/*/*/*/*") # `<org>/<repo>/<git_head>/<type>/<token>.rb`
gh_actions_artifacts = (cache/"gh-actions-artifact").directory? ? (cache/"gh-actions-artifact").children : []
attestations = (cache/"attestation").directory? ? (cache/"attestation").children : []

files.map { |path| { path:, type: nil } } +
cask_files.map { |path| { path:, type: :cask } } +
api_source_files.map { |path| { path:, type: :api_source } } +
gh_actions_artifacts.map { |path| { path:, type: :gh_actions_artifact } }
gh_actions_artifacts.map { |path| { path:, type: :gh_actions_artifact } } +
attestations.map { |path| { path:, type: :attestation } }
end

def cleanup_empty_api_source_directories(directory = cache/"api-source")
Expand Down
5 changes: 5 additions & 0 deletions Library/Homebrew/software_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,11 @@ def json
"#{name}--#{version}.#{tag}.bottle.json"
end

sig { params(checksum: String).returns(String) }
def attestation_json(checksum)
"#{checksum}-#{name}--#{version}.#{tag}.attestation.json"
end

def url_encode
ERB::Util.url_encode("#{name}-#{version}#{extname}")
end
Expand Down
Loading