Skip to content

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

Merged
thaJeztah merged 2 commits intomoby:masterfrom
microsoft:jjh/grantvmgroupaccess
Mar 23, 2019
Merged

LCOW: Add VMGroup SID to layer.vhd; fix layer folder perm#38922
thaJeztah merged 2 commits intomoby:masterfrom
microsoft:jjh/grantvmgroupaccess

Conversation

@lowenna
Copy link
Copy Markdown
Member

@lowenna lowenna commented Mar 21, 2019

Signed-off-by: John Howard [email protected]

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

John Howard added 2 commits March 21, 2019 13:12
Signed-off-by: John Howard <[email protected]>

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
```
Copy link
Copy Markdown
Contributor

@jterry75 jterry75 left a comment

Choose a reason for hiding this comment

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

LGTM (not a maintainer)

@codecov
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Contributor

LGTM

@lowenna lowenna added status/4-merge kind/experimental area/lcow Issues and PR's related to the experimental LCOW feature and removed status/0-triage labels Mar 22, 2019
@thaJeztah thaJeztah merged commit e4cc3ad into moby:master Mar 23, 2019
@thaJeztah
Copy link
Copy Markdown
Member

@jhowardmsft does this have to be cherry-picked?

@lowenna
Copy link
Copy Markdown
Member Author

lowenna commented Mar 23, 2019

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

@thaJeztah
Copy link
Copy Markdown
Member

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

Comment thread vendor.conf
github.com/Azure/go-ansiterm d6e3b3328b783f23731bc4d058875b0371ff8109
github.com/Microsoft/hcsshim ada9cb39f715fb568e1030e7613732bb4f1e4aeb
github.com/Microsoft/go-winio 4de24ed3e8c509e6d1f609a8cb6b1c9fd9816e6d
github.com/Microsoft/go-winio c599b533b43b1363d7d7c6cfda5ede70ed73ff13
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Mar 27, 2019

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Gotcha! I'll await progress there 🤗

@lowenna lowenna deleted the jjh/grantvmgroupaccess branch March 27, 2019 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/lcow Issues and PR's related to the experimental LCOW feature kind/experimental status/4-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants