bug: fix incorrect policy evaluation

Attestation collections were incorrectly being considered to have passed
the policy if any of the attestations passed, even if others failed.
This was due to the collection being added to the Passed slice multiple
times for each attestation that didn't result in a failure.  Properly
breaking out of the inner loop and only considering a collection as
passed when all of it's attestations pass fixes the issue.

Signed-off-by: Mikhail Swift <mikhail@testifysec.com>

examples/log4shell/verify.sh

@@ -19,5 +19,5 @@ printf "\nVerifying policy on nonvulnerable package...\n"
 docker run --rm -it --net host -v "$(pwd):/src" -w /src/nonvuln witness-log4shell-demo witness verify -c ../witness.yaml --artifactfile target/my-app-1.0-SNAPSHOT.jar
 
 printf "\nVerifying policy on vulnerable package...\n"
-docker run --rm -it --net host -v "$(pwd):/src" -w /src/vuln witness-log4shell-demo witness verify -c ../witness.yaml -a ./demo-attestation.json --artifactfile target/my-app-1.0-SNAPSHOT.jar
+docker run --rm -it --net host -v "$(pwd):/src" -w /src/vuln witness-log4shell-demo witness verify -c ../witness.yaml --artifactfile target/my-app-1.0-SNAPSHOT.jar
 

pkg/policy/errors.go

@@ -16,7 +16,10 @@ package policy
 
 import (
 	"fmt"
+	"strings"
 	"time"
+
+	"github.com/testifysec/witness/pkg/cryptoutil"
 )
 
 type ErrNoAttestations string
@@ -48,3 +51,43 @@ type ErrKeyIDMismatch struct {
 func (e ErrKeyIDMismatch) Error() string {
 	return fmt.Sprintf("public key in policy has expected key id %v but got %v", e.Expected, e.Actual)
 }
+
+type ErrUnknownStep string
+
+func (e ErrUnknownStep) Error() string {
+	return fmt.Sprintf("policy has no step named %v", string(e))
+}
+
+type ErrArtifactCycle string
+
+func (e ErrArtifactCycle) Error() string {
+	return fmt.Sprintf("cycle detected in step's artifact dependencies: %v", string(e))
+}
+
+type ErrMismatchArtifact struct {
+	Artifact cryptoutil.DigestSet
+	Material cryptoutil.DigestSet
+	Path     string
+}
+
+func (e ErrMismatchArtifact) Error() string {
+	return fmt.Sprintf("mismatched digests for %v", e.Path)
+}
+
+type ErrRegoInvalidData struct {
+	Path     string
+	Expected string
+	Actual   interface{}
+}
+
+func (e ErrRegoInvalidData) Error() string {
+	return fmt.Sprintf("invalid data from rego at %v, expected %v but got %T", e.Path, e.Expected, e.Actual)
+}
+
+type ErrPolicyDenied struct {
+	Reasons []string
+}
+
+func (e ErrPolicyDenied) Error() string {
+	return fmt.Sprintf("policy was denied due to:\n%v", strings.Join(e.Reasons, "\n  -"))
+}

pkg/policy/policy.go

@@ -280,25 +280,3 @@ func compareArtifacts(mats map[string]cryptoutil.DigestSet, arts map[string]cryp
 
 	return nil
 }
-
-type ErrUnknownStep string
-
-func (e ErrUnknownStep) Error() string {
-	return fmt.Sprintf("policy has no step named %v", string(e))
-}
-
-type ErrArtifactCycle string
-
-func (e ErrArtifactCycle) Error() string {
-	return fmt.Sprintf("cycle detected in step's artifact dependencies: %v", string(e))
-}
-
-type ErrMismatchArtifact struct {
-	Artifact cryptoutil.DigestSet
-	Material cryptoutil.DigestSet
-	Path     string
-}
-
-func (e ErrMismatchArtifact) Error() string {
-	return fmt.Sprintf("mismatched digests for %v", e.Path)
-}

pkg/policy/rego.go

@@ -19,31 +19,12 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"strings"
 
 	"github.com/open-policy-agent/opa/ast"
 	"github.com/open-policy-agent/opa/rego"
 	"github.com/testifysec/witness/pkg/attestation"
 )
 
-type ErrRegoInvalidData struct {
-	Path     string
-	Expected string
-	Actual   interface{}
-}
-
-func (e ErrRegoInvalidData) Error() string {
-	return fmt.Sprintf("invalid data from rego at %v, expected %v but got %T", e.Path, e.Expected, e.Actual)
-}
-
-type ErrPolicyDenied struct {
-	Reasons []string
-}
-
-func (e ErrPolicyDenied) Error() string {
-	return fmt.Sprintf("policy was denied due to:\n%v", strings.Join(e.Reasons, "\n  -"))
-}
-
 func EvaluateRegoPolicy(attestor attestation.Attestor, policies []RegoPolicy) error {
 	if len(policies) == 0 {
 		return nil

pkg/policy/step.go

@@ -93,6 +93,7 @@ func (s Step) validateAttestations(attestCollections []attestation.Collection) S
 			found[attestation.Type] = attestation.Attestation
 		}
 
+		passed := true
 		for _, expected := range s.Attestations {
 			attestor, ok := found[expected.Type]
 			if !ok {
@@ -104,7 +105,8 @@ func (s Step) validateAttestations(attestCollections []attestation.Collection) S
 					},
 				})
 
-				continue
+				passed = false
+				break
 			}
 
 			if err := EvaluateRegoPolicy(attestor, expected.RegoPolicies); err != nil {
@@ -113,9 +115,12 @@ func (s Step) validateAttestations(attestCollections []attestation.Collection) S
 					Reason:     err,
 				})
 
-				continue
+				passed = false
+				break
 			}
+		}
 
+		if passed {
 			result.Passed = append(result.Passed, collection)
 		}
 	}