New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

services: Add support for Credential Spec and SELinux #32339

Merged
merged 1 commit into from Apr 7, 2017

Conversation

@aluzzardi
Member

aluzzardi commented Apr 4, 2017

  • 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 docker/swarmkit#2075

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

@aluzzardi aluzzardi added this to the 17.05.0 milestone Apr 4, 2017

@friism

This comment has been minimized.

Show comment
Hide comment
@friism

friism Apr 4, 2017

Contributor

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.

Contributor

friism commented Apr 4, 2017

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

This comment has been minimized.

@friism

friism Apr 4, 2017

Contributor

should "value" be "path"?

@friism

friism Apr 4, 2017

Contributor

should "value" be "path"?

@friism

This comment has been minimized.

Show comment
Hide comment
@friism

friism Apr 5, 2017

Contributor

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

  1. Started WS2016 Hyper-V VM
  2. Made it a domain-controller
  3. Installed Docker on the domain controller
  4. Followed these instructions as far as creating the cred-spec file: https://github.com/Microsoft/Virtualization-Documentation/tree/live/windows-server-container-tools/ServiceAccounts
  5. 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
Contributor

friism commented Apr 5, 2017

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

  1. Started WS2016 Hyper-V VM
  2. Made it a domain-controller
  3. Installed Docker on the domain controller
  4. Followed these instructions as far as creating the cred-spec file: https://github.com/Microsoft/Virtualization-Documentation/tree/live/windows-server-container-tools/ServiceAccounts
  5. 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

This comment has been minimized.

Show comment
Hide comment
@friism

friism Apr 5, 2017

Contributor

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

Contributor

friism commented Apr 5, 2017

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

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 5, 2017

Contributor

Panic from swarmkit vendoring:

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
Contributor

cpuguy83 commented Apr 5, 2017

Panic from swarmkit vendoring:

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

This comment has been minimized.

Show comment
Hide comment
@friism

friism Apr 5, 2017

Contributor

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

Contributor

friism commented Apr 5, 2017

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

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Apr 5, 2017

Member

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?

Member

aluzzardi commented Apr 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Apr 5, 2017

Member

@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

Member

aluzzardi commented Apr 5, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 5, 2017

Contributor

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

Contributor

aaronlehmann commented Apr 5, 2017

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

@friism

This comment has been minimized.

Show comment
Hide comment
@friism

friism Apr 5, 2017

Contributor

@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

Contributor

friism commented Apr 5, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Apr 5, 2017

Member

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

dnephin commented Apr 5, 2017

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

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

@friism friism referenced this pull request Apr 6, 2017

Closed

Add credspec to compose #32393

@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

This comment has been minimized.

Show comment
Hide comment
@friism

friism Apr 6, 2017

Contributor
Contributor

friism commented Apr 6, 2017

@GordonTheTurtle GordonTheTurtle added dco/no and removed dco/no labels Apr 6, 2017

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Apr 6, 2017

Member

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

Member

aluzzardi commented Apr 6, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Apr 6, 2017

Member

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.

Member

dnephin commented Apr 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Apr 6, 2017

Member

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

Member

aluzzardi commented Apr 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 7, 2017

Contributor

Agree with @aluzzardi, design LGTM

Contributor

cpuguy83 commented Apr 7, 2017

Agree with @aluzzardi, design LGTM

c.value = &swarm.CredentialSpec{}
switch {
case strings.HasPrefix(value, "file://"):
c.value.File = strings.TrimPrefix(value, "file://")

This comment has been minimized.

@thaJeztah

thaJeztah Apr 7, 2017

Member

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)

@thaJeztah

thaJeztah Apr 7, 2017

Member

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)

This comment has been minimized.

@aluzzardi

aluzzardi Apr 7, 2017

Member

Backstory is here: docker/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)

@aluzzardi

aluzzardi Apr 7, 2017

Member

Backstory is here: docker/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)

This comment has been minimized.

@friism

friism Apr 7, 2017

Contributor

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

@friism

friism Apr 7, 2017

Contributor

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

This comment has been minimized.

@thaJeztah

thaJeztah Apr 7, 2017

Member

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

thaJeztah Apr 7, 2017

Member

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

This comment has been minimized.

@aluzzardi

aluzzardi Apr 7, 2017

Member

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

@aluzzardi

aluzzardi Apr 7, 2017

Member

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

This comment has been minimized.

@thaJeztah

thaJeztah Apr 7, 2017

Member

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

@thaJeztah

thaJeztah Apr 7, 2017

Member

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

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Apr 7, 2017

Member

Not sure what the failure on z is all about

Member

aluzzardi commented Apr 7, 2017

Not sure what the failure on z is all about

}
// Privileges defines the security options for the container.
type Privileges struct {

This comment has been minimized.

@thaJeztah

thaJeztah Apr 7, 2017

Member

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

@thaJeztah

thaJeztah Apr 7, 2017

Member

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

This comment has been minimized.

@aluzzardi

aluzzardi Apr 7, 2017

Member

/cc @diogomonica

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

@aluzzardi

aluzzardi Apr 7, 2017

Member

/cc @diogomonica

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

This comment has been minimized.

@diogomonica

diogomonica Apr 7, 2017

Contributor

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

@diogomonica

diogomonica Apr 7, 2017

Contributor

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)")

This comment has been minimized.

@aaronlehmann

aaronlehmann Apr 7, 2017

Contributor

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.

@aaronlehmann

aaronlehmann Apr 7, 2017

Contributor

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.

This comment has been minimized.

@thaJeztah

thaJeztah Apr 7, 2017

Member

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

@thaJeztah

thaJeztah Apr 7, 2017

Member

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

This comment has been minimized.

@aluzzardi

aluzzardi Apr 7, 2017

Member

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

@aluzzardi

aluzzardi Apr 7, 2017

Member

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

This comment has been minimized.

@aaronlehmann

aaronlehmann Apr 7, 2017

Contributor

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

@aaronlehmann

aaronlehmann Apr 7, 2017

Contributor

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

This comment has been minimized.

@thaJeztah

thaJeztah Apr 7, 2017

Member

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

@thaJeztah

thaJeztah Apr 7, 2017

Member

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

This comment has been minimized.

@aluzzardi

aluzzardi Apr 7, 2017

Member

Done

@aluzzardi

aluzzardi Apr 7, 2017

Member

Done

services: Add support for Credential Spec and SELinux
- 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>
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 7, 2017

Contributor

LGTM

Contributor

aaronlehmann commented Apr 7, 2017

LGTM

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi
Member

aluzzardi commented Apr 7, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 7, 2017

Member

all green, let me go ahead and merge this

Member

thaJeztah commented Apr 7, 2017

all green, let me go ahead and merge this

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32702 has succeeded
Details
janky Jenkins build Docker-PRs 41313 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1497 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12432 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1328 has succeeded
Details
@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Apr 7, 2017

Member

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.

Member

aluzzardi commented Apr 7, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment