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

Add --chown flag support for ADD/COPY commands for Windows #35521

Merged
merged 1 commit into from Aug 17, 2018

Conversation

@salah-khan
Copy link
Contributor

salah-khan commented Nov 16, 2017

This is addressing: #35507

While the issue specifies warning that chown is not supported on
Windows, this fix actually implements chown support on Windows. Built-in
accounts as well as accounts included in the SAM database of the container
are supported.

The following are valid examples:

ADD --chown=Guest . <some directory>
COPY --chown=Administrator . <some directory>
COPY --chown=Guests . <some directory>
COPY --chown=ContainerUser . <some directory>
COPY --chown=someuser . <some directory>

On Windows an owner is only granted the permission to read the security
descriptor and read/write the discretionary access control list. This
fix also grants read/write and execute permissions to the owner.

Signed-off-by: Salahuddin Khan salah@docker.com

- What I did
Added support for --chown on Windows which supports built-in accounts including ContainerAdministrator/ContatinerUser and accounts specific to the container. Examples of the following include \Administrator, \Guest and any specific users to the container.

- How I did it
This works by invoking another binary built from https://github.com/salah-khan/windows-container-utility within the container and extracts the SID information.

- How to verify it

The following Dockerfile sets both container specific and well known groups and uses ICACLS to verify that the user/group in question has the access requested.

COPY --chown=Administrator . test
COPY --chown=Guests . test2
RUN icacls \test
RUN icacls \test\*
RUN icacls \test2
RUN icacls \test2\*

The following is the output of building with the Dockerfile:

Sending build context to Docker daemon  498.2kB
Step 1/7 : FROM microsoft/windowsservercore
 ---> ce27208ad678
Step 2/7 : COPY --chown=Administrator . test
 ---> e658217806cb
Step 3/7 : COPY --chown=Guests . test2
 ---> f6c1e7a3463d
Step 4/7 : RUN icacls \test
 ---> Running in 3dcba2ec54bd
\test BUILTIN\Administrators:(F)
      BUILTIN\Administrators:(OI)(CI)(IO)(F)
      NT AUTHORITY\SYSTEM:(F)
      NT AUTHORITY\SYSTEM:(OI)(CI)(IO)(F)
      3DCBA2EC54BD\Administrator:(R,W,D,WDAC)
      3DCBA2EC54BD\Administrator:(OI)(CI)(IO)(DE,Rc,WDAC,GR,GW)
      BUILTIN\Administrators:(I)(OI)(CI)(F)
      NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F)
      3DCBA2EC54BD\Administrator:(I)(F)
      CREATOR OWNER:(I)(OI)(CI)(IO)(F)
      BUILTIN\Users:(I)(OI)(CI)(RX)
      BUILTIN\Users:(I)(CI)(AD)
      BUILTIN\Users:(I)(CI)(WD)

Successfully processed 1 files; Failed processing 0 files
Removing intermediate container 3dcba2ec54bd
 ---> 60b621288d53
Step 5/7 : RUN icacls \test\*
 ---> Running in 56295f6a34df
\test\Dockerfile BUILTIN\Administrators:(F)
                 BUILTIN\Administrators:(I)(F)
                 NT AUTHORITY\SYSTEM:(I)(F)
                 56295F6A34DF\Administrator:(I)(R,W,D,WDAC)
                 BUILTIN\Users:(I)(RX)

\test\getsid.ps1 BUILTIN\Administrators:(F)
                 BUILTIN\Administrators:(I)(F)
                 NT AUTHORITY\SYSTEM:(I)(F)
                 56295F6A34DF\Administrator:(I)(R,W,D,WDAC)
                 BUILTIN\Users:(I)(RX)

\test\OfflineSAMDBTest.exe BUILTIN\Administrators:(F)
                           BUILTIN\Administrators:(I)(F)
                           NT AUTHORITY\SYSTEM:(I)(F)
                           56295F6A34DF\Administrator:(I)(R,W,D,WDAC)
                           BUILTIN\Users:(I)(RX)

\test\owner.exe BUILTIN\Administrators:(F)
                BUILTIN\Administrators:(I)(F)
                NT AUTHORITY\SYSTEM:(I)(F)
                56295F6A34DF\Administrator:(I)(R,W,D,WDAC)
                BUILTIN\Users:(I)(RX)

\test\SAM BUILTIN\Administrators:(F)
          BUILTIN\Administrators:(I)(F)
          NT AUTHORITY\SYSTEM:(I)(F)
          56295F6A34DF\Administrator:(I)(R,W,D,WDAC)
          BUILTIN\Users:(I)(RX)

\test\SAM.LOG1 BUILTIN\Administrators:(F)
               BUILTIN\Administrators:(I)(F)
               NT AUTHORITY\SYSTEM:(I)(F)
               56295F6A34DF\Administrator:(I)(R,W,D,WDAC)
               BUILTIN\Users:(I)(RX)

\test\SAM.LOG2 BUILTIN\Administrators:(F)
               BUILTIN\Administrators:(I)(F)
               NT AUTHORITY\SYSTEM:(I)(F)
               56295F6A34DF\Administrator:(I)(R,W,D,WDAC)
               BUILTIN\Users:(I)(RX)

Successfully processed 7 files; Failed processing 0 files
Removing intermediate container 56295f6a34df
 ---> 4e6c9ad614d2
Step 6/7 : RUN icacls \test2
 ---> Running in dd923deb9a62
\test2 BUILTIN\Administrators:(F)
       BUILTIN\Administrators:(OI)(CI)(IO)(F)
       NT AUTHORITY\SYSTEM:(F)
       NT AUTHORITY\SYSTEM:(OI)(CI)(IO)(F)
       BUILTIN\Guests:(R,W,D,WDAC)
       BUILTIN\Guests:(OI)(CI)(IO)(DE,Rc,WDAC,GR,GW)
       BUILTIN\Administrators:(I)(OI)(CI)(F)
       NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F)
       BUILTIN\Guests:(I)(F)
       CREATOR OWNER:(I)(OI)(CI)(IO)(F)
       BUILTIN\Users:(I)(OI)(CI)(RX)
       BUILTIN\Users:(I)(CI)(AD)
       BUILTIN\Users:(I)(CI)(WD)

Successfully processed 1 files; Failed processing 0 files
Removing intermediate container dd923deb9a62
 ---> 7eb8b1de9c87
Step 7/7 : RUN icacls \test2\*
 ---> Running in f140da0d1429
\test2\Dockerfile BUILTIN\Administrators:(F)
                  BUILTIN\Administrators:(I)(F)
                  NT AUTHORITY\SYSTEM:(I)(F)
                  BUILTIN\Guests:(I)(R,W,D,WDAC)
                  BUILTIN\Users:(I)(RX)

\test2\getsid.ps1 BUILTIN\Administrators:(F)
                  BUILTIN\Administrators:(I)(F)
                  NT AUTHORITY\SYSTEM:(I)(F)
                  BUILTIN\Guests:(I)(R,W,D,WDAC)
                  BUILTIN\Users:(I)(RX)

\test2\OfflineSAMDBTest.exe BUILTIN\Administrators:(F)
                            BUILTIN\Administrators:(I)(F)
                            NT AUTHORITY\SYSTEM:(I)(F)
                            BUILTIN\Guests:(I)(R,W,D,WDAC)
                            BUILTIN\Users:(I)(RX)

\test2\owner.exe BUILTIN\Administrators:(F)
                 BUILTIN\Administrators:(I)(F)
                 NT AUTHORITY\SYSTEM:(I)(F)
                 BUILTIN\Guests:(I)(R,W,D,WDAC)
                 BUILTIN\Users:(I)(RX)

\test2\SAM BUILTIN\Administrators:(F)
           BUILTIN\Administrators:(I)(F)
           NT AUTHORITY\SYSTEM:(I)(F)
           BUILTIN\Guests:(I)(R,W,D,WDAC)
           BUILTIN\Users:(I)(RX)

\test2\SAM.LOG1 BUILTIN\Administrators:(F)
                BUILTIN\Administrators:(I)(F)
                NT AUTHORITY\SYSTEM:(I)(F)
                BUILTIN\Guests:(I)(R,W,D,WDAC)
                BUILTIN\Users:(I)(RX)

\test2\SAM.LOG2 BUILTIN\Administrators:(F)
                BUILTIN\Administrators:(I)(F)
                NT AUTHORITY\SYSTEM:(I)(F)
                BUILTIN\Guests:(I)(R,W,D,WDAC)
                BUILTIN\Users:(I)(RX)

Successfully processed 7 files; Failed processing 0 files
Removing intermediate container f140da0d1429
 ---> 6ce8104c2ac9
Successfully built 6ce8104c2ac9
Successfully tagged test:latest

- Description for the changelog

Add --chown flag support for ADD/COPY commands for Windows

- A picture of a cute animal (not mandatory but encouraged)

@salah-khan salah-khan force-pushed the salah-khan:35507 branch from f368570 to f63a748 Nov 21, 2017

@salah-khan salah-khan changed the title 35507 Add --chown flag support for ADD/COPY commands for Windows Add --chown flag support for ADD/COPY commands for Windows Nov 21, 2017

@johnstep
Copy link
Member

johnstep left a comment

Overall, it looks like it should work as expected. Please see pending comments. I have a couple higher-level design questions that I will follow up with separately.

SeRestorePrivilege = "SeRestorePrivilege"
SeBackupPrivilege = "SeBackupPrivilege"
SeRestorePrivilege = "SeRestorePrivilege"
SeTakeOwnershipPrivilege = "SeTakeOwnershipPrivilege"

This comment has been minimized.

@johnstep

johnstep Nov 21, 2017

Member

Since winio is vendored, the change needs to be made in https://github.com/Microsoft/go-winio, merged there, released, and then vendored back here.

In the short term, define this constant elsewhere, such as in the system package, or in the file where it is used, perhaps with a TODO comment.

func fixPermissions(source, destination string, rootIDs idtools.IDPair, overrideSkip bool) error {
// chown is not supported on Windows
func fixPermissions(source, destination string, identity idtools.Identity, overrideSkip bool) error {

This comment has been minimized.

@johnstep

johnstep Nov 21, 2017

Member

Nit: Starting a function with a blank line seems uncommon in this repository. I noticed other new functions that also start with a blank line, but will only comment on this once. :)

// chown is not supported on Windows
func fixPermissions(source, destination string, identity idtools.Identity, overrideSkip bool) error {

if identity.IdType == idtools.TypeIDSID {

This comment has been minimized.

@johnstep

johnstep Nov 21, 2017

Member

Nit: Maybe invert the condition to avoid having to indent the rest of the function.

// chown is not supported on Windows
func fixPermissions(source, destination string, identity idtools.Identity, overrideSkip bool) error {

if identity.IdType == idtools.TypeIDSID {

This comment has been minimized.

@johnstep

johnstep Nov 21, 2017

Member

Is it important to check the identity type here? It might be a good idea, but it seems like the same would apply to the Unix version, but checking for the other type. For LCOW, I recommend checking the target platform (OS), although I see LCOW does not support this feature yet, per the TODO comments in copy.go.

func fixPermissions(source, destination string, identity idtools.Identity, overrideSkip bool) error {

if identity.IdType == idtools.TypeIDSID {
var sid *windows.SID

This comment has been minimized.

@johnstep

johnstep Nov 21, 2017

Member

Nit: Consider moving this down to the first use, with type inference (with :=):

sid, err := windows.StringToSid(identity.IdSid)
return "", false, err
}

if accountName == userStr {

This comment has been minimized.

@johnstep

johnstep Nov 21, 2017

Member

Should this be a case-insensitive string comparison?


accountSid = computerSid + "-" + strconv.FormatUint(uint64(userRid), 10)

defer windows.RegCloseKey(accountKey)

This comment has been minimized.

@johnstep

johnstep Nov 21, 2017

Member

This defer should be moved up, right after opening the key.

return "", false, err
}

if accountName == groupStr {

This comment has been minimized.

@johnstep

johnstep Nov 21, 2017

Member

As with lookupNTUser, should this be a case-insensitive string comparison?


accountSid = computerSid + "-" + strconv.FormatUint(uint64(groupRid), 10)

defer windows.RegCloseKey(accountKey)

This comment has been minimized.

@johnstep

johnstep Nov 21, 2017

Member

As with logonNTUser, this defer should be moved up, right after opening the key.

return accountSid, accountLocated, nil
}

func lookupNTGroup(groupNamesKey windows.Handle, groupStr string, computerSid string) (string, bool, error) {

This comment has been minimized.

@johnstep

johnstep Nov 21, 2017

Member

This function is identical to lookupNTUser except for the variable names. Since it takes the key as a parameter, I'd eliminate this function entirely and rename the variables in lookupNTUser: maybe namesKey, nameStr, and rid. Perhaps rename the function to something like locateNTAccountFromKey.

if accountName == "ContainerAdministrator" {
return idtools.Identity{IdType: idtools.TypeIDSID, IdSid: "S-1-5-93-2-1"}, nil

} else if accountName == "ContainerUser" {

This comment has been minimized.

@johnstep

johnstep Nov 21, 2017

Member

These comparisons for ContainerAdministrator and ContainerUser should be case-insensitive.


if platform == "windows" {
return getAccountIdentity(chown, ctrRootPath)

} else {

This comment has been minimized.

@johnstep

johnstep Nov 21, 2017

Member

This else is unnecessary since the if above always returns.

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Nov 21, 2017

Looks like there's some linting issues;

14:42:02 builder/dockerfile/internals_unix.go:1::warning: file is not gofmted with -s (gofmt)
14:42:02 builder/dockerfile/internals_unix.go:1::warning: file is not goimported (goimports)
14:42:02 builder/dockerfile/builder.go:167:2:warning: struct field IdMapping should be IDMapping (golint)
14:42:02 builder/dockerfile/internals.go:204:11:warning: if block ends with a return statement, so drop this else and outdent its block (golint)
14:42:02 builder/dockerfile/internals.go:233:9:warning: if block ends with a return statement, so drop this else and outdent its block (golint)
14:42:02 builder/dockerfile/internals_test.go:271:4:warning: var IdPair should be IDPair (golint)
14:42:02 builder/dockerfile/internals_windows.go:208:33:warning: ineffectual assignment to err (ineffassign)
14:42:02 daemon/build.go:224:1:warning: comment on exported method Daemon.IdentityMapping should be of the form "IdentityMapping ..." (golint)
14:42:02 pkg/archive/archive.go:62:2:warning: struct field IdMapping should be IDMapping (golint)
14:42:02 pkg/archive/archive.go:1146:1:warning: comment on exported method Archiver.IdentityMapping should be of the form "IdentityMapping ..." (golint)
14:42:02 pkg/containerfs/archiver.go:28:2:warning: struct field IdMapping should be IDMapping (golint)
14:42:02 pkg/containerfs/archiver.go:184:1:warning: comment on exported method Archiver.IdentityMapping should be of the form "IdentityMapping ..." (golint)
14:42:02 builder/dockerfile/internals_windows.go:196:31:warning: ineffectual assignment to err (ineffassign)
14:42:02 layer/layer_store.go:51:2:warning: struct field IdMapping should be IDMapping (golint)
14:42:02 pkg/idtools/idtools.go:150:6:warning: exported type IdentityMapping should have comment or be unexported (golint)
14:42:02 pkg/idtools/idtools.go:141:2:warning: struct field IdSid should be IDSid (golint)
14:42:02 pkg/idtools/idtools.go:131:6:warning: exported type IdentityMappingType should have comment or be unexported (golint)
14:42:02 pkg/idtools/idtools.go:152:2:warning: struct field IdMappings should be IDMappings (golint)
14:42:02 pkg/idtools/idtools.go:153:2:warning: struct field Id should be ID (golint)
14:42:02 pkg/idtools/idtools.go:139:2:warning: struct field IdType should be IDType (golint)
14:42:02 pkg/idtools/idtools.go:140:2:warning: struct field IdPair should be IDPair (golint)
14:42:02 pkg/idtools/idtools.go:124:6:warning: exported type IdentityType should have comment or be unexported (golint)
14:42:02 pkg/idtools/idtools.go:127:2:warning: exported const TypeIDPair should have comment (or a comment on this block) or be unexported (golint)
14:42:02 pkg/idtools/idtools.go:134:2:warning: exported const TypeMapping should have comment (or a comment on this block) or be unexported (golint)
14:42:02 pkg/idtools/idtools.go:138:6:warning: exported type Identity should have comment or be unexported (golint)
TypeIdentity
)

type Identity struct {

This comment has been minimized.

@thaJeztah

thaJeztah Nov 21, 2017

Member

Wondering if we should have a more simplified type;

  • the type of identity to use depends on what platform/os the container is running on (correct?)
  • theoretically, this means that both a Unix UID/GID and a Windows SID could be passed, and based on what platform it's executed on, the right type could be taken

Given the above, would something like below work?

type Identity struct {
    UID int
    GID int
    SID string
}
@johnstep

This comment has been minimized.

Copy link
Member

johnstep commented Nov 28, 2017

In testing, I discovered that this only works for accounts in the SAM database of the base layer because the system registry hives are handled by kernel registry virtualization instead of the graph driver. Therefore, account lookup will fail for added users and groups. We are investigating possible solutions.

@@ -55,7 +55,7 @@ func newCopyInfos(copyInfos ...copyInfo) []copyInfo {
return copyInfos
}

// copyInstruction is a fully parsed COPY or ADD command that is passed to
// copyInstruction is a fully parsed COPY or ADD command that is `ed to

This comment has been minimized.

@johnstep

func NewIDPairIdentity(IdPair IDPair) Identity {
return Identity{IdType: TypeIDPair, IdPair: IdPair}
}

This comment has been minimized.

@johnstep

johnstep Dec 9, 2017

Member

This utility function seems fine, but I noticed most callers also initialize IdPair inline. Did you consider accepting UID and GID instead of IdPair, or adding yet another utility function?

@GordonTheTurtle

This comment has been minimized.

Copy link

GordonTheTurtle commented Dec 14, 2017

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 "35507" git@github.com:salah-khan/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353944000
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.

@salah-khan salah-khan force-pushed the salah-khan:35507 branch from 674224c to 032e65d Dec 14, 2017

@GordonTheTurtle GordonTheTurtle removed the dco/no label Dec 14, 2017

@johnstep

This comment has been minimized.

Copy link
Member

johnstep commented Mar 27, 2018

This is ready; please review: @jhowardmsft @dnephin @tonistiigi @dmcgowan @vdemeester

@vdemeester
Copy link
Member

vdemeester left a comment

@salah-khan can you take care of @dnephin's comments ?

}

if strings.EqualFold(accountName, "ContainerAdministrator") {
return idtools.Identity{SID: "S-1-5-93-2-1"}, nil

This comment has been minimized.

@vdemeester

vdemeester Apr 6, 2018

Member

Those feels a bit like magic nubmers ? could we put them into constants and add a comment on it (for documentation purpose)

return idtools.Identity{SID: "S-1-5-93-2-1"}, nil

} else if strings.EqualFold(accountName, "ContainerUser") {
return idtools.Identity{SID: "S-1-5-93-2-2"}, nil

This comment has been minimized.

@vdemeester

vdemeester Apr 6, 2018

Member

Same here

@thaJeztah
Copy link
Member

thaJeztah left a comment

Some nits and questions, but overall looks good 👍

cmd := reexec.Command("windows-fix-permissions", source, destination, identity.SID)
output := bytes.NewBuffer(nil)
cmd.Stdout = output
cmd.Stderr = output

This comment has been minimized.

@thaJeztah

thaJeztah Apr 6, 2018

Member

@salah-khan could you make this change? I agree that the shorter code is more readable

return nil
}

func fixPermissionsReexec() {
err := fixPermissionsWindows(os.Args[1], os.Args[2], os.Args[3])

This comment has been minimized.

@thaJeztah

thaJeztah Apr 6, 2018

Member

Is there a need to check if len(os.Args) > 3)? Not sure if there is any chance that this would happen, but it would panic


func getAccountIdentity(builder *Builder, accountName string, ctrRootPath string, state *dispatchState) (idtools.Identity, error) {
// This won't be called for non-Windows, but needs to be present since
// Windows has this function for obtaining NT account information.

This comment has been minimized.

@thaJeztah

thaJeztah Apr 6, 2018

Member

👍 yes; if we don't need it, I'd prefer removing

accountSid, err := sid.String()

if err != nil {
return idtools.Identity{SID: ""}, errors.Wrapf(err, "error converting SID to string")

This comment has been minimized.

@thaJeztah

thaJeztah Apr 6, 2018

Member

Would it be useful to include sid and/or accountName as part of the error?

if strings.EqualFold(accountName, "ContainerAdministrator") {
return idtools.Identity{SID: "S-1-5-93-2-1"}, nil

} else if strings.EqualFold(accountName, "ContainerUser") {

This comment has been minimized.

@thaJeztah

thaJeztah Apr 6, 2018

Member

nit: no need to use else (first if already does a return

cd "$GOPATH/src/github.com/salah-khan/windows-container-utility"
git checkout -q "$CONTAINER_UTILITY_COMMIT"

echo Building: ${DEST}/containerutility.exe

This comment has been minimized.

@thaJeztah

thaJeztah Apr 6, 2018

Member

@tiborvass @seemethere do you think this should use the docker- (future, possibly moby-) prefix? I know there's some scripts that (e.g.) cp docker-* or mv docker-* from the .zip we ship with binaries

This comment has been minimized.

@seemethere

seemethere Apr 6, 2018

Contributor

Yes it'd be useful for us when shipping Docker CE

This comment has been minimized.

@salah-khan

salah-khan Apr 12, 2018

Author Contributor

We don't use a docker- prefix on Docker running on Windows, so differentiating this from the current Windows model would make it confusing at best.

This comment has been minimized.

@johnstep

johnstep Apr 12, 2018

Member

The current executable for Windows is named dockerd.exe, not using that prefix. The Docker client is named docker.exe. I would rather use a list of files than a name prefix.

// make an array containing the original path asked for, plus (for mkAll == true)
// all path components leading up to the complete path that don't exist before we MkdirAll
// so that we can chown all of them properly at the end. If chownExisting is false, we won't
// chown the full directory path if it exists

ownerUID := identity.UID
ownerGID := identity.GID

This comment has been minimized.

@thaJeztah

thaJeztah Apr 6, 2018

Member

No need to use the intermediate variables here; it's cleaner to change the code below to

if err := lazyChown(pathComponent, identity.UID, identity.GID, nil); err != nil {

Wonder if the argument should be renamed to owner instead?

PROTECTED_DACL_SECURITY_INFORMATION = 0x80000000
PROTECTED_SACL_SECURITY_INFORMATION = 0x40000000
UNPROTECTED_DACL_SECURITY_INFORMATION = 0x20000000
UNPROTECTED_SACL_SECURITY_INFORMATION = 0x10000000

This comment has been minimized.

@thaJeztah

thaJeztah Apr 6, 2018

Member

Go convention is to use CamelCase for constants as well; https://stackoverflow.com/a/22688926/1811501

Was there a specific reason to not use that here? If not, I think we should change to CamelCase

SE_DS_OBJECT_ALL
SE_PROVIDER_DEFINED_OBJECT
SE_WMIGUID_OBJECT
SE_REGISTRY_WOW64_32KEY

This comment has been minimized.

@thaJeztah

thaJeztah Apr 6, 2018

Member

Same question here for naming

This comment has been minimized.

@salah-khan

salah-khan Apr 12, 2018

Author Contributor

This is exactly how these are named in Windows, so I kept them the same.

This comment has been minimized.

@johnstep

johnstep Apr 12, 2018

Member

Yes, they currently match the C constants, which is consistent with the Go syscall package.

Dockerfile Outdated
@@ -181,7 +181,6 @@ COPY hack/dockerfile/install/$INSTALL_BINARY_NAME.installer ./
RUN PREFIX=/opt/$INSTALL_BINARY_NAME ./install.sh $INSTALL_BINARY_NAME



This comment has been minimized.

@thaJeztah

thaJeztah Apr 6, 2018

Member

If you can revert to keep the diff focussed on the actual changes, that'd be appreciated 🤗

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jun 7, 2018

@salah-khan I noticed there were still some outstanding review comments; were you working on those?

@salah-khan salah-khan force-pushed the salah-khan:35507 branch 2 times, most recently from de19896 to 12f49c7 Jul 6, 2018

@salah-khan salah-khan force-pushed the salah-khan:35507 branch 6 times, most recently from 2d2c501 to 1e39330 Jul 26, 2018

@salah-khan
Copy link
Contributor Author

salah-khan left a comment

It doesn't make sense to include the SID or account name as the error here is not specific to either. It's an internal error that occurred when converting a valid SID (which was obtained by converting a SID string) back into a SID string (for normalization - for example if the original sid was s-1-5-xyz-abc etc. it would be normalized to S-1-5-xyz-abc). But the specific SID or Account is not responsible for the error which is both likely to be exceedingly rare and only occur in low resource scenarios.

source, _ := filepath.Split(os.Args[0])

target := "C:\\Docker"
targetExecutable := target + "\\containerutility.exe"

This comment has been minimized.

@salah-khan

salah-khan Aug 14, 2018

Author Contributor

This is already being built as part of the build process.

Add ADD/COPY --chown flag support to Windows
This implements chown support on Windows. Built-in accounts as well
as accounts included in the SAM database of the container are supported.

NOTE: IDPair is now named Identity and IDMappings is now named
IdentityMapping.

The following are valid examples:
ADD --chown=Guest . <some directory>
COPY --chown=Administrator . <some directory>
COPY --chown=Guests . <some directory>
COPY --chown=ContainerUser . <some directory>

On Windows an owner is only granted the permission to read the security
descriptor and read/write the discretionary access control list. This
fix also grants read/write and execute permissions to the owner.

Signed-off-by: Salahuddin Khan <salah@docker.com>

@salah-khan salah-khan force-pushed the salah-khan:35507 branch from 1e39330 to 763d839 Aug 14, 2018

@johnstep

This comment has been minimized.

Copy link
Member

johnstep commented Aug 16, 2018

@thaJeztah @vdemeester Please take a look. All your feedback is addressed, and everything passes.

@thaJeztah
Copy link
Member

thaJeztah left a comment

LGTM

left some notes; let me know if you want to address any of them, but I'm fine with merging as-is


var daclPresent uint32
var daclDefaulted uint32
var dacl *byte

This comment has been minimized.

@thaJeztah

thaJeztah Aug 17, 2018

Member

style nit; no need to change; generally we group these, i.e.;

var (
    daclPresent uint32
    daclDefaulted uint32
    dacl *byte
)
}

func fixPermissionsReexec() {
err := fixPermissionsWindows(os.Args[1], os.Args[2], os.Args[3])

This comment has been minimized.

@thaJeztah

thaJeztah Aug 17, 2018

Member

nit: a more idiomatic way to write these is to scope the err to the if;

if err := fixPermissionsWindows(os.Args[1], os.Args[2], os.Args[3]); err != nil {
 ....
}
var daclDefaulted uint32
var dacl *byte

err = system.GetSecurityDescriptorDacl(&securityDescriptor[0], &daclPresent, &dacl, &daclDefaulted)

This comment has been minimized.

@thaJeztah

thaJeztah Aug 17, 2018

Member

same here (not a blocker)

if err := system.GetSecurityDescriptorDacl(&securityDescriptor[0], &daclPresent, &dacl, &daclDefaulted); err != nil {
    return err
}
if strings.HasPrefix(accountName, "S-") || strings.HasPrefix(accountName, "s-") {
sid, err := windows.StringToSid(accountName)

if err == nil {

This comment has been minimized.

@thaJeztah

thaJeztah Aug 17, 2018

Member

it's ok to ignore this error (if err != nil)?

This comment has been minimized.

@salah-khan

salah-khan Aug 17, 2018

Author Contributor

Yes, this is intentional. If an organization has accounts that use S- then falling through here will check on the actual system for the account (since it's not a valid SID). I know of at least one 50000+ employee organization with accounts of S-.

This comment has been minimized.

@thaJeztah

thaJeztah Aug 17, 2018

Member

Alright, thanks for checking!

// Check if the account name is one unique to containers.
if strings.EqualFold(accountName, "ContainerAdministrator") {
return idtools.Identity{SID: system.ContainerAdministratorSidString}, nil

This comment has been minimized.

@thaJeztah

thaJeztah Aug 17, 2018

Member

nit: stray empty line

if strings.EqualFold(accountName, "ContainerAdministrator") {
return idtools.Identity{SID: system.ContainerAdministratorSidString}, nil

} else if strings.EqualFold(accountName, "ContainerUser") {

This comment has been minimized.

@thaJeztah

thaJeztah Aug 17, 2018

Member

nit: no need for else here, because the if above already does a return

@@ -18,8 +18,10 @@ func (daemon *Daemon) tarCopyOptions(container *container.Container, noOverwrite
return nil, err
}

identity := idtools.Identity{UID: user.Uid, GID: user.Gid}

This comment has been minimized.

@thaJeztah

thaJeztah Aug 17, 2018

Member

Wondering why you added the intermediate variable here, instead of putting it inline (as it was before)

func (archiver *Archiver) IDMappings() *idtools.IDMappings {
return archiver.IDMappingsVar
// IdentityMapping returns the IdentityMapping of the archiver.
func (archiver *Archiver) IdentityMapping() *idtools.IdentityMapping {

This comment has been minimized.

@thaJeztah

thaJeztah Aug 17, 2018

Member

Just leaving as note here; this is an exported method, and inside a pkg/, so will cause breaking changes for other projects that use this pkg (also for the other name/interface changes)

@johnstep johnstep merged commit b3e9f7b into moby:master Aug 17, 2018

7 of 8 checks passed

codecov/patch 35.22% of diff hit (target 50%)
Details
codecov/project 35.62% (-0.01%) compared to 1e49fdc
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 41610 has succeeded
Details
janky Jenkins build Docker-PRs 50384 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 10818 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 21692 has succeeded
Details
z Jenkins build Docker-PRs-s390x 10675 has succeeded
Details
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.