experimental failures looks unrelated
ping @tiborvass @anusha-ragunathan

@LK4D4 CI is green :)

Looks like this needs a rebase now @dperny 😢

all squared away @thaJeztah 😎

(note, i reordered these commits. it makes more sense imhoirl to put the swarm revendoring BEFORE the update; this makes every commit a valid build.)

ping @jhowardmsft @friism @johnstep PTAL

Where the listener itself created? I'm wondering if an ACL is set appropriately on the pipe? As by default it will be ACL'd to LocalSystem when dockerd is running as a (Windows) service.

@jhowardmsft Good point; it is passing nil right now, which results in NULL passed to CreateNamedPipe. This will work in testing, if running dockerd.exe directly, elevated, since the default security descriptor grants Administrators write access.

Note: The listener is created here: https://github.com/docker/docker/pull/27838/files#diff-9a7ebccfca7787cfcbb7a0dc051c6d65R15

@jhowardmsft Hmm, dockerd.exe, started as a service, runs as system for me:

$ cdb -pvr -pn dockerd.exe -c "!token -n;q" | findstr S-
User: S-1-5-18 (Well Known Group: NT AUTHORITY\SYSTEM)
 00 S-1-5-32-544 (Alias: BUILTIN\Administrators)
 01 S-1-1-0 (Well Known Group: localhost\Everyone)
 02 S-1-5-11 (Well Known Group: NT AUTHORITY\Authenticated Users)
 03 S-1-16-16384 (Label: Mandatory Label\System Mandatory Level)
Primary Group: S-1-5-18 (Well Known Group: NT AUTHORITY\SYSTEM)
Integrity Level: S-1-16-16384

$ sc qc docker
[SC] QueryServiceConfig SUCCESS

SERVICE_NAME: docker
        TYPE               : 10  WIN32_OWN_PROCESS
        START_TYPE         : 2   AUTO_START
        ERROR_CONTROL      : 1   NORMAL
        BINARY_PATH_NAME   : C:\Program Files\Docker\dockerd.exe --run-service
        LOAD_ORDER_GROUP   :
        TAG                : 0
        DISPLAY_NAME       : Docker
        DEPENDENCIES       :
        SERVICE_START_NAME : LocalSystem

Therefore, the security descriptor is fine. Is it supposed to run as system?

That wasn't I meant. It is supposed to be LocalSystem. I was questioning the ACL on the swarm named pipe.

@jhowardmsft So long as docker.exe is running elevated, I think it should be able to connect, but I will verify. Are there cases where the client runs without Administrators enabled in its token?

Absolutely - in fact I would strongly recommend the client is not run elevated, and the daemon started with the -G flag (or equivalent in the service config)

@jhowardmsft Ah, that is great, thanks! In that case, it sounds like the security descriptor should be created from the same SDDL generated here:

https://github.com/docker/docker/blob/b248de7e332b6e67b08a8981f68060e6ae629ccf/pkg/listeners/listeners_windows.go#L26-L36

Is that what you expect?

I believe so, but then again I'm not sure what is expected to be able connect to the 'swarm' pipe.

So Swarm is a nearly stand-alone process that receives all of its commands over unix socket. The docker daemon is the ONLY thing that should connect to the swarm pipe right now.

I feel like it's worth noting that I was sort of conscripted to fix this bug and so while I'm enjoying working on it, I'm also not quite sure about things like security implications. If you think things should be done a different way than they're already being done, you probably know better than me, but I'll need direct instruction on what needs to be changed and how I can check that I changed it right.

I would be inclined to say that the ACL should match the +administrators for the named pipe the engine listens to. But specifically NOT the block of code https://github.com/docker/docker/blob/b248de7e332b6e67b08a8981f68060e6ae629ccf/pkg/listeners/listeners_windows.go#L28-L36, but otherwise the same.

Alright, I'll work on getting that squared away. It will probably require a change in Swarmkit as well.

@jhowardmsft I'm sorry, I don't quite understand. what's the difference between that block of code is doing and what I should be doing?

As an aside, the suggested change does not affect the docker/docker portion of this PR at all; it requires a new PR into swarmkit. It's my opinion that right now this PR can be merged as-is, as long as a swarmkit PR goes through and is vendored before 1.13

You want these bits. Not the if socketGroup != "" {....} block.

        // allow Administrators and SYSTEM
        sddl := "D:P(A;;GA;;;BA)(A;;GA;;;SY)"
        c := winio.PipeConfig{
            SecurityDescriptor: sddl,
            MessageMode:        true,  // Use message mode so that CloseWrite() is supported
            InputBufferSize:    65536, // Use 64KB buffers to improve performance
            OutputBufferSize:   65536,
        }
        l, err := winio.ListenPipe(addr, &c)

Thanks! This change is definitely going to be in github.com/docker/swarmkit/xnet for now, and will require no modification this PR. The Swarmkit API is not changing.

Rebased onto master, dropped the vendor commit because master now has swarmkit vendored far enough.

LGTM

LGTM (although not tested). Can you open a PR to re-vendor the updated swarmkit too (or add a commit for it here again)?

Can you open a PR to re-vendor the updated swarmkit too (or add a commit for it here again)?

#27995 has it. Rebasing should do it, unless this has already been rebased since that was merged.

@aaronlehmann Nice 👍

looks like it's ready to merge