-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Windows] Add get-user-info utility #3516
[Windows] Add get-user-info utility #3516
Conversation
This utility allows us to mount the buildkitd executable inside a Windows container and fetch the SID of any existing user. This is to work around the fact that the SAM hive data structures are undocumented and there is no API to inspect an offline SAM hive to fetch the security info of an existing user. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be called inside a Windows container? It looks similar to: https://github.com/containerd/containerd/blob/main/integration/images/volume-ownership/tools/get_owner_windows.go. Might be worth to have a dedicated repo for this utility?
"encoding/json" | ||
"fmt" | ||
"os" | ||
"syscall" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use "golang.org/x/sys/windows"
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x/sys
is where new stuff gets added. The syscall
package only changes when those changes need to support the standard library.
The LookupSID()
function from the syscall
package is identical to the one in x/sys/windows
, and calls into a stable Windows API which, judging by Microsofts track record for backwards compatibility, will probably remain unchanged for many many years.
Unless we want to use an old version of go
that does not have this function in the standard library, I think we can use syscall
.
If you prefer, I can change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are already using this pkg
"golang.org/x/sys/windows" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My recollection of a discussion in the go-winio repo was that we were leaning towards preferring x/sys/windows
over syscall
where possible. I have a PR there for example which replaces all uses of syscall
with x/sys/windows
versions.
Which I would like to come back to at some point; it was delayed because there was a syscall.Handle
in a public API being changed to windows.Handle
and they wanted to batch that up some other API breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it's settled. Will replace with x/sys/windows
ASAP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, the PR and discussion I mentioned is microsoft/go-winio#197.
os.Exit(1) | ||
} | ||
username := os.Args[1] | ||
sid, _, _, err := syscall.LookupSID("", username) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sid, _, _, err := syscall.LookupSID("", username) | |
sid, _, _, err := windows.LookupSID("", username) |
The short answer is: yes. This is meant to be called inside a Windows container, whenever Slightly longer answer bellow: This utility is meant to be used here: https://github.com/moby/buildkit/pull/3517/files#diff-8265d89f801913b3b2f4c1585304405c661d143ede3ed61ec6885d0581cc1650R96-R105 Which is made available to the container, here: https://github.com/moby/buildkit/pull/3517/files#diff-9d45df0583b268e8e8fee8d2a71f04728c1ba11519660b6702f00d3f3e0d55c5R18-R30 A more detailed explanation for the need for this, can be found here: https://github.com/moby/buildkit/pull/3517/files#diff-8265d89f801913b3b2f4c1585304405c661d143ede3ed61ec6885d0581cc1650R52-R66 This spares us from having to ship a separate binary along side The idea is to be as self contained as possible. This felt like the best course of action. A separate binary is also possible if preferred. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PTAL @TBBle
Seems short and reasonable to me, in isolation. I'm still on sabbatical so haven't gone through the rest of the (greatly appreciated) work done around this. I'm not sure off-hand where in the process re-exec is supposed to drop privs; I'm actually not sure if that's necessary here either, it's just something I recall being noted around the re-exec system in dockerd (in the Windows graphdriver code) and also scattered across the Windows filesystem code in containerd. I also don't have a particular risk-vector in mind. |
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
This similar to https://github.com/docker-archive/windows-container-utility ? (something we should consider swapping in moby ?) |
This returns a marshaled #10 [6/6] RUN get-user-info administrator
#10 1.662 {"UID":0,"GID":0,"SID":"S-1-5-21-2702878673-795188819-444038987-500"}
#10 DONE 2.1s It could be extended to do more if needed, and spares us from shipping a separate binary. |
So there's two things that should be verified (I haven't looked closely how/where this exactly is used);
not sure if any of that applies here, but (just in case it's relevant) 😅 |
Ahh. Interesting. But the proposed PR that uses this, mounts Edit: Here is where this gets mounted: https://github.com/moby/buildkit/pull/3517/files#diff-9d45df0583b268e8e8fee8d2a71f04728c1ba11519660b6702f00d3f3e0d55c5R18-R30
I am not extremely familiar with the internals of how Windows containers manage memory, but I expect that the host and the container running on top don't share DLLs loaded into memory. As soon as the container exists, I expect anything loaded by those containers into memory would get released. Any user space apps on the host should be unaffected by what DLLs get loaded in guests. Will investigate in either case. |
Yes, situation on Windows could be "different". The issue in Moby was related to Perhaps others can fill be in on the more technical details 😅 |
Ahh. In this case we don't really re-exec the already running binary. We just create a new container with the binary mounted inside with a different name ( It should be the equivalent of doing: PS C:\> cmd.exe /c dir C:\test
Volume in drive C has no label.
Volume Serial Number is 1A83-F08F
Directory of C:\test
01/19/2023 04:03 PM <DIR> .
01/19/2023 01:24 PM 48,101,888 buildkitd.exe
01/19/2023 04:03 PM <SYMLINK> get-user-info.exe [C:\test\buildkitd.exe]
2 File(s) 48,101,888 bytes
1 Dir(s) 23,039,586,304 bytes free
PS C:\>
PS C:\>
PS C:\> docker run -it -e 'PATH=%PATH%;C:\test' --rm -v C:\test:C:\test a1cc73b9ec99 cmd
Microsoft Windows [Version 10.0.20348.1249]
(c) Microsoft Corporation. All rights reserved.
C:\>get-user-info administrator
{"UID":0,"GID":0,"SID":"S-1-5-21-2702878673-795188819-444038987-500"} |
The issue in moby with Projecting a binary into the container and exec'ing it as a container process should provide all the same isolation guarantees as any other container process...so long as there is no way to overwrite the host binary from inside the container. Since Windows has mandatory file locking for binaries of running processes I would imagine it wouldn't be an issue, though who knows; Windows is full of surprises. |
This utility allows us to mount the
buildkitd
executable inside a Windows container and fetch the SID of any existing user. This is to work around the fact that the SAM hive data structures are undocumented and there is no API to inspect an offline SAM hive to fetch the security info of an existing user.We will use this utility when determining the SID of the user when we conduct
FileOps
and is part of a replacement for: #3248Depends on: moby/moby#44847
Signed-off-by: Gabriel Adrian Samfira gsamfira@cloudbasesolutions.com