Merge pull request #19037 from miminar/naughty-image-signature-annotation

Automatic merge from submit-queue (batch tested with PRs 18905, 18968, 19016, 19037, 19056).

Drop image signature annotations

Because they were never meant to be allowed.

Includes and superseds #19030 (kudos to @deads2k)
Closes #19011

Resolves: [rhbz#1557607](https://bugzilla.redhat.com/show_bug.cgi?id=1557607)

/assign @bparees @deads2k 
/cc @mfojtik

Author: openshift-merge-robot

pkg/image/controller/signature/container_image_downloader.go

@@ -62,9 +62,6 @@ func (s *containerImageSignatureDownloader) DownloadImageSignatures(image *image
 		// signature.
 		sig.Name = imageapi.JoinImageStreamImage(image.Name, fmt.Sprintf("%x", sha256.Sum256(blob)))
 		sig.Content = blob
-		sig.Annotations = map[string]string{
-			SignatureManagedAnnotation: "true",
-		}
 		sig.CreationTimestamp = metav1.Now()
 		ret = append(ret, sig)
 	}

pkg/image/controller/signature/signature_import_controller.go

@@ -19,12 +19,6 @@ import (
 	imagelister "github.com/openshift/origin/pkg/image/generated/listers/image/internalversion"
 )
 
-const (
-	// SignatureManagedAnnotation marks signatures that were imported by this
-	// controller.
-	SignatureManagedAnnotation = "image.openshift.io/managed-signature"
-)
-
 type SignatureDownloader interface {
 	DownloadImageSignatures(*imageapi.Image) ([]imageapi.ImageSignature, error)
 }

pkg/image/registry/image/strategy.go

@@ -16,6 +16,9 @@ import (
 	"github.com/openshift/origin/pkg/image/util"
 )
 
+// managedSignatureAnnotation used to be set by image signature import controller as a signature annotation.
+const managedSignatureAnnotation = "image.openshift.io/managed-signature"
+
 // imageStrategy implements behavior for Images.
 type imageStrategy struct {
 	runtime.ObjectTyper
@@ -49,6 +52,8 @@ func (s imageStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Obje
 		newImage.DockerImageManifest = ""
 		newImage.DockerImageConfig = ""
 	}
+
+	removeManagedSignatureAnnotation(newImage)
 }
 
 // Validate validates a new image.
@@ -127,9 +132,20 @@ func (s imageStrategy) PrepareForUpdate(ctx apirequest.Context, obj, old runtime
 		newImage.DockerImageManifest = ""
 		newImage.DockerImageConfig = ""
 	}
+
+	removeManagedSignatureAnnotation(newImage)
 }
 
 // ValidateUpdate is the default update validation for an end user.
 func (imageStrategy) ValidateUpdate(ctx apirequest.Context, obj, old runtime.Object) field.ErrorList {
-	return validation.ValidateImageUpdate(old.(*imageapi.Image), obj.(*imageapi.Image))
+	return validation.ValidateImageUpdate(obj.(*imageapi.Image), old.(*imageapi.Image))
+}
+
+// removeManagedSignatureAnnotation removes deprecated annotation from image signatures. A bug in image update
+// logic allowed to set arbitrary annotations that would otherwise be rejected by validation.
+// Resolves rhbz#1557607
+func removeManagedSignatureAnnotation(img *imageapi.Image) {
+	for i := range img.Signatures {
+		delete(img.Signatures[i].Annotations, managedSignatureAnnotation)
+	}
 }

pkg/image/registry/image/strategy_test.go

@@ -76,13 +76,20 @@ func TestStrategyPrepareForCreate(t *testing.T) {
 
 	Strategy.PrepareForCreate(ctx, image)
 
-	if len(image.Signatures) != len(fuzzed.Signatures) {
-		t.Errorf("unexpected number of signatures: %d != %d", len(image.Signatures), len(fuzzed.Signatures))
+	testVerifySignatures(t, fuzzed, image)
+}
+
+func testVerifySignatures(t *testing.T, orig, new *imageapi.Image) {
+	if len(new.Signatures) != len(orig.Signatures) {
+		t.Errorf("unexpected number of signatures: %d != %d", len(new.Signatures), len(orig.Signatures))
 	}
 
-	for i, sig := range image.Signatures {
+	for i, sig := range new.Signatures {
+		// expect annotations to be cleared
+		delete(orig.Signatures[i].Annotations, managedSignatureAnnotation)
+
 		vi := reflect.ValueOf(&sig).Elem()
-		vf := reflect.ValueOf(&fuzzed.Signatures[i]).Elem()
+		vf := reflect.ValueOf(&orig.Signatures[i]).Elem()
 		typeOfT := vf.Type()
 
 		for j := 0; j < vf.NumField(); j++ {
@@ -98,3 +105,65 @@ func TestStrategyPrepareForCreate(t *testing.T) {
 		}
 	}
 }
+
+func TestStrategyPrepareForCreateSignature(t *testing.T) {
+	ctx := apirequest.NewDefaultContext()
+
+	original := imageapi.Image{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "image",
+		},
+	}
+
+	seed := int64(2703387474910584091) //rand.Int63()
+	fuzzed := fuzzImage(t, &original, seed)
+
+	if len(fuzzed.Signatures) == 0 {
+		t.Fatalf("fuzzifier failed to generate signatures")
+	}
+
+	for _, tc := range []struct {
+		name        string
+		annotations map[string]string
+		expected    map[string]string
+	}{
+		{
+			name:        "unset annotations",
+			annotations: nil,
+			expected:    nil,
+		},
+		{
+			name:        "empty annotations",
+			annotations: map[string]string{},
+			expected:    map[string]string{},
+		},
+		{
+			name:        "managed annotation shall be removed",
+			annotations: map[string]string{managedSignatureAnnotation: "value"},
+			expected:    map[string]string{},
+		},
+		{
+			name:        "other annotations shall stay",
+			annotations: map[string]string{"key": "value"},
+			expected:    map[string]string{"key": "value"},
+		},
+		{
+			name:        "remove and keep",
+			annotations: map[string]string{"key": "value", managedSignatureAnnotation: "true"},
+			expected:    map[string]string{"key": "value"},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			fuzzed.Signatures[0].Annotations = tc.annotations
+			image := fuzzed.DeepCopy()
+
+			Strategy.PrepareForCreate(ctx, image)
+
+			testVerifySignatures(t, fuzzed, image)
+
+			if !reflect.DeepEqual(image.Signatures[0].Annotations, tc.expected) {
+				t.Errorf("unexpected signature annotations: %s", diff.ObjectGoPrintDiff(image.Annotations, tc.expected))
+			}
+		})
+	}
+}