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.

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

attestation: add initial attestation helpers, integrate into brew install #17049

Merged
merged 15 commits into from Apr 12, 2024

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented Apr 8, 2024

Adds the basic attestation verification APIs, as well as a pre-pour check against HOMEBREW_VERIFY_ATTESTATIONS that verifies the attestation (or backfill as necessary) for bottles from homebrew-core.

See #17019.

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Adds the basic attestation verification APIs, as well
as a pre-pour check against `HOMEBREW_VERIFY_ATTESTATIONS`
that verifies the attestation (or backfill as necessary)
for bottles from homebrew-core.

Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
@woodruffw woodruffw changed the title attestation: add initial attestation helpers attestation: add initial attestation helpers, integrate into brew install Apr 8, 2024
@woodruffw woodruffw marked this pull request as ready for review April 8, 2024 20:40
@woodruffw
Copy link
Member Author

NB: I haven't written any tests yet; I want to get some eyes on this and then I'll do so once the general approach seems fine 🙂

@Bo98
Copy link
Member

Bo98 commented Apr 9, 2024

Looks good.

Probably don't any of the shell handling when we can just do:

gh = with_env("HOMEBREW_VERIFY_ATTESTATIONS" => nil) do
  ensure_executable!("gh")
end

@woodruffw
Copy link
Member Author

Probably don't any of the shell handling when we can just do:

I was wracking my brain trying to remember ensure_executable! 😅 -- I'll switch to that.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looking good so far, nice work @woodruffw!

Library/Homebrew/attestation.rb Show resolved Hide resolved
Library/Homebrew/attestation.rb Outdated Show resolved Hide resolved
Library/Homebrew/attestation.rb Outdated Show resolved Hide resolved
Library/Homebrew/attestation.rb Show resolved Hide resolved
Library/Homebrew/attestation.rb Outdated Show resolved Hide resolved
Library/Homebrew/attestation.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/update.sh Outdated Show resolved Hide resolved
Library/Homebrew/brew.sh Outdated Show resolved Hide resolved
Library/Homebrew/exceptions.rb Outdated Show resolved Hide resolved
@woodruffw
Copy link
Member Author

Good for another look! I've ratcheted down the typing + removed all of the invasive .sh changes in favor of ensure_executable!.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Approach looks good to me! Some unit tests of Library/Homebrew/attestation.rb would be sufficient for this to get merged.

@woodruffw
Copy link
Member Author

Some unit tests of Library/Homebrew/attestation.rb would be sufficient for this to get merged.

Sounds good, doing today!

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looking good! Would be good to cover a couple more of the missing lines.

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Member Author

woodruffw commented Apr 11, 2024

The coverage should be much better now, but this has revealed a logic error we made during the backfill: the homebrew-core attestations are produced using the bottle filename (e.g. zoro--20211230.monterey.bottle.tar.gz), while the backfilled attestations are bound to a digested filename (e.g. fec7e59a30acea3bf98402e763d648878693f77230541e1f5834176974487f53--zoro--20211230.monterey.bottle.tar.gz).

We could hack around this in the verification, but redoing the backfill is likely to be cleaner. So I propose that we consider this blocked until we re-perform the backfill, which shouldn't take too long.

Edit: I thought about this a bit more, and there's a sound way to handle this different: the prepended hash is SHA256(resource.url), which is both independently computable and provides a nice bit of additional domain binding in the backfill (asserting not only that the backfileld signature is for a specific extant bottle, but that bottle comes from a known URL).

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

One remaining question and then I'm ✅. Coverage is sufficient, just curious about the new method having supposedly missing coverage.

@@ -423,6 +423,11 @@ def tab_attributes
github_packages_manifest_resource_tab(github_packages_manifest_resource)
end

sig { returns(Filename) }
def filename
Filename.create(resource.owner, @tag, @spec.rebuild)
Copy link
Member

Choose a reason for hiding this comment

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

Seems weird no tests cover this, is this used anywhere right now?

Copy link
Member Author

Choose a reason for hiding this comment

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

The tests currently use a double for it, so I think that's what it doesn't get coverage. Want me to add some separate coverage for it?

Copy link
Member

Choose a reason for hiding this comment

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

Yeh or just double lower in the stack (the owner/tag/rebuild) so it still gets called?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done with 1607d04 -- I went with a separate test since the more invasive double was going to require a lot of nesting that would make the other tests harder to read, but I can shoehorn it in if that's your preference 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

(I also had to move the BottleSpecification specs to their own file, since RuboCop was complaining about having two RSpec stanzas in bottle_spec.rb.)

Signed-off-by: William Woodruff <[email protected]>
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks again @woodruffw!

@MikeMcQuaid MikeMcQuaid merged commit c683e01 into Homebrew:master Apr 12, 2024
25 checks passed
@woodruffw woodruffw deleted the ww/attestation branch April 12, 2024 15:01
@github-actions github-actions bot added the outdated PR was locked due to age label May 13, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants