Proposal Details
Allow adding Security Capabilities to SysProcAttr
on Windows. Note this is separate from the existing SecurityAttributes
struct which can be set as the ProcessAttributes
or ThreadAttributes
field.
Motivation
Recently as part of work to sandbox a subprocess, the Nomad team at HashiCorp needed to add a SECURITY_CAPABILITIES
struct to the StartupInfoEx
for a process. Because this is not exposed in SysProcAttr
this involved writing an unfortunate amount of code, much of which had to be simply lifted from the os/exec
stdlib. See helper/winexec/create.go
Implementation Notes
Previously a proposal was implemented to add a ParentProcess
field to SysProcAttr
for Windows https://github.com/golang/go/issues/44011. This was discussed around the same time as a rejected proposal to add the full StartupInfoEx
struct https://github.com/golang/go/issues/44005.
One of the reasons why the StartupInfoEx
proposal was rejected was because it resulted in ambiguity around how one would merge any default attributes with ones provided by the user. There are two options to work around this:
Option 1: Extensible
Our implementation referenced above adds a ProcThreadAttributes
field to the forked os/exec.Cmd
which is a slice of ProcThreadAttribute
. This instead could be added to SysProcAttr
as an extensible way of adding more attributes:
type SysProcAttr struct {
// ...
ProcThreadAttributes []ProcThreadAttribute
}
type ProcThreadAttribute struct {
Attribute uintptr
Value unsafe.Pointer
Size uintptr
}
When the StartupInfoEx
struct is built, we call newProcThreadAttributeList
with a count of len(ProcThreadAttributes) + 2
(taking the default attributes from syscall/exec_windows.go
). Any ProcThreadAttributes
that come from the user override those defaults if using the same Attribute
field, which makes for unambiguous behavior.
Option 2: SecurityAttributes only
An alternative would be to add a SecurityCapabilities
field to SysProcAttr
and
type SysProcAttr struct {
// ...
SecurityCapabilities SecurityCapabilities
}
type SecurityCapabilities struct {
AppContainerSid uintptr // PSID *windows.SID
Capabilities uintptr // SID_AND_ATTRIBUTES *windows.SIDAndAttributes
CapabilityCount uint32
Reserved uint32
}
It would then be up to syscall/exec_windows.go
to create the appropriate attributes for StartupInfoEx
, as we do already for the parent handle, etc.
Comment From: ianlancetaylor
CC @golang/windows
Comment From: qmuntal
Thanks for submitting this proposal. IMO supporting security capabilities would be a good addition.
Some comments:
The concern in #44005 was that attributes set by the user could conflict with parameters and attributes set by syscall.StartProcess
, and some conflicts might not be possible to reconcile. The conclusion was that we want to curate which attributes are allowed to be set by the user so syscall.StartProcess
is always coherent. This discards the option 1.
An alternative would be to add a SecurityCapabilities field to SysProcAttr and
This seems like the way to go. All new types and properties should be defined in the syscall
package, not in x/sys/windows
, as it is what os/exec
uses under the hood. I've modified a bit your proposed API to fit better in syscall
:
package syscall
type SecurityCapabilities struct {
AppContainerSid *SID
Capabilities *SIDAndAttributes
CapabilityCount uint32
Reserved uint32
}
type SysProcAttr struct {
...
SecurityCapabilities *SecurityCapabilities // if set, applies these security capabilities to the new process
}
Comment From: tgross
Makes sense to me! I should have noted in the original proposal that I/we are happy to contribute this patch. What's the next step at this point? Should we submit the patch along the lines above or should we submit a design doc?
Comment From: qmuntal
Makes sense to me! I should have noted in the original proposal that I/we are happy to contribute this patch. What's the next step at this point? Should we submit the patch along the lines above or should we submit a design doc?
We need to wait for this proposal to be discussed by the proposal committee, see the-proposal-process. There is no need for a design doc, the changes are simple enough. I would recommend holding your patch till the proposal is approved.
Comment From: qmuntal
@aclements this proposal is ready to by considered by the proposal committee. See this comment: https://github.com/golang/go/issues/65611#issuecomment-1954650716.
I know the queue is large, but it was been standing more than a year and I don't want it to be left behind.
/cc @neild @alexbrainman
Comment From: neild
The current proposal as I understand it:
Windows permits specifying a SECURITY_CAPABILITIES
when creating a new process. os.StartProcess
and os/exec.Cmd
permit setting platform-specific attributes in a syscall.SysProcAttr
.
We add a definition for this struct to the syscall package, and add a SecurityCapabilities
field to SysProcAttr
:
package syscall
type SecurityCapabilities struct {
AppContainerSid *SID
Capabilities *SIDAndAttributes
CapabilityCount uint32
Reserved uint32
}
type SysProcAttr struct {
...
SecurityCapabilities *SecurityCapabilities // if set, applies these security capabilities to the new process
}
We update syscall.StartProcess
to include this attribute in the Windows CreateProcess
call.
(Normally, we add new platform-specific APIs to golang.org/x/sys, but the fact that the os
package depends on syscall.SysProcAttr
specifically means that any new platform-specific process configuration needs to go in syscall
.)
This all sounds reasonable to me.
Comment From: aclements
This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — aclements for the proposal review group