Controller for automatic image signature import #16293

mfojtik · 2017-09-12T12:51:32Z

This controller watches all images and automatically imports the image signatures using container/image library.

Sync never remove/override existing imported signatures
This needs a config file in /etc/containers/registry.d to work properly

openshift-ci-robot · 2017-09-12T12:51:34Z

@mfojtik: Your pull request title starts with "WIP", so the do-not-merge/work-in-progress label will be added.

This label will ensure that your pull request will not be merged. Remove the prefix from your pull request title to trigger the removal of the label and allow for your pull request to be merged.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mfojtik · 2017-09-12T12:52:48Z

@smarterclayton @deads2k only the last commit is interesting. My assumption is that the resync internal will cause all images to re-get their signatures every 10 minutes, we can bump that to 24h or something if needed.

mfojtik · 2017-09-12T12:53:26Z

Import signatures from upstream registry images

mtrmac · 2017-09-12T13:07:11Z

pkg/image/controller/signature/signature_import_controller.go

+	}
+
+	updatedImage := t.(*imageapi.Image)
+	updatedImage.Signatures = signatures


Does this overwrite all pre-existing signatures?

If so, most of the concerns in #15494 (comment) still apply.

(To be clear, syncing the signature status is a welcome improvement.)

@mtrmac see the comment below.

deads2k · 2017-09-12T14:10:20Z

pkg/image/controller/signature/signature_import_controller.go

+}
+
+func (s *SignatureImportController) syncImageSignatures(key string) error {
+	obj, exists, err := s.imageCache.GetByKey(key)


use a lister

deads2k · 2017-09-12T14:10:37Z

pkg/image/controller/signature/signature_import_controller.go

+		// this means the request was successfully handled.  We should "forget" the item so that any retry
+		// later on is reset
+		s.queue.Forget(key)
+	} else {


return early and avoid the else

deads2k · 2017-09-12T14:10:45Z

pkg/image/controller/signature/signature_import_controller.go

+		s.queue.Forget(key)
+	} else {
+		// if we had an error it means that we didn't handle it, which means that we want to requeue the work
+		utilruntime.HandleError(fmt.Errorf("error syncing service, it will be retried: %v", err))


not a service

dammit... now you know from where I copied this ;-)

deads2k · 2017-09-12T14:11:08Z

pkg/image/controller/signature/signature_import_controller.go

+		return err
+	}
+	if !exists {
+		glog.V(4).Infof("Image has been deleted %v", key)


non-existence isn't noteworthy v(5) maybe

deads2k · 2017-09-12T14:12:22Z

pkg/oc/admin/image/verify-signature.go

@@ -405,6 +406,6 @@ func (ui *unparsedImage) Manifest() ([]byte, string, error) {
 }

 // Signatures is like ImageSource.GetSignatures, but the result is cached; it is OK to call this however often you need.
-func (ui *unparsedImage) Signatures() ([][]byte, error) {
+func (ui *unparsedImage) Signatures(context.Context) ([][]byte, error) {


you going ot use this to pass a timeout or something?

@deads2k this is just implementing the interface in containers/image... this func now require context because it fetches stuff from remote locations...

deads2k · 2017-09-12T14:12:57Z

pkg/image/controller/signature/signature_import_controller.go

+		glog.V(4).Infof("Failed to fetch image %s signatures: %v", image.Name, err)
+		return err
+	}
+	if len(signatures) == 0 {


This looks odd. If the signatures are gone, why aren't we removing them?

@deads2k that will be tricky... you will have to mark the ones that we imported automatically vs. those that user created manually... see my other comment

deads2k · 2017-09-12T14:13:11Z

pkg/image/controller/signature/signature_import_controller.go

+	}
+
+	updatedImage := t.(*imageapi.Image)
+	updatedImage.Signatures = signatures


is this overwriting image signatures that a user manually added?

good point... i guess what this should do is just add new signatures and do not override the existing once... @mtrmac will that fix your concerns as well? meaning that this is only going to write new signatures and don't remove existing signatures for the image.

@mtrmac I want to sort out how we are going to sync the signatures in this controller.

We can add a common prefix for automatically imported signatures which we can safely remove when the sync runs and replace with new signatures (or delete them when we don't receive any from sigstore). That will make sure we won't touch signatures created by users manually.

Only adding signatures, never removing them, would be a good 90% solution I can be very happy with.

The remaining 10/90% is marking a signature as obsolete, and probably not serving it to clients, when it is removed from the original registry (think a signature for :latest, which should be served only for the truly latest image). This is more complex if several repos serve the same image (manifest digest) and the same signature, and only one of them stops serving that signature; it’s not really obvious what to do in that case. Ideally, I guess, the signature status for each imagestream should mirror the signature status of the upstream repository for that imagestream, but that seems like a pretty big change to the model.

We can add a common prefix for automatically imported signatures

I’m not entirely sure what you mean; is that a constant like imported-signature, or some identification of the specific image stream?

mfojtik · 2017-09-12T15:33:24Z

@mtrmac @deads2k addressed comments, refactored how we store the signatures a little bit

mfojtik · 2017-09-12T15:35:56Z

pkg/image/controller/signature/signature_import_controller.go

+		}
+	}
+
+	// Only add the signatures that we received from the signature store.


@mtrmac @deads2k FYI

Basically, we are not going to touch anything that is not managed by this controller. We going to refresh the signatures that are managed, but ignore those that already exists (and are possible verified). If we don't receive signature that we already have and we manage it, we going to nuke it regardless of its verification status. I guess this is OK (iow. a signature storage remove the signature because it is no longer valid/etc. so there is no reason for keeping it around).

I agree that local verification status should not be a factor in this.

Removing a signature just because it is not available on one registry seems incorrect: AFAICT if the same image (manifest digest) is served by repo A, which serves signatures SA and S_common, and repo B, which serves signatures SB and S_common, the imported image’s signatures will keep flipping between (SA, S_common) and (SB, S_common), depending on which was the last repo synced. That doesn’t seem right; the imported image should have all of (SA, SB, S_common).

It’s clear enough that when A stops serving SA, the imported image should drop SA. It’s not obvious when S_common should be dropped. One of A/B may be the authoritative repo for S_common (or not truly authoritative but a semi-trusted mirror of the authoritative repo), and dropping the signature by the authoritative repo would be a good reason to drop it everywhere; or it may be a quite untrusted third-party copy (something on Docker Hub perhaps), which should not be able to cause the cluster to drop S_common. I can only think of changing the system to store the existence of S_common not per-manifest-digest, but per-stream, but that’s probably a big change. On balance, I’d prefer never removing signatures to removing shared signatures when they should be kept around; the redesign, or perhaps some other solution, can hopefully be done later.

(Even if a signature has been dropped by the authoritative source, and is no longer valid / applicable, there is a reason to keep the signature around for forensic investigations (“why was this backdoored image allowed to run on the cluster?”). But that’s clearly a lower-priority use case, and it is not necessarily best implemented in the registry: perhaps a per-node audit log would make more sense.)

@mtrmac so to simplify it, we should not remove signatures that we already have but keep them on the image, right?

That’s close enough for me, yes.

mfojtik · 2017-09-12T15:37:02Z

pkg/cmd/server/origin/controller/config.go

@@ -203,6 +205,10 @@ func BuildOpenshiftControllerConfig(options configapi.MasterConfig) (*OpenshiftC
 		DisableScheduledImport:                     options.ImagePolicyConfig.DisableScheduledImport,
 		ScheduledImageImportMinimumIntervalSeconds: options.ImagePolicyConfig.ScheduledImageImportMinimumIntervalSeconds,
 	}
+	ret.ImageSignatureImportControllerConfig = ImageSignatureImportControllerConfig{
+		ResyncPeriod:          10 * time.Minute,
+		SignatureFetchTimeout: 1 * time.Minute,


@deads2k ultimately we passing this to a context of containers/image where we do http.Get to fetch the sig data... This means after 1 minute we cancel the request and retry on next sync.

mfojtik · 2017-09-12T17:26:28Z

@smarterclayton one concern: isn't updating the image with signature going to trigger every image trigger?

EDIT: I guess not, because the IST will not change, so this is safe.

smarterclayton · 2017-09-21T04:03:43Z

pkg/image/controller/signature/signature_import_controller.go

+		// This will use the name of the image (sha256:xxxx) and the SHA256 of the
+		// signature itself as the signature name has to be unique for each
+		// signature.
+		sig.Name = image.Name + "@" + fmt.Sprintf("%x", sha256.Sum256(blob))


Use the helpers we have for this

smarterclayton · 2017-09-21T04:04:16Z

pkg/image/controller/signature/signature_import_controller.go

+		sig.Annotations = map[string]string{
+			SignatureManagedAnnotation: "true",
+		}
+		sig.CreationTimestamp = metav1.Now()


Doesn't the save method do this? What about UIDs?

@smarterclayton no, the timestamps are nil..

smarterclayton · 2017-09-21T04:05:07Z

pkg/image/controller/signature/signature_import_controller.go

+
+	// fetchImageSignatures allows to mock the signature download process as the
+	// container/image requires system config in /etc/containers/registries.d.
+	fetchImageSignaturesFn func(*imageapi.Image) ([]imageapi.ImageSignature, error)


make this an interface, not an embedded func.

smarterclayton · 2017-09-21T04:06:00Z

pkg/image/controller/signature/signature_import_controller.go

+	return nil
+}
+
+func (s *SignatureImportController) fetchImageSignatures(image *imageapi.Image) ([]imageapi.ImageSignature, error) {


Split this into its own type - it has nothing to do with the controller, it is just consumed by the controller. Put it in its own file.

smarterclayton

Cleanup is an issue, we can't have unbounded signature growth on an image. Has to be some control in here.

mfojtik · 2017-09-25T08:56:07Z

@mtrmac how will we deal with cleaning up the old signatures?

mtrmac · 2017-09-25T17:57:20Z

“old” is not a matter of age. It would be nice to remove signatures if the registry from which we pulled them [or if we pulled the same signature from multiple registries, all of them] has stopped publishing it — which would require tracking the signatures per registry, or I guess per image stream, not per image.

I guess it would be reasonable to place an upper limit of signatures per image, sure; note that right now it may not be obvious to the administrator where the signatures came from / which user to punish / which registry to block. (E.g. if a registry temporarily publishes 1000 signatures claiming an unrelated image identity and then stops, the administrator would see the image with 1000 signatures, but the signatures don’t record which registry they came from and the registry would no longer admit to being the source.)

mfojtik · 2017-09-26T17:52:49Z

@smarterclayton comments addressed

smarterclayton · 2017-09-26T17:55:25Z

pkg/image/controller/signature/signature_import_controller.go

+		}
+	}
+
+	if len(newImage.Signatures) > s.signatureImportLimit {


So this means you keep the old signatures. How will admin know this is happening and react? How do they react?

they can prune signatures periodically with verify image signature remove. we can improve later with auto prune.

smarterclayton · 2017-09-26T17:55:48Z

pkg/image/controller/signature/signature_import_controller_test.go

+			expectNoUpdate: true,
+			signatures:     singleFakeSignature,
+		},
+		{


limit test case testing various outcomes

…4d635a

…2b9ed9f169d

…758d2b115b1ded3

mfojtik · 2017-09-27T09:15:11Z

@smarterclayton test updated with limits... i don't think we have to worry about having more than 3 signatures per a single image for now (we can bump the default to 5 to be absolutely safe). In 3.7+ we can figure out smarter way to rotate the signatures, but I believe this is enough for the start and better than what we have now.

mfojtik · 2017-09-27T13:00:27Z

/retest

smarterclayton · 2017-09-27T15:28:40Z

Can you add an e2e conformance test? Can do in a follow up, I'll merge this now. I want the conformance test before we ship.

/lgtm

openshift-merge-robot · 2017-09-27T15:29:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, smarterclayton

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [mfojtik,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

mfojtik · 2017-09-27T15:30:02Z

/retest

ratelimiter...

openshift-merge-robot · 2017-09-27T18:58:47Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-ci-robot · 2017-09-27T19:40:25Z

@mfojtik: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/cmd	`8940bb3`	link	`/test cmd`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-merge-robot · 2017-09-27T20:59:36Z

Automatic merge from submit-queue (batch tested with PRs 16293, 16455)

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2017

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 12, 2017

openshift-merge-robot assigned liggitt and bparees Sep 12, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2017

mfojtik mentioned this pull request Sep 12, 2017

image: automatically import images signatures #15494

Closed

mtrmac reviewed Sep 12, 2017

View reviewed changes

deads2k reviewed Sep 12, 2017

View reviewed changes

mfojtik force-pushed the signature-controller branch from 7756956 to bce010b Compare September 12, 2017 15:32

mfojtik commented Sep 12, 2017

View reviewed changes

mfojtik force-pushed the signature-controller branch 4 times, most recently from 51640f1 to 3a3884a Compare September 12, 2017 21:10

mfojtik changed the title ~~WIP: Controller for automatic image signature import~~ Controller for automatic image signature import Sep 12, 2017

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2017

mfojtik force-pushed the signature-controller branch 2 times, most recently from 16acf17 to 1257b26 Compare September 13, 2017 08:59

smarterclayton reviewed Sep 21, 2017

View reviewed changes

smarterclayton suggested changes Sep 21, 2017

View reviewed changes

mfojtik force-pushed the signature-controller branch from 1257b26 to 1d6e3a6 Compare September 26, 2017 17:52

smarterclayton reviewed Sep 26, 2017

View reviewed changes

mfojtik added 4 commits September 27, 2017 10:58

bump(github.com/containers/image): dbd0a4cee2480da39048095a326506ae11…

b707c4f

…4d635a

bump(github.com/docker/go-connections): 3ede32e2033de7505e6500d6c868c…

9e36f64

…2b9ed9f169d

bump(github.com/opencontainers/image-spec): ef2b9a1d696677abd664a0879…

13b6dc8

…758d2b115b1ded3

image: add image signature importer controller

8940bb3

mfojtik force-pushed the signature-controller branch from 1d6e3a6 to 8940bb3 Compare September 27, 2017 09:13

openshift-ci-robot assigned smarterclayton Sep 27, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2017

openshift-merge-robot merged commit 4f72a6d into openshift:master Sep 27, 2017

mfojtik mentioned this pull request Oct 4, 2017

Describe how to configure automatic image signature import openshift/openshift-docs#5485

Merged

thomasmckay mentioned this pull request Oct 5, 2017

As a user, I want to enable automatic image signatures via /etc/containers/registries.d minishift/minishift#1502

Open

mfojtik deleted the signature-controller branch September 5, 2018 21:08

Controller for automatic image signature import #16293

Controller for automatic image signature import #16293

Conversation

mfojtik commented Sep 12, 2017 • edited Loading

openshift-ci-robot commented Sep 12, 2017

mfojtik commented Sep 12, 2017

mfojtik commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton left a comment

Choose a reason for hiding this comment

mfojtik commented Sep 25, 2017

mtrmac commented Sep 25, 2017

mfojtik commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Sep 27, 2017

mfojtik commented Sep 27, 2017

smarterclayton commented Sep 27, 2017

openshift-merge-robot commented Sep 27, 2017

mfojtik commented Sep 27, 2017

openshift-merge-robot commented Sep 27, 2017

openshift-ci-robot commented Sep 27, 2017 • edited Loading

openshift-merge-robot commented Sep 27, 2017

mfojtik commented Sep 12, 2017 •

edited

Loading

mfojtik commented Sep 12, 2017 •

edited

Loading

openshift-ci-robot commented Sep 27, 2017 •

edited

Loading