Add documentation for the atomic signature format #251

mtrmac · 2017-03-11T22:03:01Z

This documents the signature format, in more detail than #59.

runcom · 2017-03-28T20:19:15Z

ping @aweiteka @vbatts @rhatdan @rektide @imcleod

aweiteka · 2017-04-03T13:55:31Z

docs/atomic-signature.md

+An atomic container signature consists of a cryptographic signature which identifies
+and authenticates who signed the image, and carries as a signed payload a JSON document.
+The JSON document identifies the image being signed, and claims
+a specific identity of the image, and possibly other information about the image.


"possibly..."? This implies it's not there now but "on the roadmap" so maybe leave this out until that information is actually added?

This is trying to say that the format can be extended, both in the future and in independent implementations, i.e. it is not doomed to be an (image, identity) pair forever, and to somewhat motivate the backward/forward compatibility concerns.

It seems slightly incorrect to just say “identifies the image… and claims a specific identity”, though the “possibly” in there does seem a rather vague.

Perhaps:
"The JSON document identifies the image being signed, claims a specific identity of the image and if applicable, contains other information about the image."

aweiteka · 2017-04-03T13:57:43Z

docs/atomic-signature.md

+
+The signatures do not modify the container image (the layers, configuration, manifest, …);
+e.g. their presence does not change the manifest digest used to identify the image in
+docker/distribution servers; rather, the signatures are an “attachment” to an immutable image.


I prefer "associated", e.g. "...the signatures are associated with an immutable image by its manifest digest."

That is indeed better, updated all three instances.

aweiteka · 2017-04-03T14:00:18Z

docs/atomic-signature.md

+e.g. their presence does not change the manifest digest used to identify the image in
+docker/distribution servers; rather, the signatures are an “attachment” to an immutable image.
+An image can have any number of signatures, and image distribution systems SHOULD support
+attaching more than one signature to an image.


Maybe a bit clearer: "An image can have any number of signatures so signature distribution systems should support
associating more than one signature to an image."

Updated. (Using “associating…with” per a random dictionary and some googling; if there is a semantic difference, please tell me.)

aweiteka · 2017-04-03T14:56:56Z

docs/atomic-signature.md

+newly added cryptographic signature formats, if necessary.)
+
+Consumers of atomic container signatures SHOULD verify the cryptographic signature
+against one or more trusted public keys


Can one signature blob be verified with more than one public keys? I thought it was 1-to-1.

The user can set up a keyring with several keys, all equally acceptable for a particular namespace (e.g. a Fedora 30 primary key, Fedora 30 secondary key, Fedora security team universal emergency key).

aweiteka · 2017-04-03T15:16:53Z

docs/atomic-signature.md

+if so, they SHOULD design the output of such processing to minimize the risk of users considering the output trusted
+or in any way usable for making policy decisions about the image.)
+
+### OpenPGP signature verification


This is a good section. Can you speak here about the library? Does the library cover all of these verification steps? Is it viable to develop a custom verification method or is that not recommended?

Is it viable to develop a custom verification method or is that not recommended?

The current implementation is not magic, it is entirely possible to write another correct one (and this section, listing things to check, is also intended to help write it correctly). Actually we haven’t had an independent adversarial audit or anything similar to give us a very high level of confidence that the library one is indeed correct. :-)

Still, it is not quite trivial, it is easy to forget something, and various verification APIs make it fairly easy to miss something (e.g. the OpenShift reimplementation had, it in its initial versions, missed several aspects).

And using a shared implementation should hopefully increase the likelihood of it being good, many eyeballs and all that.

Finally, using the shared implementation means less work with updating as the spec expands, e.g. to support layered images—at the cost of more work with updating the callers as the library API changes.

Can you speak here about the library? Does the library cover all of these verification steps?

Added a general note in the very first section about code just using the library instead of worrying about anything in this document, and a parenthesized note in this section that the library should take care of this. (Such a note could really be in every section but this one looks a bit more scary than the others, so adding an extra note in here to reassure makes sense.)

mtrmac · 2017-04-03T19:03:00Z

@aweiteka Updated based on your review, much appreciated!

mtrmac · 2017-04-18T15:06:04Z

@runcom PTAL

mtrmac · 2017-04-27T19:26:12Z

@runcom ping

runcom · 2017-04-27T19:40:24Z

This looks good to me @mtrmac

mtrmac · 2017-04-27T21:05:34Z

👍

runcom · 2017-04-27T21:18:38Z

LGTM

TomSweeneyRedHat · 2017-04-27T21:24:41Z

docs/atomic-signature.md

+independent blobs containing the JSON document and a cryptographic signature).
+
+Currently the only defined cryptographic signature format is an OpenPGP signature (RFC 4880),
+but others may be added in the future.  (The blob does not contain metadata identifying the


nit, I'd drop the first semi and comma in the first two sentences here in the parens leaving:
"(The blob does not contain metadata identifying the cryptographic signature format. It is expected that most formats are sufficiently self-describing that this is not necessary and the configured expected public key provides another indication
of the expected cryptographic signature format."

TomSweeneyRedHat · 2017-04-27T21:44:51Z

docs/atomic-signature.md

+
+The consumer SHOULD have tests for its verification code which verify that signatures failing any of the above are rejected.
+
+(All of this is taken care of within the `github.com/containers/image/signature` package,


nit, I'd lose the parens here.

This sentence is not really a part of the format spec, it says nothing about the file contents. (Motivation for including it at all is #251 (comment) above.)

I guess it is more or less a footnote, but Markdown doesn’t natively support footnotes. Perhaps I should just build one manually using Unicode?

Or if you can't finagle a foot note, perhaps dropping the parens and changing this to:

Note: This signature verification is taken care of within the github.com/containers/image/signature package, its users do not have do do any of this manually.

and making the word 'Note' bold in that sentence? Your call, I trust your judgement.

Well, bold is somewhat of an opposite of a footnote :)

The document already starts with a big paragraph about the library, so I replaced this parenthesized paragraph with a much shorter parenthesis in the sentence introducing the bullet list.

TomSweeneyRedHat · 2017-04-27T21:56:06Z

docs/atomic-signature.md

+accept or reject signatures created at a later date, with possible extensions to this format),
+consumers MUST reject the signature if the `critical` object, or _any_ of its subobjects,
+contain _any_ member or data value which is unrecognized, unsupported, invalid, or in any other way unexpected.
+(At minimum, this includes unrecognized members in a JSON object, or incorrect types of expected members.)


Nit; Suggested: Drop the parens and make the last sentence here read:
"Unrecognized members in a JSON object or incorrect types of expected members should also be rejected as a minimum."

If you don't want to take that or similar, please change "(At minimum," to "(At a minimum,"

Dropped the parentheses and added an article to “at a minimum”.

“should also be rejected” is imprecise, because “unrecognized members”/“incorrect types” are a subset of the “unrecognized, unsupported, invalid” list in the previous sentence: this sentence is an example of the requirement, not an addition to it. Hence the “this includes” language trying to make this relationship clear; but perhaps that attempt has failed and this needs to be even clearer? Or just drop the sentence entirely as redundant?

TomSweeneyRedHat · 2017-04-27T22:02:49Z

docs/atomic-signature.md

+The consumer of the signature SHOULD verify the manifest digest against a fully verified signature before processing the contents of the image manifest in any other way
+(e.g. parsing the manifest further or downloading layers of the image).
+
+(Note that a single container image manifest may have several valid manifest digest values, using different algorithms.


Nit, I'd lose the outer parens here. ((Can you tell I'm not (a) parens fan yet)?)

Let’s try an “implementation notes” bullet list. (But parens are awesome! (Especially nested ones (like this (or even more nested!)), allowing a complex structure to be… easily parsed by a computer, at the cost of making it difficult to understand by people (which i guess demonstrates your point)).

mtrmac · 2017-04-28T20:13:10Z

@TomSweeneyRedHat Thanks! Updated, with the changes as a separate commit for easier review; I’ll squash them before merging if the result looks OK.

Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac force-pushed the signature-doc branch 3 times, most recently from ed8ad1d to 99ad0a5 Compare March 20, 2017 20:25

mtrmac mentioned this pull request Mar 20, 2017

Add JSON schema for the JSON embedded inside signatures #253

Merged

mtrmac force-pushed the signature-doc branch from 99ad0a5 to 4781989 Compare March 28, 2017 17:07

mtrmac force-pushed the signature-doc branch 2 times, most recently from de4415f to 16e3014 Compare March 30, 2017 17:15

aweiteka reviewed Apr 3, 2017

View reviewed changes

mtrmac force-pushed the signature-doc branch from 16e3014 to f4aede3 Compare April 3, 2017 18:51

aweiteka approved these changes Apr 3, 2017

View reviewed changes

mtrmac force-pushed the signature-doc branch 3 times, most recently from 5ef5b49 to bd9a242 Compare April 10, 2017 17:21

mtrmac force-pushed the signature-doc branch from bd9a242 to 7b9c804 Compare April 12, 2017 16:16

mtrmac force-pushed the signature-doc branch 3 times, most recently from 4fb5ef4 to e1510f8 Compare April 27, 2017 16:52

mtrmac force-pushed the signature-doc branch from e1510f8 to 3f11ff5 Compare April 27, 2017 21:05

TomSweeneyRedHat reviewed Apr 27, 2017

View reviewed changes

mtrmac force-pushed the signature-doc branch 2 times, most recently from 854d7a5 to 3f64911 Compare May 10, 2017 10:24

Add documentation for the atomic signature format

8a5d2f8

Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac force-pushed the signature-doc branch from 3f64911 to 8a5d2f8 Compare May 10, 2017 12:59

mtrmac merged commit 72e99ef into containers:master May 10, 2017

mtrmac deleted the signature-doc branch May 10, 2017 13:24

aweiteka mentioned this pull request Jun 14, 2017

Image signature format #59

Closed


		The consumer SHOULD have tests for its verification code which verify that signatures failing any of the above are rejected.

		(All of this is taken care of within the `github.com/containers/image/signature` package,

Add documentation for the atomic signature format #251

Add documentation for the atomic signature format #251

Uh oh!

Conversation

mtrmac commented Mar 11, 2017

Uh oh!

runcom commented Mar 28, 2017

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

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

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!

mtrmac commented Apr 3, 2017

Uh oh!

mtrmac commented Apr 18, 2017

Uh oh!

mtrmac commented Apr 27, 2017

Uh oh!

runcom commented Apr 27, 2017

Uh oh!

mtrmac commented Apr 27, 2017 • edited by runcom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

mtrmac May 10, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Apr 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mtrmac commented Apr 27, 2017 •

edited by runcom

Loading

runcom commented Apr 27, 2017 •

edited

Loading

mtrmac May 10, 2017 •

edited

Loading