Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC: cargo-sbom #3553
base: master
Are you sure you want to change the base?
RFC: cargo-sbom #3553
Changes from 1 commit
9462726
90ec02c
97784a7
7c12ee3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we aim to participate in such schemes does that impose any new burdens on the project. E.g. would be inaccurate reports be considered a critical issue because it could possibly let security issues go undetected?
Would, if individual EU countries implement directives on a national level which use more expansive wording that imposes additional requirements that cargo does not fulfill, that become a priority because we committed to providing "useful" SBOMs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As-is, we aren't providing the final SBOM artifact but information that can feed into it. This intentionally leaves a lot of that information to the caller to get.
I expect the mix of end user and regulatory requirements to contradict (they already were in the Pre-RFC thread) which is why I'd want stricken from the RFC a future possibility of providing a final SBOM. We likely can't keep up, we likely can't maintain the compatibility requirements, and we likely can't satisfy them without knobs for everything.
The fun of "fit for use".
We'll be providing a report of what information we have. There is more information, like from build scripts linking external libraries, that we can't provide. The usefulness of any of this is dependent on all parties involved cooperating.
That said, for what we do provide, if there is a bug, does it need a CVE? Unsure? I'd personally just consult the security folks when it happens. This is less about direct attacks and more about the quality of monitoring.
I do wonder if this would be useful as a more general unit-graph report (which it isn't far from). For example, watch tools could use this information to know what changes to watch for for future builds. That might ease some of the pressure on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to clarify in advance how much we're promising here. I suspect that down the road some large institutional users will start relying heavily on SBOMs and make noises when it's not working as they want it to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should we actually call this?
And is this a build param or a profile param?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought a
build
parameter made most sense, since there's 1 place to easily control it.CARGO_BUILD_SBOM=true
If we use profiles, then it becomes harder for tooling wrapping Cargo to unconditionally enable it for the current run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Profile does not preclude environment variables because the manifest profile is layered with the config profile.
But it looks like the layering is all-or-nothing so setting one value might be ignored or cause other values to be ignored. That might be too disruptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should capture this reasoning within the RFC's rationale section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for having this in a profile, because I very much expect people will want to have this in some profiles and not in others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Profile for now is a set of compiler settings. I am not sure if we want to expand the meaning of it to include SBOM. Also given SBOM is only meaningful to final artifacts, if we put it in profiles we need to document
profile.debug.sbom
not working for dependencies. We already have some confusions forlto
andabort
(rust-lang/cargo#9330).If people want to switch build configurations, should we work on stabilizing
config-include
instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the SBOM precursor file will be written first, is there an intention to remove it if the production of the artifact, including perhaps the execution of any
RUSTC_WORKSPACE_WRAPPER
, fails?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on this. Do you think it needs to be specified in the RFC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we uniquely identify one of several crates / build units within a package?
The main situations for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the checksum? For third-party SBOM formats, I would instead encourage them to own the checksum generation and worry people will reuse this and put their own expectations on what this means
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This text makes me wonder if "Checksum" is trying to capture version information for dependencies taken from a repository. Maybe "Checksum" and "Version" could be merged, so "Version" is the git sha when using git as your source for a crate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the challenge with replacing "version" entirely with "checksum" is that a
RUSTC_WORKSPACE_WRAPPER
might want to consume the version information to do their own logic, so it's not just useful for validation purposes (which is what the checksum provides).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the git case the sha is already part of the
source
, an example fromcargo metadata
aftercargo add futures --git=https://github.com/rust-lang/futures-rs
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these details need to be written to the sbom, or could they just be queried from
cargo metadata
based on the Id as is proposed for other information like the license?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there is one detail here that is not available from
cargo metadata
: the checksum. (But as has been mentioned elsewhere, this checksum may not be that useful, instead tools may want to be following themanifest_path
from the metadata and generating their own source file hash from that).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include env variables (I assume rustc reports to us what it read for us to fingerprint) or file paths (again, I assume rustc tells us what it read to fingerprint)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rustc mentions all read env vars in depinfo. This does not include env vars read by proc macros through std::env::var.
Depinfo also contains the read files, but as far as I'm aware, the standard library sources aren't included yet (-Zbinary-dep-depinfo enables that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this can also be moved to a future possibility so long as we ensure the format can support this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NTIA released Minimum Elements for a SBOM. Currently, mainstream SBOM formats (e.g
SPDX
,CycloneDX
,SWID
) are all extend from this minimum elements and add some optional fields.May be we can also set the format align with the NITA minimum elements, like add
author
fieldThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RFC 3052 intentionally made the author field optional.
Any SBOM initiative that can't cope with anonymously/pseudonymously authored code is overreach imo. The code can be perfectly viable. And it doesn't change the fact that authors can vanish or give false contact information anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As is mentioned below this section, you can query other metadata by looking up the dependency in
cargo metadata
based on the id, if the crate has published author metadata it will be available there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what you mean. Maybe we're talking about two dimensions, and you're right from a security point of view. Simply from the point of view of this being a tool, providing the author field may be a better way for developers to use this tool to generate SBOM. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is any other
config
relevant to include?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should we defer out all config to a future possibility so long as we make sure the format can support it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should call out that its not just SBOM format but also being compliant with internal and external regulations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not seeing what text resolved this so unresolving it