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.

Sign up for GitHub

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

conversion: mention the potential risks of blindly copying annotations #1061

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

conversion: mention the potential risks of blindly copying annotations #1061

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions conversion.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,10 @@ If there is a conflict (same key but different value) between an implicit annota

A converter MAY add annotations which have keys not specified in the image.
A converter MUST NOT modify the values of annotations specified in the image.

Note there is a risk that some annotations might be used by container runtimes to do operations that pose a security risk (such as running container hooks on the host or modifying security-related aspects of the container configuration).
As with any runtime specification configuration, generators SHOULD verify that the generated container configuration is safe before it is used to create a container.
Copy link
Member

@AkihiroSuda AkihiroSuda May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Verification is practically impossible with the current engine implementations.
Probably, runtimes have to return the list of unsafe annotation prefixes (org.systemd.*, run.oci.*) in https://github.com/opencontainers/runtime-spec/blob/main/features.md

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree being able to get a list of possibly dangerous annotation prefixes is a good idea (after all, using annotations to modify security properties is something the runtime-spec is currently silent on and arguably is outside of its security model), though I think we should simply say here that converters MAY make use of that information.

A converter MAY choose to not include annotations specified in the image, if the annotations are considered to be unsafe by the converter's security policy.
cyphar marked this conversation as resolved.
Show resolved Hide resolved
If a converter does omit annotations during the conversion, it SHOULD provide feedback to the user to indicate that an annotation has not been converted.

**Implementor's Note:** Some implementations (such as Docker/Moby) are known to indiscriminately and silently exclude all annotations specified in the image. Implementations SHOULD NOT do this.
Copy link
Member

Choose a reason for hiding this comment

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

“SHOULD NOT do this” sounds controversial.
Practically, Docker/Moby, Podman, containerd, CRI-O, etc. will have to continue doing this permanently, as any runtime annotation can be unsafe (unless we officially have the denylist)

Copy link
Member

Choose a reason for hiding this comment

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

This can be eventually a solution

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that this is controversial; honestly my favorite feature of annotations (over labels, for example) is that they are NOT inherited (either during build or during run), so this sounds like a misfeature to me 😕 don't most runtimes still keep track of which image a container was started from such that the image annotations could be queried if desired?

As a user, if any annotations were actually necessary for the correct operation of a given container, I would want that to be opt-in at the very most, especially if they cause the runtime behavior to change. 🙈

Copy link
Member Author

@cyphar cyphar May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The specification text explicitly says that this behaviour is a SHOULD (with some parts being a MUST). Any reasonable reading of the text would lead to the conclusion that dropping all labels is something one SHOULD NOT do. I didn't want to mention Docker because they don't support this part of the spec anyway, but if we have to mention them it makes very little sense to me to have a specification that reads:

Implementations SHOULD xyz.
Implementations MUST abc.
Implementations MUST NOT foo.

Implementor's note: Docker doesn't do any of this (so feel free to ignore it).

If you want to remove this feature, we can have a discussion about that, but de-facto saying this part of the specification can be safely ignored seems underhanded.

And yes, I did write this in a combative manner because an implementor note saying that a "popular implementation" (though it did take 6 years for it to actually be implemented, during which time umoci has arguably been the most popular OCI image-spec implementation) has not implemented the spec doesn't seem like something that should be tacitly endorsed.

Agreed that this is controversial; honestly my favorite feature of annotations (over labels, for example) is that they are NOT inherited (either during build or during run), so this sounds like a misfeature to me

This feature was already merged into the spec a while ago. I'm not quite clear what you mean by inheritance -- this text is talking about image labels being added to the runtime-spec annotations. Descriptor annotations are very explicitly not copied into the runtime-spec annotations, this entire text is about image labels being copied into runtime-spec annotations. This allows you to set an image label like com.nvidia.gpu.cores=4 on an image which a runtime could use to make decisions or adjust resource allocation in a way the runtime-spec does not allow.

don't most runtimes still keep track of which image a container was started from such that the image annotations could be queried if desired?

See above, I think we're talking about different things.

As a user, if any annotations were actually necessary for the correct operation of a given container, I would want that to be opt-in at the very most, especially if they cause the runtime behavior to change.

Sure, but @AkihiroSuda's preference was that we remove the feature entirely (or restrict it to the point that it cannot be used for generic features not mentioned by the image-spec explicitly). I do like his suggestion for having a list of potentially unsafe annotations in runc features though.

Copy link
Member

Choose a reason for hiding this comment

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

This file was created in 2016 (544e5ff), Docker implemented OCI media types in 2018 (moby/moby@c4f0515). I can't speak for the rationale for why this wasn't implemented in Docker's tooling (or later in containerd), but the reality is that it wasn't.

What I'm trying to say is that this is a pretty clear signal IMO that this whole feature should probably be considered optional from an implementor's perspective (given that an extremely popular implementation did not include it, and indeed even the one the spec was based on), so it feels wrong to call out Docker as being special or even unique here.

(I certainly wouldn't argue with removing the feature, but I'm also not exactly the target audience for it.)

Copy link
Member Author

@cyphar cyphar May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Docker doesn't implement any aspect of this conversion spec (despite being written specifically so they could implement it if they wanted with lots of flexibility). Even if you feel that copying labels into runtime-spec annotations is a misfeature, the annotation copying of ImageConfig fields is very critical for compatibility with runtimes (how else will a runtime know what the container's STOPSIGNAL is?) and Docker doesn't implement that either -- I don't think it's reasonable to say that we should also de-facto make that part of this spec a no-op (without removing the feature).

In my view, Docker is not compliant with this (optional) part of the image-spec. They're free to not implement it, but IMHO that means they also don't need to be mentioned because what they do in this case is not relevant.

umoci implements this behaviour as described in the spec.

Copy link
Member

Choose a reason for hiding this comment

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

I am also not sure about this wording and require the conversion from the image annotations.

One disadvantage with this feature and the deny list approach suggested in opencontainers/runtime-spec#1202 is that we will require an additional call to the OCI runtime before we can safely feed it an OCI configuration file.

Without the conversion mechanism, we can more or less safely use the same config.json with different runtimes. Using the conversion mechanism instead, we need to be careful and filter out the unsafe annotations before we can run the container.

What do you think instead about having an explicit list of domains allowed for the conversion mechanism and expecting the runtime to not use them for anything security-sensitive?

Comment on lines 130 to +137
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move up the omit section to avoid confusion with the line that says "MUST NOT modify". And to avoid pointing fingers, here's an alternate wording of the implementor's note.

Suggested change
A converter MUST NOT modify the values of annotations specified in the image.
Note there is a risk that some annotations might be used by container runtimes to do operations that pose a security risk (such as running container hooks on the host or modifying security-related aspects of the container configuration).
As with any runtime specification configuration, generators SHOULD verify that the generated container configuration is safe before it is used to create a container.
A converter MAY choose to not include annotations specified in the image, if the annotations are considered to be unsafe by the converter's security policy.
If a converter does omit annotations during the conversion, it SHOULD provide feedback to the user to indicate that an annotation has not been converted.
**Implementor's Note:** Some implementations (such as Docker/Moby) are known to indiscriminately and silently exclude all annotations specified in the image. Implementations SHOULD NOT do this.
A converter MAY omit, but MUST NOT modify, the values of annotations specified in the image.
If a converter does omit annotations during the conversion, it SHOULD provide feedback to the user to indicate that an annotation has not been converted.
Note there is a risk that some annotations might be used by container runtimes to do operations that pose a security risk (such as running container hooks on the host or modifying security-related aspects of the container configuration).
As with any runtime specification configuration, generators SHOULD verify that the generated container configuration is safe before it is used to create a container.
**Implementor's Note:** Many implementations exclude all annotations specified in the image.
For portability, dependencies on annotation conversion should be avoided.

Copy link
Member Author

Choose a reason for hiding this comment

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

"extract" means to copy the annotation in the same way that Config.Labels are copied into runtime-spec annotations. The point is that we only copy labels, not descriptor (or any other) annotations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused how that interpretation of "extract" doesn't conflict with the next line:

If there is a conflict (same key but different value) between an implicit annotation (or annotation in manifests or image indices) and an explicitly specified annotation in Config.Labels, the value specified in Config.Labels MUST take precedence.

One line is saying "don't use it" while the next says "here's the precedence".

Copy link
Member Author

@cyphar cyphar May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The first requirement is that they SHOULD NOT copy said annotations, but if they decide to copy those other annotations, labels MUST take precedence. (I forgot I made this a SHOULD NOT rather than a MUST NOT. The logic is that a runtime can set any other custom annotations as long as the Config.Label and other Config annotations take precedence, so it doesn't make sense to say you MUST NOT copy these because they have the right to set them anyway if they treat them as "defaults".)