Allow devs to set labels in container images for default capabilities. #5206

rhatdan · 2020-02-14T01:29:57Z

This patch allows users to specify the list of capabilities required
to run their container image.

Setting a image/container label "io.containers.capabilities=setuid,setgid"
tells podman that the contained image should work fine with just these two
capabilties, instead of running with the default capabilities, podman will
launch the container with just these capabilties.

If the user or image specified capabilities that are not in the default set,
the container will print an error message and will continue to run with the
default capabilities.

Signed-off-by: Daniel J Walsh [email protected]

rhatdan · 2020-02-14T01:45:29Z

@vrothberg This is what we talked about and I covered in my talk in BRNO.
PTAL
Need to get something similar for seccomp.json files

mheon · 2020-02-14T01:55:40Z

docs/source/markdown/podman-create.1.md

@@ -409,6 +409,16 @@ millions of trillions.

 Add metadata to a container (e.g., --label com.example.key=value)

+Users can set a special label **io.containers.capabilities=CAP1,CAP2,CAP3** that


I feel like this belongs elsewhere. We don't expect people to set this at podman run time, right? Instead we want them to load it from the image.

Should we instead be setting io.containers.capabilities at container creation time based on the capabilities we put in the spec, so that it will be there when we commit images?

No the goal here is to override the defaults with something more secure.

But rather than trying to get people to add the label themselves, we can make it for them from what they entered in --cap-add and --cap-drop. We know the final capabilities the container was created with, so it's pretty trivial to make the label from that.

But that is not natural behaviour for Building container images. Most people, if they are going to use this feature are going to do it via a Dockerfile. We could add the label as a side effect of --cap-add and --cap-drop, but I am not sure that would be expected by users.

I agree that most people will be using a Dockerfile - I'm mostly confused as to why people would ever want to set it directly at podman run time, instead of using --cap-add and --cap-drop.

Sure, but do you just want me to not document that you can do this?

mheon · 2020-02-14T01:56:40Z

docs/source/markdown/podman-create.1.md

+
+```
+
+$ podman run -d --label=io.containers.capabilities=setuid,setgid fedora /usr/bin/server


Why do I do this instead of --cap-add and --cap-drop? I don't know if this makes much sense outside of loading from images.

The idea is to get this data into the container image. And maybe I should add the data to Docker build.
The real goal is to get people to add
LABEL io.containers.capabilities=setuid,setgid
In their Containerfile/Dockerfile.
Then users of their container image will get this new more secure behaviour.

rhatdan · 2020-02-14T02:13:30Z

Updated to add more information to podman build.

vrothberg

A couple of nits:

Let's first get Undocker part 1) #5209 in. This adds a pkg/capabilities where we can put the label as a constant as we do in the new pkg/seccomp package. The comparison with the default capabilities could be moved into this package as well.
Please prepare and push an image to quay.io/libpod as we do for the seccomp-policy tests.

vrothberg · 2020-02-14T11:04:55Z

pkg/spec/spec.go

@@ -315,6 +315,14 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime, userM
 	}
 	configSpec := g.Config

+	var capRequired []string
+	for key, val := range config.Labels {
+		if key == "io.containers.capabilities" {


This should really be a constant.

I would love the constant to be in pkg/capabilities as we do for the seccomp policies now in pkg/seccomp.

Will have to move this to containers/common since pkg/capabilities is in there now, Also I think this should be a list. since it might change in the future if this got standardized and they picked a different name.

test/e2e/run_security_labels.go

pkg/spec/createconfig.go

vrothberg · 2020-02-14T11:07:59Z

pkg/spec/security.go

@@ -123,6 +125,25 @@ func (c *SecurityConfig) ConfigureGenerator(g *generate.Generator, user *UserCon
 		return err
 	}

+	privCapRequired := []string{}
+
+	if !c.Privileged && len(c.CapRequired) > 0 {


I think we should always set CapRequired. If it's not set via the CLI or by the image, we should use the default ones.

We can talk about this, but this could be a problem. I was thinking CapRequired was something that the user or the image had specified as the capabilies required to run the application. As opposed to the caplist, which is the "default capabilities + CapADD - CapDrop). If we set CapRequired == CapDefault and only overrode it when the label was set, then we would not be able to tell when the user specified at capability outside of the "default" list of capabilities.

Maybe we can have a quick bluejeans later? I want to show the pkg/seccomp package that we can extend for these needs.

pkg/spec/spec.go

TomSweeneyRedHat · 2020-02-14T14:47:24Z

docs/source/markdown/podman-build.1.md

@@ -272,6 +272,16 @@ BUILDAH\_ISOLATION environment variable.  `export BUILDAH_ISOLATION=oci`

 Add an image *label* (e.g. label=*value*) to the image metadata. Can be used multiple times.

+Users can set a special label **io.containers.capabilities=CAP1,CAP2,CAP3** that
+indicates the default list of Linux capabilities required for the container to
+run properly. Setting this label in a container or a container image tells podman


podman -> Podman

TomSweeneyRedHat · 2020-02-14T14:50:08Z

docs/source/markdown/podman-build.1.md

+with just the specified capabilties, as long as this list of capabilities is a
+subset of the default list.
+
+If the user or image specified capabilities that are not in the default set, the container


I find "user or image" to be a bit confusing. Could we just drop that? "If the specified capabilities..."

ooops, if dropped, the following "that" doesn't work. "If the specified capabilities are not in the default set, ..."

TomSweeneyRedHat · 2020-02-14T14:58:18Z

docs/source/markdown/podman-commit.1.md

@@ -86,6 +86,11 @@ $ podman commit -q --pause=false containerID image-committed
 e3ce4d93051ceea088d1c242624d659be32cf1667ef62f1d16d6b60193e2c7a8
 ```

+```
+$ podman commit -q --change LABEL=io.containers.capabilities=setuid,setgid epic_nobel privimage


You don't have the verbiage from build in here, should you?

I don't think so. I add data to build about the label. I just wanted to throw this in to show it as an example. We don't document other labels for commit.

This is badly formatted; should be --change 'LABEL io.containers.capabilities=setuid,setgid'

Their might be a bug in the parser, but my syntax works, and yours does not. This is also documented two lines higher.

$ podman commit --change "LABEL foo=bar" myweb dan1 Error: invalid syntax for --change: LABEL foo=bar $ podman commit --change "LABEL=foo=bar" myweb dan1 Getting image source signatures Copying blob 195be5f8be1d skipped: already exists Copying blob 5f70bf18a086 [--------------------------------------] 0.0b / 0.0b Copying config e87256c3fc done Writing manifest to image destination Storing signatures e87256c3fcda3881112f19dbfda9a733bee0472354269dc62c2c5c7e81c69e1f

test/e2e/run_security_labels.go

rh-atomic-bot · 2020-02-14T19:13:47Z

☔ The latest upstream changes (presumably #5203) made this pull request unmergeable. Please resolve the merge conflicts.

TomSweeneyRedHat · 2020-02-24T13:51:54Z

LGTM, but definitely want a @mheon head nod

mheon · 2020-02-24T14:32:30Z

I'm still iffy about documenting this in run and create - it seems like extra confusion since we already have ways to do this. Might want to talk about it at scrum.

rh-atomic-bot · 2020-02-25T14:44:21Z

☔ The latest upstream changes (presumably #5297) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2020-02-25T20:08:08Z

@mheon @vrothberg @giuseppe @baude PTAL

rhatdan · 2020-02-27T10:20:04Z

@mheon @vrothberg @giuseppe @baude PTAL
Passes tests now and removed mentions of the label in podman run and podman create.

rh-atomic-bot · 2020-02-27T16:52:39Z

☔ The latest upstream changes (presumably #5338) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2020-02-28T11:46:59Z

As @TomSweeneyRedHat would say, Happy Green Buttons. Can I get this reviewed?
@mheon @giuseppe @vrothberg @baude @QiWang19 @jwhonce

docs/source/markdown/podman-create.1.md

vrothberg

Other than the man page nits LGTM.

vrothberg · 2020-03-02T08:03:07Z

docs/source/markdown/podman-create.1.md

+
+```
+
+$ podman run -d --label=io.containers.capabilities=setuid,setgid fedora /usr/bin/server


Suggested change

$ podman run -d --label=io.containers.capabilities=setuid,setgid fedora /usr/bin/server

$ podman create -d --label=io.containers.capabilities=setuid,setgid fedora /usr/bin/server

Removing line.

vrothberg · 2020-03-02T08:03:26Z

docs/source/markdown/podman-create.1.md

@@ -1000,6 +1000,14 @@ can override the working directory by using the **-w** option.
 $ podman create alpine ls
 ```

+### Run a container with just the SETUID and SETGID Capabilities


Suggested change

### Run a container with just the SETUID and SETGID Capabilities

### Create a container with just the SETUID and SETGID Capabilities

Removing line.
We don't want to document this label in podman run or create.

docs/source/markdown/podman-build.1.md

vrothberg · 2020-03-02T08:07:29Z

I am loving the new GitHub feature to suggest changes in the reviews.

rhatdan · 2020-03-02T21:36:27Z

@vrothberg Sadly our Gating blocks it.

This patch allows users to specify the list of capabilities required to run their container image. Setting a image/container label "io.containers.capabilities=setuid,setgid" tells podman that the contained image should work fine with just these two capabilties, instead of running with the default capabilities, podman will launch the container with just these capabilties. If the user or image specified capabilities that are not in the default set, the container will print an error message and will continue to run with the default capabilities. Signed-off-by: Daniel J Walsh <[email protected]>

Signed-off-by: Valentin Rothberg <[email protected]> Signed-off-by: Daniel J Walsh <[email protected]>

vrothberg

Restarted the flaked test.
LGTM, cool feature!

giuseppe

/lgtm

openshift-ci-robot · 2020-03-03T13:11:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested review from TomSweeneyRedHat and umohnani8 February 14, 2020 01:30

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L labels Feb 14, 2020

rhatdan force-pushed the capabilities branch from 75c75c0 to b5db8b7 Compare February 14, 2020 01:35

mheon reviewed Feb 14, 2020

View reviewed changes

rhatdan force-pushed the capabilities branch from b5db8b7 to 00eceef Compare February 14, 2020 02:12

vrothberg mentioned this pull request Feb 14, 2020

Undocker part 1) #5209

Merged

vrothberg reviewed Feb 14, 2020

View reviewed changes

TomSweeneyRedHat reviewed Feb 14, 2020

View reviewed changes

test/e2e/run_security_labels.go Show resolved Hide resolved

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2020

rhatdan force-pushed the capabilities branch from 00eceef to 2fc7dc7 Compare February 23, 2020 10:59

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2020

rhatdan changed the title ~~Allow devs to set labels in container images for default capabilities.~~ [WIP] Allow devs to set labels in container images for default capabilities. Feb 23, 2020

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL and removed size/L labels Feb 23, 2020

rhatdan force-pushed the capabilities branch 2 times, most recently from 181e734 to 76606eb Compare February 23, 2020 11:10

rhatdan force-pushed the capabilities branch from 76606eb to 473fa2b Compare February 25, 2020 11:11

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2020

rhatdan force-pushed the capabilities branch from a7b1e25 to 797380b Compare February 26, 2020 12:06

openshift-ci-robot added size/L and removed size/XL labels Feb 26, 2020

rhatdan force-pushed the capabilities branch 2 times, most recently from ffadc9c to 7d31e5d Compare February 26, 2020 21:59

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2020

rhatdan force-pushed the capabilities branch from 7d31e5d to a5e5884 Compare February 27, 2020 18:19

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2020

mheon reviewed Feb 28, 2020

View reviewed changes

docs/source/markdown/podman-create.1.md Outdated Show resolved Hide resolved

vrothberg reviewed Mar 2, 2020

View reviewed changes

rhatdan force-pushed the capabilities branch from a5e5884 to bb7c7fc Compare March 2, 2020 17:27

rhatdan added 2 commits March 2, 2020 16:37

Update docs/source/markdown/podman-build.1.md

f678b3f

Signed-off-by: Valentin Rothberg <[email protected]> Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the capabilities branch from 8863600 to f678b3f Compare March 2, 2020 21:39

vrothberg reviewed Mar 3, 2020

View reviewed changes

giuseppe approved these changes Mar 3, 2020

View reviewed changes

openshift-ci-robot assigned giuseppe Mar 3, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2020

openshift-merge-robot merged commit 3bc5f43 into containers:master Mar 3, 2020

rh-atomic-bot mentioned this pull request Mar 3, 2020

Bump to Buildah v1.14.1 #5370

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023

		@@ -409,6 +409,16 @@ millions of trillions.

		Add metadata to a container (e.g., --label com.example.key=value)

		Users can set a special label io.containers.capabilities=CAP1,CAP2,CAP3 that


		```

		$ podman run -d --label=io.containers.capabilities=setuid,setgid fedora /usr/bin/server

	$ podman run -d --label=io.containers.capabilities=setuid,setgid fedora /usr/bin/server
	$ podman create -d --label=io.containers.capabilities=setuid,setgid fedora /usr/bin/server

	### Run a container with just the SETUID and SETGID Capabilities
	### Create a container with just the SETUID and SETGID Capabilities

Allow devs to set labels in container images for default capabilities. #5206

Allow devs to set labels in container images for default capabilities. #5206

Conversation

rhatdan commented Feb 14, 2020

rhatdan commented Feb 14, 2020

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

rhatdan commented Feb 14, 2020

vrothberg 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

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

rhatdan Feb 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

rh-atomic-bot commented Feb 14, 2020

TomSweeneyRedHat commented Feb 24, 2020

mheon commented Feb 24, 2020

rh-atomic-bot commented Feb 25, 2020

rhatdan commented Feb 25, 2020

rhatdan commented Feb 27, 2020

rh-atomic-bot commented Feb 27, 2020

rhatdan commented Feb 28, 2020

vrothberg 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

vrothberg commented Mar 2, 2020

rhatdan commented Mar 2, 2020

vrothberg left a comment

Choose a reason for hiding this comment

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 3, 2020

rhatdan Feb 23, 2020 •

edited

Loading

rhatdan Feb 29, 2020 •

edited

Loading