From 2954b82969c2bd3b68930ed74ee5e079adcada41 Mon Sep 17 00:00:00 2001 From: Piotr Halama Date: Wed, 20 Nov 2024 11:05:23 +0100 Subject: [PATCH 1/4] Fix image name parsing --- internal/validate/image.go | 57 +++++++++++------------- internal/validate/image_internal_test.go | 39 ++++++++++++++++ internal/validate/image_test.go | 14 +++--- 3 files changed, 71 insertions(+), 39 deletions(-) diff --git a/internal/validate/image.go b/internal/validate/image.go index a3209dc..d99ab6f 100644 --- a/internal/validate/image.go +++ b/internal/validate/image.go @@ -32,10 +32,6 @@ import ( "github.com/pkg/errors" ) -const ( - tagDelim = ":" -) - //go:generate mockery --name=ImageValidatorService type ImageValidatorService interface { Validate(ctx context.Context, image string, imagePullCredentials map[string]cliType.AuthConfig) error @@ -70,21 +66,27 @@ func (s *notaryService) Validate(ctx context.Context, image string, imagePullCre return nil } - split := strings.Split(image, tagDelim) + if len(image) == 0 { + return pkg.NewValidationFailedErr(errors.New("empty image provided")) + } - if len(split) != 2 { - return pkg.NewValidationFailedErr(errors.New("image name is not formatted correctly")) + ref, err := name.ParseReference(image) + if err != nil { + return pkg.NewValidationFailedErr(errors.Wrap(err, "image name could not be parsed")) } - imgRepo := split[0] - imgTag := split[1] + // name.ParseReference() uses default `latest` tag when no tag/digest was provided + // we want to block all images with no explicit tag/digest provided + if !imageContainsTag(image, ref) { + return pkg.NewValidationFailedErr(errors.New("image is missing tag or hash")) + } - expectedShaBytes, err := s.loggedGetNotaryImageDigestHash(ctx, imgRepo, imgTag) + expectedShaBytes, err := s.loggedGetNotaryImageDigestHash(ctx, ref) if err != nil { return err } - shaImageBytes, shaManifestBytes, err := s.loggedGetRepositoryDigestHash(ctx, image, imagePullCredentials) + shaImageBytes, shaManifestBytes, err := s.loggedGetRepositoryDigestHash(ctx, ref, imagePullCredentials) if err != nil { return err } @@ -101,6 +103,10 @@ func (s *notaryService) Validate(ctx context.Context, image string, imagePullCre return pkg.NewValidationFailedErr(errors.New("unexpected image hash value")) } +func imageContainsTag(image string, ref name.Reference) bool { + return strings.Contains(image, ref.Identifier()) +} + func (s *notaryService) isImageAllowed(imgRepo string) bool { for _, allowed := range s.AllowedRegistries { // repository is in allowed list @@ -111,23 +117,14 @@ func (s *notaryService) isImageAllowed(imgRepo string) bool { return false } -func (s *notaryService) loggedGetRepositoryDigestHash(ctx context.Context, image string, imagePullCredentials map[string]cliType.AuthConfig) ([]byte, []byte, error) { +func (s *notaryService) loggedGetRepositoryDigestHash(ctx context.Context, ref name.Reference, imagePullCredentials map[string]cliType.AuthConfig) ([]byte, []byte, error) { const message = "request to image registry" closeLog := helpers.LogStartTime(ctx, message) defer closeLog() - return s.getRepositoryDigestHash(image, imagePullCredentials) + return s.getRepositoryDigestHash(ref, imagePullCredentials) } -func (s *notaryService) getRepositoryDigestHash(image string, imagePullCredentials map[string]cliType.AuthConfig) ([]byte, []byte, error) { - if len(image) == 0 { - return nil, nil, pkg.NewValidationFailedErr(errors.New("empty image provided")) - } - - ref, err := name.ParseReference(image) - if err != nil { - return nil, nil, pkg.NewValidationFailedErr(errors.Wrap(err, "ref parse")) - } - +func (s *notaryService) getRepositoryDigestHash(ref name.Reference, imagePullCredentials map[string]cliType.AuthConfig) ([]byte, []byte, error) { remoteOptions := make([]remote.Option, 0) // check if we have credentials for the registry, and use them @@ -233,22 +230,18 @@ func getImageDigestHash(ref name.Reference, remoteOptions ...remote.Option) ([]b return digestBytes, manifestBytes, nil } -func (s *notaryService) loggedGetNotaryImageDigestHash(ctx context.Context, imgRepo, imgTag string) ([]byte, error) { +func (s *notaryService) loggedGetNotaryImageDigestHash(ctx context.Context, ref name.Reference) ([]byte, error) { const message = "request to notary" closeLog := helpers.LogStartTime(ctx, message) defer closeLog() - result, err := s.getNotaryImageDigestHash(ctx, imgRepo, imgTag) + result, err := s.getNotaryImageDigestHash(ctx, ref) return result, err } -func (s *notaryService) getNotaryImageDigestHash(ctx context.Context, imgRepo, imgTag string) ([]byte, error) { - if len(imgRepo) == 0 || len(imgTag) == 0 { - return nil, pkg.NewValidationFailedErr(errors.New("empty arguments provided")) - } - +func (s *notaryService) getNotaryImageDigestHash(ctx context.Context, ref name.Reference) ([]byte, error) { const messageNewRepoClient = "request to notary (NewRepoClient)" closeLog := helpers.LogStartTime(ctx, messageNewRepoClient) - c, err := s.RepoFactory.NewRepoClient(imgRepo, s.NotaryConfig) + c, err := s.RepoFactory.NewRepoClient(ref.Context().Name(), s.NotaryConfig) closeLog() if err != nil { return nil, pkg.NewUnknownResultErr(err) @@ -256,7 +249,7 @@ func (s *notaryService) getNotaryImageDigestHash(ctx context.Context, imgRepo, i const messageGetTargetByName = "request to notary (GetTargetByName)" closeLog = helpers.LogStartTime(ctx, messageGetTargetByName) - target, err := c.GetTargetByName(imgTag) + target, err := c.GetTargetByName(ref.Identifier()) closeLog() if err != nil { return nil, parseNotaryErr(err) diff --git a/internal/validate/image_internal_test.go b/internal/validate/image_internal_test.go index 14d13e4..f0a0769 100644 --- a/internal/validate/image_internal_test.go +++ b/internal/validate/image_internal_test.go @@ -6,6 +6,8 @@ import ( cliType "github.com/docker/cli/cli/config/types" "github.com/google/go-containerregistry/pkg/authn" + "github.com/google/go-containerregistry/pkg/name" + "github.com/stretchr/testify/assert" ) func Test_parseCredentials(t *testing.T) { @@ -70,3 +72,40 @@ func Test_parseCredentials(t *testing.T) { }) } } + +func Test_ImageContainsTag(t *testing.T) { + tests := []struct { + name string + image string + want bool + }{ + { + name: "image with no tag", + image: "image", + want: false, + }, + { + name: "image with tag", + image: "image:tag", + want: true, + }, + { + name: "image with digest", + image: "image@sha256:fdd33d7bf8cc80f223e30b4aa6c2ad705ffc7cf1a77697f37ed7232bc74484b0", + want: true, + }, + { + name: "image with tag and digest", + image: "image:tag@sha256:fdd33d7bf8cc80f223e30b4aa6c2ad705ffc7cf1a77697f37ed7232bc74484b0", + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ref, err := name.ParseReference(tt.image) + assert.NoError(t, err) + containsTag := imageContainsTag(tt.image, ref) + assert.Equal(t, tt.want, containsTag) + }) + } +} diff --git a/internal/validate/image_test.go b/internal/validate/image_test.go index 2eb1503..261c4b3 100644 --- a/internal/validate/image_test.go +++ b/internal/validate/image_test.go @@ -54,12 +54,12 @@ var ( hash: []byte{33, 98, 102, 200, 111, 196, 220, 239, 86, 25, 147, 11, 211, 148, 36, 88, 36, 194, 175, 82, 253, 33, 186, 124, 111, 160, 230, 24, 101, 125, 76, 59}, } differentHashImage = image{ - name: "nginx", + name: "docker.io/library/nginx", tag: "latest", hash: []byte{1, 2, 3, 4}, } untrustedImage = image{ - name: "nginx", + name: "docker.io/library/nginx", tag: "untrusted", } unknownImage = image{ @@ -107,17 +107,17 @@ func Test_Validate_InvalidImageName_ShouldReturnError(t *testing.T) { { name: "image name without semicolon", imageName: "makapaka", - expectedErrMsg: "image name is not formatted correctly", + expectedErrMsg: "image is missing tag or hash", }, { name: "", imageName: ":", - expectedErrMsg: "empty arguments provided", + expectedErrMsg: "image name could not be parsed", }, { - name: "image name with more than one semicolon", //TODO: IMO it's proper image name, but now is not allowed - imageName: "repo:port/image-name:tag", - expectedErrMsg: "image name is not formatted correctly", + name: "image name with more than two semicolon", //TODO: IMO it's proper image name, but now is not allowed + imageName: "repo:port/image-name:tag:hash", + expectedErrMsg: "image name could not be parsed", }, } for _, tt := range tests { From 775e56706c8ab79f412114541c8bc3294b4ae80e Mon Sep 17 00:00:00 2001 From: Piotr Halama Date: Wed, 20 Nov 2024 13:11:18 +0100 Subject: [PATCH 2/4] simplify validation --- internal/validate/image.go | 17 ++--------- internal/validate/image_internal_test.go | 39 ------------------------ internal/validate/image_test.go | 4 +-- 3 files changed, 4 insertions(+), 56 deletions(-) diff --git a/internal/validate/image.go b/internal/validate/image.go index d99ab6f..dfec289 100644 --- a/internal/validate/image.go +++ b/internal/validate/image.go @@ -66,21 +66,12 @@ func (s *notaryService) Validate(ctx context.Context, image string, imagePullCre return nil } - if len(image) == 0 { - return pkg.NewValidationFailedErr(errors.New("empty image provided")) - } - - ref, err := name.ParseReference(image) + // strict validation requires image name to contain domain and a tag, and/or sha256 + ref, err := name.ParseReference(image, name.StrictValidation) if err != nil { return pkg.NewValidationFailedErr(errors.Wrap(err, "image name could not be parsed")) } - // name.ParseReference() uses default `latest` tag when no tag/digest was provided - // we want to block all images with no explicit tag/digest provided - if !imageContainsTag(image, ref) { - return pkg.NewValidationFailedErr(errors.New("image is missing tag or hash")) - } - expectedShaBytes, err := s.loggedGetNotaryImageDigestHash(ctx, ref) if err != nil { return err @@ -103,10 +94,6 @@ func (s *notaryService) Validate(ctx context.Context, image string, imagePullCre return pkg.NewValidationFailedErr(errors.New("unexpected image hash value")) } -func imageContainsTag(image string, ref name.Reference) bool { - return strings.Contains(image, ref.Identifier()) -} - func (s *notaryService) isImageAllowed(imgRepo string) bool { for _, allowed := range s.AllowedRegistries { // repository is in allowed list diff --git a/internal/validate/image_internal_test.go b/internal/validate/image_internal_test.go index f0a0769..14d13e4 100644 --- a/internal/validate/image_internal_test.go +++ b/internal/validate/image_internal_test.go @@ -6,8 +6,6 @@ import ( cliType "github.com/docker/cli/cli/config/types" "github.com/google/go-containerregistry/pkg/authn" - "github.com/google/go-containerregistry/pkg/name" - "github.com/stretchr/testify/assert" ) func Test_parseCredentials(t *testing.T) { @@ -72,40 +70,3 @@ func Test_parseCredentials(t *testing.T) { }) } } - -func Test_ImageContainsTag(t *testing.T) { - tests := []struct { - name string - image string - want bool - }{ - { - name: "image with no tag", - image: "image", - want: false, - }, - { - name: "image with tag", - image: "image:tag", - want: true, - }, - { - name: "image with digest", - image: "image@sha256:fdd33d7bf8cc80f223e30b4aa6c2ad705ffc7cf1a77697f37ed7232bc74484b0", - want: true, - }, - { - name: "image with tag and digest", - image: "image:tag@sha256:fdd33d7bf8cc80f223e30b4aa6c2ad705ffc7cf1a77697f37ed7232bc74484b0", - want: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ref, err := name.ParseReference(tt.image) - assert.NoError(t, err) - containsTag := imageContainsTag(tt.image, ref) - assert.Equal(t, tt.want, containsTag) - }) - } -} diff --git a/internal/validate/image_test.go b/internal/validate/image_test.go index 261c4b3..bf22def 100644 --- a/internal/validate/image_test.go +++ b/internal/validate/image_test.go @@ -107,7 +107,7 @@ func Test_Validate_InvalidImageName_ShouldReturnError(t *testing.T) { { name: "image name without semicolon", imageName: "makapaka", - expectedErrMsg: "image is missing tag or hash", + expectedErrMsg: "image name could not be parsed", }, { name: "", @@ -116,7 +116,7 @@ func Test_Validate_InvalidImageName_ShouldReturnError(t *testing.T) { }, { name: "image name with more than two semicolon", //TODO: IMO it's proper image name, but now is not allowed - imageName: "repo:port/image-name:tag:hash", + imageName: "repo.com:123/image-name:tag:hash", expectedErrMsg: "image name could not be parsed", }, } From 9a24e9ec101d5d337df18f6dc1a2f8bbd5c7ec88 Mon Sep 17 00:00:00 2001 From: Piotr Halama Date: Wed, 20 Nov 2024 13:14:46 +0100 Subject: [PATCH 3/4] test --- internal/validate/image_test.go | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/internal/validate/image_test.go b/internal/validate/image_test.go index bf22def..ae01b6a 100644 --- a/internal/validate/image_test.go +++ b/internal/validate/image_test.go @@ -98,26 +98,31 @@ func Test_Validate_ProperIndex_ShouldPass(t *testing.T) { func Test_Validate_InvalidImageName_ShouldReturnError(t *testing.T) { cfg := validate.ServiceConfig{NotaryConfig: validate.NotaryConfig{}} f := setupMockFactory() + expectedErrMsg := "image name could not be parsed" tests := []struct { - name string - imageName string - expectedErrMsg string + name string + imageName string }{ { - name: "image name without semicolon", - imageName: "makapaka", - expectedErrMsg: "image name could not be parsed", + name: "image name without tag and domain", + imageName: "makapaka", }, { - name: "", - imageName: ":", - expectedErrMsg: "image name could not be parsed", + name: "image name without domain", + imageName: "makapaka:latest", }, { - name: "image name with more than two semicolon", //TODO: IMO it's proper image name, but now is not allowed - imageName: "repo.com:123/image-name:tag:hash", - expectedErrMsg: "image name could not be parsed", + name: "image name without tag", + imageName: "domain.com/makapaka", + }, + { + name: "", + imageName: ":", + }, + { + name: "image name with more than two semicolon in hash", + imageName: "repo.com:123/image-name:tag:hash", }, } for _, tt := range tests { @@ -126,7 +131,7 @@ func Test_Validate_InvalidImageName_ShouldReturnError(t *testing.T) { err := s.Validate(context.TODO(), tt.imageName, emptyAuthData) - require.ErrorContains(t, err, tt.expectedErrMsg) + require.ErrorContains(t, err, expectedErrMsg) }) } } From bcff0796e7f56934a52eefa8a6c38732c96a7eee Mon Sep 17 00:00:00 2001 From: Piotr Halama Date: Wed, 20 Nov 2024 13:24:15 +0100 Subject: [PATCH 4/4] Adjust test to provide a valid image name --- internal/admission/defaulting_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/admission/defaulting_test.go b/internal/admission/defaulting_test.go index 7aac919..0865a6d 100644 --- a/internal/admission/defaulting_test.go +++ b/internal/admission/defaulting_test.go @@ -48,7 +48,7 @@ func TestTimeout(t *testing.T) { ns := corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNs, Labels: map[string]string{pkg.NamespaceValidationLabel: pkg.NamespaceValidationEnabled}}} pod := corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod", Namespace: testNs}, - Spec: corev1.PodSpec{Containers: []corev1.Container{{Image: "test:test"}}}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Image: "eu-gcr.io/test:test"}}}, } raw, err := json.Marshal(pod)