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

tarun360 commented 1 year ago

Issue Description

Podman pull not automatically decrypting the image while pulling the images unless I explicitly provide the provider as flag --decryption-key provider:simplecrypt. This wasn't required previously with podman.

With buildah & skopeo decryption still is happening automatically without explicitly specifying the provider using flag --decryption-key provider:simplecrypt

Steps to reproduce the issue

Build a sample golang application/binary -> https://github.com/lumjjb/simple-ocicrypt-keyprovider

Configure the ocirypt.conf like below

$ cat /path/to/ocicrypt.conf
{
    "key-providers": {
        "simplecrypt": {
            "cmd": {
                "path":"/path/to/simplecrypt",
                "args": []
            }
        }
    }
}

Pull an image $ podman pull docker.io/library/alpine:latest

Encrypt this image:

$ OCICRYPT_KEYPROVIDER_CONFIG=/path/to/ocicrypt.conf podman push --encryption-key provider:simplecrypt:abc docker.io/library/alpine:latest oci-archive:encrypted

Decrypt the image without specifying the provider (fails, this is the bug):

$ OCICRYPT_KEYPROVIDER_CONFIG=/path/to/ocicrypt.conf podman pull oci-archive:encrypted
Getting image source signatures
Copying blob 5a448bb81aa6 done  
ERRO[0000] While applying layer: ApplyLayer stdout:  stderr: archive/tar: invalid tar header exit status 1 
Error: writing blob: adding layer with blob "sha256:5a448bb81aa62df09ca08c68e23f3f865e80e8b9b8a8ff3a9f20442af5297118": ApplyLayer stdout:  stderr: archive/tar: invalid tar header exit status 1

Decrypt the image with specifying the provider explicitly (works):

$ OCICRYPT_KEYPROVIDER_CONFIG=/path/to/ocicrypt.conf podman pull --decryption-key  provider:simplecrypt  oci-archive:encrypted
Getting image source signatures
Copying blob 5a448bb81aa6 done  
Copying config 4798f93a2c done  
Writing manifest to image destination
Storing signatures
4798f93a2cc876a25ef1f5ae73e7a2ff7132ddc2746fc22632a2641b318eb56c

Decrypt the image without specifying the provider using buildah (works):

$ OCICRYPT_KEYPROVIDER_CONFIG=/path/to/ocicrypt.conf buildah pull oci-archive:encrypted
Getting image source signatures
Copying blob 5a448bb81aa6 done  
Copying config 4798f93a2c done  
Writing manifest to image destination
Storing signatures
4798f93a2cc876a25ef1f5ae73e7a2ff7132ddc2746fc22632a2641b318eb56c

Describe the results you received

This is failing:

$ OCICRYPT_KEYPROVIDER_CONFIG=/path/to/ocicrypt.conf podman pull oci-archive:encrypted
Getting image source signatures
Copying blob 5a448bb81aa6 done  
ERRO[0000] While applying layer: ApplyLayer stdout:  stderr: archive/tar: invalid tar header exit status 1 
Error: writing blob: adding layer with blob "sha256:5a448bb81aa62df09ca08c68e23f3f865e80e8b9b8a8ff3a9f20442af5297118": ApplyLayer stdout:  stderr: archive/tar: invalid tar header exit status 1

Describe the results you expected

I would expect the command OCICRYPT_KEYPROVIDER_CONFIG=/path/to/ocicrypt.conf podman pull oci-archive:encrypted to work.

podman info output

$ podman info
host:
  arch: amd64
  buildahVersion: 1.29.0
  cgroupControllers: []
  cgroupManager: cgroupfs
  cgroupVersion: v1
  conmon:
    package: Unknown
    path: <erased>
    version: 'conmon version 2.1.6, commit: '
  cpuUtilization:
    idlePercent: 90.81
    systemPercent: 1.79
    userPercent: 7.4
  cpus: 56
  distribution:
    distribution: '"rhel"'
    variant: server
    version: "7.9"
  eventLogger: journald
  hostname: <erased>
  idMappings:
    gidmap:
    - container_id: 0
      host_id: 10
      size: 1
    - container_id: 1
      host_id: 1334579568
      size: 65536
    uidmap:
    - container_id: 0
      host_id: 21363
      size: 1
    - container_id: 1
      host_id: 1334579568
      size: 65536
  kernel: 3.10.0-1160.88.1.el7.x86_64
  linkmode: dynamic
  logDriver: journald
  memFree: 325461401600
  memTotal: 810750615552
  networkBackend: cni
  ociRuntime:
    name: crun
    package: Unknown
    path: <erased>
    version: |-
      crun version 1.8
      commit: 1.8
      rundir: /run/user/21363/crun
      spec: 1.0.0
      +SYSTEMD +SELINUX +APPARMOR +CAP +SECCOMP +EBPF +CRIU +YAJL
  os: linux
  remoteSocket:
    path: /run/user/21363/podman/podman.sock
  security:
    apparmorEnabled: false
    capabilities: CAP_CHOWN,CAP_DAC_OVERRIDE,CAP_FOWNER,CAP_FSETID,CAP_KILL,CAP_NET_BIND_SERVICE,CAP_SETFCAP,CAP_SETGID,CAP_SETPCAP,CAP_SETUID,CAP_SYS_CHROOT
    rootless: true
    seccompEnabled: true
    seccompProfilePath: /etc/containers/seccomp.json
    selinuxEnabled: false
  serviceIsRemote: false
  slirp4netns:
    executable: <erased>
    package: Unknown
    version: |-
      slirp4netns version 1.2.0
      commit: 656041d45cfca7a4176f6b7eed9e4fe6c11e8383
      libslirp: 4.7.0
      SLIRP_CONFIG_VERSION_MAX: 4
      libseccomp: 2.5.4
  swapFree: 2846949376
  swapTotal: 8589930496
  uptime: 318h 10m 23.00s (Approximately 13.25 days)
plugins:
  authorization: null
  log:
  - k8s-file
  - none
  - passthrough
  - journald
  network:
  - bridge
  - macvlan
  - ipvlan
  volume:
  - local
registries:
  <erased>
store:
  configFile: /u/<erased>/.config/containers/storage.conf
  containerStore:
    number: 0
    paused: 0
    running: 0
    stopped: 0
  graphDriverName: vfs
  graphOptions: {}
  graphRoot: /var/tmp/<erased>
  graphRootAllocated: 85899345920
  graphRootUsed: 12288462848
  graphStatus: {}
  imageCopyTmpDir: /var/tmp
  imageStore:
    number: 10
  runRoot: /run/user/21363/containers
  transientStore: false
  volumePath: /var/tmp/<erased>/volumes
version:
  APIVersion: 4.4.2
  Built: 315532800
  BuiltTime: Mon Dec 31 19:00:00 1979
  GitCommit: ""
  GoVersion: go1.20.1
  Os: linux
  OsArch: linux/amd64
  Version: 4.4.2

Podman in a container

No

Privileged Or Rootless

None

Upstream Latest Release

No

Additional environment details

No response

Additional information

No response

Luap99 commented 1 year ago

This wasn't required previously with podman.

If this is a regression please specify the last working version?

tarun360 commented 1 year ago

The above results are with version 4.4.2.

Before this we were using version 4.3.0, on top of which we added the patch for encryption-decryption (because this patch was only added in v4.4.0 and wasn't available in v4.3.0). With this the decryption while pulling of image was happening automatically.

Since version v4.4.2 now has the encryption-decryption feature, we now wanted to use this version, but with this we are facing the above mentioned bug.

Luap99 commented 1 year ago

@mtrmac PTAL

tarun360 commented 1 year ago

I think I found the problem. So consider two functions f1:

func DecryptConfig(decryptionKeys []string) (*encconfig.DecryptConfig, error) {
    var decryptConfig *encconfig.DecryptConfig
    if len(decryptionKeys) > 0 {
        // decryption
        dcc, err := enchelpers.CreateCryptoConfig([]string{}, decryptionKeys)
        if err != nil {
            return nil, fmt.Errorf("invalid decryption keys: %w", err)
        }
        cc := encconfig.CombineCryptoConfigs([]encconfig.CryptoConfig{dcc})
        decryptConfig = cc.DecryptConfig
    }

    return decryptConfig, nil
}

f2:

func DecryptConfig(decryptionKeys []string) (*encconfig.DecryptConfig, error) {
    decryptConfig := &encconfig.DecryptConfig{}
    if len(decryptionKeys) > 0 {
        // decryption
        dcc, err := enchelpers.CreateCryptoConfig([]string{}, decryptionKeys)
        if err != nil {
            return nil, fmt.Errorf("invalid decryption keys: %w", err)
        }
        cc := encconfig.CombineCryptoConfigs([]encconfig.CryptoConfig{dcc})
        decryptConfig = cc.DecryptConfig
    }

    return decryptConfig, nil
}

The only difference is in 2nd line of code.

f1 is used podman. f2 is used in buildah.

This small difference in these functions is cause of the discrepancy between podman & buildah's behaviour w.r.t decrypting image.

Explanation why the difference between f1 and f2 matters:

If --decryption-key flag is not passed, f1 returns nil while f2 returns decryptConfig object with empty string for decryption keys.

Now ultimately if you trace a lot of function calls, you reach at this line of code. Here:

If decryptConfig is nil (as is in case of f1 => podman), decryption is not even tried.
If decryptConfig is not nil (as is in case of f2 => buildah), then even if decryption-key string is empty, because of this init function, it still loads key-provider-config and decrypts the image successfully.

This also means we can pass just --decryption-key provider: to make sure decryptConfig is not nil (no need to specify simplecrypt), and the decryption will work.

Before this we were using version 4.3.0, on top of which we added the patch for https://github.com/containers/podman/pull/16329 (because this patch was only added in v4.4.0 and wasn't available in v4.3.0). With this the decryption while pulling of image was happening automatically.

This is incorrect from my side. We merged the above branch into v4.3.0 before this review comment by @mtrmac was resolved. So in our internal podman patch we were using f2 and not f1, and hence decryption while pulling of image was happening automatically.

mtrmac commented 1 year ago

If decryptConfig is nil (as is in case of f1 => podman), decryption is not even tried.

Thanks.

I think the above is the correct behavior: it must be possible to copy an image (e.g. using skopeo copy) just as a blob without decrypting it, so enabling decryption on every single copy is undesirable. Indiscriminately enabling decryption also has other side effects for generic copies, like preventing layer reuse.

(I’m also fairly suspicious of external environment, like environment variables, silently affecting operations — especially in a security context — although I recognize it’s sometimes very practical.)

Admittedly, for a podman pull or buildah from, unlike skopeo copy, there is no option to not decrypt: the encrypted image can’t be stored in local storage, so the only question is whether we can decrypt silently or whether we require an explicit user instruction. Still, I think it’s safer to be explicit.

So, if that config file can be used by passing a valid --decryption-key parameter, I think that’s what should happen. I have filed https://github.com/containers/buildah/pull/4745 to make Buildah behave similarly.

tarun360 commented 1 year ago

so the only question is whether we can decrypt silently or whether we require an explicit user instruction. Still, I think it’s safer to be explicit.

So, if that config file can be used by passing a valid --decryption-key parameter, I think that’s what should happen.

Even if we use function f1, you can just pass --decryption-key provider: which will make sure that decryptConfig is not nil (we don't need to pass --decryption-key provider:simplecrypt) and the decryption will be done successfully. This is more specific than not providing --decryption-key parameter at all, but still its not exactly being specific I think?

mtrmac commented 1 year ago

I don’t know how much of that is an intentional feature of the encryption library, and how much is an unintended implementation artifact; AFAICS https://github.com/containers/ocicrypt/blob/main/docs/keyprovider.md doesn’t say and I don’t know of any other documentation.

@lumjjb WDYT?

lumjjb commented 1 year ago

as i recall, the intention is to be able to let the implementation choose and the use of nil was a mechanism to display intent. That probably was not documented well here, apologies.

Also + @stefanberger who may also have context/comments.

stefanberger commented 1 year ago

I think the issue with the keyprovider 'technology' is that the keyprovider cannot easily be determined if one was to have several ones installed. We can match gpg encryption scheme to gpg keys, jwe encryption scheme to jwk keys etc. but this is not so easily possible with the generic keyprovider.

github-actions[bot] commented 1 year ago

A friendly reminder that this issue had no activity for 30 days.

rhatdan commented 1 year ago

@lumjjb @mtrmac What should we do with this issue?

mtrmac commented 1 year ago

@rhatdan This is your own https://github.com/containers/buildah/pull/4746 + a Podman change to use that shared implementation.

containers / podman