Add --chown flag support for ADD/COPY commands for Windows by salah-khan · Pull Request #35521 · moby/moby

salah-khan · 2017-11-16T06:50:21Z

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 [email protected]

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

johnstep

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.

johnstep · 2017-11-21T06:23:37Z

vendor/github.com/Microsoft/go-winio/privilege.go

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.

johnstep · 2017-11-21T06:38:21Z

builder/dockerfile/copy_windows.go

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. :)

johnstep · 2017-11-21T06:39:14Z

builder/dockerfile/copy_windows.go

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

johnstep · 2017-11-21T06:45:00Z

builder/dockerfile/copy_windows.go

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.

johnstep · 2017-11-21T06:48:55Z

builder/dockerfile/copy_windows.go

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

sid, err := windows.StringToSid(identity.IdSid)

johnstep · 2017-11-21T10:15:52Z

builder/dockerfile/internals_windows.go

Should this be a case-insensitive string comparison?

johnstep · 2017-11-21T10:16:58Z

builder/dockerfile/internals_windows.go

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

johnstep · 2017-11-21T10:22:30Z

builder/dockerfile/internals_windows.go

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

johnstep · 2017-11-21T10:22:52Z

builder/dockerfile/internals_windows.go

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

johnstep · 2017-11-21T10:31:15Z

builder/dockerfile/internals_windows.go

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.

johnstep · 2017-11-21T11:38:45Z

builder/dockerfile/internals_windows.go

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

johnstep · 2017-11-21T11:52:39Z

builder/dockerfile/internals.go

This else is unnecessary since the if above always returns.

thaJeztah · 2017-11-21T18:51:45Z

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)

thaJeztah · 2017-11-21T19:13:31Z

pkg/idtools/idtools.go

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 · 2017-11-28T20:37:56Z

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.

johnstep · 2017-12-09T19:25:33Z

builder/dockerfile/copy.go

johnstep · 2017-12-09T19:45:14Z

pkg/idtools/idtools.go

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 · 2017-12-14T04:08:09Z

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" [email protected]: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.

thaJeztah · 2018-03-22T19:01:03Z

Dockerfile

nit: unrelated change

It's not unrelated, I originally did the work here similar to runc/tini etc., however, I realized that the binaries would end up in the wrong location and should really only be built as part of the cross compile. It seems I removed an extra blank line by mistake when deleting my changes.

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

johnstep · 2018-03-27T16:46:16Z

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

vdemeester

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

vdemeester · 2018-04-06T08:36:30Z

builder/dockerfile/internals_windows.go

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

vdemeester · 2018-04-06T08:36:37Z

builder/dockerfile/internals_windows.go

thaJeztah

Some nits and questions, but overall looks good 👍

thaJeztah · 2018-04-06T20:13:15Z

builder/dockerfile/copy_windows.go

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

thaJeztah · 2018-04-06T20:16:27Z

builder/dockerfile/copy_windows.go

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

thaJeztah · 2018-04-06T20:19:40Z

builder/dockerfile/internals_unix.go

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

thaJeztah · 2018-04-06T20:29:24Z

builder/dockerfile/internals_windows.go

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

thaJeztah · 2018-04-06T20:32:37Z

builder/dockerfile/internals_windows.go

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

thaJeztah · 2018-04-06T20:54:45Z

hack/make/containerutility

@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

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

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.

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.

thaJeztah · 2018-04-06T20:58:17Z

pkg/idtools/idtools_unix.go

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?

thaJeztah · 2018-04-06T21:03:41Z

pkg/system/syscall_windows.go

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

thaJeztah · 2018-04-06T21:04:05Z

pkg/system/syscall_windows.go

Same question here for naming

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

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

thaJeztah · 2018-04-06T21:05:56Z

Dockerfile

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

thaJeztah · 2018-06-07T21:58:00Z

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

salah-khan

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.

salah-khan · 2018-07-26T20:11:02Z

builder/dockerfile/internals_windows.go

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

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 <[email protected]>

johnstep · 2018-08-16T17:26:45Z

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

thaJeztah

LGTM

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

thaJeztah · 2018-08-17T12:14:02Z

builder/dockerfile/copy_windows.go

+
+	var daclPresent uint32
+	var daclDefaulted uint32
+	var dacl *byte


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

var ( daclPresent uint32 daclDefaulted uint32 dacl *byte )

thaJeztah · 2018-08-17T12:15:53Z

builder/dockerfile/copy_windows.go

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


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 { .... }

thaJeztah · 2018-08-17T12:16:54Z

builder/dockerfile/copy_windows.go

+	var daclDefaulted uint32
+	var dacl *byte
+
+	err = system.GetSecurityDescriptorDacl(&securityDescriptor[0], &daclPresent, &dacl, &daclDefaulted)


same here (not a blocker)

if err := system.GetSecurityDescriptorDacl(&securityDescriptor[0], &daclPresent, &dacl, &daclDefaulted); err != nil { return err }

thaJeztah · 2018-08-17T12:19:36Z

builder/dockerfile/internals_windows.go

+	if strings.HasPrefix(accountName, "S-") || strings.HasPrefix(accountName, "s-") {
+		sid, err := windows.StringToSid(accountName)
+
+		if err == nil {


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

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

Alright, thanks for checking!

thaJeztah · 2018-08-17T12:20:22Z

builder/dockerfile/internals_windows.go

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


nit: stray empty line

thaJeztah · 2018-08-17T12:21:11Z

builder/dockerfile/internals_windows.go

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


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

thaJeztah · 2018-08-17T12:23:22Z

daemon/archive_tarcopyoptions_unix.go

 		return nil, err
 	}

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


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

thaJeztah · 2018-08-17T12:29:49Z

pkg/archive/archive.go

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


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)

salah-khan requested review from cpuguy83, dmcgowan, dnephin, johnstep, lowenna and tonistiigi as code owners November 16, 2017 06:50

GordonTheTurtle added the status/0-triage label Nov 16, 2017

salah-khan force-pushed the 35507 branch from f368570 to f63a748 Compare November 21, 2017 01:20

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 suggested changes Nov 21, 2017

View reviewed changes

johnstep reviewed Nov 21, 2017

View reviewed changes

builder/dockerfile/internals.go Outdated

Copy link
Copy Markdown

Member

johnstep Nov 21, 2017

Choose a reason for hiding this comment

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

This else is unnecessary since the if above always returns.

thaJeztah added status/2-code-review area/builder Build platform/windows and removed status/0-triage labels Nov 21, 2017

thaJeztah reviewed Nov 21, 2017

View reviewed changes

thaJeztah mentioned this pull request Nov 28, 2017

Fix MkdirAll* and its usage #35618

Merged

GordonTheTurtle assigned cpuguy83 Dec 1, 2017

johnstep reviewed Dec 9, 2017

View reviewed changes

builder/dockerfile/copy.go Outdated

Copy link
Copy Markdown

Member

johnstep Dec 9, 2017

Choose a reason for hiding this comment

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

Typo

johnstep reviewed Dec 9, 2017

View reviewed changes

salah-khan requested review from AkihiroSuda, tianon and vdemeester as code owners December 14, 2017 04:08

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Dec 14, 2017

salah-khan force-pushed the 35507 branch from 674224c to 032e65d Compare December 14, 2017 04:49

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Dec 14, 2017

GordonTheTurtle removed rebuild/powerpc labels Mar 16, 2018

johnstep added the rebuild/z label Mar 16, 2018

GordonTheTurtle removed the rebuild/z label Mar 16, 2018

salah-khan force-pushed the 35507 branch from 18980fc to 891c7d9 Compare March 22, 2018 18:58

thaJeztah reviewed Mar 22, 2018

View reviewed changes

salah-khan force-pushed the 35507 branch from 891c7d9 to 90acb5b Compare March 26, 2018 21:37

johnstep added the rebuild/experimental label Mar 27, 2018

GordonTheTurtle removed the rebuild/experimental label Mar 27, 2018

johnstep added the rebuild/experimental label Mar 27, 2018

GordonTheTurtle removed the rebuild/experimental label Mar 27, 2018

vdemeester suggested changes Apr 6, 2018

View reviewed changes

thaJeztah requested changes Apr 6, 2018

View reviewed changes

thaJeztah mentioned this pull request Apr 6, 2018

Secrets on Windows - grant permissions to specific users/groups #34120

Open

salah-khan force-pushed the 35507 branch 2 times, most recently from 348735d to d25f9c5 Compare April 12, 2018 02:12

johnstep added the rebuild/z label Apr 12, 2018

GordonTheTurtle removed the rebuild/z label Apr 12, 2018

johnstep added the rebuild/z label Apr 12, 2018

GordonTheTurtle removed the rebuild/z label Apr 12, 2018

johnstep added the rebuild/z label Apr 12, 2018

salah-khan commented Aug 14, 2018

View reviewed changes

builder/dockerfile/internals_windows.go Outdated

Copy link
Copy Markdown

Contributor Author

salah-khan Jul 26, 2018

Choose a reason for hiding this comment

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

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

thaJeztah approved these changes Aug 17, 2018

View reviewed changes

thaJeztah mentioned this pull request Sep 18, 2023

why do we need containerutility.exe? #41776

Closed

thaJeztah mentioned this pull request Feb 28, 2025

vendor: switch from idtools to moby/sys/user moby/buildkit#5791

Merged

coderabbitai bot mentioned this pull request Mar 17, 2026

feat: improve Windows PowerShell shell analysis wharflab/tally#311

Merged

Conversation

salah-khan commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

johnstep Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnstep commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GordonTheTurtle commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnstep commented Mar 27, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salah-khan commented Nov 16, 2017 •

edited

Loading

johnstep Nov 21, 2017 •

edited

Loading

johnstep commented Nov 28, 2017 •

edited

Loading