Skip to content
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

LCOW: Add VMGroup SID to layer.vhd; fix layer folder perm #38922

Merged
merged 2 commits into from Mar 23, 2019

Conversation

Projects
None yet
6 participants
@jhowardmsft
Copy link
Contributor

jhowardmsft commented Mar 21, 2019

Signed-off-by: John Howard jhoward@microsoft.com

Some permissions corrections here. Also needs re-vendor of go-winio.

  • Create the layer folder directory as standard, not with SDDL. It will inherit permissions from the data-root correctly.
  • Apply the VM Group SID access to layer.vhd

Permissions after this changes

Data root:

PS C:\> icacls test
test BUILTIN\Administrators:(OI)(CI)(F)
     NT AUTHORITY\SYSTEM:(OI)(CI)(F)

lcow subdirectory under dataroot

PS C:\> icacls test\lcow
test\lcow BUILTIN\Administrators:(I)(OI)(CI)(F)
          NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F)

layer.vhd in a layer folder for LCOW

.\test\lcow\c33923d21c9621fea2f990a8778f469ecdbdc57fd9ca682565d1fa86fadd5d95\layer.vhd NT VIRTUAL MACHINE\Virtual Machines:(R)
                                                                                       BUILTIN\Administrators:(I)(F)
                                                                                       NT AUTHORITY\SYSTEM:(I)(F)

And showing working

PS C:\> docker-ci-zap -folder=c:\test
INFO: Zapped successfully
PS C:\> docker run --rm alpine echo hello
Unable to find image 'alpine:latest' locally
latest: Pulling from library/alpine
8e402f1a9c57: Pull complete
Digest: sha256:644fcb1a676b5165371437feaa922943aaf7afcfa8bfee4472f6860aad1ef2a0
Status: Downloaded newer image for alpine:latest
hello

@jterry75 PTAL

jhowardmsft added some commits Mar 21, 2019

LCOW: Add SIDs to layer.vhd at creation
Signed-off-by: John Howard <jhoward@microsoft.com>

Some permissions corrections here. Also needs re-vendor of go-winio.

 - Create the layer folder directory as standard, not with SDDL. It will inherit permissions from the data-root correctly.
 - Apply the VM Group SID access to layer.vhd

Permissions after this changes

Data root:

```
PS C:\> icacls test
test BUILTIN\Administrators:(OI)(CI)(F)
     NT AUTHORITY\SYSTEM:(OI)(CI)(F)
```

lcow subdirectory under dataroot
```
PS C:\> icacls test\lcow
test\lcow BUILTIN\Administrators:(I)(OI)(CI)(F)
          NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F)
```

layer.vhd in a layer folder for LCOW
```
.\test\lcow\c33923d21c9621fea2f990a8778f469ecdbdc57fd9ca682565d1fa86fadd5d95\layer.vhd NT VIRTUAL MACHINE\Virtual Machines:(R)
                                                                                       BUILTIN\Administrators:(I)(F)
                                                                                       NT AUTHORITY\SYSTEM:(I)(F)
```

And showing working

```
PS C:\> docker-ci-zap -folder=c:\test
INFO: Zapped successfully
PS C:\> docker run --rm alpine echo hello
Unable to find image 'alpine:latest' locally
latest: Pulling from library/alpine
8e402f1a9c57: Pull complete
Digest: sha256:644fcb1a676b5165371437feaa922943aaf7afcfa8bfee4472f6860aad1ef2a0
Status: Downloaded newer image for alpine:latest
hello
```
Vendor Microsoft/go-winio@c599b53
Signed-off-by: John Howard <jhoward@microsoft.com>
@jterry75
Copy link
Contributor

jterry75 left a comment

LGTM (not a maintainer)

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 22, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@6daf5ab). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #38922   +/-   ##
=========================================
  Coverage          ?    36.9%           
=========================================
  Files             ?      614           
  Lines             ?    45374           
  Branches          ?        0           
=========================================
  Hits              ?    16745           
  Misses            ?    26338           
  Partials          ?     2291
@crosbymichael

This comment has been minimized.

Copy link
Member

crosbymichael commented Mar 22, 2019

LGTM

@thaJeztah thaJeztah merged commit e4cc3ad into moby:master Mar 23, 2019

10 checks passed

codecov/patch Coverage not affected.
Details
codecov/project No report found to compare against
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 44577 has succeeded
Details
janky Jenkins build Docker-PRs 53405 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 13781 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 5109 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 24545 has succeeded
Details
windowsRS5-process Jenkins build Docker-PRs-WoW-RS5-Process 1869 has succeeded
Details
z Jenkins build Docker-PRs-s390x 13672 has succeeded
Details
@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Mar 23, 2019

@jhowardmsft does this have to be cherry-picked?

@jhowardmsft

This comment has been minimized.

Copy link
Contributor Author

jhowardmsft commented Mar 23, 2019

@thaJeztah Not to 1809. Has 1903 branched? If so, yes, it should

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Mar 23, 2019

It was branched, but perhaps it was branched after this got merged, so pls double check (reading from my phone)

@@ -1,7 +1,7 @@
# the following lines are in sorted order, FYI
github.com/Azure/go-ansiterm d6e3b3328b783f23731bc4d058875b0371ff8109
github.com/Microsoft/hcsshim ada9cb39f715fb568e1030e7613732bb4f1e4aeb
github.com/Microsoft/go-winio 4de24ed3e8c509e6d1f609a8cb6b1c9fd9816e6d
github.com/Microsoft/go-winio c599b533b43b1363d7d7c6cfda5ede70ed73ff13

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Mar 27, 2019

Member

@jhowardmsft do you know if a new tag will be created anytime soon? (so that we can have a tagged version in the 19.03 branch)

(also for the hcsshim dependency Microsoft/hcsshim@v0.8.6...ada9cb3)

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft Mar 27, 2019

Author Contributor

@thaJeztah We're talking about the possibility of a tagged release soon, but there's still a bunch of fixes we're working on for CRI/containerd, many of which also affect docker scenarios, hence our reticence until we're in a more stable position.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Mar 27, 2019

Member

Gotcha! I'll await progress there 🤗

@jhowardmsft jhowardmsft deleted the Microsoft:jjh/grantvmgroupaccess branch Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.