Add support for GMSA CredentialSpecs from Swarmkit configs #38632

dperny · 2019-01-24T18:00:09Z

- What I did

Adds the ability to specify a swarmkit Config as a CredentialSpec, as an alternative to the existing registry and file options. Adds a new CredentialSpec URI specifier: config://. This is used to specify the ID of a swarmkit config to be used as the CredentialSpec.

- How I did it

Altered swarmkit to support distributing a config that is not mounted into a container directly. Then, altered the oci_windows.go code to get that config from the dependency store, and use the data as the GMSA config.

- How to verify it

This will need to be manually tested, as I do not have access to a Windows environment on which to test.

- Description for the changelog

Add support for using swarmkit Configs as GMSA CredentialSpecs.

GordonTheTurtle · 2019-01-24T18:00:17Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "gmsa-support" git@github.com:dperny/docker.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842357812584
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

dperny · 2019-01-24T18:02:58Z

Interestingly, this requires no CLI or API changes, because CredentialSpecs are specified using that weird URI format.

cpuguy83 · 2019-01-24T18:05:40Z

I assume this has that change for async delete in it? We need to resolve that before we can re-vendor swarmkit.

dperny · 2019-01-24T18:11:35Z

@cpuguy83 yes, sorry, I should have mentioned that. This is blocked on the async delete changes.

codecov · 2019-01-31T22:08:34Z

Codecov Report

❗ No coverage uploaded for pull request base (master@7403497). Click here to learn what that means.
The diff coverage is 89.77%.

@@            Coverage Diff            @@
##             master   #38632   +/-   ##
=========================================
  Coverage          ?   36.41%           
=========================================
  Files             ?      613           
  Lines             ?    45837           
  Branches          ?        0           
=========================================
  Hits              ?    16693           
  Misses            ?    26851           
  Partials          ?     2293

dperny · 2019-01-31T22:09:14Z

Ok, the async delete change has been reverted, and I have revendored swarmkit, so this commit no longer includes that change. This is fully ready for review.

cpuguy83 · 2019-01-31T23:26:48Z

CI is all failing on the same test

14:23:57 --- FAIL: TestServiceUpdateLabel (2.05s)
14:23:57     daemon.go:296: [d5e1505aff338] waiting for daemon to start
14:23:57     daemon.go:328: [d5e1505aff338] daemon started
14:23:57     update_test.go:64: assertion failed: error is not nil: Error response from daemon: rpc error: code = Unknown desc = update out of sequence
14:23:57     daemon.go:283: [d5e1505aff338] exiting daemon

thaJeztah · 2019-02-03T02:46:36Z

oh, wha, missed this one; guess this will have to be combined with #38672 ?

dperny · 2019-02-04T16:51:48Z

@thaJeztah yes, affirmative.

Signed-off-by: Drew Erny <drew.erny@docker.com>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

dperny · 2019-02-05T15:50:40Z

This PR is updated

Removed the vendoring commit, as swarmkit is already vendored to the correct version
Added @thaJeztah's commit from Add support for using Configs as CredentialSpecs in services #38672 to this branch
Rebased

thaJeztah

Just some quick comments from my phone 😅

thaJeztah · 2019-02-05T16:32:27Z

daemon/oci_windows.go

+				}
+				csConfig, err := c.DependencyStore.Configs().Get(csValue)
+				if err != nil {
+					return fmt.Errorf("error getting value from config store: %v", err)


This should probably use errors.Wrap to preserve the original error; something like;

return errors.Wrap(err, "failed to get value from config store")

Good catch.

thaJeztah · 2019-02-05T16:34:50Z

daemon/oci_windows.go

+
+				// if the container does not have a DependencyStore, then we
+				// return an error
+				if c.DependencyStore == nil {


Wondering (reading from my phone so have limited sight in the whole code path) if we can catch this situation during create of the container instead of here (as this is (I think) only ran at the last point, at container start)

eg somewhere in this code path (would have to check if that's the right place though)

moby/daemon/create.go

Line 25 in e7a9a7c

func (daemon *Daemon) CreateManagedContainer(params types.ContainerCreateConfig) (containertypes.ContainerCreateCreatedBody, error) {

/cc @cpuguy83

I'm not sure that a nil dependency store is even a possibility here, but golang has no way to express that a value cannot be nil, so you end up having to check it everywhere to be defensive.

dperny · 2019-02-05T17:08:06Z

@thaJeztah are you going to veto this if I push any code-review-stage changes to a new commit, instead of reordering the two commits i have or altering your commit (which is at the head of the branch)?

cpuguy83 · 2019-02-05T17:17:41Z

Is it not odd that we are pulling in swarm data at the oci config level?

dperny · 2019-02-05T17:23:38Z

It is a bit odd, but where else would we do it? This is the part of the code where we get the actual CredentialSpec data. Not a rhetorical question, by the way. Legitimately unsure where else to put it.

cpuguy83 · 2019-02-05T17:25:54Z

Can we create a credential file from the config and use that in the host config?
Potentially could be done after create if we allow the swarm agent to update the container config after create.

dperny · 2019-02-05T17:45:38Z

Seems like an awful lot of runaround, especially if it involves a temporary file. The DependencyStore is part of the container.Container type, so why not use it? Plus, it's indirected through an interface, so it's plenty decoupled from swarm.

thaJeztah · 2019-02-05T17:49:13Z

are you going to veto this if I push any code-review-stage changes to a new commit, instead of reordering the two commits i have or altering your commit (which is at the head of the branch)?

if you do a "rebase -i" you can edit the previous commit if needed. I'm on PTO, and not near a computer until later tonight or otherwise tomorrow, so reviewing "by proxy" a bit; hence I pinged Brian to help 🤗

…

On 5 Feb 2019, at 18:08, Drew Erny ***@***.***> wrote: @thaJeztah are you going to veto this if I push any code-review-stage changes to a new commit, instead of reordering the two commits i have or altering your commit (which is at the head of the branch)? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

dperny · 2019-02-06T19:23:11Z

Ugh I realize I've bungled some things up.

In Swarmkit, the Config referenced in a CredentialSpec needs to be included in the ContainerSpec's ConfigReferences, with a RuntimeTarget instead of a FileTarget. This tells Swarmkit, "This config is going to be used somewhere, but not as a file in the container"

Then, the CredentialSpec is set to use a Config and references the ConfigID. This tells the daemon, "when you want a CredentialSpec, use this config".

The reason for this being in two places is that the ConfigReferences tell you what configs you're using, and the CredentialSpec tells you what CredentialSpec you're using. This means any existing logic for Configs is contained totally to checking ConfigReferences, and does not need to shop around for the other places Configs could be hiding. The biggest benefit for this is in the validation logic, and in the dispatcher logic.

My biggest mistake so far is that I forgot to write a validation check that ensures the config referenced as a CredentialSpec is actually included in the ConfigReferences, so nothing stops you from putting a Config in the CredentialSpec that can't be used.

I've been assuming we'll take the same pattern with the Docker API, but I neglected to take into account the fact that the RuntimeTarget object is empty -- it doesn't need any data (yet). I'm unsure if JSON would allow us to specify an empty object that is semantically different from a nil object...

The ultimate intention at the CLI level was for the user to simply pass --credential-spec="configid", and then have the CLI go and get the Config and add it to both the ConfigReferences and the CredentialSpec field.

dperny · 2019-02-07T20:30:32Z

Updated the API to reflect the Runtime target.

dperny · 2019-02-07T20:32:21Z

api/types/swarm/config.go

 // ConfigReference is a reference to a config in swarm
 type ConfigReference struct {
-	File       *ConfigReferenceFileTarget
+	File       *ConfigReferenceFileTarget    `json:",omitempty"`


Does adding json:",omitempty" alter the API in an incompatible way?

Hm, don't think it's strictly incompatible (although the swagger does not include x-nullable: true). Thinking of situations where someone would expect the File field to always be there, but those situations would likely already break, because in the new situation, File would be nil, instead of {"Name": ...., .....}, so I think changing this is OK

We should update the swagger though to indicate the fields can be empty (adding that to my other comment/proposed change)

I guess strictly they're different types, but (see my other comment) that may become complicated

@dperny

…ngine Instead of having to go through files or registry values as is currently the case. While adding GMSA support to Kubernetes (kubernetes/kubernetes#73726) I stumbled upon the fact that Docker currently only allows passing Windows credential specs through files or registry values, forcing the Kubelet to perform a rather awkward dance of writing-then-deleting to either the disk or the registry to be able to create a Windows container with cred specs. This patch solves this problem by making it possible to directly pass whole base64-encoded cred specs to the engine's API. I took the opportunity to slightly refactor the method responsible for Windows cred spec as it seemed hard to read to me. Added some unit tests on Windows credential specs handling, as there were previously none. I have also tested it manually: given a Windows container using a cred spec that you would normally start with e.g. ```powershell docker run --rm --security-opt "credentialspec=file://win.json" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain # output: # my.ad.domain.com. (1) # The command completed successfully ``` can now equivalently be started with ```powershell $b64CredSpec = [System.Convert]::ToBase64String([System.IO.File]::ReadAllBytes('C:\ProgramData\docker\credentialspecs\win.json')) docker run --rm --security-opt "credentialspec=base64://$b64CredSpec" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain # same output! ``` I'll do another PR on Swarmkit after this is merged to allow services to use the same option. (It's worth noting that @dperny faced the same problem adding GMSA support to Swarmkit, to which he came up with an interesting solution - see moby#38632 - but alas these tricks are not available to the Kubelet.) Signed-off-by: Jean Rouge <rougej+github@gmail.com>

@dperny

…ngine Instead of having to go through files or registry values as is currently the case. While adding GMSA support to Kubernetes (kubernetes/kubernetes#73726) I stumbled upon the fact that Docker currently only allows passing Windows credential specs through files or registry values, forcing the Kubelet to perform a rather awkward dance of writing-then-deleting to either the disk or the registry to be able to create a Windows container with cred specs. This patch solves this problem by making it possible to directly pass whole base64-encoded cred specs to the engine's API. I took the opportunity to slightly refactor the method responsible for Windows cred spec as it seemed hard to read to me. Added some unit tests on Windows credential specs handling, as there were previously none. Added/amended the relevant integration tests. I have also tested it manually: given a Windows container using a cred spec that you would normally start with e.g. ```powershell docker run --rm --security-opt "credentialspec=file://win.json" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain # output: # my.ad.domain.com. (1) # The command completed successfully ``` can now equivalently be started with ```powershell $rawCredSpec = & cat 'C:\ProgramData\docker\credentialspecs\win.json' $escaped = $rawCredSpec.Replace('"', '\"') docker run --rm --security-opt "credentialspec=raw://$escaped" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain # same output! ``` I'll do another PR on Swarmkit after this is merged to allow services to use the same option. (It's worth noting that @dperny faced the same problem adding GMSA support to Swarmkit, to which he came up with an interesting solution - see moby#38632 - but alas these tricks are not available to the Kubelet.) Signed-off-by: Jean Rouge <rougej+github@gmail.com>

@dperny

…ngine Instead of having to go through files or registry values as is currently the case. While adding GMSA support to Kubernetes (kubernetes/kubernetes#73726) I stumbled upon the fact that Docker currently only allows passing Windows credential specs through files or registry values, forcing the Kubelet to perform a rather awkward dance of writing-then-deleting to either the disk or the registry to be able to create a Windows container with cred specs. This patch solves this problem by making it possible to directly pass whole base64-encoded cred specs to the engine's API. I took the opportunity to slightly refactor the method responsible for Windows cred spec as it seemed hard to read to me. Added some unit tests on Windows credential specs handling, as there were previously none. Added/amended the relevant integration tests. I have also tested it manually: given a Windows container using a cred spec that you would normally start with e.g. ```powershell docker run --rm --security-opt "credentialspec=file://win.json" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain # output: # my.ad.domain.com. (1) # The command completed successfully ``` can now equivalently be started with ```powershell $rawCredSpec = & cat 'C:\ProgramData\docker\credentialspecs\win.json' $escaped = $rawCredSpec.Replace('"', '\"') docker run --rm --security-opt "credentialspec=raw://$escaped" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain # same output! ``` I'll do another PR on Swarmkit after this is merged to allow services to use the same option. (It's worth noting that @dperny faced the same problem adding GMSA support to Swarmkit, to which he came up with an interesting solution - see moby/moby#38632 - but alas these tricks are not available to the Kubelet.) Signed-off-by: Jean Rouge <rougej+github@gmail.com> Upstream-commit: 7fdac7eb0ff836633c0a08c430b9472a3bfd3e20 Component: engine

@dperny

…ngine Instead of having to go through files or registry values as is currently the case. While adding GMSA support to Kubernetes (kubernetes/kubernetes#73726) I stumbled upon the fact that Docker currently only allows passing Windows credential specs through files or registry values, forcing the Kubelet to perform a rather awkward dance of writing-then-deleting to either the disk or the registry to be able to create a Windows container with cred specs. This patch solves this problem by making it possible to directly pass whole base64-encoded cred specs to the engine's API. I took the opportunity to slightly refactor the method responsible for Windows cred spec as it seemed hard to read to me. Added some unit tests on Windows credential specs handling, as there were previously none. Added/amended the relevant integration tests. I have also tested it manually: given a Windows container using a cred spec that you would normally start with e.g. ```powershell docker run --rm --security-opt "credentialspec=file://win.json" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain # output: # my.ad.domain.com. (1) # The command completed successfully ``` can now equivalently be started with ```powershell $rawCredSpec = & cat 'C:\ProgramData\docker\credentialspecs\win.json' $escaped = $rawCredSpec.Replace('"', '\"') docker run --rm --security-opt "credentialspec=raw://$escaped" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain # same output! ``` I'll do another PR on Swarmkit after this is merged to allow services to use the same option. (It's worth noting that @dperny faced the same problem adding GMSA support to Swarmkit, to which he came up with an interesting solution - see moby#38632 - but alas these tricks are not available to the Kubelet.) Signed-off-by: Jean Rouge <rougej+github@gmail.com>

@dperny

…ngine Instead of having to go through files or registry values as is currently the case. While adding GMSA support to Kubernetes (kubernetes/kubernetes#73726) I stumbled upon the fact that Docker currently only allows passing Windows credential specs through files or registry values, forcing the Kubelet to perform a rather awkward dance of writing-then-deleting to either the disk or the registry to be able to create a Windows container with cred specs. This patch solves this problem by making it possible to directly pass whole base64-encoded cred specs to the engine's API. I took the opportunity to slightly refactor the method responsible for Windows cred spec as it seemed hard to read to me. Added some unit tests on Windows credential specs handling, as there were previously none. Added/amended the relevant integration tests. I have also tested it manually: given a Windows container using a cred spec that you would normally start with e.g. ```powershell docker run --rm --security-opt "credentialspec=file://win.json" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain # output: # my.ad.domain.com. (1) # The command completed successfully ``` can now equivalently be started with ```powershell $rawCredSpec = & cat 'C:\ProgramData\docker\credentialspecs\win.json' $escaped = $rawCredSpec.Replace('"', '\"') docker run --rm --security-opt "credentialspec=raw://$escaped" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain # same output! ``` I'll do another PR on Swarmkit after this is merged to allow services to use the same option. (It's worth noting that @dperny faced the same problem adding GMSA support to Swarmkit, to which he came up with an interesting solution - see moby#38632 - but alas these tricks are not available to the Kubelet.) Signed-off-by: Jean Rouge <rougej+github@gmail.com>

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jan 24, 2019

GordonTheTurtle added the status/0-triage label Jan 24, 2019

dperny force-pushed the gmsa-support branch from 330101a to fa6eaea Compare January 24, 2019 18:01

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jan 24, 2019

dperny force-pushed the gmsa-support branch from fa6eaea to 5426599 Compare January 31, 2019 22:08

dperny and others added 2 commits February 4, 2019 14:52

Add CredentialSpec from configs support

04995fa

Signed-off-by: Drew Erny <drew.erny@docker.com>

Add support for using Configs as CredentialSpecs in services

20383d5

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

dperny force-pushed the gmsa-support branch from 5426599 to 20383d5 Compare February 5, 2019 15:49

dperny mentioned this pull request Feb 5, 2019

Add support for using Configs as CredentialSpecs in services #38672

Closed

thaJeztah requested changes Feb 5, 2019

View reviewed changes

dperny commented Feb 7, 2019

View reviewed changes

GordonTheTurtle assigned duglin Feb 8, 2019

wk8 mentioned this pull request Feb 22, 2019

Making it possible to pass Windows credential specs directly to the engine #38777

Merged

thaJeztah mentioned this pull request Mar 2, 2019

CLI: Add support for using Configs as CredentialSpecs in services docker/cli#1656

Closed

Drewster727 mentioned this pull request Aug 5, 2019

add npipe support to docker_swarm_service ansible/ansible#60073

Merged

This was referenced Aug 12, 2019

Raw credential spec in docker compose / docker stack #39730

Open

add raw credential_spec capability docker/compose#6844

Closed

thaJeztah mentioned this pull request Jun 8, 2020

Add support for --security-opt, --syscall, --ulimit...to swarm mode #25209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for GMSA CredentialSpecs from Swarmkit configs #38632

Add support for GMSA CredentialSpecs from Swarmkit configs #38632

dperny commented Jan 24, 2019

GordonTheTurtle commented Jan 24, 2019

dperny commented Jan 24, 2019

cpuguy83 commented Jan 24, 2019

dperny commented Jan 24, 2019

codecov bot commented Jan 31, 2019 •

edited

Loading

dperny commented Jan 31, 2019

cpuguy83 commented Jan 31, 2019

thaJeztah commented Feb 3, 2019

dperny commented Feb 4, 2019

dperny commented Feb 5, 2019

thaJeztah left a comment

thaJeztah Feb 5, 2019

dperny Feb 5, 2019

thaJeztah Feb 5, 2019

dperny Feb 5, 2019

dperny commented Feb 5, 2019

cpuguy83 commented Feb 5, 2019

dperny commented Feb 5, 2019 •

edited

Loading

cpuguy83 commented Feb 5, 2019

dperny commented Feb 5, 2019

thaJeztah commented Feb 5, 2019 via email

dperny commented Feb 6, 2019

dperny commented Feb 7, 2019

dperny Feb 7, 2019

thaJeztah Feb 8, 2019

Add support for GMSA CredentialSpecs from Swarmkit configs #38632

Add support for GMSA CredentialSpecs from Swarmkit configs #38632

Conversation

dperny commented Jan 24, 2019

GordonTheTurtle commented Jan 24, 2019

dperny commented Jan 24, 2019

cpuguy83 commented Jan 24, 2019

dperny commented Jan 24, 2019

codecov bot commented Jan 31, 2019 • edited Loading

Codecov Report

dperny commented Jan 31, 2019

cpuguy83 commented Jan 31, 2019

thaJeztah commented Feb 3, 2019

dperny commented Feb 4, 2019

dperny commented Feb 5, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Feb 5, 2019

Choose a reason for hiding this comment

dperny Feb 5, 2019

Choose a reason for hiding this comment

thaJeztah Feb 5, 2019

Choose a reason for hiding this comment

dperny Feb 5, 2019

Choose a reason for hiding this comment

dperny commented Feb 5, 2019

cpuguy83 commented Feb 5, 2019

dperny commented Feb 5, 2019 • edited Loading

cpuguy83 commented Feb 5, 2019

dperny commented Feb 5, 2019

thaJeztah commented Feb 5, 2019 via email

dperny commented Feb 6, 2019

dperny commented Feb 7, 2019

dperny Feb 7, 2019

Choose a reason for hiding this comment

thaJeztah Feb 8, 2019

Choose a reason for hiding this comment

codecov bot commented Jan 31, 2019 •

edited

Loading

dperny commented Feb 5, 2019 •

edited

Loading