sourcepolicy: fix policy so last rule wins

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

sourcepolicy/engine.go

@@ -25,7 +25,7 @@ var (
 // Mutations are delegated to the `Mutater` interface.
 type Engine struct {
 	pol     []*spb.Policy
-	sources map[string]*sourceCache
+	sources map[string]*selectorCache
 }
 
 // NewEngine creates a new source policy engine.
@@ -36,9 +36,9 @@ func NewEngine(pol []*spb.Policy) *Engine {
 }
 
 // TODO: The key here can't be used to cache attr constraint regexes.
-func (e *Engine) sourceCache(src *spb.Selector) *sourceCache {
+func (e *Engine) selectorCache(src *spb.Selector) *selectorCache {
 	if e.sources == nil {
-		e.sources = map[string]*sourceCache{}
+		e.sources = map[string]*selectorCache{}
 	}
 
 	key := src.MatchType.String() + " " + src.Identifier
@@ -47,7 +47,7 @@ func (e *Engine) sourceCache(src *spb.Selector) *sourceCache {
 		return s
 	}
 
-	s := &sourceCache{Selector: src}
+	s := &selectorCache{Selector: src}
 
 	e.sources[key] = s
 	return s
@@ -97,73 +97,56 @@ func (e *Engine) Evaluate(ctx context.Context, op *pb.Op) (bool, error) {
 }
 
 func (e *Engine) evaluatePolicies(ctx context.Context, srcOp *pb.SourceOp) (bool, error) {
+	for _, pol := range e.pol {
+		mut, err := e.evaluatePolicy(ctx, pol, srcOp)
+		if mut || err != nil {
+			return mut, err
+		}
+	}
+	return false, nil
+}
+
+// evaluatePolicy evaluates a single policy against a source operation.
+// If the source is mutated the policy is short-circuited and `true` is returned.
+// If the source is denied, an error will be returned.
+//
+// For Allow/Deny rules, the last matching rule wins.
+// E.g. `ALLOW foo; DENY foo` will deny `foo`, `DENY foo; ALLOW foo` will allow `foo`.
+func (e *Engine) evaluatePolicy(ctx context.Context, pol *spb.Policy, srcOp *pb.SourceOp) (bool, error) {
 	ident := srcOp.GetIdentifier()
 
 	ctx = bklog.WithLogger(ctx, bklog.G(ctx).WithFields(map[string]interface{}{
 		"ref": ident,
 	}))
 
-	for _, pol := range e.pol {
-		mut, err := e.evaluatePolicy(ctx, pol, srcOp, ident)
-		if mut || err != nil {
-			return mut, err
-		}
-	}
-	return false, nil
-}
-
-func (e *Engine) evaluatePolicy(ctx context.Context, pol *spb.Policy, srcOp *pb.SourceOp, ref string) (bool, error) {
+	var deny bool
 	for _, rule := range pol.Rules {
-		mut, err := e.evaluateRule(ctx, rule, ref, srcOp)
-		if mut || err != nil {
-			return mut, err
+		selector := e.selectorCache(rule.Selector)
+		matched, err := match(ctx, selector, ident, srcOp.Attrs)
+		if err != nil {
+			return false, errors.Wrap(err, "error matching source policy")
 		}
+		if !matched {
+			continue
+		}
+
+		switch rule.Action {
+		case spb.PolicyAction_ALLOW:
+			deny = false
+		case spb.PolicyAction_DENY:
+			deny = true
+		case spb.PolicyAction_CONVERT:
+			mut, err := mutate(ctx, srcOp, rule, selector, ident)
+			if err != nil || mut {
+				return mut, errors.Wrap(err, "error mutating source policy")
+			}
+		default:
+			return false, errors.Errorf("source policy: rule %s %s: unknown type %q", rule.Action, rule.Selector.Identifier, ident)
+		}
+	}
+
+	if deny {
+		return false, errors.Wrapf(ErrSourceDenied, "source %q denied by policy", ident)
 	}
 	return false, nil
 }
-
-func (e *Engine) evaluateRule(ctx context.Context, rule *spb.Rule, ref string, op *pb.SourceOp) (bool, error) {
-	// get cached state for this source
-	src := e.sourceCache(rule.Selector)
-
-	match, err := match(ctx, src, ref, op.Attrs)
-	if err != nil {
-		return false, errors.Wrap(err, "error evaluating rule")
-	}
-
-	bklog.G(ctx).Debug("sourcepolicy: rule match")
-
-	if !match {
-		return false, nil
-	}
-
-	switch rule.Action {
-	case spb.PolicyAction_ALLOW:
-		return false, nil
-	case spb.PolicyAction_DENY:
-		if match {
-			return false, errors.Wrapf(ErrSourceDenied, "rule %s %s applies to source %s", rule.Action, rule.Selector.Identifier, ref)
-		}
-		return false, nil
-	case spb.PolicyAction_CONVERT:
-		if rule.Updates == nil {
-			return false, errors.Errorf("missing destination for convert rule")
-		}
-
-		// TODO: This should really go in the mutator, but there's a lot of deatail we'd need to pass through.
-		dest := rule.Updates.Identifier
-		if dest == "" {
-			dest = rule.Selector.Identifier
-		}
-		dest, err = src.Format(ref, dest)
-		if err != nil {
-			return false, errors.Wrap(err, "error formatting destination")
-		}
-
-		bklog.G(ctx).Debugf("sourcepolicy: converting %s to %s, pattern: %s", ref, dest, rule.Updates.Identifier)
-
-		return mutate(ctx, op, dest, rule.Updates.Attrs)
-	default:
-		return false, errors.Errorf("source policy: rule %s %s: unknown type %q", rule.Action, rule.Selector.Identifier, ref)
-	}
-}

sourcepolicy/engine_test.go

@@ -23,6 +23,45 @@ func TestEngineEvaluate(t *testing.T) {
 	t.Run("Test convert wildcard", testConvertWildcard)
 	t.Run("Test convert multiple", testConvertMultiple)
 	t.Run("test multiple policies", testMultiplePolicies)
+	t.Run("Last rule wins", testLastRuleWins)
+}
+
+func testLastRuleWins(t *testing.T) {
+	pol := []*spb.Policy{
+		{
+			Rules: []*spb.Rule{
+				{
+					Action: spb.PolicyAction_ALLOW,
+					Selector: &spb.Selector{
+						Identifier: "docker-image://docker.io/library/busybox:latest",
+					},
+				},
+				{
+					Action: spb.PolicyAction_DENY,
+					Selector: &spb.Selector{
+						Identifier: "docker-image://docker.io/library/busybox:latest",
+					},
+				},
+				{
+					Action: spb.PolicyAction_ALLOW,
+					Selector: &spb.Selector{
+						Identifier: "docker-image://docker.io/library/busybox:latest",
+					},
+				},
+			},
+		},
+	}
+
+	e := NewEngine(pol)
+	mut, err := e.Evaluate(context.Background(), &pb.Op{
+		Op: &pb.Op_Source{
+			Source: &pb.SourceOp{
+				Identifier: "docker-image://docker.io/library/busybox:latest",
+			},
+		},
+	})
+	require.NoError(t, err)
+	require.False(t, mut)
 }
 
 func testMultiplePolicies(t *testing.T) {

sourcepolicy/formatter.go

@@ -9,7 +9,7 @@ import (
 )
 
 // Source wraps a a protobuf source in order to store cached state such as the compiled regexes.
-type sourceCache struct {
+type selectorCache struct {
 	*spb.Selector
 
 	re *regexp.Regexp
@@ -21,7 +21,7 @@ type sourceCache struct {
 // For example, if the source is a wildcard, the ref will be formatted with the wildcard in the source replacing the parameters in the destination.
 //
 //	matcher: wildcard source: "docker.io/library/golang:*"  match: "docker.io/library/golang:1.19" format: "docker.io/library/golang:${1}-alpine" result: "docker.io/library/golang:1.19-alpine"
-func (s *sourceCache) Format(match, format string) (string, error) {
+func (s *selectorCache) Format(match, format string) (string, error) {
 	switch s.MatchType {
 	case spb.MatchType_EXACT:
 		return s.Identifier, nil
@@ -67,7 +67,7 @@ func (w *wildcardCache) Match(ref string) *wildcard.Match {
 	return m
 }
 
-func (s *sourceCache) wildcard() (*wildcardCache, error) {
+func (s *selectorCache) wildcard() (*wildcardCache, error) {
 	if s.w != nil {
 		return s.w, nil
 	}
@@ -79,7 +79,7 @@ func (s *sourceCache) wildcard() (*wildcardCache, error) {
 	return s.w, nil
 }
 
-func (s *sourceCache) regex() (*regexp.Regexp, error) {
+func (s *selectorCache) regex() (*regexp.Regexp, error) {
 	if s.re != nil {
 		return s.re, nil
 	}

sourcepolicy/matcher.go

@@ -8,7 +8,7 @@ import (
 	"github.com/pkg/errors"
 )
 
-func match(ctx context.Context, src *sourceCache, ref string, attrs map[string]string) (bool, error) {
+func match(ctx context.Context, src *selectorCache, ref string, attrs map[string]string) (bool, error) {
 	for _, c := range src.Constraints {
 		switch c.Condition {
 		case spb.AttrMatch_EQUAL:

sourcepolicy/matcher_test.go

@@ -305,7 +305,7 @@ func TestMatch(t *testing.T) {
 	for _, tc := range cases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
-			matches, err := match(context.Background(), &sourceCache{Selector: &tc.src}, tc.ref, tc.attrs)
+			matches, err := match(context.Background(), &selectorCache{Selector: &tc.src}, tc.ref, tc.attrs)
 			if !tc.xErr {
 				require.NoError(t, err)
 			} else {

sourcepolicy/mutate.go

@@ -4,23 +4,40 @@ import (
 	"context"
 
 	"github.com/moby/buildkit/solver/pb"
+	spb "github.com/moby/buildkit/sourcepolicy/pb"
 	"github.com/moby/buildkit/util/bklog"
+	"github.com/pkg/errors"
 )
 
 // mutate is a MutateFn which converts the source operation to the identifier and attributes provided by the policy.
 // If there is no change, then the return value should be false and is not considered an error.
-func mutate(ctx context.Context, op *pb.SourceOp, identifier string, attrs map[string]string) (bool, error) {
-	var mutated bool
-	if op.Identifier != identifier && identifier != "" {
-		mutated = true
-		op.Identifier = identifier
+func mutate(ctx context.Context, op *pb.SourceOp, rule *spb.Rule, selector *selectorCache, ref string) (bool, error) {
+	if rule.Updates == nil {
+		return false, errors.Errorf("missing destination for convert rule")
 	}
 
-	if attrs != nil {
+	dest := rule.Updates.Identifier
+	if dest == "" {
+		dest = rule.Selector.Identifier
+	}
+	dest, err := selector.Format(ref, dest)
+	if err != nil {
+		return false, errors.Wrap(err, "error formatting destination")
+	}
+
+	bklog.G(ctx).Debugf("sourcepolicy: converting %s to %s, pattern: %s", ref, dest, rule.Updates.Identifier)
+
+	var mutated bool
+	if op.Identifier != dest && dest != "" {
+		mutated = true
+		op.Identifier = dest
+	}
+
+	if rule.Updates.Attrs != nil {
 		if op.Attrs == nil {
-			op.Attrs = make(map[string]string, len(attrs))
+			op.Attrs = make(map[string]string, len(rule.Updates.Attrs))
 		}
-		for k, v := range attrs {
+		for k, v := range rule.Updates.Attrs {
 			if op.Attrs[k] != v {
 				bklog.G(ctx).Debugf("setting attr %s=%s", k, v)
 				op.Attrs[k] = v

sourcepolicy/mutate_test.go

@@ -5,14 +5,14 @@ import (
 	"testing"
 
 	"github.com/moby/buildkit/solver/pb"
+	spb "github.com/moby/buildkit/sourcepolicy/pb"
 	"github.com/stretchr/testify/require"
 )
 
 func TestMutate(t *testing.T) {
 	type testCaseOp struct {
 		op          *pb.Op
-		dest        string
-		destAttrs   map[string]string
+		rule        *spb.Rule
 		expected    bool
 		expectedOp  *pb.Op
 		expectedErr string
@@ -27,7 +27,14 @@ func TestMutate(t *testing.T) {
 					},
 				},
 			},
-			dest:     "docker-image://docker.io/library/busybox:1.34.1-uclibc@sha256:3614ca5eacf0a3a1bcc361c939202a974b4902b9334ff36eb29ffe9011aaad83",
+			rule: &spb.Rule{
+				Selector: &spb.Selector{
+					Identifier: "docker-image://docker.io/library/busybox:1.34.1-uclibc",
+				},
+				Updates: &spb.Update{
+					Identifier: "docker-image://docker.io/library/busybox:1.34.1-uclibc@sha256:3614ca5eacf0a3a1bcc361c939202a974b4902b9334ff36eb29ffe9011aaad83",
+				},
+			},
 			expected: true,
 			expectedOp: &pb.Op{
 				Op: &pb.Op_Source{
@@ -45,7 +52,14 @@ func TestMutate(t *testing.T) {
 					},
 				},
 			},
-			dest:     "docker-image://docker.io/library/busybox:latest@sha256:3614ca5eacf0a3a1bcc361c939202a974b4902b9334ff36eb29ffe9011aaad83",
+			rule: &spb.Rule{
+				Selector: &spb.Selector{
+					Identifier: "docker-image://docker.io/library/busybox",
+				},
+				Updates: &spb.Update{
+					Identifier: "docker-image://docker.io/library/busybox:latest@sha256:3614ca5eacf0a3a1bcc361c939202a974b4902b9334ff36eb29ffe9011aaad83",
+				},
+			},
 			expected: true,
 			expectedOp: &pb.Op{
 				Op: &pb.Op_Source{
@@ -64,7 +78,14 @@ func TestMutate(t *testing.T) {
 					},
 				},
 			},
-			dest:     "docker-image://docker.io/library/busybox:latest@sha256:3614ca5eacf0a3a1bcc361c939202a974b4902b9334ff36eb29ffe9011aaad83",
+			rule: &spb.Rule{
+				Selector: &spb.Selector{
+					Identifier: "docker-image://docker.io/library/busybox:latest*",
+				},
+				Updates: &spb.Update{
+					Identifier: "docker-image://docker.io/library/busybox:latest@sha256:3614ca5eacf0a3a1bcc361c939202a974b4902b9334ff36eb29ffe9011aaad83",
+				},
+			},
 			expected: true,
 			expectedOp: &pb.Op{
 				Op: &pb.Op_Source{
@@ -82,9 +103,14 @@ func TestMutate(t *testing.T) {
 					},
 				},
 			},
-			dest:      "https://raw.githubusercontent.com/moby/buildkit/v0.10.1/README.md",
-			destAttrs: map[string]string{pb.AttrHTTPChecksum: "sha256:6e4b94fc270e708e1068be28bd3551dc6917a4fc5a61293d51bb36e6b75c4b53"},
-			expected:  true,
+			rule: &spb.Rule{
+				Selector: &spb.Selector{},
+				Updates: &spb.Update{
+					Identifier: "https://raw.githubusercontent.com/moby/buildkit/v0.10.1/README.md",
+					Attrs:      map[string]string{pb.AttrHTTPChecksum: "sha256:6e4b94fc270e708e1068be28bd3551dc6917a4fc5a61293d51bb36e6b75c4b53"},
+				},
+			},
+			expected: true,
 			expectedOp: &pb.Op{
 				Op: &pb.Op_Source{
 					Source: &pb.SourceOp{
@@ -101,9 +127,9 @@ func TestMutate(t *testing.T) {
 	ctx := context.Background()
 	for _, tc := range testCases {
 		op := *tc.op
-
 		t.Run(op.String(), func(t *testing.T) {
-			mutated, err := mutate(ctx, op.GetSource(), tc.dest, tc.destAttrs)
+			src := op.GetSource()
+			mutated, err := mutate(ctx, src, tc.rule, &selectorCache{Selector: tc.rule.Selector}, src.GetIdentifier())
 			require.Equal(t, tc.expected, mutated)
 			if tc.expectedErr != "" {
 				require.Error(t, err, tc.expectedErr)