Update KB constraint to not satisfy if raw constraint is empty. #421
Conversation
Signed-off-by: Vijay Pillai <vijay.pillai@anchore.com>
|
LGTM given the context in the PR - Is there an issue this is linked to in github? |
There is not an issue. We discovered this yesterday evening when Dan Nurmi was testing msrc images with the new grype integration. I can make an issue if you would like. |
|
No worries - just making sure I'm on the same page here is |
| @@ -37,14 +37,10 @@ func (c kbConstraint) supported(format Format) bool { | |||
| } | |||
|
|
|||
| func (c kbConstraint) Satisfied(version *Version) (bool, error) { | |||
| if c.raw == "" && version != nil { | |||
There was a problem hiding this comment.
nit: since this is surprising relative to other constraints defined, can we add more details in comments as to why this is different?
(commentary: other constraints when given a version, if there is no constraint to restrict what versions should be considered vulnerable, then that version should be considered vulnerable.
I can see how this is a problem for MSRC, which has a different semantic. We are not looking for ranges that are vulnerable, we are looking for KB values that positively match --an empty value should never match.)
| // an empty constraint is always satisfied | ||
| return true, nil | ||
| if c.raw == "" { | ||
| // an empty constraint is never satisfied |
There was a problem hiding this comment.
@wagoodman What are your thoughts here? My understanding is that this is inconsistent with how version constraints work everywhere else in Grype, and if so, I'm concerned this might be confusing to Grype devs.
There was a problem hiding this comment.
Can we add a case that tests this behavior in https://github.com/anchore/grype/blob/main/grype/matcher/msrc/matcher_test.go ? That would make the feature need more apparent
…ed to return an error if the version constraint is empty string. Signed-off-by: Vijay Pillai <vijay.pillai@anchore.com>
Signed-off-by: Vijay Pillai <vijay.pillai@anchore.com>
Signed-off-by: Vijay Pillai <vijay.pillai@anchore.com>
Vijay-P
force-pushed
the
msrc-exact-match
branch
from
ff7eaa7
to
672bd5a
Compare
…ing helper_test.go module Signed-off-by: Vijay Pillai <vijay.pillai@anchore.com>
Signed-off-by: Vijay Pillai <vijay.pillai@anchore.com>
Vijay-P
force-pushed
the
msrc-exact-match
branch
from
5c2532f
to
21d01d4
Compare
Signed-off-by: Vijay Pillai <vijay.pillai@anchore.com>
| return nil, fmt.Errorf("distro matcher failed to check constraint='%s' version='%s': %w", vuln.Constraint, verObj, err) | ||
| var e *version.NonFatalConstraintError | ||
| if errors.As(err, &e) { | ||
| log.Warn(e.Error()) |
There was a problem hiding this comment.
nit: Typically in Go, you don't need to call .Error() on errors. When passing errors to functions that take interface{}, like for formatting strings, .Error() will be called automatically if needed. So you can just pass the error itself (e in this case).
| @@ -40,3 +40,13 @@ func MustGetConstraint(constStr string, format Format) Constraint { | |||
| } | |||
| return constraint | |||
| } | |||
|
|
|||
| type NonFatalConstraintError struct { | |||
There was a problem hiding this comment.
Requesting change: It would be helpful to other devs if we added a doc comment for this error, explaining why it exists / how it should be used.
grype/version/kb_contraint.go
Outdated
| } else if version == nil { | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
nit: Since you're returning in the previous if block, you don't need the else here, so you could express this check as a distinct thought. For example:
| } else if version == nil { | |
| return true, nil | |
| } | |
| } | |
| if version == nil { | |
| return true, nil | |
| } |
grype/version/kb_contraint_test.go
Outdated
| } else { | ||
| assert.NoError(t, err, "unexpected error from kbConstraint.Satisfied: %v", err) | ||
| } | ||
| assert.Equal(t, test.satisfied, isSatisfied, "unexpected constraint check result: expected %+v, got %+v", test.satisfied, isSatisfied) |
There was a problem hiding this comment.
nit: The assert.Equal function should show a reasonable helpful explanation of what was expected vs. actual on its own, so you typically don't need to restate those values in the error message. But occasionally the assert functions output text that's not as helpful, so if you found that for this case, I think it makes sense to leave what you have.
There was a problem hiding this comment.
didn't know. changing this
Signed-off-by: Vijay Pillai <vijay.pillai@anchore.com>
Per validation with legacy MSRC matching in Engine and current understanding of how MSRC vulnerabilities are reported, an image should not be vulnerable to a vulnerability if the match constraint is empty. This PR updates the kb constraint to match this understanding.