Implement SPDX expressions #2400

hborchardt · 2023-01-21T18:29:01Z

Description

Hi, I am very interested in seeing SPDX expression support in dependency-track.

I implemented a SPDX expression parser and reimplemented the license group policy evaluator and license policy evaluator logic.
It also implements the logic for the policy operator ALL, where previously violations were added if any policy condition is violated, regardless of the choice of the policy operator.

Addressed Issue

Addresses #170

Additional Details

The parser is based on the shunting yard algorithm for infix operators and parentheses.

AND and OR operators work as expected.
WITH operator works by converting GPL-2.0 WITH classpath-exception into a GPL-2.0-with-classpath-exception license, which is available in the default license list
+ operator is interpreted as GPL-2.0+ being equivalent to GPL-2.0 OR GPL-2.0-or-later

I store the SPDX expression in the license field of the Component. It currently has a maximum length of 255 characters - should this be increased?
If a SPDX expression is available in the CycloneDX BOM, it is preferred over the content of the licenses info. If a resolvedLicense is found, the resolved license takes precedence over the SPDX expression.

I was also looking into making the SPDX expression tree information available to the frontend for visualisation - currently it is just parsed on-demand upon policy validation - but I did not find a reference of how such a "derived/computed" property of a model class would be made available to the frontend.

As this is my first time contributing to dependency-track, I am very open for criticism and comments of any sort.

Checklist

I have read and understand the contributing guidelines
~~This PR fixes a defect, and I have provided tests to verify that the fix is effective~~
This PR implements an enhancement, and I have provided tests to verify that it works as intended
This PR introduces changes to the database model, and I have added corresponding update logic
This PR introduces new or alters existing behavior, and I have updated the documentation accordingly

src/main/java/org/dependencytrack/policy/LicensePolicyEvaluator.java

sonatype-lift · 2023-01-21T18:39:51Z

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.¹

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/DependencyTrack/dependency-track/2400.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/DependencyTrack/dependency-track/2400.diff | git apply

Once you're satisfied commit and push your changes in your project.

You can preview the patch by opening the patch URL in the browser. ↩

src/main/java/org/dependencytrack/parser/spdx/expression/model/SpdxOperator.java

hborchardt · 2023-02-20T11:36:21Z

I realized that the previous interpretation of the WITH operator as an AND operator does not make sense - it makes it hard to disallow GPL-2.0 but allow GPL-2.0 WITH classpath-exception. So I changed the implementation to interpret GPL-2.0 WITH classpath-exception as GPL-2.0-with-classpath-exception

src/main/java/org/dependencytrack/policy/LicenseGroupPolicyEvaluator.java

nscuro

Thanks for the PR @hborchardt! 🙌

Beside the other comments, I think it would be beneficial to have some documentation that explains how the policy evaluation works with license expressions.

IS and IS_NOT are straightforward when used with licenses or license groups, but expressions add the dimension of license compatibility into the mix, which is not as easy to grasp.

I have to admit I struggled to understand how it's working, so I assume it will be even more of a problem for non-technical users.

src/main/java/org/dependencytrack/parser/spdx/expression/SpdxExpressionParser.java

src/test/java/org/dependencytrack/parser/spdx/expression/SpdxExpressionParserTest.java

nscuro · 2023-03-04T16:14:47Z

src/main/java/org/dependencytrack/parser/cyclonedx/util/ModelConverter.java

-                            component.setResolvedLicense(license);
+        if (licenseChoice != null) {
+            if (licenseChoice.getExpression() != null) {
+                component.setLicense(licenseChoice.getExpression());


Reusing the license field here has the downside that we can't differentiate between license name and license expression when exporting BOMs again. For example, when using the "inventory" export with a project that has components that have license expressions, the export will show:

"licenses" : [ { "license" : { "name" : "LGPL-2.1-only WITH CPE AND MIT OR BSD-3-Clause" } } ],

Introducing a separate licenseExpression field to the Component class would help to disambiguate.

As you pointed out in the PR description, a limit of 255 characters may not be able to fit all possible expressions. If you end up adding a new field, you could define the jdbcType as CLOB (translates to TEXT or MEDIUMTEXT, depending on the database) instead, which does not have a character limit.

See here for an example of a CLOB field:

dependency-track/src/main/java/org/dependencytrack/model/Analysis.java

Lines 85 to 88 in 93321f8

@Persistent(defaultFetchGroup = "true")

@Column(name = "DETAILS", jdbcType = "CLOB", allowsNull = "true")

@NotNull

private String analysisDetails;

Also note that the frontend should be updated to show the expression in the Component Details view:

I will then create a PR in the frontend project for this.

src/test/java/org/dependencytrack/parser/spdx/expression/SpdxExpressionParserTest.java

nscuro · 2023-03-04T17:29:50Z

src/main/java/org/dependencytrack/policy/LicenseGroupPolicyEvaluator.java

+        if (policy.getOperator() == Operator.ALL) {
+            if (allPoliciesViolated) {
+                return violations;
+            } else {
+                // discard found violations, because not all conditions had violations
+                return List.of();
+            }
+        } else {
+            // Operator.ANY means just give all found violations
+            return violations;
+        }


Policy operators are evaluated by PolicyEngine, and should not be re-used by condition evaluators. There have been multiple improvements to this recently, e.g. in #2336 and #2460.

I think the linked PRs fixed the problem you referred to in the PR description:

It also implements the logic for the policy operator ALL, where previously violations were added if any policy condition is violated, regardless of the choice of the policy operator.

Can you confirm this is the case?

Yes it looks like those PRs address this in a more general way, which is nice. I haven't extensively tested them, but the logic seems sound (as long as one evaluator.evaluate(...) invocation does not return more than one violation per condition - which is probably a reasonable assumption, but not totally obvious)

nscuro · 2023-03-04T17:56:16Z

src/main/java/org/dependencytrack/policy/LicenseGroupPolicyEvaluator.java

+        boolean hasViolations = false;
+        if (condition.getOperator() == PolicyCondition.Operator.IS) {
+            // call canLicenseBeUsed with inverted logic
+            if (!canLicenseBeUsed(qm, expression, LicenseGroupType.NegativeList, lg)) {


This section is very hard to understand. What is license in canLicenseBeUsed, given that both expression and lg can contain multiple licenses? What exactly is the meaning of NegativeList vs. PositiveList?

Reading it as:

if expression is incompatible with [licenses in lg]

makes sense, but the NegativeList part is throwing me off.

Any chance we can improve the wording here, so that it becomes a bit more obvious what's happening?

I changed the wording to ForbiddenLicenses and AllowedLicenses, which hopefully better captures the intent. Also added a section to the docs, explaining the details on how to use it and set it up, and what it does.

hborchardt · 2023-03-22T21:40:14Z

Thanks for the feedback - I think I managed to address all of it

msymons · 2023-04-25T11:01:22Z

@hborchardt: following the release of v4.8.0, this PR now has conflicts that need to be resolved before it can be merged. Please can you fix things? Then the PR can be reviewed.

melba-lopez · 2023-06-27T16:46:19Z

@hborchardt are you able to update this PR to not have a branch conflict?

Also, @nscuro do we want to consider (maybe in the future) incorporating CycloneDX CLI to convert SPDX to CDX so that we let that handle the import versus creating something new?

melba-lopez · 2023-07-28T19:57:04Z

@hborchardt can you update your branch? this is slated for the 4.9 release and would like for your contributions to be included!

hborchardt · 2023-07-29T08:56:01Z

Hi, just yesterday I was updating the branch accordingly, but I have to redo it now... But you can expect an updated branch in the next days

Implement a shunting yard algorithm to process the infix operators and parentheses in the SPDX expression grammar. Parses an expression into a tree of SpdxExpression/SpdxExpressionOperation objects. Signed-off-by: Hendrik Borchardt <[email protected]>

Store it when importing a cyclonedx BOM, and export it when creating one. Use it for resolving the license from the database, when the expression is just one license id. Signed-off-by: Hendrik Borchardt <[email protected]>

Recursively check license group compatibility with SpdxExpression, according to the SpdxExpressionOperations and whether the PolicyCondition specifies a positive list or negative list of licenses. As a special case, a `null` license group is interpreted as "unresolved" licenses, to be used by the LicensePolicyEvaluator. Signed-off-by: Hendrik Borchardt <[email protected]>

A license policy should be the same as a LicenseGroup policy where the group contains just one license. Also handle the case of "unresolved" keyword as condition value, for referring to unresolved licenses. Signed-off-by: Hendrik Borchardt <[email protected]>

Signed-off-by: Hendrik Borchardt <[email protected]>

nscuro

Thanks so much for being patient with us @hborchardt, apologies for letting you wait for so long.

I think this is a really cool addition, I am sure the SpdxExpressionParser will come in handy in multiple other areas as well.

While testing, I found a few more gaps:

licenseExpression is not considered when creating or updating Components in ComponentResource, such that initially setting or modifying an expression was not possible
For the frontend, the licenseExpression field was not sent to the API server as part of the update request
As noted in a comment, the manual DB schema migration is not necessary

Additionally, thanks to your parser, we can detect invalid expressions. This is super useful and allows us to prevent invalid expressions from entering the system. Validation of expressions should be performed upon BOM ingestion, and manual creation / updating of components via REST API.

I already made those changes and will push them after merging your PRs.

nscuro · 2023-08-19T19:22:32Z

src/main/java/org/dependencytrack/upgrade/v490/v490Updater.java

+    private void addLicenseExpressionColumnToComponents(Connection connection) throws Exception {
+        // The JDBC type "CLOB" is mapped to the type CLOB for H2, MEDIUMTEXT for MySQL, and TEXT for PostgreSQL and SQL Server.
+        LOGGER.info("Adding \"LICENSE_EXPRESSION\" to \"COMPONENTS\"");
+        if (DbUtil.isH2()) {
+            DbUtil.executeUpdate(connection, "ALTER TABLE \"COMPONENTS\" ADD \"LICENSE_EXPRESSION\" CLOB");
+        } else if (DbUtil.isMysql()) {
+            DbUtil.executeUpdate(connection, "ALTER TABLE \"COMPONENTS\" ADD \"LICENSE_EXPRESSION\" MEDIUMTEXT");
+        } else {
+            DbUtil.executeUpdate(connection, "ALTER TABLE \"COMPONENTS\" ADD \"LICENSE_EXPRESSION\" TEXT");
+        }
+    }


Just a note, non-intrusive schema changes like this one are completely taken care of by the ORM, so there's no manual migration needed. The intent is very much appreciated though, thanks for having the schema change in mind!

sonatype-lift bot reviewed Jan 21, 2023

View reviewed changes

src/main/java/org/dependencytrack/policy/LicensePolicyEvaluator.java Outdated Show resolved Hide resolved

hborchardt force-pushed the spdx-expressions branch 2 times, most recently from 09f512f to ee68704 Compare January 21, 2023 19:29

sonatype-lift bot reviewed Jan 21, 2023

View reviewed changes

src/main/java/org/dependencytrack/parser/spdx/expression/model/SpdxOperator.java Outdated Show resolved Hide resolved

sonatype-lift bot reviewed Jan 21, 2023

View reviewed changes

src/main/java/org/dependencytrack/parser/spdx/expression/model/SpdxOperator.java Outdated Show resolved Hide resolved

hborchardt force-pushed the spdx-expressions branch from ee68704 to a391dca Compare January 21, 2023 19:48

hborchardt force-pushed the spdx-expressions branch from a391dca to fc67ca0 Compare February 20, 2023 11:30

hborchardt force-pushed the spdx-expressions branch from fc67ca0 to ac5fb8e Compare February 20, 2023 15:18

sonatype-lift bot reviewed Feb 20, 2023

View reviewed changes

src/main/java/org/dependencytrack/policy/LicenseGroupPolicyEvaluator.java Outdated Show resolved Hide resolved

hborchardt force-pushed the spdx-expressions branch from ac5fb8e to c50b506 Compare February 20, 2023 18:37

nscuro mentioned this pull request Mar 4, 2023

deps.dev License enricher for SBOMs smithy-security/smithy#15

Merged

nscuro reviewed Mar 4, 2023

View reviewed changes

hborchardt force-pushed the spdx-expressions branch from c50b506 to c556a0f Compare March 22, 2023 21:23

hborchardt mentioned this pull request Mar 22, 2023

Add SPDX expression field to component dialog and list DependencyTrack/frontend#452

Merged

2 tasks

nscuro added this to the 4.9 milestone Apr 10, 2023

hborchardt force-pushed the spdx-expressions branch from c556a0f to b0140a3 Compare April 25, 2023 19:36

melba-lopez added the enhancement New feature or request label Jul 28, 2023

melba-lopez added the pending more information label Jul 28, 2023

nscuro mentioned this pull request Aug 1, 2023

CycloneDX SBOM files generated by trivy are not compliant with corresponding CycloneDX schema aquasecurity/trivy#4900

Closed

2 tasks

hborchardt added 4 commits August 5, 2023 09:17

Add SPDX expression parser

536ea6d

Implement a shunting yard algorithm to process the infix operators and parentheses in the SPDX expression grammar. Parses an expression into a tree of SpdxExpression/SpdxExpressionOperation objects. Signed-off-by: Hendrik Borchardt <[email protected]>

Add SPDX expression field in Component

51f651c

Store it when importing a cyclonedx BOM, and export it when creating one. Use it for resolving the license from the database, when the expression is just one license id. Signed-off-by: Hendrik Borchardt <[email protected]>

Add documentation for license violation policies

fbdf757

Signed-off-by: Hendrik Borchardt <[email protected]>

hborchardt force-pushed the spdx-expressions branch from b0140a3 to fbdf757 Compare August 5, 2023 21:55

nscuro removed the pending more information label Aug 14, 2023

nscuro self-assigned this Aug 14, 2023

nscuro approved these changes Aug 19, 2023

View reviewed changes

nscuro merged commit f4bace4 into DependencyTrack:master Aug 19, 2023
7 checks passed

nscuro mentioned this pull request Aug 19, 2023

SPDX expression support improvements #2965

Merged

2 tasks

nscuro removed their assignment Aug 19, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SPDX expressions #2400

Implement SPDX expressions #2400

hborchardt commented Jan 21, 2023 •

edited

Loading

sonatype-lift bot commented Jan 21, 2023

hborchardt commented Feb 20, 2023

nscuro left a comment

nscuro Mar 4, 2023

hborchardt Mar 22, 2023

nscuro Mar 4, 2023

hborchardt Mar 22, 2023

nscuro Mar 4, 2023

hborchardt Mar 22, 2023

hborchardt commented Mar 22, 2023

msymons commented Apr 25, 2023

melba-lopez commented Jun 27, 2023

melba-lopez commented Jul 28, 2023

hborchardt commented Jul 29, 2023

nscuro left a comment

nscuro Aug 19, 2023

	@Persistent(defaultFetchGroup = "true")
	@Column(name = "DETAILS", jdbcType = "CLOB", allowsNull = "true")
	@NotNull
	private String analysisDetails;

Implement SPDX expressions #2400

Implement SPDX expressions #2400

Conversation

hborchardt commented Jan 21, 2023 • edited Loading

Description

Addressed Issue

Additional Details

Checklist

sonatype-lift bot commented Jan 21, 2023

🛠 Lift Auto-fix

Footnotes

hborchardt commented Feb 20, 2023

nscuro left a comment

Choose a reason for hiding this comment

nscuro Mar 4, 2023

Choose a reason for hiding this comment

hborchardt Mar 22, 2023

Choose a reason for hiding this comment

nscuro Mar 4, 2023

Choose a reason for hiding this comment

hborchardt Mar 22, 2023

Choose a reason for hiding this comment

nscuro Mar 4, 2023

Choose a reason for hiding this comment

hborchardt Mar 22, 2023

Choose a reason for hiding this comment

hborchardt commented Mar 22, 2023

msymons commented Apr 25, 2023

melba-lopez commented Jun 27, 2023

melba-lopez commented Jul 28, 2023

hborchardt commented Jul 29, 2023

nscuro left a comment

Choose a reason for hiding this comment

nscuro Aug 19, 2023

Choose a reason for hiding this comment

hborchardt commented Jan 21, 2023 •

edited

Loading