-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add image encryption support and ctr support #3134
Add image encryption support and ctr support #3134
Conversation
a7c096e
to
afac701
Compare
Codecov Report
@@ Coverage Diff @@
## master #3134 +/- ##
==========================================
- Coverage 45.06% 44.25% -0.81%
==========================================
Files 114 122 +8
Lines 12739 13489 +750
==========================================
+ Hits 5741 5970 +229
- Misses 6131 6601 +470
- Partials 867 918 +51
Continue to review full report at Codecov.
|
bbc68f5
to
87e755f
Compare
713d72f
to
eb08e6d
Compare
docs/encryption.md
Outdated
|
||
# Example End User Usage | ||
|
||
We have added 3 commands in the [`ctr`](https://github.com/containerd/containerd/tree/master/cmd/ctr) client under the image module. They are `ctr image encrypt/decrypt/layerinfo`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enumerate the commands, rather than listing them as slashed items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed cda9d21a57c541e5b2692f1b0f8d29826fc4daec
# DIGEST PLATFORM SIZE ENCRYPTION RECIPIENTS | ||
0 sha256:3427d6934e7749d556be6881a17265c9817abc6447df80a09c8eecc465c5bfb3 linux/amd64 2206947 | ||
0 sha256:d9a094b6b49fc760501d44ae96f19284e86db0a51b979756ca8a0df4a2746c79 linux/arm/v6 2146469 | ||
1 sha256:ef87d8b3048d8f1f7af7605328f63aab078a1433116dc15738989551184d7a87 linux/arm/v6 191 jwe,pkcs7 [jwe], [pkcs7] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the recipients users or are they the same as the algo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some encryption technologies we can display the actual recipients, such as for example OpenPGP, for others we cannot but can only show how many there are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, even displaying the exact number of recipients may not always be possible.
JWE and PKCS7 work with key (files) rather than a keyring (like OpenPGP) and don't associate these keys with email addresses like OpenPGP does via its keyring.
docs/encryption.md
Outdated
|
||
## Decrypt | ||
|
||
The following command performs an decryption of the encrypted image `docker.io/library/alpine:enc` to the image tag `docker.io/library/alpine:latest`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence doesn't match the example below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! cda9d21a57c541e5b2692f1b0f8d29826fc4daec
docs/encryption.md
Outdated
// GetImageLayerInfo gets the image key Ids necessary for decrypting an image | ||
// We determine the KeyIds starting with the given OCI Decriptor, recursing to lower-level descriptors | ||
// until we get them from the layer descriptors | ||
func GetImageLayerInfo(ctx context.Context, cs content.Store, desc ocispec.Descriptor, lf *encryption.LayerFilter, layerIndex int32) ([]encryption.LayerInfo, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a PR with this? This interface doesn't make sense. First off, referencing layers by their index creates a dependency on layer ordering. Image format in the future might not have that model. Making it follow this model will make it hard to change in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the layerIndex now from this public function's signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated docs to reflect new change cda9d21a57c541e5b2692f1b0f8d29826fc4daec
images/encryption/encryption.go
Outdated
// LayerInfo holds information about an image layer | ||
type LayerInfo struct { | ||
// The Number of this layer in the sequence; starting at 0 | ||
Index uint32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the index to reference a layer won't be very portable if we change the image format. We need to make encryption scheme format portable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We merely use the index of a layer to present it to the user so he can choose which layer of the stack of layers to encrypt. We did assume that the function creating the array of LayerInfos would know the stacking order of the layers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a PR with this? This interface doesn't make sense. First off, referencing layers by their index creates a dependency on layer ordering. Image format in the future might not have that model. Making it follow this model will make it hard to change in the future.
Using the index to reference a layer won't be very portable if we change the image format. We need to make encryption scheme format portable.
Two things: don't encode things as "negative numbers". This usually falls apart over time. Also, let's reference layers by their digest.
I think we see where you're coming with in terms of the API definition for encryption. In order to address this comment, one way to do it is to make it such that:
- LayerFilter becomes a digest filter instead of a layer index filter
- The digest is taken by looking into the descriptor manifest
- The option to provide encryption for top-most layers would be done on the client side (i.e. in ctr), this would mean looking at the descriptor and finding the manifest of the top X layers and creating a layer digest filter for encryption. Reference to top most layer would be via index (-1 for topmost, -2 for 2nd top most - unless there is another idea for a layer addressing scheme which makes sense for a user trying to encrypt an image).
Does that sound about right? Let us know if this does not address the concern.
Given that, we also have some thoughts about having layer index filter be included as the API. We think that there is some merit in doing so, but we may not know certain developments in this area, so maybe the points we bring up are subject to that.
- It seems like containerd API needs to know the ordering of layers, since it is part of the OCI image spec, and is required in order to unpack an image. In that sense, it is a reasonable assumption that layer indexing is made available to containerd.
- This would reduce the number of calls to perform filters based on layers, and amount of data being transferred around. The digest is a good key to identify layers but requires some work beforehand to create a meaningful layer filter. If layer position is the only use case for layer filters, then it would make sense. But perhaps if media-type, size, etc. is another use case, then that would be a different valid perspective to use digest filters instead.
Let us know what you think, we are fine with either way as long as an end user is able to be given the ability to perform operations on not all layers, which is a valuable proposition in container image encryption over VM image encryption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevvooe from my understanding (and with the recent changes since your initial comments), the indexing/"layer offsets" are UX-only elements which allow the command line tools to offer simple selection of a layer for encrypt/decrypt. The indexes don't become "permanent record" data within any store/metadata that persists; the actual work happens by walking OCI manifests and descriptors, so unless the OCI spec changes, I don't see any issue with indexes that end up as "helpers" for the UX interface of choosing layers.
It is possible for digests to be the selector as well, but obviously are a bit more cumbersome for the user given the lack of (usual) access to layer digests within tools like docker CLI (although ctr
CLI will have a new layerinfo
command via this PR though, to promote that detail for user visibility of encryption status)
Stefan or Brandon can correct me if I got anything wrong about the "persistence" of the indexing; in looking across the code I see that actual operations seem to be (properly) walking actual OCI digests/manifests, not using layer indexes as a new way to navigate images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that the indexes are client-only but it's a model that we have avoided in containerd. Layers aren't generally exposed as a first-class concept. I know that the OCI spec does declare layers, but to have any chance of moving away from layers, we need to avoid baking them into various components. I think we did a fairly good job of abstracting those concepts by decoupling snapshots and image layers. I think we can do the same here.
"layer position" is an extremely fragile way to reference layers and it is not at all necessary to solve this problem. It seems like something as simple as this would be sufficient:
func EncryptImage(desc Descriptor, shouldEncrypt func(desc Descriptor) bool) (Descriptor, error)
The above takes a desc
pointing to the image and runs shouldEncrypt
function on each component that says whether or not it should be encrypted. The resulting image, with encrypted components, is then returned as a descriptor. It's format agnostic and allows you to select which parts are important for encryption. The resulting content pointed to by descriptor is assembled during the process. You can use indexes or whatever you want to manage it there. None of the details are exposed to the caller.
Note that the function above might need a bit more, but hopefully that can help on avoiding exposure of internal details. I think we can work from here and find something that works.
images/encryption/encryption.go
Outdated
|
||
// LayerFilter holds criteria for which layer to select | ||
type LayerFilter struct { | ||
// IDs of layers to touch; may be negative number to start from topmost layer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things: don't encode things as "negative numbers". This usually falls apart over time. Also, let's reference layers by their digest.
images/encryption/config/config.go
Outdated
// for adding recipients on an already encrypted image we need the | ||
// symmetric keys for the layers so we can wrap them with the recpient's | ||
// public key | ||
Operation int32 // currently only OperationAddRecipients is supported, if at all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably represent these a different methods, rather than trying to encode an operation code. We don't really do anything like this in containerd, so this isn't a good choice for the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by removing. We only support adding recipients.
images/encryption/config/config.go
Outdated
// symmetric keys for the layers so we can wrap them with the recpient's | ||
// public key | ||
Operation int32 // currently only OperationAddRecipients is supported, if at all | ||
Dc DecryptConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DecryptConfig
for the field name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed.
Thanks for the review @stevvooe ! Looks like most of the changes are straightforward and we will start working on those. The only comment we need some clarity on is on the layer filters. @stefanberger and I had a discussion to understand the comments there and put together a summary of our discussion. I'll put that under on of the inline comments! |
bf01bf2
to
f79f651
Compare
71a6a8d
to
cda9d21
Compare
|
cda9d21
to
1662c79
Compare
Build succeeded.
|
7ed31bf
to
854bcef
Compare
Build succeeded.
|
0c852b1
to
3fde38d
Compare
Build succeeded.
|
Build succeeded.
|
ab79dad
to
3867ddf
Compare
Build succeeded.
|
3867ddf
to
15a150c
Compare
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved some code around, tested encrypt, decrypt, and layer info
LGTM
Do we want to squash this before merge? At 53 commits over a period of months, git bisect would be troublesome for a lot of the ones that needed rebase, are not compilable, etc. |
I don't think it is worth squashing. Mostly new code so bisect isn't important here. |
Let me know if you'd like me to squash this and break down the changes into a single commit message. |
Lets squash it then get it in |
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> Signed-off-by: Brandon Lum <lumjjb@gmail.com>
Signed-off-by: Derek McGowan <derek@mcgstyle.net>
15a150c
to
dde436e
Compare
Squashed to 3 commits! Splitting up vendor imports with the crypto implementation and minor movement/changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; thanks for squashing @lumjjb
Thanks so much @dmcgowan and @estesp for following along the long journey of this gigantic PR! Am really glad to see this get merged!!! Thanks @crosbymichael , @AkihiroSuda for the reviews and comments too! |
@dmcgowan @estesp @crosbymichael @AkihiroSuda Thanks a lot for you help! |
Thanks @lumjjb and @stefanberger for your patience and perseverance, this was one of the trickier ones. |
This series of patches adds support for the asymmetric and symmetric layer encryption code as well as support for ctr's images subcommand to support encryption (encrypt) and decryption (decrypt) of images as well as the retrieval of information about the layers of an image (layerinfo). The latter shows the encryption scheme used for each layer. An extensions to the documentation is also added.