From e91f4e762dec86790c282e75f9a446bfef81cb6d Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Mon, 29 Jun 2026 18:51:35 -0700 Subject: [PATCH] sourcepolicy: normalize parsed source identifiers Render parsed source identifiers back to their canonical SourceOp form before source policy evaluation. This lets Git subdir cleanup use the existing source parser and avoids policy-specific Git parsing. Add String methods for source identifiers and cover them with unit tests, plus a client integration regression for canonical Git subdir policy matching. Signed-off-by: Tonis Tiigi --- client/client_test.go | 32 ++++++++++++++++++++++++ solver/llbsolver/bridge.go | 4 +++ solver/llbsolver/policy.go | 17 +++++++++++++ source/containerblob/identifier.go | 4 +++ source/containerblob/identifier_test.go | 24 ++++++++++++++++++ source/containerimage/identifier.go | 8 ++++++ source/containerimage/identifier_test.go | 19 ++++++++++++++ source/git/identifier.go | 17 +++++++++++++ source/git/identifier_test.go | 9 +++++++ source/http/identifier.go | 4 +++ source/http/identifier_test.go | 19 ++++++++++++++ source/identifier.go | 3 +++ source/local/identifier.go | 4 +++ source/local/identifier_test.go | 13 ++++++++++ worker/base/worker.go | 6 ++++- worker/worker.go | 2 ++ 16 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 source/containerblob/identifier_test.go create mode 100644 source/containerimage/identifier_test.go create mode 100644 source/http/identifier_test.go create mode 100644 source/local/identifier_test.go diff --git a/client/client_test.go b/client/client_test.go index 0763cc12d972..a83c23817f00 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -13227,6 +13227,38 @@ func testSourcePolicy(t *testing.T, sb integration.Sandbox) { }) } + t.Run("deny canonical git subdir", func(t *testing.T) { + const ( + src = "git://github.com/user/repo.git#main:../absolute/path" + canonical = "git://github.com/user/repo.git#main:absolute/path" + ) + frontend := func(ctx context.Context, c gateway.Client) (*gateway.Result, error) { + st := llb.NewState(llb.NewSource(src, nil, llb.Constraints{}).Output()) + def, err := st.Marshal(sb.Context()) + if err != nil { + return nil, err + } + return c.Solve(ctx, gateway.SolveRequest{ + Definition: def.ToPB(), + }) + } + + _, err = c.Build(sb.Context(), SolveOpt{ + SourcePolicy: &sourcepolicypb.Policy{ + Rules: []*sourcepolicypb.Rule{ + { + Action: sourcepolicypb.PolicyAction_DENY, + Selector: &sourcepolicypb.Selector{ + Identifier: canonical, + MatchType: sourcepolicypb.MatchType_EXACT, + }, + }, + }, + }, + }, "", frontend, nil) + require.ErrorContains(t, err, sourcepolicy.ErrSourceDenied.Error()) + }) + t.Run("Frontend policies", func(t *testing.T) { t.Run("deny http", func(t *testing.T) { denied := "https://raw.githubusercontent.com/moby/buildkit/v0.10.1/README.md" diff --git a/solver/llbsolver/bridge.go b/solver/llbsolver/bridge.go index 18263e57f6ff..871d41085516 100644 --- a/solver/llbsolver/bridge.go +++ b/solver/llbsolver/bridge.go @@ -163,6 +163,9 @@ func (b *llbBridge) loadResult(ctx context.Context, def *pb.Definition, cacheImp } func (b *llbBridge) policy(engine *sourcepolicy.Engine) SourcePolicyEvaluator { + if engine == nil { + return nil + } return &policyEvaluator{ llbBridge: b, engine: engine, @@ -279,6 +282,7 @@ func (b *llbBridge) resolveSourceMetadata(ctx context.Context, op *pb.SourceOp, engine := sourcepolicy.NewEngine(opt.SourcePolicies) if !withPolicy { + op.Identifier = normalizedSourceIdentifier(w, op) if _, err := engine.Evaluate(ctx, op); err != nil { return nil, errors.Wrap(err, "could not resolve image due to policy") } diff --git a/solver/llbsolver/policy.go b/solver/llbsolver/policy.go index 4490bf33b823..2b9f027201c3 100644 --- a/solver/llbsolver/policy.go +++ b/solver/llbsolver/policy.go @@ -13,6 +13,7 @@ import ( "github.com/moby/buildkit/sourcepolicy" spb "github.com/moby/buildkit/sourcepolicy/pb" "github.com/moby/buildkit/sourcepolicy/policysession" + "github.com/moby/buildkit/worker" ocispecs "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" ) @@ -32,6 +33,17 @@ type policyEvaluator struct { engine *sourcepolicy.Engine } +func normalizedSourceIdentifier(w worker.Worker, op *pb.SourceOp) string { + if w == nil { + return op.GetIdentifier() + } + id, err := w.ParseSource(op, nil) + if err != nil { + return op.GetIdentifier() + } + return id.String() +} + func (p *policyEvaluator) Evaluate(ctx context.Context, op *pb.Op) (bool, error) { return p.evaluate(ctx, op, 10) } @@ -41,6 +53,11 @@ func (p *policyEvaluator) evaluate(ctx context.Context, op *pb.Op, max int) (boo if source == nil { return false, nil } + w, err := p.resolveWorker() + if err != nil { + return false, err + } + source.Identifier = normalizedSourceIdentifier(w, source) ok, err := p.engine.Evaluate(ctx, source) if err != nil { return false, err diff --git a/source/containerblob/identifier.go b/source/containerblob/identifier.go index dbb93e41d4ad..e937c098ea37 100644 --- a/source/containerblob/identifier.go +++ b/source/containerblob/identifier.go @@ -42,6 +42,10 @@ func NewImageBlobIdentifier(str string, scheme string) (*ImageBlobIdentifier, er var _ source.Identifier = (*ImageBlobIdentifier)(nil) +func (id *ImageBlobIdentifier) String() string { + return id.Scheme() + "://" + id.Reference.String() +} + func (id *ImageBlobIdentifier) Scheme() string { if id.SchemeName == "" { return srctypes.DockerImageBlobScheme diff --git a/source/containerblob/identifier_test.go b/source/containerblob/identifier_test.go new file mode 100644 index 000000000000..0636e27c46e2 --- /dev/null +++ b/source/containerblob/identifier_test.go @@ -0,0 +1,24 @@ +package containerblob + +import ( + "testing" + + srctypes "github.com/moby/buildkit/source/types" + "github.com/stretchr/testify/require" +) + +func TestImageBlobIdentifierString(t *testing.T) { + const ref = "docker.io/library/busybox@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + + id, err := NewImageBlobIdentifier(ref, srctypes.DockerImageBlobScheme) + require.NoError(t, err) + require.Equal(t, "docker-image+blob://"+ref, id.String()) +} + +func TestOCIBlobIdentifierString(t *testing.T) { + const ref = "example.com/repo/layout@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + + id, err := NewImageBlobIdentifier(ref, srctypes.OCIBlobScheme) + require.NoError(t, err) + require.Equal(t, "oci-layout+blob://"+ref, id.String()) +} diff --git a/source/containerimage/identifier.go b/source/containerimage/identifier.go index 9b6f8088df0e..c648d546af3c 100644 --- a/source/containerimage/identifier.go +++ b/source/containerimage/identifier.go @@ -36,6 +36,10 @@ func NewImageIdentifier(str string) (*ImageIdentifier, error) { var _ source.Identifier = (*ImageIdentifier)(nil) +func (id *ImageIdentifier) String() string { + return srctypes.DockerImageScheme + "://" + id.Reference.String() +} + func (*ImageIdentifier) Scheme() string { return srctypes.DockerImageScheme } @@ -75,6 +79,10 @@ func NewOCIIdentifier(str string) (*OCIIdentifier, error) { var _ source.Identifier = (*OCIIdentifier)(nil) +func (id *OCIIdentifier) String() string { + return srctypes.OCIScheme + "://" + id.Reference.String() +} + func (*OCIIdentifier) Scheme() string { return srctypes.OCIScheme } diff --git a/source/containerimage/identifier_test.go b/source/containerimage/identifier_test.go new file mode 100644 index 000000000000..a0aa684a9f6e --- /dev/null +++ b/source/containerimage/identifier_test.go @@ -0,0 +1,19 @@ +package containerimage + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestImageIdentifierString(t *testing.T) { + id, err := NewImageIdentifier("docker.io/library/busybox:latest") + require.NoError(t, err) + require.Equal(t, "docker-image://docker.io/library/busybox:latest", id.String()) +} + +func TestOCIIdentifierString(t *testing.T) { + id, err := NewOCIIdentifier("example.com/repo/layout:latest") + require.NoError(t, err) + require.Equal(t, "oci-layout://example.com/repo/layout:latest", id.String()) +} diff --git a/source/git/identifier.go b/source/git/identifier.go index 3563da351153..71bbfd085748 100644 --- a/source/git/identifier.go +++ b/source/git/identifier.go @@ -1,6 +1,7 @@ package git import ( + "path" "strings" "github.com/containerd/containerd/v2/pkg/reference" @@ -71,6 +72,22 @@ func NewGitIdentifier(remoteURL string) (*GitIdentifier, error) { return &repo, nil } +func (id *GitIdentifier) String() string { + remote := id.Remote + if u, err := gitutil.ParseURL(remote); err == nil { + remote = u.Host + path.Join("/", u.Path) + } + + ref := srctypes.GitScheme + "://" + remote + if id.Ref != "" || id.Subdir != "" { + ref += "#" + id.Ref + if id.Subdir != "" { + ref += ":" + id.Subdir + } + } + return ref +} + func (GitIdentifier) Scheme() string { return srctypes.GitScheme } diff --git a/source/git/identifier_test.go b/source/git/identifier_test.go index a3260e5a084b..362be97b0a19 100644 --- a/source/git/identifier_test.go +++ b/source/git/identifier_test.go @@ -11,6 +11,7 @@ func TestNewGitIdentifier(t *testing.T) { tests := []struct { url string expected GitIdentifier + str string }{ { url: "ssh://root@subdomain.example.hostname:2222/root/my/really/weird/path/foo.git", @@ -37,6 +38,7 @@ func TestNewGitIdentifier(t *testing.T) { Remote: "https://github.com/moby/buildkit.git", Ref: "main", }, + str: "git://github.com/moby/buildkit.git#main", }, { url: "git://github.com/user/repo.git", @@ -51,6 +53,7 @@ func TestNewGitIdentifier(t *testing.T) { Ref: "mybranch", Subdir: "mydir/mysubdir", }, + str: "git://github.com/user/repo.git#mybranch:mydir/mysubdir", }, { url: "git://github.com/user/repo.git#:mydir/mysubdir/", @@ -72,6 +75,7 @@ func TestNewGitIdentifier(t *testing.T) { Ref: "mybranch", Subdir: "mydir/mysubdir", }, + str: "git://github.com/user/repo.git#mybranch:mydir/mysubdir", }, { url: "git@github.com:user/repo.git", @@ -132,6 +136,7 @@ func TestNewGitIdentifier(t *testing.T) { Ref: "main", Subdir: "absolute/path", }, + str: "git://github.com/user/repo.git#main:absolute/path", }, { url: "https://github.com/user/repo.git#main:../", @@ -171,6 +176,7 @@ func TestNewGitIdentifier(t *testing.T) { Ref: "main", Subdir: "absolute/path", }, + str: "git://github.com/user/repo.git#main:absolute/path", }, } for _, tt := range tests { @@ -178,6 +184,9 @@ func TestNewGitIdentifier(t *testing.T) { gi, err := NewGitIdentifier(tt.url) require.NoError(t, err) require.Equal(t, tt.expected, *gi) + if tt.str != "" { + require.Equal(t, tt.str, gi.String()) + } }) } } diff --git a/source/http/identifier.go b/source/http/identifier.go index d679dedecd4d..b98219c77e9c 100644 --- a/source/http/identifier.go +++ b/source/http/identifier.go @@ -42,6 +42,10 @@ type HeaderField struct { var _ source.Identifier = (*HTTPIdentifier)(nil) +func (id *HTTPIdentifier) String() string { + return id.URL +} + func (id *HTTPIdentifier) Scheme() string { if id.TLS { return srctypes.HTTPSScheme diff --git a/source/http/identifier_test.go b/source/http/identifier_test.go new file mode 100644 index 000000000000..0b025bb1fe45 --- /dev/null +++ b/source/http/identifier_test.go @@ -0,0 +1,19 @@ +package http + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestHTTPIdentifierString(t *testing.T) { + id, err := NewHTTPIdentifier("example.com/file.tar.gz", false) + require.NoError(t, err) + require.Equal(t, "http://example.com/file.tar.gz", id.String()) +} + +func TestHTTPSIdentifierString(t *testing.T) { + id, err := NewHTTPIdentifier("example.com/file.tar.gz", true) + require.NoError(t, err) + require.Equal(t, "https://example.com/file.tar.gz", id.String()) +} diff --git a/source/identifier.go b/source/identifier.go index f591e9e34e32..749a25d79c21 100644 --- a/source/identifier.go +++ b/source/identifier.go @@ -11,6 +11,9 @@ var ( ) type Identifier interface { + // String returns the canonical source operation identifier. + String() string + // Scheme returns the scheme of the identifier so that it can be routed back // to an appropriate Source. Scheme() string diff --git a/source/local/identifier.go b/source/local/identifier.go index 32662302a8f4..0222a654e579 100644 --- a/source/local/identifier.go +++ b/source/local/identifier.go @@ -24,6 +24,10 @@ func NewLocalIdentifier(str string) (*LocalIdentifier, error) { return &LocalIdentifier{Name: str}, nil } +func (id *LocalIdentifier) String() string { + return srctypes.LocalScheme + "://" + id.Name +} + func (*LocalIdentifier) Scheme() string { return srctypes.LocalScheme } diff --git a/source/local/identifier_test.go b/source/local/identifier_test.go new file mode 100644 index 000000000000..4af658a1fd9b --- /dev/null +++ b/source/local/identifier_test.go @@ -0,0 +1,13 @@ +package local + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestLocalIdentifierString(t *testing.T) { + id, err := NewLocalIdentifier("context") + require.NoError(t, err) + require.Equal(t, "local://context", id.String()) +} diff --git a/worker/base/worker.go b/worker/base/worker.go index c066fd3d10f7..9f38754b1014 100644 --- a/worker/base/worker.go +++ b/worker/base/worker.go @@ -465,6 +465,10 @@ func (w *Worker) PruneCacheMounts(ctx context.Context, ids map[string]bool) erro return nil } +func (w *Worker) ParseSource(op *pb.SourceOp, platform *pb.Platform) (source.Identifier, error) { + return w.SourceManager.Identifier(&pb.Op_Source{Source: op}, platform) +} + func (w *Worker) ResolveSourceMetadata(ctx context.Context, op *pb.SourceOp, opt sourceresolver.Opt, sm *session.Manager, jobCtx solver.JobContext) (*sourceresolver.MetaResponse, error) { if opt.SourcePolicies != nil { return nil, errors.New("source policies can not be set for worker") @@ -487,7 +491,7 @@ func (w *Worker) ResolveSourceMetadata(ctx context.Context, op *pb.SourceOp, opt } } - id, err := w.SourceManager.Identifier(&pb.Op_Source{Source: op}, platform) + id, err := w.ParseSource(op, platform) if err != nil { return nil, err } diff --git a/worker/worker.go b/worker/worker.go index 3ac6ebf811cb..41d10e668f0a 100644 --- a/worker/worker.go +++ b/worker/worker.go @@ -15,6 +15,7 @@ import ( "github.com/moby/buildkit/solver" "github.com/moby/buildkit/solver/llbsolver/cdidevices" "github.com/moby/buildkit/solver/pb" + "github.com/moby/buildkit/source" "github.com/moby/buildkit/util/leaseutil" "github.com/moby/buildkit/util/network" ocispecs "github.com/opencontainers/image-spec/specs-go/v1" @@ -37,6 +38,7 @@ type Worker interface { LoadRef(ctx context.Context, id string, hidden bool) (cache.ImmutableRef, error) // ResolveOp resolves Vertex.Sys() to Op implementation. ResolveOp(v solver.Vertex, s frontend.FrontendLLBBridge, sm *session.Manager, proxyOpt ProxyOpt) (solver.Op, error) + ParseSource(op *pb.SourceOp, platform *pb.Platform) (source.Identifier, error) ResolveSourceMetadata(ctx context.Context, op *pb.SourceOp, opt sourceresolver.Opt, sm *session.Manager, jobCtx solver.JobContext) (*sourceresolver.MetaResponse, error) DiskUsage(ctx context.Context, opt client.DiskUsageInfo) ([]*client.UsageInfo, error) Exporter(name string, sm *session.Manager) (exporter.Exporter, error)