Image encryption/decryption support in skopeo #732

harche · 2019-10-01T19:26:07Z

Add support for image encryption/decryption in skopeo

For this to work, containers/image needs to support encrypted images. There is a PR by @lumjjb to add the support in containers/image, containers/image#673. As soon as that PR is merged and is vendored into skopeo I will update this PR to use vendored containers/image.

Signed-off-by: Harshal Patil harshal.patil@in.ibm.com
Signed-off-by: Brandon Lum lumjjb@gmail.com

cmd/skopeo/copy.go

vrothberg

Just a few nits and comments. Overall, LGTM 👍

Since it's not yet in the OCI spec and potentially subject to change, I prefer marking the {de,en}cryption supports in the man pages and the help message as experimental to avoid users from depending on it.
@mtrmac @mrunalp @rhatdan WDYT?

@TomSweeneyRedHat, could you have a look? Your doc-reviewing skills would be greatly appreciated :)

vrothberg · 2019-10-07T09:10:12Z

docs/skopeo-copy.1.md

+openssl genrsa -out private.key 1024
+openssl rsa -in private.key -pubout > public.key
+
+./skopeo copy --encryption-key jwe:./public.key oci:local_nginx:latest oci:try-encrypt:enc


Can we name the destination oci:local_nginx:encrypted? I think this will highlight the relationship with the source a bit more. Same for the commands below.

vrothberg · 2019-10-07T09:11:07Z

docs/skopeo-copy.1.md

@@ -80,6 +84,25 @@ To copy and sign an image:
 $ skopeo copy --sign-by dev@example.com atomic:example/busybox:streaming atomic:example/busybox:gold
 ```

+To encrypt an image:
+```sh
+./skopeo copy docker://docker.io/library/nginx:latest oci:local_nginx:latest


s/\.skopeo/\$ skopeo/ same for the commands below (including openssl).

vrothberg · 2019-10-07T09:16:58Z

integration/copy_test.go

+ defer os.RemoveAll(dir3)
+ dir4, err := ioutil.TempDir("", "copy-4")
+ c.Assert(err, check.IsNil)
+ defer os.RemoveAll(dir4)


Can we name those variables as something more descriptive? E.g., dir1 as unencryptedDir? It make the tests easier to understand and ultimately easier to maintain in the future.

vrothberg · 2019-10-07T09:17:47Z

docs/skopeo-copy.1.md

@@ -30,6 +30,10 @@ If the authorization state is not found there, $HOME/.docker/config.json is chec

 **--sign-by=**_key-id_ add a signature using that key ID for an image name corresponding to _destination-image_

+**--encryption-key** _Key_ a reference prefixed with the encryption protocol to use. e.g. jwe:/path/to/key.pem or pgp:admin@example.com or pkcs7:/path/to/x509-file


e.g. -> For instance,.

Which protocols are supported? Are there any other requirements? We should add a sentence (or two) about that.

vrothberg · 2019-10-07T09:25:09Z

I added a WIP - to the title to prevent us from accidentally merging the c/image fork. We first need to merge containers/image#673 and can then merge this PR here.

mtrmac

(This is a full review, apart from the actual ocicrypt design/implementation. Is anyone going to review that, or are we just going to accept that on faith?)

mtrmac · 2019-10-07T19:52:52Z

cmd/skopeo/copy.go

+ encryptionKeys := opts.encryptionKeys.Value()
+ ecc, err := enchelpers.CreateCryptoConfig(encryptionKeys, []string{})
+ if err != nil {
+ return err


Please wrap almost all errors at the top level in fmt.Errorf that add end-user context, e.g. in this case making it clear that it’s the --encryption-key option list that is invalid.

While fixing I realized that if the user specifies both --encryption-key and --decryption-key it won't be possible to tell which one is invalid. So I moved that check outside in a separate if condition.

mtrmac · 2019-10-07T19:56:14Z

cmd/skopeo/copy.go

+ var encLayers *[]int
+ if len(ccs) > 0 {
+ cc := encconfig.CombineCryptoConfigs(ccs)
+ if len(opts.decryptionKeys.Value()) > 0 {


Does this need to be in two stages checking opts.decryptionKeys.Value() twice? AFAICS CombineCryptoConfigs only combines encryption configs with encryption configs, and decryption configs with decryption configs. Actually because it checks opts.decryptionKeys.Value, and not cc.EncryptConfig, it seems clear that CombineCryptoConfigs can’t affect do any encryption-decryption interactions without breaking this logic anyway.

(Why have encryption and decryption in a single config type at all, if they are always segregated by use?)

mtrmac · 2019-10-07T20:08:21Z

Since it's not yet in the OCI spec and potentially subject to change, I prefer marking the {de,en}cryption supports in the man pages and the help message as experimental to avoid users from depending on it.
@mtrmac @mrunalp @rhatdan WDYT?

Yeah, there’s the dependency cycle between only releasing a final version of the code after a final version of an interoperable specification, and only releasing a final version of an interoperable specification after there is a production implementation (or two?).

At this point I think making it as experimental is appropriate, as well as being careful about implying any endorsement by OCI (which this PR does not do).

lumjjb · 2019-10-08T13:26:21Z

apart from the actual ocicrypt design/implementation.

@mtrmac there are some reviews on this but they are not part of the current repo since it was moved. "Archived" discussions on the ocicrypt library can be found in the PRs listed below. There were quite a bit of discussion on calls as well. the discussions were between IBM, our internal NIST counterpart and Docker. In addition to that, there are some notes on the discussions in the OCI weekly dev call as well in the HackMD.

Of course, more reviews are always welcome :)

Reviews:

vrothberg · 2019-11-26T09:02:01Z

The c/image PR is merged now, so we can vendor c/image directly. The CI is currently failing with the vendor checks as we're now enforcing go 1.13. This version of golang behaves slightly different and removes some unneeded files from ./vendor. I'll open a PR to add a make vendor-in-container target.

If you don't have 1.13 installed, you can use a container:

$ podman run --privileged --rm --env HOME=/root -v `pwd`:/src -w /src docker.io/library/golang:1.13 make vendor

harche · 2019-11-26T09:21:28Z

@vrothberg I just pushed by updating vendor with go 1.13.4.

About directly importing c/image, right now in go.mod we refer to the specific release of the c/image. So IMO we will have to wait for the next release of c/image unless we are OK with replacing github.com/containers/image/v5 v5.0.0 with github.com/containers/image/v5 502848a1358b079fe809f00b14df01dfd00192eb in skopeo's go.mod.

vrothberg · 2019-11-26T09:25:07Z

go.mod

@@ -21,3 +22,5 @@ require (
 github.com/urfave/cli v1.22.1
 go4.org v0.0.0-20190218023631-ce4c26f7be8e // indirect
 )
+
+replace github.com/containers/image/v5 => github.com/lumjjb/image/v5 v5.0.0-20191125184705-a298da5c535d


Please drop the replace here and use commit 502848a1358b for vendoring c/image/v5.

vrothberg · 2019-11-26T09:27:04Z

@vrothberg I just pushed by updating vendor with go 1.13.4.

About directly importing c/image, right now in go.mod we refer to the specific release of the c/image. So IMO we will have to wait for the next release of c/image unless we are OK with replacing github.com/containers/image/v5 v5.0.0 with github.com/containers/image/v5 502848a1358b079fe809f00b14df01dfd00192eb in skopeo's go.mod.

We need to vendor commit 502848a1358b from c/image as we don't want to depend on a private fork of c/image but to always use the latest required commit. Once we cut a new release for skopeo, we should vendor a release of c/image though.

harche · 2019-11-26T09:31:46Z

@vrothberg I just pushed by updating vendor with go 1.13.4.
About directly importing c/image, right now in go.mod we refer to the specific release of the c/image. So IMO we will have to wait for the next release of c/image unless we are OK with replacing github.com/containers/image/v5 v5.0.0 with github.com/containers/image/v5 502848a1358b079fe809f00b14df01dfd00192eb in skopeo's go.mod.

We need to vendor commit 502848a1358b from c/image as we don't want to depend on a private fork of c/image but to always use the latest required commit. Once we cut a new release for skopeo, we should vendor a release of c/image though.

Done. Thanks.

Signed-off-by: Harshal Patil <harshal.patil@in.ibm.com> Signed-off-by: Brandon Lum <lumjjb@gmail.com>

vrothberg

LGTM
@rhatdan @giuseppe @TomSweeneyRedHat PTAL

giuseppe

LGTM

harche force-pushed the pr_branch branch from e09604c to 9a17dc6 Compare October 1, 2019 19:27

mtrmac reviewed Oct 1, 2019

View reviewed changes

cmd/skopeo/copy.go Outdated Show resolved Hide resolved

harche force-pushed the pr_branch branch 2 times, most recently from f51d9e0 to c1c53ee Compare October 4, 2019 13:58

vrothberg reviewed Oct 7, 2019

View reviewed changes

vrothberg changed the title ~~Image encryption/decryption support in skopeo~~ WIP - Image encryption/decryption support in skopeo Oct 7, 2019

vrothberg mentioned this pull request Oct 7, 2019

Added Container Image Encryption/Decryption support containers/image#673

Merged

mtrmac reviewed Oct 7, 2019

View reviewed changes

harche force-pushed the pr_branch branch 2 times, most recently from be068e4 to 9e4bf8b Compare October 11, 2019 12:49

lumjjb force-pushed the pr_branch branch from 9e4bf8b to 595316c Compare October 22, 2019 19:16

harche closed this Oct 23, 2019

harche deleted the pr_branch branch October 23, 2019 11:51

harche reopened this Oct 23, 2019

harche force-pushed the pr_branch branch 2 times, most recently from 2a76e2f to 3134246 Compare October 29, 2019 03:54

harche force-pushed the pr_branch branch from 3134246 to 42ba515 Compare November 11, 2019 15:09

harche force-pushed the pr_branch branch from 42ba515 to c3459a6 Compare November 21, 2019 08:33

lumjjb force-pushed the pr_branch branch from 0268af9 to e9c93eb Compare November 24, 2019 16:53

harche force-pushed the pr_branch branch from e9c93eb to b14b6dd Compare November 26, 2019 03:52

harche force-pushed the pr_branch branch from b14b6dd to 8261652 Compare November 26, 2019 09:15

vrothberg reviewed Nov 26, 2019

View reviewed changes

harche force-pushed the pr_branch branch 2 times, most recently from 9451589 to 5fbf26c Compare November 26, 2019 09:29

Image encryption/decryption support in skopeo

39ff039

Signed-off-by: Harshal Patil <harshal.patil@in.ibm.com> Signed-off-by: Brandon Lum <lumjjb@gmail.com>

harche force-pushed the pr_branch branch from 5fbf26c to 39ff039 Compare November 26, 2019 09:47

vrothberg changed the title ~~WIP - Image encryption/decryption support in skopeo~~ Image encryption/decryption support in skopeo Nov 26, 2019

vrothberg reviewed Nov 26, 2019

View reviewed changes

giuseppe approved these changes Nov 26, 2019

View reviewed changes

rhatdan merged commit ce6ec77 into containers:master Nov 26, 2019

lumjjb mentioned this pull request Dec 17, 2019

Support for image encryption and decryption containers/image#634

Closed

harche mentioned this pull request Feb 6, 2020

Partial image encryption support #814

Merged

harche mentioned this pull request Feb 21, 2020

[WIP] Adding support for encrypted container images kubernetes/kubernetes#78975

Closed

lumjjb mentioned this pull request Jul 9, 2020

Proposal: New Mediatype - Container Image Encryption opencontainers/image-spec#747

Open

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image encryption/decryption support in skopeo #732

Image encryption/decryption support in skopeo #732

harche commented Oct 1, 2019

vrothberg left a comment

vrothberg Oct 7, 2019

vrothberg Oct 7, 2019

vrothberg Oct 7, 2019

vrothberg Oct 7, 2019

vrothberg Oct 7, 2019

vrothberg commented Oct 7, 2019

mtrmac left a comment

mtrmac Oct 7, 2019

harche Oct 11, 2019

mtrmac Dec 9, 2019

mtrmac Oct 7, 2019

mtrmac commented Oct 7, 2019

lumjjb commented Oct 8, 2019 •

edited

Loading

vrothberg commented Nov 26, 2019

harche commented Nov 26, 2019

vrothberg Nov 26, 2019

vrothberg commented Nov 26, 2019

harche commented Nov 26, 2019

vrothberg left a comment

giuseppe left a comment

		@@ -30,6 +30,10 @@ If the authorization state is not found there, $HOME/.docker/config.json is chec

		--sign-by=_key-id_ add a signature using that key ID for an image name corresponding to _destination-image_

		--encryption-key _Key_ a reference prefixed with the encryption protocol to use. e.g. jwe:/path/to/key.pem or pgp:admin@example.com or pkcs7:/path/to/x509-file

Image encryption/decryption support in skopeo #732

Image encryption/decryption support in skopeo #732

Conversation

harche commented Oct 1, 2019

vrothberg left a 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

vrothberg commented Oct 7, 2019

mtrmac left a 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

mtrmac commented Oct 7, 2019

lumjjb commented Oct 8, 2019 • edited Loading

vrothberg commented Nov 26, 2019

harche commented Nov 26, 2019

Choose a reason for hiding this comment

vrothberg commented Nov 26, 2019

harche commented Nov 26, 2019

vrothberg left a comment

Choose a reason for hiding this comment

giuseppe left a comment

Choose a reason for hiding this comment

lumjjb commented Oct 8, 2019 •

edited

Loading