[WIP] verify signature for untagged references #129

runcom · 2016-10-13T11:28:52Z

Hopefully the last piece needed for projectatomic/docker#200. This will allow us by default to pull an untagged reference (e.g. by digest) when the signature is valid for a tag.

This is still WIP - I would like feedback on the direction I'm going.

Signed-off-by: Antonio Murdaca [email protected]

runcom · 2016-10-13T11:29:09Z

@mtrmac PTAL even if this is still WIP

runcom · 2016-10-13T11:32:49Z

signature/policy_eval_signedby.go

 		validateSignedDockerReference: func(ref string) error {
+			r := image.Reference().DockerReference()
+			if _, isCanonical := r.(reference.Canonical); isCanonical || reference.IsNameOnly(r) {
+				if !pr.UntaggedReference.matchesDockerReference(image, ref) {


not sure...if it's a canonical reference, should we match the digest in the signature with the image's manifest digest? /cc @mtrmac

That definitely needs to happen somewhere, and right now it doesn’t. This is a bad fit, though, we should be verifying the user-specified digest even if no signatures exist or are required. So far I was thinking that the individual ImageSources which support digest references should do that in their GetManifests; it could probably be shared in copy.Image, OTOH it seems reasonable that this should be rejected for any use of that ImageSource.

That definitely needs to happen somewhere, and right now it doesn’t. This is a bad fit, though, we should be verifying the user-specified digest even if no signatures exist or are required. So far I was thinking that the individual ImageSources which support digest references should do that in their GetManifests; it could probably be shared in copy.Image, OTOH it seems reasonable that this should be rejected for any use of that ImageSource.

isn't this a separate issue though? I agree we need to verify the user provided digest against what we retrieve from the network though.

I was more talking about this new UntaggedReference match. So the default will be matchAnyTag which means an user is trying to pull testimage@sha256:HEX_1 but the user has only signed testimage:latest and latest's manifest happens to be sha256:HEX_1.
Question, how do we handle this for matchAnyTag? The signature is valid for the tag and the user requests the digest for that tag so we allow that pull since the tag's manifest digest == user's pull reference digest. What happen otherwise? Shall we error out? Anyhow, I think this confusion come from matchAnyTag since it will match only the tag whose manifest's digest == user's pull reference digsest and not really any tag for an untagged reference.

Also, why do we call it UntaggedReference? an user can't initiate a pull w/o a valid tag or reference. We should call it CanonicalReference maybe. The pull all tags case in docker isn't different since what it does underneath is to append every tag to the image name and download the image:tag (hence it's not untagged)

That definitely needs to happen somewhere, and right now it doesn’t. This is a bad fit, though, we should be verifying the user-specified digest even if no signatures exist or are required. So far I was thinking that the individual ImageSources which support digest references should do that in their GetManifests; it could probably be shared in copy.Image, OTOH it seems reasonable that this should be rejected for any use of that ImageSource.

I don't agree GetManifest(s) should do this check though. GetManifest just returns raw data, better to have callers verify digests, image sources are pretty low level, I wouldn't want to clutter them.

isn't this a separate issue though? I agree we need to verify the user provided digest against what we retrieve from the network though.

Yes, it is orthogonal to this PR.

Question, how do we handle this for matchAnyTag?

Well, the signature doesn’t just contain a tag which we happily apply to any manifest; it also contains a manifest digest. We will accept a signature which says sha256:HEX_1 and testimage:anythingwedontcare. The “anythingwedontcare” is the “any tag” in this terminology.

What happen otherwise?

What is the “otherwise” case?

Also, why do we call it UntaggedReference? an user can't initiate a pull w/o a valid tag or reference.

Well, it is about a reference, and the reference in this case doesn’t have a tag. (I guess atomic pull doesn’t work right now if there isn’t a tag, if that’s what you mean, but things like skopeo copy docker:// docker:// generally do.) I am definitely open to other suggestions.

We should call it CanonicalReference maybe.

Canonical is jargon, and more importantly it is not necessarily obvious to everyone that it means “with a @digest".signedIdentityWhenReferencingByDigest`…no.

can we assume people always want to pull-by-digest to match the tag in the signature and just have an "referencesByDigest = prohibit"?

pull-by-digest to match the tag in the signature
= that the signature should be also with a digest, or = to match any tag?
The latter would override effects of signedIdentity: exactReference.

Not naming the default behavior is an nice dodge :) Well, it could be prohibit | allow… and then we can just use true/false with a field name something like allowDigestReferences.

I like reject|allow 😄

~~referencesByDigest <- if allow this will of course verify user's digest == sig's digest as usual and then signedIdentity can kick in nicely after this right?~~ No, then we'll have the same issue that we want to allow pull-by-digest and we still need matchRepository...

From IRC chat

matchExact (which is the default signedIdentity) could turn into matchRepository if pull-by-digest and check that user's requested digest == digest in signature. We may probably still need referencesByDigest = reject|allow to allow admins to opt out of digets references and enforcing pulls by tag. See #129 (comment) also.

mtrmac · 2016-10-13T12:50:00Z

signature/policy_reference_match.go

+	if !reference.IsNameOnly(intended) {
+		return true
+	}
+	return signature.String() == intended.String()


This does not actually Prohibit?

nah, this was meant to be the matchPrecisely case

mtrmac · 2016-10-13T12:53:29Z

I didn’t review this line by line yet, but structurally this seems reasonable.

I can’t immediately see why there needs to be a separate Untagged{Prohibit,MatchAnyTag,MatchPrecisely} when we already have prmMatchExact and prmMatchRepository. Perhaps the implementations should be shared but the JSON syntax should be separate to better match the use case?

Anyway, I’ll return to this for a more detailed look later.

runcom · 2016-10-13T13:26:41Z

I can’t immediately see why there needs to be a separate Untagged{Prohibit,MatchAnyTag,MatchPrecisely} when we already have prmMatchExact and prmMatchRepository. Perhaps the implementations should be shared but the JSON syntax should be separate to better match the use case?

yup, I'm fine with that, I started coding w/o thinking about that just to have a working PR to test projectatomic/docker#200.

Thanks, I'll fix this up and re-ping you.

mtrmac · 2016-10-13T13:50:42Z

signature/policy_eval_signedby.go

+				if !pr.UntaggedReference.matchesDockerReference(image, ref) {
+					return PolicyRequirementError(fmt.Sprintf("Signature for identity %s is not accepted", ref))
+				}
+				return nil


Hum, this is awkward. If signedIdentity is exactSomething (i.e. overrides the repo name being matched), we would want to reuse this in the pull-by-digest case as well so that the user only needs to specify the name override once.

mmm but w/o this one would still add "matchRepository" to pull-by-digest which is awkward as well no?

Perhaps, for the UntaggedReference, or whatever it ends up being called, don’t reuse PolicyReferenceMatch but define an entirely new enum. This validateSignedDockerReference callback would then call pr.SignedIdentity.matchesDockerReference with an extra parameter, that enum value.

right, not sure I fully understand what you want to see though, matchesDockerReference(image types.UnparsedImage, signatureDockerReference string, untaggedPolicy PolicyUntaggedReferenceMatch)? where PolicyUntaggedReferenceMatch:

type PolicyUntaggedReferenceMatch interface { // ??? }

type sbUntaggedReferenceMatch string

mirroring how sbKeyType does it I think.

(policy_types.go somewhat mixes public types, with public members and public constructors, and the like, for manually creating policies in code, and internal implementation details, like the interfaces which are really private to policy_eval* and friends.)

alright, clear, so, when the user requests an image by digest this callback is called and matchesDockerReference is called with that sb string and do an if/else based on whether the image requested is canonical or not (I don't think it can happen to receive a request for reference.IsNameOnly right? even if I see in prmMatchExact you check that...)

It is not captured in the type system, but yes, types.ImageReference.DockerReference documents !reference.IsNameOnly.

mtrmac · 2016-10-13T15:46:18Z

BTW I am a bit tempted to make the default matchExact if unspecified, so that the default policy is strict and anything allowed is explicitly visible in the configuration when someone is reading it. (But it would be perfectly fine for atomic trust to set the filed to matchRepository by default.)

I realize this is probably bordering on crazily unusable…

runcom · 2016-10-17T09:37:56Z

@mtrmac PTAL as soon as you can, this version of the patch based on our discussions above works great with docker (except I need to polish, document and test the code :))

Signed-off-by: Antonio Murdaca <[email protected]>

mtrmac

So, for now:

The semantics of referencesByDigest is unclear (documentation does not match code)
The semantics of the new parameters of matchesDockerReference is undocumented.
Basically every single line, and every decision point (whether it is an extra if line or a comparison) should have a test, if at all possible).

I can help with the tests once the semantics and the approach are clear, to some extent; but the intent of the code is not immediately clear now.

mtrmac · 2016-10-24T14:57:42Z

signature/policy_config.go

 }

-// newPolicyRequirementFromJSON parses JSON data into a PolicyReferenceMatch implementation.
+// newPolicyReferenceFromJSON parses JSON data into a PolicyReferenceMatch implementation.


Better (thanks!), but still misses a Match

mtrmac · 2016-10-24T14:58:51Z

signature/policy_config.go

 	}
+	if referencesByDigest != SBKeyTypeUntaggedReferenceAllow && referencesByDigest != SBKeyTypeUntaggedReferenceReject {
+		return nil, InvalidPolicyFormatError("referencesByDigest accepts either allow or reject")
+	}


A test, please.

mtrmac · 2016-10-24T15:00:26Z

signature/policy_config.go

+		KeyPath:            keyPath,
+		KeyData:            keyData,
+		SignedIdentity:     signedIdentity,
+		ReferencesByDigest: referencesByDigest,


A test, please. (I would expect TestNewPRSignedBy to fail right now.)

mtrmac · 2016-10-24T15:01:06Z

signature/policy_config.go

 		case "signedIdentity":
 			return &signedIdentity
+		case "referencesByDigest":
+			return &tmp.ReferencesByDigest


A test, please. (Invalid value, duplicated value)

mtrmac · 2016-10-24T15:01:12Z

signature/policy_config.go


+	if tmp.ReferencesByDigest == "" {
+		tmp.ReferencesByDigest = SBKeyTypeUntaggedReferenceAllow
+	}


A test, please.

mtrmac · 2016-10-24T15:08:13Z

signature/policy_types.go

 	SignedIdentity PolicyReferenceMatch `json:"signedIdentity"`
+
+	// ReferencesByDigest specifies whether the signature is valid when the reference isn't tagged (e.g. canonical).
+	ReferencesByDigest sbUntaggedReferenceMatch `json:"referencesByDigest"`


The field name and the type name and the enum values all need to be named consistently.

mtrmac · 2016-10-24T15:10:59Z

docs/policy.json.md


 Exactly one of `keyPath` and `keyData` must be present, containing a GPG keyring of one or more public keys.  Only signatures made by these keys are accepted.

+The `referencesByDigest` field controls whether an reference with a digest should be taken into consideration when checking signatures or be rejected altogheter.


This seems unclear to me, the user does not have the context of our PR. What is “reference” and what is “taken into consideration”?

Perhaps something like

The referencesByDigest field controls treatment of images which are referred to (pulled, named, etc.) using a digest reference (e.g. busybox@sha256:… instead of a tag (busybox:latest).

mtrmac · 2016-10-24T15:50:12Z

docs/policy.json.md

+  ```json
+  {"referencesByDigest: allow"}
+  ```
+- References by digest are rejected and no signature verification is performed.


s,and no signature verification is preformed,regardless of the existence and/or contents of signatures, if any; users must refer to images using a tag, ? “no signature verification is performed” could be misinterpreted to mean that something is let through.

Nothing actually seems to implement it that way???! This ends up being used in the various if byDigest conditionals, at the end of both branches there is always an extra comparison. (BTW I’d expect a single enforcement point in prSignedBy.isSignatureAuthorAccepted, perhaps even before doing any cryptography.)

mtrmac · 2016-10-24T15:51:42Z

docs/policy.json.md

+
+- References by digest are allowed (default). In this case, the digest should match the one from the signature for the verification to be successful.
+  ```json
+  {"referencesByDigest: allow"}


The {…} form for signedIdentity is used because a signedIdentity value is such an object; but the above is not a valid "type":"signedBy" object. Perhaps, just use a bulleted list like - "allow": Reference by digest are …

mtrmac · 2016-10-24T15:57:46Z

signature/policy_eval.go

+	//
+	// TODO(runcom): document byDigest
+	// TODO(runcom): document digest
+	matchesDockerReference(image types.UnparsedImage, signatureDockerReference, signatureManifstDigest string, byDigest bool) bool


Yes, document. I’ll continue reading the code to figure this out, but the intent needs to be clearly stated in words as well.

mtrmac · 2016-10-24T16:33:38Z

Trying to document what this code does right now:

`signedIdentity`	`referencesByDigest`	user’s reference	requirement
`matchExact`	either	tag	signature is for a repo:tag which matches user’s input
`matchExact`	`reject`	digest	signature is for a repo@digest which matches user’s input (not necessarily matching the real manifest!)
`matchExact`	`allow`	digest	signature is for a repo@digest which matches user’s input (not necessarily matching the real manifest!) OR signature is a repo[$anything], repo matches user’s, signed real manifest matches digest in user’s input
`matchRepository`	either	[tag]	signature is a repo[$anything], repo matches user’s
`matchRepository`	`reject`	digest	signature is a repo[$anything], repo matches user’s
`matchRepository`	`allow`	digest	signature is a repo[$anything], repo matches user’s AND signed real manifest matches digest in user’s input
`exactReference`	either	tag	signature is for a repo:tag which matches user’s input
`exactReference`	`reject`	digest	signature is for a repo:tag which matches user’s input (not necessarily matching the real manifest!)
`exactReference`	`allow`	digest	signature is for a repo:tag which matches user’s input (not necessarily matching the real manifest!) OR signature is a repo$anything, repo matches user’s, signed real manifest matches digest in user’s input
`exactRepository`	either	[tag]	signature is a repo[$anything], repo matches user’s
`exactRepository`	`reject`	digest	signature is a repo[$anything], repo matches user’s
`exactRepository`	`allow`	digest	signature is a repo[$anything], repo matches user’s AND signed real manifest matches digest in user’s input.

WIP I will continue the table, but right here when matchRepository/allow is more strict than matchRepository/reject I am either mistaken or the design is too unclear/confusing.

(Update: matchRepository and exactRepository are the same; OTOH matchExact ad exactReference re inconsistent in treatment of IsNameOnly values.)

BTW: What happens if the used reference is both with a digest and tag? Can it happen?
AFAICS c/i/d/reference.ParseNamed parses everything into either a tagged or digested, preferring digested; but nothing in the API seems to make it impossible for image.Reference().DockerReference() to be both.

mtrmac · 2016-10-24T18:18:39Z

… actually, the exactRe* cases don’t care about image.Reference().DockerReference() at all; the whole point is that the users’ reference is, for the purpose of signature matching, superseded by a reference in the policy. (Users’ digest still must match, per the enforcement in UnparsedImage.Manifest, but that happens before signatures come into play.) And matchRepository doesn’t care about the tag/digest by definition.

So, @runcom, what about https://github.com/mtrmac/image/tree/untagged-reference-sig-check instead? Without exactRe*, the only remaining case which really cares about image.Reference().DockerReference().{Tag(),Digest()} is matchExact, so we can make this much more local. (Or can we? I have a nasty feeling that we’ve been through this an I am now going full circle…) Or, actually, we can leave matchExact as it is, add a new matchTagOrAnyDigest (or something), and make that a default for signedBy; anyway, details.

(Doesn’t have tests, isn‘t production ready, yadda yadda. But it compiles and it could be enough to determine whether this would work for docker/docker or whether I am missing something.)

runcom · 2016-10-24T18:37:07Z

@mtrmac I'll test that out as soon as I remove #129 from in projectatomic/docker#200 (or tomorrow at last)

runcom · 2016-10-25T10:41:14Z

So, @runcom, what about https://github.com/mtrmac/image/tree/untagged-reference-sig-check instead? Without exactRe*, the only remaining case which really cares about image.Reference().DockerReference().{Tag(),Digest()} is matchExact, so we can make this much more local. (Or can we? I have a nasty feeling that we’ve been through this an I am now going full circle…)

@mtrmac seems like you didn't forget anything and your branch seems to work correctly with docker! Let me know if I can grab it, add tests and whatnot and re-submit a PR here in containers/image (we'll likely fix pull-by-digest in atomic with this!)

Here is the working docker code projectatomic/docker#209

@mtrmac pls check out the testing done so far in docker https://github.com/projectatomic/docker/blob/docker-1.12/integration-cli/docker_cli_pull_local_test.go#L635

Update for changed images.Type API

runcom commented Oct 13, 2016

View reviewed changes

mtrmac reviewed Oct 13, 2016

View reviewed changes

runcom force-pushed the untagged-reference-sig-check branch from b2cd10e to d2c406e Compare October 13, 2016 13:44

mtrmac reviewed Oct 13, 2016

View reviewed changes

runcom force-pushed the untagged-reference-sig-check branch 5 times, most recently from b58f688 to dbc0bda Compare October 17, 2016 09:32

runcom force-pushed the untagged-reference-sig-check branch 5 times, most recently from 1e87956 to 5dcfae5 Compare October 19, 2016 16:22

x

066c523

Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the untagged-reference-sig-check branch from 5dcfae5 to 066c523 Compare October 21, 2016 14:40

mtrmac requested changes Oct 24, 2016

View reviewed changes

mtrmac mentioned this pull request Nov 7, 2016

Add support for digest references to signature enforcement #156

Merged

runcom closed this Nov 7, 2016

giuseppe pushed a commit to giuseppe/image that referenced this pull request Jan 24, 2017

Merge pull request containers#129 from mtrmac/api-update

1822312

Update for changed images.Type API


		Exactly one of `keyPath` and `keyData` must be present, containing a GPG keyring of one or more public keys. Only signatures made by these keys are accepted.

		The `referencesByDigest` field controls whether an reference with a digest should be taken into consideration when checking signatures or be rejected altogheter.

[WIP] verify signature for untagged references #129

[WIP] verify signature for untagged references #129

Uh oh!

Conversation

runcom commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Oct 13, 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

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!

runcom Oct 14, 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!

runcom Oct 14, 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!

mtrmac commented Oct 13, 2016

Uh oh!

runcom commented Oct 13, 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom Oct 13, 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!

mtrmac commented Oct 13, 2016

Uh oh!

runcom commented Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac left a comment

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

runcom commented Oct 13, 2016 •

edited

Loading

runcom Oct 14, 2016 •

edited

Loading

runcom Oct 14, 2016 •

edited

Loading

runcom Oct 13, 2016 •

edited

Loading

runcom commented Oct 17, 2016 •

edited

Loading

mtrmac commented Oct 24, 2016 •

edited

Loading

runcom commented Oct 25, 2016 •

edited

Loading