Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: include upstream/source name and version in purl #497

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

ivanstanev
Copy link
Contributor

@ivanstanev ivanstanev commented Jun 2, 2023

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

The source version of a package may not be the same as the package version, which is why we should track them whenever they differ. This change ensures we populate an "upstream" qualifier (similarly to how Syft does this) with the source name and version. "upstream" is a non-standard qualifier, but matches Syft for consistency.

Tracking the source version is important because Snyk's vuln intel on Debian is based on the source version not the binary version. Some packages like postgres-client have differing binary and source versions, which results in reporting false positives once the wrong vulnerable version range is matched.

@ivanstanev ivanstanev force-pushed the feat/upstream-in-purl branch 3 times, most recently from c405b8e to 5b86192 Compare June 6, 2023 16:07
@ivanstanev ivanstanev self-assigned this Jun 6, 2023
@ivanstanev ivanstanev marked this pull request as ready for review June 6, 2023 16:07
@ivanstanev ivanstanev requested a review from a team as a code owner June 6, 2023 16:07
lib/analyzer/package-managers/apt.ts Outdated Show resolved Hide resolved
lib/analyzer/package-managers/apt.ts Outdated Show resolved Hide resolved
lib/analyzer/package-managers/apt.ts Outdated Show resolved Hide resolved
lib/analyzer/package-managers/apt.ts Outdated Show resolved Hide resolved
lib/analyzer/package-managers/apt.ts Outdated Show resolved Hide resolved
@ivanstanev ivanstanev force-pushed the feat/upstream-in-purl branch 2 times, most recently from 2d6d283 to 4a1cb16 Compare June 8, 2023 15:47
@ivanstanev ivanstanev force-pushed the feat/upstream-in-purl branch 2 times, most recently from 4f5e385 to 4832cda Compare June 8, 2023 16:09
The source version of a package may not be the same as the package version, which is why we should track them whenever they differ. This change ensures we populate an "upstream" qualifier (similarly to how Syft does this) with the source name and version. "upstream" is a non-standard qualifier, but matches Syft for consistency.

Tracking the source version is important because Snyk's vuln intel on Debian is based on the source version not the binary version. Some packages like postgres-client have differing binary and source versions, which results in reporting false positives once the wrong vulnerable version range is matched.
@github-actions
Copy link

github-actions bot commented Jun 16, 2023

Expected release notes (by @ivanstanev)

features:
include upstream/source name and version in purl (a45bc34)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

"version": "1.0.9.8.4",
},
},
Object {
"id": "libsigsegv/libsigsegv2@2.10-4+b1",
"info": Object {
"name": "libsigsegv/libsigsegv2",
"purl": "pkg:deb/libsigsegv2@2.10-4%2Bb1?upstream=libsigsegv%402.10-4",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example of a properly encoded purl 🙏

Copy link
Contributor

@tommyknows tommyknows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Note: we only attach the upstream qualifier if the Source is set - we omit it if the package doesn't have a source set.

I know we decided that we're always adding the source even if it may be equal to the package name, but I agree that in the case of the source not being set at all, this hasn't been decided 😄

I'm approving this PR, as I don't have any objections to omitting it if it's not set in the apt-DB.

I'll leave it to you to decide :)

@shlomiSnyk shlomiSnyk removed their request for review June 19, 2023 10:46
"version": "5.6",
},
},
Object {
"id": "openssl/libssl1.1@1.1.1d-0+deb10u3",
"info": Object {
"name": "openssl/libssl1.1",
"purl": "pkg:deb/libssl1.1@1.1.1d-0%2Bdeb10u3?upstream=openssl",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added a test to our container-sbom-export library and that doesn't seem to escape the special chars:

pkg:deb/debian/libssl1.1@1.1.1d-0+deb10u3?upstream=openssl&distro=buster

I'm wondering if it should really be escaped? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's the purl libraries that behave differently, as the TS seems to escape it and the Go one doesn't? 🥴

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Purl spec says that "purl is just a URL", so I'd argue URL rules apply.
That means however that a plus-sign is a valid character and does not need to be escaped unless it's in the query component of the URL.
See this S/O answer for more details.

Copy link
Contributor

@tommyknows tommyknows Jun 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

purl's Go library's Parse function calls a url.PathUnescape function, which mentions in it's docs that:

PathUnescape is identical to QueryUnescape except that it does not unescape '+' to ' ' (space).

😄

So both examples (escaped and not) work!
And the JS library behaves the same!

So both are valid :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking! Fascinating differences 😄 But that suggests that the SBOM export not encoding the + will break in vuln when we try to parse it, resulting in a broken version?

@ivanstanev ivanstanev merged commit c36351d into main Jun 28, 2023
13 checks passed
@ivanstanev ivanstanev deleted the feat/upstream-in-purl branch June 28, 2023 13:11
@team-lumos
Copy link
Collaborator

🎉 This PR is included in version 6.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants