Add encryption decryption feature #16329

gupttaru · 2022-10-28T06:26:36Z

This PR adds encryption decryption functionality, just like those present in buildah and skopeo. containers/common/libimage already supports encryption-decryption of images, so its a Podman only change.

This would allow us to encrypt the image while pushing it to a registry and decrypt the image while pulling it from a registry.

The commands would be as follows:

podman push --encryption-key protocol:keyfile IMAGE
podman pull --decyrption-key protocol:keyfile IMAGE

Additionally,

podman run --decryption-key <key> IMAGE [command [arg …]]
would automatically pull the image from a registry, decrypt it using decryption key and run the container.

Signed-off-by: Tarun Gupta [email protected]

Does this PR introduce a user-facing change?

Yes.

This PR adds the flag `--encryption-key` and `--encrypt-layer` to the push command, and `--decryption-key` to pull and run command.

rhatdan · 2022-10-28T09:26:31Z

cmd/podman/common/create.go

+			"*Experimental* key needed to decrypt the image (e.g. /path/to/key.pem)",
+		)
+		_ = cmd.RegisterFlagCompletionFunc(annotationFlagName, completion.AutocompleteNone)
+


Remove this blank line to make linter happy.

rhatdan · 2022-10-28T09:29:09Z

I am fine with this.

podman build --decryption-key is already supported.

rhatdan · 2022-10-28T09:30:29Z

cmd/podman/common/create.go

+		createFlags.StringSliceVar(
+			&cf.DecryptionKeys,
+			decryptionKeysFlagName, []string{},
+			"*Experimental* key needed to decrypt the image (e.g. /path/to/key.pem)",


Why is this Experimental? It is not in Buildah?

Removed "Experimental"

rhatdan · 2022-10-28T09:31:26Z

cmd/podman/images/pull.go

@@ -107,6 +108,9 @@ func pullFlags(cmd *cobra.Command) {
 	flags.StringVar(&pullOptions.Authfile, authfileFlagName, auth.GetDefaultAuthFile(), "Path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override")
 	_ = cmd.RegisterFlagCompletionFunc(authfileFlagName, completion.AutocompleteDefault)

+	decryptionKeysFlagName := "decryption-key"
+	flags.StringSliceVar(&pullOptions.DecryptionKeys, decryptionKeysFlagName, nil, "*Experimental* key needed to decrypt the image (e.g. /path/to/key.pem)")


Remove "Experimental"

rhatdan · 2022-10-28T09:31:56Z

cmd/podman/images/push.go

@@ -121,6 +124,14 @@ func pushFlags(cmd *cobra.Command) {
 	flags.StringVar(&pushOptions.CompressionFormat, compressionFormat, "", "compression format to use")
 	_ = cmd.RegisterFlagCompletionFunc(compressionFormat, common.AutocompleteCompressionFormat)

+	encryptionKeysFlagName := "encryption-key"
+	flags.StringSliceVar(&pushOptions.EncryptionKeys, encryptionKeysFlagName, nil, "*Experimental* key with the encryption protocol to use needed to encrypt the image (e.g. jwe:/path/to/key.pem)")


Remove Experimental

rhatdan · 2022-10-28T09:32:03Z

cmd/podman/images/push.go

+	_ = cmd.RegisterFlagCompletionFunc(encryptionKeysFlagName, completion.AutocompleteDefault)
+
+	encryptLayersFlagName := "encrypt-layer"
+	flags.IntSliceVar(&pushOptions.EncryptLayers, encryptLayersFlagName, nil, "*Experimental* layers to encrypt, 0-indexed layer indices with support for negative indexing (e.g. 0 is the first layer, -1 is the last layer). If not defined, will encrypt all layers if encryption-key flag is specified")


Remove Experimental

rhatdan · 2022-10-28T09:33:54Z

Needs tests.

rhatdan · 2022-10-28T09:34:14Z

Overall @gupttaru this looks good, thanks.

gupttaru

Addressed comments by @rhatdan , and added tests

gupttaru · 2022-11-10T05:38:14Z

cmd/podman/common/create.go

+		createFlags.StringSliceVar(
+			&cf.DecryptionKeys,
+			decryptionKeysFlagName, []string{},
+			"*Experimental* key needed to decrypt the image (e.g. /path/to/key.pem)",


Removed "Experimental"

gupttaru · 2022-11-10T05:38:38Z

cmd/podman/common/create.go

+			"*Experimental* key needed to decrypt the image (e.g. /path/to/key.pem)",
+		)
+		_ = cmd.RegisterFlagCompletionFunc(annotationFlagName, completion.AutocompleteNone)
+


gupttaru · 2022-11-10T05:38:47Z

cmd/podman/images/pull.go

@@ -107,6 +108,9 @@ func pullFlags(cmd *cobra.Command) {
 	flags.StringVar(&pullOptions.Authfile, authfileFlagName, auth.GetDefaultAuthFile(), "Path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override")
 	_ = cmd.RegisterFlagCompletionFunc(authfileFlagName, completion.AutocompleteDefault)

+	decryptionKeysFlagName := "decryption-key"
+	flags.StringSliceVar(&pullOptions.DecryptionKeys, decryptionKeysFlagName, nil, "*Experimental* key needed to decrypt the image (e.g. /path/to/key.pem)")


gupttaru · 2022-11-10T05:38:51Z

cmd/podman/images/push.go

@@ -121,6 +124,14 @@ func pushFlags(cmd *cobra.Command) {
 	flags.StringVar(&pushOptions.CompressionFormat, compressionFormat, "", "compression format to use")
 	_ = cmd.RegisterFlagCompletionFunc(compressionFormat, common.AutocompleteCompressionFormat)

+	encryptionKeysFlagName := "encryption-key"
+	flags.StringSliceVar(&pushOptions.EncryptionKeys, encryptionKeysFlagName, nil, "*Experimental* key with the encryption protocol to use needed to encrypt the image (e.g. jwe:/path/to/key.pem)")


gupttaru · 2022-11-10T05:38:55Z

cmd/podman/images/push.go

+	_ = cmd.RegisterFlagCompletionFunc(encryptionKeysFlagName, completion.AutocompleteDefault)
+
+	encryptLayersFlagName := "encrypt-layer"
+	flags.IntSliceVar(&pushOptions.EncryptLayers, encryptLayersFlagName, nil, "*Experimental* layers to encrypt, 0-indexed layer indices with support for negative indexing (e.g. 0 is the first layer, -1 is the last layer). If not defined, will encrypt all layers if encryption-key flag is specified")


gupttaru · 2022-11-11T14:36:34Z

In the Cirrus CI pipeline, some of the tests related to things I haven't touched are failing:

[+1357s] [Fail] Podman search [It] podman search format json list tags 
[+1357s] /var/tmp/go/src/github.com/containers/podman/test/e2e/search_test.go:137

[+1513s] [Fail] Podman run networking [It] podman do not tamper with joined network ns interfaces 
[+1513s] /var/tmp/go/src/github.com/containers/podman/test/e2e/run_networking_test.go:834

[+1513s] [Fail] Podman create with --ip flag [It] Podman create with specified static IP has correct IP 
[+1513s] /var/tmp/go/src/github.com/containers/podman/test/e2e/create_staticip_test.go:75

[+1469s] [Fail] Podman search [It] podman search format json list tags 
[+1469s] /var/tmp/go/src/github.com/containers/podman/test/e2e/search_test.go:1

Could someone help with the above errors?

Also the tests I have added for checking the encryption-decryption feature are passing except one: podman-remote is able to pull encrypted image without providing decryption key (link to CI logs). So the following is NOT throwing an error:

# podman-remote [options] run -d --name registry -p 5000:5000 quay.io/libpod/registry:2.8 /entrypoint.sh /etc/docker/registry/config.yml
# podman-remote [options] push --encryption-key jwe:/tmp/podman_test2360113671/key.rsa.pub --tls-verify=false --remove-signatures quay.io/libpod/alpine:latest localhost:5000/my-alpine
# podman-remote [options] rmi quay.io/libpod/alpine:latest
# podman-remote [options] pull localhost:5000/my-alpine

I am unable to at the moment exactly find the reason for this. Trying to debug this at the moment.

rhatdan · 2022-11-11T15:44:26Z

First can you squash your commits

git rebase -i origin
git push --force

@mtrmac @vrothberg PTAL

mtrmac

ACK to the non-test code, based on a fairly cursory skim.

podman-remote is able to pull encrypted image without providing decryption key

That sounds like a blocker for now. It might not be a bug at all, but it does need to be understood, so that we are certain we are not publishing unencrypted images when the user asks for encrypted. (OTOH I didn’t now read any part of the tests — this is admittedly just an emphasis of the known without contributing anything new.)

gupttaru · 2022-11-14T06:02:26Z

It might not be a bug at all, but it does need to be understood, so that we are certain we are not publishing unencrypted images when the user asks for encrypted.

Most probably there might be a problem with test code (link), which might be pushing unencrypted images somehow, even though I am passing the --encryption-key flag. Though I am unable to exactly see why this problem is occurring only in podman-remote, and not podman.

Also, what could be the reason for failing of other unit-tests not related to this added feature, as mentioned above?

mtrmac · 2022-11-14T13:35:14Z

> which might be pushing unencrypted images somehow… I am unable to exactly see why this problem is occurring only in podman-remote,

I can’t see any new fields added in pkg/bindings, so my first guess is that the RPC to pass options to the remote server is simply missing in this PR. (But I’m only slightly familiar with how the remote mode works.)

Does it even make sense to support this on remote? Compare #15163 (comment) . (But if it is not supported, the request to push with encryption must fail instead of pushing without encryption.)

rhatdan · 2022-11-14T21:05:56Z

Yes you need to change pkg/api and maybe pkg/bindings to pass the option accross the protocol to the server.

gupttaru · 2022-11-15T07:43:01Z

Yes you need to change pkg/api and maybe pkg/bindings to pass the option accross the protocol to the server.

Thanks, I am not well acquainted with the podman-remote mode, so missed this.

Does it even make sense to support this on remote? Compare #15163 (comment) . (But if it is not supported, the request to push with encryption must fail instead of pushing without encryption.)

I do agree with this comment. Sending the key over a protocol would be risky. The keys are usually made available on the machine which is pushing/pulling the image, in a secure manner, using some KMS (Key Management Service) or some Secret Manager. For example: Ocicrypt keyprovider protocol allows using a KMS for providing keys in a secure manner.

Similar to other flags which are not supported on podman-remote, I think we could hide the encryption related flags for remote mode, for example as done here. If we hide the flag for remote mode, I don't think we would need any changes in pkg/api and pkg/bindings

mtrmac · 2022-11-15T14:12:12Z

I’d still like to see code in the remote client that makes sure that request fails: a field in pkg/domains/entities that is silently ignored is a time bomb otherwise.

Admittedly we apparently don’t do that in other cases, like --sign-by; we probably should. The failure case with ignoring the --encryption-key option is so much worse, though, that this is much more important to do for that option.

rhatdan · 2022-11-15T15:06:21Z

SGTM
Reject these commands in remote mode. BTW This means they would not be able to be used on MAC and Windows platforms.
You can make podman-remote throw an error when using any of these options, including --sign-by.
The man pages should reflect that these are not supported for Remote as well.

gupttaru · 2022-11-17T08:03:02Z

Admittedly we apparently don’t do that in other cases, like --sign-by; we probably should. The failure case with ignoring the --encryption-key option is so much worse

You can make podman-remote throw an error when using any of these options, including --sign-by.

Given that we need to make this change for all the options that are not supported by podman-remote (--sign-by, --encryption-key etc.) can we make that a part of a separate issue/PR? For now, I can make the encryption-key flag conform with how other flags are treated. Admittedly, currently I am not well acquainted with the remote side of Podman, and not sure how this change would look.

rhatdan · 2022-11-17T10:38:45Z

Yes we can fix the others in a different commit, but for now you have to fix the new options to not run tests in remote mode.

mtrmac · 2022-11-17T13:33:02Z

For now, I can make the encryption-key flag conform with how other flags are treated.

I think the encryption-key flag must cause an error at the API level (not just the CLI), as part of this PR. The consequences of silently ignoring it are materially worse than the others.

cmd/podman/images/push.go

docs/source/markdown/options/decryption-key.md

gupttaru · 2022-11-18T13:18:19Z

I think the encryption-key flag must cause an error at the API level (not just the CLI), as part of this PR.

Understood.

Based on my current understanding of API code of Podman, this change would involve defining a query parameter for encryption-key in Podman libpod API's PushImage function (here), and then if the encryption-key is actually passed as a query parameter by the user, then raising an error.

Likewise, defining query parameters for options encrypt-layer and decryption-key, and then raising errors from libpod's API's functions PushImage and ImagesPull.

So these would only involve changes in files pkg/api/handlers/libpod/{images_push,images_pull}.go

I couldn't think of any changes in pkg/bindings/images/{push,pull}.go

mtrmac · 2022-11-21T13:51:30Z

If you want to do all the work to add an actual query parameter to the HTTP API, send it and parse it, you might just as well hook it up right through to the actual server implementation (but, per the other discussion, I’m not sure that would actually work at all).

Instead, just modify the client binary; the PR adds fields to pkg/domain/entities.ImagePushOptions, so all consumers of that struct should either implement the new fields or reject it. E.g. https://github.com/gupttaru/podman/blob/613789a9e5eace8e005fbddd1e2ca7ac2a818ed1/pkg/domain/infra/tunnel/images.go#L242 needs to check for the new fields, and fail if they are set.

If we don’t add the field to the HTTP API, the pkg/bindings clients to the HTTP API will remain unaffected, because there would be no new field to reject.

(The package names are very confusing to me. I’m increasingly tempted to recommend renaming to pkg/http/server, pkg/http/client, pkg/engine/local, pkg/engine/remote, even with all the churn that would cause.)

gupttaru · 2022-11-22T06:16:46Z

Thanks @mtrmac for your help.

I have made the requisite changes in /pkg/domain/infra/tunnel/images.go to raise an error if the encryption or decryption related variables are set. You can see these changes here and here.

The package names are very confusing to me

I also find them quite confusing because of which I was finding it hard to understand the changes requried which you mentioned above. Would it be possible add a small README.md file for such folders to explain what they do? For example, I don't know the full-form of abi in pkg/domain/infra/abi; nor was it very clear to me that the tunnel folder would be handling the remote client related things. A small README.md (similar to what we have for pkg/bindings) would be quite helpful for future open-source collaborators I believe.

mtrmac

Thanks!

The implementation seems reasonable. (I didn’t review the tests in detail.)

I’ll leave the final approval to actual Podman maintainers.

test/e2e/common_test.go

mtrmac

So much for me reviewing…

The tests show actual failures that need fixing.

pkg/util/utils.go

Signed-off-by: Tarun1 Gupta <[email protected]>

gupttaru · 2022-11-24T11:24:23Z

Thanks @mtrmac. The tests are passing now.

rhatdan · 2022-11-28T11:29:43Z

/approve
/lgtm
Thanks @gupttaru

openshift-ci · 2022-11-28T11:29:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gupttaru, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Oct 28, 2022

gupttaru force-pushed the encryption-decryption-feature branch from e778c76 to 01ba20f Compare October 28, 2022 07:28

rhatdan reviewed Oct 28, 2022

View reviewed changes

gupttaru commented Nov 10, 2022

View reviewed changes

gupttaru force-pushed the encryption-decryption-feature branch from bd272da to c942994 Compare November 10, 2022 11:10

gupttaru requested a review from rhatdan November 11, 2022 14:36

rhatdan marked this pull request as ready for review November 11, 2022 15:44

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 11, 2022

mtrmac reviewed Nov 12, 2022

View reviewed changes

gupttaru force-pushed the encryption-decryption-feature branch from abd03e1 to c284455 Compare November 14, 2022 05:43

gupttaru force-pushed the encryption-decryption-feature branch from c284455 to a8680b6 Compare November 17, 2022 13:39

gupttaru force-pushed the encryption-decryption-feature branch from a8680b6 to 613789a Compare November 17, 2022 13:43

TomSweeneyRedHat reviewed Nov 17, 2022

View reviewed changes

cmd/podman/images/push.go Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Nov 17, 2022

View reviewed changes

docs/source/markdown/options/decryption-key.md Show resolved Hide resolved

gupttaru force-pushed the encryption-decryption-feature branch from 613789a to 788c486 Compare November 22, 2022 05:59

mtrmac reviewed Nov 23, 2022

View reviewed changes

test/e2e/common_test.go Outdated Show resolved Hide resolved

mtrmac reviewed Nov 23, 2022

View reviewed changes

pkg/util/utils.go Outdated Show resolved Hide resolved

gupttaru force-pushed the encryption-decryption-feature branch from 788c486 to 5822e39 Compare November 24, 2022 07:29

Adding encryption decryption feature

3bb9ed4

Signed-off-by: Tarun1 Gupta <[email protected]>

gupttaru force-pushed the encryption-decryption-feature branch from 5822e39 to 3bb9ed4 Compare November 24, 2022 09:56

openshift-ci bot assigned rhatdan Nov 28, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2022

openshift-merge-robot merged commit c1db4f8 into containers:main Nov 28, 2022

vrothberg mentioned this pull request Jan 19, 2023

podman pull: support decrypting images #15163

Closed

tarun360 mentioned this pull request Apr 14, 2023

Podman pull not decrypting the image unless I explicitly specify the key-provider #18196

Open

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023

Add encryption decryption feature #16329

Add encryption decryption feature #16329

Conversation

gupttaru commented Oct 28, 2022 • edited Loading

Does this PR introduce a user-facing change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Oct 28, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Oct 28, 2022

rhatdan commented Oct 28, 2022

gupttaru 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

gupttaru commented Nov 11, 2022

rhatdan commented Nov 11, 2022

mtrmac left a comment

Choose a reason for hiding this comment

gupttaru commented Nov 14, 2022

mtrmac commented Nov 14, 2022

rhatdan commented Nov 14, 2022

gupttaru commented Nov 15, 2022

mtrmac commented Nov 15, 2022

rhatdan commented Nov 15, 2022

gupttaru commented Nov 17, 2022

rhatdan commented Nov 17, 2022

mtrmac commented Nov 17, 2022

gupttaru commented Nov 18, 2022

mtrmac commented Nov 21, 2022

gupttaru commented Nov 22, 2022

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

gupttaru commented Nov 24, 2022

rhatdan commented Nov 28, 2022

openshift-ci bot commented Nov 28, 2022

gupttaru commented Oct 28, 2022 •

edited

Loading