[docker-1.12] Signature verification on pull #200

runcom · 2016-09-30T13:56:16Z

This patch implements signature verification through containers/image directly into the daemon.

Depends on containers/image#99 (you can workaround that by following containers/image#99 (comment))

/cc @mtrmac @rhatdan @ddarrah

rhatdan · 2016-09-30T14:12:15Z

What happens if the policy file does not exist? Do we default succeed?

runcom · 2016-09-30T14:12:54Z

What happens if the policy file does not exist? Do we default succeed?

we default fail

since this is going to be part of the docker package we'll ship a default policy.json and if it doesn't exists in /etc/containers/policy.json we badly bail out

mtrmac · 2016-09-30T14:53:54Z

policy.json

+            "type": "insecureAcceptAnything"
+        }
+    ]
+}


I‘d rather not have two RPMs owning policy.json in the distribution at the same time… but I don’t have a strong opinion on which one should own it.

Yes I don't think docker should own it. Probably best if owned by skopeo.

@lsm5, Miloslav suggested to have a "skopeo-container-policy" package which just ships the policy.json file under /etc/containers/. This package will be Required by both skopeo and docker.

mtrmac · 2016-09-30T14:54:30Z

distribution/pull_v2.go

@@ -85,9 +92,51 @@ func (p *v2Puller) Pull(ctx context.Context, ref reference.Named) (err error) {
 	return err
 }

+func (p *v2Puller) checkTrusted(ref reference.Named) (reference.Named, error) {


ACK on this function; didn’t look at the rest in too much detail.

runcom · 2016-10-10T21:51:05Z

distribution/pull_v2.go

@@ -94,12 +95,24 @@ func (p *v2Puller) Pull(ctx context.Context, ref reference.Named) (err error) {

 func (p *v2Puller) checkTrusted(ref reference.Named) (reference.Named, error) {
 	p.originalRef = ref
-	imgRef, err := docker.NewReference(ref)
+	dockerRef, err := containersImageRef.ParseNamed(ref.String())


@mtrmac PTAL at this since in projectatomic/docker it's the only way to have a consistent reference parsing for signatures. It's working like this, pinging you to understand if I'm missing something. (this is related to containers/image#123).
Without this change, ref is parsed using projectatomic/docker's reference and contains the hostname and makes the signatures check fails.

ACK. It is a bit horrible, but that’s what we get with Golang’s interface-as-collection-of-method-signatures type system instead of an interface-as-a-name-and-explicitly-committed-to-protocol.

Please make sure to add a comment explaining why we can't use docker.NewReference without the ParseNamed(ref.String()) roundtrip so that this isn’t “cleaned up” later when we forget or a new maintainer comes.

runcom · 2016-10-12T09:35:07Z

@rhatdan @mtrmac alright, integration tests are green locally (finally), I need only containers/image#126 merged and a revendor here and this seems really in a good shape. The last blocker I have is containers/image#99 which I'm working on right now but I feel we can start building rpms (in Fedora at least) so people can start testing this out.

rhatdan · 2016-10-12T12:51:13Z

OK I see checks are still failing?

runcom · 2016-10-12T12:59:07Z

@rhatdan nah, those are related to the RH CI which has been always broken for various reasons like the current one:

/usr/local/go/pkg/tool/linux_amd64/link: running gcc failed: fork/exec /usr/bin/gcc: cannot allocate memory

which is unrelated to this PR

runcom · 2016-10-12T13:03:37Z

alright, containers/image#126 got merged, now we're only missing containers/image#99 which I'm working on but it's not a total blocker so I'd say we can start testing this out /cc @lsm5 @ddarrah @rhatdan

runcom · 2016-10-24T18:46:44Z

@mtrmac PTAL - This is based on containers/image master code so, in order to pull-by-digest AND verify with GPG one must use https://github.com/containers/image/blob/master/docs/policy.json.md#a-temporary-work-around-to-allow-accessing-any-image-by-digest. Please ack on this.

@jwhonce @rhatdan @imcleod I'm running integration tests locally, after that, ready to merge!

mtrmac

A possible improvement, but LGTM.

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

distribution/pull_v2_unix.go

+		DockerAuthConfig:            &dockerAuthConfig,
+		DockerRegistryUserAgent:     dockerversion.DockerUserAgent(c),
+	}
+	img, err := imgRef.NewImage(ctx)


This could now be more secure with a imgRef.NewImageSource→image.UnparsedFromSource, the way copy.Image does it; it would skip all of the risky manifest parsing image.FromUnparsedImage is doing which nothing here seems to need. (AFAICT this change wouldn’t affect the behavior with manifest lists, but that would be worth checking).

ack, will fix it if it's not totally breaking manifest lists.

mtrmac · 2016-10-24T19:00:42Z

distribution/pull_v2.go

+		ref, err = p.checkTrusted(ctx, ref)
+		if err != nil {
+			return allowV1Fallback(err)
+		}


Wait, what happens on the v1 fallback? Do we just accept any unsigned image on that path? So all the attacker needs to do is to DoS the signature fetch/verification so that we fall back?

I forgot about this but meant to ping you, what do we do here? I'm ok not falling back.

we can even add a flag on the daemon which explicitly allow v1 fallback.

alright, v1 fallback is now disabled if there's any error checking the image's signature

runcom · 2016-10-24T20:14:16Z

@rhatdan @jwhonce @imcleod this is good to be merged now

Last thing I've added is a flag to completely opt-out of image's sigs verification (just in case anyone is having troubles! as discussed on IRC with @mtrmac)

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

runcom · 2016-10-24T20:52:47Z

"finger crossed" lol

mtrmac reviewed Sep 30, 2016

View reviewed changes

This was referenced Oct 1, 2016

types,docker: add DockerAuthConfig containers/image#113

Merged

Manifest lists break signatures verification? containers/image#114

Closed

manifest: enable DockerV2List containers/image#115

Merged

runcom force-pushed the verify-sigs-on-pull-1.12 branch from 2cd2234 to 30cdf42 Compare October 7, 2016 14:55

This was referenced Oct 10, 2016

Match a signature for any tag when pulling/coping by digest containers/image#99

Closed

docker: fork docker/docker/reference pkg containers/image#123

Merged

runcom force-pushed the verify-sigs-on-pull-1.12 branch 2 times, most recently from 670365c to 7481164 Compare October 10, 2016 21:49

runcom commented Oct 10, 2016

View reviewed changes

runcom force-pushed the verify-sigs-on-pull-1.12 branch 3 times, most recently from 08b2f4d to 75e6672 Compare October 11, 2016 14:59

runcom mentioned this pull request Oct 11, 2016

types: add DockerRegistryUserAgent to SystemContext containers/image#124

Merged

runcom force-pushed the verify-sigs-on-pull-1.12 branch from 75e6672 to fe7686d Compare October 11, 2016 16:24

This was referenced Oct 11, 2016

pluggable transports? containers/image#125

Closed

docker: reuse docker/distribution registry errros containers/image#126

Merged

runcom force-pushed the verify-sigs-on-pull-1.12 branch from fe7686d to 5ada5fe Compare October 12, 2016 09:51

runcom force-pushed the verify-sigs-on-pull-1.12 branch from 5ada5fe to 06825df Compare October 12, 2016 13:00

This was referenced Oct 12, 2016

add sigstore default configuration containers/skopeo#233

Merged

[WIP] verify signature for untagged references containers/image#129

Closed

runcom force-pushed the verify-sigs-on-pull-1.12 branch from bf404d5 to 691ce93 Compare October 17, 2016 09:37

runcom mentioned this pull request Oct 17, 2016

[fedora-1.10.3] Signature verification on pull #206

Closed

runcom mentioned this pull request Oct 17, 2016

[docker-1.12-2] Signature verification on pull #205

Merged

runcom force-pushed the verify-sigs-on-pull-1.12 branch 5 times, most recently from 407a421 to 967b10c Compare October 21, 2016 14:48

runcom force-pushed the verify-sigs-on-pull-1.12 branch from 967b10c to 8ad06e4 Compare October 24, 2016 18:42

mtrmac approved these changes Oct 24, 2016

View reviewed changes

mtrmac reviewed Oct 24, 2016

View reviewed changes

runcom force-pushed the verify-sigs-on-pull-1.12 branch from 8ad06e4 to 556d579 Compare October 24, 2016 20:11

runcom force-pushed the verify-sigs-on-pull-1.12 branch 3 times, most recently from 1519241 to 8fc3c69 Compare October 24, 2016 20:46

Add images signatures verification with containers/image

918c0a7

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

runcom force-pushed the verify-sigs-on-pull-1.12 branch from 8fc3c69 to 918c0a7 Compare October 24, 2016 20:50

rhatdan merged commit f97f83e into projectatomic:docker-1.12 Oct 24, 2016

runcom deleted the verify-sigs-on-pull-1.12 branch October 24, 2016 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[docker-1.12] Signature verification on pull #200

[docker-1.12] Signature verification on pull #200

runcom commented Sep 30, 2016 •

edited

Loading

rhatdan commented Sep 30, 2016

runcom commented Sep 30, 2016 •

edited

Loading

mtrmac Sep 30, 2016

rhatdan Sep 30, 2016

runcom Sep 30, 2016

rhatdan Sep 30, 2016

mtrmac Sep 30, 2016

runcom Oct 10, 2016 •

edited

Loading

mtrmac Oct 11, 2016

runcom commented Oct 12, 2016 •

edited

Loading

rhatdan commented Oct 12, 2016

runcom commented Oct 12, 2016 •

edited

Loading

runcom commented Oct 12, 2016

runcom commented Oct 24, 2016

mtrmac left a comment

mtrmac Oct 24, 2016

runcom Oct 24, 2016 •

edited

Loading

mtrmac Oct 24, 2016

runcom Oct 24, 2016

runcom Oct 24, 2016

runcom Oct 24, 2016

runcom commented Oct 24, 2016 •

edited

Loading

runcom commented Oct 24, 2016

[docker-1.12] Signature verification on pull #200

[docker-1.12] Signature verification on pull #200

Conversation

runcom commented Sep 30, 2016 • edited Loading

rhatdan commented Sep 30, 2016

runcom commented Sep 30, 2016 • 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

runcom Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runcom commented Oct 12, 2016 • edited Loading

rhatdan commented Oct 12, 2016

runcom commented Oct 12, 2016 • edited Loading

runcom commented Oct 12, 2016

runcom commented Oct 24, 2016

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runcom Oct 24, 2016 • 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

runcom commented Oct 24, 2016 • edited Loading

runcom commented Oct 24, 2016

runcom commented Sep 30, 2016 •

edited

Loading

runcom commented Sep 30, 2016 •

edited

Loading

runcom Oct 10, 2016 •

edited

Loading

runcom commented Oct 12, 2016 •

edited

Loading

runcom commented Oct 12, 2016 •

edited

Loading

runcom Oct 24, 2016 •

edited

Loading

runcom commented Oct 24, 2016 •

edited

Loading