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

Added configurable reference generation between the components of a multi-language SBOM #1567

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

malice00
Copy link
Contributor

When generating a multi-language SBOM, the first generated sub-BOM's parentComponent is used as the parent for the mixed SBOM -- but there are no references between the different sub-BOMs.
This fix adds references to all remaining sub-BOMs as a 'provides'-entries. It is also possible to add them as 'dependsOn', so that tools can actually show all components in the dependency-tree -- I noticed that eg Dependency-Track doesn't show references in 'provides'.

Small included change: GRADLE_XXX EnvVars in documentation grouped together, a duplicate EnvVar removed from docs.

@malice00 malice00 requested a review from prabhu as a code owner January 15, 2025 23:37
@prabhu
Copy link
Collaborator

prabhu commented Jan 16, 2025

Cool idea!

docs/ENV.md Outdated
@@ -6,6 +6,7 @@ The following environment variables are available to configure the bom generatio
| ------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| CDXGEN_DEBUG_MODE | Set to `debug` to enable debug messages |
| GITHUB_TOKEN | Specify GitHub token to prevent traffic shaping while querying license and repo information |
| MULTI_BOM_COMPONENT_REF | When building a multi-language BOM, choose how the references between the components is handled. This can be useful if other tooling used to eg visualize the dependency-tree only uses 'dependsOn' (as the name dependency-tree more or less implies). Possible values: `dependsOn`, `provides`. Default: `provides`. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work with our configuration files? .cdxgenrc etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know, I didn't know there are config files! 😄 I'll try it and let you know what I find/try to fix it if it doen't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prabhu Is it currently possible to put EnvVars in the config files? I can't seem to find any documentation on that, or in yargs for that matter... Or is this a feature that needs to be added?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. I think only command line arguments can be passed via config files. Do we need this variable or can we just assume dependsOn ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's already lots of EnvVar-configs, so we could maybe leave it like that? Or add it as a cli argument.
Maybe it would be good to clean that up a bit at some point and have both support all current args and EnvVars?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant, do we need this new variable MULTI_BOM_COMPONENT_REF at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily, I just thought it would be good the have the option to choose. I initially didn't have it, then decided to change the default to provides, since this made more sense to me.
I'll remove it and set it to dependsOn; We can always add something if a request is made. 😄

lib/cli/index.js Outdated
const multiBomComponentRef =
"dependsOn" === process.env.MULTI_BOM_COMPONENT_REF
? "dependsOn"
: "provides";
Copy link
Collaborator

@prabhu prabhu Jan 16, 2025

Choose a reason for hiding this comment

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

provides is only useful for CBOMs at this point. We can use default of dependsOn?

Copy link
Contributor Author

@malice00 malice00 Jan 16, 2025

Choose a reason for hiding this comment

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

I didn't realize that. I figured that logically it should not be a 'dependsOn' relation, my personal use however will be that every time because of the dependency-tree in Dependency-Track. So yeah, I don't mind turning the default around!

? "dependsOn"
: "provides";
let parentDependencies = dependencies.find(
(d) => d["ref"] === parentComponent["bom-ref"],
Copy link
Collaborator

@prabhu prabhu Jan 16, 2025

Choose a reason for hiding this comment

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

Should we handle the case, where this could match the child of the parent component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure if I follow you here... bom-refs should be unique, right? So in what case would this match a child component?

Copy link
Collaborator

Choose a reason for hiding this comment

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

parentComponent could have a list of components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is actually already handled in 31c082d!

@prabhu
Copy link
Collaborator

prabhu commented Jan 16, 2025

Is it possible to add a repo test to demonstrate this useful feature? I will fix the current repo tests failures in a different branch.

@malice00
Copy link
Contributor Author

Is it possible to add a repo test to demonstrate this useful feature? I will fix the current repo tests failures in a different branch.

That should definitely be possible, my testing-project is already part of the repotests anyway. I'll add some tests in later today.

@prabhu
Copy link
Collaborator

prabhu commented Jan 16, 2025

Can you also rebase. The repotests are fixed now.

@malice00 malice00 marked this pull request as draft January 16, 2025 15:13
@malice00
Copy link
Contributor Author

All discussed issues should be included, now let's hope all tests work... 😉

@malice00
Copy link
Contributor Author

Darn, more commits were added. after I last pulled! Another rebase was done... 😄

@malice00 malice00 marked this pull request as ready for review January 17, 2025 08:49
Copy link
Collaborator

@prabhu prabhu left a comment

Choose a reason for hiding this comment

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

Thank you for your patience and support!

@prabhu prabhu merged commit c41dfe0 into CycloneDX:master Jan 17, 2025
20 checks passed
@malice00 malice00 deleted the multi_bom_ref branch January 17, 2025 22:44
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.

2 participants