Added Container Image Encryption/Decryption support #673

lumjjb · 2019-08-02T15:53:01Z

This PR contains a POC for image encryption/decryption. Based on encryption work as presented at KubeCon CN 2019. Currently, we have POCs for cri-o and skopeo that can consume this.

Available consumer examples:

Below is a summary of what is modified. We can squash the commits if preferred.

Types - `types/types.go`

Added SupportsEncryption as part of Image interface (backed by genericManifest)
Added CryptoConfig as part of SystemContext

Manifest Changes - `manifest/`

Added encryption mediatypes consts
Modified OCI1.UpdateLayerInfos to also update Mediatype if provided
Added SupportsEncryption into the genericManifest interface

Copy - `copy/`

In general: Added ability to encrypt/decrypt in copy.Image
Added parameter to newDigestingReader to skip checks in the case where digest is not known (i.e. during encryption, etc.)
Used CryptoConfig from SystemContext
Added EncryptConfig and EncryptLayers to copy.Options to specify encryption when calling Image
Modified imageCopier and copier constructs to propagate crypto configuration
All of the work for actual encryption/decryption of streams are handled in copyLayer, with an added toEncrypt parameter that is passed into the function.
Decryption is done on source if encrypted
Encryption is done on dest if specified in copy.Options
Note: Currently encryption supports OCI image as a source since copy.copyAndUpdateManifest applies changes to the src manifest, and docker schema1/2 lacks a way to define encryption via MediaTypes and Annotations.

mrunalp · 2019-08-13T17:08:49Z

@vrothberg @mtrmac @rhatdan ptal.

vrothberg · 2019-08-13T17:34:27Z

Hi @lumjjb, thanks for the PR! I won't find time to review this week to review but will have a look next week.

One thing I noticed while quickly skimming the code is that we add a lot of code from containerd. I wonder if we couldn't vendor in the packages from containerd directly (assuming no code changes needed)? The intention behind is to avoid redundancy and duplicate work, for instance, when fixing a regression.

mrunalp · 2019-08-13T17:37:13Z

I think such code should probably live in a library which can be imported into the CRI implementations as needed. We are happy to host it at github.com/containers if that works for you.

lumjjb · 2019-08-13T17:46:51Z

Yup - i am actually working on a commit now to remove that dependency. there are no hard dependencies on containerd. We were only using some errdefs which we didn't clean up properly. Pushing a commit now

EDIT: @vrothberg I think i see what you are talking about... the headers of the files that we had are actually no longer in containerd and will be an independent library. The containerd headers are just left over artifacts from before.

lumjjb · 2019-08-13T17:51:01Z

I think such code should probably live in a library which can be imported into the CRI implementations as needed. We are happy to host it at github.com/containers if that works for you.

yep! That sounds good. We have the code living in one of our repositories now... in https://github.com/stefanberger/imgcrypt/tree/master/pkg/encryption... Yea - i think it would be perfect if we could have one living in the containers/ path, and when we get this merged into OCI, we can decide if we should migrate it there.

mrunalp · 2019-08-13T17:57:11Z

@lumjjb sounds good 👍 I can create the repository for you if give me the desired name for the repo.

lumjjb · 2019-08-13T17:58:41Z

@lumjjb sounds good 👍 I can create the repository for you if give me the desired name for the repo.

@mrunalp Awesome, ocicrypt should be a good name! Thanks for hosting us :D

rhatdan · 2019-08-13T18:00:56Z

Yes my first response to this would be to get it into a vendor and then it would be easier to review. We don't want any exposed interfaces to this code in containers/image other then the ones we support.

mrunalp · 2019-08-13T18:02:20Z

@lumjjb I have created https://github.com/containers/ocicrypt. Who all should be added as maintainers? :)

mrunalp · 2019-08-13T18:03:53Z

@lumjjb I have given you admin rights, so you should be able to manage the repo :)

lumjjb · 2019-08-13T18:05:17Z

@mrunalp Awesome Thanks!!

mrunalp · 2019-08-13T18:05:28Z

I have added Stefan as well but now you should be able to add other people as needed. Let me know if something doesn't work.

lumjjb · 2019-08-14T14:05:28Z

Updated the PR to vendor containers/ocicrypt and re-did commits to help reviewing

copy/copy.go

image/docker_schema1.go

image/docker_schema2.go

image/manifest.go

lumjjb · 2019-08-15T15:38:44Z

Addressed @rhatdan 's comments in 25b2de6

vrothberg

Comments from the first round of review. Most comments are minor nits.

Before merging this PR, I want #563 and #639 to be merged, especially for having the fixes for correctly setting the mime types.

@lumjjb, let's wait for @mtrmac's comments before addressing mine.

@lumjjb, could you drop instructions how we can test the code? Ideally, I'd like to use Skopeo to copy an encrypted image from some registry.

copy/copy.go

vrothberg · 2019-08-19T07:48:33Z

copy/copy.go

+		}
+
+		srcInfo.Digest = d
+		srcInfo.Size = -1


Note to self: this looks like it could interfere with the progress bars.

vendor.conf

vrothberg · 2019-08-19T07:59:58Z

copy/copy.go

+	// not allow the image to be provisioned.
+	if ic.checkAuthorization {
+		if srcInfo.MediaType == manifest.DockerV2Schema2LayerGzipEncMediaType ||
+			srcInfo.MediaType == manifest.DockerV2Schema2LayerEncMediaType {


It's unclear to me how this can work. The hard-coded mime types indicate that schema 2 would support encryption but ...

// SupportsEncryption returns if encryption is supported for the manifest type func (m *manifestSchema2) SupportsEncryption(context.Context) bool { return false }

Regarding hard-coding the .MediaType. I prefer adding an Encrypted bool field to BlobInfo and let the Manifest update it, so that the individual implementations can take care of it. We do something similar in https://github.com/containers/image/pull/563/files#diff-b3c9ca9ec48c31df0619e70595f4bd62R84-R91.

I think this is a mistake on our part. We should rename DockerV2Schema2LayerGzipEncMediaType to use OCI instead.

I prefer adding an Encrypted bool field to BlobInfo and let the Manifest update it
I think this would work as well!

Kind of related to this - there is one issue that we were running into that is bugging us a little.

copyUpdatedConfigAndManifest(

image/copy/copy.go

Line 554 in 2178abd

pi, err := ic.src.UpdatedImage(ctx, *ic.manifestUpdates)

) updates the manifest of the SRC and then copies it over to DST. Which is a bit tricky here because dockerv2 doesn't support encryption, but it should not prevent encrypting a dockerv2 image into an OCI encrypted image. I was wondering if it could be done by copying SRC to DST then applying the layer updates? What are your thoughts about this?

I think this can be solved by doing something similar to https://github.com/containers/image/pull/563/files#diff-b3c9ca9ec48c31df0619e70595f4bd62R84-R91. While updating the manifests, we check each BlobInfo and update the mime types accordingly.

I think the src vs dst update would still be an issue - would you think we should move the code block from

image/copy/copy.go

Lines 541 to 559 in 2178abd

if !reflect.DeepEqual(*ic.manifestUpdates, types.ManifestUpdateOptions{InformationOnly: ic.manifestUpdates.InformationOnly}) {

if !ic.canModifyManifest {

return nil, errors.Errorf("Internal error: copy needs an updated manifest but that was known to be forbidden")

}

if !ic.diffIDsAreNeeded && ic.src.UpdatedImageNeedsLayerDiffIDs(*ic.manifestUpdates) {

// We have set ic.diffIDsAreNeeded based on the preferred MIME type returned by determineManifestConversion.

// So, this can only happen if we are trying to upload using one of the other MIME type candidates.

// Because UpdatedImageNeedsLayerDiffIDs is true only when converting from s1 to s2, this case should only arise

// when ic.c.dest.SupportedManifestMIMETypes() includes both s1 and s2, the upload using s1 failed, and we are now trying s2.

// Supposedly s2-only registries do not exist or are extremely rare, so failing with this error message is good enough for now.

// If handling such registries turns out to be necessary, we could compute ic.diffIDsAreNeeded based on the full list of manifest MIME type candidates.

return nil, errors.Errorf("Can not convert image to %s, preparing DiffIDs for this case is not supported", ic.manifestUpdates.ManifestMIMEType)

}

pi, err := ic.src.UpdatedImage(ctx, *ic.manifestUpdates)

if err != nil {

return nil, errors.Wrap(err, "Error creating an updated image manifest")

}

pendingImage = pi

}

down, and so do something like

manifest, err := ic.c.src.Manifest(...) ... if err := ic.c.dest.PutManifest(ctx, manifest); err != nil { return nil, errors.Wrap(err, "Error writing manifest") } _, err := ic.c.dest.UpdatedImage(ctx, *ic.manifestUpdates) if err != nil { return nil, errors.Wrap(err, "Error creating an updated image manifest") } manifest, err := ic.c.src.Manifest(...)

copy/copy.go

lumjjb · 2019-08-19T18:17:21Z

Generate RSA keys for testing

openssl genrsa -out private.key 1024
openssl rsa -in private.key -pubout > public.key

Skopeo (to make, we use make binary-local due to some issue with container build).

# Encrypting image
./skopeo copy --dest-tls-verify=false --dest-recipient jwe:./public.key oci:nginx_local:latest  oci:skopeo_nginx_jwe:latest

# Decrypting image
./skopeo copy --src-tls-verify=false --src-key ./private.key  oci:skopeo_nginx_jwe:latest oci:nginx_ocilayout:latest

We have a PR open to update that branch to use the new changes with the ocicrypt refactor, PR is as follows: harche/skopeo#3. It's night time IST timezone, so will be merged in tomorrow probably.

lumjjb · 2019-09-11T19:48:26Z

@vrothberg addressed the nits and most of the comments in lumjjb#4. I think we can discuss the other ones during the call tomorrow!

vrothberg · 2019-09-12T13:08:08Z

@vrothberg addressed the nits and most of the comments in lumjjb#4. I think we can discuss the other ones during the call tomorrow!

Thanks, @lumjjb! I will have a look at the code today before the meeting; looking forward to it.

I was focusing on #563 which should give you a good base for setting the new media type(s).

vrothberg

A couple of (very) minor nits.

I need to have a close look at copyBlobFromStream(...) but need a fresher brain. I'm especially interested if we can simplify setting the media types a bit more.

@lumjjb, how would we expose encryption support on the CLI (e.g., skopeo copy ...)?

vrothberg · 2019-09-12T15:56:37Z

copy/copy.go

+	// If nil, don't encrypt any layers.
+	// If non-nil and len==0, denotes encrypt all layers.
+	// integers in the slice represent 0-indexed layer indices, with support for negative
+	// indexing. i.e. 0 is the first layer, -1 is the last (top-most) layer.


Is the negative indexing a requirement or nice-to-have? It's a common source of bugs and unless needed, I'd prefer to have a positive indexing.

It's helpful in the containers images usecase, because people want to usually encrypt the top-most layers. If we can do this to the caller of copy.Image, that is fine as well, and we move the abstraction of negative indexing to the tool.

copy/copy.go

vrothberg · 2019-09-12T16:15:14Z

copy/copy.go

@@ -117,6 +141,16 @@ type Options struct {
 	Progress         chan types.ProgressProperties // Reported to when ProgressInterval has arrived for a single artifact+offset.
 	// manifest MIME type of image set by user. "" is default and means use the autodetection to the the manifest MIME type
 	ForceManifestMIMEType string
+	CheckAuthorization    bool


Can we add a description for it?

Maybe we can make it a member of encconfig.EncryptConfig?

Adding a description for CheckAuthorization.

The use of CheckAuthorization is a concept only for runtimes, so we abstracted it away from the main encryption library config. The main reason it's there is due to the implementation of caching in the runtime instead of something that is spec related. The runtimes use the higher level interface, which directly takes in a bool for CheckAuthorization (called unwrapOnly):

https://github.com/containers/ocicrypt/blob/master/encryption.go#L174

Extra info if you are curious about why encconfig.CryptoConfig is a public interface:

A little segway about the design of the encryption library. There are 2 layers of it, the low level layer only deals with crypto - nothing about OCI or container images. This has interfaces for crypto primitives. This is the main consumer of encconfig.CryptoConfig.

I think we will add a note to the file. The encconfig.* is package is designed for extensibility for crypto primitives - for better crypto agility.

https://github.com/containers/ocicrypt/blob/master/keywrap/keywrap.go#L25

The runtimes use the higher level interface, which directly takes in a bool for CheckAuthorization:

https://github.com/containers/ocicrypt/blob/master/encryption.go#L174

Adding the comment:

// CheckAuthorization is an indication if the image does not need to be decrypted, but only // requires checking if the it can be decrypted. // This is an optimization for cached images to see if provided keys have the authorization // to use the cached content without having to decrypt the layers again.

copy/copy.go

mtrmac · 2019-11-05T11:49:35Z

copy/copy.go

@@ -257,6 +297,10 @@ func (c *copier) copyOneImage(ctx context.Context, policyContext *signature.Poli
 		return nil, errors.Wrapf(err, "Error initializing image from source %s", transports.ImageName(c.rawSource.Reference()))
 	}

+	if options.EncryptLayers != nil && !src.SupportsEncryption(ctx) {


Is the “TODO: Remove src.SupportsEncryption call…” item supposed to stay there, or should that already have been removed?

mtrmac

Just one random implementation comment for now.

lumjjb · 2019-11-07T14:08:42Z

@mtrmac Updated with the changes we talked about during the call. - I think there's somethign we can do with the blob info cache, but I think it would be practically useful only when we get to cri-o, so we can look at that when we look at cri-o and KEP integration.

lumjjb · 2019-11-22T20:50:39Z

rebased

lumjjb · 2019-11-24T16:41:00Z

rebased

lumjjb · 2019-11-24T17:53:53Z

@mrunalp @rhatdan As per discussions. PR is up to date and ready to merge!

Testing has been configured in containers/skopeo#732 as CopySuite.TestCopyEncryption , which we can merge right after this (CI passing).

In addition, several manual tests have also been carried out for additional verification: https://gist.github.com/lumjjb/bda1faf8086a57c1a9261d951b9ddb6a

The outstanding discussions are documented in:

Re-design BlobInfo *Operation #755: Re-design BlobInfo *Operation
Have copyUpdatedConfigAndManifest update not rely on src manifest restrictions #746: Have copyUpdatedConfigAndManifest update not rely on src manifest restrictions

rhatdan · 2019-11-25T16:29:27Z

Looks like we have one more rebase...

Signed-off-by: Brandon Lum <[email protected]>

lumjjb · 2019-11-25T18:48:22Z

rebased and poked CI, seems to be stuck on "receiving job"

lumjjb · 2019-11-26T03:59:55Z

Looks like we have one more rebase...

Rebased and CI ran success!

vrothberg · 2019-11-26T08:58:16Z

As discussed, merging now. Thanks @lumjjb for this great work!

lumjjb · 2019-11-26T14:20:12Z

Awesome! Thank you! 👍 👍 👍

Both the referenced features were dropped in the final version of containers#673 which added these comments. Signed-off-by: Miloslav Trmač <[email protected]>

lumjjb force-pushed the encryption-policy branch from 5f3231a to ccc935b Compare August 2, 2019 16:00

mrunalp requested a review from vrothberg August 5, 2019 19:46

lumjjb force-pushed the encryption-policy branch 3 times, most recently from 402841d to 38685e8 Compare August 9, 2019 18:25

lumjjb force-pushed the encryption-policy branch 4 times, most recently from fbfe8c7 to 8df5dc1 Compare August 13, 2019 20:49

rhatdan reviewed Aug 15, 2019

View reviewed changes

copy/copy.go Outdated Show resolved Hide resolved

image/docker_schema1.go Outdated Show resolved Hide resolved

image/docker_schema2.go Outdated Show resolved Hide resolved

image/manifest.go Outdated Show resolved Hide resolved

vrothberg reviewed Aug 19, 2019

View reviewed changes

lumjjb force-pushed the encryption-policy branch from 28698e7 to 3165d1b Compare September 6, 2019 13:38

vrothberg reviewed Sep 12, 2019

View reviewed changes

lumjjb force-pushed the encryption-policy branch from 5c75f09 to b333212 Compare October 28, 2019 18:07

mtrmac reviewed Nov 5, 2019

View reviewed changes

mtrmac requested changes Nov 5, 2019

View reviewed changes

lumjjb mentioned this pull request Nov 6, 2019

Have copyUpdatedConfigAndManifest update not rely on src manifest restrictions #746

Closed

lumjjb force-pushed the encryption-policy branch 2 times, most recently from 2f8ff59 to c937289 Compare November 12, 2019 04:15

lumjjb force-pushed the encryption-policy branch 2 times, most recently from 96f54f8 to 368d7cc Compare November 22, 2019 20:50

lumjjb mentioned this pull request Nov 24, 2019

Re-design BlobInfo *Operation #755

Open

lumjjb force-pushed the encryption-policy branch from 368d7cc to 53d9e53 Compare November 24, 2019 16:40

lumjjb added 4 commits November 25, 2019 11:57

Implemented image encryption

df5f162

Signed-off-by: Brandon Lum <[email protected]>

Added details on when src.SupportsEncryption can be removed

c6be71f

Signed-off-by: Brandon Lum <[email protected]>

Added comments to redesign LayerCompression/Crypto

657f918

Signed-off-by: Brandon Lum <[email protected]>

Cleaner refactoring to determineManifestConversion

688611c

Signed-off-by: Brandon Lum <[email protected]>

lumjjb force-pushed the encryption-policy branch from 53d9e53 to d978613 Compare November 25, 2019 16:58

Reverted newDigestingReader to validate blob during decryption

a298da5

Signed-off-by: Brandon Lum <[email protected]>

lumjjb force-pushed the encryption-policy branch from d978613 to a298da5 Compare November 25, 2019 18:47

vrothberg merged commit 502848a into containers:master Nov 26, 2019

lumjjb mentioned this pull request Dec 17, 2019

Support for image encryption and decryption #634

Closed

mtrmac added a commit to mtrmac/image that referenced this pull request Mar 10, 2023

Remove some inaccurate comments

2cac6eb

Both the referenced features were dropped in the final version of containers#673 which added these comments. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac mentioned this pull request Mar 10, 2023

Remove some inaccurate comments #1880

Merged

	if !reflect.DeepEqual(*ic.manifestUpdates, types.ManifestUpdateOptions{InformationOnly: ic.manifestUpdates.InformationOnly}) {
	if !ic.canModifyManifest {
	return nil, errors.Errorf("Internal error: copy needs an updated manifest but that was known to be forbidden")
	}
	if !ic.diffIDsAreNeeded && ic.src.UpdatedImageNeedsLayerDiffIDs(*ic.manifestUpdates) {
	// We have set ic.diffIDsAreNeeded based on the preferred MIME type returned by determineManifestConversion.
	// So, this can only happen if we are trying to upload using one of the other MIME type candidates.
	// Because UpdatedImageNeedsLayerDiffIDs is true only when converting from s1 to s2, this case should only arise
	// when ic.c.dest.SupportedManifestMIMETypes() includes both s1 and s2, the upload using s1 failed, and we are now trying s2.
	// Supposedly s2-only registries do not exist or are extremely rare, so failing with this error message is good enough for now.
	// If handling such registries turns out to be necessary, we could compute ic.diffIDsAreNeeded based on the full list of manifest MIME type candidates.
	return nil, errors.Errorf("Can not convert image to %s, preparing DiffIDs for this case is not supported", ic.manifestUpdates.ManifestMIMEType)
	}
	pi, err := ic.src.UpdatedImage(ctx, *ic.manifestUpdates)
	if err != nil {
	return nil, errors.Wrap(err, "Error creating an updated image manifest")
	}
	pendingImage = pi
	}

Added Container Image Encryption/Decryption support #673

Added Container Image Encryption/Decryption support #673

Conversation

lumjjb commented Aug 2, 2019 • edited Loading

Types - types/types.go

Manifest Changes - manifest/

Copy - copy/

mrunalp commented Aug 13, 2019

vrothberg commented Aug 13, 2019

mrunalp commented Aug 13, 2019

lumjjb commented Aug 13, 2019 • edited Loading

lumjjb commented Aug 13, 2019

mrunalp commented Aug 13, 2019

lumjjb commented Aug 13, 2019

rhatdan commented Aug 13, 2019

mrunalp commented Aug 13, 2019

mrunalp commented Aug 13, 2019

lumjjb commented Aug 13, 2019

mrunalp commented Aug 13, 2019

lumjjb commented Aug 14, 2019

lumjjb commented Aug 15, 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

lumjjb commented Aug 19, 2019 • edited Loading

lumjjb commented Sep 11, 2019

vrothberg commented Sep 12, 2019 • edited Loading

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

lumjjb Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

lumjjb commented Nov 7, 2019

lumjjb commented Nov 22, 2019

lumjjb commented Nov 24, 2019

lumjjb commented Nov 24, 2019 • edited Loading

rhatdan commented Nov 25, 2019

lumjjb commented Nov 25, 2019

lumjjb commented Nov 26, 2019

vrothberg commented Nov 26, 2019

lumjjb commented Nov 26, 2019

lumjjb commented Aug 2, 2019 •

edited

Loading

Types - `types/types.go`

Manifest Changes - `manifest/`

Copy - `copy/`

lumjjb commented Aug 13, 2019 •

edited

Loading

lumjjb commented Aug 19, 2019 •

edited

Loading

vrothberg commented Sep 12, 2019 •

edited

Loading

lumjjb Sep 12, 2019 •

edited

Loading

lumjjb commented Nov 24, 2019 •

edited

Loading