Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix ResolveImageConfig to evaluate source policy #3956

Merged
merged 1 commit into from
Jul 10, 2023
Merged

Fix ResolveImageConfig to evaluate source policy #3956

merged 1 commit into from
Jul 10, 2023

Conversation

cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Jun 16, 2023 •
edited

Before this change, ResolveImageConfig was unaware of source policies. This means that:

  1. Images for denied sources may be resolved
  2. Image configs may get pulled for sources that are later converted to a different image

The update makes it so the image resolver first runs a given ref through the source policy and uses any mutated ref for the actual resolve (instead of the original ref).
It also returns the mutated ref so it can be used correctly by the frontend (e.g. don't want to do llb.Image(oldRef@resolvedDigest)).

Fixes #3911

@cpuguy83 cpuguy83 force-pushed the source_policy_resolve_image branch 2 times, most recently from b9f72e4 to 283ccb3 Compare June 16, 2023 17:39
@tonistiigi tonistiigi added this to the v0.12.0 milestone Jun 27, 2023
@tonistiigi
Copy link
Member

Looks like panic in CI:

    sandbox.go:286: panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    sandbox.go:286: 	panic: runtime error: invalid memory address or nil pointer dereference
    sandbox.go:286: [signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x11f790f]
    sandbox.go:286: 
    sandbox.go:286: goroutine 30 [running]:
    sandbox.go:286: go.opentelemetry.io/otel/sdk/trace.(*recordingSpan).End.func1()
    sandbox.go:286: 	/src/vendor/go.opentelemetry.io/otel/sdk/trace/span.go:383 +0x2a
    sandbox.go:286: go.opentelemetry.io/otel/sdk/trace.(*recordingSpan).End(0xc0003e8900, {0x0, 0x0, 0x26afc60?})
    sandbox.go:286: 	/src/vendor/go.opentelemetry.io/otel/sdk/trace/span.go:421 +0xa89
    sandbox.go:286: panic({0x16ce440, 0x2681ed0})
    sandbox.go:286: 	/usr/local/go/src/runtime/panic.go:884 +0x213
    sandbox.go:286: github.com/moby/buildkit/sourcepolicy.(*Engine).evaluatePolicy(0xc0005b93d0?, {0x1b9d9a8, 0xc0003210e0}, 0x0, 0xc000204ba0)
    sandbox.go:286: 	/src/sourcepolicy/engine.go:123 +0x1af

@cpuguy83 cpuguy83 force-pushed the source_policy_resolve_image branch 10 times, most recently from 4466473 to ca5b0c0 Compare June 30, 2023 20:18
@cpuguy83 cpuguy83 marked this pull request as ready for review June 30, 2023 21:09
@cpuguy83
Copy link
Member Author

All green here now.

tonistiigi
tonistiigi reviewed Jul 7, 2023
View reviewed changes
client/client_test.go Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
frontend/dockerui/namedcontext.go Outdated
Platform: opt.Platform,
ResolveMode: opt.ResolveMode,
LogName: fmt.Sprintf("[context %s] load metadata for %s", nameWithPlatform, ref),
ResolverType: llb.ResolverTypeRegistry,
})
if err != nil {
var e imageutil.ResolveToNonImageError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be a pointer because it looks like &ResolveToNonImageError{} is returned when this error happens.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its passed into errors.As as a pointer. We don't ever propagate this error beyond this point, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://go.dev/play/p/BQwN9fEeWHl

I don't quite understand how the current one even builds though. I get a build error if I don't remove pointer from func (e ResolveToNonImageError) Error() string { in that example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the error in the playground but it makes very little sense to me why it would be failing or why the test suite is passing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh it is passing because that's a panic not a compile error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still doesn't look correct to me in the latest version. Now it doesn't error but the As() check never matches. It's the case1 vs case2 example in the playground example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we could have a test for this as seems to be quite easy to break this behavior.

frontend/dockerui/namedcontext.go Outdated Show resolved Hide resolved
frontend/dockerui/namedcontext.go Outdated Show resolved Hide resolved
frontend/dockerui/namedcontext.go Show resolved Hide resolved
sourcepolicy/engine.go Outdated Show resolved Hide resolved
frontend/attestations/sbom/sbom.go Outdated Show resolved Hide resolved
@cpuguy83 cpuguy83 force-pushed the source_policy_resolve_image branch from ca5b0c0 to 04b2bf6 Compare July 7, 2023 17:25
tonistiigi
tonistiigi reviewed Jul 7, 2023
View reviewed changes
frontend/dockerui/namedcontext.go Outdated Show resolved Hide resolved
frontend/dockerui/namedcontext.go Outdated
Platform: opt.Platform,
ResolveMode: opt.ResolveMode,
LogName: fmt.Sprintf("[context %s] load metadata for %s", nameWithPlatform, ref),
ResolverType: llb.ResolverTypeRegistry,
})
if err != nil {
var e imageutil.ResolveToNonImageError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

@cpuguy83 cpuguy83 force-pushed the source_policy_resolve_image branch 2 times, most recently from 1145f69 to a118f4e Compare July 8, 2023 21:45
@cpuguy83
Fix ResolveImageConfig to evaluate source policy
330cf7a 
Before this change, ResolveImageConfig was unaware of source policies.
This means that:

1. Images for denied sources may be resolved
2. Image configs may get pulled for sources that are later converted to
   a different image

The update makes it so the image resolver first runs a given ref through
the source policy and uses any mutated ref for the actual resolve
(instead of the original ref).
It also returns the mutated ref so it can be used correctly by the
frontend (e.g. don't want to do llb.Image(oldRef@resolvedDigest)).

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83 cpuguy83 force-pushed the source_policy_resolve_image branch from a118f4e to 330cf7a Compare July 10, 2023 17:45
@tonistiigi tonistiigi merged commit 6b99094 into moby:master Jul 10, 2023
54 checks passed
@cpuguy83 cpuguy83 deleted the source_policy_resolve_image branch July 11, 2023 02:43
@thaJeztah thaJeztah mentioned this pull request Aug 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.12.0
Development

Successfully merging this pull request may close these issues.

source policy resolves image digest before matching
2 participants
@cpuguy83 @tonistiigi