proposal: x/sys/svc/mgr: Add "ConnectWithPermissions" and "OpenServiceWithPermissions" to allow for control on the windows permissions used

[tomqwpl]

Proposal Details

Current the "Connect" method uses a fixed value of "windows.SC_MANAGER_ALL_ACCESS" when calling "windows.OpenSCManager". Similarly, the "OpenService" method uses a fixed value of "windows.SERVICE_ALL_ACCESS" when calling "windows.OpenService".

This proposal adds "ConnectWithPermissions" and "OpenServiceWithPermissions". These would both take an additional "uint32" value that is simply passed through to the underlying windows methods. The existing methods would delegate to these new methods passing the existing permissions values.

So you would have:

func Connect() (*Mgr, error) {
	return ConnectRemote("")
}

func ConnectWithPermissions(access uint32) (*Mgr, error) {
	return ConnectRemoteWithPermissions("", access)
}

func ConnectRemote(host string) (*Mgr, error) {
	return ConnectRemoteWithPermissions(host, windows.SC_MANAGER_ALL_ACCESS)
}

func ConnectRemoteWithPermissions(host string, access uint32) (*Mgr, error) {
...
h, err := windows.OpenSCManager(s, nil, access)
...

Then

func (m *Mgr) OpenService(name string) (*Service, error) {
	return m.OpenServiceWithPermissions(name, windows.SERVICE_ALL_ACCESS)
}

func (m *Mgr) OpenServiceWithPermissions(name string, access uint32) (*Service, error) {
	h, err := windows.OpenService(m.Handle, syscall.StringToUTF16Ptr(name), access)
...

[alexbrainman]

@tomqwpl thanks for your proposal.

OpenServiceWithPermissions SGTM.

But I suggest you adjust your proposal to add just

func ConnectWithPermissions(host string, access uint32) (*Mgr, error)

instead of ConnectWithPermissions and ConnectRemoteWithPermissions. Single function is enough to achieve what you require.

And please add documentation strings for new APIs that everyone will see.

Thank you.

Alex

[tomqwpl]

And please add documentation strings for new APIs that everyone will see.

In the PR sure. I didn't want to do it here in the comment as it got in the way what I was trying to communicate. The purpose here was a proposal for additions to the interface and to get that approved before working up a PR.

But I suggest you adjust your proposal to add just...

I did it the way I did simply because it was the simplest way of implementing it, it was the way it falls out most naturally. As it stands, all of the implementation is within ConnectRemote. To preserve that and avoid code duplication or a bigger refactor, you need to just add a permissions argument to that function. Whilst I don't personally need to to "ConnectRemoteWithPermissions" I can see how you might want to, so it seemed unnecessary to artificially not have that option. Also, ConnectWithPermissions where I need to pass "" if I want to connect to the local system seems unnecessarily obscure (not obvious that's what you need to do). For the sake of adding an extra delegation function, you got a clearer interface. So for me, the suite of functions presented represented the best option. But if you want just the single function, that's fine.

[gopherbot]

Change https://go.dev/cl/576796 mentions this issue: windows/svc/mgr: Allow perms with windows svc mgr

[ianlancetaylor]

CC @golang/windows