Implement encrypted push and pull/bud/from using encrypted images #2271
Conversation
cmd/buildah/pull.go
Outdated
| // decryption | ||
| dcc, err := enchelpers.CreateCryptoConfig([]string{}, iopts.decryptionKeys) | ||
| if err != nil { | ||
| return fmt.Errorf("Invalid decryption keys: %v", err) |
commit.go
Outdated
| @@ -451,7 +465,22 @@ func Push(ctx context.Context, image string, dest types.ImageReference, options | |||
| systemContext.DirForceCompress = true | |||
| } | |||
| var manifestBytes []byte | |||
| if manifestBytes, err = retryCopyImage(ctx, policyContext, dest, maybeCachedSrc, dest, "push", getCopyOptions(options.Store, options.ReportWriter, nil, systemContext, options.ManifestType, options.RemoveSignatures, options.SignBy), options.MaxRetries, options.RetryDelay); err != nil { | |||
| /* | |||
There was a problem hiding this comment.
Not big fans of commented out code...
There was a problem hiding this comment.
Should have squashed my commits, my bad. I will squash current commits and then changes on top of that.
cmd/buildah/push.go
Outdated
| @@ -76,6 +80,9 @@ func init() { | |||
| flags.BoolVarP(&opts.removeSignatures, "remove-signatures", "", false, "don't copy signatures when pushing image") | |||
| flags.StringVar(&opts.signBy, "sign-by", "", "sign the image using a GPG key with the specified `FINGERPRINT`") | |||
| flags.StringVar(&opts.signaturePolicy, "signature-policy", "", "`pathname` of signature policy file (not usually used)") | |||
| flags.StringSliceVar(&opts.encryptionKeys, "encryption-key", nil, "*Experimental* key with the encryption protocol to use needed to encrypt the image (e.g. jwe:/path/to/key.pem)") | |||
There was a problem hiding this comment.
Are both flags required to push? If yes, there should be an error returned by the CLI if only one is specified.
There was a problem hiding this comment.
encryptLayers is optional. Let me add additional text to the tooltip to communicate that.
cmd/buildah/push.go
Outdated
| @@ -76,6 +80,9 @@ func init() { | |||
| flags.BoolVarP(&opts.removeSignatures, "remove-signatures", "", false, "don't copy signatures when pushing image") | |||
| flags.StringVar(&opts.signBy, "sign-by", "", "sign the image using a GPG key with the specified `FINGERPRINT`") | |||
| flags.StringVar(&opts.signaturePolicy, "signature-policy", "", "`pathname` of signature policy file (not usually used)") | |||
| flags.StringSliceVar(&opts.encryptionKeys, "encryption-key", nil, "*Experimental* key with the encryption protocol to use needed to encrypt the image (e.g. jwe:/path/to/key.pem)") | |||
| flags.IntSliceVar(&opts.encryptLayers, "encrypt-layer", nil, "*Experimental* the 0-indexed layer indices, with support for negative indexing (e.g. 0 is the first layer, -1 is the last layer)") | |||
cmd/buildah/push.go
Outdated
| encLayers = &iopts.encryptLayers | ||
| ecc, err := enchelpers.CreateCryptoConfig(iopts.encryptionKeys, []string{}) | ||
| if err != nil { | ||
| return fmt.Errorf("Invalid encryption keys: %v", err) |
pull.go
Outdated
| @@ -275,7 +280,8 @@ func pullImage(ctx context.Context, store storage.Store, srcRef types.ImageRefer | |||
| }() | |||
|
|
|||
| logrus.Debugf("copying %q to %q", transports.ImageName(srcRef), destName) | |||
| if _, err := retryCopyImage(ctx, policyContext, maybeCachedDestRef, srcRef, srcRef, "pull", getCopyOptions(store, options.ReportWriter, sc, nil, "", options.RemoveSignatures, ""), options.MaxRetries, options.RetryDelay); err != nil { | |||
| logrus.Debugf("OCI Decrypt config : %v", options.OciDecryptConfig) | |||
There was a problem hiding this comment.
Should we only output this iff there is an encrypted layer
cmd/buildah/bud.go
Outdated
| decConfig = cc.DecryptConfig | ||
| } else { | ||
| // Buildah pull should always try to decrypt image when pulled | ||
| decConfig = &encconfig.DecryptConfig{} |
There was a problem hiding this comment.
Why not make this the default and when initilizing, then eliminate the else block
|
command completions changes required. |
|
Thanks for comments @rhatdan ! Addressed them.
Forgive my ignorance.. Is there a process to do this? Or is this a manual edit of |
|
Manually edit them. Just add the options. |
|
Now I will ask for the Big pain. some tests to make sure this works. |
|
Concept looks good to me, nothing out of whack that I spotted. We will most likely need to do this for at least 'podman pull' too, maybe others as well. The build side of things should go in without a problem, but we'll need to touch up the |
|
i'm running into some weird flag issues in running the tests out of # bats ./tests/push.bat
...
✗ push
...
$ /home/vagrant/go/src/github.com/containers/buildah/tests/../buildah commit
-D
--format docker
--reference-time
/tmp/tmpdbb3572b5a974dc1a0dd361f/reference-time-file
--signature-policy
/home/vagrant/go/src/github.com/containers/buildah/tests/policy.json
working-container
scratch-image-docker
unknown flag: --format docker
[ rc=1 (** EXPECTED 0 **) ]
#/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
#| FAIL: exit code is 1; expected 0
#\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
✗ buildah push image to docker and docker registry
(from function `_prefetch' in file tests/helpers.bash, line 71,
in test file tests/push.bats, line 148)
`_prefetch busybox' failed with status 125
# [checking for: busybox]
# [restoring from cache: /tmp/buildah-image-cache.4027 / busybox]
Error: unknown flag: --root /tmp/tmp9db9842473732348fdfd7845/root --storage-driver vfs
|
|
No idea what is going on. |
|
EDIT: |
|
@lumjjb: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@rhatdan figured it out... looks like it was an outdated |
|
@lumjjb: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
3 similar comments
|
@lumjjb: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@lumjjb: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@lumjjb: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
lumjjb
force-pushed
the
enc_cont
branch
2 times, most recently
from
a7763b9
to
8aaba12
Compare
lumjjb
changed the title
|
@rhatdan @TomSweeneyRedHat Added the tests and |
lumjjb
changed the title
cmd/buildah/bud.go
Outdated
| @@ -66,6 +68,7 @@ func init() { | |||
| // BUD is a all common flags | |||
| budFlags := buildahcli.GetBudFlags(&budFlagResults) | |||
| budFlags.StringVar(&budFlagResults.Runtime, "runtime", util.Runtime(), "`path` to an alternate runtime. Use BUILDAH_RUNTIME environment variable to override.") | |||
| flags.StringSliceVar(&budFlagResults.DecryptionKeys, "decryption-key", nil, "*Experimental* key needed to decrypt the image") | |||
There was a problem hiding this comment.
Use lower case for help messages.. But I would drop "Experimental" and just leave that in the man page.
cmd/buildah/bud.go
Outdated
| // decryption | ||
| dcc, err := enchelpers.CreateCryptoConfig([]string{}, iopts.BudResults.DecryptionKeys) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "Invalid decryption keys") |
cmd/buildah/from.go
Outdated
| @@ -72,6 +75,7 @@ func init() { | |||
| flags.BoolVar(&opts.pullAlways, "pull-always", false, "pull the image even if the named image is present in store") | |||
| flags.BoolVar(&opts.pullNever, "pull-never", false, "do not pull the image, use the image present in store if available") | |||
| flags.BoolVarP(&opts.quiet, "quiet", "q", false, "don't output progress information when pulling images") | |||
| flags.StringSliceVar(&opts.decryptionKeys, "decryption-key", nil, "*experimental* key needed to decrypt the image") | |||
cmd/buildah/from.go
Outdated
| // decryption | ||
| dcc, err := enchelpers.CreateCryptoConfig([]string{}, iopts.decryptionKeys) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "Invalid decryption keys") |
cmd/buildah/pull.go
Outdated
| @@ -57,6 +60,7 @@ func init() { | |||
| flags.StringVar(&opts.creds, "creds", "", "use `[username[:password]]` for accessing the registry") | |||
| flags.BoolVarP(&opts.removeSignatures, "remove-signatures", "", false, "don't copy signatures when pulling image") | |||
| flags.StringVar(&opts.signaturePolicy, "signature-policy", "", "`pathname` of signature policy file (not usually used)") | |||
| flags.StringSliceVar(&opts.decryptionKeys, "decryption-key", nil, "*experimental* key needed to decrypt the image") | |||
cmd/buildah/push.go
Outdated
| @@ -76,6 +80,9 @@ func init() { | |||
| flags.BoolVarP(&opts.removeSignatures, "remove-signatures", "", false, "don't copy signatures when pushing image") | |||
| flags.StringVar(&opts.signBy, "sign-by", "", "sign the image using a GPG key with the specified `FINGERPRINT`") | |||
| flags.StringVar(&opts.signaturePolicy, "signature-policy", "", "`pathname` of signature policy file (not usually used)") | |||
| flags.StringSliceVar(&opts.encryptionKeys, "encryption-key", nil, "*experimental* key with the encryption protocol to use needed to encrypt the image (e.g. jwe:/path/to/key.pem)") | |||
cmd/buildah/push.go
Outdated
| @@ -76,6 +80,9 @@ func init() { | |||
| flags.BoolVarP(&opts.removeSignatures, "remove-signatures", "", false, "don't copy signatures when pushing image") | |||
| flags.StringVar(&opts.signBy, "sign-by", "", "sign the image using a GPG key with the specified `FINGERPRINT`") | |||
| flags.StringVar(&opts.signaturePolicy, "signature-policy", "", "`pathname` of signature policy file (not usually used)") | |||
| flags.StringSliceVar(&opts.encryptionKeys, "encryption-key", nil, "*experimental* key with the encryption protocol to use needed to encrypt the image (e.g. jwe:/path/to/key.pem)") | |||
| flags.IntSliceVar(&opts.encryptLayers, "encrypt-layer", nil, "*experimental* the 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") | |||
cmd/buildah/push.go
Outdated
| encLayers = &iopts.encryptLayers | ||
| ecc, err := enchelpers.CreateCryptoConfig(iopts.encryptionKeys, []string{}) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "Invalid encryption keys") |
commit.go
Outdated
| @@ -132,6 +133,19 @@ type PushOptions struct { | |||
| MaxRetries int | |||
| // RetryDelay is how long to wait before retrying a push attempt. | |||
| RetryDelay time.Duration | |||
|
|
|||
| // If OciEncryptConfig is non-nil, it indicates that an image should be encrypted. | |||
There was a problem hiding this comment.
This should start with OciEncryptConfig not If.
commit.go
Outdated
| @@ -319,7 +333,8 @@ func (b *Builder) Commit(ctx context.Context, dest types.ImageReference, options | |||
| } | |||
|
|
|||
| var manifestBytes []byte | |||
| if manifestBytes, err = retryCopyImage(ctx, policyContext, maybeCachedDest, maybeCachedSrc, dest, "push", getCopyOptions(b.store, options.ReportWriter, nil, systemContext, "", false, options.SignBy), options.MaxRetries, options.RetryDelay); err != nil { | |||
| logrus.Debugf("Calling from Commit") | |||
| if manifestBytes, err = retryCopyImage(ctx, policyContext, maybeCachedDest, maybeCachedSrc, dest, "push", getCopyOptions(b.store, options.ReportWriter, nil, systemContext, "", false, options.SignBy, nil, nil, nil), options.MaxRetries, options.RetryDelay); err != nil { | |||
There was a problem hiding this comment.
Rather then extending retryCopyImage, could you just pass in options...
There was a problem hiding this comment.
I think we're extending getCopyOptions here. Is this fine or are you thinking something like this?
options := getCopyOptions(...)
options.decryptionKey = ...
There was a problem hiding this comment.
I ready this wrong. I was thinking the copy options and options.MaxRetries, I hate functions which just keep growing their params. Is there a way we can consolidate.
There was a problem hiding this comment.
Have a couple ideas ... But seems like this would be a separate PR.
For getCopyOptions it looks like the only special case is the store... Maybe something like this? I guess it would be a different PR for this though.
initCopyOptionsWithStore(store, &cp.Options{
ReportWriter: reportWriter,
SourceCtx: sourceCtx,
DestinationCtx: destinationCtx,
ForceManifestMIMEType: manifestType,
RemoveSignatures: removeSignatures,
SignBy: addSigner,
OciEncryptConfig: ociEncryptConfig,
OciDecryptConfig: ociDecryptConfig,
OciEncryptLayers: ociEncryptLayers,
})
Consolidating the retry options
type copyRetryOptions struct{
maxRetries int
retryDelay time.Duration
}
|
@mtrmac @vrothberg @nalind PTAL |
|
getEncryptConfig is not used? |
lumjjb
force-pushed
the
enc_cont
branch
2 times, most recently
from
88c1934
to
db18a06
Compare
|
Thanks. good catch. Rebased master and squashed as well. |
lumjjb
force-pushed
the
enc_cont
branch
4 times, most recently
from
a6074a9
to
13337bb
Compare
Signed-off-by: Brandon Lum <lumjjb@gmail.com>
Signed-off-by: Brandon Lum <lumjjb@gmail.com>
Signed-off-by: Brandon Lum <lumjjb@gmail.com>
Signed-off-by: Brandon Lum <lumjjb@gmail.com>
Signed-off-by: Brandon Lum <lumjjb@gmail.com>
|
@mtrmac @TomSweeneyRedHat Addressed the comments! I am thinking that I could do another PR after this one to write something in |
| **--encryption-key** *key* | ||
|
|
||
| The [protocol:keyfile] specifies the encryption protocol, which can be JWE (RFC7516), PGP (RFC4880), and PKCS7 (RFC2315) and the key material required for image encryption. For instance, jwe:/path/to/key.pem or pgp:admin@example.com or pkcs7:/path/to/x509-file. | ||
|
|
docs/buildah-bud.md
Outdated
| @@ -178,6 +178,10 @@ The [username[:password]] to use to authenticate with the registry if required. | |||
| If one or both values are not supplied, a command line prompt will appear and the | |||
| value can be entered. The password is entered without echo. | |||
|
|
|||
| **--decryption-key** *key* | |||
There was a problem hiding this comment.
I'd suggest the following, ditto for the other man pages with decryption-key
| **--decryption-key** *key* | |
| **--decryption-key** *key[:passphrase]* |
|
One nit scattered across several of the pages, otherwise LGTM. |
Signed-off-by: Brandon Lum <lumjjb@gmail.com>
|
Fixed! |
|
Tests are still failing? |
|
Interesting, it was just passing the last commit and just doc changes... let me check it out. |
|
Ah - seems to have something to do with Quay.io being down.
|
|
bors retest |
|
Kicked the test runs again, hopefully quay.io is happy again. |
|
bors r+ |
|
Build succeeded: |
|
🎉 |
|
Awesome work @lumjjb ! |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces the ability to encrypt an OCI container image when pushing, and also allows the use of encrypted OCI container images when building a Dockerfile or pulling an encrypted image from the registry.
This is the initial implementation idea. There are a few more things that I am working on right now before taking out of DRAFT, but would like some feedback on the general idea.
TODO in progress by me:
How to verify it
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Yes
buildah pushwith--encryption-keybuildah pullwith--decryption-keybuildah budwith--decryption-key