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 note about ImageID to the config section of manifest.md #1173

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

tianon
Copy link
Member

@tianon tianon commented Feb 29, 2024

The config digest is used by a number of popular runtimes/orchestrators as an identifier for an image with an amount of assumed uniqueness.

The `config` digest is used by a number of popular runtimes/orchestrators as an identifier for an image with an amount of assumed uniqueness.

Signed-off-by: Tianon Gravi <[email protected]>
Copy link
Member

@sajayantony sajayantony left a comment

Choose a reason for hiding this comment

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

This is a good place to emphasize and link the uniqueness of imageId.

Copy link

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

Sorry for the wall of text but I wanted to add a bit of context for my question below. There are quite a few new concepts we are introducing with the WASM artifact since it is both "runnable" and "not runnable" and some of the ideas here are new. I am also learning as I go here with lots of historical context in OCI and so might have somethings wrong.
 
If I understood correctly, this requirement for Container images is related to making sure that the configuration in mediaType.config applied matches the layers that were set up in the for the container on the file system exactly. Otherwise you could potentially apply the wrong configuration to the wrong image and do bad thingstm.

note: It was mentioned that this is a work around for a time before the config was content addressable as it is now and so might not be needed from a technical standpoint but tooling might still rely on this specifically.

We brought this question up around needing a unique configuration WASM artifact type. The current design of the config doesn't have a container "config" section on purpose to make it as portable as possible. We want to be able to run inside container runtimes but also outside container runtimes using only WASM runtimes. WASM also provides new ways to create portability through ideas like wasi-virt

In the case where it is running inside the container runtime, we want the container runtime to configure a scratch image (blank rootfs) and then the wasm runtime will run with in the namespaces set up which adds additional sandboxing. The wasm runtime will run the wasm binaries from within the "container" set up by the container runtime (but they are not included in the rootfs).

The wasm mediaType.config configuration we've defined so far is the set of import and exports that are required for the wasm binary to execute. This configuration, by definition of the wasm component model, could have different binaries that can implement config and so isn't necessarily unique. It does give enough information to validate that the binaries provided in the layers in the manifest satisfy the configuration and for the wasm runtime to determine if it can run it.

Since we really have two different "configs", in two different "runtime" scenarios, I think there is a bit of confusion. The first configuration is the "image" config from the container viewpoint and is the is the same across all wasm artifacts (a blank config and rootfs). The second config is the wasm config and manifests might have different layers that match the same mediaType.config.

Hopefully that sets up a bit of context and mostly makes sense (please correct me where I have this wrong). So my question is:

  • since the base "container image" is designed to be the same across all the wasm artifacts, do we need a unique identifier of the container configuration?
  • since there are checks to validate that the wasm binaries in the layers match the wasm configuration provided do we need a unique identifier for the configuration?

@@ -57,6 +57,8 @@ Unlike the [image index](image-index.md), which contains information about a set

To set an effectively null or empty config and maintain portability see the [guidance for an empty descriptor](#guidance-for-an-empty-descriptor) below, and `DescriptorEmptyJSON` of the reference code.

If this image manifest will be "runnable" by a runtime of some kind, it is strongly recommended to ensure it includes enough data to be unique (such as the `rootfs` and `diff_ids` included in `application/vnd.oci.image.config.v1+json`) so that it has a unique [`ImageID`](config.md#imageid).

Choose a reason for hiding this comment

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

Suggested change
If this image manifest will be "runnable" by a runtime of some kind, it is strongly recommended to ensure it includes enough data to be unique (such as the `rootfs` and `diff_ids` included in `application/vnd.oci.image.config.v1+json`) so that it has a unique [`ImageID`](config.md#imageid).
If this image manifest will be "runnable" by a runtime of some kind, it is strongly recommended to ensure the config includes enough data to be unique (an example is the `rootfs` and `diff_ids` included in `application/vnd.oci.image.config.v1+json` but doesn't need to be diff ids) so that it has a unique [`ImageID`](config.md#imageid).

@@ -57,6 +57,8 @@ Unlike the [image index](image-index.md), which contains information about a set

To set an effectively null or empty config and maintain portability see the [guidance for an empty descriptor](#guidance-for-an-empty-descriptor) below, and `DescriptorEmptyJSON` of the reference code.

If this image manifest will be "runnable" by a runtime of some kind, it is strongly recommended to ensure it includes enough data to be unique (such as the `rootfs` and `diff_ids` included in `application/vnd.oci.image.config.v1+json`) so that it has a unique [`ImageID`](config.md#imageid).
Copy link
Contributor

Choose a reason for hiding this comment

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

Some minor rewording, but otherwise LGTM. We should define "runnable" at some point, but that can be separate, perhaps with #1141.

Suggested change
If this image manifest will be "runnable" by a runtime of some kind, it is strongly recommended to ensure it includes enough data to be unique (such as the `rootfs` and `diff_ids` included in `application/vnd.oci.image.config.v1+json`) so that it has a unique [`ImageID`](config.md#imageid).
If this image manifest will be "runnable" by a runtime of some kind, it is RECOMMENDED to ensure the config includes sufficient content to be unique (such as the `rootfs` and `diff_ids` included in `application/vnd.oci.image.config.v1+json`) so that it has a unique [`ImageID`](config.md#imageid).

@sudo-bmitch
Copy link
Contributor

If I understood correctly, this requirement for Container images is related to making sure that the configuration in mediaType.config applied matches the layers that were set up in the for the container on the file system exactly. Otherwise you could potentially apply the wrong configuration to the wrong image and _do bad things_tm.

The bad things people worry about are runtimes improperly deduping images. So you may have a config for v1.2 of a WASM image being deployed by a WASM capable runtime, and try to upgrade it to v1.3 and the runtime would respond that it already has that image id and keep running the v1.2 WASM application.

That could also be used maliciously by those that want to poison the cache and potentially access data they shouldn't have access to, by creating a WASM app with a configuration matching a well known trusted image. If the malicious image is pulled first, for testing in an untrusted sandbox, and the trusted image is later requested to run with secure data, the malicious image would be run with access to the secure data.

@jsturtevant
Copy link

Thanks! Two follow up questions:

  • Since this could be a security concern and the current runtimes do in fact rely on this should this be a MUST instead of recommendation?
  • My understand was that this was a leftover from earlier days before the config was content addressable. Should this be encoded in the spec for backwards compatibility or is there a better way to represent the unique image that the spec should point to so that newer runtimes don't need to use the config as the unique Image ID?

Copy link

@endocrimes endocrimes left a comment

Choose a reason for hiding this comment

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

We actually just ran into this in the Spin project (fermyon/spin#2322).

I think it would likely be helpful if this was more of a "MUST" than a "strong recommendation" given the potential security concerns of serving the wrong content, but it's great that this is making it into the spec.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

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

LGTM

@sudo-bmitch sudo-bmitch merged commit 6a983fd into opencontainers:main Mar 7, 2024
4 checks passed
@tianon tianon deleted the note-imageid branch March 7, 2024 18:51
@tianon
Copy link
Member Author

tianon commented Mar 25, 2024

Hah, #743 is very related 🚀

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