Verify signatures on pull #157

mtrmac · 2016-08-10T23:03:51Z

Adds /etc/atomic/policy.json, and a command-line option to override (skopeo --policy some_path copy …), and enforces this policy in skopeo copy.

make install now installs default-policy.json to /etc/atomic/policy.json; the policy is set to insecureAcceptAnything for all images by default. (This may not be desirable long-term, but it is a reasonable transition strategy for now I think. The policy always uses the most specific matching scope, so it is easy to e.g. pin a key and require signatures for a specific repository namespace.)

Note that with merging this, ./skopeo will abort if the policy in /etc/atomic does not exist. This should not affect real deployments, but may be noticeable with local checkouts. A bit regrettable, but we do need to fail closed if a policy can’t be read. (Use ./skopeo --policy default-policy.json … in a checkout.)

This depends on, and vendors in, unmerged containers/image#49 and containers/image#50 , I will revendor as necessary.

@lsm5 , note that adding the config file will affect packaging.

@rhatdan , any comments on the path /etc/atomic/policy.json?

mtrmac · 2016-08-10T23:08:05Z

(5) Container image signing and verification with a single key

lsm5 · 2016-08-11T00:43:55Z

On Wed, Aug 10, 2016 at 04:03:54PM -0700, Miloslav Trmač wrote:

@lsm5 , note that adding the config file will affect packaging.

sure thing, thanks for the headsup

Lokesh
Freenode: lsm5
GPG: 0xC7C3A0DD

runcom · 2016-08-25T13:29:42Z

I like /etc/atomic/policy.json

Unsure about failing hard if not /etc/atomic exists though - I guess I have no strong opinion, perhaps just tell something useful to the user suffices

LGTM though, reviewed #50 as well

mtrmac · 2016-08-25T13:54:00Z

Unsure about failing hard if not /etc/atomic exists though - I guess I have no strong opinion, perhaps just tell something useful to the user suffices

$ ./skopeo copy dir:a dir:b
FATA[0000] Error loading verification policy: open /etc/atomic/policy.json: no such file or directory

runcom · 2016-08-25T13:56:33Z

Unsure about failing hard if not /etc/atomic exists though - I guess I have no strong opinion, perhaps just tell something useful to the user suffices
$ ./skopeo copy dir:a dir:b
FATA[0000] Error loading verification policy: open /etc/atomic/policy.json: no such file or directory

yup, saying I'm fine with that :)

rhatdan · 2016-08-25T14:30:20Z

Should the path be vendor neutral? Or is this a path handed to container/images?

runcom · 2016-08-25T14:37:48Z

after meeting: /etc/container seems ok

mtrmac · 2016-08-25T14:52:47Z

The path comes from containers/image, so that various tools all use the same file.

To be precise, the path would be /etc/container/policy.json? (I can’t find any package owning /etc/container in F24, so I’m not sure where to look for other users of that directory and possible problems.)

mtrmac · 2016-08-25T16:34:16Z

Revendored to use /etc/containers/policy.json (pending re-review of containers/image#49 ), and simplified getPolicyContext a little.

runcom · 2016-08-25T17:09:32Z

cmd/skopeo/main.go

+// getPolicyContext handles the global "policy" flag.
+func getPolicyContext(c *cli.Context) (*signature.PolicyContext, error) {
+	policyPath := c.GlobalString("policy")
+	var policy *signature.Policy // This could be cached across calls, if we had an application context.


Can be contacted in a single var block

Yeah, OTOH it is easier to attach the “This could be cached” comment to the policy variable when it is on a separate line.

I believe it's a somehow golang idiom, you can still add comments on top of each variable in a var block

runcom · 2016-08-25T17:10:42Z

Just a nit, LGTM otherwise

Feel free to merge containers/image bit, re vendor here and merge here

This ordinarily uses the compiled-in default, but allows per-command override. No users yet. Note that this adds an URL to policy documentation within containers/image, and that URL does not exist at the moment.

…on tests (skopeo copy) will soon ALWAYS require a present policy file. So, install one by (make install), and ensure that integration tests do so as well. Also simplifies the usage of install(1) a bit.

Finally, load and enforce the policy. NOTE that this breaks a simple ./skopeo from a built directory if you don't have /etc/atomic/policy.json installed for other reasons; use (./skopeo --policy default-policy.json) instead.

mtrmac · 2016-08-25T17:44:07Z

Thanks for the review!

I have decided to drop the dependency on containers/image#50 ; the cost is that the link in skopeo.1 will be pointing at a non-existent URL for a time, the benefit is that this makes containers/image#50 a branch with no relationships and greatly simplifies the structure of outstanding branches.

mtrmac mentioned this pull request Aug 10, 2016

Copy signing integration tests #158

Merged

mtrmac mentioned this pull request Aug 10, 2016

Add documentation for the signature verification policy files containers/image#50

Merged

mtrmac force-pushed the verify-on-pull branch 2 times, most recently from 265a829 to a104f2d Compare August 22, 2016 14:59

mtrmac force-pushed the verify-on-pull branch 2 times, most recently from 525aa32 to 28ec068 Compare August 25, 2016 16:33

runcom reviewed Aug 25, 2016
View reviewed changes

mtrmac added 4 commits August 25, 2016 19:36

Vendor after merging mtrmac/image:default-policy

8826f09

Add a new --policy flag.

d5d6bc2

This ordinarily uses the compiled-in default, but allows per-command override. No users yet. Note that this adds an URL to policy documentation within containers/image, and that URL does not exist at the moment.

Add a default policy file, install it in (make install) and integrati…

d446233

…on tests (skopeo copy) will soon ALWAYS require a present policy file. So, install one by (make install), and ensure that integration tests do so as well. Also simplifies the usage of install(1) a bit.

Implement policy enforcement in (skopeo copy)

76a1498

Finally, load and enforce the policy. NOTE that this breaks a simple ./skopeo from a built directory if you don't have /etc/atomic/policy.json installed for other reasons; use (./skopeo --policy default-policy.json) instead.

mtrmac force-pushed the verify-on-pull branch from 28ec068 to 76a1498 Compare August 25, 2016 17:42

mtrmac merged commit 821f938 into containers:master Aug 25, 2016

mtrmac deleted the verify-on-pull branch August 25, 2016 18:02

mtrmac mentioned this pull request Sep 1, 2016

avoid policy.json check? #181

Closed

github-actions bot added the locked - please file new issue/PR label Oct 9, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2023

Verify signatures on pull #157

Verify signatures on pull #157

Uh oh!

Conversation

mtrmac commented Aug 10, 2016

Uh oh!

mtrmac commented Aug 10, 2016

Uh oh!

lsm5 commented Aug 11, 2016

sure thing, thanks for the headsup

Uh oh!

runcom commented Aug 25, 2016

Uh oh!

mtrmac commented Aug 25, 2016

Uh oh!

runcom commented Aug 25, 2016

Uh oh!

rhatdan commented Aug 25, 2016

Uh oh!

runcom commented Aug 25, 2016

Uh oh!

mtrmac commented Aug 25, 2016

Uh oh!

mtrmac commented Aug 25, 2016

Uh oh!

runcom Aug 25, 2016

Choose a reason for hiding this comment

Uh oh!

runcom Aug 25, 2016

Choose a reason for hiding this comment

Uh oh!

mtrmac Aug 25, 2016

Choose a reason for hiding this comment

Uh oh!

runcom Aug 25, 2016

Choose a reason for hiding this comment

Uh oh!

runcom commented Aug 25, 2016

Uh oh!

mtrmac commented Aug 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants