Don't set a default PATH for Windows

[thaJeztah]

• introduced in dockerfile: set correct default path and shell based on OS #1747
• relates to / (partially) addresses Unable to use Buildkit with Windows containers #616
  • specifically Unable to use Buildkit with Windows containers #616 (comment)
• relates to pkg/system: move some functions to a new home moby#44275 (comment)

Commit ecf070a (#1747) updated DefaultPathEnv to return platform specific values, but also defined a default PATH for Windows.

However, Windows does not have a default PATH, and the PATH to use must be set by the container image. Also see moby/moby@3c177dc, which removed the default PATH on Windows;

This is deliberately empty on Windows as the default path will be set by
the container. Docker has no context of what the default path should be.

[thaJeztah]

/cc @TBBle @tonistiigi @corhere

[thaJeztah]

Right, so this is failing currently; I wonder if some code path sets the default from the host if nothing is set (looks like it may be inheriting from the current environment?) I'm not familiar enough with the code here

=== RUN   TestClientGatewayIntegration/TestClientGatewayContainerPlatformPATH/worker=containerd/windows_path
    build_test.go:1201: 
        	Error Trace:	build_test.go:1201
        	Error:      	Not equal: 
        	            	expected: ""
        	            	actual  : "/sbin:/usr/sbin:/bin:/usr/bin"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-
        	            	+/sbin:/usr/sbin:/bin:/usr/bin
        	Test:       	TestClientGatewayIntegration/TestClientGatewayContainerPlatformPATH/worker=containerd/windows_path

I can work on that, but perhaps first see if there's objections against this change

[TBBle]

As described in the linked comment, this change makes sense to me, although I haven't reviewed the patch. As noted, this was on my list of "hacked around and will dig into later to try and make it work correctly" patches in my experimental (now very old) branch.

I am much too far temporally from this codebase to guess at where that test failure is coming from. If it's inheriting from the test-execution environment, that's mildly concerning, but such a behaviour may have been being covered up by the current behaviour of always setting the path to a fixed value in the cases where it might have been inherited from outside?

[corhere]

Seeing as how the empty string is (almost?) always a special case, I think https://github.com/golang/go/wiki/CodeReviewComments#in-band-errors applies here.

Suggested change

	func DefaultPathEnv(os string) string {
	func DefaultPathEnv(os string) (string, bool) {

[thaJeztah]

updated; added the bool return.

[tonistiigi]

I don't understand where that "must be set by the image" is coming from. Yes, in wcow environment variables can be part of the rootfs tars because of the registry files, but they can also be in image configuration.

I don't see any reason to add these exceptions to the codebase where they need to be understood by the devs and also by people writing the Dockerfiles. To have a reasonable behavior that works with ENV in Dockerfiles the images should define PATH in the config and users who wish to change it should do it with ENV command, the same way as the Linux users do. Yes, there is another method to do it but it doesn't mean it needs to be used.

[thaJeztah]

Windows is... different.

First of all, Windows doesn't support FROM scratch, so every Windows image will have a "Windows" flavour as base image. It's up to that base image to define the PATH (in whatever form it does).

So unless your creating a base image (which means you're either Working for Microsoft, or violating your Windows License), the PATH should be defined by the base image, so the "default" is determined by the base image.

Of course, as an image author you may override that default, but BuildKit itself should not try to come up with a default, because there isn't any.

To be fair, I don't think we should've set a default for non-Windows images either (and also have let it up to the base image (if any)), but that ship sailed. Changing that would be a breaking change, but so is #1747, which changed the default to be different from any container system created in the past 7 Years).

[tonistiigi]

I agree that the base image should set the PATH. The default we have should only be a fallback that is almost never used(it is almost never used in Linux as well). People who create Windows images should set the PATH in the image config the same way people who create Linux images do, not ask tools to add and maintain a bunch of exceptions for them. This is also confusing for the users when ENV PATH=$PATH has unexpected behavior.

[thaJeztah]

The issue is that (but don't ask me about details, @TBBle or @kevpar may be able to explain it better), that setting a PATH breaks things on Windows.

For example, take the image config for mcr.microsoft.com/windows/servercore:ltsc2022 does NOT have a PATH;

docker buildx imagetools inspect --raw mcr.microsoft.com/windows/servercore:ltsc2022@sha256:06bb218927a8a95b424acfad0e0e6ae0fb5694c0b510f9930ed5881075eaec2f | jq .
{
  "architecture": "amd64",
  "config": {
    "Hostname": "",
    "Domainname": "",
    "User": "",
    "AttachStdin": false,
    "AttachStdout": false,
    "AttachStderr": false,
    "Tty": false,
    "OpenStdin": false,
    "StdinOnce": false,
    "Env": null,
    "Cmd": [
      "c:\\windows\\system32\\cmd.exe"
    ],
    "Image": "",
    "Volumes": null,
    "WorkingDir": "",
    "Entrypoint": null,
    "OnBuild": null,
    "Labels": null
  },
  "created": "2022-10-07T22:13:48.3974633Z",
  "history": [
    {
      "created": "2022-04-22T01:12:09.4542389Z",
      "created_by": "Apply image 10.0.20348.643"
    },
    {
      "created": "2022-10-07T22:13:48.3974633Z",
      "created_by": "Install update 10.0.20348.1129"
    }
  ],
  "os": "windows",
  "os.version": "10.0.20348.1129",
  "rootfs": {
    "type": "layers",
    "diff_ids": [
      "sha256:b9bda40f596a8f8648c218b23e552acf9498c039eb0a48c222cfcd5a9af9e841",
      "sha256:7a72bd51b9c8b0510fea5addf0b46e5de102813083abe316d4603a050e7ad473"
    ]
  }
}

When I tried to update the classic builder to use the default PATH from this repository, things break. Not exactly sure, but I somewhat expect that in this case either cmd or PowerShell define the path, but when setting a PATH as environment variable, that overrides that default, rendering the image broken;

Step 1/13 : ARG WINDOWS_BASE_IMAGE=mcr.microsoft.com/windows/servercore
Step 2/13 : ARG WINDOWS_BASE_IMAGE_TAG=ltsc2022
Step 3/13 : ARG BUSYBOX_VERSION=FRP-3329-gcf0fa4d13
Step 4/13 : ARG BUSYBOX_SHA256SUM=bfaeb88638e580fc522a68e69072e305308f9747563e51fa085eec60ca39a5ae
Step 5/13 : FROM ${WINDOWS_BASE_IMAGE}:${WINDOWS_BASE_IMAGE_TAG}
ltsc2022: Pulling from windows/servercore
97f65a0ec59e: Pulling fs layer
4486102fd382: Pulling fs layer
4486102fd382: Verifying Checksum
4486102fd382: Download complete
97f65a0ec59e: Verifying Checksum
97f65a0ec59e: Download complete
97f65a0ec59e: Pull complete
4486102fd382: Pull complete
Digest: sha256:2846453bcfaee661d563e29d6513191dccf7280bb826b2da9d6e7226fba2db94
Status: Downloaded newer image for mcr.microsoft.com/windows/servercore:ltsc2022
 ---> 2caccf6918d4
Step 6/13 : RUN mkdir C:\tmp && mkdir C:\bin
 ---> Running in 320b82116e6d
Removing intermediate container 320b82116e6d
 ---> 5e9277f6f4b4
Step 7/13 : ARG BUSYBOX_VERSION
 ---> Running in a85cdf300ed4
Removing intermediate container a85cdf300ed4
 ---> a0d354994a80
Step 8/13 : ARG BUSYBOX_SHA256SUM
 ---> Running in 99f510599835
Removing intermediate container 99f510599835
 ---> ae8d3ccee635
Step 9/13 : ADD https://frippery.org/files/busybox/busybox-w32-${BUSYBOX_VERSION}.exe /bin/busybox.exe

 ---> 0c2ef53e2a7c
Step 10/13 : RUN powershell     if ((Get-FileHash -Path /bin/busybox.exe -Algorithm SHA256).Hash -ne $Env:BUSYBOX_SHA256SUM) {         Throw \"Checksum validation failed\"     }
 ---> Running in 65d36517920b
'powershell' is not recognized as an internal or external command,
operable program or batch file.
The command 'cmd /S /C powershell     if ((Get-FileHash -Path /bin/busybox.exe -Algorithm SHA256).Hash -ne $Env:BUSYBOX_SHA256SUM) {         Throw \"Checksum validation failed\"     }' returned a non-zero code: 1

I guess this is where the (LONG ) discussions we had about moby/moby#29048 come to play; the PATH environment variable cannot be read from the environment, but must be read back from the process that's running (e.g. PowerShell or cmd.exe).

So probably the only options for the builder are;

• Don't set PATH and acknowledge that "we don't know"
• Execute the container's process and ask it what the variable is (implement GETENV)

[TBBle]

The breakage above happens because PowerShell lives in C:\WINDOWS\System32\WindowsPowerShell\v1.0\ which is in-the path on systems with PowerShell present (and hence not on nanoserver). See also microsoft/terminal#7418 for other env-var traps in process launching (again, with PowerShell).

moby/moby#31525 (comment) seems to be the best reference for how we got here. The ENV in Dockerfile and the registry-held persistent env-vars (both machine and user) are parallel sources of truth, and the simplest workaround is to just not use ENV PATH in Windows Dockerfiles, in favour of RUN cmd /c setx /m PATH=${PATH};.... (Or RUN reg <options that append to a multi-string value> to save you using ${PATH}, etc.)

This is most-compatible with installers that add paths to the registry as well. Once you've ENV PATH'd once, you then need to ENV PATH after each installer executes too as you lose access to all past and future registry-based env-path changes.

That's not a great workaround, as it puts extra load on the Dockerfile writer, and requires RUN, which blocks non-executable-environment layer-editing tools. That's also true of GETENV as described, although if some brave soul wants to use a hive-loading library, e.g., https://github.com/gabriel-samfira/go-hivex, to implement GETENV (or really env-substitution) without booting a container, that option exists and I think it less-awful than it might seem at-first. (Still not great though).

Along those lines, making Windows-platform builders transform Dockerfile ENV into a RUN setx instead of writing to the container image config would resolve down to a single source of truth, with the same limitations as above and with extra build-time cost of launching another container for the ENV command, which would be surprising to long-time users.

As I understand, there's no good way to tell a Windows process being launched to append an env-var to the registry-provided values, since the Windows process launch takes a single set of env-vars, and it's up to the launching process to assemble them from either copying the existing environment, or requesting a new environment block from the OS and hence using only the registry (basically only explorer and similar process launchers do the latter, and we can't do it outside a running container anyway). This is where Windows's approach differs from Linux, where if you need to append to the path, you add an export PATH=${PATH}; to the end of .bashrc (user) or /etc/environment (system), and hence it's trivially additive across container image layers as those are processed after the Dockerfile ENV, while on Windows, Dockerfile ENV hides the system and user env-vars.

A more esoteric solution would be some wild inversion of the above, which I am not currently volunteering to write: At the end of every RUN, the Windows registry env-vars are exported into the container config ENV block (matching the merge/override behaviours as they are processed in Windows, perhaps with a secret RUN env where we don't keep the scratch layer, or using a hive-reading library). That way Dockerfile ENV is the canonical way to make changes, but changes by installers or setx /m are correctly captured as well.

(Year-later edit: This last idea is me being 6 years late to the table with GETENV as mentioned in the previous comment, and it's already been rejected for performance reasons).

The similar issue around stale env-vars in Windows Terminal tabs has been around for years without successful resolution. It's slightly more complex because its two sources of truth overlap (one contains an old version of the other, so there's a three-way merge in the mix), where here a fairly simple merge is sufficient.

[gabriel-samfira]

Adding my 2 cents:

There is also the annoying fact that a potential installer may set just the system PATH or the user PATH...or both. So the PATH will differ based on what we set in the USER stanza.

For example:

USER ContainerAdministrator
RUN echo %PATH%
USER gabriel
RUN echo %PATH%

The two RUN commands above may yield different results, and that may be something that is expected by the consumers of the image.

Not setting the PATH as an env var is, I think, desirable on Windows.

[puetzk]

You really don't want to do setx /m PATH=${PATH};... either, because that takes expanded value of PATH from the current process environment (where it includes appended per-user values, expanded substitutions of any REG_EXPAND_SZ %OTHER% references, etc) and puts that whole string back into HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Session Manager\Environment. So your per-user stuff has leaked over to other users, any references are now copied-by-value, your own user values will be appended a second time (and thus become duplicated if setx is used like this a second time...).

[TBBle]

As far as I know, this is the last open issue that prevents generally-found-on-the-Internet Dockerfiles from building correctly.

The are two core difference from Linux containers at work here.

First is that Linux does not have a canonical, always-used place to store registry keys; they are generally applied by the shell, and in a container, you often don't have a shell, so the common tool idiom of "append to /.<shell>rc" doesn't work.

In Windows, we do have a single canonical OS store for environment variables, that tools know how to update, and which is applied (and expected to be applied) even if you don't run a shell.

Secondly, in Linux, most tools install into known standard locations, e.g., /usr/bin and so do not rely on environment variable changes such as PATH to be found and functional. Tools that sandbox applications, such as snap and flatpak, or installations into /opt, either put symlinks into some in-PATH directory, or are just a hassle to use.

In Windows, there is no such standard location, and the expected behaviour for all installers is like the /opt setup on Linux (and more-recently, full sandboxing), which implies PATH modification because there's no in-PATH directory to drop a symlink into. Tools like chocolatey have to maintain their own in-PATH directory to drop symlinks into when packaging tools that do not have Windows-native packages, so they still do the PATH modification once, at tool installation.

In both cases, single-binary drop-ins, cross-stage copies, or in-image-compiled binaries tend to require a PATH modification as no installer is used.

---

So if we want to make the Dockerfile ENV PATH (or all of ENV) canonical, we need one or more of the following changes:

• take the union of all possible default paths in all base images as the hard-coded default path
• have the upstream base image build process changed to export the PATH env-var for some user (Probably ContainerAdmin) as a final step. (hardcoded in the build process, or using something like GETENV)
• Implement GETENV and after any RUN (or explicitly?) re-export the current user's calculated PATH (or all of ENV) back into the image config to capture changes by installers.
• Require users to hardcode the PATH-modification (and any other ENV changes) of any installer or installers run in any build step back into the Dockerfile.
• Reading the default PATH or all of ENV from the image registry files or with RUN when not set in the image config file.
• Co-ordinate with HCS implementation to ensure it doesn't generate an environment block from the registry.

This produces a similar experience for Linux containers, however it produces work for Windows users because unlike Linux, there's no rational "default path" (**/bin in various forms) where most installers install their stuff; most Windows installers add to the PATH if they need to be command-line callable. So you need to follow RUN with ENV PATH="$PATH;C:\Program Files\Thing I just Installed\1.2\bin" much more often than you do in Linux.

The alternative is that we allow the usual Windows path-management process to function, which requires some combination of the following:

• Not setting a default PATH env-var.
• Disrecommending ENV PATH=$PATH:... (or all of ENV, even) in WCOW Dockerfiles in favour of RUN setx /M PATH=%PATH%:..., for cases where an installer was not used or did not update the registry record already.
• Actually redirect the ENV command in Windows containers to be RUN setx /M, hence never actually ending up with an ENV block in a WCOW container image.

My personal preference leans towards the second approach, simply because I expect Windows Containers to be Windows-idiom containers, i.e. Windows-based developers are who I consider the target audience for Windows Containers.

[thaJeztah]

Marked this as deprecated, because there is no default to be set

[thaJeztah]

And updated this function to return a bool to indicate whether there is a default; also tried to capture some of this in the function's GoDoc, and links to relevant discussions.

[thaJeztah]

Looks like doing so broke tests; is PATH already set somehow? (but to a linux default path?)

[thaJeztah]

Ah, hm, right, so test is running a busybox image on Linux;

buildkit/client/build_test.go

Lines 1370 to 1371 in 6a0cd7c

	func testClientGatewayContainerPlatformPATH(t *testing.T, sb integration.Sandbox) {
	requiresLinux(t)

And checks the output of echo $PATH, with the expectation that that is changed because of the windows argument 🤔;

buildkit/client/build_test.go

Lines 1418 to 1421 in 6a0cd7c

	pid1, err := ctr.Start(ctx, client.StartRequest{
	Args: []string{"/bin/sh", "-c", "echo -n $PATH"},
	Stdout: &nopCloser{output},
	})

Not sure what should be tested there; at most, we could test if the $PATH matches the original PATH env from the image's config (if set)

[TBBle]

Hmm, yeah. Seems like an inappropriate test to me, since we don't actually support RUN steps for Windows images on non-Windows AFAIK. In fact, why doesn't c.NewContainer or ctr.Start fail here? It seems weird to me that this ever got this far, but maybe the "we don't support this" or "inconsistent request for Windows platform on a Linux-annotated image" failure-handling is at a higher level.

We will probably want Windows-side tests to ensure that we aren't breaking PATH (including covering setx /M and setx across a couple of users to ensure we aren't leaking PATH changes across users. Since we don't run the integration test suite on Windows yet, we can probably defer adding that test, and just drop the "windows path" branch of this one.

[tonistiigi]

(The discussion here has gone beyond the scope of the PR and would be better in an issue, but I guess it is too late now. My comment is for the general discussion, not in response to the current code.)

So, to the question of what to do with PATH on windows my position is:

1. Windows images should define PATH in their image config like Linux images do. This is not only a question about how we build images otherwise, how to avoid Windows-specific hacks from the maintenance side and extra cognitive load for user, but also needed from purely inspection/auditing side. Especially with all the provenance features we have been adding.
2. As a build tool, we can make the migration from images that don't define PATH in image config to the ones that do easier. For example, when Dockerfile returns image without a PATH we should show a warning. In a more pedantic mode (or maybe default) we could check at the end of the build that the registry value and image metadata value match and warn or error if they don't. I'm not a fan of GETENV because it is an absurd concept for unix (and existing Dockerfile) users who know that do read env you just use $. But I might be open to for example a build-arg that auto-reads PATH value to the image config from base image. And maybe if the case of updating PATH when new software is installed is hard, it can be something like RUN --windows-update-path ./my-installer. All these should be implemented in the frontend(Dockerfile) level, running additional LLB containers to read the values. I don't really want any of these hacks to leak to LLB API.

[TBBle]

(Agree about the PR-issue thing. >_< I probably should have pushed out into an issue when I revived this last week, since I was already looking past the scope of the PR due to the existing discussions suggesting a different approach. Sorry. I'll try and make some time this weekend to create a ticket and summarise the high-level discussion status, but I can't commit that right now.)

For GETENV, my use of it here was intended to be what you described as a build-arg, an internal detail used to support ENV and/or RUN behaviours rather than a Dockerfile command users need to know about. It just happens that the existing proposed implementation for moby was named that, even though I think it was also not supposed to be visible in Dockerfile.

I had indeed been considering the idea that a Dockerfile frontend fork could be implemented that handles RUN and/or ENV to make this transparent to the user, even exploring both solution spaces. I haven't actually tried a Dockerfile frontend on Windows BuildKit yet, so it was only idle speculation.

However, I'm not really clear what benefit keeping PATH in the image config rather than in the image layer provides to provenance, inspection, and auditing, given that what would be calls to ENV PATH would instead be an equivalent call to RUN setx PATH in the history; anything that changes the PATH as a side-effect in the Windows image (a Windows installer) would equally be invisible in a Linux container history by virtue being an "install into /**/bin" action inside an install script. The container registry is of course part of the image, so attempting to change the path underneath would change the layer hash.

Also, to be clear... Just PATH? The actual PATH env on Windows has other env-vars interpolated into it at environment-block creation (RUN for our purposes), so we either need to extract all those too, or only deal with the post-interpolation version and hence generate surprise for, e.g. calls to nvm use failing. (I actually don't know off-hand if the layer config PATH env can do interpolation like that, or if the builder needs to resolve all such things completely.) (Also, nvm use is a bad example for a useful thing in a container. It just happens to be the first entry I saw in my system's path that relies on process-creation-time env-var interpolation. Go and dotnet use it too for %USERPROFILE%, but that shouldn't change between runs. Per-user config gets involved here... Huh. I just realised BuildKit and Docker include sbin in the default path even for non-admin users on Linux. I never noticed that.)

[gabriel-samfira]

In the absence of an issue, I'll just pile on here >_<.

1. Windows images should define PATH in their image config like Linux images do. This is not only a question about how we build images otherwise, how to avoid Windows-specific hacks from the maintenance side and extra cognitive load for user, but also needed from purely inspection/auditing side. Especially with all the provenance features we have been adding.

There is a longer discussion to be had here about what should be considered a hack, and what behaviour is actually native to the operating system that we simply need to account for.

Windows has a different relation to environment variables in a few key ways:

• All environment variables are stored in registry hives
  • There are user specific registry hives that contain environment variables for specific users
  • Some environment variable values may differ from the system environment variables
• Applications may be installed that set up different env variables or different values for existing env variables based on user/scope
• Some applications will not work if an environment variable is set up incorrectly or not at all
  • To name a few: TEMP (needed by os.TempDir() for example) and PsModulePath (needed by powershell to find its modules), SystemRoot, USERPROFILE. There are more.
• Almost all windows applications will fail to function if you boot up a system with an empty env.

As @TBBle has stated, the PATH variable is a special case here, because in Linux, it must be set explicitly, either in a shell rc file or as an explicit variable before you start any app that needs to call a binary without giving a full path.

On Windows, depending on the application you're running a number of variables may have equal or greater importance to PATH.

The handling of environment variables is baked into the OS itself, and not asked of the user (at least not for their initial value - more details follow). Users have the ability to augment variable values, but it's not expected for them to set the initial values. This is also for a few technical reasons. Windows can have many root file systems. Every drive letter is a root filesystem in Windows. And while containers are limited to C:\, on a host, windows can be installed in D:\ for example. All relevant environment variables are set at install time for this reason as well.

Any installer that needs to change or set an environment variables that deals with paths will need to first interrogate existing environment values, and use those values to compute any changes it needs to apply. Any changes done will persist in registry hives.

As you can imagine, the container version of Windows is just a stripped down version of Windows itself, so the plumbing and assumptions described above, carry over from Windows proper.

As far as Windows users are concerned, the PATH variable is equally important to the PSModulePath variable or the PATHEXT variable (which dictates which extensions mark files that can be executed - .exe, .bat, etc).

While having 1:1 feature and assumption parity between Linux and Windows is desirable, and something I've always tried to strive for when integrating Windows into various well established projects (I love projects that are agnostic and provide a seamless experience regardless of OS), in some cases it simply is not attainable. Either because it breaks assumptions users may have in regard to the way their application behaves on a platform (ie: my app wants to set system wide PATH to include different folders than user PATH), or because the functionality simply isn't there.

I don't really want any of these hacks to leak to LLB API.

Completely agree. If there's no choice moving forward but to add the PATH variable explicitly, it will most likely be done in frontend. However, I do hope that for Windows at least, we can just skip defaulting to a PATH if one is not set and leave windows to its own devices. There was a recent issue in containerd that was handled in a similar fashion (and which reminded me of this PR):

• Fix windows default path overwrite containerd/containerd#9118