Skip to content
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 the design document #1

Merged
merged 1 commit into from
Feb 10, 2022
Merged

Add the design document #1

merged 1 commit into from
Feb 10, 2022

Conversation

arronwy
Copy link
Member

@arronwy arronwy commented Nov 11, 2021

Co-authored-by: Liu Jiang [email protected]
Signed-off-by: Arron Wang [email protected]

@stevenhorsman
Copy link
Member

I think this looks like a great start. One thing I expected, but didn't spot (it might be included as part of the OCI spec) was the support for pulling images from authenticated registries via username:password / API key as I think that is one of the core features.

@jiangliu
Copy link
Member

I think this looks like a great start. One thing I expected, but didn't spot (it might be included as part of the OCI spec) was the support for pulling images from authenticated registries via username:password / API key as I think that is one of the core features.

Thanks for review, pulling images with authentication should be covered by OCI distribution spec:)

Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a few minor comments mainly about future things. I think the doc looks good.

docs/design.md Outdated Show resolved Hide resolved
docs/design.md Outdated
The `image-rs` crate supports both encrypted and unencrypted container images since
[Encrypted Container Image](https://github.com/opencontainers/artifacts/pull/15)
is mandatory for many confidential container usage scenarios. It also supports image signature verification to ensure
integrity.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some interesting policy questions here. I think from a security perspective we only want to support unencrypted containers that are verified, but should we really enforce this? How strict do we want to be and which entity should be in charge of setting the policy?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That depends on the threaten model of confidential containers. It should be configurable which types of images are supported:

  1. encrypted with signature, mandatory
  2. encrypted without signature, mandatory
  3. unencrypted with signature, configurable and should be enabled
  4. unencrypted without signature, configurable and may be disabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it's really just number 4 that we need to be very careful with. From a security perspective we shouldn't allow unencrypted and unsigned containers at all, but from a usability perspective those are exactly the container images that most people are probably using now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep in mind that this potentially applies not just to containers, but to the individual layers of a container. As in we will definitely want to support container images where some layers are encrypted and others aren't. If we verify the signature of the entire image this should be safe. I'm not sure if there is any reason to have per-layer signatures.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fitzthum , security policy part are updated, pls review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

docs/design.md Outdated Show resolved Hide resolved

### Non-Goals
* Image push, content discovery/management operations for remote registries
* Image encryption and signing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that everything involving provisioning an image is a non-goal that would bloat image-rs but let's make sure that we provide good documentation about how a user can setup images using other tools. image-rs isn't very useful if people can't easily upload encrypted/signed images.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe a "Howto" doc helps here:)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooi, is it not in scope as there are already existing tools to encrypt+sign or is that we don't want to bloat the binary? If it's the latter, we can simply build two binaries from the same source (one to encrypt/sign the other to decrypt/check) to avoid any packaging bloat.

If this project supported encrypting/signing, that would make writing unit tests easier. But at an integration level, the CI should encrypt with other tooling too! ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This project will leverage ocicrypt-rs to do decryption and image signing verification, and we may focuse integration test for compatiable with existing container build tools like skopeo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We hope to focus on the most valuable features at the first stage:)
Let's get something usable then we could extend it on demand:)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @arronwy and @jiangliu here, image distribution and building should be kept out of scope for now and that should be very clear.

To make it even clearer, we could indeed describe how to do those steps with other tools and libraries.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we added the document for generate test data in another PR which will describe how to do those steps, and will link them when related PR are merged.

docs/design.md Show resolved Hide resolved
@jiangliu
Copy link
Member

I think this looks like a great start. One thing I expected, but didn't spot (it might be included as part of the OCI spec) was the support for pulling images from authenticated registries via username:password / API key as I think that is one of the core features.

How about this?
"Support pulling OCI/docker v2 images from registries with or without authentication."

docs/design.md Outdated Show resolved Hide resolved
docs/design.md Outdated
Comment on lines 70 to 90
if needed. After image is unpacked, the image format module will handle
the format conversion between oci v1 and docker v2.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is local caching relevant at all?
Or is it the case that after everything is decrypted and unpacked the original blobs will be destroyed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, we are not very clear about this yet:(
For most cases, we do not care about local caching. But not sure whether it will be needed for image data sharing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caching might be good if you have more than one of the same container inside a pod. We definitely don't want to download a container more than once if it can be avoided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If caching is to be supported, is additional security needed for it? And how is it managed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should support the case where an image is shared by multiple containers within a pod.
On the other handle, it's challenge to share container images among pods, even not support by current hardware yet.
So let's leave the hard part to the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, to be clear: We're talking about caching inside the guest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will add this info.

Copy link
Member

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @arronwy. A few comments...

docs/design.md Show resolved Hide resolved
docs/design.md Outdated Show resolved Hide resolved
docs/design.md Outdated Show resolved Hide resolved
Comment on lines +24 to +33
1. encrypted with signature, mandatory
2. encrypted without signature, mandatory
3. unencrypted with signature, configurable and should be enabled
4. unencrypted without signature, configurable and may be disabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be quicker to understand if it was formatted as a table.

Image type Image Signature? Layer encryption? ...

Also, is this scenario supported: a signed image that contains encrypted layers and unencrypted layers? (I think this should be stated clearly, maybe using the table above).

tbh, I'm wondering if we need an additional term since "encrypted image" doesn't specify if the image is signed. How about:

  • secured image: a signed image with all its levels encrypted.

Other questions:

  • What forms of signature are supported?
  • What forms of encryption are supported?
  • Can an image contain multiple encrypted layers where each layer uses a different encryption scheme?
  • What happens if an image layer fails to decrypt? Presumably, it is "invalid", but this should be stated.
  • What happens if a signing key is revoked or expired? Again, I think this should be stated explicitly.
  • Shouldn't "the whole system" require secure NTP (NTS) to stop the possibility of the host changing its clock which could affect the ability (or the validity) of signature verification? Plus Secure DNS (DoH or DoT)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For image encryption opencontainers/artifacts#15 this PR describe the details on encrypted mediatype definitions for oci image spec.
For image signning, we still don't have detail definition yet, it is next step work for CC and upstream also have a POC: kata-containers/kata-containers#3023 and issue in attestation agent to support image signning verification: https://github.com/confidential-containers/attestation-agent/issues/24

docs/design.md Show resolved Hide resolved
docs/design.md Outdated Show resolved Hide resolved

### Non-Goals
* Image push, content discovery/management operations for remote registries
* Image encryption and signing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooi, is it not in scope as there are already existing tools to encrypt+sign or is that we don't want to bloat the binary? If it's the latter, we can simply build two binaries from the same source (one to encrypt/sign the other to decrypt/check) to avoid any packaging bloat.

If this project supported encrypting/signing, that would make writing unit tests easier. But at an integration level, the CI should encrypt with other tooling too! ;)

docs/design.md Outdated Show resolved Hide resolved
docs/design.md Outdated
Comment on lines 70 to 90
if needed. After image is unpacked, the image format module will handle
the format conversion between oci v1 and docker v2.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If caching is to be supported, is additional security needed for it? And how is it managed?

@arronwy arronwy mentioned this pull request Dec 2, 2021
21 tasks
@bpradipt
Copy link
Member

@arronwy [email protected] can this be closed as the initial design doc is already merged via #3 ?

@jiangliu
Copy link
Member

jiangliu commented Feb 9, 2022

@arronwy [email protected] can this be closed as the initial design doc is already merged via #253 ?

It's a different doc:)
This one should also be merged.

docs/design.md Show resolved Hide resolved
docs/design.md Outdated Show resolved Hide resolved
docs/design.md Outdated Show resolved Hide resolved
docs/design.md Outdated Show resolved Hide resolved

### Non-Goals
* Image push, content discovery/management operations for remote registries
* Image encryption and signing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @arronwy and @jiangliu here, image distribution and building should be kept out of scope for now and that should be very clear.

To make it even clearer, we could indeed describe how to do those steps with other tools and libraries.

docs/design.md Outdated
Comment on lines 70 to 90
if needed. After image is unpacked, the image format module will handle
the format conversion between oci v1 and docker v2.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, to be clear: We're talking about caching inside the guest.

docs/design.md Outdated
After container image is downloaded, `image-rs` will talk with remote
attestation service to verify the image signature and decrypt the image
if needed. After image is unpacked, the image format module will handle
the format conversion between oci v1 and docker v2.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to convert between oci v1 and docker v2?

docs/design.md Outdated Show resolved Hide resolved
docs/design.md Outdated Show resolved Hide resolved
docs/design.md Outdated Show resolved Hide resolved
Co-authored-by: Liu Jiang <[email protected]>
Signed-off-by: Arron Wang <[email protected]>
@sameo
Copy link
Member

sameo commented Feb 10, 2022

@fitzthum @jodh-intel Are you ok with that PR?

Copy link
Member

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @arronwy and @jiangliu.

lgtm

@sameo sameo merged commit 8106e1a into confidential-containers:main Feb 10, 2022
eldios added a commit to switchboard-xyz/guest-components that referenced this pull request Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants