Correctly match JVM version ranges #2114
Conversation
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
7c9bbf2 to
a07fe7d
Compare
8086627 to
8a0e6d8
Compare
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
8a0e6d8 to
8f38ac0
Compare
|
Hmm, something here appears to cause matches for python and go binaries to get dropped. Any ideas why that may be? It may actually be the syft changes which are causing it |
It's something on the grype matcher side. I tried with an old syft sbom and it still drops the python binary vuln matches |
|
Yeah, I was trying to figure out last night why this was happening -- I figured it out this morning: we stop at the first matcher that claims it can find a match for a package. The JVM matcher was making this claim for all binary type packages, which means it would stop the correct (stock) matcher from being used in other cases (like golang and python). I added the JVM matcher to make certain we're always searching by CPE, however, in this case it's only a version comparison change and the stock matcher already searches by CPE by default. I'll remove the JVM matcher for now and let the stock matcher to continue to raise these matches up. |
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
| expected map[string]string | ||
| }{ | ||
| { | ||
| name: "go-case", |
There was a problem hiding this comment.
nit: does go-case have some significance? I see this a lot, I thought it was a copy/paste thing and people maybe forgot to update the name
There was a problem hiding this comment.
this is an alex-ism that I haven't kicked yet (you're not missing anything). "go-case" for me is the most basic passing test that should "obviously" pass and the cases that follow change a single attribute/quality relative to that go case.
| require.NotNil(t, s) | ||
|
|
||
| matchers := matcher.NewDefaultMatchers(matcher.Config{}) | ||
| // TODO: we need to use the API default configuration, not something hard coded here |
There was a problem hiding this comment.
I'm confused by this comment: these defaults don't match the API defaults, do they? E.g. Java is not using CPEs by default
There was a problem hiding this comment.
they don't but we should be is all this is saying -- but you're right this is not mimicking the default configuration (and we weren't earlier either).
| t.Run(test.version+"_constraint_"+test.constraint, func(t *testing.T) { | ||
| constraint, err := newJvmConstraint(test.constraint) | ||
| require.NoError(t, err) | ||
| test.assertVersionConstraint(t, JVMFormat, constraint) |
There was a problem hiding this comment.
Is this supposed to be using the test.version somehow?
There was a problem hiding this comment.
yeah, it's a little hidden. The test case structs here are shared across all of the tests and they have what you're looking for:
grype/grype/version/helper_test.go
Line 31 in 34b4885
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
This builds on anchore/syft#3217 being able to correctly reason about pre and post JEP 223 versions (1.8.0 = 8.0.0). Specifically, when a package is detected to be a JVM package, and the vuln data indicates it's for a JVM package, then we'll correctly reason about JVM versions when making the comparison. For instance
1.8.0_372==8.0.372based on the JEP223 spec and convention of the patch version in popular jdk releases. Today this nuance is not taken into consideration.Additionally, this PR uses the CPE update field, which is commonly used in the vulnerability field for
<= v1.8JVMs -- without using this field we could never make the correct comparison.Closes #1718