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

Add support for custom external references #428

Merged
merged 4 commits into from
Jan 15, 2024
Merged

Conversation

vy
Copy link
Contributor

@vy vy commented Nov 10, 2023

This PR addresses #421 and adds the configured external references into two different places of the generated SBOM:

  1. $.metadata.component.externalReferences
  2. $.components[].externalReferences (only for $.components contained by the reactor)

While (1) only requires the basic information about the current Maven module that is being processed, (2) was pretty tricky to get right. For (2), the parent module needs to obtain the external references configured for child modules. Hence, using the value injected into the

@Parameter
private ExternalReference[] externalReferences;

field of the mojo is of no use there. In DefaultModelConverter#convert(MojoExecution, Artifact, CycloneDxSchema.Version, boolean), I locate the matching plugin and its matching execution, and merge the configuration of the two. Notice the .distinct() during this merge – without that, merged external references contain duplicates. My suspicion is getEffectiveMavenProject() doesn't always return the effective POM.

@garydgregory
Copy link

FYI, this issues Apache Log4j.

@hboutemy
Copy link
Contributor

@vy do you really need to be able to customize external references in each child module? defining a value in the parent is not sufficient?

I understand the theory of this, but implementing it means that this plugins' code is doing Maven job in parallel to Maven (= mapping plugin execution configuration to objects): this is complex and fragile

I locate the matching plugin and its matching execution, and merge the configuration of the two. Notice the .distinct() during this merge – without that, merged external references contain duplicates. My suspicion is getEffectiveMavenProject() doesn't always return the effective POM.

your suspicion is not exactly right: the effective POM is the effective POM: it's your merge that does not mimic real Maven way of merging configuration from each level of effective Maven POM = exactly what I told before, you are trying to reimplement a Maven mechanism, that is risky

@garydgregory
Copy link

CC: @ppkarwasz

@ppkarwasz
Copy link
Contributor

I believe the makeAggregateBom goal logic would become much simpler if the execution order was:

  1. the makeBom goal is executed for all the modules and the SBOMs artifacts are attached to them,
  2. the root project just merges the SBOMs of all its sub-modules.

Unfortunately I have no idea on how to implement this in Maven. @hboutemy any suggestions?

The current order of execution is problematic, since the root project must do the same work as its submodules, including the resolution of the CycloneDX configuration of submodules.

@vy
Copy link
Contributor Author

vy commented Dec 30, 2023

@vy do you really need to be able to customize external references in each child module? defining a value in the parent is not sufficient?

I think it is indeed a good idea to re-evaluate our assumptions. @stevespringett, you were the major lead in hinting us to place the VDR/VEX references in the following two places:

  1. $.metadata.component.externalReferences
  2. $.components[].externalReferences (only for $.components contained by the reactor)

@stevespringett, can we live with only (1)? What would be the cons of dropping (2)?

I understand the theory of this, but implementing it means that this plugins' code is doing Maven job in parallel to Maven (= mapping plugin execution configuration to objects): this is complex and fragile

@hboutemy, can we explicitly document this caveat of this feature and see how far we can go? Maybe even put a disclaimer stating the feature is experimental?

@stevespringett
Copy link
Member

  1. $.metadata.component.externalReferences
  2. $.components[].externalReferences (only for $.components contained by the reactor)

@stevespringett, can we live with only (1)? What would be the cons of dropping (2)?

Granularity would be the only con that I can see. By limiting to $.metadata.component.externalReferences, the parent and all modules would be able to specify additional external references, but would not be able to specify them for any component dependencies they may include.

If the assumption is that a supplier or open source project (e.g. Apache Logging) wants to specify external references for VDR/VEX, then limiting to the first option should have no impact on their ability to do so.

@hboutemy
Copy link
Contributor

hboutemy commented Jan 4, 2024

@ppkarwasz

I believe the makeAggregateBom goal logic would become much simpler if the execution order was:

Yes, but it's not what Maven does currently: I finally opened the Maven Jira issue that I wanted to open for a few years to better document the current issues and future solutions https://issues.apache.org/jira/browse/MNG-7991

@vy @stevespringett

Granularity would be the only con that I can see. By limiting to $.metadata.component.externalReferences, the parent and all modules would be able to specify additional external references, but would not be able to specify them for any component dependencies they may include.

to me, adding external references makes only sense for supplier on what he provides: I don't see why a supplier would add references for his dependencies, given it would be specific only to his own use of the dependencies

If the assumption is that a supplier or open source project (e.g. Apache Logging) wants to specify external references for VDR/VEX, then limiting to the first option should have no impact on their ability to do so.

I think this is what the current Apache Logging case is about, and a very good step forward

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

Except the major API change, this looks good to me.

src/main/java/org/cyclonedx/maven/ModelConverter.java Outdated Show resolved Hide resolved
@vy
Copy link
Contributor Author

vy commented Jan 5, 2024

@hboutemy, I have updated the branch to only contain the $.metadata.component.externalReferences enrichment. I personally prefer the initial version, but I trust your judgement. Let me know if there is anything further I need to provide.

Copy link
Contributor

@hboutemy hboutemy left a comment

Choose a reason for hiding this comment

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

I can merge the PR and do the changes after, but it's better to not do the unwanted changes for dependency artifacts

@vy
Copy link
Contributor Author

vy commented Jan 15, 2024

I can merge the PR and do the changes after, but it's better to not do the unwanted changes for dependency artifacts

@hboutemy, I have addressed your remarks. Would you mind checking again, please?

@hboutemy hboutemy merged commit 8836cbd into CycloneDX:master Jan 15, 2024
3 of 4 checks passed
@hboutemy
Copy link
Contributor

thanks a lot @vy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants