Skip to content

Conversation

@AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Jan 30, 2024

Note

This PR is being reverted for compatibility reason:


Prior to this commit, readOnly volumes were not recursively read-only and could result in compromise of data;
e.g., even if /mnt was mounted as read-only, its submounts such as /mnt/usbstorage were not read-only.

This commit utilizes runc's "rro" bind mount option to make read-only bind mounts literally read-only. The "rro" bind mount options is implemented by calling mount_setattr(2) with MOUNT_ATTR_RDONLY and AT_RECURSIVE.

The "rro" bind mount options requires kernel >= 5.12, with runc >= 1.1 or a compatible runtime such as crun >= 1.4.

When the "rro" bind mount options is not available, containerd falls back to the legacy non-recursive read-only mounts by default.

The behavior is configurable via /etc/containerd/config.toml:

version = 2
[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc]
  # treat_ro_mounts_as_rro ("Enabled"|"IfPossible"|"Disabled")
  # treats read-only mounts as recursive read-only mounts.
  # An empty string means "IfPossible".
  # "Enabled" requires Linux kernel v5.12 or later.
  # This configuration does not apply to non-volume mounts such as "/sys/fs/cgroup".
  treat_ro_mounts_as_rro = ""

Replaces:

Note: this change does not affect non-CRI clients such as ctr, nerdctl, and Docker/Moby. RRO mounts have been supported since nerdctl v0.14 (containerd/nerdctl#511) and Docker v25 (moby/moby#45278).

@AkihiroSuda
Copy link
Member Author

/retest

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 .. very nice just a nit on the config doc..

Prior to this commit, `readOnly` volumes were not recursively read-only and
could result in compromise of data;
e.g., even if `/mnt` was mounted as read-only, its submounts such as
`/mnt/usbstorage` were not read-only.

This commit utilizes runc's "rro" bind mount option to make read-only bind
mounts literally read-only. The "rro" bind mount options is implemented by
calling `mount_setattr(2)` with `MOUNT_ATTR_RDONLY` and `AT_RECURSIVE`.

The "rro" bind mount options requires kernel >= 5.12, with runc >= 1.1 or
a compatible runtime such as crun >= 1.4.

When the "rro" bind mount options is not available, containerd falls back
to the legacy non-recursive read-only mounts by default.

The behavior is configurable via `/etc/containerd/config.toml`:
```toml
version = 2
[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc]
  # treat_ro_mounts_as_rro ("Enabled"|"IfPossible"|"Disabled")
  # treats read-only mounts as recursive read-only mounts.
  # An empty string means "IfPossible".
  # "Enabled" requires Linux kernel v5.12 or later.
  # This configuration does not apply to non-volume mounts such as "/sys/fs/cgroup".
  treat_ro_mounts_as_rro = ""
```

Replaces:
- kubernetes/enhancements issue 3857
- kubernetes/enhancements PR 3858

Note: this change does not affect non-CRI clients such as ctr, nerdctl, and Docker/Moby.
RRO mounts have been supported since nerdctl v0.14 (containerd/nerdctl PR 511)
and Docker v25 (moby/moby PR 45278).

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
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

@mxpv mxpv added this pull request to the merge queue Feb 1, 2024
@AkihiroSuda AkihiroSuda requested review from dims and dmcgowan February 1, 2024 18:47
Merged via the queue into containerd:main with commit ac54047 Feb 1, 2024
@AkihiroSuda AkihiroSuda changed the title cri: make read-only mounts recursively read-only [Reverted in #9747] cri: make read-only mounts recursively read-only Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cri Container Runtime Interface (CRI) size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants