Privilege Profile support #2075

aluzzardi · 2017-03-30T01:31:53Z

Replaces #2070
Related to #1964 #1944

Trying to resurrect the past attempts at this. The main requirements are selinux and credential spec which are blockers - seccomp and apparmor are nice to have.

I mapped what we have in docker's --security-opt in here. Since we need a couple of those options I figured I could port everything that's in there (that's why I left capabilities out for the time being).

Is this the right direction?
For credential spec, in docker we add an URI (either file://path or registry://key). Do we want to continue in the same direction or do we want to just embed the blob?
For SELinux, in docker we are using a micro format at the API level (--security-opt="label:user:USER"). Here I tried to make that strongly typed
For seccomp and apparmor, I'm assuming we're using a profile already loaded on the machine.

/cc @cyli @ehazlett @johnstep @aaronlehmann @dhiltgen @diogomonica @tiborvass

aluzzardi · 2017-03-30T01:34:58Z

Further notes: In #1964 we were trying to group options by OS. I initially agreed but I feel like it might be overkill. It's relatively safe to assume that those options apply only to supported platforms.

After all, it's not just because you're running linux that SELinux or AppArmor options are going to work: you actually need host level support.

aluzzardi · 2017-03-30T01:37:56Z

@diogomonica @cyli In the original PR, we were encapsulating those options into a "raw" object (which I assume paved the road to entitlements).

Could you shed some light?

aluzzardi · 2017-03-30T01:39:39Z

For seccomp and apparmor, I assume the profiles are available locally.

Maybe at some point we might want to inject them (and I am not sure how feasible that is - I don't think we can just inject new stuff).

Anyway, that's not happening now for sure. Do we need to mark them as "local"? Or are we good with this interpretation?

codecov · 2017-03-30T01:41:18Z

Codecov Report

Merging #2075 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2075      +/-   ##
==========================================
- Coverage   57.25%   57.16%   -0.09%     
==========================================
  Files         117      117              
  Lines       20124    20124              
==========================================
- Hits        11521    11503      -18     
- Misses       7273     7292      +19     
+ Partials     1330     1329       -1

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 209c29e...cac0545. Read the comment docs.

cpuguy83 · 2017-03-30T01:59:33Z

For seccomp, we take the profile directly from the client.

aluzzardi · 2017-03-30T02:23:37Z

@cpuguy83 say again? Where is it stored?

AkihiroSuda · 2017-03-30T05:08:07Z

How #1722 relates to this PR?

Also, I guess 8000 CapAdd/CapDrop is intentionally omitted from this PR, but could you please elaborate the current plan about supporting CapAdd/CapDrop?

cpuguy83 · 2017-03-30T12:34:49Z

@aluzzardi example: --security-opt seccomp:/path/to/profile. In this case the profile would be on the client, it is read in and passed as configuration. The microformats of microformats.

cyli · 2017-03-30T20:41:38Z

@aluzzardi - Yes the original plan was for the privilege profile to contain a full list of the raw capabilities to be passed to the service, and the entitlements would be a UI feature that generated that list.

I think I may be behind on what we're currently planning on doing though - AFAIK for plugins, will the the spec object just be the raw object from docker? If so, I think that includes the docker object HostConfig which includes the security opts, etc? Is this going to work differently for containers in swarmkit, which would use this spec instead?

aaronlehmann · 2017-03-31T21:44:20Z

For credential spec, in docker we add an URI (either file://path or registry://key). Do we want to continue in the same direction or do we want to just embed the blob?

How about making this more strongly typed by making it a oneof? We can start with fields for a file path or registry path. If we decide to support embedding the blob, that could be a third field we add later.

For seccomp and apparmor, I'm assuming we're using a profile already loaded on the machine.

Sounds like another situation where we might appreciate having a oneof later.

friism · 2017-04-01T01:20:22Z

@PatrickLang do you have feedback on this?

For credential spec, in docker we add an URI (either file://path or registry://key). Do we want to continue in the same direction or do we want to just embed the blob?

I think the question is whether we should persist with the current approach of requiring operator to distribute a file or registry setting on all hosts in the swarm where a service is to be run, or whether we should do something smarter to that on docker service create the docker CLI will read a credentialspec file on the current host and then the credentialspec is stored in swarm with the service metadata.

diogomonica · 2017-04-02T22:39:33Z

@n4ss @docker/security-team

PatrickLang · 2017-04-03T21:37:41Z

@friism I don't think users like deploying extra files to hosts. The fewer the better - ie just install the Docker engine and your plugins, join the swarm and done. Pushing these through swarm also makes it easier if I want to redeploy a service with a different Active Directory service account. That way I don't need to update things stored on the hosts before deploying the service.

aluzzardi · 2017-04-04T00:57:48Z

Few changes:

Renamed PrivilegeProfile to Privileges
Added a oneof for CredentialSpec
Removed AppArmor and Seccomp - out of the scope for now

Questions:

Should the oneof for CredentialSpec contains sub messages, such as FileReference and RegistryReference rather than just a string?
Should CredentialSpec be outside of Privileges, since it's not entirely privilege related?

aaronlehmann · 2017-04-04T01:05:14Z

Should the oneof for CredentialSpec contains sub messages, such as FileReference and RegistryReference rather than just a string?

Personally I think a string is fine. If we're referencing a local file, a file path seems like the right way to do that, and I don't expect we'd need to add more meta-information. If it turns out we do, we already have the oneof, so it's easy to add a new way to reference the file.

Should CredentialSpec be outside of Privileges, since it's not entirely privilege related?

I don't understand CredentialSpec well enough to give a good answer, but my understanding of these protos was that CredentialSpec was related enough to security/privileges to belong there.

cpuguy83 · 2017-04-04T01:07:59Z

I think normally this would be a registry reference?

aluzzardi · 2017-04-04T01:09:47Z

@diogomonica Another question for you ... (and for you @dhiltgen ?)

This is what I have for SELinux:

	message SELinuxContext {
		string user = 1;
		string role = 2;
		string type = 3;
		string level = 4;
	}

I see there's also a label:disable - what is that and how should we represent it?

aluzzardi · 2017-04-04T02:30:15Z

Updated with the implementation

aaronlehmann · 2017-04-04T03:06:38Z

agent/exec/dockerapi/container.go

+			hc.SecurityOpt = append(hc.SecurityOpt, "credentialspec=file://"+credentials.GetFile())
+		case *api.Privileges_CredentialSpec_Registry:
+			hc.SecurityOpt = append(hc.SecurityOpt, "credentialspec=registry://"+credentials.GetRegistry())
+		}


Error out if the CredentialSpec type is unrecognized?

aaronlehmann · 2017-04-04T03:07:44Z

agent/exec/dockerapi/container.go

+	selinux := privileges.SELinuxContext
+	if selinux != nil {
+		if selinux.User != "" {
+			hc.SecurityOpt = append(hc.SecurityOpt, "label=user:%s"+selinux.User)


Is the %s in here intentional? It looks like a mistake.

Same comment for the other 3 selinux fields

cpuguy83 · 2017-04-04T14:13:45Z

lable:disable just means to disable selinux for that container.
Could handle with a oneof I guess? It's either disabled or specify the other options?

cpuguy83 · 2017-04-04T14:43:03Z

The label:disable bit is handled by the selinux library, btw.

aluzzardi · 2017-04-04T20:23:54Z

Updated with disabled

aluzzardi · 2017-04-04T20:52:32Z

Thanks @cpuguy83

Updated, should be good to go - PTAL

cpuguy83 · 2017-04-04T20:57:31Z

LGTM is there a easy way to test this?

aluzzardi · 2017-04-04T20:59:13Z

@cpuguy83 no :( Kindly relying on @dhiltgen for SELinux and @friism for CredentialSpec.

There's nothing on the CI side to help test those, so it must be manual for now

friism · 2017-04-04T21:24:44Z

on it

aluzzardi · 2017-04-05T01:00:15Z

I've decided to remove the code from here, protos only.

The one and only implementation is moby/moby#32339

Can we agree on the protos and move on?

I would need an LGTM from someone from security (ping @docker/security-team @n4ss @diogomonica) and an overall datatype LGTM (ping @aaronlehmann @cpuguy83)

Once this PR is merged we'll shift the focus on the implementation details in moby/moby#32339

aaronlehmann · 2017-04-05T01:12:20Z

LGTM

cpuguy83 · 2017-04-05T02:44:06Z

LGTM

friism · 2017-04-05T02:48:32Z

I tested credential-spec: moby/moby#32339 (comment)

aluzzardi · 2017-04-05T18:51:19Z

Thanks @friism @cpuguy83 @aaronlehmann!

Merging by EOD - @diogomonica @n4ss @cyli Any chance of getting a quick review by then?

@dhiltgen verified that the SELinuxContext (at least the Disable bit) is working correctly with moby/moby#32339

diogomonica · 2017-04-05T21:21:51Z

@aluzzardi will this allow us to pass client-sided full seccomp.json files to be used?

n4ss · 2017-04-05T23:56:17Z

@aluzzardi sorry for the late review, LGTM

@diogomonica it will require an additional field in Privileges (https://github.com/docker/docker/pull/32339/files#diff-a8f1f5d69ceb8aca0cd086c7220c35f1R41), I guess.

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

aluzzardi · 2017-04-06T01:32:12Z

@diogomonica seccomp and apparmor were dropped from this PR. SELinux and CredentialSpec only

aluzzardi · 2017-04-06T01:38:29Z

Thanks everyone!

n4ss · 2017-04-06T13:21:42Z

@aluzzardi is there a reason why it's SELinux only on linux? Or is it just a part.1 PR?

aluzzardi force-pushed the privilege-profile branch from c6b9d7a to d546688 Compare March 30, 2017 01:33

aluzzardi force-pushed the privilege-profile branch from d546688 to fcb86f8 Compare March 30, 2017 01:41

aluzzardi force-pushed the privilege-profile branch from fcb86f8 to b199dfe Compare April 4, 2017 00:56

aluzzardi force-pushed the privilege-profile branch from b199dfe to 13543fb Compare April 4, 2017 02:27

aaronlehmann reviewed Apr 4, 2017

View reviewed changes

aluzzardi mentioned this pull request Apr 4, 2017

services: Add support for Credential Spec and SELinux moby/moby#32339

Merged

aluzzardi force-pushed the privilege-profile branch 2 times, most recently from 504f23e to dfbc682 Compare April 4, 2017 20:10

aluzzardi force-pushed the privilege-profile branch 2 times, most recently from 0d6833c to b2d9985 Compare April 5, 2017 00:56

aluzzardi force-pushed the privilege-profile branch from b2d9985 to fbede6d Compare April 6, 2017 01:31

Privilege Profile support

ca E544 c0545

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

aluzzardi force-pushed the privilege-profile branch from fbede6d to cac0545 Compare April 6, 2017 01:31

aluzzardi merged commit d2e48a3 into moby:master Apr 6, 2017

aluzzardi deleted the privilege-profile branch April 6, 2017 01:38

aaronlehmann mentioned this pull request Apr 11, 2017

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

Closed

thaJeztah mentioned this pull request Aug 10, 2023

Add support for security opts on containers #3149

Merged

Privilege Profile support #2075

Privilege Profile support #2075

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!