config.md: allow empty mappings for [r]idmap #1224

giuseppe · 2023-09-05T20:06:42Z

crun currently allows to specify an empty mapping for [r]idmap, and to default to the mappings specified for the container user namespace.

Change the specifications to allow such behavior.

config.md

rata

LGTM, left some minor comments :)

config.md

rata · 2023-09-06T10:04:01Z

config.md

- `idmap` | SHOULD | Indicates that the mount has `uidMappings` and `gidMappings` specified. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If supported, the runtime MUST return an error if this option is provided and either of `uidMappings` or `gidMappings` are empty or not present.
- `ridmap` | SHOULD | Indicates that the mount has `uidMappings` and `gidMappings` specified, and the mapping is applied recursively [^3]. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If supported, the runtime MUST return an error if this option is provided and either of `uidMappings` or `gidMappings` are empty or not present.
+ `idmap` | SHOULD | Indicates that the mount must have an idmapping applied. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If `uidMappings` or `gidMappings` are not specified, the runtime MAY use the container's user namespace mapping. If there are no `uidMappings` and `gidMappings` specified and the container isn't using user namespaces, an error MUST be returned. This SHOULD be implemented using [`mount_setattr(MOUNT_ATTR_IDMAP)`][mount_setattr.2], available since Linux 5.12.
+ `ridmap` | SHOULD | Indicates that the mount must have an idmapping applied, and the mapping is applied recursively [^3]. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If `uidMappings` or `gidMappings` are not specified, the runtime MAY use the container's user namespace mapping. If there are no `uidMappings` and `gidMappings` specified and the container isn't using user namespaces, an error MUST be returned. This SHOULD be implemented using [`mount_setattr(MOUNT_ATTR_IDMAP)`][mount_setattr.2], available since Linux 5.12.


Suggested change

`ridmap` | SHOULD | Indicates that the mount must have an idmapping applied, and the mapping is applied recursively [^3]. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If `uidMappings` or `gidMappings` are not specified, the runtime MAY use the container's user namespace mapping. If there are no `uidMappings` and `gidMappings` specified and the container isn't using user namespaces, an error MUST be returned. This SHOULD be implemented using [`mount_setattr(MOUNT_ATTR_IDMAP)`][mount_setattr.2], available since Linux 5.12.

`ridmap` | SHOULD | Indicates that the mount MUST have an idmapping applied, and the mapping is applied recursively [^3]. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If `uidMappings` or `gidMappings` are not specified, the runtime MAY use the container's user namespace mapping. If there are no `uidMappings` and `gidMappings` specified and the container isn't using user namespaces, [error MUST be returned](runtime.md#errors). This SHOULD be implemented using [`mount_setattr(MOUNT_ATTR_IDMAP)`][mount_setattr.2], available since Linux 5.12.

rata

I think the current version can make the high-level container runtime's lifes more difficult for no good reason. IMHO it is simpler if these options MUST be specified with mappings in the mount.

rata · 2023-09-07T09:56:03Z

config.md

@@ -146,8 +146,8 @@ Runtimes MUST/SHOULD/MAY implement the following option strings for Linux:
 `sync` | MUST | [^1]
 `tmpcopyup` | MAY | copy up the contents to a tmpfs
 `unbindable` | MUST | [^2] (bind mounts)
- `idmap` | SHOULD | Indicates that the mount has `uidMappings` and `gidMappings` specified. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If supported, the runtime MUST return an error if this option is provided and either of `uidMappings` or `gidMappings` are empty or not present.
- `ridmap` | SHOULD | Indicates that the mount has `uidMappings` and `gidMappings` specified, and the mapping is applied recursively [^3]. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If supported, the runtime MUST return an error if this option is provided and either of `uidMappings` or `gidMappings` are empty or not present.
+ `idmap` | SHOULD | Indicates that the mount MUST have an idmapping applied. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If `uidMappings` or `gidMappings` are not specified, the runtime MAY use the container's user namespace mapping. If there are no `uidMappings` and `gidMappings` specified and the container isn't using user namespaces, an [error MUST be returned](runtime.md#errors). This SHOULD be implemented using [`mount_setattr(MOUNT_ATTR_IDMAP)`][mount_setattr.2], available since Linux 5.12.


Using my high-level container development hat, what is the point of this flag?

We can't use idmap mounts safely without checking two things: (a) if we will request idmap mounts to the low-level container runtime, (b) if the low level container runtime supports idmap mounts.

Having to do both things now is not ideal, but that is the reality today. But these flags adds more ways in which idmap mounts can be enabled, that is: userns are enabled and some mount has one of these flags. Checking for more ways is just more painful. And is convenient to check for the use at the config.json level, as that is where we have the runtime features subcommand exposed.

Furthermore, the fact that it MAY be enabled or not if no mappings are used, it is just not something we can use safely: if we expect the runtime to use idmap mounts, we pass this flag and it is not honored, then the volumes will have garbage as UID/GID. So we will always either use it with mappings or not use the flag, the MAY behavior doesn't help high-level container runtimes, given the particularities of this feature.

I think this flag can just be removed. We can also keep it, although it doesn't add any new info: we can say it can be used to signal that idmap mounts MUST be used and mappings MUST be specified, but if the mappings are specified is enough for the runtime to MUST do an idmap mount or generate an error anyways, so I see no point on having this flag.

I think the reason is that runtimes that do not support idmap will pass down this option to the mount, thus the mount will fail. So this is a valid alternative to check for its support through features. If the runtime doesn't support idmapped mounts, the container creation will fail

Hmm, more or less, for runc, for bind-mounts it is ignored: #1222 (comment).

And there you said it was ignored in crun too: #1222 (comment)

I'm okay with keeping this one if you want, but I'd change it so the mappings in the mount are a MUST. This way, high-level container runtimes don't have to check lot of different ways that might enable this functionality before using it.

The mount won't fail for bind-mounts unless you have special handling for it, though I have sent a runc PR for us to block it in the case of bind-mounts. I had hoped this would allow us to avoid the need for features to figure out if it would be silently ignored, but we needed both.

I don't have a preference for whether we should have default behaviour if there are no mappings specified. Crun already does this and the behaviour seems reasonable so I suggested we include it. As for why we need the flag at all -- we need ridmap and it seems strange to not have idmap (and, once we have the check I mentioned above this would be a way to ensure within config.json that the idmapping arguments are being checked).

Furthermore, the fact that it MAY be enabled or not if no mappings are used, it is just not something we can use safely: if we expect the runtime to use idmap mounts, we pass this flag and it is not honored, then the volumes will have garbage as UID/GID.

I missed this on the first reading -- it seems to me that you are misreading the old (and new) text. It says if uidMappings and gidMappings are unspecified, you MAY use the container's mappings. If they are specified, obviously that line doesn't apply and you MUST use the specified mappings (the MUST line for this is in the definition of uidMappings and gidMappings, not here). If you want it to be more explicit we could do something like:

Suggested change

`idmap` | SHOULD | Indicates that the mount MUST have an idmapping applied. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If `uidMappings` or `gidMappings` are not specified, the runtime MAY use the container's user namespace mapping. If there are no `uidMappings` and `gidMappings` specified and the container isn't using user namespaces, an [error MUST be returned](runtime.md#errors). This SHOULD be implemented using [`mount_setattr(MOUNT_ATTR_IDMAP)`][mount_setattr.2], available since Linux 5.12.

`idmap` | SHOULD | Indicates that the mount MUST have an idmapping applied. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If `uidMappings` or `gidMappings` are specified for the mount, the runtime MUST use those values for the mount's mapping. If they are not specified, the runtime MAY use the container's user namespace mapping, otherwise an [error MUST be returned](runtime.md#errors). If there are no `uidMappings` and `gidMappings` specified and the container isn't using user namespaces, an [error MUST be returned](runtime.md#errors). This SHOULD be implemented using [`mount_setattr(MOUNT_ATTR_IDMAP)`][mount_setattr.2], available since Linux 5.12.

But it seems somewhat redundant to me.

thanks, I've amended these changes and pushed a new version

@cyphar I'm not missing that, I'm saying exactly that is problematic.

these flags adds more ways in which idmap mounts can be enabled, that is: userns are enabled and some mount has one of these flags. Checking for more ways is just more painful.

High-level runtimes sometimes need to check the config.json to see if idmap mounts are used (as to carry the info of the features subcommand higher in the call-stack is sometimes inconvenient). This adds one more way that they could be used and therefore one more way the high-level runtimes need to check to see if idmap mounts will be used.

Exactly that is not convenient. We have to check for the uidMapping/gidMapping of all mounts to see if it is used. With this PR, we will have to check that and see if maybe some other flag was used, because that can mean they are used too.

IMHO, it is just not helping for most use cases.

The practical issue here is that crun already has this feature, and we've introduced the flag into the spec with slightly different semantics. In practice, this isn't the end of the world because there isn't a MUST requirement that uidMapping and gidMapping need to be specified (in fact, this PR actually improves the language to describe how these flags work -- my original wording was a bit rushed). It seems unlikely that crun will be able to remove the feature without impacting users, meaning that you will need to check for idmap anyway (unless you plan to only support runc as the OCI runtime). The difference is that now all OCI runtimes will have uniform behaviour.

FWIW, it would've been nice if crun had used something like crun-... as a prefix to its extensions to avoid this issue, but runc has tmpcopyup so this isn't a crun-only issue and so it's understandable that we ended up in this situation.

In retrospect, I think we should have added ridmap and idmap from the outset and required their use for id-mapped mounts. This would've resolved the issues that resulted in the need for the extra features bits. Right now all we can do is SHOULD their use (which we already do).

Ideally you would only have to check for one thing (presence of idmap or ridmap) but unfortunately you also need to check for uidMappings and gidMappings because they aren't required. However, as I mentioned above, if you support using crun then you will need to check for idmap as well anyway.

rata · 2023-09-07T09:57:04Z

config.md

- `idmap` | SHOULD | Indicates that the mount has `uidMappings` and `gidMappings` specified. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If supported, the runtime MUST return an error if this option is provided and either of `uidMappings` or `gidMappings` are empty or not present.
- `ridmap` | SHOULD | Indicates that the mount has `uidMappings` and `gidMappings` specified, and the mapping is applied recursively [^3]. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If supported, the runtime MUST return an error if this option is provided and either of `uidMappings` or `gidMappings` are empty or not present.
+ `idmap` | SHOULD | Indicates that the mount MUST have an idmapping applied. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If `uidMappings` or `gidMappings` are not specified, the runtime MAY use the container's user namespace mapping. If there are no `uidMappings` and `gidMappings` specified and the container isn't using user namespaces, an [error MUST be returned](runtime.md#errors). This SHOULD be implemented using [`mount_setattr(MOUNT_ATTR_IDMAP)`][mount_setattr.2], available since Linux 5.12.
+ `ridmap` | SHOULD | Indicates that the mount MUST have an idmapping applied, and the mapping is applied recursively [^3]. This option SHOULD NOT be passed to the underlying [`mount(2)`][mount.2] call. If `uidMappings` or `gidMappings` are not specified, the runtime MAY use the container's user namespace mapping. If there are no `uidMappings` and `gidMappings` specified and the container isn't using user namespaces, an [error MUST be returned](runtime.md#errors). This SHOULD be implemented using [`mount_setattr(MOUNT_ATTR_IDMAP)`][mount_setattr.2], available since Linux 5.12.


Idem, I think this flag MUST only be used with mappings (otherwise generate an error) and, of course, signal that the idmap mounts MUST be recursive.

The text "Indicates that the mount MUST have an idmapping applied, and the mapping is applied recursively" IMHO is sufficient to say that the mapping MUST be recursive. As for the first point, the discussion in the other thread applies -- this allows for easier configuration and matches crun's behaviour for idmap.

rata

IMHO, changing this as I've just suggested is better. Let me know what others think

giuseppe · 2023-09-08T10:31:12Z

@cyphar @AkihiroSuda could you please take a look?

cyphar · 2023-09-09T13:03:40Z

I will take a proper look when I get home tomorrow evening

giuseppe · 2023-09-20T10:39:49Z

@cyphar any chance you can take a look? :-)

AkihiroSuda · 2023-09-25T18:55:20Z

Needs rebase

AkihiroSuda · 2023-09-25T18:55:32Z

@cyphar PTAL, and let's release v1.1.1 soon

giuseppe · 2023-09-26T14:59:24Z

rebased

giuseppe · 2023-10-12T08:33:05Z

@cyphar PTAL

cyphar

LGTM. I've left a suggestion for more explicit wording that might make @rata happier, but I don't think it's necessary (a reading of this section along with the section defining uidMappings and gidMappings leads to the clear understanding that specifying uidMappings and gidMappings takes precedence over this extra idmap and ridmap behaviour).

crun currently allows to specify an empty mapping for [r]idmap, and to default to the mappings specified for the container user namespace. Change the specifications to allow such behavior. Signed-off-by: Giuseppe Scrivano <[email protected]>

AkihiroSuda · 2023-11-06T16:58:16Z

@cyphar May I assume your LGTM is still valid?

cyphar

Still LGTM.

The runtime-spec just merged this PR: opencontainers/runtime-spec#1224 This means that it is now possible to request idmap mounts by specifying "idmap" or "ridmap" in the mount options, without any mappings. Let's add a check to see if they are requested in that way too. Signed-off-by: Rodrigo Campos <[email protected]>

The runtime-spec just merged this PR: opencontainers/runtime-spec#1224 This means that it is now possible to request idmap mounts by specifying "idmap" or "ridmap" in the mount options, without any mappings. Let's add a check to see if they are requested in that way too. Signed-off-by: Rodrigo Campos <[email protected]> Signed-off-by: Vivek Radhakrishnan <[email protected]>

The runtime-spec just merged this PR: opencontainers/runtime-spec#1224 This means that it is now possible to request idmap mounts by specifying "idmap" or "ridmap" in the mount options, without any mappings. Let's add a check to see if they are requested in that way too. Signed-off-by: Rodrigo Campos <[email protected]>

giuseppe requested review from AkihiroSuda, crosbymichael, cyphar, dqminh, hqhq, kolyshkin, mrunalp, thaJeztah, tianon, vbatts and utam0k as code owners September 5, 2023 20:06

giuseppe mentioned this pull request Sep 5, 2023

config: add idmap and ridmap mount options #1222

Merged

AkihiroSuda approved these changes Sep 5, 2023

View reviewed changes

AkihiroSuda added this to the v1.1.1 milestone Sep 5, 2023

cyphar reviewed Sep 6, 2023

View reviewed changes

config.md Show resolved Hide resolved

giuseppe force-pushed the allow-empty-mapping-for-idmap branch 2 times, most recently from 977d7ba to ba0a750 Compare September 6, 2023 08:02

rata approved these changes Sep 6, 2023

View reviewed changes

giuseppe force-pushed the allow-empty-mapping-for-idmap branch from ba0a750 to 656e466 Compare September 6, 2023 13:00

rata reviewed Sep 7, 2023

View reviewed changes

rata suggested changes Sep 7, 2023

View reviewed changes

AkihiroSuda approved these changes Sep 9, 2023

View reviewed changes

giuseppe force-pushed the allow-empty-mapping-for-idmap branch from 656e466 to 5248b3f Compare September 26, 2023 07:16

AkihiroSuda requested a review from cyphar October 22, 2023 13:51

cyphar approved these changes Oct 23, 2023

View reviewed changes

config.md: allow empty mappings for [r]idmap

021ba94

crun currently allows to specify an empty mapping for [r]idmap, and to default to the mappings specified for the container user namespace. Change the specifications to allow such behavior. Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the allow-empty-mapping-for-idmap branch from 5248b3f to 021ba94 Compare October 24, 2023 08:59

AkihiroSuda requested a review from cyphar October 25, 2023 04:51

cyphar approved these changes Nov 16, 2023

View reviewed changes

AkihiroSuda approved these changes Nov 16, 2023

View reviewed changes

AkihiroSuda merged commit 9923541 into opencontainers:main Nov 16, 2023
3 checks passed

rata mentioned this pull request Dec 4, 2023

pkg/process: Check using idmap mount options too containerd/containerd#9461

Merged

utam0k mentioned this pull request Jan 5, 2024

Release v1.2.0 #1242

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config.md: allow empty mappings for [r]idmap #1224

config.md: allow empty mappings for [r]idmap #1224

giuseppe commented Sep 5, 2023

rata left a comment

rata Sep 6, 2023

rata left a comment

rata Sep 7, 2023

giuseppe Sep 7, 2023

rata Sep 8, 2023

cyphar Sep 9, 2023 •

edited

Loading

cyphar Oct 23, 2023

giuseppe Oct 24, 2023

rata Oct 24, 2023 •

edited

Loading

cyphar Nov 16, 2023

rata Sep 7, 2023

cyphar Oct 23, 2023

rata left a comment

giuseppe commented Sep 8, 2023

cyphar commented Sep 9, 2023

giuseppe commented Sep 20, 2023

AkihiroSuda commented Sep 25, 2023

AkihiroSuda commented Sep 25, 2023

giuseppe commented Sep 26, 2023

giuseppe commented Oct 12, 2023

cyphar left a comment

AkihiroSuda commented Nov 6, 2023

cyphar left a comment

config.md: allow empty mappings for [r]idmap #1224

config.md: allow empty mappings for [r]idmap #1224

Conversation

giuseppe commented Sep 5, 2023

rata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rata 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

cyphar Sep 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rata Oct 24, 2023 • edited Loading

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

rata left a comment

Choose a reason for hiding this comment

giuseppe commented Sep 8, 2023

cyphar commented Sep 9, 2023

giuseppe commented Sep 20, 2023

AkihiroSuda commented Sep 25, 2023

AkihiroSuda commented Sep 25, 2023

giuseppe commented Sep 26, 2023

giuseppe commented Oct 12, 2023

cyphar left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Nov 6, 2023

cyphar left a comment

Choose a reason for hiding this comment

cyphar Sep 9, 2023 •

edited

Loading

rata Oct 24, 2023 •

edited

Loading