From cc01c09f8b479e1714f9e2f2cf00ecf3e65fce1b Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Fri, 28 Jun 2024 16:49:56 +0000 Subject: [PATCH] windows: remove winappcontainer and winexec helpers (#23448) This removes helper winappcontainer and winexec helper code, since it is no longer needed after #23432 --- .github/workflows/test-windows.yml | 4 +- helper/winappcontainer/winappcontainer.go | 342 --------- .../winappcontainer/winappcontainer_test.go | 78 --- helper/winexec/create.go | 151 ---- helper/winexec/winexec.go | 663 ------------------ helper/winexec/winexec_test.go | 84 --- 6 files changed, 1 insertion(+), 1321 deletions(-) delete mode 100644 helper/winappcontainer/winappcontainer.go delete mode 100644 helper/winappcontainer/winappcontainer_test.go delete mode 100644 helper/winexec/create.go delete mode 100644 helper/winexec/winexec.go delete mode 100644 helper/winexec/winexec_test.go diff --git a/.github/workflows/test-windows.yml b/.github/workflows/test-windows.yml index cc293ca03d2..4af5b4d1535 100644 --- a/.github/workflows/test-windows.yml +++ b/.github/workflows/test-windows.yml @@ -89,9 +89,7 @@ jobs: github.com/hashicorp/nomad/drivers/docker \ github.com/hashicorp/nomad/client/lib/fifo \ github.com/hashicorp/nomad/client/logmon \ - github.com/hashicorp/nomad/client/allocrunner/taskrunner/template \ - github.com/hashicorp/nomad/helper/winappcontainer \ - github.com/hashicorp/nomad/helper/winexec + github.com/hashicorp/nomad/client/allocrunner/taskrunner/template - uses: actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce # v3.1.2 with: name: results.xml diff --git a/helper/winappcontainer/winappcontainer.go b/helper/winappcontainer/winappcontainer.go deleted file mode 100644 index 1b3d687ede9..00000000000 --- a/helper/winappcontainer/winappcontainer.go +++ /dev/null @@ -1,342 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -//go:build windows - -package winappcontainer - -import ( - "errors" - "fmt" - "regexp" - "syscall" - "unsafe" - - "github.com/hashicorp/go-hclog" - "github.com/hashicorp/nomad/helper/winexec" - "golang.org/x/sys/windows" -) - -var ( - userenvDLL = windows.NewLazySystemDLL("userenv.dll") - procCreateAppContainerProfile = userenvDLL.NewProc("CreateAppContainerProfile") - procDeleteAppContainerProfile = userenvDLL.NewProc("DeleteAppContainerProfile") - procDeriveAppContainerSidFromAppContainerName = userenvDLL.NewProc("DeriveAppContainerSidFromAppContainerName") - - ErrAccessDeniedToCreateSandbox = errors.New("Nomad does not have sufficient permission to create the template rendering AppContainer") - ErrInvalidArg = errors.New("Windows returned E_INVALIDARG, this is a bug in Nomad") - - invalidContainerName = regexp.MustCompile(`[^-_. A-Za-z0-9]+`) -) - -const ( - // https://learn.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants - FILE_ALL_ACCESS uint32 = 2032127 - - // https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute - PROC_THREAD_ATTRIBUTE_SECURITY_CAPABILITIES uint32 = 0x20009 // 131081 - - // https://learn.microsoft.com/en-us/windows/win32/api/userenv/nf-userenv-createappcontainerprofile - WindowsResultOk uintptr = 0x0 // S_OK - WindowsResultErrAccessDenied uintptr = 0x80070005 // E_ACCESS_DENIED - WindowsResultErrAlreadyExists uintptr = 0x800700b7 // HRESULT_FROM_WIN32(ERROR_ALREADY_EXISTS) - WindowsResultErrInvalidArg uintptr = 0x80070057 // E_INVALIDARG - WindowsResultBadEnvironment uintptr = 0x8007000a // BAD_ENVIRONMENT - - ExitCodeFatal int = 13 // typically this is going to be a bug in Nomad - - // sidBufferSz is the size of the buffer that the PSID will be written - // to. The sys/x/windows.LookupSID method gets a INSUFFICIENT_BUFFER error - // that is uses to retry with a larger size, but the methods we're calling - // don't. Empirically, the buffer is getting populated by a *pointer* to the - // PSID, so this should only need to be a 64-bit word long, but the failure - // mode if we're wrong breaks template rendering, so give ourselves some - // room to screw it up. - sidBufferSz int = 128 -) - -func cleanupSID(sid *windows.SID) func() { - return func() { - windows.FreeSid(sid) - } -} - -func taskIDtoContainerName(id string) string { - return trimString(invalidContainerName.ReplaceAllString(id, "-"), 64) -} - -func trimString(s string, max int) string { - if s == "" { - // makes testing easier to handle this gracefully - return "appcontainer" - } - if max > len(s) { - max = len(s) - } - max = max - 1 // less a trailing NULL - return s[:max] -} - -type AppContainerConfig struct { - Name string - AllowedPaths []string -} - -func CreateAppContainer(log hclog.Logger, cfg *AppContainerConfig) error { - sid, cleanup, err := createAppContainerProfile(log, cfg.Name) - if err != nil { - return fmt.Errorf("could not create AppContainer profile: %w", err) - } - defer cleanup() - - for _, path := range cfg.AllowedPaths { - err := allowNamedObjectAccess(log, sid, path) - if err != nil { - return fmt.Errorf("could not grant object access: %w", err) - } - } - - return nil -} - -func createAppContainerProfile(log hclog.Logger, taskID string) (*windows.SID, func(), error) { - - containerName := taskIDtoContainerName(taskID) - pszAppContainerName, err := windows.UTF16PtrFromString(containerName) - if err != nil { - return nil, nil, fmt.Errorf( - "container name %q could not be encoded to utf16: %w", containerName, err) - } - - taskID = trimString(taskID, 512) - pszDisplayName, err := windows.UTF16PtrFromString(taskID) - if err != nil { - return nil, nil, fmt.Errorf( - "task ID %q could not be encoded to utf16: %w", taskID, err) - } - - pszDescription, err := windows.UTF16PtrFromString( - "template renderer AppContainer for " + taskID) - if err != nil { - return nil, nil, fmt.Errorf( - "description for task ID %q could not be encoded to utf16: %w", taskID, err) - } - - var pCapabilities uintptr // PSID_AND_ATTRIBUTES - var dwCapabilityCount uint32 // DWORD - - // note: this buffer gets populated with a pointer to a PSID, and the - // resulting handle needs to be freed here in the caller - sidBuffer := make([]byte, sidBufferSz) - - // USERENVAPI HRESULT CreateAppContainerProfile( - // [in] PCWSTR pszAppContainerName, - // [in] PCWSTR pszDisplayName, - // [in] PCWSTR pszDescription, - // [in] PSID_AND_ATTRIBUTES pCapabilities, - // [in] DWORD dwCapabilityCount, - // [out] PSID *ppSidAppContainerSid - // ); - // https://learn.microsoft.com/en-us/windows/win32/api/userenv/nf-userenv-createappcontainerprofile - result, _, err := procCreateAppContainerProfile.Call( - uintptr(unsafe.Pointer(pszAppContainerName)), - uintptr(unsafe.Pointer(pszDisplayName)), - uintptr(unsafe.Pointer(pszDescription)), - uintptr(pCapabilities), - uintptr(dwCapabilityCount), - uintptr(unsafe.Pointer(&sidBuffer)), - ) - ppSidAppContainerSid := (*windows.SID)(unsafe.Pointer(&sidBuffer[0])) - - switch result { - case WindowsResultOk: - if !ppSidAppContainerSid.IsValid() { - return nil, nil, fmt.Errorf("creating AppContainer returned invalid SID: %v", - ppSidAppContainerSid.String()) - } - - log.Debug("created new AppContainer", "sid", ppSidAppContainerSid.String()) - return ppSidAppContainerSid, cleanupSID(ppSidAppContainerSid), nil - - case WindowsResultErrAccessDenied, WindowsResultBadEnvironment: - // we cannot sandbox if Nomad is running with insufficient privs, so in - // that case we rely on the file system permissions that the user gave - // Nomad - return nil, nil, ErrAccessDeniedToCreateSandbox - - case WindowsResultErrAlreadyExists: - // WARNING: this method will return a derived SID even if the container - // doesn't already exist, so it's critical that we don't "optimize" this - // method by checking first! - return deriveAppContainerSID(taskID) - - case WindowsResultErrInvalidArg: - return nil, nil, ErrInvalidArg - - default: - // note: the error we get here is always non-nil and always reports - // sucess for known error codes - return nil, nil, fmt.Errorf("creating AppContainer failed: (%x) %v", - result, syscall.Errno(result)) - } - -} - -// deriveAppContainerSID gets the AppContainer SID that should be associated -// with the given task ID. Note that if the AppContainer exists, Windows will -// give us the SID that it should have, so we can only call this if we know that -// we've already created the AppContainer -func deriveAppContainerSID(taskID string) (*windows.SID, func(), error) { - - containerName := taskIDtoContainerName(taskID) - pszAppContainerName, err := windows.UTF16PtrFromString(containerName) - if err != nil { - return nil, nil, fmt.Errorf( - "container name %q could not be encoded to utf16: %w", containerName, err) - } - - // note: this buffer gets populated with a pointer to a PSID, and the - // resulting handle needs to be freed here in the caller - sidBuffer := make([]byte, sidBufferSz) - - // USERENVAPI HRESULT DeriveAppContainerSidFromAppContainerName( - // [in] PCWSTR pszAppContainerName, - // [out] PSID *ppsidAppContainerSid - // ); - // https://learn.microsoft.com/en-us/windows/win32/api/userenv/nf-userenv-deriveappcontainersidfromappcontainername - result, _, err := procDeriveAppContainerSidFromAppContainerName.Call( - uintptr(unsafe.Pointer(pszAppContainerName)), - uintptr(unsafe.Pointer(&sidBuffer)), - ) - switch result { - case WindowsResultOk: - ppSidAppContainerSid := (*windows.SID)(unsafe.Pointer(&sidBuffer[0])) - if !ppSidAppContainerSid.IsValid() { - return nil, nil, fmt.Errorf("looking up AppContainer SID returned invalid SID: %v", - ppSidAppContainerSid.String()) - } - - return ppSidAppContainerSid, cleanupSID(ppSidAppContainerSid), nil - default: - return nil, nil, fmt.Errorf("looking up AppContainer SID failed: errno=%v, err=%w", - syscall.Errno(result), err) - } -} - -// allowNamedObjectAccess grants inheritable R/W access to the object path for -// the AppContainer SID -func allowNamedObjectAccess(log hclog.Logger, sid *windows.SID, path string) error { - pathAccess := windows.EXPLICIT_ACCESS{ - AccessPermissions: windows.ACCESS_MASK(FILE_ALL_ACCESS), - AccessMode: windows.GRANT_ACCESS, - Inheritance: windows.OBJECT_INHERIT_ACE | windows.CONTAINER_INHERIT_ACE, - Trustee: windows.TRUSTEE{ - MultipleTrustee: nil, - MultipleTrusteeOperation: windows.NO_MULTIPLE_TRUSTEE, - TrusteeForm: windows.TRUSTEE_IS_SID, - TrusteeType: windows.TRUSTEE_IS_GROUP, - TrusteeValue: windows.TrusteeValueFromSID(sid), - }, - } - - pathSD, err := windows.GetNamedSecurityInfo( - path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) - if err != nil { - return fmt.Errorf("could not GetNamedSecurityInfo for %q: %w", path, err) - } - - acl, _, err := pathSD.DACL() - if err != nil { - return fmt.Errorf("could not get DACL for %q: %w", path, err) - } - - newACL, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{pathAccess}, acl) - if err != nil { - return fmt.Errorf("could not create new DACL for %q: %w", path, err) - } - - err = windows.SetNamedSecurityInfo( - path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, newACL, nil) - if err != nil { - return fmt.Errorf("could not SetNamedSecurityInfo for %q: %w", path, err) - } - - log.Trace("AppContainer access configured", "sid", sid, "path", path) - return nil -} - -func DeleteAppContainer(log hclog.Logger, taskID string) error { - - containerName := taskIDtoContainerName(taskID) - pszAppContainerName, err := windows.UTF16PtrFromString(containerName) - if err != nil { - return fmt.Errorf( - "container name %q could not be encoded to utf16: %w", containerName, err) - } - - // USERENVAPI HRESULT DeleteAppContainerProfile( - // [in] PCWSTR pszAppContainerName - // ); - // https://learn.microsoft.com/en-us/windows/win32/api/userenv/nf-userenv-deleteappcontainerprofile - result, _, err := procDeleteAppContainerProfile.Call( - uintptr(unsafe.Pointer(pszAppContainerName)), - ) - - switch result { - case WindowsResultOk: // we get this if AppContainer doesn't exist - log.Debug("deleted AppContainer") - return nil - - case WindowsResultErrInvalidArg: - return ErrInvalidArg - - default: - // note: the error we get here is always non-nil and always reports - // sucess for known error codes - return fmt.Errorf("deleting AppContainer failed: errno=%v, err=%w", - syscall.Errno(result), err) - } - -} - -func CreateProcThreadAttributes(taskID string) ([]winexec.ProcThreadAttribute, func(), error) { - - sid, cleanup, err := deriveAppContainerSID(taskID) - if err != nil { - return nil, cleanup, fmt.Errorf("could not get SID for app container: %w", err) - } - - procThreadAttrs, err := createProcThreadAttributes(sid) - if err != nil { - return nil, cleanup, fmt.Errorf("could not create proc attributes: %w", err) - } - - return procThreadAttrs, cleanup, nil -} - -type SecurityCapabilities struct { - AppContainerSid uintptr // PSID *windows.SID - Capabilities uintptr // SID_AND_ATTRIBUTES *windows.SIDAndAttributes - CapabilityCount uint32 - Reserved uint32 -} - -// createProcThreadAttributes returns ProcThreadAttributes so that winexec.Cmd -// can set the SecurityCapabilities on the process -func createProcThreadAttributes(containerSID *windows.SID) ([]winexec.ProcThreadAttribute, error) { - - sd, err := windows.NewSecurityDescriptor() - if err != nil { - return nil, fmt.Errorf("could not create new security descriptor: %w", err) - } - sd.SetOwner(containerSID, true) - - sc := &SecurityCapabilities{AppContainerSid: uintptr(unsafe.Pointer(containerSID))} - - return []winexec.ProcThreadAttribute{ - { - Attribute: uintptr(PROC_THREAD_ATTRIBUTE_SECURITY_CAPABILITIES), - Value: unsafe.Pointer(sc), - Size: uintptr(unsafe.Sizeof(*sc)), - }}, nil -} diff --git a/helper/winappcontainer/winappcontainer_test.go b/helper/winappcontainer/winappcontainer_test.go deleted file mode 100644 index fb6632b9beb..00000000000 --- a/helper/winappcontainer/winappcontainer_test.go +++ /dev/null @@ -1,78 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -//go:build windows - -package winappcontainer - -import ( - "context" - "io" - "os" - "testing" - "time" - - "github.com/hashicorp/nomad/ci" - "github.com/hashicorp/nomad/helper/testlog" - "github.com/hashicorp/nomad/helper/winexec" - "github.com/shoenig/test/must" -) - -// TestAppContainer_CatStdin runs a "cat"-like command in an AppContainer and -// pipes data into stdin. We use TestCatHelper to do this so that we don't need -// to rely on external programs -func TestAppContainer_CatStdin(t *testing.T) { - ci.Parallel(t) - t.Helper() - - path, _ := os.Executable() - - containerCfg := &AppContainerConfig{ - Name: t.Name(), - AllowedPaths: []string{path}, - } - logger := testlog.HCLogger(t) - err := CreateAppContainer(logger, containerCfg) - if err != nil { - // if the tests are running as an unprivileged user, we might not be - // able to create the sandbox, but in that case we're not vulnerable to - // the attacks this is intended to prevent anyways - must.EqError(t, err, ErrAccessDeniedToCreateSandbox.Error()) - } - - t.Cleanup(func() { - must.NoError(t, DeleteAppContainer(logger, t.Name())) - }) - - procThreadAttrs, cleanup, err := CreateProcThreadAttributes(t.Name()) - must.NoError(t, err) - t.Cleanup(cleanup) - - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) - defer cancel() - - args := []string{"-test.run=TestCatHelper", "--"} - cmd := winexec.CommandContext(ctx, path, args...) - cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1") - cmd.ProcThreadAttributes = procThreadAttrs - - input := "Input string\nLine 2" - stdin, _ := cmd.StdinPipe() - go func() { - defer stdin.Close() - io.WriteString(stdin, input) - }() - - bs, err := cmd.CombinedOutput() - must.EqError(t, err, "exit status 7") - must.Eq(t, input, string(bs)) -} - -func TestCatHelper(t *testing.T) { - if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" { - t.Skip("this should only be run as part of the tests above") - return - } - io.Copy(os.Stdout, os.Stdin) - os.Exit(7) -} diff --git a/helper/winexec/create.go b/helper/winexec/create.go deleted file mode 100644 index bd6da3ef36a..00000000000 --- a/helper/winexec/create.go +++ /dev/null @@ -1,151 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -//go:build windows - -package winexec - -import ( - "errors" - "fmt" - "os" - "runtime" - "syscall" - "unsafe" - - "golang.org/x/sys/windows" -) - -var EINVAL = errors.New("EINVAL") - -func (c *Cmd) createProcess( - path string, commandLine []string, - userProcThreadAttrs []ProcThreadAttribute, - attr *syscall.ProcAttr, -) (*os.Process, error) { - - // Much like in os/exec Command, we're creating the process directly without - // creating a shell. Unlike what we're doing for Linux/Unix, we're creating - // this process directly into the AppContainer rather than starting the - // process and dropping privs, and we control all the initial arguments that - // enforce we're calling the particular binary we want. - cli := windows.ComposeCommandLine(commandLine) - wCommandLine, err := windows.UTF16PtrFromString(cli) - if err != nil { - return nil, fmt.Errorf("could not create UTF16 pointer from cli: %w", err) - } - - var wCurrentDir *uint16 - if c.Dir != "" { - wCurrentDir, err = windows.UTF16PtrFromString(c.Dir) - if err != nil { - return nil, fmt.Errorf("could not create UTF16 pointer from currentDir: %w", err) - } - } - - parentProcess, _ := windows.GetCurrentProcess() - p := parentProcess - fd := make([]windows.Handle, len(attr.Files)) - for i := range attr.Files { - if attr.Files[i] > 0 { - destinationProcessHandle := parentProcess - err := windows.DuplicateHandle( - p, windows.Handle(attr.Files[i]), - destinationProcessHandle, &fd[i], 0, true, windows.DUPLICATE_SAME_ACCESS) - if err != nil { - return nil, err - } - defer windows.DuplicateHandle( - parentProcess, fd[i], 0, nil, 0, false, windows.DUPLICATE_CLOSE_SOURCE) - } - } - - procThreadAttrs, err := mergeProcThreadAttrs(fd, userProcThreadAttrs) - if err != nil { - return nil, err - } - - startupInfo := new(windows.StartupInfoEx) - startupInfo.Cb = uint32(unsafe.Sizeof(*startupInfo)) // Cb: size of struct in bytes - startupInfo.ProcThreadAttributeList = procThreadAttrs.List() - startupInfo.StdInput = fd[0] - startupInfo.StdOutput = fd[1] - startupInfo.StdErr = fd[2] - startupInfo.Flags = syscall.STARTF_USESTDHANDLES - - flags := uint32(windows.CREATE_UNICODE_ENVIRONMENT | - windows.EXTENDED_STARTUPINFO_PRESENT) - - envBlock, err := createEnvBlock(attr.Env) - if err != nil { - return nil, err - } - - outProcInfo := new(windows.ProcessInformation) - err = windows.CreateProcess( - nil, //appName - wCommandLine, - nil, // procSecurity - nil, // threadSecurity - true, // inheritHandles, - flags, - envBlock, - wCurrentDir, - &startupInfo.StartupInfo, - outProcInfo) - if err != nil { - return nil, fmt.Errorf("could not CreateProcess: %w", err) - } - - defer windows.CloseHandle(windows.Handle(outProcInfo.Thread)) - - // this ensures we don't call the finalizers on the attr.Files before we - // make the syscall. See stdlib's os/exec_posix.go for another example. - runtime.KeepAlive(fd) - runtime.KeepAlive(attr) - - return os.FindProcess(int(outProcInfo.ProcessId)) -} - -// ref https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute -// actual value from https://docs.rs/windows-sys/latest/windows_sys/Win32/System/Threading/constant.PROC_THREAD_ATTRIBUTE_HANDLE_LIST.html and empirically tested -const PROC_THREAD_ATTRIBUTE_HANDLE_LIST = 0x20002 // 131074 - -type ProcThreadAttribute struct { - Attribute uintptr - Value unsafe.Pointer - Size uintptr -} - -func mergeProcThreadAttrs( - fd []windows.Handle, - userAttrs []ProcThreadAttribute, -) (*windows.ProcThreadAttributeListContainer, error) { - - newLen := len(userAttrs) + 1 - - procThreadAttrs, err := windows.NewProcThreadAttributeList(uint32(newLen)) - if err != nil { - return nil, fmt.Errorf("could not create NewProcThreadAttributeList: %v", err) - } - - err = procThreadAttrs.Update( - uintptr(PROC_THREAD_ATTRIBUTE_HANDLE_LIST), - unsafe.Pointer(&fd[0]), - uintptr(len(fd))*unsafe.Sizeof(fd[0])) - if err != nil { - return nil, fmt.Errorf("could not update procthread attrs: %v", err) - } - - for _, userAttr := range userAttrs { - err = procThreadAttrs.Update( - userAttr.Attribute, - userAttr.Value, - userAttr.Size) - if err != nil { - return nil, fmt.Errorf("could not update procthread attrs: %v", err) - } - } - - return procThreadAttrs, nil -} diff --git a/helper/winexec/winexec.go b/helper/winexec/winexec.go deleted file mode 100644 index e6e503a59af..00000000000 --- a/helper/winexec/winexec.go +++ /dev/null @@ -1,663 +0,0 @@ -// Copyright 2009 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// TODO(tgross): almost everything in this file is lifted directly from the -// stdlib's os/exec/exec.go and syscall/exec_windows.go, stripped down to remove -// non-Windows bits, some legacy cruft from upstream, and methods we don't care -// about here. This gives us the StdinPipe and CombinedOutput methods we want, -// but adds the ProcThreadAttributeList which we need for running Windows -// applications in AppContainers. Ideally we'd get this feature upstreamed and -// then we could remove this package entirely. A similar proposal was rejected -// in https://github.com/golang/go/issues/44005 but hopefully using this package -// as example of the lift involved we can advocate for getting the issue -// reconsidered. - -//go:build windows - -package winexec - -import ( - "bytes" - "context" - "errors" - "io" - "io/fs" - "os" - "os/exec" - "path/filepath" - "runtime" - "strings" - "syscall" - "time" - "unicode/utf16" -) - -type Cmd struct { - *exec.Cmd - - // these are all private fields of exec.Cmd that we're hoisting into this - // struct so that we can access them in the methods we're implementing - ctx context.Context - childIOFiles []io.Closer - parentIOPipes []io.Closer - goroutine []func() error - goroutineErr <-chan error - ctxResult <-chan ctxResult - - // ProcThreadAttributes will get merged with the one that gets created - // automatically for StartupInfoEx - ProcThreadAttributes []ProcThreadAttribute -} - -// A ctxResult reports the result of watching the Context associated with a -// running command (and sending corresponding signals if needed). -// This is lifted from os/exec/exec.go -type ctxResult struct { - err error - timer *time.Timer -} - -// CommandContext returns a new Cmd with a given context. Note we return the -// concrete struct and not an interface here because callers need to update -// fields on the inner exec.Cmd directly -func CommandContext(ctx context.Context, name string, arg ...string) *Cmd { - if ctx == nil { - panic("nil Context") - } - innerCmd := exec.Command(name, arg...) - - cmd := &Cmd{} - cmd.Cmd = innerCmd - cmd.ctx = ctx - cmd.Cancel = func() error { - return cmd.Process.Kill() - } - - if filepath.Base(name) == name { - lp, err := exec.LookPath(name) - if lp != "" { - cmd.Path = lp - } - if err != nil { - cmd.Err = err - } - } - - return cmd -} - -func (c *Cmd) StdinPipe() (io.WriteCloser, error) { - if c.Stdin != nil { - return nil, errors.New("exec: Stdin already set") - } - if c.Process != nil { - return nil, errors.New("exec: StdinPipe after process started") - } - pr, pw, err := os.Pipe() - if err != nil { - return nil, err - } - c.Stdin = pr - c.childIOFiles = append(c.childIOFiles, pr) - c.parentIOPipes = append(c.parentIOPipes, pw) - return pw, nil -} - -func (c *Cmd) CombinedOutput() ([]byte, error) { - if c.Stdout != nil { - return nil, errors.New("exec: Stdout already set") - } - if c.Stderr != nil { - return nil, errors.New("exec: Stderr already set") - } - var b bytes.Buffer - c.Stdout = &b - c.Stderr = &b - err := c.Run() - return b.Bytes(), err -} - -func (c *Cmd) Run() error { - err := c.Start() - if err != nil { - return err - } - return c.Wait() -} - -func (c *Cmd) Start() error { - - if c.Process != nil { - return errors.New("exec: already started") - } - - started := false - defer func() { - closeDescriptors(c.childIOFiles) - c.childIOFiles = nil - - if !started { - closeDescriptors(c.parentIOPipes) - c.parentIOPipes = nil - } - }() - - if c.Path == "" && c.Err == nil { - c.Err = errors.New("exec: no command") - } - if c.Err != nil { - return c.Err - } - - if c.Cancel != nil && c.ctx == nil { - return errors.New("exec: command with a non-nil Cancel was not created with CommandContext") - } - if c.ctx != nil { - select { - case <-c.ctx.Done(): - return c.ctx.Err() - default: - } - } - - childFiles := make([]*os.File, 0, 3) - - stdin, err := c.childStdin() - if err != nil { - return err - } - childFiles = append(childFiles, stdin) - - stdout, err := c.childStdout() - if err != nil { - return err - } - childFiles = append(childFiles, stdout) - - stderr, err := c.childStderr(stdout) - if err != nil { - return err - } - childFiles = append(childFiles, stderr) - - env, err := c.environ() - if err != nil { - return err - } - - attr := &syscall.ProcAttr{ - Dir: c.Dir, - Files: []uintptr{ - childFiles[0].Fd(), - childFiles[1].Fd(), - childFiles[2].Fd(), - }, - Env: env, - Sys: c.SysProcAttr, - } - - c.Process, err = c.createProcess(c.Path, c.Args, c.ProcThreadAttributes, attr) - if err != nil { - return err - } - started = true - - if len(c.goroutine) > 0 { - goroutineErr := make(chan error, 1) - c.goroutineErr = goroutineErr - - type goroutineStatus struct { - running int - firstErr error - } - statusc := make(chan goroutineStatus, 1) - statusc <- goroutineStatus{running: len(c.goroutine)} - for _, fn := range c.goroutine { - go func(fn func() error) { - err := fn() - status := <-statusc - if status.firstErr == nil { - status.firstErr = err - } - status.running-- - if status.running == 0 { - goroutineErr <- status.firstErr - } else { - statusc <- status - } - }(fn) - } - c.goroutine = nil - } - - if (c.Cancel != nil || c.WaitDelay != 0) && c.ctx != nil && c.ctx.Done() != nil { - resultc := make(chan ctxResult) - c.ctxResult = resultc - go c.watchCtx(resultc) - } - - return nil -} - -func (c *Cmd) environ() ([]string, error) { - var err error - env := c.Env - if env == nil { - return os.Environ(), nil - } - env, dedupErr := dedupEnv(env) - if err == nil { - err = dedupErr - } - return addCriticalEnv(env), nil -} - -// dedupEnv returns a copy of env with any duplicates removed, in favor of -// later values. -// Items not of the normal environment "key=value" form are preserved unchanged. -// Except on Plan 9, items containing NUL characters are removed, and -// an error is returned along with the remaining values. -func dedupEnv(env []string) ([]string, error) { - return dedupEnvCase(true, false, env) -} - -// dedupEnvCase is dedupEnv with a case option for testing. -// If caseInsensitive is true, the case of keys is ignored. -// If nulOK is false, items containing NUL characters are allowed. -func dedupEnvCase(caseInsensitive, nulOK bool, env []string) ([]string, error) { - // Construct the output in reverse order, to preserve the - // last occurrence of each key. - var err error - out := make([]string, 0, len(env)) - saw := make(map[string]bool, len(env)) - for n := len(env); n > 0; n-- { - kv := env[n-1] - - // Reject NUL in environment variables to prevent security issues (#56284); - // except on Plan 9, which uses NUL as os.PathListSeparator (#56544). - if !nulOK && strings.IndexByte(kv, 0) != -1 { - err = errors.New("exec: environment variable contains NUL") - continue - } - - i := strings.Index(kv, "=") - if i == 0 { - // We observe in practice keys with a single leading "=" on Windows. - // TODO(#49886): Should we consume only the first leading "=" as part - // of the key, or parse through arbitrarily many of them until a non-"="? - i = strings.Index(kv[1:], "=") + 1 - } - if i < 0 { - if kv != "" { - // The entry is not of the form "key=value" (as it is required to be). - // Leave it as-is for now. - // TODO(#52436): should we strip or reject these bogus entries? - out = append(out, kv) - } - continue - } - k := kv[:i] - if caseInsensitive { - k = strings.ToLower(k) - } - if saw[k] { - continue - } - - saw[k] = true - out = append(out, kv) - } - - // Now reverse the slice to restore the original order. - for i := 0; i < len(out)/2; i++ { - j := len(out) - i - 1 - out[i], out[j] = out[j], out[i] - } - - return out, err -} - -func addCriticalEnv(env []string) []string { - if runtime.GOOS != "windows" { - return env - } - for _, kv := range env { - k, _, ok := strings.Cut(kv, "=") - if !ok { - continue - } - if strings.EqualFold(k, "SYSTEMROOT") { - // We already have it. - return env - } - } - return append(env, "SYSTEMROOT="+os.Getenv("SYSTEMROOT")) -} - -func (c *Cmd) watchCtx(resultc chan<- ctxResult) { - select { - case resultc <- ctxResult{}: - return - case <-c.ctx.Done(): - } - - var err error - if c.Cancel != nil { - if interruptErr := c.Cancel(); interruptErr == nil { - // We appear to have successfully interrupted the command, so any - // program behavior from this point may be due to ctx even if the - // command exits with code 0. - err = c.ctx.Err() - } else if errors.Is(interruptErr, os.ErrProcessDone) { - // The process already finished: we just didn't notice it yet. - // (Perhaps c.Wait hadn't been called, or perhaps it happened to race with - // c.ctx being cancelled.) Don't inject a needless error. - } else { - err = wrappedError{ - prefix: "exec: canceling Cmd", - err: interruptErr, - } - } - } - if c.WaitDelay == 0 { - resultc <- ctxResult{err: err} - return - } - - timer := time.NewTimer(c.WaitDelay) - select { - case resultc <- ctxResult{err: err, timer: timer}: - // c.Process.Wait returned and we've handed the timer off to c.Wait. - // It will take care of goroutine shutdown from here. - return - case <-timer.C: - } - - killed := false - if killErr := c.Process.Kill(); killErr == nil { - // We appear to have killed the process. c.Process.Wait should return a - // non-nil error to c.Wait unless the Kill signal races with a successful - // exit, and if that does happen we shouldn't report a spurious error, - // so don't set err to anything here. - killed = true - } else if !errors.Is(killErr, os.ErrProcessDone) { - err = wrappedError{ - prefix: "exec: killing Cmd", - err: killErr, - } - } - - if c.goroutineErr != nil { - select { - case goroutineErr := <-c.goroutineErr: - // Forward goroutineErr only if we don't have reason to believe it was - // caused by a call to Cancel or Kill above. - if err == nil && !killed { - err = goroutineErr - } - default: - // Close the child process's I/O pipes, in case it abandoned some - // subprocess that inherited them and is still holding them open - // (see https://go.dev/issue/23019). - // - // We close the goroutine pipes only after we have sent any signals we're - // going to send to the process (via Signal or Kill above): if we send - // SIGKILL to the process, we would prefer for it to die of SIGKILL, not - // SIGPIPE. (However, this may still cause any orphaned subprocesses to - // terminate with SIGPIPE.) - closeDescriptors(c.parentIOPipes) - // Wait for the copying goroutines to finish, but report ErrWaitDelay for - // the error: any other error here could result from closing the pipes. - _ = <-c.goroutineErr - if err == nil { - err = ErrWaitDelay - } - } - - // Since we have already received the only result from c.goroutineErr, - // set it to nil to prevent awaitGoroutines from blocking on it. - c.goroutineErr = nil - } - - resultc <- ctxResult{err: err} -} - -// ErrWaitDelay is returned by (*Cmd).Wait if the process exits with a -// successful status code but its output pipes are not closed before the -// command's WaitDelay expires. -var ErrWaitDelay = errors.New("exec: WaitDelay expired before I/O complete") - -// wrappedError wraps an error without relying on fmt.Errorf. -type wrappedError struct { - prefix string - err error -} - -func (w wrappedError) Error() string { - return w.prefix + ": " + w.err.Error() -} - -func (w wrappedError) Unwrap() error { - return w.err -} - -func (c *Cmd) Wait() error { - if c.Process == nil { - return errors.New("exec: not started") - } - if c.ProcessState != nil { - return errors.New("exec: Wait was already called") - } - - state, err := c.Process.Wait() - if err == nil && !state.Success() { - err = &exec.ExitError{ProcessState: state} - } - c.ProcessState = state - - var timer *time.Timer - if c.ctxResult != nil { - watch := <-c.ctxResult - timer = watch.timer - // If c.Process.Wait returned an error, prefer that. - // Otherwise, report any error from the watchCtx goroutine, - // such as a Context cancellation or a WaitDelay overrun. - if err == nil && watch.err != nil { - err = watch.err - } - } - - if goroutineErr := c.awaitGoroutines(timer); err == nil { - // Report an error from the copying goroutines only if the program - // otherwise exited normally on its own. Otherwise, the copying error - // may be due to the abnormal termination. - err = goroutineErr - } - closeDescriptors(c.parentIOPipes) - c.parentIOPipes = nil - - return err -} - -func (c *Cmd) awaitGoroutines(timer *time.Timer) error { - defer func() { - if timer != nil { - timer.Stop() - } - c.goroutineErr = nil - }() - - if c.goroutineErr == nil { - return nil // No running goroutines to await. - } - - if timer == nil { - if c.WaitDelay == 0 { - return <-c.goroutineErr - } - - select { - case err := <-c.goroutineErr: - // Avoid the overhead of starting a timer. - return err - default: - } - - // No existing timer was started: either there is no Context associated with - // the command, or c.Process.Wait completed before the Context was done. - timer = time.NewTimer(c.WaitDelay) - } - - select { - case <-timer.C: - closeDescriptors(c.parentIOPipes) - // Wait for the copying goroutines to finish, but ignore any error - // (since it was probably caused by closing the pipes). - _ = <-c.goroutineErr - return ErrWaitDelay - - case err := <-c.goroutineErr: - return err - } -} - -func closeDescriptors(closers []io.Closer) { - for _, fd := range closers { - fd.Close() - } -} - -func (c *Cmd) childStdin() (*os.File, error) { - if c.Stdin == nil { - f, err := os.Open(os.DevNull) - if err != nil { - return nil, err - } - c.childIOFiles = append(c.childIOFiles, f) - return f, nil - } - - if f, ok := c.Stdin.(*os.File); ok { - return f, nil - } - - pr, pw, err := os.Pipe() - if err != nil { - return nil, err - } - - c.childIOFiles = append(c.childIOFiles, pr) - c.parentIOPipes = append(c.parentIOPipes, pw) - c.goroutine = append(c.goroutine, func() error { - _, err := io.Copy(pw, c.Stdin) - if skipStdinCopyError(err) { - err = nil - } - if err1 := pw.Close(); err == nil { - err = err1 - } - return err - }) - return pr, nil -} - -func (c *Cmd) childStdout() (*os.File, error) { - return c.writerDescriptor(c.Stdout) -} - -func (c *Cmd) childStderr(childStdout *os.File) (*os.File, error) { - if c.Stderr != nil && interfaceEqual(c.Stderr, c.Stdout) { - return childStdout, nil - } - return c.writerDescriptor(c.Stderr) -} - -func (c *Cmd) writerDescriptor(w io.Writer) (*os.File, error) { - if w == nil { - f, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0) - if err != nil { - return nil, err - } - c.childIOFiles = append(c.childIOFiles, f) - return f, nil - } - - if f, ok := w.(*os.File); ok { - return f, nil - } - - pr, pw, err := os.Pipe() - if err != nil { - return nil, err - } - - c.childIOFiles = append(c.childIOFiles, pw) - c.parentIOPipes = append(c.parentIOPipes, pr) - c.goroutine = append(c.goroutine, func() error { - _, err := io.Copy(w, pr) - pr.Close() // in case io.Copy stopped due to write error - return err - }) - return pw, nil -} - -// interfaceEqual protects against panics from doing equality tests on -// two interfaces with non-comparable underlying types. -func interfaceEqual(a, b any) bool { - defer func() { - recover() - }() - return a == b -} - -func skipStdinCopyError(err error) bool { - // Ignore ERROR_BROKEN_PIPE and ERROR_NO_DATA errors copying - // to stdin if the program completed successfully otherwise. - // See Issue 20445. - const _ERROR_NO_DATA = syscall.Errno(0xe8) - pe, ok := err.(*fs.PathError) - return ok && - pe.Op == "write" && pe.Path == "|1" && - (pe.Err == syscall.ERROR_BROKEN_PIPE || pe.Err == _ERROR_NO_DATA) -} - -// createEnvBlock converts an array of environment strings into -// the representation required by CreateProcess: a sequence of NUL -// terminated strings followed by a nil. -// Last bytes are two UCS-2 NULs, or four NUL bytes. -// If any string contains a NUL, it returns (nil, EINVAL). -func createEnvBlock(envv []string) (*uint16, error) { - if len(envv) == 0 { - return &utf16.Encode([]rune("\x00\x00"))[0], nil - } - length := 0 - for _, s := range envv { - if IndexByteString(s, 0) != -1 { - return nil, EINVAL - } - length += len(s) + 1 - } - length += 1 - - b := make([]byte, length) - i := 0 - for _, s := range envv { - l := len(s) - copy(b[i:i+l], []byte(s)) - copy(b[i+l:i+l+1], []byte{0}) - i = i + l + 1 - } - copy(b[i:i+1], []byte{0}) - - return &utf16.Encode([]rune(string(b)))[0], nil -} - -func IndexByteString(s string, c byte) int { - for i := 0; i < len(s); i++ { - if s[i] == c { - return i - } - } - return -1 -} diff --git a/helper/winexec/winexec_test.go b/helper/winexec/winexec_test.go deleted file mode 100644 index 0fa2ca61d07..00000000000 --- a/helper/winexec/winexec_test.go +++ /dev/null @@ -1,84 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -//go:build windows - -package winexec - -import ( - "context" - "io" - "os" - "os/exec" - "testing" - "time" - - "github.com/hashicorp/nomad/ci" - "github.com/shoenig/test/must" -) - -type execCmd interface { - StdinPipe() (io.WriteCloser, error) - CombinedOutput() ([]byte, error) -} - -// TestWinExec_CatStdin runs a "cat"-like command and pipes data into stdin. We -// use TestCatHelper to do this so that we don't need to rely on external -// programs -func TestWinExec_CatStdin(t *testing.T) { - ci.Parallel(t) - - testCases := []struct { - name string - factory func(context.Context, string, ...string) execCmd - }{ - { - name: "winexec.CommandContext", - factory: func(ctx context.Context, name string, args ...string) execCmd { - cmd := CommandContext(ctx, name, args...) - cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1") - return cmd - }, - }, - { - // run the exact same test as above, using os/exec's version, so - // that we can verify we have the exact same behavior - name: "os/exec.CommandContext", - factory: func(ctx context.Context, name string, args ...string) execCmd { - cmd := exec.CommandContext(ctx, name, args...) - cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1") - return cmd - }, - }, - } - - for _, tc := range testCases { - path, _ := os.Executable() - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) - defer cancel() - - args := []string{"-test.run=TestCatHelper", "--"} - cmd := tc.factory(ctx, path, args...) - - input := "Input string\nLine 2" - stdin, _ := cmd.StdinPipe() - go func() { - defer stdin.Close() - io.WriteString(stdin, input) - }() - - bs, err := cmd.CombinedOutput() - must.EqError(t, err, "exit status 7") - must.Eq(t, input, string(bs)) - } -} - -func TestCatHelper(t *testing.T) { - t.Helper() - if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" { - t.Skip("this should only be run as part of the tests above") - return - } - io.Copy(os.Stdout, os.Stdin) - os.Exit(7) -}