services: Add support for Credential Spec and SELinux #32339
Conversation
| selinux := privileges.SELinuxContext | ||
| if selinux != nil { | ||
| if selinux.User != "" { | ||
| hc.SecurityOpt = append(hc.SecurityOpt, "label=user:%s"+selinux.User) |
There was a problem hiding this comment.
Are the instances of %s in these strings intentional?
There was a problem hiding this comment.
Right - not at all. I don't have neither of those environments set up so I wasn't able to test. Waiting for some help on that front. Fixed
|
I'm still working on getting an AD controller working to test end-to-end, but I think the basic plumbing is working: @PatrickLang @jhowardmsft do you concur? I'm working on end-to-end test. |
There was a problem hiding this comment.
@aluzzardi I don't know if it's a practical or useful CI test, but you could do the equivalent of
docker service create --credential-spec file://foo.json something
... and then verify that the hostconfig of the containers started for the task include:
"SecurityOpt": [
"credentialspec=file://foo.json"
],
| case strings.HasPrefix(value, "registry://"): | ||
| c.value.Registry = strings.TrimPrefix(value, "registry://") | ||
| default: | ||
| return errors.New("Invalid credential spec - value must be prefixed file:// or registry:// followed by a value") |
|
Alright, I think I've now tested this end-to-end. Here's what I did:
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
44c9f2d9f481 microsoft/windowsservercore:latest "powershell -c Sle..." 4 minutes ago Up 4 minutes test.1.fwpjwck926a7as86g8ljy6qkw |
|
@aluzzardi since config is coming, do you think we can get support for that in a follow-up PR? #32336 |
|
Panic from swarmkit vendoring: |
|
@aluzzardi if appropriate, can this be labelled for 17.05? |
|
Thanks for all the help testing this, @friism ! It's already milestoned for 17.05, I'll make it a P1. Configs are freshly under review so I think it would be premature to interface with them until they get merged. However, we can already make sure credential spec and configs are compatible. @aaronlehmann: I was thinking of doing something like |
|
@cpuguy83 I know there have been some changes to the allocator recently (where to panic originates), does that ring a bell @aaronlehmann @dongluochen @aboch @yongtang? I don't think how it can be related to this PR |
|
@aluzzardi: A fix is in progress in #32283. This has been affecting a lot of PRs. I made #32283 a P1 earlier today. |
|
@aluzzardi we need this in the compose-file format for https://github.com/docker/docker/tree/master/cli/compose/schema/data |
|
To add it to Compose:
I'm thinking the schema would be something like this: {
"privileges": {
"oneOf": [
{
"type": "object",
"additionalProperties": false,
"properties": {
"file": {"type": "string"},
"registry": {"type": "string"},
},
},
{
"type": "object",
"additionalProperties": false,
"properties": {
"disable": {"type": "boolean"},
"user": {"type": "string"},
"role": {"type": "string"},
"type": {"type": "string"},
"level": {"type": "string"},
},
}, |
There was a problem hiding this comment.
Do you get an error if you try and run a linux service with CredentialSpec or a windows service with SELinuxContext?
I think this is the first time we're adding these platform specific flags to services, so I think we should consider grouping them together in the API. As we add more of these it will be a lot easier to document and for users to figure out.
ContainerSpec:
Linux:
SEContext: ...
Windows:
CredentialSpec: ...|
@dnephin fwiw the flags themselves are annotated with OS specificity: https://github.com/docker/docker/pull/32339/files#diff-728a7fe135201867a6b4b645adc9553dR547 |
|
@dnephin The thing is, this is not top-level, this is under We'd need to have something like Or we could just have Also, we already have a few Linux specific API options / flags, such as Thoughts on that, @dnephin @aaronlehmann? |
+1
I think there is a good reason. To make it clear that the options are platform specific. |
|
I hear you but I don't think that's the right move. Rather than grouping flags by functionality (e.g. It's fine to have options that only apply to a platform, as long as it's documented. Ultimately, flags will be supported on platforms where they are supported. You can set Therefore I think this is the way to go |
|
Agree with @aluzzardi, design LGTM |
| c.value = &swarm.CredentialSpec{} | ||
| switch { | ||
| case strings.HasPrefix(value, "file://"): | ||
| c.value.File = strings.TrimPrefix(value, "file://") |
There was a problem hiding this comment.
Was just wondering; in the file:// case, does the file have to be present on each node? Should we send it's content? (or store it as a secret)
There was a problem hiding this comment.
Backstory is here: moby/swarmkit#2075
I proposed to add the content directly, but the review went the other way.
The reasoning is to support file and registry now (same as docker run), and in the future, support config (or secret)
There was a problem hiding this comment.
@thaJeztah just to summarize, the problem is that the value is fairly bulky, so there's no way it can be in-lined in docker-compose files nor in docker service create invocations. @aluzzardi considered making the cli resolve the value from file or registry, but that won't work if user is interacting with a GUI or web app that doesn't have filesystem or registry context.
As @aluzzardi, we can get something more elegant with config://.
There was a problem hiding this comment.
Are these (file:// and registry://) actual schemes, and expected to follow the standards for that (https://en.wikipedia.org/wiki/File_URI_scheme), including url-encoding?
file://localhost/c|/WINDOWS/my-credential-spec.txt
file:///c|/WINDOWS/my-credential-spec.txt
file://localhost/c:/WINDOWS/my-credential-spec.txt
file:///c:/WINDOWS/my-credential-spec.txt
Or;
file://C:\Documents\foobar.txt
There was a problem hiding this comment.
It's the same format we already support today in --security-opt credentialspec=, which has been defined by MSFT.
I think it's the latter approach and I 1) trust their judgment 2) believe we should be using whatever we're already using in docker run --security-opt credentialspec=...
There was a problem hiding this comment.
Looks like it only allows locations relative to config.root, but docs are unclear if it allows a path or only a filename https://github.com/docker/docker/pull/23389/files#diff-0189e098e6ba3aeffd9ee321ee6aca8aR4532
My preference would be to use a standard, but given that it's already like this, this doesn't make it worse I guess
|
Not sure what the failure on |
| } | ||
|
|
||
| // Privileges defines the security options for the container. | ||
| type Privileges struct { |
There was a problem hiding this comment.
t.b.h., still a bit on the fence on naming this Privileges, as SELinux is not really "privileges", "CredentialSpec" perhaps, but if we add AppArmor profiles and Seccomp profiles, we're really diverging from this.
If this is the "security profile" perhaps SecurityOptions, Security, SecurityConfig, or SecurityProfile
There was a problem hiding this comment.
/cc @diogomonica
I think this will end up containing AppArmor, Seccomp, Capabilities, ... and everything that made up the --privileged flag
There was a problem hiding this comment.
The goal is to have all privilege related data in Privileges.SELinux and Seccomp included.
| @@ -509,6 +548,9 @@ func addServiceFlags(flags *pflag.FlagSet, opts *serviceOptions) { | |||
|
|
|||
| flags.StringVarP(&opts.workdir, flagWorkdir, "w", "", "Working directory inside the container") | |||
| flags.StringVarP(&opts.user, flagUser, "u", "", "Username or UID (format: <name|uid>[:<group|gid>])") | |||
| flags.Var(&opts.credentialSpec, flagCredentialSpec, "Credential spec for managed service account (Windows only)") | |||
There was a problem hiding this comment.
I wonder if (Windows only) is necessary since the flag is already conditional on a Windows daemon. I don't think we say (Linux only) for Linux-specific flags.
There was a problem hiding this comment.
Wondering if it should be conditional for Windows daemon. This is for services, and the manager could be a Windows daemon, but the task/service can be deployed on Linux. I think it should be always shown
There was a problem hiding this comment.
Actually, I should probably remove the conditional on Windows daemon? You can have Windows workers with Linux managers
There was a problem hiding this comment.
Good point. Yes, let's remove the conditional. This reminds me of #31897. We probably have other issues like this.
There was a problem hiding this comment.
LOL, looks like we came to the same conclusion at exactly the same time
daemon/cluster/convert/container.go
Outdated
| containerSpec.Privileges.CredentialSpec = &swarmapi.Privileges_CredentialSpec{} | ||
|
|
||
| if c.Privileges.CredentialSpec.File != "" && c.Privileges.CredentialSpec.Registry != "" { | ||
| return nil, errors.New("cannot specify both `file` and `registry` credential specs") |
There was a problem hiding this comment.
I think we typically don't use backticks like this in error messages. Sometimes we use double quotes.
daemon/cluster/convert/container.go
Outdated
| Registry: c.Privileges.CredentialSpec.Registry, | ||
| } | ||
| } else { | ||
| return nil, errors.New("must either provide `file` or `registry` for credential spec") |
There was a problem hiding this comment.
I think we typically don't use backticks like this in error messages. Sometimes we use double quotes.
There was a problem hiding this comment.
Also, this might be a little too strict. I think an empty CredentialSpec provided to the REST API should be treated the same as no credential spec.
- Defined "normalized" type for Credential Spec and SELinux - Added --credential-spec to docker service create & update - SELinux is API only at the time Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>
|
LGTM |
|
all green, let me go ahead and merge this |
|
Thanks @thaJeztah :) I will do:
However, for flags for SELinux, I believe @diogomonica wants to limit this to the API level, no CLI support. |
services: Add support for Credential Spec and SELinux
This is work in progress and vendors in moby/swarmkit#2075
/cc @ehazlett @johnstep @aaronlehmann @dhiltgen @diogomonica @vieux @friism