Skip to content

fix: create windows npipe with the right security descriptor #4872

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.

Sign up for GitHub

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

Closed
profnandaa wants to merge 1 commit into from
Closed

fix: create windows npipe with the right security descriptor #4872

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/buildkitd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Copy Markdown
Collaborator Author

@profnandaa profnandaa Apr 24, 2024

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

Copy link
Copy Markdown
Collaborator

@gabriel-samfira gabriel-samfira Apr 24, 2024

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.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Apr 24, 2024

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

case "fd":
return listenFD(listenAddr, tlsConfig)
Expand Down