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

SBOM improvements #17254

Merged
merged 1 commit into from May 9, 2024
Merged

SBOM improvements #17254

merged 1 commit into from May 9, 2024

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented May 8, 2024

  • write a schema when installing formulae (if not already present)
  • cache the schema on disk rather than downloading it every time
  • make more methods/attributes private
  • allow validation to be optional, only enable for Homebrew developers at installation time
  • use the tab for more, correct information
  • ensure that dependencies/bottles are written correctly
  • use new SBOM 3 schema URL
  • improve test coverage

@SMillerDev
Copy link
Member

I'm not sure how useful the SBOM writing on install is since it's supposed to be a record of the build.

@MikeMcQuaid
Copy link
Member Author

I'm not sure how useful the SBOM writing on install is since it's supposed to be a record of the build.

@SMillerDev My thinking is: it still records the build but this avoids the need to backfill this information into all bottles and also gets it when building from source.

@MikeMcQuaid MikeMcQuaid force-pushed the sbom_tweaks branch 2 times, most recently from 1db103d to e6e344b Compare May 8, 2024 15:02
@SMillerDev
Copy link
Member

One thing I noticed (that might be worth checking here) is that the build time is always a 0 unix timestamp due to our reproducible builds. If you have any ideas for that, let me know. Otherwise maybe we add a TODO: figure out how to record the actual build time

@Bo98
Copy link
Member

Bo98 commented May 8, 2024

Depending on json_schemer on formula installs is a little messy since it depends on a native gem so we may want to visit whether we can have an alternative gem that doesn't do that.

@MikeMcQuaid
Copy link
Member Author

Depending on json_schemer on formula installs

@Bo98 It's not doing that unless you've set HOMEBREW_DEVELOPER. I could make this even stricter and only do so on HOMEBREW_ENFORCE_SBOM.

@Bo98
Copy link
Member

Bo98 commented May 8, 2024

Yes, HOMEBREW_ENFORCE_SBOM would be nice as HOMEBREW_DEVELOPER is fairly liberally set in many CIs that don't install gems.

@MikeMcQuaid
Copy link
Member Author

Yes, HOMEBREW_ENFORCE_SBOM would be nice as HOMEBREW_DEVELOPER is fairly liberally set in many CIs that don't install gems.

@Bo98 In this case it doesn't matter because the require? call will just fail. Would you rather the warning message is not printed in that case? What harm is it causing? What is the case/path you're concerned about causing issues with?

On reflection: HOMEBREW_DEVELOPER is definitely the right setting here. It already does stuff like e.g. raises in some places instead of catching the exception, raises deprecation failures rather than warnings, etc.

I'm all for trying to avoid breaking based on actual usage rather than documentation but HOMEBREW_DEVELOPER is something we always have and always will use as the front-line for stricter handing like this.

Library/Homebrew/sbom.rb Outdated Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member Author

One thing I noticed (that might be worth checking here) is that the build time is always a 0 unix timestamp due to our reproducible builds. If you have any ideas for that, let me know. Otherwise maybe we add a TODO: figure out how to record the actual build time

  • We could set this properly for the "there's no SBOM already" case
  • We could put this in the bottle manifest (if we don't already, have a feeling we do) and then use that information to then populate the SBOM from the bottle manifest at pour time without harming reproducibility

Happy to add this on with a ✅ now and I'll add it in tomorrow.

@Bo98
Copy link
Member

Bo98 commented May 8, 2024

Would you rather the warning message is not printed in that case? What harm is it causing? What is the case/path you're concerned about causing issues with?

This + #17255 will lead to warning noise in some of our CI.

Example: https://github.com/Homebrew/brew/blob/master/.github/workflows/actionlint.yml

Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Haven't combed through the code but overall idea looks great. Thanks for this ❤️

@MikeMcQuaid
Copy link
Member Author

This + #17255 will lead to warning noise in some of our CI.

I've adjusted this to avoid printing any warnings when the gem is missing and instead lean in on the HOMEBREW_ENFORCE_SBOM when we want to actually error. Similarly, I've made brew install --build-bottle not attempt to validate the SBOM and instead do it at brew bottle time.

  • We could put this in the bottle manifest (if we don't already, have a feeling we do) and then use that information to then populate the SBOM from the bottle manifest at pour time without harming reproducibility

Did this (but with Tab instead of bottle manifest)

Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

The relocating of functions is making the diff a bit hard to read, but generally this looks good

Library/Homebrew/sbom.rb Show resolved Hide resolved
},
],
externalRefs: [
{
referenceCategory: "PACKAGE-MANAGER",
referenceLocator: "pkg:brew/#{dependency["full_name"]}@#{dependency["version"]}",
referenceLocator: "pkg:brew/#{dependency["full_name"]}@#{dependency["pkg_version"]}",
Copy link
Member

Choose a reason for hiding this comment

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

One thing I see now in testing that I didn't consider before, we might need to URL encode the full name since it can contain @ too. @woodruffw you know the purl spec better. Is pkg:brew/homebrew/core/python@3.12@3.12.3 allowed?

Copy link
Member Author

Choose a reason for hiding this comment

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

@SMillerDev merged this to get the fixes in but: I'll open up a follow-up PR to address this once @woodruffw confirms 👍🏻

- write a schema when installing formulae (if not already present)
- cache the schema on disk rather than downloading it every time
- make more methods/attributes `private`
- allow validation to be optional, only enable for Homebrew developers
  at installation time
- use the tab for more, correct information
- ensure that dependencies/bottles are written correctly
- use new SBOM 3 schema URL
- improve test coverage
@MikeMcQuaid MikeMcQuaid enabled auto-merge May 9, 2024 12:19
@MikeMcQuaid MikeMcQuaid merged commit 6ba8404 into master May 9, 2024
25 checks passed
@MikeMcQuaid MikeMcQuaid deleted the sbom_tweaks branch May 9, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants