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.

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

Ensure read access when resolving run image location #2010

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions internal/fakes/fake_access_checker.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package fakes

type FakeAccessChecker struct {
RegistriesToFail []string
}

func NewFakeAccessChecker() *FakeAccessChecker {
return &FakeAccessChecker{}
}

func (f *FakeAccessChecker) Check(repo string) bool {
for _, toFail := range f.RegistriesToFail {
if toFail == repo {
return false
}
}

return true
}
2 changes: 1 addition & 1 deletion pkg/client/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error {
return errors.Wrapf(err, "invalid builder %s", style.Symbol(opts.Builder))
}

runImageName := c.resolveRunImage(opts.RunImage, imgRegistry, builderRef.Context().RegistryStr(), bldr.DefaultRunImage(), opts.AdditionalMirrors, opts.Publish)
runImageName := c.resolveRunImage(opts.RunImage, imgRegistry, builderRef.Context().RegistryStr(), bldr.DefaultRunImage(), opts.AdditionalMirrors, opts.Publish, c.accessChecker)

fetchOptions := image.FetchOptions{
Daemon: !opts.Publish,
Expand Down
3 changes: 3 additions & 0 deletions pkg/client/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ func testBuild(t *testing.T, when spec.G, it spec.S) {
subject *Client
fakeImageFetcher *ifakes.FakeImageFetcher
fakeLifecycle *ifakes.FakeLifecycle
fakeAccessChecker *ifakes.FakeAccessChecker
defaultBuilderStackID = "some.stack.id"
defaultWindowsBuilderStackID = "some.windows.stack.id"
defaultBuilderImage *fakes.Image
Expand All @@ -80,6 +81,7 @@ func testBuild(t *testing.T, when spec.G, it spec.S) {
var err error

fakeImageFetcher = ifakes.NewFakeImageFetcher()
fakeAccessChecker = ifakes.NewFakeAccessChecker()
fakeLifecycle = &ifakes.FakeLifecycle{}

tmpDir, err = os.MkdirTemp("", "build-test")
Expand Down Expand Up @@ -136,6 +138,7 @@ func testBuild(t *testing.T, when spec.G, it spec.S) {
logger: logger,
imageFetcher: fakeImageFetcher,
downloader: blobDownloader,
accessChecker: fakeAccessChecker,
lifecycleExecutor: fakeLifecycle,
docker: docker,
buildpackDownloader: buildpackDownloader,
Expand Down
20 changes: 20 additions & 0 deletions pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,13 @@ type BuildpackDownloader interface {
Download(ctx context.Context, buildpackURI string, opts buildpack.DownloadOptions) (buildpack.BuildModule, []buildpack.BuildModule, error)
}

//go:generate mockgen -package testmocks -destination ../testmocks/mock_access_checker.go github.com/buildpacks/pack/pkg/client AccessChecker

// AccessChecker is an interface for checking remote images for read access
type AccessChecker interface {
Check(repo string) bool
}

// Client is an orchestration object, it contains all parameters needed to
// build an app image using Cloud Native Buildpacks.
// All settings on this object should be changed through ClientOption functions.
Expand All @@ -90,6 +97,7 @@ type Client struct {
keychain authn.Keychain
imageFactory ImageFactory
imageFetcher ImageFetcher
accessChecker AccessChecker
downloader BlobDownloader
lifecycleExecutor LifecycleExecutor
buildpackDownloader BuildpackDownloader
Expand Down Expand Up @@ -125,6 +133,14 @@ func WithFetcher(f ImageFetcher) Option {
}
}

// WithAccessChecker supply your own AccessChecker.
// A AccessChecker returns true if an image is accessible for reading.
func WithAccessChecker(f AccessChecker) Option {
return func(c *Client) {
c.accessChecker = f
}
}

// WithDownloader supply your own downloader.
// A Downloader is used to gather buildpacks from both remote urls, or local sources.
func WithDownloader(d BlobDownloader) Option {
Expand Down Expand Up @@ -225,6 +241,10 @@ func NewClient(opts ...Option) (*Client, error) {
}
}

if client.accessChecker == nil {
client.accessChecker = image.NewAccessChecker(client.logger, client.keychain)
}

if client.buildpackDownloader == nil {
client.buildpackDownloader = buildpack.NewDownloader(
client.logger,
Expand Down
10 changes: 10 additions & 0 deletions pkg/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/sclevine/spec"
"github.com/sclevine/spec/report"

"github.com/buildpacks/pack/pkg/image"
"github.com/buildpacks/pack/pkg/logging"
"github.com/buildpacks/pack/pkg/testmocks"
h "github.com/buildpacks/pack/testhelpers"
Expand Down Expand Up @@ -122,4 +123,13 @@ func testClient(t *testing.T, when spec.G, it spec.S) {
h.AssertEq(t, cl.registryMirrors, registryMirrors)
})
})

when("#WithAccessChecker", func() {
it("uses AccessChecker provided", func() {
ac := &image.Checker{}
cl, err := NewClient(WithAccessChecker(ac))
h.AssertNil(t, err)
h.AssertSameInstance(t, cl.accessChecker, ac)
})
})
}
21 changes: 15 additions & 6 deletions pkg/client/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func (c *Client) parseTagReference(imageName string) (name.Reference, error) {
return ref, nil
}

func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, runImageMetadata builder.RunImageMetadata, additionalMirrors map[string][]string, publish bool) string {
func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, runImageMetadata builder.RunImageMetadata, additionalMirrors map[string][]string, publish bool, accessChecker AccessChecker) string {
if runImage != "" {
c.logger.Debugf("Using provided run-image %s", style.Symbol(runImage))
return runImage
Expand All @@ -44,6 +44,7 @@ func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, run
runImageMetadata.Image,
runImageMetadata.Mirrors,
additionalMirrors[runImageMetadata.Image],
accessChecker,
)

switch {
Expand Down Expand Up @@ -107,8 +108,8 @@ func contains(slc []string, v string) bool {
return false
}

func getBestRunMirror(registry string, runImage string, mirrors []string, preferredMirrors []string) string {
runImageList := append(append(append([]string{}, preferredMirrors...), runImage), mirrors...)
func getBestRunMirror(registry string, runImage string, mirrors []string, preferredMirrors []string, accessChecker AccessChecker) string {
runImageList := filterImageList(append(append(append([]string{}, preferredMirrors...), runImage), mirrors...), accessChecker)
for _, img := range runImageList {
ref, err := name.ParseReference(img, name.WeakValidation)
if err != nil {
Expand All @@ -119,9 +120,17 @@ func getBestRunMirror(registry string, runImage string, mirrors []string, prefer
}
}

if len(preferredMirrors) > 0 {
return preferredMirrors[0]
return runImageList[0]
}

func filterImageList(imageList []string, accessChecker AccessChecker) []string {
var accessibleImages []string

for i, img := range imageList {
if accessChecker.Check(img) {
accessibleImages = append(accessibleImages, imageList[i])
}
}

return runImage
return accessibleImages
}
35 changes: 28 additions & 7 deletions pkg/client/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/sclevine/spec/report"

"github.com/buildpacks/pack/internal/builder"
ifakes "github.com/buildpacks/pack/internal/fakes"
"github.com/buildpacks/pack/pkg/logging"
h "github.com/buildpacks/pack/testhelpers"
)
Expand All @@ -31,6 +32,7 @@ func testCommon(t *testing.T, when spec.G, it spec.S) {
gcrRegistry string
gcrRunMirror string
stackInfo builder.StackMetadata
accessChecker *ifakes.FakeAccessChecker
assert = h.NewAssertionManager(t)
)

Expand All @@ -54,27 +56,28 @@ func testCommon(t *testing.T, when spec.G, it spec.S) {
},
},
}
accessChecker = ifakes.NewFakeAccessChecker()
})

when("passed specific run image", func() {
it("selects that run image", func() {
runImgFlag := "flag/passed-run-image"
runImageName := subject.resolveRunImage(runImgFlag, defaultRegistry, "", stackInfo.RunImage, nil, false)
runImageName := subject.resolveRunImage(runImgFlag, defaultRegistry, "", stackInfo.RunImage, nil, false, accessChecker)
assert.Equal(runImageName, runImgFlag)
})
})

when("publish is true", func() {
it("defaults to run-image in registry publishing to", func() {
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, true)
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, true, accessChecker)
assert.Equal(runImageName, gcrRunMirror)
})

it("prefers config defined run image mirror to stack defined run image mirror", func() {
configMirrors := map[string][]string{
runImageName: {defaultRegistry + "/unique-run-img"},
}
runImageName := subject.resolveRunImage("", defaultRegistry, "", stackInfo.RunImage, configMirrors, true)
runImageName := subject.resolveRunImage("", defaultRegistry, "", stackInfo.RunImage, configMirrors, true, accessChecker)
assert.NotEqual(runImageName, defaultMirror)
assert.Equal(runImageName, defaultRegistry+"/unique-run-img")
})
Expand All @@ -83,7 +86,7 @@ func testCommon(t *testing.T, when spec.G, it spec.S) {
configMirrors := map[string][]string{
runImageName: {defaultRegistry + "/unique-run-img"},
}
runImageName := subject.resolveRunImage("", "test.registry.io", "", stackInfo.RunImage, configMirrors, true)
runImageName := subject.resolveRunImage("", "test.registry.io", "", stackInfo.RunImage, configMirrors, true, accessChecker)
assert.NotEqual(runImageName, defaultMirror)
assert.Equal(runImageName, defaultRegistry+"/unique-run-img")
})
Expand All @@ -92,7 +95,7 @@ func testCommon(t *testing.T, when spec.G, it spec.S) {
// If publish is false, we are using the local daemon, and want to match to the builder registry
when("publish is false", func() {
it("defaults to run-image in registry publishing to", func() {
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, false)
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, false, accessChecker)
assert.Equal(runImageName, defaultMirror)
assert.NotEqual(runImageName, gcrRunMirror)
})
Expand All @@ -101,7 +104,7 @@ func testCommon(t *testing.T, when spec.G, it spec.S) {
configMirrors := map[string][]string{
runImageName: {defaultRegistry + "/unique-run-img"},
}
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, configMirrors, false)
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, configMirrors, false, accessChecker)
assert.NotEqual(runImageName, defaultMirror)
assert.Equal(runImageName, defaultRegistry+"/unique-run-img")
})
Expand All @@ -110,10 +113,28 @@ func testCommon(t *testing.T, when spec.G, it spec.S) {
configMirrors := map[string][]string{
runImageName: {defaultRegistry + "/unique-run-img"},
}
runImageName := subject.resolveRunImage("", defaultRegistry, "test.registry.io", stackInfo.RunImage, configMirrors, false)
runImageName := subject.resolveRunImage("", defaultRegistry, "test.registry.io", stackInfo.RunImage, configMirrors, false, accessChecker)
assert.NotEqual(runImageName, defaultMirror)
assert.Equal(runImageName, defaultRegistry+"/unique-run-img")
})
})

when("desirable run-image is not accessible", func() {
it.Before(func() {
accessChecker.RegistriesToFail = []string{
gcrRunMirror,
stackInfo.RunImage.Image,
}
})

it.After(func() {
accessChecker.RegistriesToFail = nil
})

it("selects the first accessible run-image", func() {
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, true, accessChecker)
assert.Equal(runImageName, defaultMirror)
})
})
})
}
4 changes: 3 additions & 1 deletion pkg/client/rebase.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ func (c *Client) Rebase(ctx context.Context, opts RebaseOptions) error {
"",
runImageMD,
opts.AdditionalMirrors,
opts.Publish)
opts.Publish,
c.accessChecker,
)

if runImageName == "" {
return errors.New("run image must be specified")
Expand Down
7 changes: 5 additions & 2 deletions pkg/client/rebase_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func testRebase(t *testing.T, when spec.G, it spec.S) {
when("#Rebase", func() {
var (
fakeImageFetcher *ifakes.FakeImageFetcher
fakeAccessChecker *ifakes.FakeAccessChecker
subject *Client
fakeAppImage *fakes.Image
fakeRunImage *fakes.Image
Expand All @@ -37,6 +38,7 @@ func testRebase(t *testing.T, when spec.G, it spec.S) {

it.Before(func() {
fakeImageFetcher = ifakes.NewFakeImageFetcher()
fakeAccessChecker = ifakes.NewFakeAccessChecker()

fakeAppImage = fakes.NewImage("some/app", "", &fakeIdentifier{name: "app-image"})
h.AssertNil(t, fakeAppImage.SetLabel("io.buildpacks.lifecycle.metadata",
Expand All @@ -54,8 +56,9 @@ func testRebase(t *testing.T, when spec.G, it spec.S) {

fakeLogger := logging.NewLogWithWriters(&out, &out)
subject = &Client{
logger: fakeLogger,
imageFetcher: fakeImageFetcher,
logger: fakeLogger,
imageFetcher: fakeImageFetcher,
accessChecker: fakeAccessChecker,
}
})

Expand Down
41 changes: 41 additions & 0 deletions pkg/image/access_checker.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package image

import (
"github.com/buildpacks/imgutil/remote"
"github.com/google/go-containerregistry/pkg/authn"

"github.com/buildpacks/pack/pkg/logging"
)

type Checker struct {
logger logging.Logger
keychain authn.Keychain
}

func NewAccessChecker(logger logging.Logger, keychain authn.Keychain) *Checker {
checker := &Checker{
logger: logger,
keychain: keychain,
}

if checker.keychain == nil {
checker.keychain = authn.DefaultKeychain
}

Check warning on line 23 in pkg/image/access_checker.go

View check run for this annotation

Codecov / codecov/patch

pkg/image/access_checker.go#L22-L23

Added lines #L22 - L23 were not covered by tests

return checker
}

func (c *Checker) Check(repo string) bool {
img, err := remote.NewImage(repo, c.keychain)
if err != nil {
return false
}

Check warning on line 32 in pkg/image/access_checker.go

View check run for this annotation

Codecov / codecov/patch

pkg/image/access_checker.go#L31-L32

Added lines #L31 - L32 were not covered by tests

if ok, err := img.CheckReadAccess(); ok {
c.logger.Debugf("CheckReadAccess succeeded for the run image %s", repo)
return true

Check warning on line 36 in pkg/image/access_checker.go

View check run for this annotation

Codecov / codecov/patch

pkg/image/access_checker.go#L35-L36

Added lines #L35 - L36 were not covered by tests
} else {
c.logger.Debugf("CheckReadAccess failed for the run image %s, error: %s", repo, err.Error())
return false
}
}
34 changes: 34 additions & 0 deletions pkg/image/access_checker_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package image_test

import (
"bytes"
"testing"

"github.com/buildpacks/lifecycle/auth"
"github.com/sclevine/spec"
"github.com/sclevine/spec/report"

"github.com/buildpacks/pack/pkg/image"
"github.com/buildpacks/pack/pkg/logging"
h "github.com/buildpacks/pack/testhelpers"
)

func TestChecker(t *testing.T) {
spec.Run(t, "Checker", testChecker, spec.Report(report.Terminal{}))
}

func testChecker(t *testing.T, when spec.G, it spec.S) {
when("#Check", func() {
it("fails when checking dummy image", func() {
buf := &bytes.Buffer{}

keychain, err := auth.DefaultKeychain("pack.test/dummy")
h.AssertNil(t, err)

ic := image.NewAccessChecker(logging.NewSimpleLogger(buf), keychain)

h.AssertFalse(t, ic.Check("pack.test/dummy"))
h.AssertContains(t, buf.String(), "DEBUG: CheckReadAccess failed for the run image pack.test/dummy")
})
})
}
Loading