Image Signing #8371

miminar · 2016-04-06T11:50:15Z

Allow to add signatures to image objects. They are treated as opaque blobs.

Added a new cluster resource images/signatures.

Added system:image-signer role that athorizes to update image signatures.

TODOs:

define signature types (e.g. gpg/base64 or just base64).
decide whether user can update signatures being tagged in the project, he administrates
write some more tests

miminar · 2016-04-06T11:51:57Z

cc @mtrmac, @aweiteka

miminar · 2016-04-06T11:52:17Z

[test]

aweiteka · 2016-04-06T12:31:30Z

pkg/cmd/server/bootstrappolicy/constants.go

 	ImagePusherRoleName       = "system:image-pusher"
 	ImageBuilderRoleName      = "system:image-builder"
 	ImagePrunerRoleName       = "system:image-pruner"
+	ImageSignerRoleName       = "system:image-signer"


We want project admins to be able to manage image signatures. Since this is so related to registry should we also grant privs to registry-admin role?

Yes, that makes sense.

We want project admins to be able to manage image signatures. Since this is so related to registry should we also grant privs to registry-admin role?

You can't do that with a cluster-scoped resource, you'll need to project (proJECT) access at a project (PROject) scope. (english is weird).

You need to expose this via a synthetic project-scoped resource that can create the cluster-scoped resources in a controlled way.

We want project admins to be able to manage image signatures.

How does that work when a single image is shared between several projects?

How does that work when a single image is shared between several projects?

I think that update is the wrong verb. It seems like signatures are an additive affair. If liggitt signs it, deads can still sign too.

I think the get case is a little interesting. I don't know that we want to automatically allow someone in ns-1 to see signatures created via the resource in ns-2. The ability to do that would be good, but I haven't thought about whether it should always be that way. You could end up in cases where you're seeing all sorts of signatures you know nothing about.

aweiteka · 2016-04-06T12:43:18Z

Maybe I'm missing something but I'm only seeing a 'put' API call and permissions. Where's the 'get' story to validate a signature locally?

miminar · 2016-04-06T13:13:43Z

@aweiteka you can get the image object already using API calls like Images().Get() or ImageStreamImages(isName).Get(). I presume it's ok to give signatures anyone, who has access to the image object.

aweiteka · 2016-04-06T13:20:02Z

you can get the image object already using API calls like Images().Get() or ImageStreamImages(isName).Get().

Ok. Makes sense.

I presume it's ok to give signatures anyone, who has access to the image object.

Yes, I think so.

mtrmac · 2016-04-06T13:56:09Z

Thanks, makes sense.

signatureStrategy.PrepareForUpdate seems fragile but I have no idea how the etcd storage interaction works so I have nothing helpful to contribute.

Re: type, I’d suggest atomic rather than saying gpg; if at all possible we want to steer users to using the provided tools (and especially the future provided policy evaluation engine) instead of suggesting they should be parsing the contents manually; in fact this ability to provide metadata without users having to parse the contents is one of the primary reasons for having an ImageSignature object instead of a raw blob.

We will probably eventually need to add more fields to ImageSignature to allow nice and quick UI; we can guess now (signed image name? some kind of signing fingerprint?) or defer that until a proper UX design gives us the real requirements.

miminar · 2016-04-06T16:01:25Z

Addressed some review items.

signatureStrategy.PrepareForUpdate seems fragile

I share your opinion, but that's the best I can do right now.

If type is supposed to be something like gpg or base64, this is not what we want: we have an array of signatures so that we can attach multiple signature to a single image, but we only have one signature type (and we wouldn’t want to add new types just to support multisignatures).

Do we need the type at all then? What if we later decide to change the signature algorithm/encoding? During a migration, shall we take an old image and recreate the signatures with the desired type or throw them away?

We will probably eventually need to add more fields to ImageSignature

Shouldn't be a problem to add later. Fingerprint sounds reasonable, can we be more specific right now?

mtrmac · 2016-04-06T16:23:02Z

If type is supposed to be something like gpg or base64, this is not what we want: we have an array of signatures so that we can attach multiple signature to a single image, but we only have one signature type (and we wouldn’t want to add new types just to support multisignatures).

Sorry, that comment was completely nonsensical (and I deleted it instead of saying so): I have overlooked that ImageHasSignature is comparing Blob, I thought it is only matching Type. My bad.

Do we need the type at all then? What if we later decide to change the signature algorithm/encoding?

Ideally the signature library (currently in skopeo) should be able to deal with format changes/alternatives/detection transparently; the current code does not need the type field. But it is very reasonable for OpenShift to protect itself against future format changes (or adding other non-skopeo signature mechanisms) using the separate metadata field.

During a migration, shall we take an old image and recreate the signatures with the desired type or throw them away?

In general the registry may not (should not?) have access to the private keys necessary to resign everything; during a migration, either old signatures, and support for them, and the data needed to verify them, can be preserved unmodified, or they will become invalid and can at best be thrown away.

miminar · 2016-04-07T09:15:56Z

Flake #8397.

• Failure [337.093 seconds]
Kubectl client
/data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/test/e2e/kubectl.go:1146
  Update Demo
  /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/test/e2e/kubectl.go:161
    should scale a replication controller [Conformance] [It]
    /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/test/e2e/kubectl.go:150

    Apr  7 04:48:41.087: Timed out after 300 seconds waiting for name=update-demo pods to reach valid state

    /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/test/e2e/util.go:1469

smarterclayton · 2016-04-08T03:02:20Z

Is there a standard convention for describing the type of a signature? An RFC? A mime-type? A convention in use in wide standard in linux? If none of those exist, I would expect us to probably choose a mime type or similar with the appropriate qualifications, especially if the signature is itself in a standard format.

mtrmac · 2016-04-08T17:22:37Z

Is there a standard convention for describing the type of a signature?

Kind of.

At the moment the cryptographic signature is GPG-generated, i.e. RFC 4880 should apply (though GPG might be deviating, I don’t know).

OTOH:

RFC 4880, and MIME, deals with binary payloads; we need to base64-or-something- encode it into a JSON string so saying “RFC 4880” would not be exactly a precise match.
The “payload” carried within the GPG signature is our own JSON format which is not standardized anywhere; just knowing it is GPG is not sufficient to verify a container.
For interoperability with IdM systems we may want to support X.509 as well, and 99% of all code should treat both formats equally, i.e. as blobs to be only signed/verified with the provided tools. Sure, we can add another type saying “This is CMS”, but that should not affect how anything touching the signature code will work with it.

Overall I think saying “this is GPG” is not really the semantics we want. I expect there are two kinds of clients:

Simple management (display, copy, delete) should not touch the blob at all, and use the additional metadata (enabled but not yet provided by this patch, and hypothetically added when the signature is added to the image) to show what the signature is of and about. E.g. the web UI would show that there are three signatures with $these identities, just by reading the additional metadata; copying images between registries would copy the signatures and all metadata without validating or interpreting the contents in any way.
Tools which really work with the signatures (creating a new signature, validating the signature) most of all need to know that this is an “atomic” signature (i.e. something that they can work with at all, and not a completely different format coming from the future); how to deal with the blob would be up to them.
- (If these tools relied on extra metadata from the image object, e.g. to distinguish between GPG and X.509, we would have to think about providing and preserving this metadata with every other image storage/transport mechanism, and that complicates everything. Sadly given the existing Docker format we do need signatures as separate attachments, so the transfer/storage mechanisms do need to care that the signatures exist; but it seems much nicer if all the transfer/storage mechanisms just know that they need to preserve an unstructured blob instead of blob+metadata. And we can of course embed metadata into the blob, without making every single storage/transport mechanism aware and more complex.)

soltysh · 2016-04-11T10:25:08Z

/cc

miminar · 2016-04-11T13:23:02Z

Shall we perhaps split the metadata into a descriptor identifying the signare and have additional Metadata atribute for the rest that can be obtained from the blob?

`Signature{
    // inspired by manifest schema of unnamed company;
    // descriptor uniquely identifies a blob
    Descriptor: {
        Type: "atomic-gpg-v1",
        // a change in blob string is reflected here and vice-versa
        Fingerprint: "xxxxxxxxxxxxxx",
    },
    // parsed from the Blob
    Metadata: {
        Issuer: "Issuer's Name",
        CreationTimestamp: (unversioned.Time),
    },
    // opaque binary string
    Blob: "xxxxxxxxxxxxxxxxxxxx",
},

mtrmac · 2016-04-11T13:30:10Z

Could you elaborate? I can’t see what is the difference between “identifying the signature” and “additional metadata”.

(FWIW I’d prefer not exposing the signature metadata in plaintext objects by default just because the metadata exists; until the signature is verified per the current policy (which may be different than the policy at the time of adding the signature), none of the metadata is reliable.)

mtrmac · 2016-04-11T13:32:33Z

FWIW the blob itself uniquely identifies the blob :-) OTOH neither the issuer name, issuer key fingerprint, nor the name claimed by the signature are obviously unique.

mtrmac · 2016-04-11T13:52:16Z

OTOH neither the issuer name, issuer key fingerprint, nor the name claimed by the signature are obviously unique.

Probably the (signer identification, signed image identification) pair could be a good identification (it is not trivially unique but if there are collisions, the valid and latest-expiring signature should work as a substitute for any others).

Then, the tradeoff would be whether to do this as a pair of strings with internal, vaguely defined structure like

"issuer": "gpg-fingerprint:ABCD",
"image-identity": "docker:foo.redhat.com/rhel7/rhel:9.10.11"

or whether to have the full structure codified in JSON:

"issuer": {
  "type": "gpg",
  "gpg-fingerprint": "ABCD"
}, "image-identity": {
  "type": "docker-reference",
  "docker-reference": "foo.redhat.com/rhel7/rhel:9.10.11"
}

or something similar.

Also, I guess we will eventually need a “signature applies to this image”/“signature applies to one of the base images” flag so that the UI doesn’t confuse users by showing various derived images all signed as rhel7/rhel.

miminar · 2016-04-11T14:12:33Z

Also, I guess we will eventually need a “signature applies to this image”/“signature applies to one of the base images” flag so that the UI doesn’t confuse users by showing various derived images all signed as rhel7/rhel.

I don't feel pleasant about an idea of having 200 images, each having the same signature blobs created for the rhel image they're based on. I'd rather require an import of all the base images into etcd with their signatures and refer to them.

FWIW the blob itself uniquely identifies the blob

Right, but comparing them might is expensive.

or whether to have the full structure codified in JSON:

I we are pretty sure these attributes won't change, I'd reflect it in signature's structure which is serializable into json.

mtrmac · 2016-04-11T14:22:23Z

Also, I guess we will eventually need a “signature applies to this image”/“signature applies to one of the base images” flag so that the UI doesn’t confuse users by showing various derived images all signed as rhel7/rhel.

I don't feel pleasant about an idea of having 200 images, each having the same signature blobs created for the rhel image they're based on. I'd rather require an import of all the base images into etcd with their signatures and refer to them.

It is kind of ugly, yes, but also cheap enough, and it allows for a better user workflow because it removes interdependencies. With this, an ISV can publish their application, built on top of a RHEL base image:

Without the ISV also publishing the stand-alone RHEL image
Without requiring every client of that ISV to also have access to that RHEL image (and that particular version of that RHEL image, if we ever did development-only subscriptions)
Without requiring network access to r.a.rh.com
Without building extra infrastructure to automatically pull the RHEL image when the ISV’s application is pulled.
Without all of this breaking when r.a.rh.com goes down or has an outage and deletes the base image.

mtrmac · 2016-04-11T14:25:34Z

Right, but comparing them might is expensive.

The signatures are a few hundred of bytes long at the moment; even if it were a few kilobytes, that would be cheap enough — and we can always compute a cryptograhic hash to use as an ID. The UI issues of identifying blobs by meaningless binary data seem to be a bigger concern.

aweiteka · 2016-04-13T11:59:16Z

The UI issues of identifying blobs by meaningless binary data seem to be a bigger concern.

It sounds like we would benefit from a design discussion on how we expect users to interact with these signatures.

mtrmac · 2016-04-25T14:29:21Z

Can we proceed with discussing/designing how signatures are described and presented to the user?

Per #8371 (comment) , I think the major options are:

Metadata is for display to humans only, content of the SignedBy and SignedIdentity strings are subject to change and not an API:

type ImageSignature struct{
Type ImageSignatureType // "atomic" the only supported value
Blob string // base64(?)-encoded
SignedBy string // No parsing supported, but an example could be gpg:$fingerprint
SignedIdentity string // No parsing supported, but an example could be docker:registry.access.redhat.com/rhel7/rhel:7.2.3
}

Metadata is structured for machine processing and reliable parsing (possibly allows better UI, e.g. showing username links instead of raw GPG fingerprints or the UI indicating whether the signature applies to this image or a base one).

type ImageSignature struct {
Type ImageSignatureType // "atomic" the only supported value
Blob string // base64(?)-encoded
SignedBy struct {
    Type string // "gpg" the only supported value
    GPGFingerprint string // Mandatory iff type == "gpg"
}
SignedIdentity struct {
    Type string "docker-reference" the only currently supported value
    DockerReference string // Mandatory iff type = "docker-reference", e.g. "registry.access.redhat.com/rhel7/rhel:7.2.3"
}
}

miminar · 2016-04-26T11:42:29Z

I'd prefer the 1st option. It's in analogy to image digests (sha256:xxxxxxx). The values can be printed as they are. I presume they are optional. The only mandatory attributes are Type and Blob. I'd also add an attribute CreationTimestamp.

miminar · 2016-04-27T12:37:40Z

I've updated the fields. @mltrmac, could you please confirm the final shape?

@openshift/api-review, this needs your attention.

smarterclayton · 2016-06-06T17:36:56Z

pkg/image/api/v1/types.go

+// SignatureSubject holds information about a person or entity who created the signature.
+type SignatureSubject struct {
+	SignatureGenericEntity `json:",inline"`
+	// If present, it is a fingerprint of public key belonging to the subject used to verify image signature.


Add an example of a fingerprint, and any other detail that is required to understand why it is used.

Why is this not PublicKeyFingerprint if it's a fingerprint?

Add an example of a fingerprint, and any other detail that is required to understand why it is used.

Added an example.

Why is this not PublicKeyFingerprint if it's a fingerprint?

I corrected the godoc. Since this serves solely an informative purpose, I find whole fingerprint too long to display. The long key ID (64bits) prefixed with 0x should be appropriate for this purpose. (e.g. 0x685ebe62bf278440).

miminar · 2016-06-07T07:41:50Z

@smarterclayton, @deads2k: thank you for the review. Items have been addressed.

soltysh · 2016-06-07T13:29:25Z

pkg/image/api/validation/validation.go

+
+	var trustedCondition, forImageCondition *api.SignatureCondition
+	for i := range signature.Conditions {
+		cond := &signature.Conditions[i]


Why doing this that way and not using for-each loop?

Because in that way I'd be taking reference to a temporary cond value which is reassigned in each iteration. So in the end both trustedCondition and forImageCondition would point to the same value equal to the last item of signature.Conditions.

Right, my bad. Sorry.

for-each loops unfortunately suck hard. I've been bitten a bunch of times

miminar · 2016-06-08T11:27:54Z

Comments addressed. @smarterclayton, @soltysh, thanks for the review! Anything else?

Which is an array of signatures with mostly opaque data. There are two mandatory fields of each signature: - Type - the only supported value for now is "atomic" - Content - opaque binary string There's one known signature type "atomic" with two mandatory and one optional attributes: - ImageHash - hash of a signed image - ImageIdentity - signed claim representing an origin of the image - Created - optional; the time of signarure's creation Following attributes are added for future signing extensions: - SignedClaims - a map of interesting signed claims that maybe added later to the signature format but not that important to become first-class fields - IssuedBy - issuer of signing certificate if any - IssuedTo - subject of signing certificate or an image signer Signed-off-by: Michal Minar <[email protected]>

openshift-bot · 2016-06-08T12:35:30Z

Evaluated for origin test up to 57d134c

openshift-bot · 2016-06-08T14:00:53Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4593/)

soltysh · 2016-06-08T15:05:35Z

LGTM

miminar · 2016-06-09T11:23:43Z

@smarterclayton merge?

smarterclayton · 2016-06-09T20:20:52Z

pkg/image/api/types.go

+	// If present, it is a human readable key id of public key belonging to the subject used to verify image
+	// signature. It should contain at least 64 lowest bits of public key's fingerprint (e.g.
+	// 0x685ebe62bf278440).
+	PublicKeyID string


Can you explain why this would be ID vs Fingerprint? Is ID in common use to describe this field?

Just trying to establish what someone familiar with signatures would expect in the usage here - fingerprint vs id.

Key ID is a shortened key fingerprint. Usually its lowest 32 or 64 bits. My reasoning for including ID instead of fingerprint is that this field is meant to be for diplaying. Not for verification. Clients that want to verify the signature should deal with Content field. What you usually see while working with gpg keys is just key ids because they are relatively short.

$ gpg2 --list-keys /home/miminar/.gnupg/pubring.gpg -------------------------------- pub rsa2048/71445503 2014-01-13 [SC] [expired: 2016-01-13] uid [ expired] Michal Minar <[email protected]> ... $ gpg --fingerprint 0x71445503 pub rsa2048/71445503 2014-01-13 [SC] [expired: 2016-01-13] Key fingerprint = BF4A E972 503B 49BC A6D2 6836 F7B3 5329 7144 5503 uid [ expired] Michal Minar <[email protected]>

The full fingerprint may be very long depending on algorithm used. It this were PublicKeyFingerprint, I'd rather store it in binary format []byte and let the rendering client choose whatever formating it prefers.

Ok, that's the clarity I wanted.

smarterclayton · 2016-06-10T14:23:17Z

API approved, [merge]

openshift-bot · 2016-06-10T14:25:22Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4707/) (Image: devenv-rhel7_4348)

openshift-bot · 2016-06-10T14:25:23Z

Evaluated for origin merge up to 57d134c

0xmichalis · 2016-06-11T12:02:42Z

pkg/image/api/types.go

+
+// SignatureCondition describes an image signature condition of particular kind at particular probe time.
+type SignatureCondition struct {
+	// Type of job condition, Complete or Failed.


cp error

Thanks! Addressed in #9181

miminar force-pushed the image-with-signatures branch from 9c24be0 to e0901a0 Compare April 6, 2016 12:07

aweiteka reviewed Apr 6, 2016
View reviewed changes

miminar force-pushed the image-with-signatures branch from d30fe3c to 908027f Compare April 7, 2016 07:31

smarterclayton added the needs-api-review label Apr 26, 2016

miminar force-pushed the image-with-signatures branch from 908027f to f1a127e Compare April 27, 2016 12:33

miminar changed the title ~~[WIP] Image Signing~~ Image Signing Apr 27, 2016

miminar force-pushed the image-with-signatures branch from f1a127e to 74c0de3 Compare April 27, 2016 12:39

smarterclayton reviewed Jun 6, 2016
View reviewed changes

mtrmac mentioned this pull request Jun 7, 2016

OpenShift native signatures containers/skopeo#93

Merged

miminar force-pushed the image-with-signatures branch 3 times, most recently from 1d45892 to eaabf65 Compare June 7, 2016 07:31

soltysh reviewed Jun 7, 2016
View reviewed changes

miminar force-pushed the image-with-signatures branch from eaabf65 to 055e8f6 Compare June 8, 2016 11:27

miminar force-pushed the image-with-signatures branch from 055e8f6 to bb3d370 Compare June 8, 2016 11:29

miminar force-pushed the image-with-signatures branch from bb3d370 to 57d134c Compare June 8, 2016 11:46

smarterclayton reviewed Jun 9, 2016
View reviewed changes

smarterclayton added api-approved and removed needs-api-review labels Jun 10, 2016

openshift-bot merged commit f7be652 into openshift:master Jun 10, 2016

stevekuznetsov mentioned this pull request Jun 10, 2016

Incorrect MountFlags value in Docker daemon unit file: Cannot start container ... Path ... is mounted on ... but it is not a shared or slave mount #9279

Closed

0xmichalis reviewed Jun 11, 2016
View reviewed changes

miminar deleted the image-with-signatures branch June 20, 2016 06:12

This was referenced Jun 27, 2016

DO NOT MERGE: Storing signatures in OpenShift annotations containers/image#11

Closed

OpenShift native signatures containers/image#14

Merged

Image Signing #8371

Image Signing #8371

Uh oh!

Conversation

miminar commented Apr 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miminar commented Apr 6, 2016

Uh oh!

miminar commented Apr 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k May 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweiteka commented Apr 6, 2016

Uh oh!

miminar commented Apr 6, 2016

Uh oh!

aweiteka commented Apr 6, 2016

Uh oh!

mtrmac commented Apr 6, 2016

Uh oh!

miminar commented Apr 6, 2016

Uh oh!

mtrmac commented Apr 6, 2016

Uh oh!

miminar commented Apr 7, 2016

Uh oh!

smarterclayton commented Apr 8, 2016

Uh oh!

mtrmac commented Apr 8, 2016

Uh oh!

soltysh commented Apr 11, 2016

Uh oh!

miminar commented Apr 11, 2016

Uh oh!

mtrmac commented Apr 11, 2016

Uh oh!

mtrmac commented Apr 11, 2016

Uh oh!

mtrmac commented Apr 11, 2016

Uh oh!

miminar commented Apr 11, 2016

Uh oh!

mtrmac commented Apr 11, 2016

Uh oh!

mtrmac commented Apr 11, 2016

Uh oh!

aweiteka commented Apr 13, 2016

Uh oh!

mtrmac commented Apr 25, 2016

Uh oh!

miminar commented Apr 26, 2016

Uh oh!

miminar commented Apr 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miminar commented Jun 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

miminar commented Apr 6, 2016 •

edited

Loading

deads2k May 9, 2016 •

edited

Loading

openshift-bot commented Jun 10, 2016 •

edited

Loading