Skip to content
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

Pass file handle to execd on windows #164

Closed
ekcasey opened this issue Nov 17, 2020 · 6 comments · Fixed by #203
Closed

Pass file handle to execd on windows #164

ekcasey opened this issue Nov 17, 2020 · 6 comments · Fixed by #203
Assignees
Labels
api/buildpack bug Something isn't working
Milestone

Comments

@ekcasey
Copy link
Member

ekcasey commented Nov 17, 2020

In our current exec.d specification we expect the lifecycle to provide a file descriptor at index 3 in the child's file descriptor table, to which the child is expected to write it's output.

On windows a child may inherit an additional file handle (if configured properly by the parent), however that handle will not be associated with file descriptor 3 unless we use undocumented features of the windows CreateProcess syscall that are not exposed by the go stdlib.

Background from the python docs

Using CreateProcess(), handles are only inherited if their inheritable flag (HANDLE_FLAG_INHERIT) is set and the bInheritHandles parameter of CreateProcess() is TRUE; all file descriptors except standard streams (0, 1, 2) are closed in the child process, even if bInheritHandles is TRUE.

While we could potentially work around this. It seems like the more natural pattern on windows is to pass the file handle (a uint32) to the exec.d process either as a positional argument or an environment variables. I propose we pass the hex representation of the file handle to the exec.d process in an environment variable named CNB_EXEC_D_HANDLE.

More background:

@ekcasey ekcasey added bug Something isn't working api/buildpack labels Nov 17, 2020
@micahyoung
Copy link
Member

micahyoung commented Nov 18, 2020

I like the env var handle approach, though I wonder if something like a named pipe - which are supported in Golang in Microsoft's winio package or otherwise through standard Windows APIs - could be closer to the intuitiveness of /dev/fd/3 (maybe \\.\pipe\cnb_exec_d). I haven't tried working with the Windows syscalls for parsing handles yet, but I assume it would be simpler for exec.d executables to setup and write to named-pipes than parsing and validating handles.

basic parent/child process demo: https://github.com/micahyoung/winio-named-pipes-explore

container.d winio usage: https://github.com/containerd/containerd/blob/master/cio/io_windows.go#L52

It does appear possible to set permissions and write-only functionality on the name pipe as well. Also, my demo suggests that ContainerUser can create it's own named pipes and read them without any additional privileges.

Update:

After playing around with both approaches a bit, I'm more in favor of the original handle-as-env-var approach. The go code to parse and process the env var isn't too onerous and the security model is much better - the named pipes would be global to the OS and the SDDL format for securing them isn't amendable to a single process group. There's also concurrency questions about allowing multiple processes to write to that pipe.

To mitigate some of the UX issues, perhaps optional helper executables/libraries can be created to be used by the exec.d executables to simplify setting env vars, which could hide away both the toml format and the handle parsing at the same time.

@micahyoung
Copy link
Member

micahyoung commented Nov 18, 2020

FWIW, as I bet you did, I searched around to see if there's any more idiomatic-equivalents of FD 3 and there doesn't appear to be any. Powershell has a conflicting use of it's Handle 3 where it's used for Warning output, which is perhaps more reason to not consider "3" as special for Windows exec.d executables. However, cmd's usage of Handle 3 does appear similar to unix though, Handles 3 through 9 are user-defined.

I also checked to see what was done with Cloud Foundry launcher which does inherit the parents handles, but given how different the use-cases are here, it didn't feel relevant.

@ekcasey
Copy link
Member Author

ekcasey commented Nov 20, 2020

@micahyoung thanks for looking into this!

@ekcasey
Copy link
Member Author

ekcasey commented Mar 18, 2021

Resolved by #203

@ekcasey ekcasey closed this as completed Mar 18, 2021
@ekcasey
Copy link
Member Author

ekcasey commented Mar 18, 2021

Oops, got ahead of myself there. #203 hasn't merged yet

@ekcasey ekcasey reopened this Mar 18, 2021
@ekcasey
Copy link
Member Author

ekcasey commented Mar 26, 2021

Resolved by #203 (for realz this time)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/buildpack bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants