llbsolver: support pinning sources (same as the "Dockerfile.pin" proposal but agnostic to frontends)

[AkihiroSuda]

Alternative to

• dockerfile: support Dockerfile.pin for pinning sources #2816

This version is implemented on the llbsolver side and agnostic to the LLB frontends.
See solver/llbsolver/vertex.go:loadLLB().

See docs/build-repro.md for the usage.

Reproducing the pinned dependencies

Reproducing the pinned dependencies is supported in the master branch of BuildKit.

e.g.,

buildctl build --frontend dockerfile.v0 --local dockerfile=. --local context=. --source-policy-file Dockerfile.pol

An example Dockerfile.pol:

{
    "sources": [
      {
        "type": "docker-image",
        "ref": "docker.io/library/alpine:latest",
        "pin": "sha256:4edbd2beb5f78b1014028f4fbb99f3237d9561100b6881aabbf5acce2c4f9454"
      },
      {
        "type": "http",
        "ref": "https://raw.githubusercontent.com/moby/buildkit/v0.10.1/README.md",
        "pin": "sha256:6e4b94fc270e708e1068be28bd3551dc6917a4fc5a61293d51bb36e6b75c4b53"
      }
    ]
}

• sources: a subset of the ."containerimage.buildinfo".sources property of the metadata.json

Reproduction is currently supported for the following source types:

• docker-image
• http

---

In a follow-up PR, we may want to let the dockerfile frontend to load Dockerfile.pol in the context automatically. (Needs further discussion)

[AkihiroSuda]

cc @tonistiigi @cpuguy83 @sudo-bmitch

[AkihiroSuda]

@tonistiigi Could you check the design of this too 🙏

[tonistiigi]

I think this is generally what we want.

Some things to consider though. We need to make sure we can update this format in the future. I see this as a general policy control for sources that the build uses. So this can also be used for user to say that image needs to be signed(also by who?). Or that all images need to be signed etc. Then there are controls like images are only allowed to come from a specific registries. Some mirror control maybe also could belong here. Or that all the sources need to be defined, eg. builder is not allowed to pull any external sources that are not listed in this policy.

The last one may also have two modes: one is images that are actual sources for the build result(eg. the ones in buildinfo), but there are also just images used by a random subbuild in the frontend. Eg. if I'm building Docker and internally Runc's Dockerfile is also built, my policy could be that I want to make sure runc authenticity is validated but what is happening inside the Runc Dockerfile is not something I care about. It could be the opposite thought that I want to lock down everything recursively(eg. go.sum).

Would be good to try to define a format that would satisfy all these cases. Even if we currently don't have the capability to implement all that, we can then make sure that we have a viable path to upgrade without breaking users' security expectations.

---

I think sending the policy together with the user request is the correct approach, but we do need to think about the cases where a build is being invoked from Git URL directly as well. If the repo defines a policy, then it should be loaded by the Dockerfile frontend from some file definition. The file format itself can be a follow-up but would be good to leave ways for the frontend to load additional policies(unless the user wants to override with theirs of course).

[tonistiigi]

I don't think this should be saved in VertexOptions and then called during the resolve . Multiple builds with same LLB (but different policy) would share the VertexOptions.

Better would be to actually rewrite the LLB while it is getting loaded into the graph. So if image(alpine) is converted to image(alpine@sha) then its vertex digest now also changes and matches another build that called image(alpine@sha) directly.

[AkihiroSuda]

Where would be the right place to rewrite the LLB?

[tonistiigi]

Somewhere in

buildkit/solver/llbsolver/vertex.go

Lines 223 to 229 in 6805940

	if v, ok := cache[dgst]; ok {
	return v, nil
	}
	op, ok := allOps[dgst]
	if !ok {
	return nil, errors.Errorf("invalid missing input digest %s", dgst)
	}

probably. You probably need to keep mapping of original digest to new digest after the *pb.Op has been rewritten so that the parent that has input with the old digest also gets modified to the new one.

Note that all this conversion/mapping is only specific to the current load closure. Once the LLB is passed to the solver, it doesn't know anymore if it came directly from the user or passed a conversion step.

[AkihiroSuda]

Updated like this

diff --git a/solver/llbsolver/vertex.go b/solver/llbsolver/vertex.go
index 4f36c2ed..e27158c7 100644
--- a/solver/llbsolver/vertex.go
+++ b/solver/llbsolver/vertex.go
@@ -1,11 +1,13 @@
 package llbsolver
 
 import (
+       "context"
        "fmt"
        "strings"
 
        "github.com/containerd/containerd/platforms"
        "github.com/moby/buildkit/solver"
+       "github.com/moby/buildkit/solver/llbsolver/llbmutator"
        "github.com/moby/buildkit/solver/pb"
        "github.com/moby/buildkit/source"
        "github.com/moby/buildkit/util/entitlements"
@@ -143,8 +145,8 @@ func (dpc *detectPrunedCacheID) Load(op *pb.Op, md *pb.OpMetadata, opt *solver.V
        return nil
 }
 
-func Load(def *pb.Definition, opts ...LoadOpt) (solver.Edge, error) {
-       return loadLLB(def, func(dgst digest.Digest, pbOp *pb.Op, load func(digest.Digest) (solver.Vertex, error)) (solver.Vertex, error) {
+func Load(ctx context.Context, def *pb.Definition, mutator llbmutator.LLBMutator, opts ...LoadOpt) (solver.Edge, error) {
+       return loadLLB(ctx, def, mutator, func(dgst digest.Digest, pbOp *pb.Op, load func(digest.Digest) (solver.Vertex, error)) (solver.Vertex, error) {
                opMetadata := def.Metadata[dgst]
                vtx, err := newVertex(dgst, pbOp, &opMetadata, load, opts...)
                if err != nil {
@@ -187,12 +189,13 @@ func newVertex(dgst digest.Digest, op *pb.Op, opMeta *pb.OpMetadata, load func(d
 
 // loadLLB loads LLB.
 // fn is executed sequentially.
-func loadLLB(def *pb.Definition, fn func(digest.Digest, *pb.Op, func(digest.Digest) (solver.Vertex, error)) (solver.Vertex, error)) (solver.Edge, error) {
+func loadLLB(ctx context.Context, def *pb.Definition, mutator llbmutator.LLBMutator, fn func(digest.Digest, *pb.Op, func(digest.Digest) (solver.Vertex, error)) (solver.Vertex, error)) (solver.Edge, error) {
        if len(def.Def) == 0 {
                return solver.Edge{}, errors.New("invalid empty definition")
        }
 
        allOps := make(map[digest.Digest]*pb.Op)
+       mutatedDigests := make(map[digest.Digest]digest.Digest) // key: old, val: new
 
        var dgst digest.Digest
 
@@ -202,9 +205,33 @@ func loadLLB(def *pb.Definition, fn func(digest.Digest, *pb.Op, func(digest.Dige
                        return solver.Edge{}, errors.Wrap(err, "failed to parse llb proto op")
                }
                dgst = digest.FromBytes(dt)
+               if mutator != nil {
+                       mutated, err := mutator.Mutate(ctx, &op)
+                       if err != nil {
+                               return solver.Edge{}, errors.Wrap(err, "failed to call the LLB Mutator")
+                       }
+                       if mutated {
+                               dtMutated, err := op.Marshal()
+                               if err != nil {
+                                       return solver.Edge{}, err
+                               }
+                               dgstMutated := digest.FromBytes(dtMutated)
+                               mutatedDigests[dgst] = dgstMutated
+                               dgst = dgstMutated
+                       }
+               }
                allOps[dgst] = &op
        }
 
+       // Rewrite the input digests if some op was mutated
+       for _, op := range allOps {
+               for _, inp := range op.Inputs {
+                       if mutatedDgst, ok := mutatedDigests[inp.Digest]; ok {
+                               inp.Digest = mutatedDgst
+                       }
+               }
+       }
+
        if len(allOps) < 2 {
                return solver.Edge{}, errors.Errorf("invalid LLB with %d vertexes", len(allOps))
        }

[AkihiroSuda]

@tonistiigi

Would be good to try to define a format that would satisfy all these cases.

What do we need to do to ensure the extensibility?
Can we just add new JSON properties to type SourcePolicy struct ?

I think sending the policy together with the user request is the correct approach, but we do need to think about the cases where a build is being invoked from Git URL directly as well. If the repo defines a policy, then it should be loaded by the Dockerfile frontend from some file definition.

Yes, the dockerfile frontend should automatically load Dockerfile.pol when it is present in the build context.
The filename may need more bikeshedding 😅, so I guess we should revisit that in another PR.

[tonistiigi]

What do we need to do to ensure the extensibility?
Can we just add new JSON properties to type SourcePolicy struct ?

What happens when client adds new keys to policy but daemon uses the old type. Do we need some versioning here to detect that daemon is actually capable of applying the policy that user sends.

Yes, the dockerfile frontend should automatically load Dockerfile.pol when it is present in the build context.

Correct, but note that Frontend does not have access to control API where this new field is exposed atm. It is somewhat tricky actually as currently all the frontend calls that are part of same user build share the same LLBBridge (eg. they can have one policy definition). I think what we want is some kind of sub-bridge model where every frontend gets their own policy for any call that happens by that frontend. And when a frontend calls another frontend then ideally these policies would merge together same way as the user policy and the root frontend policy do.

[cpuguy83]

re: should pick up this file in the build context

This is nice, but also should be able to apply this from outside the build context.
Policy is generally defined by the team responsible for actually running docker build.
Modifying the build context is problematic in many cases as it dirties the git tree.

[tonistiigi]

@cpuguy83 Yes, policy is passed with the build request by the user. So for that case it can come from project-specific definition or from machine defaults or specific definition for a specific run. Then if frontends add their own policies they can limit the policy more (add own conversions) but can't escape the main policy(eg. can't use images from a specific registry if user policy does not allow it).

[tonistiigi]

Alias does not have a definition in here I believe.

I think rather than buildinfo this structure is more similar to the named contexts #2521 feature, except this time implemented in the LLB level and applying to all builds that are part of the same user request.

So it shouldn't be only possible to set the digest(pin) for the same source, but for example map "alpine:latest" to "busybox:latest", image ref from a registry to a local oci-layout:// directory. Replace Git URL with HTTP URL etc. Maybe also replace a source with another LLB definition like linked targets in buildx bake do (not sure how that would be defined though).

[AkihiroSuda]

Removed Alias.
I guess we will add back this as "Replace" later (in another PR)
cc @cpuguy83

[AkihiroSuda]

Do we need some versioning here to detect that daemon is actually capable of applying the policy that user sends.

Added Version int = 1

[jedevc]

I like the look of this 👀

One potential use for me is that I'd love to be able to attach a public signing key into the SourcePolicySource, which would then be able to check a signature on the source (with different implementations for git, oci images, files, etc).

The replace directive seems like a good idea to me as well, treating it as the llb-level named contexts seems like a good pattern for it. I would like it if we could make sure that this didn't conflict with the Pin value, and we should still respect it in that case. So maybe Pin should be optional, with a blank Pin indicating a wildcard? - that would also allow a user to list all of the "allowed sources" without pinning them to exact versions, so we could have a SourcePolicy that only permitted explicitly listed sources at some point.

[jedevc]

Is there any reason we couldn't use the capability-style checking that we have for versioning things today? We don't have explicit version numbers in other places I don't think, and capabilities would let us have a fine-grained control over small differences in the API over time.

[AkihiroSuda]

@tonistiigi WDYT?

[tonistiigi]

This is true for API controls. But if we imagine that in the future user would have a way to write a policy into some file, then if client that converts it into this policy object would ignore things it does not understand. Even if the server understands these fields client does not know what caps to request from the daemon.

This could be possibly implemented as user writing a version into a file and the client just converting that version to a CAP_POLICY_VERSION_N before sending. Then a special field in here is not needed.

[AkihiroSuda]

This could be possibly implemented as user writing a version into a file

What file?

[tonistiigi]

Not defined atm but there needs to be some way user can pass this that is converted to SourcePolicy struct. Read also the points about frontend integration that would probably also use the same file.

[AkihiroSuda]

Removed the version field and added CapSourcePolicy

[jedevc]

If the policy is part of controlapi.SolveRequest, it should probably be passed as part of frontend.SolveRequest?

[AkihiroSuda]

Decoupled from frontend.SolveRequest to follow the entitlement model
#2816 (comment)

[tonistiigi]

message SourcePolicySource {
	string Type = 1;
	SourcePolicyAction Action = 2; 
	string SourceIdentifier = 3; // wildcard string
	repeated SourceAttrConstraint = 4;
	
	string DestinationIdentifier = 5;
	map<string, string> DestinationAttr= 6;
	# repeated TrustChain RequiredSignatures = 7;
}

enum SourcePolicyAction {
	ALLOW
	DENY
	CONVERT
}

message SourceAttrConstraint {
	string AttrKey
	string AttrValue
	AttrMatch Condition
}

enum AttrMatch {
	EQUAL
	NOTEQUAL
	MATCHES
}

message SourcePolicy {
	int64 Version = 1; // Currently 1
	repeated SourcePolicySource Sources = 2;
}

I think this should be quite future compatible. SourceIdentifier is a wildcard comparison against https://github.com/moby/buildkit/blob/d1b0d8a/solver/pb/ops.proto#L179-L181 . Maybe without the protocol. Type is separately so that identifier value can be normalized based on it. Attrs can be used to match a specific attribute value.

Policy can allow, deny or convert a source. In case on conversion identifier is replaced and new attribute values can be defined. For example in images this would mean setting identifier to ref@sha256 and for HTTP it would mean setting the http.checksum attribute.

Every source is checked against all policy conditions. Earlier rule might deny the source, and next may allow it. If source is converted, then the new identifier needs to match the policy again (there needs to be some infinite loop protection).

Some examples:

No images are allowed except if they are busybox latest or any alpine:

[
{"docker-image", "DENY", "*"},
{"docker-image", "ALLOW", "docker.io/library/alpine:*"},
{"docker-image", "ALLOW", "docker.io/library/busybox:latest"},
]

Any Busybox points to Alpine (like a mirror), Alpine:latest is pinned (meaning busybox:latest is pinned as well)?

[
{"docker-image", "CONVERT", "docker.io/library/busybox:*", nil, "docker-image://docker.io/library/alpine:$1"},
{"docker-image", "CONVERT", "docker.io/library/alpine:latest", nil, "docker-image://docker.io/library/alpine:latest@sha256:123456"},
]

(might need a special key that sets the value a regexp?)

If build uses Alpine then use the local OCI-layout version instead

[
{"docker-image", "CONVERT", "docker.io/library/alpine:latest", nil, "oci-layout://contentstoreid@sha256"},
]

@AkihiroSuda WDYT?

fyi @slimslenderslacks

[AkihiroSuda]

Thanks @tonistiigi

SourcePolicyAction

One concern is that this policy structure looks an imperative program (like iptables rules) rather than a declarative structure (like go.mod and go.sum).
It might be hard to implement an automated locking tool like go mod tidy.

map<string, string>= 6;

What is this map for?

int64 Version = 1; // Currently 1

Do we need version? #2943 (comment)

docker-image://docker.io/library/alpine:$1

What is $1?

[tonistiigi]

One concern is that this policy structure looks an imperative program (like iptables rules) rather than a declarative structure (like go.mod and go.sum).

If the goal is for user to actually define policies, eg. images that are allowed/denied, require certain signatures etc then it can't be done with something like go mod tidy. If you are only interested of pinning, then you would only have convert rules in your policy and then you can do automatic update based on some conditions etc. Pinning is a subset, but you can use only that if you want to.

The way I would imagine this defined is that you have one policy for your project and one for your whole host. Then they would be applied on top of each other. In the host file you, for example, would likely not put any pins. In the project one, you would likely not deny a specific image but either deny all (undefined) images or define specific signatures that your images require(also something that tidy possibly can't do). Then there are the use-cases where you use conversion rules the same as --build-context today during dev without a specific policy file.

What is this map for?

New Attrs for the conversion result, fixed.

Do we need version?

No, it was just carried over.

What is $1?

First match for the wildcard component. Same as regexp. I'm flexible on how this would be actually defined. Eg. in that example busybox:test would be converted to alpine:test.

[AkihiroSuda]

@cpuguy83 SGTY?

[cpuguy83]

What do you think of using a oneof instead of special keys like oci-layout://? I guess the API has to be appended to for any new format to be supported (maybe not bad for policy like this).

I'm a bit worried about bugs in the policy engine leading to infinite loops or possibly suprising behaviors with the way things are suggested here.
Taking the example:

[
{"docker-image", "DENY", "*"},
{"docker-image", "ALLOW", "docker.io/library/alpine:*"},
{"docker-image", "ALLOW", "docker.io/library/busybox:latest"},
]

I would almost say if someone were to append a CONVERT rule where the original ref is not already allowed then the image should be denied.

Just trying to think of all the cases here.

[cpuguy83]

Agree.

[cpuguy83]

Do we have consensus on this?

[jedevc]

@cpuguy83 no objections in over a week, so tempted to say that we do have consensus 😄

[AkihiroSuda]

Consensus on avoiding OPA/Rego, and considering wasm later.
But still not sure how the buildkit-specific config would look like (to machine and humans).

Is anyone working on design and POC?

[tonistiigi]

to machine

Anything missing from the proposal on top of this comment?

and humans

I think this shouldn't be a requirement for this PR. I imagine the human format would also for example have policy for pushes that is not part of buildkit solver at all, just validation for the build parameters.

[tonistiigi]

As a follow up I think we also need to change the behavior or git source. Currently image tag with a digest ignores the tag value, if the image name has been updated then we just get the old one. For URLs there is no way to get the old data, so we error instead if the content has changed. For Git (iiuc) we pull by SHA. That might mean that the result is slightly different as the .git directory is different when making a clone of a ref or a commit. I think we need to somehow try to fake the ref in that case and make it look like as we checked out a ref that was at a specific commit even if that branch is not on that commit anymore. Maybe we should still validate that the current branch needs to contain the commit. If someone force-pushed the history then in that case it might be better to let the user know.

[cpuguy83]

Just wanted to check where we are with this.
Are we all on the same page in terms of changes that need to be done?

Anything missing from the machine config laid out above by @tonistiigi?