API for providing untrusted data about unverified signatures #208

mtrmac · 2017-01-11T19:52:16Z

This utlimately adds signature.UntrustedSignatureInformation and signature.GetUntrustedSignatureInformationWithoutVerifying; https://github.com/mtrmac/skopeo/blob/unverified-contents/cmd/skopeo/signing.go#L93 is an example user, with output like

{
    "UntrustedDockerManifestDigest": "sha256:a59906e33509d14c036c8678d687bd4eec81ed7c4b8ce907b888c607f6a1e0e6",
    "UntrustedDockerReference": "mtrmac/busybox-test:latest",
    "UntrustedCreatorID": "atomic 0.1.0-dev",
    "UntrustedTimestamp": 1472502383,
    "UntrustedShortKeyIdentifier": "5A33F660B38479DF"
}

(For a real code review, see the individual commit messages for detailed descriptions. But right now this is missing tests, so please do not merge.)

mtrmac · 2017-01-11T19:55:42Z

signature/signature.go

+	UntrustedDockerManifestDigest digest.Digest
+	UntrustedDockerReference      string // FIXME: more precise type?
+	UntrustedCreatorID            *string
+	UntrustedTimestamp            *int64


Should this be a stupid int64 or a time.Time? time.Time would be nicer to use at the cost of a tiny risk that the int64→Time conversion might be vulnerable (though looking at the implementation that seems extremely unlikely, for the current implementation at least).

Actually, there is no risk/tradeoff here; we can do the UnmarshalJSON into an int64 in the private untrustedSignature, and do the conversion to time.Time in GetUntrustedSignatureInformationWithoutVerifying. Changed like that.

I still think this should be *time.Time so the consumers life will be easier (they don't have to convert this to a time which is what you want to display to users).

Right now the only consumer is GetUntrustedSignatureInformationWithoutVerifying, so placement of the conversion makes no difference to the total complexity of the package.

Using the native int64 here is nice in that it explicitly corresponds to the JSON semantics, and we are more certain that we can round-trip the value and that the caller setting the value is aware about the conversion (e.g., compared to time.Time, that this records neither timezones nor sub-second resolutions).

Though, perhaps, before we start using this field in earnest and before we ship the first version of a validating parser of this fields, perhaps we should change the type to a float64 (with its sub-second resolution and a more restricted precision), which is what JSON actually stores? The int64 also has conversion issues. @runcom ?

mfojtik · 2017-01-11T21:11:59Z

I will be better to have time.Time as it will need to be converted anyway to store as api field.

…

-- Michal Fojtik

On 11 January 2017 at 20:55:42, Miloslav Trmač ***@***.***) wrote: ***@***.**** commented on this pull request. ------------------------------ In signature/signature.go <#208 (review)>: > type Signature struct { DockerManifestDigest digest.Digest DockerReference string // FIXME: more precise type? } -// Wrap signature to add to it some methods which we don't want to make public. -type privateSignature struct { - Signature +// untrustedSignature is a parsed content of a signature. +type untrustedSignature struct { + UntrustedDockerManifestDigest digest.Digest + UntrustedDockerReference string // FIXME: more precise type? + UntrustedCreatorID *string + UntrustedTimestamp *int64 Should this be a stupid int64 or a time.Time? time.Time would be nicer to use at the cost of a *tiny* risk that the int64→Time conversion might be vulnerable (though looking at the implementation that seems *extremely* unlikely, for the *current* implementation at least). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#208 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACsaFJrN6YOUEIgOOKuwGtiYS5bo4oeks5rRTO-gaJpZM4Lg_xY> .

mfojtik · 2017-01-12T11:20:32Z

signature/signature.go

+// There is NO REASON to expect the values to be correct, or not intentionally misleading
+// (including things like “✅ Verified by $authority”)
+type UntrustedSignatureInformation struct {
+	UntrustedDockerManifestDigest digest.Digest


Strip the Untrusted prefix from the fields (the struct is enough)

The type name is invisible with info, err := …; and the point of use of the structure and the point of calling the producer of the value may be pretty far apart.

I think it is more Go idiomatic to not repeat the prefix if the struct in the functions that struct define. The callers should use this like:

untrustedInfo, err := .... untrustedInfo.DockerManifestDigest()

WDYT?

Honestly I am fairly uncomfortable with providing the unverified information because I think there is a pretty big risk either code, or, more likely, humans will trust it anyway (while I do realize that providing it is unavoidable), and naming the fields this way is my last line of defense against inadvertent mistakes. Of course nothing forces any consumer to use these names in an UI.

mfojtik · 2017-01-12T11:21:03Z

signature/signature.go

+	UntrustedDockerManifestDigest digest.Digest
+	UntrustedDockerReference      string // FIXME: more precise type?
+	UntrustedCreatorID            *string
+	UntrustedTimestamp            *int64


This should be really *time.Time

mfojtik · 2017-01-16T10:01:18Z

signature/json.go

+func int64Field(m map[string]interface{}, fieldName string) (int64, error) {
+	untyped, ok := m[fieldName]
+	if !ok {
+		return -1, jsonFormatError(fmt.Sprintf("Field %s missing", fieldName))


no need to return -1 here. the 0 is default for int64 when not set.

Well it is returning -1, not 0. Anyway, there is no “good” undefined value to return here if the caller makes a mistake and ignores err. -1 is perhaps slightly less likely to be valid than a non-negative number… but it really is anybody’s guess whether that makes it more likely to be noticed as invalid or more likely to have completely unpredicted effects. (Go’s error handling sucks, it would be nice to have something like optional checked exceptions, so that a caller can’t ignore an error by mistake without it being very visible.)

It seems to me not to be worth it to bother with named return parameters so that we can skip explicitly naming a value returned in the error case—especially because that also makes a decision, namely to use 0, only that decision is not explicitly visible.

@mtrmac the called should never ignore "err", doing so is a programmer error and we can't prevent all human errors :-)

I'm fine with -1, just pointing out it is something unusual to me as usually if the return value is not defined, we default to a default value of the type (which in case of int64 is '0').

@mtrmac the called should never ignore "err", doing so is a programmer error and we can't prevent all human errors :-)

True, and yet, we are in the business of engineering tools and interfaces to minimize errors anyway. (Ordinarily I wouldn’t bother, but the signature subpackage brings out my worst pedantic tendencies.)

mfojtik · 2017-01-16T15:33:44Z

LGTM

@mtrmac @runcom is there something left in this PR (that cannot be a follow up?)

mtrmac · 2017-01-17T21:28:16Z

Updated:

Added tests
Separated unrelated trivial fixes into Trivial fixes #213.
(Tried making the timestamp a float64, but that doesn't have enough precision to represent nanosecond timestamps, so this turned out to be a dead end.)

@mfojtik @runcom PTAL, this is now ready for review.

mfojtik · 2017-01-18T10:10:24Z

signature/mechanism.go


 	"github.com/mtrmac/gpgme"
+	"golang.org/x/crypto/openpgp"


this wont work for openshift, we will not build with gpgme :(

This does not add any new uses of gpgme. Getting rid of gpgme happens in #207 not here (though the two do touch the same code, so the latter one will have to be modified. I expect the newly added UntrustedSignatureContents to remain in mechanism.go as a shared helper, called by both GPG backends.)

mfojtik · 2017-01-19T15:01:11Z

LGTM

runcom · 2017-01-19T15:06:22Z

👍

…ng a Signature Instead of a privateSignature containing a Signature, and using the privateSignature type to attach private implementatinos of json.Marshaler and json.Unmarshaler and other private methods, use a completely separate private untrustedSignature type. This allows us to use scarier Untrusted… names for the members, but the only real code change is that verifyAndExtractSignature now needs to do a member-by-member copy instead of copying the full Signature struct. Signed-off-by: Miloslav Trmač <[email protected]>

.Unix() ignores the time location, so .UTC() does’t really do anything. Signed-off-by: Miloslav Trmač <[email protected]>

Instead of silently embedding values in untrustedSignature.MarshalJSON (and having to add a marshalJSONWithvariables to work around this), make the creator ID and timestamp explicit fields of untrustedSignature, and MarshalJSON a simple marshaller of existing data. The values are now filled by calling newUntrustedSignature. Now that the fields are explicit, we can also record them by untrustedSignature.UnmarshalJSON. This also explicitly defines the timestamp to be an integer, instead of allowing floating-point values, because the JSON float64 is not precise enough for nanosecond timstamps. For now, we reject fractional values, which will allow us to record the nanosecond part separately in the future if it became necessary. Signed-off-by: Miloslav Trmač <[email protected]>

Add signature.GetUntrustedSignatureInformationWithoutVerifying, which returns the contents of a signature without doing any cryptographic verification. Expected uses are 1) debugging verification failures, and (CAREFULLY!) 2) listing which signatures are stored in a registry along with an image. It is STRONGLY recommended to do cryptographic verification in the latter case, and to clearly indicate untrusted signatures in the UI. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac · 2017-01-20T21:36:46Z

Tests pass in containers/skopeo#293.

runcom · 2017-01-20T22:53:31Z

Feel free to merge

mtrmac · 2017-01-23T15:40:50Z

👍

mtrmac commented Jan 11, 2017

View reviewed changes

mfojtik reviewed Jan 12, 2017

View reviewed changes

mtrmac force-pushed the unverified-contents branch 2 times, most recently from f8c8ea5 to ba21734 Compare January 13, 2017 17:03

mfojtik reviewed Jan 16, 2017

View reviewed changes

mtrmac force-pushed the unverified-contents branch 9 times, most recently from 8d83159 to 626c43f Compare January 17, 2017 21:26

mtrmac changed the title ~~RFC WIP: API for providing untrusted data about unverified signatures~~ API for providing untrusted data about unverified signatures Jan 17, 2017

mfojtik reviewed Jan 18, 2017

View reviewed changes

mtrmac force-pushed the unverified-contents branch from 626c43f to d8face5 Compare January 19, 2017 23:23

mtrmac added 4 commits January 20, 2017 21:11

Don’t call .UTC() on a time.Time before .Unix()

69464bd

.Unix() ignores the time location, so .UTC() does’t really do anything. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac force-pushed the unverified-contents branch from d8face5 to 596d319 Compare January 20, 2017 20:12

mtrmac mentioned this pull request Jan 20, 2017

Vendor in mtrmac/image:unverified-contents and add a (skopeo dump-untrusted-signature-contents-without-verification) containers/skopeo#293

Merged

mtrmac merged commit 361643f into containers:master Jan 23, 2017

mtrmac deleted the unverified-contents branch January 23, 2017 15:41

mtrmac mentioned this pull request Jan 23, 2017

Allow choosing between a gpgme and openpgp signature backend using a build tag #207

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API for providing untrusted data about unverified signatures #208

API for providing untrusted data about unverified signatures #208

mtrmac commented Jan 11, 2017

mtrmac Jan 11, 2017

mtrmac Jan 13, 2017

mfojtik Jan 16, 2017

mtrmac Jan 16, 2017

mfojtik commented Jan 11, 2017 via email

mfojtik Jan 12, 2017

mtrmac Jan 12, 2017

mfojtik Jan 16, 2017

mtrmac Jan 16, 2017

mfojtik Jan 12, 2017

mfojtik Jan 16, 2017

mtrmac Jan 16, 2017

mfojtik Jan 16, 2017

mtrmac Jan 17, 2017

mfojtik commented Jan 16, 2017

mtrmac commented Jan 17, 2017

mfojtik Jan 18, 2017

mtrmac Jan 18, 2017

mfojtik commented Jan 19, 2017

runcom commented Jan 19, 2017 •

edited

Loading

mtrmac commented Jan 20, 2017

runcom commented Jan 20, 2017

mtrmac commented Jan 23, 2017 •

edited by runcom

Loading


		"github.com/mtrmac/gpgme"
		"golang.org/x/crypto/openpgp"

API for providing untrusted data about unverified signatures #208

API for providing untrusted data about unverified signatures #208

Conversation

mtrmac commented Jan 11, 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

mfojtik commented Jan 11, 2017 via email

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 Jan 16, 2017

mtrmac commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Jan 19, 2017

runcom commented Jan 19, 2017 • edited Loading

mtrmac commented Jan 20, 2017

runcom commented Jan 20, 2017

mtrmac commented Jan 23, 2017 • edited by runcom Loading

runcom commented Jan 19, 2017 •

edited

Loading

mtrmac commented Jan 23, 2017 •

edited by runcom

Loading