Merge pull request #3656 from tonistiigi/policy-error

policy: mark policy progress vertex on DENY build failures

build/build.go

@@ -272,18 +272,18 @@ func warnOnNoOutput(ctx context.Context, nodes []builder.Node, opts map[string]O
 	logrus.Warnf("%s. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load", warnNoOutputBuf.String())
 }
 
-func newBuildRequests(ctx context.Context, docker *dockerutil.Client, cfg *confutil.Config, drivers map[string][]*noderesolver.ResolvedNode, w progress.Writer, opts map[string]Options) (_ map[string][]*reqForNode, _ func(), retErr error) {
+func newBuildRequests(ctx context.Context, docker *dockerutil.Client, cfg *confutil.Config, drivers map[string][]*noderesolver.ResolvedNode, w progress.Writer, opts map[string]Options) (_ map[string][]*reqForNode, _ func(error), retErr error) {
 	reqForNodes := make(map[string][]*reqForNode)
 
-	var releasers []func()
+	var releasers []func(error)
-	releaseAll := func() {
+	releaseAll := func(inErr error) {
 		for _, fn := range releasers {
-			fn()
+			fn(inErr)
 		}
 	}
 	defer func() {
 		if retErr != nil {
-			releaseAll()
+			releaseAll(retErr)
 		}
 	}()
 
@@ -432,7 +432,9 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opts map[
 	if err != nil {
 		return nil, err
 	}
-	defer release()
+	defer func() {
+		release(err)
+	}()
 
 	// validate that all links between targets use same drivers
 	if err := validateTargetLinks(reqForNodes, drivers, opts); err != nil {

build/opt.go

@@ -213,18 +213,27 @@ func (l *policyProgressLogger) sendVertexComplete(started time.Time, err error)
 	l.ch <- &client.SolveStatus{Vertexes: []*client.Vertex{&vtx}}
 }
 
-func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt *Options, bopts gateway.BuildOpts, cfg *confutil.Config, pw progress.Writer, docker *dockerutil.Client) (_ *client.SolveOpt, release func(), err error) {
+func isPolicyEvaluationError(policies []*policy.Policy, err error) bool {
+	for _, p := range policies {
+		if p != nil && p.IsPolicyError(err) {
+			return true
+		}
+	}
+	return false
+}
+
+func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt *Options, bopts gateway.BuildOpts, cfg *confutil.Config, pw progress.Writer, docker *dockerutil.Client) (_ *client.SolveOpt, release func(error), err error) {
 	nodeDriver := node.Driver
-	defers := make([]func(), 0, 2)
+	defers := make([]func(error), 0, 2)
-	releaseF := func() {
+	releaseF := func(inErr error) {
 		for _, f := range defers {
-			f()
+			f(inErr)
 		}
 	}
 
 	defer func() {
 		if err != nil {
-			releaseF()
+			releaseF(err)
 		}
 	}()
 
@@ -432,7 +441,9 @@ func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt *O
 					if err != nil {
 						return nil, nil, err
 					}
-					defers = append(defers, cancel)
+					defers = append(defers, func(error) {
+						cancel()
+					})
 					opt.Exports[i].Output = func(_ map[string]string) (io.WriteCloser, error) {
 						return w, nil
 					}
@@ -478,7 +489,9 @@ func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt *O
 	if err != nil {
 		return nil, nil, err
 	}
-	defers = append(defers, releaseLoad)
+	defers = append(defers, func(error) {
+		releaseLoad()
+	})
 
 	if opt.Inputs.policy == nil {
 		if len(opt.Policy) > 0 {
@@ -512,8 +525,13 @@ func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt *O
 		if len(policyFiles) > 0 {
 			policyLogger = newPolicyProgressLogger(pw, fmt.Sprintf("loading policies %s", strings.Join(policyFiles, ", ")))
 		}
+		var policies []*policy.Policy
 		if policyLogger != nil {
-			defers = append(defers, func() {
+			defers = append(defers, func(inErr error) {
+				if len(policysession.DenyMessages(inErr)) > 0 || isPolicyEvaluationError(policies, inErr) {
+					policyLogger.Close(inErr)
+					return
+				}
 				policyLogger.Close(nil)
 			})
 		}
@@ -537,6 +555,7 @@ func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt *O
 				VerifierProvider: policy.SignatureVerifier(cfg),
 				DefaultPlatform:  defaultPlatform(bopts),
 			})
+			policies = append(policies, p)
 			cbs = append(cbs, p.CheckPolicy)
 			if popt.Strict {
 				if bopts.LLBCaps.Supports(pb.CapSourcePolicySession) != nil {

policy/policy_error_test.go

@@ -0,0 +1,51 @@
+package policy
+
+import (
+	"errors"
+	"testing"
+
+	gwpb "github.com/moby/buildkit/frontend/gateway/pb"
+	solverpb "github.com/moby/buildkit/solver/pb"
+	"github.com/moby/buildkit/sourcepolicy/policysession"
+	"github.com/stretchr/testify/require"
+)
+
+func TestPolicyIsPolicyError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{
+			name: "matches-recorded-source",
+			err:  errors.New("failed to solve: error evaluating the source policy: source \"docker-image://busybox:latest\" not allowed by policy: action DENY"),
+			want: true,
+		},
+		{
+			name: "does-not-match-without-buildkit-pattern",
+			err:  errors.New("failed to parse dockerfile for docker-image://busybox:latest"),
+			want: false,
+		},
+		{
+			name: "does-not-match-unrelated-error",
+			err:  errors.New("failed to solve: error evaluating the source policy: source \"docker-image://alpine:latest\" not allowed by policy: action DENY"),
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			p := NewPolicy(Opt{})
+			req := &policysession.CheckPolicyRequest{
+				Source: &gwpb.ResolveSourceMetaResponse{
+					Source: &solverpb.SourceOp{
+						Identifier: "docker-image://busybox:latest",
+					},
+				},
+			}
+			p.recordDenyIdentifier(req)
+
+			require.Equal(t, tt.want, p.IsPolicyError(tt.err))
+		})
+	}
+}

policy/validate.go

@@ -10,6 +10,7 @@ import (
 	"path"
 	"slices"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/containerd/platforms"
@@ -32,6 +33,9 @@ import (
 type Policy struct {
 	opt   Opt
 	funcs []fun
+
+	denyMu          sync.Mutex
+	denyIdentifiers map[string]struct{}
 }
 
 type state struct {
@@ -84,6 +88,43 @@ func (p *Policy) log(level logrus.Level, format string, v ...any) {
 	p.opt.Log(level, fmt.Sprintf(format, v...))
 }
 
+func (p *Policy) recordDenyIdentifier(req *policysession.CheckPolicyRequest) {
+	if p == nil || req == nil || req.Source == nil || req.Source.Source == nil {
+		return
+	}
+
+	p.denyMu.Lock()
+	defer p.denyMu.Unlock()
+
+	if p.denyIdentifiers == nil {
+		p.denyIdentifiers = make(map[string]struct{})
+	}
+	id := strings.TrimSpace(req.Source.Source.Identifier)
+	if id == "" {
+		return
+	}
+	p.denyIdentifiers[id] = struct{}{}
+}
+
+func (p *Policy) IsPolicyError(err error) bool {
+	if p == nil || err == nil {
+		return false
+	}
+	errText := err.Error()
+	// TODO: replace this string matching with a typed BuildKit error that is
+	// always attached for policy DENY decisions.
+	p.denyMu.Lock()
+	defer p.denyMu.Unlock()
+	for id := range p.denyIdentifiers {
+		pattern := fmt.Sprintf("source %q not allowed by policy: action %s", id, moby_buildkit_v1_sourcepolicy.PolicyAction_DENY.String())
+		if strings.Contains(errText, pattern) {
+			return true
+		}
+	}
+
+	return false
+}
+
 func (p *Policy) CheckPolicy(ctx context.Context, req *policysession.CheckPolicyRequest) (*policysession.DecisionResponse, *gwpb.ResolveSourceMetaRequest, error) {
 	if req.Source == nil || req.Source.Source == nil {
 		return nil, nil, errors.Errorf("no source info in request")
@@ -307,6 +348,9 @@ func (p *Policy) CheckPolicy(ctx context.Context, req *policysession.CheckPolicy
 	for _, dm := range resp.DenyMessages {
 		p.log(logrus.InfoLevel, " - %s", dm.Message)
 	}
+	if resp.Action == moby_buildkit_v1_sourcepolicy.PolicyAction_DENY {
+		p.recordDenyIdentifier(req)
+	}
 
 	return resp, nil, nil
 }

tests/policy_build.go

@@ -1,11 +1,13 @@
 package tests
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
 	"net/url"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/containerd/continuity/fs/fstest"
@@ -13,6 +15,7 @@ import (
 	"github.com/distribution/reference"
 	"github.com/docker/buildx/util/gitutil"
 	"github.com/docker/buildx/util/gitutil/gittestutil"
+	"github.com/moby/buildkit/client"
 	"github.com/moby/buildkit/identity"
 	"github.com/moby/buildkit/util/contentutil"
 	"github.com/moby/buildkit/util/testutil"
@@ -25,6 +28,7 @@ import (
 var policyBuildTests = []func(t *testing.T, sb integration.Sandbox){
 	testBuildPolicyAllow,
 	testBuildPolicyDeny,
+	testBuildPolicyDenyProgressStream,
 	testBuildPolicyImageName,
 	testBuildPolicyEnv,
 	testBuildPolicyHTTP,
@@ -103,6 +107,80 @@ decision := {"allow": allow, "deny_msg": deny_msg}
 	require.Contains(t, string(out), "DENY")
 }
 
+func testBuildPolicyDenyProgressStream(t *testing.T, sb integration.Sandbox) {
+	skipNoCompatBuildKit(t, sb, ">= 0.26.0-0", "policy input requires BuildKit v0.26.0+")
+	dockerfile := []byte(`
+FROM busybox:latest
+RUN echo policy-nope
+`)
+	policyFile := []byte(`
+package docker
+
+default allow = false
+
+allow if not input.image
+
+decision := {"allow": allow}
+`)
+	dir := tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+		fstest.CreateFile("policy.rego", policyFile, 0600),
+	)
+	policyPath := filepath.Join(dir, "policy.rego")
+
+	cmd := buildxCmd(sb, withDir(dir), withArgs(
+		"build",
+		"--progress=rawjson",
+		"--policy", "filename="+policyPath,
+		"--output=type=cacheonly",
+		dir,
+	))
+	out, err := cmd.CombinedOutput()
+	outStr := string(out)
+	require.Error(t, err, outStr)
+
+	policyVertexName := "loading policies " + policyPath
+	sawPolicyVertex := false
+	sawPolicyCompleted := false
+	policyVertexDigest := ""
+	policyVertexError := ""
+
+	for line := range strings.SplitSeq(outStr, "\n") {
+		line = strings.TrimSpace(line)
+		if !strings.HasPrefix(line, "{") {
+			continue
+		}
+		var st client.SolveStatus
+		require.NoError(t, json.Unmarshal([]byte(line), &st), line)
+		for _, v := range st.Vertexes {
+			if v == nil {
+				continue
+			}
+			if v.Name == policyVertexName {
+				sawPolicyVertex = true
+				if v.Digest != "" {
+					policyVertexDigest = string(v.Digest)
+				}
+			}
+			if policyVertexDigest == "" || string(v.Digest) != policyVertexDigest {
+				continue
+			}
+			if v.Completed != nil {
+				sawPolicyCompleted = true
+			}
+			if v.Error != "" {
+				policyVertexError = v.Error
+			}
+		}
+	}
+
+	require.True(t, sawPolicyVertex, outStr)
+	require.True(t, sawPolicyCompleted, outStr)
+	require.Contains(t, policyVertexError, "not allowed by policy", outStr)
+	require.Contains(t, outStr, "not allowed by policy")
+}
+
 func testBuildPolicyImageName(t *testing.T, sb integration.Sandbox) {
 	skipNoCompatBuildKit(t, sb, ">= 0.26.0-0", "policy input requires BuildKit v0.26.0+")
 	registry, err := sb.NewRegistry()