specs: Add minimum raw security profiles needed by plugins to ContainerSpec and PluginSpec #1964

cyli · 2017-02-17T04:11:15Z

This adds per-architecture security profiles to the ContainerSpec (it would also have to be added the PluginSpec). These are currently the minimum options needed in security profiles to unblock plugins. This does not cover all the security options that docker run supports.

The linux raw security profile came from the documented plugin config file, under the "Linux" section. Networks, mounts, env vars, and args I assume can also already be mapped to existing ContainerSpec or ServiceSpec fields.

I have not managed to find out what else should go into a raw windows security profile.

This continues the work done in #1944, but is currently only the specs, and not any kind of implementation.

cc @diogomonica @ehazlett @aluzzardi @tiborvass

codecov-io · 2017-02-17T04:20:49Z

Codecov Report

Merging #1964 into master will increase coverage by 0.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1964      +/-   ##
==========================================
+ Coverage   53.47%   53.62%   +0.15%     
==========================================
  Files         109      109              
  Lines       18919    18919              
==========================================
+ Hits        10116    10146      +30     
+ Misses       7570     7535      -35     
- Partials     1233     1238       +5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be6e3d3...76b143b. Read the comment docs.

diogomonica · 2017-02-17T04:26:26Z

api/types.proto

+	}
+}
+
+// WindowsRawSecurityProfile provides any low level privileges a windows container needs


s/any/all ?

diogomonica · 2017-02-17T04:27:41Z

api/types.proto

+	repeated string capabilities = 1;
+
+	// Devices needed by the service
+	repeated Device devices = 2;


does it make sense to have Devices if the only thing that it contains is a path string?

Good point - I left it in because I wasn't sure if we'd need anything else. https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md#devices has a bunch of other options, but https://docs.docker.com/engine/extend/config/ only has the name, description, and path. (I figured the name and description are probably not necessary, but wasn't sure)

Do any plugins require bind mounts?

Yes, mounts is an option - ContainerSpec already has the Mounts field, though, so I thought that could be used instead?

Or are you suggesting that devices could also use the Mounts field?

I would think we would need at least the type, path and major,minor for device support. /cc @justincormack

Docker (as opposed to runc) reads the device information off the host device (so you cannot create a device in the container which does not exist on the host, which is normally ok). It does need the rwm options for the device cgroups as well though, so another string.

Added, thanks @justincormack

diogomonica · 2017-02-17T04:28:08Z

Is this supposed to cover the minimum essential to unlock swarm plugins?

cyli · 2017-02-17T04:32:16Z

@diogomonica Yes, that is the goal. Let me update the description and title.

aaronlehmann · 2017-02-17T05:02:03Z

api/specs.proto

@@ -264,6 +264,9 @@ message ContainerSpec {
 	// task will exit and a new task will be rescheduled elsewhere. A container
 	// is considered unhealthy after `Retries` number of consecutive failures.
 	HealthConfig healthcheck = 16;
+
+	// SecurityProfiles specifies per-architecture security options required by the container
+	SecurityProfiles security_profiles = 21;


Is this the right wording? My understanding was that security profiles would be high-level privilege grants, but ServiceSpecs would contain low level capabilities.

I'm not sure what the right wording should be. My understanding is similar, in that was that these would be low level, and security profiles would be high level grants that would map to low level capabilities.

I believe the current plan is to mapped on the CLI side, but possibly we'd need support for the mapping later in swarm (@aluzzardi brought up the point that it's easier to change types and versions of things in swarm than it is to change CLI option behavior).

The major problem w/ it being in swarm is the expectation between service creation across multiple versions of the mapping.

if a service worked by doing docker service create --grant=networking my-service, and we change swarm's mapping server-side, that same cmd-line might not work in a future version of swarm.

aaronlehmann · 2017-02-17T06:08:28Z

Or are you suggesting that devices could also use the Mounts field?

Just wondering if we should centralize the declarations of what elevated privileges a service requires, including bind mounts.

diogomonica · 2017-02-17T17:19:51Z

@aaronlehmann @cyli yeah, I think aggregating this into a PrivilegeProfile or something of the sort might make sense.

cyli · 2017-02-17T18:00:40Z

Just wondering if we should centralize the declarations of what elevated privileges a service requires, including bind mounts.

I've deprecated the mounts in ContainerSpec, and moved it to the per-architecture specific profiles for now. I guess we need to be able to read from both for a while?

I think aggregating this into a PrivilegeProfile or something of the sort might make sense.

Renamed everything PrivilegeProfile instead of SecurityProfile

aaronlehmann · 2017-02-17T18:05:46Z

I've deprecated the mounts in ContainerSpec, and moved it to the per-architecture specific profiles for now. I guess we need to be able to read from both for a while?

I'm not sure we want to deprecate Mounts in ContainerSpec. I envisioned having actual mount instructions in ContainerSpec, but having mount privileges in a separate place that can easily be checked by some future authorization code. The agent would refuse to act on any mounts that aren't authorized by the privileges structure. I may be thinking about this the wrong way and encouraging an excessively complicated solution. I'd be curious to hear what @diogomonica thinks.

diogomonica · 2017-02-17T18:43:18Z

@aaronlehmann having privileges in one place, and actual mount requests in a different place, is a recipe for authZ problems.

The most obvious example being the creation of a service spec that doesn't request privileges, but includes mounts, and the authz code merely checks if privileges for mount were requested, allowing it to pass by unchecked.

cyli · 2017-02-17T19:14:15Z

api/types.proto

+	repeated Mount mounts = 1 [(gogoproto.nullable) = false];
+
+	// Linux capabilities required by the container
+	repeated string capabilities = 2;


Rather than string, should we just enumerate the caps listed here: https://github.com/opencontainers/runc/blob/master/libcontainer/SPEC.md#security ?

Are you proposing it actually being an enum? What would be the path for dealing with changes in capabilities?

Slightly concerned that inspecting profiles directly becomes a bit harder, but it will definitely be a lot more compressed on the wire this way.

If more capabilities were added, then we could add them to the enum? Are capabilities in linux every deprecated/removed?

I don't feel strongly that it should be an enum - just thought it'd be easier to check if there were invalid capabilities, etc. I'm fine with leaving it a string.

nothing in Linux is ever removed. They might become useless eventually. If you are going to use an enum you should probably use the real values http://lxr.free-electrons.com/source/include/uapi/linux/capability.h#L96 onwards

I'm down w/ enum

justincormack · 2017-02-17T21:05:36Z

Sorry what "mount privileges" are being discussed?

cyli · 2017-02-21T23:16:21Z

@justincormack @diogomonica Added the caps as an enum on the profile. PTAL

So do the mount instructions in the privileges section make sense, or should I move them back to container spec?

aaronlehmann · 2017-02-21T23:49:04Z

api/types.proto

@@ -883,4 +883,4 @@ message MaybeEncryptedRecord {
 	Algorithm algorithm = 1;
 	bytes data = 2;
 	bytes nonce = 3;
-}
+}


Final newline got removed here

aaronlehmann · 2017-02-21T23:57:46Z

So do the mount instructions in the privileges section make sense, or should I move them back to container spec?

I don't think they make sense in the privileges section, because they include a lot that's not related at all to privileges, such as target path inside the container, and driver options.

My thought here was to keep the mount instructions where they are now, but require a corresponding privilege grant in PrivilegeProfiles for each one. These grants could be checked by authZ code. #1964 (comment)

@diogomonica didn't seem positive about this:

having privileges in one place, and actual mount requests in a different place, is a recipe for authZ problems.

The most obvious example being the creation of a service spec that doesn't request privileges, but includes mounts, and the authz code merely checks if privileges for mount were requested, allowing it to pass by unchecked.

My thinking about this case is that the agent must refuse to act on any mount instructions that aren't accompanied by a privilege grant.

diogomonica · 2017-02-22T00:49:44Z

If you feel strongly about it, I'm ok w/ that tradeoff @aaronlehmann

cyli · 2017-02-22T00:51:35Z

Since both have to come through the specs, one in the container spec, one in the privilege profiles which are also in the container spec, would that be just copying the same stuff in 2 places? Since both have to come from the user?

aaronlehmann · 2017-02-22T00:56:06Z

If you feel strongly about it, I'm ok w/ that tradeoff

No, I don't feel strongly about it. Just trying to find the best tradeoff. The privilege profiles don't seem like the right place to place detailed mount instructions with driver options and so on. And it doesn't seem right either to have authZ code look into the ContainerSpec to see what mounts a service is requesting and make sure those mounts are okay. But I'm not sure what's best here and none of the options would be horrible for me.

Since both have to come through the specs, one in the container spec, one in the privilege profiles which are also in the container spec, would that be just copying the same stuff in 2 places? Since both have to come from the user?

It would be slightly redundant but the ContainerSpec would include detailed parameters for the mount, and PrivilegeProfile would only say "this service wants to mount /path".

anusha-ragunathan · 2017-02-22T18:55:02Z

api/privileges.proto

+	// Devices needed by the service
+	repeated Device devices = 3;
+
+	// Rather than provide a list of all devices, just mount every device on the node


If this is the equivalent of allow-all-devices, then the comment should read allow read write and modify access to all devices on the node.

Also better to rename the boolean to allow-all-devices

Will do, thanks!

anusha-ragunathan · 2017-02-22T19:15:05Z

PR takes care of adding the profiles to PluginSpec as well. Can you please update the commit title and description accordingly?

tiborvass · 2017-02-22T19:31:32Z

So I have an important bikeshedding problem #oxymoron.

First, PrivilegeProfile make me think that everything that is privileged is in there, but I am not sure if that's the case. Bind mounts? I start to think it's more like a HostConfig.

I also wonder if we shouldn't just not have a name at all and simply have LinuxCapabilities under a Linux field. Simply embed in ContainerSpec the capabilities and devices field, and same for pluginspec.

I can't seem to find a satisfying solution :S

tiborvass · 2017-02-22T19:52:38Z

api/privileges.proto

+
+
+// PrivilegeProfiles specifies per-architecture security configuration/permissions
+message PrivilegeProfiles {


If we decide to go with this name, shouldn't this be singular?

It's plural because it has profiles for both windows and linux.

diogomonica · 2017-02-22T21:28:29Z

@tiborvass where will you put the sec comp profile? Inside LinuxCapabilities?

tiborvass · 2017-02-22T21:30:42Z

@diogomonica a field named Seccomp of type LinuxSeccomp, under Linux field of the spec.

diogomonica · 2017-02-22T21:47:40Z

/shrug

tiborvass · 2017-02-22T21:51:27Z

@diogomonica basically I'm wondering if it's fine to not "bundle" the various fields like Capabilities, Devices, Seccomp into one object that we have trouble finding a name for because it wouldn't be complete.

diogomonica · 2017-02-22T21:55:59Z

@aluzzardi has opinions

cyli · 2017-02-22T22:04:02Z

@tiborvass: just to summarize, is your proposal:

message ContainerSpec {
    ...
    LinuxOptions linux
    WindowsOptions windows
}

message LinuxOptions {
    repeated Capabilities capabilities
    repeated Devices devices
    bool allow_all_devices
}

message WindowsOptions {
    string credential_specs
}

And similarly for plugin spec? (I have no strong opinions - happy to change if folks feel this works better)

tiborvass · 2017-02-22T22:09:43Z

@cyli yes. Not saying that's the best thing. Just that, we did something similar for plugins. If we are happy with a name for PrivilegeProfile then that's fine too.

aluzzardi · 2017-02-23T01:47:30Z

Considering this is flat in docker/docker (the platform part), should we do the same here? e.g. have Capabilities at the top level which would be ignored by windows.

cyli · 2017-02-23T02:40:22Z

My preference, without any additional context, is have it in sub objects, mainly because there'd be a lot of duplication otherwise between the plugin spec and the container spec.

Within the context of translatation from swarm to docker, it seems like some translation already needs to happen for a full list of capabilities -> CapAdd and CapDrop, and from strings to the enum.

Alternately, do we want just want to copy and paste CapAdd, CapDrop (repeated strings), and Devices from the HostConfig for easier translation, instead of providing the full list of capabilities? The plugin config looks like it specifies only CapAdd: https://github.com/docker/docker/blob/master/plugin/v2/plugin_linux.go#L118.

diogomonica · 2017-02-23T04:03:26Z

@aluzzardi shouldn't this be in TaskSpec instead of PluginSpec? Particularly if this will be shared by ContainerSpec later for services.

cyli · 2017-02-23T22:46:06Z

@diogomonica Oh that's true - it could go in the TaskSpec instead of both the ContainerSpec and PluginSpec - it just wouldn't apply to the NetworkAttachmentSpec runtime.

So are we ok with just going with CapAdd and CapDrop instead of the full list of Capabilities?

diogomonica · 2017-02-24T00:52:45Z

@cyli is the goal to allow some kind of default? I'm worried that by going w/ cap add and cap drop we don't get to a world where the AuthZ plugin has all the information needed to do it's job, and has to rely on some external knowledge of what the current "default" is, and if current denied capability Y is enabled by not dropping it.

cyli · 2017-02-24T01:38:05Z

@diogomonica Yes, to allow the docker default, and also to make translation easier with the docker.

Agree that it's not ideal that the authz plugin would have to know about what the current docker default is, and also this means that upgrading a docker version could potentially what capabilities a service is configured with (although that's currently the case already I think).

Although possibly #1744 could help with that (if the interpolated specs interpolated the defaults (and I guess had an extra field that was the full list of capabilities?)

diogomonica · 2017-02-26T22:46:08Z

Why are we trying not o have the full list of caps in the first place?

justincormack · 2017-02-26T23:05:14Z

The problem is that the docker client does not know what the full cap set is, as it is a daemon side decision (at present a constant one but this really needs to change). For plugins they need to specify the full set so it is ok but this does not match up with normal containers

…

On 26 Feb 2017 14:46, "Diogo Mónica" ***@***.***> wrote: Why are we trying not o have the full list of caps in the first place? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1964 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdcPJfrKThiqy8E5fkRXvEU_6yRtePUks5rggCygaJpZM4MD2ma> .

diogomonica · 2017-02-28T17:52:39Z

@justincormack shouldn't we have a mechanism to provide the recommended full profile an image has to run with? Ideally this would come inside of the image itself, when pulling from the hub, but adding it to the API and having the remote AuthZ plugin do the intersection between both might not be a bad way to go forward.

cyli · 2017-03-23T23:37:02Z

Just double-checking - should this be closed? Are we going to go with opaque plugin blobs and maybe Docker's HostConfig instead?

aluzzardi · 2017-03-23T23:58:25Z

Isn't this related to profiles, in a way? We'll probably have to have something similar somewhere

cyli · 2017-03-24T00:04:41Z

@aluzzardi Profiles/high level grants I think are going to be mainly a CLI mapping for now. Possibly we want to update the HostConfig in Docker eventually? cc @diogomonica

aaronlehmann · 2017-04-11T23:23:26Z

I sort of lost track of how this fits in with everything else. It seems to overlap with #2075, but #2075 did not add the capabilities. If I understood correctly, capabilities are a hard requirement for 17.06. It sounds like we should either resurrect this to add the capabilities or open something separate, perhaps drawing on the work that took place here.

cc @aluzzardi

cyli · 2017-07-15T01:35:36Z

I think this is no longer needed as plugins will be an opaque GenericRuntimeSpec and others should use https://github.com/docker/libentitlement.

cyli force-pushed the caps-in-service-spec branch from c8d3cff to 6aa749c Compare February 17, 2017 04:13

diogomonica reviewed Feb 17, 2017

View reviewed changes

cyli changed the title ~~specs: Add security profiles to ContainerSpec~~ specs: Add minimum raw security profiles needed by plugins to ContainerSpec Feb 17, 2017

cyli force-pushed the caps-in-service-spec branch 2 times, most recently from 6ab5d00 to 5607598 Compare February 17, 2017 04:42

aaronlehmann reviewed Feb 17, 2017

View reviewed changes

cyli force-pushed the caps-in-service-spec branch from 5607598 to 24b72b5 Compare February 17, 2017 17:59

cyli commented Feb 17, 2017

View reviewed changes

cyli force-pushed the caps-in-service-spec branch from 24b72b5 to 654985c Compare February 21, 2017 23:14

cyli force-pushed the caps-in-service-spec branch 3 times, most recently from 2f11893 to 0ad78b5 Compare February 21, 2017 23:37

aaronlehmann reviewed Feb 21, 2017

View reviewed changes

cyli force-pushed the caps-in-service-spec branch 2 times, most recently from 8e9adc2 to 3cdbd23 Compare February 22, 2017 00:42

anusha-ragunathan reviewed Feb 22, 2017

View reviewed changes

tiborvass reviewed Feb 22, 2017

View reviewed changes

cyli changed the title ~~specs: Add minimum raw security profiles needed by plugins to ContainerSpec~~ specs: Add minimum raw security profiles needed by plugins to ContainerSpec and PluginSpec Feb 22, 2017

cyli mentioned this pull request Feb 22, 2017

Add support for devices with "service create" #1244

Open

ehazlett mentioned this pull request Mar 10, 2017

Swarm-wide plugin deployment and lifecycle management moby/moby#31401

Open

aluzzardi mentioned this pull request Mar 30, 2017

Privilege Profile support #2075

Merged

cyli closed this Jul 15, 2017



		// PrivilegeProfiles specifies per-architecture security configuration/permissions
		message PrivilegeProfiles {

specs: Add minimum raw security profiles needed by plugins to ContainerSpec and PluginSpec #1964

specs: Add minimum raw security profiles needed by plugins to ContainerSpec and PluginSpec #1964

Conversation

cyli commented Feb 17, 2017 • edited Loading

codecov-io commented Feb 17, 2017 • edited Loading

Codecov Report

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

ehazlett Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diogomonica commented Feb 17, 2017

cyli commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diogomonica Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

aaronlehmann commented Feb 17, 2017 via email

diogomonica commented Feb 17, 2017

cyli commented Feb 17, 2017

aaronlehmann commented Feb 17, 2017

diogomonica commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justincormack commented Feb 17, 2017

cyli commented Feb 21, 2017

Choose a reason for hiding this comment

aaronlehmann commented Feb 21, 2017

diogomonica commented Feb 22, 2017

cyli commented Feb 22, 2017

aaronlehmann commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anusha-ragunathan commented Feb 22, 2017

tiborvass commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diogomonica commented Feb 22, 2017

tiborvass commented Feb 22, 2017 • edited Loading

diogomonica commented Feb 22, 2017

tiborvass commented Feb 22, 2017

diogomonica commented Feb 22, 2017

cyli commented Feb 22, 2017

tiborvass commented Feb 22, 2017

aluzzardi commented Feb 23, 2017

cyli commented Feb 23, 2017

diogomonica commented Feb 23, 2017

cyli commented Feb 23, 2017

diogomonica commented Feb 24, 2017

cyli commented Feb 24, 2017 • edited Loading

diogomonica commented Feb 26, 2017

justincormack commented Feb 26, 2017 via email

diogomonica commented Feb 28, 2017

cyli commented Mar 23, 2017 • edited Loading

aluzzardi commented Mar 23, 2017

cyli commented Mar 24, 2017

aaronlehmann commented Apr 11, 2017

cyli commented Jul 15, 2017

cyli commented Feb 17, 2017 •

edited

Loading

codecov-io commented Feb 17, 2017 •

edited

Loading

ehazlett Feb 17, 2017 •

edited

Loading

diogomonica Feb 17, 2017 •

edited

Loading

cyli Feb 17, 2017 •

edited

Loading

tiborvass commented Feb 22, 2017 •

edited

Loading

cyli commented Feb 24, 2017 •

edited

Loading

cyli commented Mar 23, 2017 •

edited

Loading