From 048ebea48304a8a38e5af682c5bfc440976da83f Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 18 Feb 2026 17:19:54 -0800 Subject: [PATCH] policy: mark policy progress vertex on DENY build failures Track denied source identifiers during policy evaluation and flag the policy progress vertex as failed when BuildKit returns a matching DENY error pattern. This improves the progress output of policy error and shows last policy logs with the build error. Signed-off-by: Tonis Tiigi --- build/build.go | 14 ++++++---- build/opt.go | 35 +++++++++++++++++------ policy/policy_error_test.go | 56 +++++++++++++++++++++++++++++++++++++ policy/validate.go | 44 +++++++++++++++++++++++++++++ 4 files changed, 135 insertions(+), 14 deletions(-) create mode 100644 policy/policy_error_test.go diff --git a/build/build.go b/build/build.go index 738a82190..dab40e7d8 100644 --- a/build/build.go +++ b/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() - releaseAll := func() { + var releasers []func(error) + 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 { diff --git a/build/opt.go b/build/opt.go index d0758406a..44e5d4e1f 100644 --- a/build/opt.go +++ b/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) - releaseF := func() { + defers := make([]func(error), 0, 2) + 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 { diff --git a/policy/policy_error_test.go b/policy/policy_error_test.go new file mode 100644 index 000000000..b1afcdc2d --- /dev/null +++ b/policy/policy_error_test.go @@ -0,0 +1,56 @@ +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 TestPolicyIsPolicyErrorMatchesRecordedSource(t *testing.T) { + p := NewPolicy(Opt{}) + req := &policysession.CheckPolicyRequest{ + Source: &gwpb.ResolveSourceMetaResponse{ + Source: &solverpb.SourceOp{ + Identifier: "docker-image://busybox:latest", + }, + }, + } + p.recordDenyIdentifier(req) + + err := errors.New("failed to solve: error evaluating the source policy: source \"docker-image://busybox:latest\" not allowed by policy: action DENY") + require.True(t, p.IsPolicyError(err)) +} + +func TestPolicyIsPolicyErrorDoesNotMatchWithoutBuildkitPattern(t *testing.T) { + p := NewPolicy(Opt{}) + req := &policysession.CheckPolicyRequest{ + Source: &gwpb.ResolveSourceMetaResponse{ + Source: &solverpb.SourceOp{ + Identifier: "docker-image://busybox:latest", + }, + }, + } + p.recordDenyIdentifier(req) + + err := errors.New("failed to parse dockerfile for docker-image://busybox:latest") + require.False(t, p.IsPolicyError(err)) +} + +func TestPolicyIsPolicyErrorDoesNotMatchUnrelatedError(t *testing.T) { + p := NewPolicy(Opt{}) + req := &policysession.CheckPolicyRequest{ + Source: &gwpb.ResolveSourceMetaResponse{ + Source: &solverpb.SourceOp{ + Identifier: "docker-image://busybox:latest", + }, + }, + } + p.recordDenyIdentifier(req) + + err := errors.New("failed to solve: error evaluating the source policy: source \"docker-image://alpine:latest\" not allowed by policy: action DENY") + require.False(t, p.IsPolicyError(err)) +} diff --git a/policy/validate.go b/policy/validate.go index 91fd56112..ef0ae71a3 100644 --- a/policy/validate.go +++ b/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 }