fix(sbom): use correct field for licenses in CycloneDX reports

[DmitriyLewen]

Description

We need to use one of three correct field for component license (see #9042 for more details)

Solution

Normalize and check each package license:

1. Single license:
   1.1. valid SPDX ID use licenseChoise.License.ID
   1.2. text license - use licenseChoise.License.Name
   1.3. invalid SPDX ID - use licenseChoise.License.Name
2. SPDX expression:
   1.4. all parts of expression are valid SPDX IDs/SPDX exceptions - use licenseChoise.Expression
   1.5. for other cases use licenseChoise.License.Name

Examples of changes:

CycloneDX report for centos:8:
Before

...
          "license": {
            "name": "GPLv2+"
          }
...
          "license": {
            "name": "Public Domain"
          }
...
      "licenses": [
        {
          "license": {
            "name": "GPLv2+ and LGPLv2+"
          }
        }
...
          "license": {
            "name": "BSD with advertising"
          }

After:

...
          "license": {
            "id": "GPL-2.0-or-later"
          }
...
          "license": {
            "name": "Public-Domain"
          }
...
      "licenses": [
        {
          "expression": "GPL-2.0-or-later AND LGPL-2.0-or-later"
        }
...
          "license": {
            "name": "BSD-3-Clause WITH advertising"
          }

Related issues

• Close bug(cyclonedx): incorrect field for licenses #9042

Checklist

• I've read the guidelines for contributing to this repository.
• I've followed the conventions in the PR title.
• I've added tests that prove my fix is effective or that my feature works.
• I've updated the documentation with the relevant information (if needed).
• I've added usage information (if the PR introduces new options)
• I've included a "before" and "after" example to the description (if the PR is a user interface change).

[knqyf263]

I remember we did something similar for SPDX. If we define a method, can we avoid repeating code in SPDX and CycloneDX?

diff --git a/pkg/licensing/expression/types.go b/pkg/licensing/expression/types.go
index 7399f7462..c237464e1 100644
--- a/pkg/licensing/expression/types.go
+++ b/pkg/licensing/expression/types.go
@@ -14,6 +14,7 @@ var (
 
 type Expression interface {
 	String() string
+	IsSPDXLicense() bool
 }
 
 type Token struct {
@@ -42,6 +43,10 @@ func (s SimpleExpr) String() string {
 	return s.License
 }
 
+func (s SimpleExpr) IsSPDXLicense() bool {
+	return ValidateSPDXLicense(s.String())
+}
+
 type CompoundExpr struct {
 	left        Expression
 	conjunction Token
@@ -49,7 +54,11 @@ type CompoundExpr struct {
 }
 
 func NewCompoundExpr(left Expression, conjunction Token, right Expression) CompoundExpr {
-	return CompoundExpr{left: left, conjunction: conjunction, right: right}
+	return CompoundExpr{
+		left:        left,
+		conjunction: conjunction,
+		right:       right,
+	}
 }
 
 func (c CompoundExpr) Conjunction() Token {
@@ -81,3 +90,11 @@ func (c CompoundExpr) String() string {
 	}
 	return fmt.Sprintf("%s %s %s", left, c.conjunction.literal, right)
 }
+
+func (c CompoundExpr) IsSPDXLicense() bool {
+	if c.conjunction.token == WITH {
+		// e.g. A WITH B
+		return c.left.IsSPDXLicense() && ValidateSPDXException(c.right.String())
+	}
+	return c.left.IsSPDXLicense() && c.right.IsSPDXLicense()
+}

[DmitriyLewen]

I used your logic.
But I'm worried that the new logic is more confusing.

40ea836:
I used IsSPDXExpression function (but it function should check SPDX exception to correct work).
d621fc4:
I optimized logic for SPDX:

• remove exception check for IsSPDXLicense
• don't check nested licenses for valid SPDX expression.

PS expression.Normalize function splits expressions, so exception can be simpeExpr.

[DmitriyLewen]

We can't skip validation for replaceOtherLicenses, because it doesn't work if last license expression is non-SPDX license.
https://github.com/aquasecurity/trivy/actions/runs/15873912882/job/44756852978?pr=9057

So it means that we will overcheck SPDX expressions.
e.g. A or (B and C) license
now we check once each license expression:

1. A or (B and C)
2. A
3. B and C
4. B
5. C

But after changes we will check:

1. A or (B and C) + A, B, C, B and C
2. A
3. (B and C) + B + C
4. B
5. C

[knqyf263]

To be honest, I still don’t quite understand why this commit is necessary. I think the issue with unnecessary recursion could be resolved by simply returning a SimpleExpr.

diff --git a/pkg/sbom/spdx/marshal.go b/pkg/sbom/spdx/marshal.go
index e170beea2..e2aa9860e 100644
--- a/pkg/sbom/spdx/marshal.go
+++ b/pkg/sbom/spdx/marshal.go
@@ -451,7 +451,7 @@ func (m *Marshaler) normalizeLicenses(licenses []string) (string, []*spdx.OtherL
                        if e.IsSPDXExpression() {
                                // Use SimpleExpr for a valid SPDX license with an exception,
                                // to avoid parsing the license and exception separately.
-                               return e
+                               return expression.SimpleExpr{License: e.String()}
                        }
 
                        licenseName = e.String()
diff --git a/pkg/sbom/spdx/marshal_private_test.go b/pkg/sbom/spdx/marshal_private_test.go
index 60d7945ef..26c540d65 100644
--- a/pkg/sbom/spdx/marshal_private_test.go
+++ b/pkg/sbom/spdx/marshal_private_test.go
@@ -70,10 +70,24 @@ func TestMarshaler_normalizeLicenses(t *testing.T) {
                {
                        name: "happy path with WITH operator",
                        input: []string{
-                               "AFL 2.0",
+                               "GPLv2",
+                               "AFL 3.0 with wrong-exceptions",
+                               "LGPL 2.0 and unknown-license and GNU LESSER",
                                "AFL 3.0 with Autoconf-exception-3.0",
                        },
-                       wantLicenseName: "AFL-2.0 AND AFL-3.0 WITH Autoconf-exception-3.0",
+                       wantLicenseName: "GPL-2.0-only AND LicenseRef-51373b28fab165e9 AND LGPL-2.0-only AND LicenseRef-a0bb0951a6dfbdbe AND LGPL-2.1-only AND AFL-3.0 WITH Autoconf-exception-3.0",
+                       wantOtherLicenses: []*spdx.OtherLicense{
+                               {
+                                       LicenseIdentifier: "LicenseRef-51373b28fab165e9",
+                                       LicenseName:       "AFL-3.0 WITH wrong-exceptions",
+                                       ExtractedText:     `This component is licensed under "AFL-3.0 WITH wrong-exceptions"`,
+                               },
+                               {
+                                       LicenseIdentifier: "LicenseRef-a0bb0951a6dfbdbe",
+                                       LicenseName:       "unknown-license",
+                                       ExtractedText:     `This component is licensed under "unknown-license"`,
+                               },
+                       },
                },
                {
                        name: "happy path with non-SPDX exception",

Both the existing tests and the one you added are passing. If there are any cases that are failing, I’d appreciate it if you could share a test or an example.

[knqyf263]

Additionally, I'm unsure why we need a complex logic, like sync.Once, in CycloneDX. Am I missing something?

git diff pkg/sbom/cyclonedx/marshal.go 
diff --git a/pkg/sbom/cyclonedx/marshal.go b/pkg/sbom/cyclonedx/marshal.go
index 9c1e00e71..08314b051 100644
--- a/pkg/sbom/cyclonedx/marshal.go
+++ b/pkg/sbom/cyclonedx/marshal.go
@@ -8,7 +8,6 @@ import (
        "sort"
        "strconv"
        "strings"
-       "sync"
 
        cdx "github.com/CycloneDX/cyclonedx-go"
        "github.com/package-url/packageurl-go"
@@ -312,35 +311,33 @@ func (m *Marshaler) normalizeLicense(license string) cdx.LicenseChoice {
        license = strings.ReplaceAll(license, "-with-", " WITH ")
        license = strings.ReplaceAll(license, "-WITH-", " WITH ")
 
-       var licenseChoice cdx.LicenseChoice
-       onceValidateSPDXLicense := sync.Once{}
-       validateSPDXLicense := func(expr expression.Expression) expression.Expression {
-               // We only need to check the main license (not its parts).
-               onceValidateSPDXLicense.Do(func() {
-                       _, compoundExpr := expr.(expression.CompoundExpr)
-                       switch {
-                       case !expr.IsSPDXExpression():
-                               // Use licenseChoice.license.name for invalid SPDX ID / SPDX expression
-                               licenseChoice.License = &cdx.License{Name: expr.String()}
-                       case compoundExpr:
-                               // Use licenseChoice.expression for valid SPDX expression (with any conjunction)
-                               // e.g. "GPL-2.0 WITH Classpath-exception-2.0" or "GPL-2.0 AND MIT"
-                               licenseChoice.Expression = expr.String()
-                       default:
-                               // Use licenseChoice.license.id for valid SPDX ID
-                               licenseChoice.License = &cdx.License{ID: expr.String()}
-                       }
-               })
-
-               return expr
-       }
-
-       _, err := expression.Normalize(license, licensing.NormalizeLicense, expression.NormalizeForSPDX, validateSPDXLicense)
+       normalizedLicenses, err := expression.Normalize(license, licensing.NormalizeLicense, expression.NormalizeForSPDX)
        if err != nil {
                // Not fail on the invalid license
                m.logger.Warn("Unable to marshal SPDX licenses", log.String("license", license))
                return cdx.LicenseChoice{}
        }
+
+       // Case 1: the license is not a valid SPDX ID or SPDX expression
+       if !normalizedLicenses.IsSPDXExpression() {
+               // Use LicenseChoice.License.Name for invalid SPDX ID / SPDX expression
+               return cdx.LicenseChoice{
+                       License: &cdx.License{Name: normalizedLicenses.String()},
+               }
+       }
+
+       // Case 2: the license is a valid SPDX ID or SPDX expression
+       var licenseChoice cdx.LicenseChoice
+       switch normalizedLicenses.(type) {
+       case expression.SimpleExpr:
+               // Use LicenseChoice.License.ID for valid SPDX ID
+               licenseChoice.License = &cdx.License{ID: normalizedLicenses.String()}
+       case expression.CompoundExpr:
+               // Use LicenseChoice.Expression for valid SPDX expression (with any conjunction)
+               // e.g. "GPL-2.0 WITH Classpath-exception-2.0" or "GPL-2.0 AND MIT"
+               licenseChoice.Expression = normalizedLicenses.String()
+       }
+
        return licenseChoice
 }

[DmitriyLewen]

I think the issue with unnecessary recursion could be resolved by simply returning a SimpleExpr.

I don't know why i didn't think about this...
Thanks for help!

like sync.Once, in CycloneDX

I don't know why i tried to solve this issue in Normalize function 😄
Thanks!

I updated PR - 94e9d07
It should works now

[knqyf263]

Isn't IsSPDXExpression better than IsSPDXLicense? Both a single license and a compound license are also expressions according to the spec.

idstring = 1*(ALPHA / DIGIT / "-" / "." )

license-id = <short form license identifier in Annex A.1>

license-exception-id = <short form license exception identifier in Annex A.2>

license-ref = ["DocumentRef-"(idstring)":"]"LicenseRef-"(idstring)

simple-expression = license-id / license-id"+" / license-ref

compound-expression = (simple-expression /

  simple-expression "WITH" license-exception-id /

  compound-expression "AND" compound-expression /

  compound-expression "OR" compound-expression /

  "(" compound-expression ")" )

license-expression = (simple-expression / compound-expression)

[DmitriyLewen]

I renamed this function because "IsSPDXLicense" was not quite correct.
I don't know why i renamed this function today...
Thanks!

updated in 363074a