Add ambient and bounding capability support #675

crosbymichael · 2017-02-02T22:58:16Z

Closes #668

Signed-off-by: Michael Crosby [email protected]

wking · 2017-02-02T23:17:21Z

schema/config-schema.json

-                        "$ref": "defs-linux.json#/definitions/Capability"
-                    }
+                    "type": "object",
+					"properties": {


Probably want to reformat this to get consistent indents (make -C schema fmt).

wking · 2017-02-02T23:19:26Z

schema/config-schema.json

+					"properties": {
+						"bounding": {
+							"id": "https://opencontainers.org/schema/bundle/process/linux/capabilities/bounding",
+							"$ref": "defs-linux.json#/definitions/Capability"


This (and ambient) should still be arrays. So:

"type": "array", "items": { "$ref": "defs-linux.json#/definitions/Capability" }

instead of the line I'm commenting on.

wking · 2017-02-02T23:21:17Z

config.md

  Valid values are the strings for capabilities defined in [the man page](http://man7.org/linux/man-pages/man7/capabilities.7.html).
+  capabilities contains the following properties:
+    * **`bounding`** (array of strings, OPTIONAL) - the 'bounding' field is the whitelist of bounding capabilities that are kept for the process.
+	* **`ambient`** (array of strings, OPTIONAL) - the 'ambient' field is the whitelist of ambient capabilities that are kept for the process.


Inconsistent indent? And you can probably drop “the '…' field is the” and just start in:

ambient (array of strings, OPTIONAL) A whitelist of …

wking · 2017-02-02T23:26:39Z

config.md

-        "CAP_NET_BIND_SERVICE"
-    ],
+    "capabilities": {
+		"bounding": [


Inconsistent indent (all the rest of the examples use four spaces).

justincormack · 2017-02-03T10:22:40Z

LGTM

mrunalp · 2017-02-03T17:58:13Z

While this looks good, we do manipulate the inherited, permitted and effective sets as well in runc. Should we make a statement about that or rename bounding to something else?

crosbymichael · 2017-02-06T18:44:32Z

@mrunalp I guess if we want to be uber explicit with the settings we can add all the fields but I don't think its totally useful for most containers. Everyone will set the additional fields the same anyways so keeping a bounding set makes things much more straightforward.

justincormack · 2017-02-06T18:47:07Z

I think I can imagine a situation where you want permitted but not effective or inherited, eg you want to allow some suid executables to work, but the capabilities to be only executed by those, not by the initial process. So I think its probably better to have all four sets, even if most applications just set e=i=p.

crosbymichael · 2017-02-07T00:46:47Z

@mrunalp what do you think? Should we add the rest and remove bounding?

Anyone else have any input on this?

mrunalp · 2017-02-07T04:10:54Z

@crosbymichael We need ambient and bounding for sure. If we add the other 3 sets as well then we will have to make a statement that the runtime will apply these to the init process before it execs the user specified process. The capabilities could change on the basis of being root or not, executed binary being setuid or not and having any filesystem capabilities.

crosbymichael · 2017-02-08T18:46:22Z

@mrunalp not much of a difference from today right?

mrunalp · 2017-02-08T19:44:17Z

@crosbymichael Yes, that's what we do today. I think we can just add all the sets and leave it up to the user to set them correctly. For e.g. whatever is set in ambient has to be in both permitted and inherited. We can just point the user to the man page for capabilities.

crosbymichael · 2017-02-08T20:02:11Z

Ok, I updated this PR with the changes

mrunalp · 2017-02-08T20:17:06Z

config.md

+    * **`effective`** (array of strings, OPTIONAL) - the 'bounding' field is the whitelist of bounding capabilities that are kept for the process.
+    * **`inheritable`** (array of strings, OPTIONAL) - the 'bounding' field is the whitelist of bounding capabilities that are kept for the process.
+    * **`permitted`** (array of strings, OPTIONAL) - the 'bounding' field is the whitelist of bounding capabilities that are kept for the process.
+    * **`ambient`** (array of strings, OPTIONAL) - the 'ambient' field is the whitelist of ambient capabilities that are kept for the process.


We need bounding as well.

mrunalp · 2017-02-08T21:36:06Z

config.md

+                "CAP_AUDIT_WRITE",
+                "CAP_KILL",
+            ],
+            "ambient": [


Just for correctness, we will have set permitted/inheritable as well to atleast the same caps as ambient. We could set p/i/e to same as bounding in this example.

Actually ambient has a different cap so would need to include that as well in p/i and optionally in e as well.

justincormack · 2017-02-13T14:34:55Z

config.md

+    "capabilities": {
+        "bounding": [
+            "CAP_AUDIT_WRITE",
+            "CAP_KILL",


the example should have CAP_NET_BIND_SERVICE in the bounding set or it cannot be applied as an ambient capability.

CAP_NET_BIND_SERVICE also needs to be added to permitted/inherited.
From capabilities(7)

The ambient capability set obeys the invariant that no capability can ever be ambient if it is not both permitted and inheritable.

@mrunalp does that mean we should or should not have it in bounding?

@crosbymichael We should have it in bounding, permitted and inheritable.

crosbymichael · 2017-02-17T18:05:35Z

@mrunalp fixed.

mrunalp · 2017-02-17T18:12:26Z

LGTM

wking · 2017-02-17T18:19:41Z

config.md

@@ -132,7 +132,13 @@ For Windows, see links for details about [mountvol](http://ss64.com/nt/mountvol.
 * **`env`** (array of strings, OPTIONAL) with the same semantics as [IEEE Std 1003.1-2001's `environ`][ieee-1003.1-2001-xbd-c8.1].
 * **`args`** (array of strings, REQUIRED) with similar semantics to [IEEE Std 1003.1-2001 `execvp`'s *argv*][ieee-1003.1-2001-xsh-exec].
  This specification extends the IEEE standard in that at least one entry is REQUIRED, and that entry is used with the same semantics as `execvp`'s *file*.
-* **`capabilities`** (array of strings, OPTIONAL) is an array that specifies the set of capabilities of the process(es) inside the container. Valid values are platform-specific. For example, valid values for Linux are defined in the [CAPABILITIES(7)](http://man7.org/linux/man-pages/man7/capabilities.7.html) man page.
+* **`capabilities`** (object of strings, OPTIONAL) is an array that specifies the set of capabilities of the process(es) inside the container. Valid values are platform-specific. For example, valid values for Linux are defined in the [CAPABILITIES(7)](http://man7.org/linux/man-pages/man7/capabilities.7.html) man page.


I don't see any precedence for “object of …”, and would recommend just using “object” (which is what we use for annotations).

wking · 2017-02-17T18:23:28Z

config.md

-* **`capabilities`** (array of strings, OPTIONAL) is an array that specifies the set of capabilities of the process(es) inside the container. Valid values are platform-specific. For example, valid values for Linux are defined in the [CAPABILITIES(7)](http://man7.org/linux/man-pages/man7/capabilities.7.html) man page.
+* **`capabilities`** (object of strings, OPTIONAL) is an array that specifies the set of capabilities of the process(es) inside the container. Valid values are platform-specific. For example, valid values for Linux are defined in the [CAPABILITIES(7)](http://man7.org/linux/man-pages/man7/capabilities.7.html) man page.
+  capabilities contains the following properties:
+    * **`effective`** (array of strings, OPTIONAL) - the 'effective' field is the whitelist of effective capabilities that are kept for the process.


We usually use backticks around property names, so “'effective'” → “effective” (and similarly in the following lines). Grepping for ' in our *.md, I see no examples of single-quoted property names in master:

$ git grep "'[^' ]*'" origin/master -- *.md origin/master:config.md:For Solaris, the mount entry corresponds to the 'fs' resource in zonecfg(8). origin/master:config.md: As an example, the ['no_new_privs' kernel doc](https://www.kernel.org/doc/Documentation/prctl/no_new_privs.txt) has more information on how this is achieved using a prctl system call on Linux.

Both of those are references to external stuff (not OCI properties), and I think they should have been backticked as well ;).

Also, the new “whitelist” language would require the removal of this runtime-tools check and give configuration authors no way to clearly drop a given capability. I'd rather we leave these arrays as “this will be the set of process capabilities after create finishes” and just focus on adding the additional sets in this PR. We can address the change from “this set” to “at least this set” in a separate PR.

wking · 2017-02-17T18:32:11Z

specs-go/config.go

-	// Capabilities are Linux capabilities that are kept for the container.
-	Capabilities []string `json:"capabilities,omitempty" platform:"linux"`
+	// Capabilities are Linux capabilities that are kept for the process.
+	Capabilities *LinuxCapabilities `json:"capabilities,omitempty" platform:"linux"`


@jlbutler just removed the Linux-specific-ness of process.capabilities in #673 (at least in the Markdown spec). With this change, it becomes even less clear to me how other OSes are supposed to configure their capabilities (although I wasn't clear on that since #673, so it's not a lot worse ;).

No other systems have capabilities; there was a withdrawn Posix draft that the Linux implementation was based on, and then customised for Linux. They should have remained Linux specific.

No other systems have capabilities…

capabilies(7) references this for the withdrawn POSIX.1e draft, and that has:

Why Posix.1e was abandoned is difficult to understand from today's (July 2014) point of view. Solaris, Irix, Linux, and probably other Unices seemed to recognize the standard. On the other hand the FreeBSD project found counter arguments and didn't integrate capabilities ('fine grained privileges') by default.

I don't have access to Solaris or Irix for testing, or enough interest in them to find relevant docs, but I'd caution against making this Linux-specific (again) without consulting someone with Solaris access. On the other hand, maybe the policy is “unless you document your valid values, we may drop support for this property on your system”, and I'd be +1 for that, giving the Solaris folks some reasonable period to provide the valid-values docs, and then dropping Solaris capability support if they don't.

But here we are adding the Linux specific types such as ambient. Solaris capabilities are a bit similar by the look of it (they have E, I, P but different capabilities) but that doesn't mean they should be at the top level.

Solaris docs are http://docs.oracle.com/cd/E23824_01/html/821-1456/prbac-2.html

Solaris capabilities are a bit similar by the look of it (they have E, I, P but different capabilities) but that doesn't mean they should be at the top level.

Is that “I don't think there is enough overlap to be worth sharing the same namespace”? If the Solaris folks are ok using bounding instead of their limited, we could just make ambient Linux-specific. In other situations where only the values are different (e.g. mounts entries), the maintainer preference seems to be “share the same property, but define the different per-platform values”.

On the other hand, if Windows has nothing equivalent, the pro-namespacing policy here suggests namespacing the whole capabilities structure to something POSIX-ish-specific. I tried to raise the “how will this be handled on Windows?” issue in #673, but am still not clear on if/whether Windows supports something capability-like.

crosbymichael · 2017-02-22T18:41:03Z

@mrunalp @hqhq pushed another update to fix last bits of feedback

mrunalp · 2017-02-22T19:38:09Z

config.md

@@ -132,7 +132,13 @@ For Windows, see links for details about [mountvol](http://ss64.com/nt/mountvol.
 * **`env`** (array of strings, OPTIONAL) with the same semantics as [IEEE Std 1003.1-2001's `environ`][ieee-1003.1-2001-xbd-c8.1].
 * **`args`** (array of strings, REQUIRED) with similar semantics to [IEEE Std 1003.1-2001 `execvp`'s *argv*][ieee-1003.1-2001-xsh-exec].
  This specification extends the IEEE standard in that at least one entry is REQUIRED, and that entry is used with the same semantics as `execvp`'s *file*.
-* **`capabilities`** (array of strings, OPTIONAL) is an array that specifies the set of capabilities of the process(es) inside the container. Valid values are platform-specific. For example, valid values for Linux are defined in the [CAPABILITIES(7)](http://man7.org/linux/man-pages/man7/capabilities.7.html) man page.
+* **`capabilities`** (object, OPTIONAL) is an array that specifies the set of capabilities of the process(es) inside the container. Valid values are platform-specific. For example, valid values for Linux are defined in the [CAPABILITIES(7)](http://man7.org/linux/man-pages/man7/capabilities.7.html) man page.


Can we reword is an array to is an object containing arrays of capability types or something similar?

mrunalp · 2017-02-22T21:50:34Z

LGTM

hqhq · 2017-02-22T21:54:04Z

config.md

+    * **`bounding`** (array of strings, OPTIONAL) - the `bounding` field is a array of bounding capabilities that are kept for the process.
+    * **`inheritable`** (array of strings, OPTIONAL) - the `inheritable` field is a array of inheritable capabilities that are kept for the process.
+    * **`permitted`** (array of strings, OPTIONAL) - the `permitted` field is a array of permitted capabilities that are kept for the process.
+    * **`ambient`** (array of strings, OPTIONAL) - the `ambient` field is a array of ambient capabilities that are kept for the process.


LGTM when typo is fixed

Closes opencontainers#668 Signed-off-by: Michael Crosby <[email protected]>

crosbymichael · 2017-02-22T22:48:24Z

@hqhq @dqminh fixed the typos

hqhq · 2017-02-22T22:49:46Z

LGTM

mrunalp · 2017-02-22T22:51:03Z

LGTM

dqminh · 2017-02-22T22:51:49Z

LGTM

Fix a JSON typo which snuck in with eb114f0 (Add ambient and bounding capability support, 2017-02-02, opencontainers#675). Signed-off-by: W. Trevor King <[email protected]>

…gain) Roll back the genericization from 718f9f3 (minor narrative cleanup regarding config compatibility, 2017-01-30, opencontainers#673). Lifting the restriction there seems to have been motivated by "Solaris supports capabilities", but that was before the split into a capabilities object which happened in eb114f0 (Add ambient and bounding capability support, 2017-02-02, opencontainers#675). It's not clear if Solaris supports ambient caps, or what Solaris API rlimits or noNewPrivileges were punting to [1]. And John Howard has recently confirmed that Windows does not support capabilities and is unlikely to do so in the future [2]. John's statement didn't directly address rlimits or noNewPrivileges, but we can always restore any of these properties to the Solaris/Windows platforms if/when we get docs about which API we're punting to on those platforms. Also add some backticks, remove the hyphens in "OPTIONAL) - the", standardize lines I touch to use "the process" [3], and use four-space indents here to keep Pandoc happy (see 7795661 (runtime.md: Fix sub-bullet indentation, 2016-06-08, opencontainers#495). [1]: opencontainers#673 (comment) [2]: opencontainers#810 (comment) [3]: opencontainers#809 (comment) Signed-off-by: W. Trevor King <[email protected]>

Roll back the genericization from 718f9f3 (minor narrative cleanup regarding config compatibility, 2017-01-30, opencontainers#673). Lifting the restriction there seems to have been motivated by "Solaris supports capabilities", but that was before the split into a capabilities object which happened in eb114f0 (Add ambient and bounding capability support, 2017-02-02, opencontainers#675). It's not clear if Solaris supports ambient caps, or what Solaris API noNewPrivileges were punting to [1]. And John Howard has recently confirmed that Windows does not support capabilities and is unlikely to do so in the future [2]. He also confirmed that Windows does not support rlimits [3]. John's statement didn't directly address noNewPrivileges, but we can always restore any of these properties to the Solaris/Windows platforms if/when we get docs about which API we're punting to on those platforms. Also add some backticks, remove the hyphens in "OPTIONAL) - the", standardize lines I touch to use "the process" [4], and use four-space indents here to keep Pandoc happy (see 7795661 (runtime.md: Fix sub-bullet indentation, 2016-06-08, opencontainers#495). [1]: opencontainers#673 (comment) [2]: opencontainers#810 (comment) [3]: opencontainers#835 (comment) [4]: opencontainers#809 (comment) Signed-off-by: W. Trevor King <[email protected]>

crosbymichael force-pushed the caps branch from 110d793 to 7639653 Compare February 2, 2017 23:00

wking reviewed Feb 2, 2017

View reviewed changes

crosbymichael force-pushed the caps branch from 7639653 to 3487dff Compare February 2, 2017 23:20

wking reviewed Feb 2, 2017

View reviewed changes

crosbymichael force-pushed the caps branch 2 times, most recently from d9d6641 to 7e6e147 Compare February 2, 2017 23:22

wking reviewed Feb 2, 2017

View reviewed changes

crosbymichael force-pushed the caps branch from 7e6e147 to 1223f59 Compare February 3, 2017 00:44

justincormack approved these changes Feb 3, 2017

View reviewed changes

justincormack mentioned this pull request Feb 3, 2017

State when the runtime should and must not apply Linux ambient capabities #668

Closed

crosbymichael force-pushed the caps branch 2 times, most recently from e4e08fc to c119f26 Compare February 8, 2017 20:01

mrunalp reviewed Feb 8, 2017

View reviewed changes

crosbymichael force-pushed the caps branch 2 times, most recently from 359352a to b26f15e Compare February 8, 2017 21:13

mrunalp reviewed Feb 8, 2017

View reviewed changes

crosbymichael force-pushed the caps branch 2 times, most recently from 5d7188d to 84955e1 Compare February 9, 2017 21:29

justincormack reviewed Feb 13, 2017

View reviewed changes

crosbymichael force-pushed the caps branch from 84955e1 to bf4bb4f Compare February 13, 2017 17:40

wking reviewed Feb 17, 2017

View reviewed changes

crosbymichael force-pushed the caps branch from 8c09a64 to d02c140 Compare February 17, 2017 19:07

mrunalp reviewed Feb 22, 2017

View reviewed changes

crosbymichael force-pushed the caps branch from d02c140 to e3824c4 Compare February 22, 2017 21:42

hqhq reviewed Feb 22, 2017

View reviewed changes

Add ambient and bounding capability support

eb114f0

Closes opencontainers#668 Signed-off-by: Michael Crosby <[email protected]>

crosbymichael force-pushed the caps branch from e3824c4 to eb114f0 Compare February 22, 2017 22:47

mrunalp merged commit ac9f8e0 into opencontainers:master Feb 22, 2017

crosbymichael deleted the caps branch February 22, 2017 23:00

vbatts mentioned this pull request Mar 6, 2017

Bump version #710

Merged

wking mentioned this pull request Mar 6, 2017

config: Remove trailing comma in CAP_KILL examples #711

Merged

wking mentioned this pull request May 18, 2017

config: Make capabilities, noNewPrivileges, and rlimits Linux-only (again) #835

Closed

jodh-intel mentioned this pull request May 18, 2017

vendor: Update runtime-spec package for compatibility with CRI-O containers/virtcontainers#259

Merged

lucab mentioned this pull request Jul 20, 2017

caps: wishlists and filling gaps oracle/railcar#1

Closed

redbaron mentioned this pull request Dec 15, 2017

Always use ambient capabilities if no new privileges set, not otherwise opencontainers/runc#1286

Closed

jcberthon mentioned this pull request Feb 14, 2018

Can't bind to privileged ports as non-root moby/moby#8460

Closed

alban mentioned this pull request Jul 25, 2018

effective and permitted capability sets cannot be taken into account #979

Closed

Add ambient and bounding capability support #675

Add ambient and bounding capability support #675

Conversation

crosbymichael commented Feb 2, 2017

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

justincormack commented Feb 3, 2017

mrunalp commented Feb 3, 2017

crosbymichael commented Feb 6, 2017

justincormack commented Feb 6, 2017

crosbymichael commented Feb 7, 2017

mrunalp commented Feb 7, 2017

crosbymichael commented Feb 8, 2017

mrunalp commented Feb 8, 2017

crosbymichael commented Feb 8, 2017

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

crosbymichael commented Feb 17, 2017

mrunalp commented Feb 17, 2017 • edited by caniszczyk 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

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

crosbymichael commented Feb 22, 2017

Choose a reason for hiding this comment

mrunalp commented Feb 22, 2017 • edited by caniszczyk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crosbymichael commented Feb 22, 2017

hqhq commented Feb 22, 2017 • edited by caniszczyk Loading

mrunalp commented Feb 22, 2017 • edited by caniszczyk Loading

dqminh commented Feb 22, 2017 • edited by caniszczyk Loading

mrunalp commented Feb 17, 2017 •

edited by caniszczyk

Loading

mrunalp commented Feb 22, 2017 •

edited by caniszczyk

Loading

hqhq commented Feb 22, 2017 •

edited by caniszczyk

Loading

mrunalp commented Feb 22, 2017 •

edited by caniszczyk

Loading

dqminh commented Feb 22, 2017 •

edited by caniszczyk

Loading