feat: add Bitnami matcher #2538
Conversation
Signed-off-by: juan131 <juan.ariza@broadcom.com>
grype/matcher/bitnami/matcher.go
Outdated
| // Bitnami packages' metadata are built from the package URL (instead of CPE, hence CPE match is not supported) | ||
| // ref: https://github.com/anchore/syft/blob/main/syft/pkg/bitnami.go#L3-L13 | ||
| // ref: https://github.com/anchore/syft/blob/main/syft/pkg/cataloger/bitnami/package.go#L18-L45 | ||
| return internal.MatchPackageByEcosystemAndCPEs(store, p, m.Type(), false) |
There was a problem hiding this comment.
@willmurphyscode should we include some mechanisms to match based on the pURL?
There was a problem hiding this comment.
Yes, you want to do something besides CPEs here. CPEs are pretty bad, and it looks like we're not making up CPEs for the bitnami packages we find (which is probably a good thing).
Since Bitnami is neither a language ecosystem or a distro, you'll want to add a similar top level function similar to either
grype/grype/matcher/internal/language.go
Line 31 in ee6d91d
grype/grype/matcher/internal/distro.go
Line 16 in ee6d91d
But basically the pattern will be searching by package type + package name + package version (essentially a PURL).
I think this CPE searching code can be removed - we don't expect (or want) to use CPEs where we can avoid it.
@wagoodman might have some more information on building the actual matcher and search changes.
There was a problem hiding this comment.
Thanks for the context @willmurphyscode!
FYI: I was assigned a top priority task so you should expect a couple of days delay on my work here. I'll probably resume it next Monday.
There was a problem hiding this comment.
I did a 1st approach at b6e1858
Signed-off-by: juan131 <juan.ariza@broadcom.com>
Signed-off-by: juan131 <juan.ariza@broadcom.com>
There was a problem hiding this comment.
Thanks for the PR!
I think the big question I have is: Bitnami has its own go-version lib, and uses strings like 7.4.2-3 to indicate the third bitnami release of upstream version 7.4.2 (please correct me if I'm wrong). On the other hand, your OSV data has type SEMVER on its affected version ranges. But it seems to me that since you have -3 or whatever on the end of version ranges, you're using an ecosystem-specific matcher.
I was sort of expecting a bitnami-specific entry in the version comparison files at https://github.com/anchore/grype/tree/main/grype/version, and I realize I was sort of expecting the OSV data to have ranges of type ECOSYSTEM. Did I miss something here? Do we not need to pull in https://github.com/bitnami/go-version and use it?
Happy to sync up on this as well.
| @@ -72,6 +72,8 @@ func KnownPackageSpecifierOverrides() []PackageSpecifierOverride { | |||
|
|
|||
| // jenkins plugins are a special case since they are always considered to be within the java ecosystem | |||
| {Ecosystem: string(pkg.JenkinsPluginPkg), ReplacementEcosystem: ptr(string(pkg.JavaPkg))}, | |||
| // Bitnami is a special case since it's not a language ecosystem but is a package type | |||
| {Ecosystem: "bitnami", ReplacementEcosystem: ptr(string(pkg.BitnamiPkg))}, | |||
There was a problem hiding this comment.
This isn't needed. Since pkg.Bitnami is the string "bitnami" anyway ( se https://github.com/anchore/syft/blob/main/syft/pkg/type.go#L16 ) this line is a no-op.
| ) | ||
|
|
||
| var typeOrder = map[Type]int{ | ||
| ExactDirectMatch: 1, | ||
| ExactIndirectMatch: 2, | ||
| CPEMatch: 3, | ||
| PURLMatch: 4, |
There was a problem hiding this comment.
I don't think we want a new match type. This is exact-direct since we are searching a vulnerability source we know should govern this package for this exact package. (Indirect is its own type because it signals that we searched via an upstream/source package name, because CPE is its own way of searching. But a PURL search is effectively an exact direct search in this case).
|
|
||
| var ErrEmptyPURLMatch = errors.New("attempted PURL match against package with no PURL") | ||
|
|
||
| func MatchPackageByPURL(provider vulnerability.Provider, p pkg.Package, upstreamMatcher match.MatcherType) ([]match.Match, error) { |
There was a problem hiding this comment.
I wonder if this would be simpler as a facade over
grype/grype/matcher/internal/language.go
Line 31 in 4b35276
It looks like its copying some things from the CPE matcher that we don't need, like the confidence of 0.9 and the special JVM version handling.
| @@ -85,6 +85,8 @@ func FormatFromPkg(p pkg.Package) Format { | |||
| switch p.Type { | |||
| case syftPkg.ApkPkg: | |||
| return ApkFormat | |||
| case syftPkg.BitnamiPkg: | |||
There was a problem hiding this comment.
This surprised me. I'll write a longer comment later, but I thought Bitnami had custom versions, like 7.4.2-12, and would need a custom version object. Many ecosystems have a custom version object.
| @@ -7,15 +7,15 @@ import ( | |||
| "github.com/scylladb/go-set/strset" | |||
| ) | |||
|
|
|||
| type CPEPackageParameter struct { | |||
| type PackageParameter struct { | |||
There was a problem hiding this comment.
I think a lot of the changes in this file go away if we're not making PURL its own match type or really its own search type, but using an ecosystem search. (Ecosystem search is in language.go because most ecosystems are languages. I thought that Bitnami would be more of an outlier in this regard, but it seems not.)
|
Hi @juan131 it seems like I wrote a ton of words on here and maybe didn't help very much. I'll try to distill it a bit (sorry for reviewing super verbosely earlier):
Let me know if you'd like to sync up, or if I can do anything to clarify or help out here. |
|
Sorry for the lack of updates on this PR! Easter came and a big part of the team was on vacations so I had to focus on other stuff.
Great! Also versioning, right?
As you mentioned, Bitnami does uses its own versioning. It's almost identical to semver with a small modification: pre-releases don't exist and instead they're considered revisions. It's explained in the link below:
Exactly! Although we use an |
|
@juan131 Thanks for getting back to me! I have two big questions regarding the versioning:
|
|
As you pointed out, we do use This is sth we need to take into account so here at Grype. Does it make sense? |
|
There's an important caveat with having two versioning: What happens when a CVE lists a If Syft inspects an ArgoCD container and the SPDX shipped on it includes sth like this: {
"SPDXID": "SPDXRef-argo-cd",
"name": "argo-cd",
"versionInfo": "1.5.0-1",
"downloadLocation": "git+https://github.com/argoproj/argo-cd/#refs/tags/v1.5.0",
"licenseConcluded": "Apache-2.0",
"licenseDeclared": "Apache-2.0",
"filesAnalyzed": false,
"externalRefs": [
{
"referenceCategory": "SECURITY",
"referenceType": "cpe23Type",
"referenceLocator": "cpe:2.3:*:linuxfoundation:argo_continuous_delivery:1.5.0:*:*:*:*:kubernetes:*:*"
},
{
"referenceCategory": "PACKAGE-MANAGER",
"referenceType": "purl",
"referenceLocator": "pkg:bitnami/argo-cd@1.5.0-1?arch=arm64&distro=debian-12"
}
],
"copyrightText": "NOASSERTION"
},The version would be Why? That revision implies we created a new "Bitnami package revision" for ArgoCD and that new revision may change the bash logic we use during the container initialization or the flags we used to compile the ArgoCD binaries. However, that new package revision uses exactly the same upstream source code we used to create the previous one. As a consequence, given Bitnami Vulndb data is created based on the upstream project vulnerabilities, |
Are you saying we should only use the semver component for version comparisons and drop everything after the hyphen? That is, nothing is every fixed by a hyphen, but is only ever fixed by pulling in more upstream code? I'm never going to see a fixed event with Or maybe you're saying |
Yes, I know it's weird but I think it's the way to go. In the unlikely case that a Bitnami revision ( |
Is that VEX data something we'll have access too? Should we be adding that to the Vunnel provider? Has that every been used? How often? I know you all opened #1826 which is about supporting VEX documents. Is that why? |
We are currently producing VEX data but don't expose it publicly. It's something we exclusively offer to our TAC customers.
Yes, it is important for us that scanners have support for VEX so our customers can consume the VEX data we produce via the scanners. (e.g. we can say "run Grype passing this VEX data via the
I don't think VEX data should be used by default to discard vulnerabilities (regardless we make it public in the future or not). It should be only applied if the user activates some flags acknowledging awareness about its use. |
|
@juan131 thanks for answering my questions about the version comparison. I think we still may need some custom version comparison logic in Grype for bitnami packages: $ grype bitnami/redis:7.4.1
...
NAME INSTALLED FIXED-IN TYPE VULNERABILITY SEVERITY
redis 7.4.1-1 7.4.1, 7.2.8 bitnami BIT-redis-2024-31227 UnknownThis looks incorrect to me, because it looks like redis So I think we need a custom version object that will look at the exression I also think we don't need the new grype/grype/matcher/internal/language.go Line 31 in 2aedfb5 So I think at a high level the two changes we need to get this in are:
Let me know if you have questions! Thanks! |
Summary
After anchore/vunnel#447, anchore/grype-db#217 and anchore/syft#3065 this PR attempts to use Bitmani vulndb data for matching package vulnerabilities.