[Merged] features-linux: Expose idmap information #1219

rata · 2023-08-21T14:27:55Z

High level container runtimes sometimes need to know if the OCI runtime supports idmap mounts or not, as the OCI runtime silently ignores unknown fields.

This means that if it doesn't support idmap mounts, a container with userns will be started, without idmap mounts, and the files created on the volumes will have a "garbage" owner/group. Furthermore, as the userns mapping is not guaranteed to be stable over time, it will be completely unusable.

Let's expose idmap support in the features subcommand, so high level container runtimes use the feature safely.

cc @giuseppe @AkihiroSuda

giuseppe

LGTM

specs-go/features/features.go

AkihiroSuda

Thanks

cyphar · 2023-08-22T08:00:01Z

features-linux.md

+
+**`mountExtensions`** (object, OPTIONAL) represents the runtime's implementation status of different mount features.
+
+* **`idmap`** (bool, OPTIONAL) represents whether the runtime supports idmap mounts using the uidMappings and gidMappings properties of the mount.


I don't think a boolean is sufficient to describe the state of idmapped mount support.

At the moment, runc only supports this for one specific case (which will be fixed by opencontainers/runc#3985). Once we fix this, runc will only support these flags for bind mounts. However, the specification allows you to set these fields for any mount, which runc doesn't currently support (because it would require reworking a lot of our mount infrastructure). I don't know if crun supports arbitrary mounts to have this flag added either (@giuseppe?). Same for youki (@utam0k?).

The problem with a boolean is that it's not clear to me whether this should be true or false in any of these cases. Does true mean that setting the idmapping flags is supported at all (even with restrictions), or that it is fully supported?

I don't know if there is a nice way to describe all of these cases. You could have a string saying which filesystem types are supported, but bind-mounts are not treated as a filesystem type by the spec... We can probably ignore the way runc currently supports idmapped mounts because I suspect nobody is going to release a runtime with the feature that is that restricted.

What about having two flags: "mountExtensions": ["idmapFixed", "idmapFlexible"]

That would work for the level of idmapped supports, but for mount types I think we will need something as well.

For instance, in runc we would need to explicitly work to add support for tmpfs because we have special handling for tmpcopyup (as well as proc, sys, cgroup -- but none of those filesystems have FS_ALLOW_IDMAP so it's not super important).

Now that I think about it, we could just have a list of supported filesystem types (the runtime could use /proc/filesystems and filter out filesystems they need special handling for, or just have a fixed list they support). For bind-mounts we might be able have idmapBind as an extension? Or we could add bind as a filesystem type...

Maybe this could be an object like:

"mountExtensions": { "idmap": { "modes": ["sameAsUserns", "arbitrary"], "filesystemTypes": ["ext4", "tmpfs", "foobarfs"], "supportsBind": true } }

wdyt?

Also we need to figure out how to deal with recursive mounts -- see #1216. If we add a separate flag for this, we will need to add an entry to this struct.

SGTM, but probably s/Userns/UserNS/, as Userns may look like Users

To be fair, the fact this is silently ignored is a pretty serious issue

What exactly is silently ignored and with which runc version(s)?

I was referring to the issue you mention in the PR description -- the new fields are ignored on older runc versions (by design) and thus you end up with misconfigured mounts.

That is exactly what I did, for this to be extensible. Can you have another look? :)

Ah, you pushed a new version since my last comment. My bad. 😅

@cyphar Also, crun is using open_tree(), mount_setattr() and move_mount(); not fsconfig, for idmap mounts.

@giuseppe Sorry, by "arbitrary mounts", I mean do you support doing idmapped mounts for non-bindmounts? (In other words, do you use fsopen/fsconfig to configure mounts and apply idmapped mounts to them? Can I create a type: tmpfs mount with an id mapping?)

no, that is not supported at the moment. idmapped mounts work only with bind mounts

IIUC what is relevant for this PR is solved. Otherwise, please feel free to re-open.

I'll keep the conversation as "unresolved" so that it's not hidden when you open the PR after it's been merged. I can open an issue to discuss additional fields we might need for users to fully understand what features are supported.

cyphar

Aside from these changes, looks good.

features-linux.md

High level container runtimes sometimes need to know if the OCI runtime supports idmap mounts or not, as the OCI runtime silently ignores unknown fields. This means that if it doesn't support idmap mounts, a container with userns will be started, without idmap mounts, and the files created on the volumes will have a "garbage" owner/group. Furthermore, as the userns mapping is not guaranteed to be stable over time, it will be completely unusable. Let's expose idmap support in the features subcommand, so high level container runtimes use the feature safely. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>

rata · 2023-08-23T13:40:26Z

@cyphar All should be solved now, PTAL

cyphar

LGTM, thanks!

AkihiroSuda · 2023-08-23T15:28:21Z

features-linux.md

+* **`idmap`** (object, OPTIONAL) represents whether the runtime supports idmap mounts using the `uidMappings` and `gidMappings` properties of the mount.
+  * **`enabled`** (bool, OPTIONAL) represents whether the runtime parses and attempts to use the `uidMappings` and `gidMappings` properties of mounts if provided.
+    Note that it is possible for runtimes to have partial implementations of id-mapped mounts support (such as only allowing mounts which have mappings matching the container's user namespace, or only allowing the id-mapped bind-mounts).
+    In such cases, runtimes MUST still set this value to `true`, to indicate that the runtime recognises the `uidMappings` and `gidMappings` properties.


How to detect whether the runtime supports arbitrary mapping?

@AkihiroSuda We can add other fields for that.

I'm not convinced it is useful, though. From the high-level container runtime, it is not that you will choose different mappings if that is supported or not. You need the mappings you need, and all you care about is that runc will not just ignore the setting and create a big mess (as files in volumes will be owned by the hostUID/GID, etc.).

If that is not supported, what you want is runc to throw an error, not discover it via features IMHO. So, I don't see why exposing that would be useful.

Knowing the support level is useful for the same reason the rest of the features subcommand is useful -- it means you don't have to trial-and-error test which features are available.

@AkihiroSuda I will open an issue where we can flesh out which extra fields we want (based on the above discussion). I felt that merging this as-is is okay, as we can discuss the details of extending it separately.

@cyphar can you cc me?

Oh, it is already open

This PR upstream added the spec for the mountExtensions feature field: opencontainers/runtime-spec#1219 This commit just implements that and updates the OCI max version implemented to the one currently used in the spec (an unreleased version). It is not clear if in the future, the version of unreleased specs will be changed to something else: opencontainers/runtime-spec#1221 But this is what is currently accepted. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>

AkihiroSuda · 2023-09-09T13:04:17Z

@cyphar Next time can we use the merge button on the GitHub web UI?
On the web UI this PR is marked as "closed" and caused a confusion to me.

cyphar · 2023-09-09T13:05:34Z

Putting "closed" in the merge commit used to not cause issues, it seems GitHub has changed something. I will avoid putting it in the future.

I prefer merging from the cli because my merge commits are signed that way.

rata requested review from AkihiroSuda, crosbymichael, cyphar, dqminh, giuseppe, hqhq, kolyshkin, mrunalp, thaJeztah, tianon, vbatts and utam0k as code owners August 21, 2023 14:27

rata force-pushed the rata/features-expose-idmap branch from acde3ce to 66f3c14 Compare August 21, 2023 14:29

giuseppe approved these changes Aug 21, 2023

View reviewed changes

AkihiroSuda added this to the v1.1.1 milestone Aug 21, 2023

AkihiroSuda reviewed Aug 21, 2023

View reviewed changes

specs-go/features/features.go Show resolved Hide resolved

AkihiroSuda approved these changes Aug 21, 2023

View reviewed changes

cyphar requested changes Aug 22, 2023

View reviewed changes

rata requested a review from cyphar August 22, 2023 09:56

rata force-pushed the rata/features-expose-idmap branch from 66f3c14 to a3ba2b0 Compare August 22, 2023 09:56

cyphar mentioned this pull request Aug 22, 2023

config: add idmap and ridmap mount options #1222

Merged

cyphar requested changes Aug 23, 2023

View reviewed changes

features-linux.md Outdated Show resolved Hide resolved

features-linux.md Outdated Show resolved Hide resolved

features-linux.md Outdated Show resolved Hide resolved

rata force-pushed the rata/features-expose-idmap branch from a3ba2b0 to f329913 Compare August 23, 2023 13:39

rata requested a review from cyphar August 23, 2023 13:40

cyphar approved these changes Aug 23, 2023

View reviewed changes

cyphar closed this in 4fec88f Aug 23, 2023

rata deleted the rata/features-expose-idmap branch August 23, 2023 13:53

AkihiroSuda reviewed Aug 23, 2023

View reviewed changes

cyphar mentioned this pull request Aug 23, 2023

features: mountExtensions: how best to represent feature support for idmap? #1223

Open

This was referenced Aug 24, 2023

features: Support mountExtensions containers/crun#1279

Merged

Check idmap mounts support before creating a container cri-o/cri-o#7262

Closed

AkihiroSuda modified the milestone: v1.1.1 Sep 9, 2023

AkihiroSuda changed the title ~~features-linux: Expose idmap information~~ [Merged] features-linux: Expose idmap information Sep 9, 2023

utam0k mentioned this pull request Jan 5, 2024

Release v1.2.0 #1242

Merged

12 tasks

rata mentioned this pull request Feb 14, 2024

runk: implement the unimplemented features kata-containers/kata-containers#4373

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged] features-linux: Expose idmap information #1219

[Merged] features-linux: Expose idmap information #1219

rata commented Aug 21, 2023 •

edited

giuseppe left a comment

AkihiroSuda left a comment

cyphar Aug 22, 2023 •

edited

AkihiroSuda Aug 22, 2023

cyphar Aug 22, 2023

cyphar Aug 22, 2023 •

edited

AkihiroSuda Aug 22, 2023

cyphar Aug 22, 2023

rata Aug 22, 2023

giuseppe Aug 23, 2023

rata Aug 23, 2023

cyphar Aug 23, 2023 •

edited

cyphar left a comment

rata commented Aug 23, 2023 •

edited

cyphar left a comment

AkihiroSuda Aug 23, 2023

rata Aug 23, 2023

cyphar Aug 23, 2023

rata Aug 24, 2023

rata Aug 24, 2023

AkihiroSuda commented Sep 9, 2023

cyphar commented Sep 9, 2023


		`mountExtensions` (object, OPTIONAL) represents the runtime's implementation status of different mount features.

		* `idmap` (bool, OPTIONAL) represents whether the runtime supports idmap mounts using the uidMappings and gidMappings properties of the mount.

[Merged] features-linux: Expose idmap information #1219

[Merged] features-linux: Expose idmap information #1219

Conversation

rata commented Aug 21, 2023 • edited

giuseppe left a comment

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

cyphar Aug 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar Aug 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar Aug 23, 2023 • edited

Choose a reason for hiding this comment

cyphar left a comment

Choose a reason for hiding this comment

rata commented Aug 23, 2023 • edited

cyphar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Sep 9, 2023

cyphar commented Sep 9, 2023

rata commented Aug 21, 2023 •

edited

cyphar Aug 22, 2023 •

edited

cyphar Aug 22, 2023 •

edited

cyphar Aug 23, 2023 •

edited

rata commented Aug 23, 2023 •

edited