Verify image signatures #12616

mfojtik · 2017-01-23T13:54:37Z

This will allow image-auditor role to verify the image signatures (for images that were pushing using skopeo or atomic push) using local GPG keys.

Note that this is just to provide the information about whether the signature content is valid or not. It does not indicate whether the image is valid or not to be scheduled on node (it is node job to revoke running image with invalid signature when policy.json does not allow the signature itself).

The command has following syntax:

$ oc adm verify-image-signature  sha256:<digest> --confirm
sha256:<digest> signature is verified (signed by key: "<keyID>")

The --confirm flag indicates whether the Image object on the server should be updated to reflect the current verification state.

To remove the signature verification you can use:

$ oc adm verify-image-signature sha256:<digest> --confirm --remove

In both cases the "user" have to be at least "image-auditor" (@smarterclayton I don't think we need to add 'image-admin' role as the auditor seems pretty accurate and already exists?).

In addition, the describer for image is updated to show signature verification status (if image has signature) example: https://gist.github.com/mfojtik/834f51f92672f70885d1c0a724aa2d2a

mfojtik · 2017-01-23T13:57:49Z

Note that we are supposed to use the https://github.com/containers/image library here to perform the verification, however in the current state we can't pull it to openshift godeps because:

The openpgp support is not merged yet
Allow choosing between a gpgme and openpgp signature backend using a build tag containers/image#207 adding the openpgp introduces a new build flag
It causes mess with Kubernetes imports (as containers/image pulls latest Kubernetes in...)

This PR bundles the piece we need from containers/image we need to perform the openpgp verification and it can be replaced with the library when it become ready.

@smarterclayton do we want to hold off this until the containers/image is ready?

smarterclayton · 2017-01-23T23:30:19Z

Why does containers/image depend on Kube? That's a no-no for us. On Jan 23, 2017, at 8:57 AM, Michal Fojtik <notifications@github.com> wrote: Note that we are supposed to use the https://github.com/containers/image library here to perform the verification, however in the current state we can't pull it to openshift godeps because: 1. The openpgp support is not merged yet 2. containers/image#207 <containers/image#207> adding the openpgp introduces a new build flag 3. It causes mess with Kubernetes imports (as containers/image pulls latest Kubernetes in...) This PR bundles the piece we need from containers/image we need to perform the openpgp verification and it can be replaced with the library when it become ready. @smarterclayton <https://github.com/smarterclayton> do we want to hold off this until the containers/image is ready? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12616 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p2SJ-wariStN5ug0ld8Ko6Csly6gks5rVLHfgaJpZM4LrEKf> .

mfojtik · 2017-01-24T09:17:30Z

@smarterclayton they fixed it but another two emerge...

containers/image#207 (comment)

and I'm not really commited to bump distribution and go-connections :-/

miminar · 2017-01-24T10:09:56Z

pkg/image/registry/image/strategy.go

@@ -39,9 +39,6 @@ func (s imageStrategy) PrepareForCreate(ctx kapi.Context, obj runtime.Object) {
 	if err := api.ImageWithMetadata(newImage); err != nil {
 		utilruntime.HandleError(fmt.Errorf("Unable to update image metadata for %q: %v", newImage.Name, err))
 	}
-
-	// clear signature fields that will be later set by server once it's able to parse the content
-	s.clearSignatureDetails(newImage)


~~Validate the signatures.~~ Right, the signatures are validated anyway.

@miminar i think the signatures are already validated in API validation.

miminar · 2017-01-24T11:04:48Z

pkg/cmd/cli/describe/describer.go

+		formatString(out, "\tStatus", "Verified")
+		formatString(out, "\tIssued By", s.IssuedBy.CommonName)
+		if len(s.Conditions) > 0 {
+			for _, c := range s.Conditions {


Could you squash conditions of the same time? (Print just the latest value)

miminar · 2017-01-24T11:07:40Z

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

+	"github.com/spf13/cobra"
+	kapi "k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/unversioned"
+	kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"


Split OpenShift, kube and other deps into groups.

miminar · 2017-01-24T11:10:45Z

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

+
+	This command verifies if the signature attached to an image is trusted by
+	using the provided public GPG key.
+	By default this command will not record the signature condition back to the Image object but only


By default comma
a signature condition

miminar · 2017-01-24T11:17:02Z

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

+	if o.Confirm && !o.Remove {
+		if me, err := o.Client.Users().Get("~"); err != nil {
+			return err
+		} else {


no else is necessary

it is because me exists only in the if scope.

miminar · 2017-01-24T11:26:58Z

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

+		return err
+	}
+	if len(img.Signatures) == 0 {
+		return fmt.Errorf("%s does not have signature", img.Name)


miminar · 2017-01-24T11:29:33Z

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

+			if o.Confirm {
+				fmt.Fprintf(o.ErrOut, "%s: %v\n", o.InputImage, signatureErr)
+			} else {
+				return fmt.Errorf("%s: %v", o.InputImage, signatureErr)


I'd like to see all the errors regardless of --confirm flag.

you will, this is here just to support --remove

miminar · 2017-01-24T11:37:54Z

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

+			}
+		}
+
+		if o.Confirm {


I don't see o.Remove being handled.

miminar · 2017-01-24T11:39:05Z

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

+				return err
+			}
+		} else {
+			fmt.Fprintf(o.Out, "(add --confirm to record signature verification status to server\n")


not closed parenthesis

miminar · 2017-01-24T11:40:37Z

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

+			// TODO: This should not be just a key id but a human-readable identity.
+			img.Signatures[i].IssuedBy.CommonName = signedBy
+			// Record updated information back to the server
+			if _, err := o.Client.Images().Update(img); err != nil {


Can this be called just once for all the signatures?

mtrmac · 2017-01-24T15:50:33Z

This PR bundles the piece we need from containers/image we need to perform the openpgp verification and it can be replaced with the library when it become ready.

Sorry, merely doing a GPG verification and throwing away the contents is ABSOLUTELY INSUFFICIENT. This way any properly signed blob applies to any image; I can take a valid signature of RHEL7.3, attach it it to a container image of backdoored Kali Linux, and it would be accepted!

mfojtik · 2017-01-24T15:55:56Z

@mtrmac agree, I will add the verification of the message content as well (iow. I will read the signature and compare the critical.image against the image you are verifying the image.

Note that this is just a temporary solution, we still want to take advantage of containers/image once we resolve our dependencies.

mtrmac · 2017-01-24T16:05:41Z

@mtrmac agree, I will add the verification of the message content as well (iow. I will read the signature and compare the critical.image against the image you are verifying the image.

critical.identity needs to be verified as well (probably with an option for the user to use an user-specified string instead of the registry location), otherwise an attacker can substitute RHEL 5.1.0 for RHEL 7.3.10.

mfojtik · 2017-01-24T17:49:50Z

@mtrmac that one is tricky...

Given I have this in message: {"identity":{"docker-reference":"172.30.115.179:5000/test/test:latest"} and I have the 'Image' object from OpenShift. What I can do is verify this against dockerImageReference which unfortunatelly does not specify the "tag" (I assume you can have multiple tags what are still the same image). The format of dockerImageReference is 172.30.115.179:5000/test/test@sha256:7e1396956931054689085b8351ebffbaa12ce2df732bcc1a062ec910a5049762.

Will a comparison of 172.30.115.179:5000/test/test be enough?

mtrmac · 2017-01-24T17:52:56Z

Will a comparison of 172.30.115.179:5000/test/test be enough?

That depends on what the image is (is this a daily build created within this cluster? An base image from a supplier), and that’s one of the reasons why the fairly complex policy.json format exists.

mtrmac · 2017-01-24T17:54:25Z

… but note that if the UI asks the user to specify a manifest digest (“image ID”) but not the tag, then the user can not make a distinction between rhel:8.5.4 and rhel:8.5.5. That must be possible in the UI.

mtrmac · 2017-01-24T17:56:54Z

So, the minimal, policy.json-less version, is that the user specifies a tag (always), and may (optionally) be allowed to also specify a manifest digest which must match. Then name:tag must match the contents of the critical.identity.docker-reference, +- docker.io/library etc. canonicalization, and the digest must match the manifest digest of the actual image AND critical.image.docker-manifest-digest.

miminar · 2017-01-25T07:35:44Z

Will a comparison of 172.30.115.179:5000/test/test be enough?

In manifest schema 1, there's a Tag specified in the manifest - equal to the tag the docker daemon used to upload the image. That may be what you want. However, manifest schema 2 doesn't have it.

miminar · 2017-01-25T08:20:21Z

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

+	// .gnupg/pubring.gpg will be ignored.
+	if len(o.PublicKeyFilename) > 0 {
+		if o.Remove {
+			return kcmdutil.UsageError(cmd, "cannot use public key when removing verifications status")


verification

miminar · 2017-01-25T08:24:16Z

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

+// removeImageSignature removes the current image signature from the Image object by
+// erasing all signature fields that were previously set (when image signature was
+// previously verified).
+func (o *VerifyImageSignatureOptions) removeImageSignature(s *imageapi.ImageSignature) {


Name of this suggests that the whole signature will be removed. What about clearSignatureVerificationStatus?

miminar · 2017-01-25T08:29:26Z

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

+		// If we have error and --confirm then remove the signature from the image which
+		// changes the image to "unverified" state.
+		if signatureErr != nil || signatureContentErr != nil {
+			o.removeImageSignature(&img.Signatures[i])


Can we record the error in the signature as as a condition {"Type": "Trusted", "Status": "false", "Reason": "error", "Message": err}?

@miminar we can but then you have to deal with the existing "trusted" condition that is set to "true", so should you set that to "false" now? ;-)

Exactly ... {"Type": "Trusted", "Status": "False"} ➡️ `"Trusted": "False" ➡️ not trusted.

When you're dealing with "Trusted" condition, you have to check the status for "True" anyway to consider the image trusted.

mfojtik · 2017-01-25T12:20:32Z

@mtrmac I think we should discuss this IRL, but my take on this is:

I think matching the digest guarantees that the content which was signed is verified
You can have multiple tags pointing to the same digest, does that make the signature invalid for those tags, even if the content is the same?
I can see the point where you don't want to trust images that have third-party registry in their name, iow. you don't want to send your authentication credentials to remote random IP address... but that seems like something the node enforcement should guarantee by policy.json.

However, I think the "digest" guarantees the content (if now, Docker will be out of business now).

What I'm inclined to do here is to add extra option --verify-identity that will do this strict identity matching. However, as @miminar pointed out, we don't have "tags" in v2 schema so I think we can do that just for v1 images.

mtrmac · 2017-01-25T13:51:52Z

Will a comparison of 172.30.115.179:5000/test/test be enough?
In manifest schema 1, there's a Tag specified in the manifest - equal to the tag the docker daemon used to upload the image. That may be what you want. However, manifest schema 2 doesn't have it.

No; that value is inside the manifest and already authenticated by the manifest digest.

The point is to match what the user wanted with the image they got. With manifest digest matching, it is impossible to use a RHEL 8.20.1 signature for a RHEL 5.0.0vulnerable image; but without docker reference matching it is still possible to use a RHEL 5.0.0vulnerable image with an old RHEL 5.0.0vulnerable signature, and the user would accept that combination even if they actually wanted a RHEL 8.20.1.

mfojtik · 2017-01-25T14:02:39Z

@mtrmac would it be enough to get the IST based on what the image identity has and check that the IST points to an image with digest matching the signature digest?

This will only work for images that has been imported to OpenShift (iow. has IST already created), I don't think this will work for external images where we will have to fetch the manifest. That brings up an interesting question whether we are OK sending user credentials to the IP:PORT contained in the identity (guess that is OK if the signature key is valid? ;-)

mtrmac · 2017-01-25T14:09:24Z

I think matching the digest guarantees that the content which was signed is verified

Verified to be what? The ISV producing the image will not have an individual public key for every image (that wouldn’t make sense, in that case managing trust in unsigned images would be just as hard as managing trust in public keys), so necessarily there will be multiple images produced by the same ISV signed by the same key.

And then it is insufficient to verify that “this image has been sometime in the past been signed by $ISV”; an overwhelming majority of such images will be old vulnerable versions. Or they may be properly signed images with debugging enabled such that they log highly private data into the logs.

The user must know and specify what image they expect to get, and the signature must not only prove that the image came from that ISV, but also that it is the expected identity.

When things align well, a registry reference (registry-hostname:5000/repo/name:tag) used to fetch the image alone inherently specifies “what image the user expects to get”. When they don’t align well, see [ref] below.

Yes, it is sad that the “docker reference” format has no defined semantics and sometimes users care about the total identity up to the tag (r.a.rh.com/rhel7/rhel:7.50.2), and sometimes anything after the host name is quite substitutable (ci-cluster.example.com/daily-builds-never-in-production/productX/versionY/dayZ), but we can’t change that.

You can have multiple tags pointing to the same digest, does that make the signature invalid for those tags, even if the content is the same?

By default, yes. That’s why an image may have multiple signatures. And that’s also why the policy.json format supports more relaxed matching, where the repository name is matched but the tags are ignored.

I can see the point where you don't want to trust images that have third-party registry in their name, iow. you don't want to send your authentication credentials to remote random IP address... but that seems like something the node enforcement should guarantee by policy.json.

No, signature verification is too late for that; by the time signature verification happens, at least the manifest has been fetched (= credentials have already been sent). Not sending credentials for $serverA to $serverB is something which must be implemented in the client, it is not something the signature verification can help with.

[ref] For third parties, there is a different concern: the signature will contain the third-party registry hostname (isv.example.com/product:version), so when mirroring the images into a local registry, those signatures will no longer match the path used to fetch them (customer.example.com/isv-mirror/product:version). Again, the policy.json format supports defining such rules.

However, I think the "digest" guarantees the content (if now, Docker will be out of business now).

The digest guarantees immutability of the content, i.e. that all images which are fetched using the digest are identical. It does not guarantee identity of the content: just because all nodes on the cluster have exactly the same backdoored image does not make that image safe to use.

Or, to put it another way, it is perfectly fine for the user to say oc adm verify-image-signature $digest if the user knows what $digest value to use—but how does the user know what $digest to use? If the answer to that is “ask the registry for what digest a $tag maps to” (which is, in most cases, the best the user can do), then the registry is trusted (= can answer with any value it wants, and that’s what will be used) and we can just not do signatures at all.

Or, yet another way, the signature is a cryptographic assertion that the ISV has, as $identity, published an image with $digest. $digest then allows fetching the whole image and knowing that that’s what the ISV intended, but $identity is what says that $digest is what the user wants.

What I'm inclined to do here is to add extra option --verify-identity that will do this strict identity matching.

That really must be the default, at least by the time we start signing RHEL images (hopefully in a few months).

However, as @miminar pointed out, we don't have "tags" in v2 schema so I think we can do that just for v1 images.

(No, tags in the manifest are irrelevant. User intent is what matters.)

mtrmac · 2017-01-25T14:10:21Z

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

+	}
+	type critical struct {
+		Image criticalImage `json:"image"`
+	}


Any unknown JSON field in critical, and any subobjects, must cause the signature to be rejected. That's what critical means in this format.

This is still outstanding.

mtrmac · 2017-01-25T14:11:41Z

@mtrmac would it be enough to get the IST based on what the image identity has and check that the IST points to an image with digest matching the signature digest?

Sorry, what is an ”IST”?

mfojtik · 2017-01-25T14:16:38Z

Or, to put it another way, it is perfectly fine for the user to say oc adm verify-image-signature $digest if the user knows what $digest value to use—but how does the user know what $digest to use? If the answer to that is “ask the registry for what digest a $tag maps to” (which is, in most cases, the best the user can do), then the registry is trusted (= can answer with any value it wants, and that’s what will be used) and we can just not do signatures at all.

# oc get images --as=system:admin | grep test/test
sha256:afe4f6df9095f3c34510277fb4da74c4c7275cf9f8cef31711dd9f6ac5db7ca9   172.30.115.179:5000/test/test@sha256:afe4f6df9095f3c34510277fb4da74c4c7275cf9f8cef31711dd9f6ac5db7ca9
# oc adm verify-image-signature sha256:afe4f6df9095f3c34510277fb4da74c4c7275cf9f8cef31711dd9f6ac5db7ca9 --as=system:admin  --confirm
sha256:afe4f6df9095f3c34510277fb4da74c4c7275cf9f8cef31711dd9f6ac5db7ca9 signature is verified (signed by key: "1B9044044ECF04945897EE88172B61E538AAC0EE")

IST is ImageStreamTag. See: b73ce56

mfojtik · 2017-01-25T14:18:56Z

@mtrmac note that above won't work if the ImageStreamTag "test/test:latest" does not exists (eg. it was not imported). Which might be fine for verification.

mtrmac · 2017-01-25T14:21:01Z

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

+		return err
+	}
+
+	if tag.Image.Name != digest {


This verifies that there exists an image with reference pointing to digest. Where is the user intent?

@mtrmac that verifies that the "tag" specified in the critical.identity exists and the image associated with that tag have digest that matches the critical.image.

Yes but that tag value may be anything. How does that protect against the RHEL 8.20.5 vs. 5.0.0vulnerable substitution?

@mtrmac I don't understand what you mean with substitution. How do you substitute that in signature without re-pushing the image again which will result in a different digest and that image, imported in OpenShift will have unverified signature by default.

Pushing an image using a different tag (or repo) does not have to modify the manifest:

With schema 2, neither the repo nor the tag are in the manifest (it is possible to tag a single image with multiple tags in multiple repos)

With schema 1, strictly speaking the repo and tag are included in the manifest, so pushing to a different destination would change the manifest, but in practice most implementations (including recent OpenShift versions) completely ignore those fields (which is, overall, good, BTW).

mtrmac · 2017-01-25T14:25:22Z

# oc get images --as=system:admin | grep test/test
sha256:afe4f6df9095f3c34510277fb4da74c4c7275cf9f8cef31711dd9f6ac5db7ca9   172.30.115.179:5000/test/test@sha256:afe4f6df9095f3c34510277fb4da74c4c7275cf9f8cef31711dd9f6ac5db7ca9
# oc adm verify-image-signature sha256:afe4f6df9095f3c34510277fb4da74c4c7275cf9f8cef31711dd9f6ac5db7ca9 --as=system:admin  --confirm
sha256:afe4f6df9095f3c34510277fb4da74c4c7275cf9f8cef31711dd9f6ac5db7ca9 signature is verified (signed by key: "1B9044044ECF04945897EE88172B61E538AAC0EE")

Attack (on b73ce56 ): take an existing old test/vulnerable, with an existing signature for test/vulnerable, which exists on the registry but does not have the verified flag (perhaps it has never had it, perhaps it has been explicitly remove). Publish it on the registry as test/test. The code will see a test/vulnerable signature which points to the expected af…a9 image identity, which passes the test, and mark the image as correctly signed test/test.

mfojtik · 2017-01-25T14:32:04Z

@mtrmac if the signature has identity set to "test/vulneable" and you import it is as "test/test", the identity won't match with "test/test" and the verification will fail for that image.

mfojtik · 2017-01-25T14:35:32Z

@mtrmac ok, maybe I understand the attack you mentioned... that means, we have to compare it with the o.InputImage which is the digest you expect the image to have. In that case we just need to match not against digest in signature but against the provided image id (sha256), which match the user intent to verify the image is the one it claims to be.

See 85b5b00

mtrmac · 2017-01-25T16:02:57Z

@mtrmac if the signature has identity set to "test/vulneable" and you import it is as "test/test", the identity won't match with "test/test" and the verification will fail for that image.

Maybe I am just being stupid—which code path where does that? It certainly doesn’t happen on image import into the registry, and I couldn't see it in this code either.

mtrmac · 2017-01-25T16:11:38Z

that means, we have to compare it with the o.InputImage which is the digest you expect the image to have. In that case we just need to match not against digest in signature but against the provided image id (sha256), which match the user intent to verify the image is the one it claims to be.

That needs to happen also, but the human does not assign any meaning to a digest. Where did the human get the digest, and how does the human know that the digest is the digest of the ISV-provided image and not a digest of any other backdoored image uploaded by anyone else with write access to the registry?

AFAICT, when verifying an image signature, the human must specify the identity, either implicitly:

verify-signature-of myRepository:5000/myproject/image:version

(pulling that tag, using the digest that tag points to, and verifying that a signature claims a combination of that tag and that digest) or perhaps explicitly

verify-signature-of --expecting-identity registry.access.redhat.com/rhel7/rhel:8.20.5 myRepository:5000/rhelmirror/rhel7:currentbaseimage

(pulling from myRepository, and verifying that a signature claims r.a.rh.com/rhel7).

(The latter allows you to create an implementation which does not depend on policy.json; instead of declaratively writing a policy which says “myRepository:5000/rhelmirror must contain identities from
r.a.rh.com/rhel7/rhel”, users can write a script like

for tag in $(list-image-tags myRepository:5000/rhelmirror); do
    verify-signature-of --expecting-identity r.a.rh.com/rhel7/rhel:$(map-tag $tag) myRepository:5000/rhelmirror:$tag
done

)

miminar · 2017-01-25T19:22:13Z

Excuse me and sorry for my ignorace. I'm getting lost in the discussion above. Can we build up a list of necessarry checks that need to pass in order for trusted condition to be set?

If I understand this correctly, from what @mtrmac says, following need to hold true for at least one signature S of image I being verified:

S must be signed by a private key corresponding to the given or known public key
S must be parse-able and valid signature
the pull-spec in S in {"identity":{"docker-reference": "pull-spec" } must be pull-able and refer to particular digest D (it needs to be fetched at the beginning of the verification)
the digest D must match digest in signature's {"image":{"docker-manifest-digest": "digest"}
the digest D must match the name of the image object I

Is that all? Have I forgotten something?

On the other hand, here's a list of items that do not matter for the image to be considered trusted (again, please correct me if I'm wrong with an explanation, why?):

invalid, not matching, or in any other way bad signature attached to I as long as there's S that is trusted
image.DockerImageReference

this is just one of possibly many pull-specs the image is available at. It's obtained on the first import of the image. Thus it can be totally unrelated to the signature's identity.

an exact reference given on command line identifying the image to be verified (e.g. myRepository:5000/rhelmirror/rhel7:currentbaseimage)

I assume this reference is given solely for the purpose of finding the image in etcd. If true, then it doesn't matter if we identify the image by istag, isimage or just sha.

verify-signature-of --expecting-identity registry.access.redhat.com/rhel7/rhel:8.20.5 myRepository:5000/rhelmirror/rhel7:currentbaseimage

I don't quite get the meaning of --expecting-identity registry.access.redhat.com/rhel7/rhel:8.20.5. Does it add another point to the mandatory conditions above saying that S must have identity matching to this? And if there's no such signature, clear all the verified statuses from image's signatures?

mtrmac · 2017-01-26T16:01:25Z

If I understand this correctly, from what @mtrmac says, following need to hold true for at least one signature S of image I being verified:

S must be signed by a private key corresponding to the given or known public key
S must be parse-able and valid signature
the pull-spec in S in {"identity":{"docker-reference": "pull-spec" } must be pull-able and refer to particular digest D (it needs to be fetched at the beginning of the verification)
the digest D must match digest in signature's {"image":{"docker-manifest-digest": "digest"}
the digest D must match the name of the image object I
Is that all? Have I forgotten something?

Using the above terminology:
0. The verifier must know what image they want to get, E, an “expected docker reference”. RHEL:5.0.0vulnerable is a perfectly fine RHEL:5.0.0vulnerable image, but an unacceptable RHEL:8.20.5 image. This is not a property of the image blob. *E may be equal the image location, if it is “canonical” (e.g. for docker.io/library/busybox:25, E would be docker.io/library/busybox:25), and that’s a good default. For non-canonical locations (e.g. mirrors), E must be explicitly configured somewhere.

Based on E, decide on which public keys are acceptable. (Not every public key is acceptable for signing every E! We don’t want the LOB application ISV to be able to sign RHEL images.)
Given E and the set of public keys, there must be at least one signature which matches the following:
a. The signature is correctly signed by one of the acceptable public keys.
b. The contents of the signature is a JSON, with a good reason to believe that the consumer of the signature can process it the way the producer intended: it has the expected format identifier, all expected fields present, no unknown fields anywhere in critical, no duplicate fields, no other strange or surprising contents.
c. The critical.identity.docker-reference field must match E (exactly by default; more relaxed matching may be configurable, for specific subsets of E (e.g. registry host names or individual repositories))
d. If all of the above matches, critical.image.docker-manifest-digest identifies the signed image. That can either be used to pull-by-digest (pull-repo@critical.image.docker-manifest-digest), or to match the result of another tag lookup (pull-repo:sometag, read the signatures for that tag, verify them as above, then verify that the digest of the resulting manifest matches critical.image.docker-manifest-digest)

mfojtik · 2017-01-27T11:32:09Z

@mtrmac @miminar added --expected-identity parameter as we discussed. Now the verification command is:

$ oc adm verify-image-signature <imageID> \
    --expected-identity="172.30.115.179:5000/test/test:latest" --confirm

mfojtik · 2017-01-30T10:14:29Z

@mtrmac @miminar @smarterclayton if there are no further objections about this command, I'm going to add some tests.

miminar · 2017-02-07T11:27:26Z

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

+	}
+
+	cmd.Flags().BoolVar(&opts.Confirm, "confirm", opts.Confirm, "If true, the result of the verification will be recorded to an image object.")
+	cmd.Flags().BoolVar(&opts.Remove, "remove", opts.Remove, "If set, the current signature verification will be removed from the image.")


current is a bit misleading. All signature verification statuses shall be removed, right?

miminar · 2017-02-07T11:37:10Z

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

+
+	verifyImageSignatureExample = templates.Examples(`
+	# Verify the image signature using the public key and record the status as a condition to image
+	%[1]s sha256:c841e9b64e4579bd56c794bdd7c36e1c257110fd2404bebbb8b613e4935228c4 --public-key=production.gpg --confirm


Can this include an example of removal?

miminar · 2017-02-07T11:38:45Z

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

+	opts := &VerifyImageSignatureOptions{ErrOut: errOut, Out: out}
+	cmd := &cobra.Command{
+		Use:     fmt.Sprintf("%s IMAGE [--confirm]", name),
+		Short:   "Verifies the given IMAGE signature with local public key",


I believe this should be Verify that the given IMAGE is signed with a good signature. And define the good in the long description.

@miminar s/good/trusted/ and I will explain what "trusted" means. ok?

miminar · 2017-02-07T11:41:36Z

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

+
+	// If --public-key is provided only this key will be used for verification and the
+	// .gnupg/pubring.gpg will be ignored.
+	if len(o.PublicKeyFilename) > 0 {


miminar · 2017-02-07T11:42:52Z

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

+	verifyImageSignatureLongDesc = templates.LongDesc(`
+	Verifies the imported image signature using the local public key.
+
+	This command verifies if the signature attached to an image is trusted by


I'd find it helpful, if this help described what trusted means.

miminar · 2017-02-07T11:50:09Z

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

+		return err
+	}
+	// Verify the tag exists
+	tag, err := o.Client.ImageStreamTags(ref.Namespace).Get(ref.Name, ref.Tag)


I believe the :tag part is optional in critical.identity.docker-reference. OTOH it can contain digest instead. @mtrmac, can you please confirm?

References either with a tag or a digest can happen. NameOnly references should not.

(In principle, an attacker can put anything invalid in there, including empty strings, completely invalid reference formats, NameOnly references, or references which contain both a tag and a digest.)

miminar · 2017-02-07T11:54:05Z

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

+			fmt.Fprintf(o.ErrOut, "%s identity cannot be verified: %v\n", o.InputImage, identityError)
+		}
+
+		// If we have error and --confirm then remove the signature from the image which


Remove the comment or rewrite to match what's happening. (I believe the code says it all).

miminar · 2017-02-07T11:58:34Z

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

@@ -73,6 +75,9 @@ func (o *VerifyImageSignatureOptions) Complete(f *clientcmd.Factory, cmd *cobra.
 		return kcmdutil.UsageError(cmd, "exactly one image must be specified")
 	}
 	o.InputImage = args[0]
+	if len(o.ExpectedIdentity) == 0 {
+		return kcmdutil.UsageError(cmd, "the --expected-identity must be specified")


Since it's a required argument, can it be turned into regular positional argument?

Either way, cobra.Command.Use needs to be updated.

miminar · 2017-02-07T11:59:08Z

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

@@ -128,7 +133,7 @@ func (o *VerifyImageSignatureOptions) verifySignature(signature []byte) (string,

 // verifySignatureContent verifies that the signature content matches the given image.
 // TODO: This should be done by calling the 'containers/image' library in future.
-func (o *VerifyImageSignatureOptions) verifySignatureContent(content []byte) (string, string, error) {
+func (o *VerifyImageSignatureOptions) verifySignatureContent(content []byte) (string, error) {


Please, describe the returned value.

miminar · 2017-02-07T12:13:20Z

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

 		if signatureContentErr != nil {
 			fmt.Fprintf(o.ErrOut, "%s signature content cannot be verified: %v\n", o.InputImage, signatureContentErr)
 		}

-		identityError := o.verifyImageIdentity(identity, digest)
+		identityError := o.verifyImageIdentity(identity)


It's perfectly fine to have multiple signatures included within an image with different identity references. The way this code is structured, prevents me from having two signatures with good verification status set at the same time, if they have different identity references.

mfojtik · 2017-02-09T11:39:15Z

@miminar fixed.

miminar · 2017-02-14T08:53:32Z

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

+	opts := &VerifyImageSignatureOptions{ErrOut: errOut, Out: out}
+	cmd := &cobra.Command{
+		Use:     fmt.Sprintf("%s IMAGE [--confirm]", name),
+		Short:   "Verify that the given IMAGE signature is trusted",


the given IMAGE is trusted

(signature is not given)

the command is oc adm verify-image-signature :-)

That doesn't change the fact the image signature is nowhere given nor specified. It just may be associated with the given image.

Hmm, the name of the command is misleading as well.

OTOH the “has trust been verified” flag is a property of a signature, not of the image.

I'm open to any name suggestions for the command :-)

The intent of this command is to verify the signature that is attached to an image is "trusted" (by GPG key) and "valid" (by signature content).

Signature is not specified BUT it has to be present as part of the image you want to verify.

It is somewhat specified by the expected identity (though that may still match multiple).

Another thing perhaps worth pointing out is that AFAICS the “verify” operation will also automatically unset the verification status for any non-matching signature; i.e. it is impossible to mark two signatures with two different identities as verified at the same time. I don’t really have any suggestion about this, though, and this is perfectly fine with me for the first version.

It kinda seems to me that this command is really oc adm manage-image-signature-verification-status with --remove as a possible operation, but that’s impossibly cumbersome to type… I suck at naming.

$ oc adm image-signature verify --identity=XYZ [IMAGE] $ oc adm image-signature remove [IMAGE]

is this better?

That seems to suggest that [IMAGE] should be $IMAGE $SIGNATURE. Really, I suck at UI and I’ll shut up.

That's good. I'd like even more oc adm image-signature verify [--confirm] IDENTITY IMAGE but you don't have to spoil me be fulfilling all my tiny wishes 😄

miminar · 2017-02-14T09:03:31Z

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

+		Critical critical `json:"critical"`
+	}
+	m := message{}
+	if err := json.Unmarshal(content, &m); err != nil {


According to @mtrmac, you should first unmarshal into an interface{} and ensure it doesn't contain any unrecognized fields.

Any unknown JSON field in critical, and any subobjects, must cause the signature to be rejected. That's what critical means in this format.

miminar · 2017-02-14T09:11:20Z

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

+func NewCmdVerifyImageSignature(name, fullName string, f *clientcmd.Factory, out, errOut io.Writer) *cobra.Command {
+	opts := &VerifyImageSignatureOptions{ErrOut: errOut, Out: out}
+	cmd := &cobra.Command{
+		Use:     fmt.Sprintf("%s IMAGE [--confirm]", name),


Since --expected-identity is mandatory, it should be listed here. I'd still prefer the expected identity as a positional argument though.

what you mean with positional argument?

oc verify-image-signature EXPECTED_IDENTITY IMAGE [--confirm]

openshift-bot · 2017-02-26T21:37:11Z

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

mfojtik · 2017-03-30T13:42:25Z

Closing in favor of #13585

mfojtik force-pushed the verify-image-signature branch 2 times, most recently from 4599f49 to e3bf3a3 Compare January 23, 2017 17:48

miminar suggested changes Jan 24, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2017

miminar reviewed Jan 25, 2017

View reviewed changes

mfojtik force-pushed the verify-image-signature branch from 6bf7f8e to 562ebf6 Compare January 25, 2017 12:15

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2017

mtrmac suggested changes Jan 25, 2017

View reviewed changes

mfojtik force-pushed the verify-image-signature branch from 40d2a61 to 1dc186d Compare January 27, 2017 11:31

mfojtik force-pushed the verify-image-signature branch 2 times, most recently from 73b49ef to 31418cf Compare January 27, 2017 12:31

mfojtik changed the title ~~[DO_NOT_MERGE] WIP: Verify image signatures~~ Verify image signatures Jan 30, 2017

miminar suggested changes Feb 7, 2017

View reviewed changes

mfojtik force-pushed the verify-image-signature branch from 31418cf to a1cd312 Compare February 9, 2017 11:17

mfojtik added 5 commits February 9, 2017 12:26

image: allow to update image signature fields externally

f397c24

image: describe image signatures

0cfe7cd

bump(golang.org/x/crypto): 1f22c0103821b9390939b6776727195525381532

ecf72c1

image: add adm verify-image-signature command

4ea38cf

updated generated completetion and docs

bd5bc5d

mfojtik force-pushed the verify-image-signature branch from a1cd312 to bd5bc5d Compare February 9, 2017 11:38

miminar suggested changes Feb 14, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2017

mfojtik mentioned this pull request Mar 30, 2017

Verify image signature #13585

Merged

mfojtik closed this Apr 4, 2017

mfojtik deleted the verify-image-signature branch September 5, 2018 21:23

Verify image signatures #12616

Verify image signatures #12616

Conversation

mfojtik commented Jan 23, 2017

mfojtik commented Jan 23, 2017

smarterclayton commented Jan 23, 2017 via email

mfojtik commented Jan 24, 2017

miminar Jan 24, 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

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

mtrmac commented Jan 24, 2017

mfojtik commented Jan 24, 2017

mtrmac commented Jan 24, 2017

mfojtik commented Jan 24, 2017 • edited Loading

mtrmac commented Jan 24, 2017

mtrmac commented Jan 24, 2017

mtrmac commented Jan 24, 2017

miminar commented Jan 25, 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 Jan 25, 2017 • edited Loading

mtrmac commented Jan 25, 2017

mfojtik commented Jan 25, 2017

mtrmac commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Jan 25, 2017

mfojtik commented Jan 25, 2017

mfojtik commented Jan 25, 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

mtrmac commented Jan 25, 2017 • edited Loading

mfojtik commented Jan 25, 2017

mfojtik commented Jan 25, 2017 • edited Loading

mtrmac commented Jan 25, 2017

mtrmac commented Jan 25, 2017

miminar commented Jan 25, 2017

mtrmac commented Jan 26, 2017

mfojtik commented Jan 27, 2017 • edited Loading

mfojtik commented Jan 30, 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

mfojtik commented Feb 9, 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

mfojtik Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miminar Jan 24, 2017 •

edited

Loading

mfojtik commented Jan 24, 2017 •

edited

Loading

mfojtik commented Jan 25, 2017 •

edited

Loading

mtrmac commented Jan 25, 2017 •

edited

Loading

mfojtik commented Jan 25, 2017 •

edited

Loading

mfojtik commented Jan 27, 2017 •

edited

Loading

mfojtik Feb 15, 2017 •

edited

Loading