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

Update KB constraint to not satisfy if raw constraint is empty. #421

Merged
merged 8 commits into from
Sep 21, 2021
10 changes: 9 additions & 1 deletion grype/matcher/common/distro_matchers.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
package common

import (
"errors"
"fmt"

"github.com/anchore/grype/internal/log"

"github.com/anchore/grype/grype/match"
"github.com/anchore/grype/grype/pkg"
"github.com/anchore/grype/grype/version"
Expand Down Expand Up @@ -32,7 +35,12 @@ func FindMatchesByPackageDistro(store vulnerability.ProviderByDistro, d *distro.
// if the constraint it met, then the given package has the vulnerability
isPackageVulnerable, err := vuln.Constraint.Satisfied(verObj)
if err != nil {
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)
} else {
return nil, fmt.Errorf("distro matcher failed to check constraint='%s' version='%s': %w", vuln.Constraint, verObj, err)
}
}

if !isPackageVulnerable {
Expand Down
14 changes: 14 additions & 0 deletions grype/version/constraint.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,17 @@ func MustGetConstraint(constStr string, format Format) Constraint {
}
return constraint
}

// NonFatalConstraintError should be used any time an unexpected but recoverable condition is encountered while
// checking version constraint satisfaction. The error should get returned by any implementer of the Constraint
// interface. If returned by the Satisfied method on the Constraint interface, this error will be caught and
// logged as a warning in the FindMatchesByPackageDistro function in grype/matcher/common/distro_matchers.go
type NonFatalConstraintError struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

constraint Constraint
version *Version
message string
}

func (e NonFatalConstraintError) Error() string {
return fmt.Sprintf("Matching raw constraint %s against version %s caused a non-fatal error: %s", e.constraint, e.version, e.message)
}
18 changes: 10 additions & 8 deletions grype/version/kb_contraint.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,16 @@ func (c kbConstraint) supported(format Format) bool {
}

func (c kbConstraint) Satisfied(version *Version) (bool, error) {
if c.raw == "" && version != nil {
Copy link
Contributor

@wagoodman wagoodman Sep 17, 2021

Choose a reason for hiding this comment

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

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
} else if version == nil {
if c.raw != "" {
// a non-empty constraint with no version given should always fail
return false, nil
if c.raw == "" {
// an empty constraint is never satisfied
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoa — #421 (comment)

return false, &NonFatalConstraintError{
constraint: c,
version: version,
message: "Unexpected data in DB: Empty raw version constraint.",
}
}

if version == nil {
return true, nil
}

Expand All @@ -57,7 +59,7 @@ func (c kbConstraint) Satisfied(version *Version) (bool, error) {

func (c kbConstraint) String() string {
if c.raw == "" {
return "none (kb)"
return fmt.Sprintf("%q (kb)", c.raw)
}
return fmt.Sprintf("%s (kb)", c.raw)
}
52 changes: 39 additions & 13 deletions grype/version/kb_contraint_test.go
Original file line number Diff line number Diff line change
@@ -1,28 +1,54 @@
package version

import (
"errors"
"github.com/stretchr/testify/assert"
"testing"
)

func TestVersionKbConstraint(t *testing.T) {
tests := []testCase{
{version: "", constraint: "", satisfied: true},
{version: "", constraint: "foo", satisfied: false},
{version: "1", constraint: "foo", satisfied: false},
{version: "1", constraint: "1", satisfied: true},
{version: "878787", constraint: "979797 || 101010 || 878787", satisfied: true},
{version: "478787", constraint: "979797 || 101010 || 878787", satisfied: false},
tests := []struct {
name string
version string
constraint string
satisfied bool
shouldErr bool
errorAssertion func(t *testing.T, err error)
}{
{name: "no constraint no version raises error", version: "", constraint: "", satisfied: false, shouldErr: true, errorAssertion: func(t *testing.T, err error) {
var expectedError *NonFatalConstraintError
assert.ErrorAs(t, err, &expectedError, "Unexpected error type from kbConstraint.Satisfied: %v", err)
}},
{name: "no constraint with version raises error", version: "878787", constraint: "", satisfied: false, shouldErr: true, errorAssertion: func(t *testing.T, err error) {
var expectedError *NonFatalConstraintError
assert.ErrorAs(t, err, &expectedError, "Unexpected error type from kbConstraint.Satisfied: %v", err)
}},
{name: "no version is unsatisifed", version: "", constraint: "foo", satisfied: false},
{name: "version constraint mismatch", version: "1", constraint: "foo", satisfied: false},
{name: "matching version and constraint", version: "1", constraint: "1", satisfied: true},
{name: "base keyword matching version and constraint", version: "base", constraint: "base", satisfied: true},
{name: "version and OR constraint match", version: "878787", constraint: "979797 || 101010 || 878787", satisfied: true},
{name: "version and OR constraint mismatch", version: "478787", constraint: "979797 || 101010 || 878787", satisfied: false},
}

for _, test := range tests {
t.Run(test.name(), func(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
constraint, err := newKBConstraint(test.constraint)
if !errors.Is(err, test.constErr) {
t.Fatalf("unexpected constraint error: '%+v'!='%+v'", err, test.constErr)
}
assert.NoError(t, err, "unexpected error from newKBConstraint: %v", err)

version, err := NewVersion(test.version, KBFormat)
assert.NoError(t, err, "unexpected error from NewVersion: %v", err)

test.assert(t, KBFormat, constraint)
isSatisfied, err := constraint.Satisfied(version)
if test.shouldErr {
if test.errorAssertion != nil {
test.errorAssertion(t, err)
} else {
assert.Error(t, err)
}
} else {
assert.NoError(t, err, "unexpected error from kbConstraint.Satisfied: %v", err)
}
assert.Equal(t, test.satisfied, isSatisfied, "unexpected constraint check result")
})
}
}