Use package ID from decoded SBOMs when provided (#1872)

* fix: use package id from cyclonedx when provided

Signed-off-by: James Neate <jamesmneate@gmail.com>

* override package IDs from converted SBOMs

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* fix typo

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* remove extractSyftID function

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

---------

Signed-off-by: James Neate <jamesmneate@gmail.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Co-authored-by: Alex Goodman <wagoodman@users.noreply.github.com>

internal/cmptest/location.go

@@ -2,6 +2,7 @@ package cmptest
 
 import (
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 
 	"github.com/anchore/syft/syft/file"
 )
@@ -9,7 +10,7 @@ import (
 type LocationComparer func(x, y file.Location) bool
 
 func DefaultLocationComparer(x, y file.Location) bool {
-	return cmp.Equal(x.Coordinates, y.Coordinates) && cmp.Equal(x.AccessPath, y.AccessPath)
+	return cmp.Equal(x.Coordinates, y.Coordinates, cmpopts.IgnoreUnexported(file.Coordinates{})) && cmp.Equal(x.AccessPath, y.AccessPath)
 }
 
 func LocationComparerWithoutLayer(x, y file.Location) bool {

syft/format/common/spdxhelpers/to_format_model.go

@@ -268,21 +268,34 @@ func toRootPackage(s source.Description) *spdx.Package {
 }
 
 func toSPDXID(identifiable artifact.Identifiable) spdx.ElementID {
+	id := string(identifiable.ID())
+	if strings.HasPrefix(id, "SPDXRef-") {
+		// this is already an SPDX ID, no need to change it (except for the prefix)
+		return spdx.ElementID(helpers.SanitizeElementID(strings.TrimPrefix(id, "SPDXRef-")))
+	}
 	maxLen := 40
-	id := ""
 	switch it := identifiable.(type) {
 	case pkg.Package:
+		if strings.HasPrefix(id, "Package-") {
+			// this is already an SPDX ID, no need to change it
+			return spdx.ElementID(helpers.SanitizeElementID(id))
+		}
 		switch {
 		case it.Type != "" && it.Name != "":
-			id = fmt.Sprintf("Package-%s-%s-%s", it.Type, it.Name, it.ID())
+			id = fmt.Sprintf("Package-%s-%s-%s", it.Type, it.Name, id)
 		case it.Name != "":
-			id = fmt.Sprintf("Package-%s-%s", it.Name, it.ID())
+			id = fmt.Sprintf("Package-%s-%s", it.Name, id)
 		case it.Type != "":
-			id = fmt.Sprintf("Package-%s-%s", it.Type, it.ID())
+			id = fmt.Sprintf("Package-%s-%s", it.Type, id)
 		default:
-			id = fmt.Sprintf("Package-%s", it.ID())
+			id = fmt.Sprintf("Package-%s", id)
 		}
 	case file.Coordinates:
+		if strings.HasPrefix(id, "File-") {
+			// this is already an SPDX ID, no need to change it. Note: there is no way to reach this case today
+			// from within syft, however, this covers future cases where the ID can be overridden
+			return spdx.ElementID(helpers.SanitizeElementID(id))
+		}
 		p := ""
 		parts := strings.Split(it.RealPath, "/")
 		for i := len(parts); i > 0; i-- {
@@ -296,9 +309,7 @@ func toSPDXID(identifiable artifact.Identifiable) spdx.ElementID {
 			}
 			p = path.Join(part, p)
 		}
-		id = fmt.Sprintf("File-%s-%s", p, it.ID())
+		id = fmt.Sprintf("File-%s-%s", p, id)
-	default:
-		id = string(identifiable.ID())
 	}
 	// NOTE: the spdx library prepend SPDXRef-, so we don't do it here
 	return spdx.ElementID(helpers.SanitizeElementID(id))

syft/format/common/spdxhelpers/to_format_model_test.go

@@ -861,6 +861,34 @@ func Test_toSPDXID(t *testing.T) {
 			},
 			expected: "Package-npm-some-package",
 		},
+		{
+			name: "package with existing SPDX ID",
+			it: func() pkg.Package {
+				p := pkg.Package{
+					Type: pkg.NpmPkg,
+					Name: "some-package",
+				}
+				// SPDXRef- prefix is removed on decode (when everything is working as it should)
+				p.OverrideID("Package-npm-some-package-extra!")
+				return p
+			}(),
+			// note: we still sanitize out the "!" which is not allowed in SPDX IDs
+			expected: "Package-npm-some-package-extra",
+		},
+		{
+			name: "package with existing SPDX Ref",
+			it: func() pkg.Package {
+				p := pkg.Package{
+					Type: pkg.NpmPkg,
+					Name: "some-package",
+				}
+				// someone incorrectly added SPDXRef- prefix
+				p.OverrideID("SPDXRef-Package-npm-some-package-extra!")
+				return p
+			}(),
+			// note: we still sanitize out the "!" which is not allowed in SPDX IDs
+			expected: "Package-npm-some-package-extra",
+		},
 	}
 
 	for _, test := range tests {

syft/format/common/spdxhelpers/to_syft_model.go

@@ -509,7 +509,12 @@ func toSyftPackage(p *spdx.Package) pkg.Package {
 		Metadata: extractMetadata(p, info),
 	}
 
+	if p.PackageSPDXIdentifier != "" {
+		// always prefer the IDs from the SBOM over derived IDs
+		sP.OverrideID(artifact.ID(p.PackageSPDXIdentifier))
+	} else {
 		sP.SetID()
+	}
 
 	return *sP
 }

syft/format/common/spdxhelpers/to_syft_model_test.go

@@ -664,7 +664,7 @@ func Test_directPackageFiles(t *testing.T) {
 		Packages: []*spdx.Package{
 			{
 				PackageName:           "some-package",
-				PackageSPDXIdentifier: "1",
+				PackageSPDXIdentifier: "1", // important!
 				PackageVersion:        "1.0.5",
 				Files: []*spdx.File{
 					{
@@ -689,7 +689,7 @@ func Test_directPackageFiles(t *testing.T) {
 		Name:    "some-package",
 		Version: "1.0.5",
 	}
-	p.SetID()
+	p.OverrideID("1") // the same as the spdxID on the package element
 	f := file.Location{
 		LocationData: file.LocationData{
 			Coordinates: file.Coordinates{
@@ -730,3 +730,32 @@ func Test_directPackageFiles(t *testing.T) {
 
 	require.Equal(t, s, got)
 }
+
+func Test_useSPDXIdentifierOverDerivedSyftArtifactID(t *testing.T) {
+	doc := &spdx.Document{
+		SPDXVersion: "SPDX-2.3",
+		Packages: []*spdx.Package{
+			{
+				PackageName:           "some-package",
+				PackageSPDXIdentifier: "1", // important!
+				PackageVersion:        "1.0.5",
+				Files: []*spdx.File{
+					{
+						FileName:           "some-file",
+						FileSPDXIdentifier: "2",
+						Checksums: []spdx.Checksum{
+							{
+								Algorithm: "SHA1",
+								Value:     "a8d733c64f9123",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	s, err := ToSyftModel(doc)
+
+	assert.Nil(t, err)
+	assert.NotNil(t, s.Artifacts.Packages.Package("1"))
+}

syft/format/cyclonedxjson/encoder_test.go

@@ -3,6 +3,7 @@ package cyclonedxjson
 import (
 	"bytes"
 	"flag"
+	"regexp"
 	"strings"
 	"testing"
 
@@ -116,6 +117,14 @@ func TestCycloneDxImageEncoder(t *testing.T) {
 func redactor(values ...string) testutil.Redactor {
 	return testutil.NewRedactions().
 		WithValuesRedacted(values...).
+		WithPatternRedactorSpec(
+			testutil.PatternReplacement{
+				// only the source component bom-ref (not package or other component bom-refs)
+				Search:  regexp.MustCompile(`"component": \{[^}]*"bom-ref":\s*"(?P<redact>.+)"[^}]*}`),
+				Groups:  []string{"redact"}, // use the regex to anchore the search, but only replace bytes within the capture group
+				Replace: "redacted",
+			},
+		).
 		WithPatternRedactors(
 			map[string]string{
 				// UUIDs
@@ -126,9 +135,6 @@ func redactor(values ...string) testutil.Redactor {
 
 				// image hashes
 				`sha256:[A-Fa-f0-9]{64}`: `sha256:redacted`,
-
-				// BOM refs
-				`"bom-ref":\s*"[^"]+"`: `"bom-ref":"redacted"`,
 			},
 		)
 }

syft/format/cyclonedxjson/test-fixtures/snapshot/TestCycloneDxDirectoryEncoder.golden

@@ -17,14 +17,14 @@
       ]
     },
     "component": {
-      "bom-ref":"redacted",
+      "bom-ref": "redacted",
       "type": "file",
       "name": "some/path"
     }
   },
   "components": [
     {
-      "bom-ref":"redacted",
+      "bom-ref": "4dd25c6ee16b729a",
       "type": "library",
       "name": "package-1",
       "version": "1.0.1",
@@ -61,7 +61,7 @@
       ]
     },
     {
-      "bom-ref":"redacted",
+      "bom-ref": "pkg:deb/debian/package-2@2.0.1?package-id=39392bb5e270f669",
       "type": "library",
       "name": "package-2",
       "version": "2.0.1",
@@ -91,7 +91,7 @@
       ]
     },
     {
-      "bom-ref":"redacted",
+      "bom-ref": "os:debian@1.2.3",
       "type": "operating-system",
       "name": "debian",
       "version": "1.2.3",

syft/format/cyclonedxjson/test-fixtures/snapshot/TestCycloneDxImageEncoder.golden

@@ -17,7 +17,7 @@
       ]
     },
     "component": {
-      "bom-ref":"redacted",
+      "bom-ref": "redacted",
       "type": "container",
       "name": "user-image-input",
       "version": "sha256:redacted"
@@ -25,7 +25,7 @@
   },
   "components": [
     {
-      "bom-ref":"redacted",
+      "bom-ref": "72567175418f73f8",
       "type": "library",
       "name": "package-1",
       "version": "1.0.1",
@@ -66,7 +66,7 @@
       ]
     },
     {
-      "bom-ref":"redacted",
+      "bom-ref": "pkg:deb/debian/package-2@2.0.1?package-id=4b756c6f6fb127a3",
       "type": "library",
       "name": "package-2",
       "version": "2.0.1",
@@ -100,7 +100,7 @@
       ]
     },
     {
-      "bom-ref":"redacted",
+      "bom-ref": "os:debian@1.2.3",
       "type": "operating-system",
       "name": "debian",
       "version": "1.2.3",

syft/format/cyclonedxxml/encoder_test.go

@@ -90,16 +90,24 @@ func TestCycloneDxImageEncoder(t *testing.T) {
 func redactor(values ...string) testutil.Redactor {
 	return testutil.NewRedactions().
 		WithValuesRedacted(values...).
+		WithPatternRedactorSpec(
+			testutil.PatternReplacement{
+				// only the source component bom-ref (not package or other component bom-refs)
+				Search:  regexp.MustCompile(`<component bom-ref="(?P<redact>[^"]*)" type="file">`),
+				Groups:  []string{"redact"}, // use the regex to anchore the search, but only replace bytes within the capture group
+				Replace: "redacted",
+			},
+		).
 		WithPatternRedactors(
 			map[string]string{
 				// dates
 				`([0-9]+)-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])[Tt]([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9]|60)(\.[0-9]+)?(([Zz])|([+|\-]([01][0-9]|2[0-3]):[0-5][0-9]))`: `redacted`,
 
-				// image hashes and BOM refs
+				// image hashes
 				`sha256:[A-Za-z0-9]{64}`: `sha256:redacted`,
 
-				// serial numbers and BOM refs
+				// serial numbers
-				`(serialNumber|bom-ref)="[^"]+"`: `$1="redacted"`,
+				`(serialNumber)="[^"]+"`: `$1="redacted"`,
 			},
 		)
 }

syft/format/cyclonedxxml/test-fixtures/snapshot/TestCycloneDxDirectoryEncoder.golden

@@ -16,7 +16,7 @@
     </component>
   </metadata>
   <components>
-    <component bom-ref="redacted" type="library">
+    <component bom-ref="4dd25c6ee16b729a" type="library">
       <name>package-1</name>
       <version>1.0.1</version>
       <licenses>
@@ -34,7 +34,7 @@
         <property name="syft:location:0:path">/some/path/pkg1</property>
       </properties>
     </component>
-    <component bom-ref="redacted" type="library">
+    <component bom-ref="pkg:deb/debian/package-2@2.0.1?package-id=39392bb5e270f669" type="library">
       <name>package-2</name>
       <version>2.0.1</version>
       <cpe>cpe:2.3:*:some:package:2:*:*:*:*:*:*:*</cpe>
@@ -47,7 +47,7 @@
         <property name="syft:metadata:installedSize">0</property>
       </properties>
     </component>
-    <component bom-ref="redacted" type="operating-system">
+    <component bom-ref="os:debian@1.2.3" type="operating-system">
       <name>debian</name>
       <version>1.2.3</version>
       <description>debian</description>

syft/format/cyclonedxxml/test-fixtures/snapshot/TestCycloneDxImageEncoder.golden

@@ -11,13 +11,13 @@
         </component>
       </components>
     </tools>
-    <component bom-ref="redacted" type="container">
+    <component bom-ref="f28a4ba3ddfdddad" type="container">
       <name>user-image-input</name>
       <version>sha256:redacted</version>
     </component>
   </metadata>
   <components>
-    <component bom-ref="redacted" type="library">
+    <component bom-ref="72567175418f73f8" type="library">
       <name>package-1</name>
       <version>1.0.1</version>
       <licenses>
@@ -36,7 +36,7 @@
         <property name="syft:location:0:path">/somefile-1.txt</property>
       </properties>
     </component>
-    <component bom-ref="redacted" type="library">
+    <component bom-ref="pkg:deb/debian/package-2@2.0.1?package-id=4b756c6f6fb127a3" type="library">
       <name>package-2</name>
       <version>2.0.1</version>
       <cpe>cpe:2.3:*:some:package:2:*:*:*:*:*:*:*</cpe>
@@ -50,7 +50,7 @@
         <property name="syft:metadata:installedSize">0</property>
       </properties>
     </component>
-    <component bom-ref="redacted" type="operating-system">
+    <component bom-ref="os:debian@1.2.3" type="operating-system">
       <name>debian</name>
       <version>1.2.3</version>
       <description>debian</description>

syft/format/internal/cyclonedxutil/helpers/decoder.go

@@ -5,7 +5,6 @@ import (
 
 	"github.com/CycloneDX/cyclonedx-go"
 
-	"github.com/anchore/packageurl-go"
 	"github.com/anchore/syft/syft/artifact"
 	"github.com/anchore/syft/syft/linux"
 	"github.com/anchore/syft/syft/pkg"
@@ -66,12 +65,16 @@ func collectPackages(component *cyclonedx.Component, s *sbom.SBOM, idMap map[str
 	case cyclonedx.ComponentTypeApplication, cyclonedx.ComponentTypeFramework, cyclonedx.ComponentTypeLibrary:
 		p := decodeComponent(component)
 		idMap[component.BOMRef] = p
-		syftID := extractSyftPacakgeID(component.BOMRef)
+		if component.BOMRef != "" {
-		if syftID != "" {
+			// always prefer the IDs from the SBOM over derived IDs
-			idMap[syftID] = p
+			p.OverrideID(artifact.ID(component.BOMRef))
-		}
+		} else {
-		// TODO there must be a better way than needing to call this manually:
 			p.SetID()
+		}
+		syftID := p.ID()
+		if syftID != "" {
+			idMap[string(syftID)] = p
+		}
 		s.Artifacts.Packages.Add(*p)
 	}
 
@@ -82,19 +85,6 @@ func collectPackages(component *cyclonedx.Component, s *sbom.SBOM, idMap map[str
 	}
 }
 
-func extractSyftPacakgeID(i string) string {
-	instance, err := packageurl.FromString(i)
-	if err != nil {
-		return ""
-	}
-	for _, q := range instance.Qualifiers {
-		if q.Key == "package-id" {
-			return q.Value
-		}
-	}
-	return ""
-}
-
 func linuxReleaseFromComponents(components []cyclonedx.Component) *linux.Release {
 	for i := range components {
 		component := &components[i]

syft/format/internal/cyclonedxutil/helpers/decoder_test.go

@@ -421,3 +421,39 @@ func Test_decodeDependencies(t *testing.T) {
 		})
 	}
 }
+
+func Test_useBomRefOverDerivedSyftArtifactID(t *testing.T) {
+
+	packageWithId := cyclonedx.Component{
+		BOMRef:     "pkg:maven/org.springframework.boot/spring-boot-starter-test?package-id=646a5a71a4abeee0",
+		Type:       cyclonedx.ComponentTypeLibrary,
+		Name:       "spring-boot-starter-test",
+		Version:    "",
+		PackageURL: "pkg:maven/org.springframework.boot/spring-boot-starter-test",
+	}
+
+	packageWithoutId := cyclonedx.Component{
+		BOMRef:     "pkg:maven/org.springframework.boot/spring-boot-starter-webflux",
+		Type:       cyclonedx.ComponentTypeLibrary,
+		Name:       "spring-boot-starter-webflux",
+		Version:    "",
+		PackageURL: "pkg:maven/org.springframework.boot/spring-boot-starter-webflux",
+	}
+
+	bom := cyclonedx.BOM{Metadata: nil,
+		Components: &[]cyclonedx.Component{
+			packageWithId,
+			packageWithoutId,
+		}}
+
+	s, err := ToSyftModel(&bom)
+
+	assert.Nil(t, err)
+	assert.NotNil(t, s.Artifacts.Packages.Package("pkg:maven/org.springframework.boot/spring-boot-starter-test?package-id=646a5a71a4abeee0"))
+
+	pkgsWithoutID := s.Artifacts.Packages.PackagesByName(packageWithoutId.Name)
+
+	assert.Len(t, pkgsWithoutID, 1)
+	assert.NotEqual(t, "", pkgsWithoutID[0].ID())
+
+}

syft/format/internal/testutil/redactor.go

@@ -28,6 +28,7 @@ func (r RedactorFn) Redact(b []byte) []byte {
 
 type PatternReplacement struct {
 	Search  *regexp.Regexp
+	Groups  []string
 	Replace string
 }
 
@@ -39,7 +40,67 @@ func NewPatternReplacement(r *regexp.Regexp) PatternReplacement {
 }
 
 func (p PatternReplacement) Redact(b []byte) []byte {
+	if len(p.Groups) == 0 {
 		return p.Search.ReplaceAll(b, []byte(p.Replace))
+	}
+
+	return p.redactNamedGroups(b)
+}
+
+func (p PatternReplacement) redactNamedGroups(b []byte) []byte {
+	groupsToReplace := make(map[string]bool)
+	for _, g := range p.Groups {
+		groupsToReplace[g] = true
+	}
+
+	subexpNames := p.Search.SubexpNames()
+
+	return p.Search.ReplaceAllFunc(b, func(match []byte) []byte {
+		indexes := p.Search.FindSubmatchIndex(match)
+		if indexes == nil {
+			return match
+		}
+
+		result := make([]byte, len(match))
+		copy(result, match)
+
+		// keep track of the offset as we replace groups
+		offset := 0
+
+		// process each named group
+		for i, name := range subexpNames {
+			// skip the full match (i==0) and groups we don't want to replace
+			if i == 0 || !groupsToReplace[name] {
+				continue
+			}
+
+			// get the start and end positions of this group
+			startPos := indexes[2*i]
+			endPos := indexes[2*i+1]
+
+			// skip if the group didn't match
+			if startPos < 0 || endPos < 0 {
+				continue
+			}
+
+			// adjust positions based on previous replacements
+			startPos += offset
+			endPos += offset
+
+			// replace the group with our replacement text
+			beforeGroup := result[:startPos]
+			afterGroup := result[endPos:]
+
+			// calculate the new offset
+			oldLen := endPos - startPos
+			newLen := len(p.Replace)
+			offset += (newLen - oldLen)
+
+			result = append(beforeGroup, append([]byte(p.Replace), afterGroup...)...) //nolint:gocritic
+		}
+
+		return result
+	})
 }
 
 // Replace by value //////////////////////////////
@@ -86,6 +147,13 @@ func (r *Redactions) WithPatternRedactors(values map[string]string) *Redactions
 	return r
 }
 
+func (r *Redactions) WithPatternRedactorSpec(values ...PatternReplacement) *Redactions {
+	for _, v := range values {
+		r.redactors = append(r.redactors, v)
+	}
+	return r
+}
+
 func (r *Redactions) WithValueRedactors(values map[string]string) *Redactions {
 	for k, v := range values {
 		r.redactors = append(r.redactors,

syft/format/syftjson/to_syft_model.go

@@ -351,9 +351,7 @@ func toSyftPackage(p model.Package, idAliases map[string]string) pkg.Package {
 		Metadata:  p.Metadata,
 	}
 
-	// we don't know if this package ID is truly unique, however, we need to trust the user input in case there are
+	// always prefer the IDs from the SBOM over derived IDs
-	// external references to it. That is, we can't derive our own ID (using pkg.SetID()) since consumers won't
-	// be able to historically interact with data that references the IDs from the original SBOM document being decoded now.
 	out.OverrideID(artifact.ID(p.ID))
 
 	// this alias mapping is currently defunct, but could be useful in the future.

syft/internal/fileresolver/container_image_squash_test.go

@@ -512,6 +512,7 @@ func Test_imageSquashResolver_resolvesLinks(t *testing.T) {
 func compareLocations(t *testing.T, expected, actual []file.Location) {
 	t.Helper()
 	ignoreUnexported := cmpopts.IgnoreUnexported(file.LocationData{})
+	ignoreUnexportedCoord := cmpopts.IgnoreUnexported(file.Coordinates{})
 	ignoreMetadata := cmpopts.IgnoreFields(file.LocationMetadata{}, "Annotations")
 	ignoreFS := cmpopts.IgnoreFields(file.Coordinates{}, "FileSystemID")
 
@@ -520,6 +521,7 @@ func compareLocations(t *testing.T, expected, actual []file.Location) {
 
 	if d := cmp.Diff(expected, actual,
 		ignoreUnexported,
+		ignoreUnexportedCoord,
 		ignoreFS,
 		ignoreMetadata,
 	); d != "" {

syft/pkg/package.go

@@ -27,7 +27,7 @@ type Package struct {
 	Type      Type             `cyclonedx:"type"`                   // the package type (e.g. Npm, Yarn, Python, Rpm, Deb, etc)
 	CPEs      []cpe.CPE        `hash:"ignore"`                      // all possible Common Platform Enumerators (note: this is NOT included in the definition of the ID since all fields on a CPE are derived from other fields)
 	PURL      string           `hash:"ignore"`                      // the Package URL (see https://github.com/package-url/purl-spec)
-	Metadata  interface{}      // additional data found while parsing the package source
+	Metadata  any              // additional data found while parsing the package source
 }
 
 func (p *Package) OverrideID(id artifact.ID) {