support changing of lsm mount context on restore #3068

adrianreber · 2021-07-07T13:00:17Z

Wire through CRIU's support to change the mount context on restore.

This is especially useful if restoring a container in a different pod.

Single container restore uses the same SELinux process label and same mount context as during checkpointing. If a container is being restored into an existing pod the process label and the mount context needs to be changed to the context of the pod.

Changing process label on restore is already supported by runc. This patch adds the possibility to change the mount context.

This requires yet unreleased version of CRIU 3.16.

Proposed changelog entry:

New features:
* checkpoint/restore: add an option (`--lsm-mount-context`) to set
  a different LSM mount context on restore. (#3068)

man/runc-restore.8.md

libcontainer/container_linux.go

man/runc-restore.8.md

adrianreber · 2021-07-08T07:56:31Z

Made all requested changes and force pushed.

man/runc-restore.8.md

adrianreber · 2021-07-12T08:53:58Z

Both man page changes applied.

kolyshkin · 2021-07-21T00:40:14Z

@adrianreber could you please rebase (needed to run latest CI)?

kolyshkin

LGTM

kolyshkin · 2021-07-28T03:23:40Z

@adrianreber needs a rebase 🙏🏻

adrianreber · 2021-07-28T06:37:31Z

@adrianreber needs a rebase 🙏🏻

rebased

kolyshkin

LGTM

kolyshkin · 2021-08-24T01:58:02Z

@thaJeztah PTAL

thaJeztah · 2021-08-24T14:59:32Z

Overall looks good; I'm not a very SELinux person, but I noticed that we also have

--process-label label : Set the asm process label for the process commonly used with selinux(7).

Which uses label as term. Is it clear that both apply a label?

Just asking; I'm fine with this if this term is clearer to more SELinux-savvy users 😅

kolyshkin · 2021-08-24T17:41:43Z

mount(8) man page clearly talks about contexts (although it has no less than 4 options for that), so I guess "context" is the right term here.
Option --process-label sets SELinux values via /proc using go-selinux pkg. The package uses term "label", the proc(5) (when describing /proc/[pid]/attr files such as attr/exec) uses the terms "attribute", "label", and "[security] context", with "attribute" being the most generic term.
From selinux(7) it seems that "security context" and "label" terms are interchangeable.

Overall, it seems OK as it is (process -- label, mount -- context). We also have --lsm-profile which argument is called label.

kolyshkin · 2021-08-24T17:42:13Z

@rhatdan can you please review this from the POV of terminology used?

rhatdan · 2021-08-24T19:38:08Z

man/runc-restore.8.md

@@ -74,6 +74,14 @@ daemon. See [criu --lazy-pages option](https://criu.org/CLI/opt/--lazy-pages).
 : Specify an LSM profile to be used during restore. Here _type_ can either be
 **apparamor** or **selinux**, and _label_ is a valid LSM label. For example,
 **--lsm-profile "selinux:system_u:system_r:container_t:s0:c82,c137"**.
+By default, the checkpointed LSM profile is used upon restore.
+
+**--lsm-mount-context** _context_


I hate typing extra letters, why not just --mount-context? Or --mount-label, since context is not a common user facing term.

As a matter of fact, I think all through runc, it is referred to as the MountLabel.

@adrianreber WDYT about --mount-context or --mount-label?

The only reason not to rename it is to keep the options consistent between criu and runc.

Another reason not to rename it is that Podman already uses this option if it finds support for it in runc.

OK I'll leave this decision to @rhatdan 🦆

I hate typing extra letters, why not just --mount-context? Or --mount-label, since context is not a common user facing term.

@rhatdan I think this option is not likely to be used very often directly by users from command-line and using an explicit name clarifies the meaning of "context".

Yeah I'm with @kolyshkin. I think it would be nice to use something a bit less cryptic -- the fact that podman already expects this (presumably because crun already supports this?) is a bit of a shame.

I thought a bit more about the Podman problem and it is not really a problem. Podman currently checks for the existence of --lsm-mount-context and tells the user early if runc does not support it. So we can still call it whatever we want in runc and adapt Podman.

Should I change it to --mount-label? Would that be a good name for everyone?

@kolyshkin Are you sure? The example given in the docs is --lsm-mount-context "system_u:object_r:container_file_t:s0:c82,c137". --lsm-profile takes type:profile.

However, since we already have --lsm-profile maybe --lsm-mount-context is more consistent. Since it looks like we've copied the CRIU argument names in the past we should probably just stick with it...

@cyphar right, I mixed it with --lsm-profile which does have apparmor or selinux prefix.

Since it looks like we've copied the CRIU argument names in the past we should probably just stick with it...

Fair point. Adding --lsm-mount-label to the list of possible names (naming is hard!)

adrianreber · 2021-09-13T10:12:05Z

Rebased.

adrianreber · 2021-09-14T06:18:13Z

So, did we agree on not changing the name of the option? Is this ready to be merged?

cyphar

LGTM. Yeah, let's keep it in line with the other --lsm-* options.

Signed-off-by: Adrian Reber <areber@redhat.com>

Wire through CRIU's support to change the mount context on restore. This is especially useful if restoring a container in a different pod. Single container restore uses the same SELinux process label and same mount context as during checkpointing. If a container is being restored into an existing pod the process label and the mount context needs to be changed to the context of the pod. Changing process label on restore is already supported by runc. This patch adds the possibility to change the mount context. Signed-off-by: Adrian Reber <areber@redhat.com>

adrianreber · 2021-09-20T09:39:45Z

I had to rebase again for go.mod conflicts.

kolyshkin

LGTM

thaJeztah

LGTM

adrianreber force-pushed the 2021-07-07-lsm-mount-context branch from bc90e5f to 02139ea Compare July 7, 2021 13:01

kolyshkin reviewed Jul 7, 2021

View reviewed changes

man/runc-restore.8.md Outdated Show resolved Hide resolved

kolyshkin reviewed Jul 7, 2021

View reviewed changes

man/runc-restore.8.md Outdated Show resolved Hide resolved

kolyshkin reviewed Jul 7, 2021

View reviewed changes

man/runc-restore.8.md Outdated Show resolved Hide resolved

kolyshkin reviewed Jul 7, 2021

View reviewed changes

libcontainer/container_linux.go Outdated Show resolved Hide resolved

kolyshkin reviewed Jul 7, 2021

View reviewed changes

man/runc-restore.8.md Show resolved Hide resolved

kolyshkin added the area/checkpoint-restore label Jul 7, 2021

adrianreber force-pushed the 2021-07-07-lsm-mount-context branch from 02139ea to d8f84d4 Compare July 8, 2021 07:55

kolyshkin mentioned this pull request Jul 8, 2021

build(deps): bump github.com/checkpoint-restore/go-criu/v5 from 5.0.0 to 5.1.0 #3077

Closed

kolyshkin reviewed Jul 12, 2021

View reviewed changes

man/runc-restore.8.md Outdated Show resolved Hide resolved

kolyshkin reviewed Jul 12, 2021

View reviewed changes

man/runc-restore.8.md Outdated Show resolved Hide resolved

kolyshkin added this to the 1.1.0 milestone Jul 12, 2021

kolyshkin added the impact/changelog label Jul 12, 2021

adrianreber force-pushed the 2021-07-07-lsm-mount-context branch from d8f84d4 to 1c169eb Compare July 12, 2021 08:53

adrianreber force-pushed the 2021-07-07-lsm-mount-context branch from 1c169eb to 5b1e281 Compare July 16, 2021 09:38

adrianreber force-pushed the 2021-07-07-lsm-mount-context branch from 5b1e281 to 712fc9c Compare July 21, 2021 06:58

kolyshkin previously approved these changes Jul 21, 2021

View reviewed changes

adrianreber dismissed kolyshkin’s stale review via a9a34a2 July 28, 2021 06:36

adrianreber force-pushed the 2021-07-07-lsm-mount-context branch from 712fc9c to a9a34a2 Compare July 28, 2021 06:36

kolyshkin previously approved these changes Jul 28, 2021

View reviewed changes

rhatdan reviewed Aug 24, 2021

View reviewed changes

cyphar mentioned this pull request Sep 9, 2021

Release 1.1.0 #3195

Closed

kolyshkin added the status/needs-rebase label Sep 10, 2021

adrianreber dismissed kolyshkin’s stale review via 142f3c3 September 13, 2021 10:11

adrianreber force-pushed the 2021-07-07-lsm-mount-context branch from a9a34a2 to 142f3c3 Compare September 13, 2021 10:11

cyphar previously approved these changes Sep 19, 2021

View reviewed changes

adrianreber added 2 commits September 20, 2021 10:01

Vendor in go-criu v5.1.0

412d68d

Signed-off-by: Adrian Reber <areber@redhat.com>

adrianreber dismissed cyphar’s stale review via 43b36dc September 20, 2021 08:01

adrianreber force-pushed the 2021-07-07-lsm-mount-context branch from 142f3c3 to 43b36dc Compare September 20, 2021 08:01

kolyshkin requested review from cyphar and thaJeztah September 20, 2021 16:34

kolyshkin approved these changes Sep 20, 2021

View reviewed changes

thaJeztah approved these changes Sep 20, 2021

View reviewed changes

thaJeztah merged commit 206c16a into opencontainers:master Sep 20, 2021

rst0git mentioned this pull request Jun 21, 2024

restore: fix container restore into pod containers/podman#23056

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support changing of lsm mount context on restore #3068

support changing of lsm mount context on restore #3068

adrianreber commented Jul 7, 2021 •

edited by kolyshkin

Loading

adrianreber commented Jul 8, 2021

adrianreber commented Jul 12, 2021

kolyshkin commented Jul 21, 2021

kolyshkin left a comment

kolyshkin commented Jul 28, 2021

adrianreber commented Jul 28, 2021

kolyshkin left a comment

kolyshkin commented Aug 24, 2021

thaJeztah commented Aug 24, 2021

kolyshkin commented Aug 24, 2021

kolyshkin commented Aug 24, 2021

rhatdan Aug 24, 2021

rhatdan Aug 24, 2021

kolyshkin Aug 31, 2021

adrianreber Sep 1, 2021

kolyshkin Sep 2, 2021

rst0git Sep 9, 2021

adrianreber Sep 10, 2021

cyphar Sep 10, 2021

kolyshkin Sep 16, 2021

thaJeztah Sep 17, 2021

adrianreber commented Sep 13, 2021

adrianreber commented Sep 14, 2021

cyphar left a comment

adrianreber commented Sep 20, 2021

kolyshkin left a comment

thaJeztah left a comment

support changing of lsm mount context on restore #3068

support changing of lsm mount context on restore #3068

Conversation

adrianreber commented Jul 7, 2021 • edited by kolyshkin Loading

Proposed changelog entry:

adrianreber commented Jul 8, 2021

adrianreber commented Jul 12, 2021

kolyshkin commented Jul 21, 2021

kolyshkin left a comment

Choose a reason for hiding this comment

kolyshkin commented Jul 28, 2021

adrianreber commented Jul 28, 2021

kolyshkin left a comment

Choose a reason for hiding this comment

kolyshkin commented Aug 24, 2021

thaJeztah commented Aug 24, 2021

kolyshkin commented Aug 24, 2021

kolyshkin commented Aug 24, 2021

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

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

adrianreber commented Sep 13, 2021

adrianreber commented Sep 14, 2021

cyphar left a comment

Choose a reason for hiding this comment

adrianreber commented Sep 20, 2021

kolyshkin left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

adrianreber commented Jul 7, 2021 •

edited by kolyshkin

Loading