feat(license): improve license normalization #7131

pbaumard · 2024-07-09T15:04:08Z

Description

Normalize "+", "-only" and "-or-later" suffixes
Many new mappings from oss-review-toolkit
Space normalization, including newlines
Normalize US / UK spelling for license / licence.
Remove "THE " prefix
Remove common suffixes: " LICENSE", " LICENSED", -"LICENSE" and "-LICENSED"
Add MIT-0
Add tests

Related issues

Close feat(license): Improve license normalization #7189

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).

- Remove "THE " prefix - Remove "LICENSE " suffix - Fix lowercase key - Add BSD LICENSE 3 - Add ECLIPSE PUBLIC LICENSE 1.0 - Add MIT-0 - Add tests

CLAassistant · 2024-07-09T15:04:14Z

All committers have signed the CLA.

CLAassistant · 2024-07-09T15:04:14Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

DmitriyLewen

Hello @pbaumard
Thanks for your work!

LGTM.
Can you fix tests?

pkg/licensing/normalize_test.go

pkg/licensing/normalize.go

DmitriyLewen

Hello @pbaumard
Sorry for delay.

Left comments. Take a look when you have time.

pkg/licensing/normalize.go

DmitriyLewen · 2024-07-24T09:48:01Z

pkg/licensing/normalize.go

+var mapping = make(map[string]expression.SimpleExpr)
+
+func addMap(name, key string, hasPlus bool) {
+	license := normalizeKeyAndSuffix(name)


IIUC licenses are already normalized.
Do we need this?

The license from normalizeKeyAndSuffix is only used for the following assertion with the panic error.

I added some comemnts to make it more explicit.

The check could be made in unit test but it means having mapping public.

I think I understand your idea - you want to check the newly added licenses (from another PR later).

I think we don't need to do this check every time we start Trivy.

The test can be done in a unit test, but this means that the mapping will be public.

You have added so many licenses, so I am not sure that many licenses will be added later.
Maybe a comment before mapping to the instructions for the new licenses will be enough. wdyt?

Especially with latest commit with the version regular expression, it's becoming quite difficult to make sure to only add standardized keys.

I added an InvalidMappingKeys used in test and which might also be used later if adding new mappings from CLI or other mean becomes possible.

DmitriyLewen · 2024-07-24T10:11:23Z

pkg/sbom/cyclonedx/marshal.go

@@ -288,7 +290,7 @@ func (*Marshaler) Licenses(licenses []string) *cdx.Licenses {
 	choices := lo.Map(licenses, func(license string, i int) cdx.LicenseChoice {
 		return cdx.LicenseChoice{
 			License: &cdx.License{
-				Name: license,
+				Name: NormalizeLicense(license),


Can we use this function here?

trivy/pkg/licensing/normalize.go

Lines 735 to 737 in 907683d

func Normalize(name string) string {

return NormalizeLicense(name).String()

}

I removed the CycloneDX normalization from this PR to reduce the scope.

From cycloneDX spec that would ideally mean having :

Id: "A valid SPDX license ID"

Name: "If SPDX does not define the license used, this field may be used to provide the license name",

integration/testdata/fluentd-multiple-lockfiles.cdx.json.golden

DmitriyLewen · 2024-07-24T10:36:50Z

pkg/fanal/test/integration/testdata/goldens/packages/amazon-2.json.golden

We do not check all fields for OS packages:

trivy/pkg/fanal/test/integration/library_test.go

Lines 321 to 324 in 051ac39

for i := 0; i < len(expectedPkgs); i++ {

require.Equal(t, expectedPkgs[i].Name, detail.Packages[i].Name, tc.name)

require.Equal(t, expectedPkgs[i].Version, detail.Packages[i].Version, tc.name)

}

Therefore, there is no point in updating the OS packages for the files pkg/fanal/test/integration/testdata/goldens/packages/*.json.golden.

I reverted those changes.

Sorry, I confused you a little.
containerd tests use full files.
I have updated these golden files in 39f796e.

Normalized SPDX license should be in Id field.

DmitriyLewen · 2024-07-25T03:24:40Z

pkg/licensing/normalize.go

+var mapping = make(map[string]expression.SimpleExpr)
+
+func addMap(name, key string, hasPlus bool) {
+	license := normalizeKeyAndSuffix(name)


I think I understand your idea - you want to check the newly added licenses (from another PR later).

I think we don't need to do this check every time we start Trivy.

The test can be done in a unit test, but this means that the mapping will be public.

You have added so many licenses, so I am not sure that many licenses will be added later.
Maybe a comment before mapping to the instructions for the new licenses will be enough. wdyt?

integration/testdata/fluentd-multiple-lockfiles.cdx.json.golden

DmitriyLewen · 2024-07-25T04:33:58Z

pkg/fanal/test/integration/testdata/goldens/packages/amazon-2.json.golden

Sorry, I confused you a little.
containerd tests use full files.
I have updated these golden files in 39f796e.

DmitriyLewen · 2024-07-25T04:34:45Z

pkg/fanal/test/integration/testdata/goldens/packages/vulnimage.json.golden

    },
    "Version": "1.6.3-r0",
    "Arch": "x86_64",
    "SrcName": "apr",
    "SrcVersion": "1.6.3-r0",
    "Licenses": [
-      "ASL2.0"
+      "Apache-2.02.0"


@pbaumard can you take a look?

Fixed in latest commit.

DmitriyLewen · 2024-07-31T08:43:18Z

pkg/licensing/normalize.go

+func addMap(name, key string, hasPlus bool) {
+	mapping[name] = expression.SimpleExpr{License: key, HasPlus: hasPlus}
+}


looks like we don't need this function anymore and we can just put everything in the map right away

Done in latest commit, even if I am not sure it is more readable that way.

DmitriyLewen · 2024-07-31T09:06:52Z

pkg/licensing/normalize.go

+var versionRegexpString = "([A-UW-Z)]{2,})( LICENSE)?\\s*[,(-]?\\s*(V|V\\.|VERSION|VERSION-|-)?\\s*([1-9](\\.\\d)*)[)]?"
+
+// case insensitive version match anywhere in string
+var versionRegexp = regexp.MustCompile("(?i)" + versionRegexpString)
+
+// version suffix match
+var versionSuffixRegexp = regexp.MustCompile(versionRegexpString + "$")


I'm not sure I understand where we need this.
Can you add an example in the comment?

I tried to avoid using regex if possible.
Maybe we can avoid using this regex here

The many version variations are in normalize_test.

Most of the mappings in current trivy version or in oss-review-toolkit/ort are because of slight variations in the way the version is declared in the license.

So this regexp allows to:

greatly limit the number of mappings

avoid missing version mappings

This regexp is strict by checking only version suffixes.

DmitriyLewen · 2024-07-31T09:08:16Z

pkg/licensing/normalize_test.go

@@ -8,6 +8,219 @@ import (
 	"github.com/aquasecurity/trivy/pkg/licensing"
 )

+func TestMap(t *testing.T) {
+	assert.Empty(t, licensing.InvalidMappingKeys((nil)))


Suggested change

assert.Empty(t, licensing.InvalidMappingKeys((nil)))

assert.Empty(t, licensing.InvalidMappingKeys(nil))

InvalidMappingKeys no more used in latest commit.

DmitriyLewen · 2024-07-31T09:11:17Z

pkg/fanal/analyzer/pkg/apk/apk_test.go

@@ -453,3 +450,23 @@ func TestParseApkInfo(t *testing.T) {
 		})
 	}
 }
+
+func TestParseLicense(t *testing.T) {


I think this is a redundant test.
We see this function working in TestParseApkInfo.

TestLaxSplitLicense was moved to normalize_test in latest commit.

DmitriyLewen · 2024-07-31T09:16:34Z

pkg/licensing/normalize.go

+func InvalidMappingKeys(licenseToNormalized map[string]expression.SimpleExpr) []string {
+	if licenseToNormalized == nil {
+		licenseToNormalized = mapping
+	}
+	var invalid []string
+	for key := range licenseToNormalized {
+		standardized := standardizeKeyAndSuffix(key)
+		if standardized.License != key {
+			invalid = append(invalid, key)
+		}
+	}
+	return invalid
+}


What if we move this function to the test.
To get mapping, we can create a function like GetBuiltinRules() for secrets.

normalize_test is now in package licensing to use internal mapping and functions.

…rmalize_test

DmitriyLewen

@pbaumard Thanks for work!
And sorry for delay.

LGTM.
Left 1 small comment.

DmitriyLewen · 2024-08-20T07:44:00Z

pkg/licensing/normalize_test.go

+			[]string{
+				"Apache+",
+			},
+			"Apache-2.0+",
+			"Apache-2.0",


Can you add field names for better readability in these tests?
e.g.:

Suggested change

[]string{

"Apache+",

},

"Apache-2.0+",

"Apache-2.0",

licenses: []string{

"Apache+",

},

normalized: "Apache-2.0+",

normalizedKey: "Apache-2.0",

Field names have been added in last commit.

…e-normalize

DmitriyLewen

Hello @pbaumard
Thanks for your work and sorry for delays.

@knqyf263 take a look, when you have time, please.

Signed-off-by: knqyf263 <[email protected]>

knqyf263

It's an awesome contribution! I planned to add this kind of improvement but didn't find the time. Thanks for your great work and patience.

(feat)Improve license normalization

44b0164

- Remove "THE " prefix - Remove "LICENSE " suffix - Fix lowercase key - Add BSD LICENSE 3 - Add ECLIPSE PUBLIC LICENSE 1.0 - Add MIT-0 - Add tests

pbaumard requested a review from knqyf263 as a code owner July 9, 2024 15:04

Fix indenting

5adc524

knqyf263 requested a review from DmitriyLewen July 9, 2024 17:41

DmitriyLewen reviewed Jul 10, 2024

View reviewed changes

Fix format and test

6b52af5

DmitriyLewen reviewed Jul 11, 2024

View reviewed changes

pkg/licensing/normalize_test.go Outdated Show resolved Hide resolved

mapping back to private and other suffixes ignore from SPDX

4dc5070

DmitriyLewen reviewed Jul 12, 2024

View reviewed changes

pkg/licensing/normalize.go Show resolved Hide resolved

pbaumard added 11 commits July 12, 2024 10:10

Do not change unmapped license name

60d29b0

Additional mappings from oss-review-toolkit and space normalization

ab62262

Fix double space

d0101fc

Add back GPL WITH AUTOCONF EXCEPTION

cd7d779

Normalize CycloneDX licenses and fix tests

aa7ee3c

Add MPL mappings

c21e723

Update golden test files

3055e10

Normalize suffixes using existing expression package

6c168f0

Fix copyright_test

6ec91a5

Add ValidateDocument in SPDX test

0f4c8cb

Fix linter

907683d

pbaumard requested a review from DmitriyLewen July 18, 2024 08:52

DmitriyLewen reviewed Jul 24, 2024

View reviewed changes

pbaumard added 5 commits July 24, 2024 15:21

Revert since only Name and Version are checked

5074f24

Revert cycloneDX normalization

8e6f9d3

Normalized SPDX license should be in Id field.

More tests

b8bb817

Comments

56093d9

Fix golden

85b8781

test(integration): update golden files

39f796e

DmitriyLewen reviewed Jul 25, 2024

View reviewed changes

Fix apk license parsing and use version regexp for license normalization

5f7cf6d

pbaumard requested a review from DmitriyLewen July 26, 2024 16:47

DmitriyLewen reviewed Jul 31, 2024

View reviewed changes

Check map in test; make map directly & move TestLaxSplitLicense to no…

aff6ebd

…rmalize_test

pbaumard requested a review from DmitriyLewen August 1, 2024 11:05

DmitriyLewen reviewed Aug 20, 2024

View reviewed changes

pbaumard and others added 3 commits September 2, 2024 17:01

Add field names in test

0134266

Merge branch 'aquasecurity:main' into feature/better-license-normalize

b85b781

Golden test update

8663dcd

pbaumard requested a review from DmitriyLewen September 3, 2024 09:10

Merge remote-tracking branch 'origin/main' into feature/better-licens…

932e937

…e-normalize

DmitriyLewen approved these changes Sep 11, 2024

View reviewed changes

test: add a file for private functions

f3f7c39

Signed-off-by: knqyf263 <[email protected]>

knqyf263 approved these changes Sep 11, 2024

View reviewed changes

knqyf263 enabled auto-merge September 11, 2024 06:30

knqyf263 added this pull request to the merge queue Sep 11, 2024

Merged via the queue into aquasecurity:main with commit 6472e3c Sep 11, 2024
12 checks passed

pbaumard deleted the feature/better-license-normalize branch September 11, 2024 13:06

aqua-bot mentioned this pull request Sep 11, 2024

release: v0.56.0 [main] #7447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(license): improve license normalization #7131

feat(license): improve license normalization #7131

pbaumard commented Jul 9, 2024 •

edited

Loading

CLAassistant commented Jul 9, 2024 •

edited

Loading

CLAassistant commented Jul 9, 2024

DmitriyLewen left a comment

DmitriyLewen left a comment

DmitriyLewen Jul 24, 2024

pbaumard Jul 24, 2024

DmitriyLewen Jul 25, 2024

pbaumard Jul 26, 2024

DmitriyLewen Jul 24, 2024

pbaumard Jul 24, 2024

DmitriyLewen Jul 24, 2024

pbaumard Jul 24, 2024

DmitriyLewen Jul 25, 2024

DmitriyLewen Jul 25, 2024

DmitriyLewen Jul 25, 2024

DmitriyLewen Jul 25, 2024

pbaumard Jul 26, 2024

DmitriyLewen Jul 31, 2024

pbaumard Aug 1, 2024

DmitriyLewen Jul 31, 2024

DmitriyLewen Jul 31, 2024

pbaumard Aug 1, 2024

DmitriyLewen Jul 31, 2024

pbaumard Aug 1, 2024

DmitriyLewen Jul 31, 2024

pbaumard Aug 1, 2024

DmitriyLewen Jul 31, 2024

pbaumard Aug 1, 2024

DmitriyLewen left a comment

DmitriyLewen Aug 20, 2024

pbaumard Sep 2, 2024

DmitriyLewen left a comment

knqyf263 left a comment •

edited

Loading

	func Normalize(name string) string {
	return NormalizeLicense(name).String()
	}

	for i := 0; i < len(expectedPkgs); i++ {
	require.Equal(t, expectedPkgs[i].Name, detail.Packages[i].Name, tc.name)
	require.Equal(t, expectedPkgs[i].Version, detail.Packages[i].Version, tc.name)
	}

	assert.Empty(t, licensing.InvalidMappingKeys((nil)))
	assert.Empty(t, licensing.InvalidMappingKeys(nil))

feat(license): improve license normalization #7131

feat(license): improve license normalization #7131

Conversation

pbaumard commented Jul 9, 2024 • edited Loading

Description

Related issues

Checklist

CLAassistant commented Jul 9, 2024 • edited Loading

CLAassistant commented Jul 9, 2024

DmitriyLewen left a comment

Choose a reason for hiding this comment

DmitriyLewen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitriyLewen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitriyLewen left a comment

Choose a reason for hiding this comment

knqyf263 left a comment • edited Loading

Choose a reason for hiding this comment

pbaumard commented Jul 9, 2024 •

edited

Loading

CLAassistant commented Jul 9, 2024 •

edited

Loading

knqyf263 left a comment •

edited

Loading