x/sys/windows: allow specifying Security Capabilities in SysProcAttr

[tgross]

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 #44011. This was discussed around the same time as a rejected proposal to add the full StartupInfoEx struct #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.

[ianlancetaylor]

CC @golang/windows

[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
}

[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?

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

[qmuntal]

@aclements this proposal is ready to by considered by the proposal committee. See this comment: #65611 (comment).

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

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

[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

[aclements]

Let's either drop the Reserved field or make it unexported. From a Go perspective, this is a detail of C struct layout and compatibility. We can keep the layout compatible for implementation convenience, but shouldn't expose that at a Go API level.