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

feat: implement RFC 3553 to add SBOM support #13709

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

justahero
Copy link
Contributor

@justahero justahero commented Apr 5, 2024

What does this PR try to resolve?

This PR is an implementation of RFC 3553 to add support to generate pre-cursor SBOM files for compiled artifacts in Cargo.

How should we test and review this PR?

The RFC 3553 adds a new option to Cargo to emit SBOM pre-cursor files. A project can be configured either by the new Cargo config field sbom.

# .cargo/config.toml
[build]
sbom = true

or using the environment variable CARGO_BUILD_SBOM=true. The sbom option is an unstable feature and requires the -Zsbom flag to enable it.

Check out this branch & compile Cargo. Pick a Cargo project to test it on, then run:

CARGO_BUILD_SBOM=true <path/to/compiled/cargo>/target/debug/cargo build -Zsbom

All generated *.cargo-sbom.json files are located in the target folder alongside their artifacts. To list all generated files use:

find ./target -name "*.cargo-sbom.json"

then check their content. To see the current output format, see these examples.

What does the PR not solve?

The PR leaves a task(s) open that are either out of scope or should be done in a follow-up PRs.

Additional information

There are a few things that I would like to get feedback on, in particular the generated JSON format is not final. Currently it holds the information listed in the RFC 3553, but it could be further enriched with information only available during builds.

During the implementation a number of questions arose.

  • Is using the UnitGraph the right structure to determine all dependencies?
  • Is the location in the compile method to generate the SBOM files appropriate?
  • Is there important information missing from the generated output, some fields that should be omitted?
  • How best to check JSON output in the related tests in testsuite, are useful tests missing?
  • Are custom build script dependencies correctly resolved?

Thanks @arlosi, @RobJellinghaus and @lfrancke for initial guidance & feedback.

@rustbot
Copy link
Collaborator

rustbot commented Apr 5, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-configuration Area: cargo config files and env vars A-unstable Area: nightly unstable support S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2024
@heisen-li
Copy link
Contributor

Much respect for your contribution.

From my kind reminders, it seems appropriate to modify the documentation of the corresponding sections, e.g. Configuration, Environment Variables.

@weihanglo
Copy link
Member

Thanks for the reminder, @heisen-li. Would love to see a doc update, though we should probably focus on the design discussion first, as the location of the configuration is not yet decided. (See rust-lang/rfcs#3553 (comment)).

@epage
Copy link
Contributor

epage commented Apr 9, 2024

One approach for the docs (if this is looking to be merged) is to put the env and config documentation fragments in the Unstable docs.

@justahero justahero force-pushed the rfc3553/cargo-sbom-support branch from 190682e to ae0881c Compare May 2, 2024 19:54
src/cargo/core/compiler/mod.rs Show resolved Hide resolved
src/cargo/core/compiler/build_runner/mod.rs Show resolved Hide resolved
src/cargo/core/compiler/build_runner/mod.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/output_sbom.rs Outdated Show resolved Hide resolved
assert!(p.bin("foo").with_extension("cargo-sbom.json").is_file());
assert_eq!(
1,
p.glob(p.target_debug_dir().join("libfoo.cargo-sbom.json"))
Copy link
Member

Choose a reason for hiding this comment

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

I guess we might need to deal with different naming convention on different platform. (Windows specifically?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the glob call can be simplified in a way that it works for all platforms.

tests/testsuite/sbom.rs Outdated Show resolved Hide resolved
tests/testsuite/sbom.rs Outdated Show resolved Hide resolved
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Just note that I reviewed this as-is, didn't really think too much for the design itself. Thank you for working on this!

src/cargo/core/compiler/build_config.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/build_config.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/output_sbom.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented May 3, 2024

☔ The latest upstream changes (presumably #13571) made this pull request unmergeable. Please resolve the merge conflicts.

@justahero justahero force-pushed the rfc3553/cargo-sbom-support branch 4 times, most recently from 1cfd71a to 376fe1e Compare May 6, 2024 13:42
@rustbot rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label May 6, 2024
@justahero justahero force-pushed the rfc3553/cargo-sbom-support branch 4 times, most recently from 67332d6 to 0aa10e9 Compare May 7, 2024 11:13
@justahero justahero marked this pull request as ready for review May 7, 2024 11:53
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Now I like the idea of having this PR to explore SBOM format. I'll post back issues we've found so far to the RFC. Thank you :)

src/cargo/core/compiler/build_runner/mod.rs Outdated Show resolved Hide resolved
src/doc/src/reference/unstable.md Show resolved Hide resolved
tests/testsuite/sbom.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/output_sbom.rs Show resolved Hide resolved
src/cargo/core/compiler/output_sbom.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/output_sbom.rs Outdated Show resolved Hide resolved
}

#[derive(Serialize)]
struct Sbom {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a dependencies field for this top-level Sbom?

(Just a question. I don't really know if other SBOM formats need it to recover the dependency graph)

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, yes. Copying my comment from the RFC:

Note that there are two ways of looking at dependencies: what each package needs, and the final resolved graph.

For example, if one package depends on rand with features = ["std", "getrandom"], and another with features = ["std", "simd_support"], the final resolved features will be ["std", "getrandom", "simd_support"]. Depending on the use case you may need either or both representations (direct package dependencies and the resolved graph).

cargo metadata exposes both (under "packages" and "resolve" fields), but inaccurately:

I think it would be best for the SBOM to also expose both, accurately this time.

So what I would like to see is two resolved dependency trees: one for normal dependencies and one for build dependencies, matching the way feature resolver v2 works.

src/cargo/core/compiler/output_sbom.rs Outdated Show resolved Hide resolved
tests/testsuite/sbom.rs Outdated Show resolved Hide resolved
tests/testsuite/sbom.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/build_runner/mod.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/build_config.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/output_sbom.rs Show resolved Hide resolved
src/cargo/core/compiler/output_sbom.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/output_sbom.rs Show resolved Hide resolved
src/cargo/core/compiler/output_sbom.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/output_sbom.rs Outdated Show resolved Hide resolved
@rustbot rustbot added the A-testing-cargo-itself Area: cargo's tests label May 28, 2024
@justahero justahero force-pushed the rfc3553/cargo-sbom-support branch 2 times, most recently from 284636f to 1e9f5c7 Compare May 28, 2024 10:24
@bors
Copy link
Contributor

bors commented May 31, 2024

☔ The latest upstream changes (presumably #13992) made this pull request unmergeable. Please resolve the merge conflicts.

@justahero
Copy link
Contributor Author

Rebased the PR.

{
"build_type": "build",
"dependencies": [
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned with what happens when a crate is compiled two different ways under resolver v2. For example, once as a build dependency and once as a normal dependency. In this list of dependencies how would we tell the two entries apart?

Would it be better for the dependencies to be a sequence of indicies into the packages list?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also really like to see a test of this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @arlosi ,

I refactored the code that collects all packages & their dependencies. First all packages are collected, then for each package the indices of its dependencies are determined. It simplifies the serialization code slightly. Would like to get your feedback, if this is what you had in mind.

@bors
Copy link
Contributor

bors commented Jul 4, 2024

☔ The latest upstream changes (presumably #13900) made this pull request unmergeable. Please resolve the merge conflicts.

@justahero justahero force-pushed the rfc3553/cargo-sbom-support branch 3 times, most recently from 0529d01 to c978691 Compare July 12, 2024 09:13
@bors
Copy link
Contributor

bors commented Jul 26, 2024

☔ The latest upstream changes (presumably #13947) made this pull request unmergeable. Please resolve the merge conflicts.

@justahero justahero force-pushed the rfc3553/cargo-sbom-support branch 7 times, most recently from adf4f19 to 0c7f60f Compare August 5, 2024 08:17
@justahero justahero requested a review from arlosi August 5, 2024 13:56
@bors
Copy link
Contributor

bors commented Oct 4, 2024

☔ The latest upstream changes (presumably #14576) made this pull request unmergeable. Please resolve the merge conflicts.

Similar to the generation of `depinfo` files, a function is called to
generated SBOM precursor file named `output_sbom`. It takes the
`BuildRunner` & the current `Unit`. The `sbom` flag can be specified as
a cargo build option, but it's currently not configured fully. To
test the generation the flag is set to `true`.

* use SBOM types to serialize data

Output source, profile & dependencies

Trying to fetch all dependencies

This ignores dependencies for custom build scripts. The output should be
similar to what `cargo tree` reports.

Output package dependencies

This is similar to what the `cargo metadata` command outputs.

Extract logic to fetch sbom output files

This extracts the logic to get the list of SBOM output file paths into
its own function in `BuildRunner` for a given Unit.

Add test file to check sbom output

* add test to check project with bin & lib
* extract sbom config into helper function

Add build type to dependency

Add test to read JSON

Still needs to check output.

Guard sbom logic behind unstable feature

Add test with custom build script

Integrate review feedback

* disable `sbom` config when `-Zsbom` is not passed as unstable option
* refactor tests
* add test

Expand end-to-end tests

This expands the tests to reflect end-to-end tests by comparing the
generated JSON output files with expected strings.

* add test helper to compare actual & expected JSON content
* refactor setup of packages in test

Add 'sbom' section to unstable features doc

Append SBOM file suffix instead of replacing

Instead of replacing the file extension, the `.cargo-sbom.json` suffix
is appended to the output file. This is to keep existing file extensions
in place.

* refactor logic to set `sbom` property from build config
* expand build script related test to check JSON output

Integrate review feedback

* use `PackageIdSpec` instead of only `PackageId` in SBOM output
* change `version` of a dependency to `Option<Version>`
* output `Vec<CrateType>` instead of only the first found crate type
* output rustc workspace wrapper
* update 'warning' string in test using `[WARNING]`
* use `serde_json::to_writer` to serialize SBOM
* set sbom suffix in tests explicitely, instead of using `with_extension`

Output additional fields to JSON

In case a unit's profile differs from the profile information on root
level, it's added to the package information to the JSON output.

The verbose output for `rustc -vV` is also written to the `rustc` field
in the SBOM.

* rename `fetch_packages` to `collect_packages`
* update JSON in tests to include profile information

Add test to check multiple crate types

Add test to check artifact name conflict

Use SbomProfile to wrap Profile type

This adds the `SbomProfile` to convert the existing `Profile` into, to
expose relevant fields. For now it removes the `strip` field, while
serializing all other fields. It should keep the output consistent, even
when fields in the `Profile` change, e.g. new field added.

Document package profile

* only export `profile` field in case it differs from root profile

Add test to check different features

The added test uses a crate with multiple features. The main crate uses
the dependency in the normal build & the custom build script with
different features.

Refactor storing of package dependencies

All dependencies for a package are indices into the `packages` list now.
This sets the correct association between a dependency & its associated
package.

* remove `SbomDependency` struct

Refactor tests to use snapbox
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation A-testing-cargo-itself Area: cargo's tests A-unstable Area: nightly unstable support S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants