services: Add support for Credential Spec and SELinux by aluzzardi · Pull Request #32339 · moby/moby

aluzzardi · 2017-04-04T03:57:35Z

Defined "normalized" type for Credential Spec and SELinux
Added --credential-spec to docker service create & update
SELinux is API only at the time

This is work in progress and vendors in moby/swarmkit#2075

/cc @ehazlett @johnstep @aaronlehmann @dhiltgen @diogomonica @vieux @friism

aaronlehmann · 2017-04-04T17:27:16Z

daemon/cluster/executor/container/container.go

Are the instances of %s in these strings intentional?

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

friism · 2017-04-04T22:46:46Z

I'm still working on getting an AD controller working to test end-to-end, but I think the basic plumbing is working:

> cat C:\programdata\docker\credentialspecs\foo.json
{"CmsPlugins":["ActiveDirectory"],"DomainJoinConfig":{"DnsName":"contoso.com","Guid":"244818ae-87ca-4fcd-92ec-e79e5252348a","DnsTreeName":"contoso.com","NetBiosName":"DEMO","Sid":"S-1-5-21-2126729477-2524075714-3094792973","MachineAccountName"
:"WebApplication1"},"ActiveDirectoryConfig":{"GroupManagedServiceAccounts":[{"Name":"WebApplication1","Scope":"DEMO"},{"Name":"WebApplication1","Scope":"contoso.com"}]}}

> docker service create --name test --credential-spec file://foo.json microsoft/nanoserver powershell -c Sleep 3600

time="2017-04-04T15:21:17.179843400-07:00" level=debug msg="HCSShim::CreateContainer id=b895cb38ae93841c16ebdef2a63abe5a2f7b58eb7d97c21e065690132c97e46d config={\"SystemType\":\"Container\",\"Name\":\"b895cb38ae93841c16ebdef2a63abe5a2f7b58eb7d
97c21e065690132c97e46d\",\"Owner\":\"docker\",\"IsDummy\":false,\"IgnoreFlushesDuringBoot\":true,\"LayerFolderPath\":\"C:\\\\ProgramData\\\\Docker\\\\windowsfilter\\\\b895cb38ae93841c16ebdef2a63abe5a2f7b58eb7d97c21e065690132c97e46d\",\"Layers\
":[{\"ID\":\"5555ae51-03c7-5ac4-b2b9-fbc3b4d838a9\",\"Path\":\"C:\\\\ProgramData\\\\Docker\\\\windowsfilter\\\\fda01a39e82d72a92408297ce37efbe31c2bf7e0fa20522e0727bc7e01a6d14b\"},{\"ID\":\"ea024d7c-19d8-515f-9f69-9bdb8407f33e\",\"Path\":\"C:\\
\\ProgramData\\\\Docker\\\\windowsfilter\\\\ae1baf289a4b53aa3456f1fdb7090eb9a45444b99c5869ef80fd2ea4896feeab\"}],\"Credentials\":\"{\\\"CmsPlugins\\\":[\\\"ActiveDirectory\\\"],\\\"DomainJoinConfig\\\":{\\\"DnsName\\\":\\\"contoso.com\\\",\\\"
Guid\\\":\\\"244818ae-87ca-4fcd-92ec-e79e5252348a\\\",\\\"DnsTreeName\\\":\\\"contoso.com\\\",\\\"NetBiosName\\\":\\\"DEMO\\\",\\\"Sid\\\":\\\"S-1-5-21-2126729477-2524075714-3094792973\\\",\\\"MachineAccountName\\\":\\\"WebApplication1\\\"},\\
\"ActiveDirectoryConfig\\\":{\\\"GroupManagedServiceAccounts\\\":[{\\\"Name\\\":\\\"WebApplication1\\\",\\\"Scope\\\":\\\"DEMO\\\"},{\\\"Name\\\":\\\"WebApplication1\\\",\\\"Scope\\\":\\\"contoso.com\\\"}]}}\\r\\n\",\"HostName\":\"b895cb38ae93
\",\"MappedDirectories\":[],\"SandboxPath\":\"C:\\\\ProgramData\\\\Docker\\\\windowsfilter\",\"HvPartition\":true,\"EndpointList\":[\"e58d3ad6-3e2e-4f3a-81a7-6f4ccfe1fc03\"],\"HvRuntime\":{\"ImagePath\":\"C:\\\\ProgramData\\\\Docker\\\\windows
filter\\\\fda01a39e82d72a92408297ce37efbe31c2bf7e0fa20522e0727bc7e01a6d14b\\\\UtilityVM\"},\"Servicing\":false,\"AllowUnqualifiedDNSQuery\":true}"
time="2017-04-04T15:21:17.580443600-07:00" level=debug msg="HCSShim::CreateContainer succeeded id=b895cb38ae93841c16ebdef2a63abe5a2f7b58eb7d97c21e065690132c97e46d handle=60539248"

@PatrickLang @jhowardmsft do you concur?

I'm working on end-to-end test.

friism

@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"
            ],

friism · 2017-04-04T23:08:40Z

cli/command/service/opts.go

should "value" be "path"?

friism · 2017-04-05T02:12:23Z

Alright, I think I've now tested this end-to-end. Here's what I did:

Started WS2016 Hyper-V VM
Made it a domain-controller
Installed Docker on the domain controller
Followed these instructions as far as creating the cred-spec file: https://github.com/Microsoft/Virtualization-Documentation/tree/live/windows-server-container-tools/ServiceAccounts
docker service create --name test --credential-spec file://WebApplication1.json microsoft/windowsservercore powershell -c Sleep 3600

CONTAINER ID        IMAGE                                COMMAND                  CREATED             STATUS              PORTS               NAMES
44c9f2d9f481        microsoft/windowsservercore:latest   "powershell -c Sle..."   4 minutes ago       Up 4 minutes                            test.1.fwpjwck926a7as86g8ljy6qkw

PS C:\Users\Administrator> docker exec 44c9f2d9f481 nltest.exe /parentdomain
corp.yodirectory.com. (1)
The command completed successfully

friism · 2017-04-05T02:14:18Z

@aluzzardi since config is coming, do you think we can get support for that in a follow-up PR? #32336

cpuguy83 · 2017-04-05T02:50:49Z

Panic from swarmkit vendoring:

Details

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x1209e40]

goroutine 148 [running]:
panic(0x1790e80, 0xc420014050)
	/usr/local/go/src/runtime/panic.go:500 +0x1a1
github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator.(*Allocator).allocateService(0xc42109f2c0, 0x7f19471bf1a8, 0xc421220840, 0xc4207a8b40, 0x0, 0x0)
	/go/src/github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator/network.go:810 +0x1a0
github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator.(*Allocator).doNetworkAlloc(0xc42109f2c0, 0x7f19471bf1a8, 0xc421220840, 0x1842ee0, 0xc420ce7460)
	/go/src/github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator/network.go:324 +0x5e7
github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator.(*Allocator).(github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator.doNetworkAlloc)-fm(0x7f19471bf1a8, 0xc421220840, 0x1842ee0, 0xc420ce7460)
	/go/src/github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator/allocator.go:123 +0x52
github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator.(*Allocator).run(0xc42109f2c0, 0x7f19471bf1a8, 0xc421220840, 0xc421226180, 0xc4211e3c00, 0x19c5c4c, 0x7, 0xc4211dcdc0, 0xc4211dcdb0)
	/go/src/github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator/allocator.go:183 +0x124
github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator.(*Allocator).Run.func2.1(0xc4211dcd80, 0xc42109f2c0, 0xc4211dcd60, 0xc421226180, 0xc4211e3c00, 0x19c5c4c, 0x7, 0xc4211dcdc0, 0xc4211dcdb0)
	/go/src/github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator/allocator.go:150 +0x8a
created by github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator.(*Allocator).Run.func2
	/go/src/github.com/docker/docker/vendor/github.com/docker/swarmkit/manager/allocator/allocator.go:151 +0x1ba

friism · 2017-04-05T03:03:25Z

@aluzzardi if appropriate, can this be labelled for 17.05?

aluzzardi · 2017-04-05T03:30:45Z

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 docker service create --config mycredentials --credential-spec config://mycredentials and have the agent write the credential spec to a tmp file and pass it as a file://... back to the engine. Do you see any problem with that? @friism Could someone knowledgeable on the matter review my logic/UX?

aluzzardi · 2017-04-05T03:32:33Z

@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

aaronlehmann · 2017-04-05T05:52:20Z

@aluzzardi: A fix is in progress in #32283. This has been affecting a lot of PRs. I made #32283 a P1 earlier today.

friism · 2017-04-05T21:14:13Z

@aluzzardi we need this in the compose-file format for docker stack deploy -c too. Can you do that in this PR or in follow-up?

https://github.com/docker/docker/tree/master/cli/compose/schema/data

cc @dnephin @vieux

dnephin · 2017-04-05T21:30:45Z

To add it to Compose:

update the jsonschema in cli/compose/schema/data
add it to cli/compose/types/types.go
add it to the type converter in cli/compose/convert/service.go

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"},
        },
  },

dnephin

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

friism · 2017-04-06T16:35:28Z

@dnephin fwiw the flags themselves are annotated with OS specificity: https://github.com/docker/docker/pull/32339/files#diff-728a7fe135201867a6b4b645adc9553dR547

aluzzardi · 2017-04-06T18:15:09Z

@dnephin The thing is, this is not top-level, this is under Privileges.

We'd need to have something like Privileges // Window // CredentialSpec and Privileges // Linux // SELinuxOptions. And then we should replicate that for every other "sub-group" of options we have.

Or we could just have Windows // CredentialSpec, but in that case, we'd have a ton of options stashed under the platform for no reason (e.g. SELinux and tty).

Also, we already have a few Linux specific API options / flags, such as tty, groups`, ...

Thoughts on that, @dnephin @aaronlehmann?

dnephin · 2017-04-06T20:51:58Z

Or we could just have Windows // CredentialSpec

+1

we'd have a ton of options stashed under the platform for no reason

I think there is a good reason. To make it clear that the options are platform specific.

aluzzardi · 2017-04-06T21:38:32Z

I hear you but I don't think that's the right move. Rather than grouping flags by functionality (e.g. Privileges), we'll end up with highly unrelated groups (e.g. tty next to selinux next to apparmor next to other stuff).

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 SELinux under Linux all you want, it's not because you are running Linux that it's going to work anyway (do you have selinux enabled in your system? the tooling installed? the daemon started with --selinux-enabled)?

Therefore I think this is the way to go

cpuguy83 · 2017-04-07T01:29:12Z

Agree with @aluzzardi, design LGTM

thaJeztah · 2017-04-07T17:52:43Z

cli/command/service/opts.go

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)

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)

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

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

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

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

aluzzardi · 2017-04-07T17:55:23Z

Not sure what the failure on z is all about

thaJeztah · 2017-04-07T17:55:41Z

api/types/swarm/container.go

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

/cc @diogomonica

I think this will end up containing AppArmor, Seccomp, Capabilities, ... and everything that made up the --privileged flag

The goal is to have all privilege related data in Privileges.SELinux and Seccomp included.

aaronlehmann · 2017-04-07T17:59:34Z

cli/command/service/opts.go

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.

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

Actually, I should probably remove the conditional on Windows daemon? You can have Windows workers with Linux managers

Good point. Yes, let's remove the conditional. This reminds me of #31897. We probably have other issues like this.

LOL, looks like we came to the same conclusion at exactly the same time

aaronlehmann · 2017-04-07T18:01:29Z

daemon/cluster/convert/container.go

I think we typically don't use backticks like this in error messages. Sometimes we use double quotes.

aaronlehmann · 2017-04-07T18:01:43Z

daemon/cluster/convert/container.go

I think we typically don't use backticks like this in error messages. Sometimes we use double quotes.

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 <[email protected]>

aaronlehmann · 2017-04-07T20:56:44Z

LGTM

thaJeztah

Had two questions #32339 (comment), and #32339 (comment)

cc @diogomonica @aluzzardi

aluzzardi · 2017-04-07T22:47:01Z

Two answers :)

thaJeztah

changes LGTM

we need to add the flags for SELinux (do we want that in 17.05?)
docs (API version history, swagger, CLI reference)
completion scripts

We can do those in a follow up

thaJeztah · 2017-04-07T23:37:09Z

all green, let me go ahead and merge this

aluzzardi · 2017-04-07T23:42:03Z

Thanks @thaJeztah :)

I will do:

Docs update
Completion scripts

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

aluzzardi added this to the 17.05.0 milestone Apr 4, 2017

GordonTheTurtle added the status/0-triage label Apr 4, 2017

aaronlehmann reviewed Apr 4, 2017

View reviewed changes

aaronlehmann added status/1-design-review status/needs-vendoring and removed status/0-triage labels Apr 4, 2017

carlfischer1 mentioned this pull request Apr 4, 2017

Add isolation option to docker service #31616

Closed

friism reviewed Apr 4, 2017

View reviewed changes

cli/command/service/opts.go Outdated

Copy link

Contributor

friism Apr 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should "value" be "path"?

aluzzardi mentioned this pull request Apr 5, 2017

Privilege Profile support moby/swarmkit#2075

Merged

aluzzardi added the priority/P1 Important: P1 issues are a top priority and a must-have for the next release. label Apr 5, 2017

aluzzardi changed the title ~~[WIP] services: Add support for Credential Spec and SELinux~~ services: Add support for Credential Spec and SELinux Apr 6, 2017

friism mentioned this pull request Apr 6, 2017

Add credspec to compose #32393

Closed

dnephin reviewed Apr 6, 2017

View reviewed changes

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Apr 6, 2017

thaJeztah reviewed Apr 7, 2017

View reviewed changes

aluzzardi removed the status/needs-vendoring label Apr 7, 2017

thaJeztah reviewed Apr 7, 2017

View reviewed changes

aaronlehmann reviewed Apr 7, 2017

View reviewed changes

services: Add support for Credential Spec and SELinux

89a995a

- 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 <[email protected]>

thaJeztah requested changes Apr 7, 2017

View reviewed changes

thaJeztah approved these changes Apr 7, 2017

View reviewed changes

thaJeztah added docs/revisit impact/api impact/changelog impact/cli impact/documentation labels Apr 7, 2017

thaJeztah merged commit 091b5e6 into moby:master Apr 7, 2017

aluzzardi deleted the selinux branch April 7, 2017 23:42

aluzzardi mentioned this pull request Apr 10, 2017

service privileges: API docs & completion #32483

Closed

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#32339 from aluzzardi/selinux

1b132eb

services: Add support for Credential Spec and SELinux

friism mentioned this pull request Jun 23, 2017

compose-file reference should include credential_spec documentation docker/docs#3687

Closed

thaJeztah mentioned this pull request Jul 7, 2017

[RFC] UX/Consistency: the state of credential spec docker/cli#309

Open

thaJeztah mentioned this pull request Jul 15, 2017

Service privileges: CLI reference docs & completion docker/cli#338

Merged

thaJeztah mentioned this pull request May 9, 2018

[epic] add more options to service create / service update #25303

Open

thaJeztah mentioned this pull request Feb 1, 2019

Service privileges: API docs #34002

Merged

Conversation

aluzzardi commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

friism commented Apr 4, 2017

Uh oh!

friism left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

friism commented Apr 5, 2017

Uh oh!

friism commented Apr 5, 2017

Uh oh!

cpuguy83 commented Apr 5, 2017

Uh oh!

friism commented Apr 5, 2017

Uh oh!

aluzzardi commented Apr 5, 2017

Uh oh!

aluzzardi commented Apr 5, 2017

Uh oh!

aaronlehmann commented Apr 5, 2017

Uh oh!

friism commented Apr 5, 2017

Uh oh!

dnephin commented Apr 5, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

friism commented Apr 6, 2017

Uh oh!

aluzzardi commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin commented Apr 6, 2017

Uh oh!

aluzzardi commented Apr 6, 2017

Uh oh!

cpuguy83 commented Apr 7, 2017

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!

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!

aluzzardi commented Apr 7, 2017

Uh oh!

thaJeztah Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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

aluzzardi commented Apr 4, 2017 •

edited

Loading

aluzzardi commented Apr 6, 2017 •

edited

Loading

thaJeztah Apr 7, 2017 •

edited

Loading