-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix: create windows npipe with the right security descriptor #4872
Conversation
There was already code that was creating the npipe with the right descriptors, as it has been for other like docker[1]. This fix uses the main.getLocalListener instead of the generic one from `containerd/sys`. fixes moby#4864 Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
@@ -639,6 +639,9 @@ func getListener(addr string, uid, gid int, tlsConfig *tls.Config) (net.Listener | |||
if tlsConfig != nil { | |||
bklog.L.Warnf("TLS is disabled for %s", addr) | |||
} | |||
if proto == "npipe" { | |||
return getLocalListener(listenAddr) | |||
} | |||
return sys.GetLocalListener(listenAddr, uid, gid) |
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.
@gabriel-samfira had also created getLocalListener
for UNIX https://github.com/moby/buildkit/blob/master/cmd/buildkitd/main_unix.go#L51C1-L62 , which isn't being called...
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.
Sadly, the current getLocalListener()
function is way to lax in terms of DACLs.
We need to parse the --group
flag, resolve that to a SID, and use that SID to create a proper SDDL. We need to resolve the SID every time, because on Windows, the SID will always be different for each machine that was sysprepped. Also, if the machine is joined to an active directory, the group may be one belonging to AD, which will have a different SID compared to local groups. ie: you can have localhost\Docker
and also mydomain\Docker
.
The good news is that it should be easy to split the handling of the --group
flag. If we omit it, we can default to none on Windows, and only allow the builtin Administrators
group.
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.
Sadly, the current getLocalListener() function is way to lax in terms of DACLs.
Just double checking; that's the implementation in BuildKit only, correct? Because that one's missing this part of the code (to apply the group); https://github.com/moby/moby/blob/faf84d7f0a1f2e6badff6f720a3e1e559c356fff/daemon/listeners/listeners_windows.go#L29-L35
Or is the code in Moby also incorrect, by applying the fixed SID as starting point?;
https://github.com/moby/moby/blob/faf84d7f0a1f2e6badff6f720a3e1e559c356fff/daemon/listeners/listeners_windows.go#L26-L27
// allow Administrators and SYSTEM, plus whatever additional users or groups were specified
sddl := "D:P(A;;GA;;;BA)(A;;GA;;;SY)"
@profnandaa I can test, but wait output do you get from running |
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 need to check if this isn't relaxing security too much for default value and implement --group
per #4864 (comment) , but I guess for testing experimental feature this is fine for now.
|
@tonistiigi -- ok sure, created a tracking issue for that -- #4873 ; since Gabriel also had a TODO with the doubts. |
Yes, this now allows RW for any authenticated user, but it was previously allowing RO access for Everyone and ANONYMOUS LOGON. |
I still have doubts about those permissions. We should probably remove access for If we're in a rush now, allowing only Long term, we should handle the buildkit/cmd/buildkitd/main_windows.go Line 29 in 5fce077
but with the resolved SID instead of I suggest we allow:
|
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 safety, we should either limit the named pipe to Administrators
or properly handle the --group
flag.
@gabriel-samfira -- thanks for weighing in! Let me work on resolving the SID from the buildkit/cmd/buildkitd/main_unix.go Lines 51 to 62 in 5fce077
Do we fix that to get the @tonistiigi -- I'll have to look at this (my) tomorrow, you may want to remove this from the Thanks! |
@profnandaa PR in #4875 |
closing in favor of #4875, thanks Tonis! |
There was already code that was creating the npipe with the right descriptors, as it has been for other like docker[1].
This fix uses the main.getLocalListener instead of the generic one from
containerd/sys
.__
[1] https://github.com/moby/moby/blob/master/daemon/listeners/listeners_windows.go#L25
fixes #4864
Before and After (running from non-Admin terminal):