fix(sbom): use PURL or Group and Name in case of Java #5154

j1nka · 2023-09-08T12:13:22Z

Description

If another SCA generates CycloneDX file, it may does not have fields like "group" and "name" in SBOM file. As example, an Dependency Track. There's problem in handling SBOM files, where Trivy trying to get data from specific SBOM fields, but not from "purl" field of SBOM (CycloneDX-json) file.

Related issues

Close There's no result if SBOM file doesn't contain fields "group" and "name" in package info of JAVA projects #5151

Checklist

[ X] I've read the guidelines for contributing to this repository.
[X ] 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).

nikpivkin · 2023-09-13T06:01:52Z

pkg/sbom/cyclonedx/unmarshal.go

@@ -410,8 +411,26 @@ func toTrivyCdxComponent(component cdx.Component) ftypes.Component {

 func getPackageName(typ string, component cdx.Component) string {
 	// Jar uses `Group` field for `GroupID`


This comment is no longer needed

Thanks! Done.

nikpivkin · 2023-09-13T06:08:18Z

pkg/sbom/cyclonedx/unmarshal.go

+	// Split the package into its parts
+	parts := strings.Split(pkg, "/")
+
+	// Get Group


Why this comment? It almost duplicates the variable name

Agreed! Done.

nikpivkin · 2023-09-13T06:09:11Z

pkg/sbom/cyclonedx/unmarshal.go

+
+	pkgWithoutVersion := fmt.Sprintf("%s:%s", group, nameWOVersion)
+
+	return pkgWithoutVersion


Suggested change

return pkgWithoutVersion

return fmt.Sprintf("%s:%s", group, nameWOVersion)

Thanks again :) Done

nikpivkin · 2023-09-13T06:10:15Z

@j1nka I left a couple of small comments.

knqyf263 · 2023-09-13T08:00:41Z

I don't remember, but there might be a reason why we use groups. I'd defer to @DmitriyLewen. He will be back next week.

DmitriyLewen

Hello @j1nka
Thanks for your work!

Some languages have nuances with lowercase in purl (e.g. Go, npm).
To avoid problem with that - we use original names (#4306 (comment)).

Maven plugin uses Name and Group field.
We try to adhere to the language format.

But i understand your point.
I left a comment. Take a look, please.

Regards, Dmitriy

DmitriyLewen · 2023-09-18T04:05:10Z

pkg/sbom/cyclonedx/unmarshal.go

+func convertMavenPackage(pkg string) string {
+	// Split the package into its parts
+	parts := strings.Split(pkg, "/")
+
+	group := parts[1]
+
+	// Get FullName with Version
+	nameWithVersion := parts[len(parts)-1]
+
+	// Remove the Version from the package
+	nameWOVersion := strings.Split(nameWithVersion, "@")[0]
+
+	return fmt.Sprintf("%s:%s", group, nameWOVersion)
+}


What if we handle all possible ways?
Something like that:

Suggested change

func convertMavenPackage(pkg string) string {

// Split the package into its parts

parts := strings.Split(pkg, "/")

group := parts[1]

// Get FullName with Version

nameWithVersion := parts[len(parts)-1]

// Remove the Version from the package

nameWOVersion := strings.Split(nameWithVersion, "@")[0]

return fmt.Sprintf("%s:%s", group, nameWOVersion)

}

func convertMavenPackage(component cdx.Component) string {

// By default, Jar uses `Group` field for `GroupID`

// Use original `ArtifactID` and `GroupID`

if component.Group != "" {

return fmt.Sprintf("%s:%s", component.Group, component.Name)

}

// if `Name` and `Group` fields don't exist - unmarshal purl

if component.Name == "" {

// Split the package into its parts

parts := strings.Split(component.PackageURL, "/")

group := parts[1]

// Get FullName with Version

nameWithVersion := parts[len(parts)-1]

// Remove the Version from the package

nameWOVersion := strings.Split(nameWithVersion, "@")[0]

return fmt.Sprintf("%s:%s", group, nameWOVersion)

}

// in some cases `GroupID:ArtifactID` is stored in `Name`

return component.Name

}

According to the specification, name is a required field.

oh... right. Thanks!
Then, we can check that the "Name" field contains "GroupID".

@nikpivkin @DmitriyLewen thanks for your advices! Added some more checks. Main idea is to catch smth like:
1st Version of SBOM (we don't have Group and that's why we can get empty list of vulners):

{ "bom-ref": "", "name": "", "purl": "", "scope": "", "type": "", "version": "" }

2nd Version of SBOM (we have both - Group and Name):

{ "group" : "", "name" : "", "version" : "", "cpe" : "", "purl" : "", "externalReferences" : [ { "type" : "", "url" : "" } ], "type" : "", "bom-ref" : "" }

your examples are not valid.
e.g. Name is required field.

your examples are not valid. e.g. Name is required field.

Hm, i removed all data in this two examples just to show fields. In both cases we have name, but in first we don't have group. Maybe i didn't understand you comment abiut "name is required".

Yes if purl and group are missing we miss the package. In last commit I've added checks like you told me - check the group. You want me to add some checks to identify if purl is present? Because at the moment I don't check if purl is present and trying to parse it.

oh... i didn't understand that you removed all values from these fields ( i thought these field are empty).

You want me to add some checks to identify if purl is present

I think we need to add that.
group and purl can be omitted. So in this case we need to get package name from name field.
It is strange, but it is possible:

{ "bom-ref": "", "name": "", "scope": "", "type": "", "version": "" }

I

Ok! Will do it. But if we don't have group or purl what we need in return? In case of Java only name will be not enough to get vulnerability data

Use name field. It is better than nothing.
It is also possible that name will use <GroupID>:<ArtifactID> format.
e.g. we used this format in name before #4674.

Hope i get it right :) Added checks and name as default behavior if there is no group or purl

j1nka · 2023-09-18T07:13:48Z

Hello @j1nka Thanks for your work!

Some languages have nuances with lowercase in purl (e.g. Go, npm). To avoid problem with that - we use original names (#4306 (comment)).

Maven plugin uses Name and Group field. We try to adhere to the language format.

But i understand your point. I left a comment. Take a look, please.

Regards, Dmitriy

@DmitriyLewen thanks for the clarification! But i'm talking only about Java projects and nothing else. It may happen that we don't get vulnerability data from SBOM because sometimes we don't have "Group"

DmitriyLewen · 2023-09-18T07:28:01Z

It may happen that we don't get vulnerability data from SBOM because sometimes we don't have "Group"

Can you share some example for this case?
I am wondering where these apps store GroupID.

e.g. before #4674 we save ArtifactID + GroupID in name field.

j1nka · 2023-09-18T07:38:42Z

It may happen that we don't get vulnerability data from SBOM because sometimes we don't have "Group"

Can you share some example for this case? I am wondering where these apps store GroupID.

e.g. before #4674 we save ArtifactID + GroupID in name field.

Sure! Two examples above are the real SBOMs generated by different tools (not Trivy)

DmitriyLewen · 2023-09-18T08:48:50Z

pkg/sbom/cyclonedx/unmarshal.go

+		// Split the package into its parts
+		parts := strings.Split(component.PackageURL, "/")
+
+		group := parts[1]
+
+		// Get FullName with Version
+		nameWithVersion := parts[len(parts)-1]
+
+		// Remove the Version from the package
+		nameWOVersion := strings.Split(nameWithVersion, "@")[0]
+
+		return fmt.Sprintf("%s:%s", group, nameWOVersion)
+		// In case we don't have Group or PURL


We already parsed purl here -

trivy/pkg/sbom/cyclonedx/unmarshal.go

Line 347 in 14d3f02

pkg := p.Package()

We can use this.

We already parsed purl here -

trivy/pkg/sbom/cyclonedx/unmarshal.go

Line 347 in 14d3f02

pkg := p.Package()

We can use this.

Great idea, thanks! Made some changes

DmitriyLewen · 2023-09-19T08:33:55Z

pkg/sbom/cyclonedx/unmarshal.go

+func getPackageName(purl *purl.PackageURL, component cdx.Component) string {
+	if purl.Type == packageurl.TypeMaven {
+		return convertMavenPackage(purl, component)
 	}
 	return component.Name


i rechecked code before this line and found that we don't parse CycloneDX without purl -

trivy/pkg/sbom/cyclonedx/unmarshal.go

Lines 337 to 340 in eea3320

if component.PackageURL == "" {

log.Logger.Warnf("Skip the component (BOM-Ref: %s) as the PURL is empty", component.BOMRef)

return false, "", nil, ErrPURLEmpty

}

So we can remove this check (sorry for confusing you).

I think we can remove purl == "" check and move logic to getPackageName and use parsed package name (pkg.Name = getPackageName(p.Type, pkg.Name, component)). Something like that:

Suggested change

func getPackageName(purl *purl.PackageURL, component cdx.Component) string {

if purl.Type == packageurl.TypeMaven {

return convertMavenPackage(purl, component)

}

return component.Name

func getPackageName(typ, pkgNameFromPurl string, component cdx.Component) string {

if typ == packageurl.TypeMaven {

// Jar uses `Group` field for `GroupID`

if component.Group != "" {

return fmt.Sprintf("%s:%s", component.Group, component.Name)

} else {

// use name derived from purl if `Group` doesn't exist

return pkgNameFromPurl

}

}

return component.Name

}

Wow :) Great idea! Thanks a lot! Done :)

DmitriyLewen

Looks good now.
@j1nka Thanks for your work!

Can you update title of this PR?
(we don't use purl instead of name and group name => we use one of these fields.

j1nka · 2023-09-20T08:57:09Z

Looks good now. @j1nka Thanks for your work!

Can you update title of this PR? (we don't use purl instead of name and group name => we use one of these fields.

Hi! Sure. Smth like that? Thanks for your help and patience, Dmitriy!

DmitriyLewen

Great! Thanks!

@knqyf263 i approved this PR.

j1nka · 2023-09-29T07:41:42Z

Great! Thanks!

@knqyf263 i approved this PR.

@knqyf263 @DmitriyLewen Hi! Is there anything else I should to to merge this PR? Thanks!

knqyf263 · 2023-10-03T09:10:16Z

@nikpivkin requested changes. We cannot merge the PR until @nikpivkin approves it.

Changed unmarshalling to PURL

80378ef

j1nka requested a review from knqyf263 as a code owner September 8, 2023 12:13

knqyf263 requested a review from nikpivkin September 8, 2023 13:39

nikpivkin requested changes Sep 13, 2023

View reviewed changes

j1nka added 2 commits September 13, 2023 09:49

Changes according to comments

fa8b37d

Correct comment fix

d38ade7

knqyf263 requested a review from DmitriyLewen September 13, 2023 08:00

DmitriyLewen reviewed Sep 18, 2023

View reviewed changes

More checks added for Maven

e3c0b9a

Added checks for PURL

14d3f02

DmitriyLewen reviewed Sep 18, 2023

View reviewed changes

Changed PURL parsing to existing func

9d841d4

DmitriyLewen reviewed Sep 19, 2023

View reviewed changes

changed tp pkgNameFromPurl if Group does not exist

cce80e6

DmitriyLewen reviewed Sep 20, 2023

View reviewed changes

j1nka changed the title ~~fix(sbom): use PURL instead of Group and Name~~ fix(sbom): use PURL or Group and Name in case of Java Sep 20, 2023

DmitriyLewen approved these changes Sep 20, 2023

View reviewed changes

knqyf263 approved these changes Oct 3, 2023

View reviewed changes

knqyf263 requested a review from nikpivkin October 3, 2023 09:09

knqyf263 enabled auto-merge October 3, 2023 09:10

knqyf263 added this pull request to the merge queue Oct 3, 2023

nikpivkin approved these changes Oct 3, 2023

View reviewed changes

Merged via the queue into aquasecurity:main with commit 393bfdc Oct 3, 2023
13 checks passed

DmitriyLewen mentioned this pull request Oct 23, 2023

There's no result if SBOM file doesn't contain fields "group" and "name" in package info of JAVA projects #5153

Closed

2 tasks

aqua-bot mentioned this pull request May 29, 2024

release: v0.52.0 [main] #6809

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sbom): use PURL or Group and Name in case of Java #5154

fix(sbom): use PURL or Group and Name in case of Java #5154

j1nka commented Sep 8, 2023

nikpivkin Sep 13, 2023

j1nka Sep 13, 2023

nikpivkin Sep 13, 2023

j1nka Sep 13, 2023

nikpivkin Sep 13, 2023

j1nka Sep 13, 2023

nikpivkin commented Sep 13, 2023

knqyf263 commented Sep 13, 2023

DmitriyLewen left a comment

DmitriyLewen Sep 18, 2023

nikpivkin Sep 18, 2023

DmitriyLewen Sep 18, 2023

j1nka Sep 18, 2023

DmitriyLewen Sep 18, 2023

j1nka Sep 18, 2023

DmitriyLewen Sep 18, 2023

j1nka Sep 18, 2023

DmitriyLewen Sep 18, 2023

j1nka Sep 18, 2023

j1nka commented Sep 18, 2023

DmitriyLewen commented Sep 18, 2023

j1nka commented Sep 18, 2023

DmitriyLewen Sep 18, 2023

j1nka Sep 18, 2023

DmitriyLewen Sep 19, 2023

j1nka Sep 20, 2023

DmitriyLewen left a comment

j1nka commented Sep 20, 2023

DmitriyLewen left a comment

j1nka commented Sep 29, 2023

knqyf263 commented Oct 3, 2023

		@@ -410,8 +411,26 @@ func toTrivyCdxComponent(component cdx.Component) ftypes.Component {

		func getPackageName(typ string, component cdx.Component) string {
		// Jar uses `Group` field for `GroupID`


		pkgWithoutVersion := fmt.Sprintf("%s:%s", group, nameWOVersion)

		return pkgWithoutVersion

	return pkgWithoutVersion
	return fmt.Sprintf("%s:%s", group, nameWOVersion)

	if component.PackageURL == "" {
	log.Logger.Warnf("Skip the component (BOM-Ref: %s) as the PURL is empty", component.BOMRef)
	return false, "", nil, ErrPURLEmpty
	}

-func getPackageName(purl *purl.PackageURL, component cdx.Component) string {
-	if purl.Type == packageurl.TypeMaven {
-		return convertMavenPackage(purl, component)
-	}
-	return component.Name
+func getPackageName(typ, pkgNameFromPurl string, component cdx.Component) string {
+	if typ == packageurl.TypeMaven {
+	        // Jar uses `Group` field for `GroupID`
+		if component.Group != "" {
+			return fmt.Sprintf("%s:%s", component.Group, component.Name)
+		} else {
+		        // use name derived from purl if `Group` doesn't exist
+			return pkgNameFromPurl
+		}
+	}
+	return component.Name
+}

fix(sbom): use PURL or Group and Name in case of Java #5154

fix(sbom): use PURL or Group and Name in case of Java #5154

Conversation

j1nka commented Sep 8, 2023

Description

Related issues

Checklist

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

nikpivkin commented Sep 13, 2023

knqyf263 commented Sep 13, 2023

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

j1nka commented Sep 18, 2023

DmitriyLewen commented Sep 18, 2023

j1nka commented Sep 18, 2023

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

j1nka commented Sep 20, 2023

DmitriyLewen left a comment

Choose a reason for hiding this comment

j1nka commented Sep 29, 2023

knqyf263 commented Oct 3, 2023