fix: acquire privileges to access special files on windows

[profnandaa]

fixes #4985

Windows has some special / metadata files that need special privileges to access, even if the process is running as Admin.
Some of those files (across nanoserver and servercore) currently are:

\\Program Files\\Windows Defender Advanced Threat Protection\\Classification\\Configuration
\\ProgramData\\Microsoft\\Windows Defender Advanced Threat Protection\\Cache
\\ProgramData\\Microsoft\\Windows Defender Advanced Threat Protection\\Cyber
\\ProgramData\\Microsoft\\Windows Defender Advanced Threat Protection\\DLP
\\ProgramData\\Microsoft\\Windows Defender Advanced Threat Protection\\DataCollection
\\ProgramData\\Microsoft\\Windows Defender Advanced Threat Protection\\Downloads
\\ProgramData\\Microsoft\\Windows Defender Advanced Threat Protection\\Platform
\\ProgramData\\Microsoft\\Windows Defender Advanced Threat Protection\\SenseCM
\\ProgramData\\Microsoft\\Windows Defender Advanced Threat Protection\\SenseNDR
\\ProgramData\\Microsoft\\Windows Defender Advanced Threat Protection\\Temp\\PSScriptOutputs
\\ProgramData\\Microsoft\\Windows Defender Advanced Threat Protection\\Trace
\\System Volume Information
\\WcSandboxState
\\Windows\\System32\\LogFiles\\WMI\\RtBackup
\\Windows\Globalization\\ICU\\icudtl.dat

Additionally, we have directories that don't need to be exported, I have separated that work in a separate issue here #5011

Also, I'm currently working on enabling the skipped integration tests on Windows, and most of them are using local exporter, therefore will be depending on this change e.g.:

buildkit/frontend/dockerfile/dockerfile_test.go

Lines 348 to 359 in eed17a4

	_, err = f.Solve(sb.Context(), c, client.SolveOpt{
	Exports: []client.ExportEntry{
	{
	Type: client.ExporterLocal,
	OutputDir: destDir,
	},
	},
	LocalMounts: map[string]fsutil.FS{
	dockerui.DefaultLocalNameDockerfile: dir,
	dockerui.DefaultLocalNameContext: dir,
	},
	}, nil)

--

Sample Test

buildctl build `
--frontend=dockerfile.v0 `
--local context=C:\dev\play\dockerfiles\export_local `
--local dockerfile=C:\dev\play\dockerfiles\export_local `
--output type=tar,dest=C:\dev\tmp\exp-2.tar `
--progress plain --no-cache

Before:

#7 exporting to client tarball
#7 sending tarball
#7 sending tarball 0.1s done
#7 ERROR: open C:\Users\nandaa\AppData\Local\Temp\buildkit-mount91402518\System Volume Information: Access is denied.

After fix:

#7 exporting to client tarball
#7 sending tarball
#7 sending tarball 30.6s done
#7 DONE 32.1s

[profnandaa]

Ref: #4837 (comment)

[gabriel-samfira]

There might be files you forgot to commit, as I don't see where the list of files you mentioned are excluded.

As for the windows specific privilege escalation, I would only apply it in the local exporter:

buildkit/exporter/local/export.go

Lines 173 to 176 in f9b730c

	eg.Go(export(ctx, p.ID, r, inp.Attestations[p.ID]))
	}
	} else {
	eg.Go(export(ctx, "", inp.Ref, nil))

There is no need to do that in fsutil. The functions in fsutil may be used in multiple situations and I don't think all those situations will require escalated privileges.

[profnandaa]

@gabriel-samfira -- so, I did manage to do for type=tar but for local, wrapping session/filesync/diffcopy.go:25 block didn't have any effect. Even tried separating out fsutil.Send(stream.Context(), stream, fs, progress) alone, same.

Might you have any pointers? My hunch is that it's because the call breaks into goroutines and escapes the privilege binding?

buildkit/session/filesync/diffcopy.go

Line 25 in aebcc1f

return errors.WithStack(fsutil.Send(stream.Context(), stream, fs, progress))

I agree, would have been nice not to touch fsutil.

[gabriel-samfira]

My hunch is that it's because the call breaks into goroutines and escapes the privilege binding?

I think that's indeed what's happening. The RunWithPrivileges function locks the current thread for just the function that is about to run, then escalates privileges. At the end it releases the thread and restores previous privileges.

If the execution happens in another thread, that thread won't have the same privileges.

One more heavy handed way to do this is to try to escalate privileges process-wide using:

https://github.com/microsoft/go-winio/blob/bdc6c112805ce13d7b235dc6257f258d958f5471/privilege.go#L108

something like

winio.EnableProcessPrivileges([]string{winio.SeBackupPrivilege})
defer winio.DisableProcessPrivileges([]string{winio.SeBackupPrivilege})

// do the thing

Enabling process wide privileges is not ideal if it's being done in multiple places of the code base. There is a chance (I am not vastly familiar with internals) that if you revoke the privilege once you are done in a defer, you may remove the privilege, while it's needed by some other go routine somewhere else in the code.

There is one other place that EnableProcessPrivilege is used in fsutil, here:

https://github.com/tonistiigi/fsutil/blob/91a3fc46842c58b62dd4630b688662842364da49/copy/copy_windows.go#L62-L74

That can actually be converted to RunWithPrivileges as it only sets the security descriptor on a file and can run in the same thread. But my point is that enabling/disabling privileges process wide needs to be done carefully. And we need to be aware of the potential hard to debug scenarios when something fails intermittently without obvious reason.

Adding this to fsutil might make sense, but it feels equally heavy handed to require elevated privileges for every Walk(), even when it's called for paths that we should have access to normally. The fsutil package is generic. The case we're trying to address here is specific to the particular need we have here.

@tonistiigi what are your thoughts here?

[profnandaa]

Thanks for that. Sure, I agree we need caution for process-wide escalation.

[profnandaa]

Then as for the file skipping, also this needed to be at the place walking is happening, which is in fsutil; I though of leaving that out so not to pollute the fsutil too much?

Right now those metadata files are being exported which I think it's okay, to have the end user deal with the files as they so wish?

    Directory: C:\dev\tmp\sample-1

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d----            5/8/2021  9:43 AM                ProgramData
d----           5/31/2024  4:36 PM                System Volume Information <----
d----           5/10/2024 11:17 PM                Users
d----            6/7/2024 10:44 AM                WcSandboxState <----
d----           5/10/2024 11:17 PM                Windows
-a---            6/6/2024 12:44 PM             97 hello.txt
-a---           5/10/2024 11:14 PM           5647 License.txt

[gabriel-samfira]

The WcSandboxState should be skipped. It gets created by the host OS when mounting layers. It is of no use to users.

It may be worth asking the server team about this.

[profnandaa]

Sure, I can add that the exclusion, the team was okay with skipping that, was listed among what's skipped in their layer exports:

"\\System Volume Information",
"\\ProgramData\\Microsoft\\Diagnosis",
"\\WcSandboxState",

Do you suggest adding that in the fsutil side or sending it from the caller?

[gabriel-samfira]

sending it from the caller makes more sense. If fsutil allows setting an exclusion list, that would be great.

[profnandaa]

opened a separate issue for that here #5011

[billywr]

recalling my approval, I didn't notice PR is still in draft mode....

[profnandaa]

@gabriel-samfira @tonistiigi PTAL, also updated the PR description.

[gabriel-samfira]

With the caveat that I already mentioned, this looks ok.

[profnandaa]

@tonistiigi @thompson-shaun -- moved it to milestone v0.14.2, hope that's ok?