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.

@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?

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 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.

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

@cpuguy83 say again? Where is it stored?

How #1722 relates to this PR?

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

@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.

@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?

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.

@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.

@n4ss @docker/security-team

@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.

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?

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.

I think normally this would be a registry reference?

@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?

Updated with the implementation

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?

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

Updated with disabled

Thanks @cpuguy83

Updated, should be good to go - PTAL

LGTM is there a easy way to test this?

@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

on it

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

LGTM

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

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

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

@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.

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

Thanks everyone!

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