CommandWriter Close() infinite loop

[kke]

https://github.com/masterzen/winrm/blob/master/command.go#L227-L230

// Close method wrapper
// commandWriter implements io.Closer interface
func (w *commandWriter) Close() error {
	w.eof = true
	return w.Close()
}

I'm not a go-wizard, but doesn't this create an infinite loop and cause a stack overflow panic? I got this:

runtime: goroutine stack exceeds 1000000000-byte limit
fatal error: stack overflow

runtime stack:
runtime.throw(0x1611bb0, 0xe)
	/usr/local/Cellar/go/1.13.7/libexec/src/runtime/panic.go:774 +0x72
runtime.newstack()
	/usr/local/Cellar/go/1.13.7/libexec/src/runtime/stack.go:1046 +0x6e9
runtime.morestack()
	/usr/local/Cellar/go/1.13.7/libexec/src/runtime/asm_amd64.s:449 +0x8f

goroutine 173 [running]:
github.com/masterzen/winrm.(*commandWriter).Close(0xc00023ecc0, 0x0, 0x0)
	/Users/kimmo/Projects/go/pkg/mod/github.com/masterzen/winrm@v0.0.0-20200518133226-8ad2e8823256/command.go:227 +0x52 fp=0xc02085a360 sp=0xc02085a358 pc=0x1404082
github.com/masterzen/winrm.(*commandWriter).Close(0xc00023ecc0, 0x0, 0x0)
	/Users/kimmo/Projects/go/pkg/mod/github.com/masterzen/winrm@v0.0.0-20200518133226-8ad2e8823256/command.go:229 +0x2f fp=0xc02085a388 sp=0xc02085a360 pc=0x140405f
github.com/masterzen/winrm.(*commandWriter).Close(0xc00023ecc0, 0x0, 0x0)
	/Users/kimmo/Projects/go/pkg/mod/github.com/masterzen/winrm@v0.0.0-20200518133226-8ad2e8823256/command.go:229 +0x2f fp=0xc02085a3b0 sp=0xc02085a388 pc=0x140405f
github.com/masterzen/winrm.(*commandWriter).Close(0xc00023ecc0, 0x0, 0x0)
	/Users/kimmo/Projects/go/pkg/mod/github.com/masterzen/winrm@v0.0.0-20200518133226-8ad2e8823256/command.go:229 +0x2f fp=0xc02085a3d8 sp=0xc02085a3b0 pc=0x140405f
github.com/masterzen/winrm.(*commandWriter).Close(0xc00023ecc0, 0x0, 0x0)

...and so on

[masterzen]

This indeed seems quite suspicious.
I think this should really be w.Command.Close().

Can you implement it and let me know if that indeed solves your original issue?
If not, then that means we're going to have to resume #82 and finish that work.

[kke]

@masterzen

I think this should really be w.Command.Close().

Hmm, should closing Stdin really close the command? Something comparable to echo $secret_stuff | a_command_that_takes_some_time would die instantly after the echo completes. Maybe it should just do the w.eof = true part and return nil.

Can you implement it and let me know if that indeed solves your original issue?

I'll try

[masterzen]

As said in #82, it probably requires closing the stream with sendInput(nil) then later do a command.Close() (probably from RunWithString and RunWithInput).

[kke]

In the python lib there's this:

    def _winrm_send_input(self, protocol, shell_id, command_id, stdin, eof=False):
        rq = {'env:Envelope': protocol._get_soap_header(
            resource_uri='http://schemas.microsoft.com/wbem/wsman/1/windows/shell/cmd',
            action='http://schemas.microsoft.com/wbem/wsman/1/windows/shell/Send',
            shell_id=shell_id)}
        stream = rq['env:Envelope'].setdefault('env:Body', {}).setdefault('rsp:Send', {})\
            .setdefault('rsp:Stream', {})
        stream['@Name'] = 'stdin'
        stream['@CommandId'] = command_id
        stream['#text'] = base64.b64encode(to_bytes(stdin))
        if eof:
            stream['@End'] = 'true'
        rs = protocol.send_message(xmltodict.unparse(rq))

Notice: stream['@End'] = 'true'

https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/connection/winrm.py#L430-L442

I don't see this library sending the @EnD anywhere, I believe it should be done somehow in request.go:

func NewSendInputRequest(uri, shellId, commandId string, input []byte, params *Parameters) *soap.SoapMessage {
	if params == nil {
		params = DefaultParameters
	}
	message := soap.NewMessage()

	defaultHeaders(message, uri, params).
		Action("http://schemas.microsoft.com/wbem/wsman/1/windows/shell/Send").
		ResourceURI("http://schemas.microsoft.com/wbem/wsman/1/windows/shell/cmd").
		ShellId(shellId).
		Build()

	content := base64.StdEncoding.EncodeToString(input)

	send := message.CreateBodyElement("Send", soap.DOM_NS_WIN_SHELL)
	streams := message.CreateElement(send, "Stream", soap.DOM_NS_WIN_SHELL)
	streams.SetAttr("Name", "stdin")
	streams.SetAttr("CommandId", commandId)
	streams.SetContent(content)
	return message
}

I don't think sendInput(nil) alone accomplishes anything but send an empty request. Maybe here in request.go it should have something like if input == nil { streams.SetAttr("End", "true") } (though this will always create an extra request, the @End = true could have been embedded in the previous message in case it ended in EOF, but I don't know if that is worth optimizing)

I tried this approach and it seems to work.

With:

....
	streams.SetContent(content)
	if input == nil {
		streams.SetAttr("End", "true")
	}
	return message
}

and doing a w.sendInput(nil) in the stdin Close() it does seem to do the trick.

[masterzen]

Interestingly I can't find the documentation about @End in:
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-wsmv/af694324-e034-404d-a61d-de57c8cba74f
nor:
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-wsmv/2629bd02-7269-4d7a-9c6c-a0e5e8ade2bc

The ruby implementation doesn't seem to send this attribute either.

I'm going to test over the week-end and let you know what would be the best strategy.
What command are you trying to call and send Input to?

[kke]

Well for example docker login -u %s --password-stdin and feed it with a password.

The current connection + stdin test I added to investigate this problem is:

func testConnection(host *api.Host) error {
	log.Infof("%s: testing connection", host.Address)
	privCheck := "$currentPrincipal = New-Object Security.Principal.WindowsPrincipal([Security.Principal.WindowsIdentity]::GetCurrent()); if ($currentPrincipal.IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)) { Write-Host 'User has admin privileges'; exit 0; } else { Write-Host 'User does not have admin privileges'; exit 1 }"
	err := host.ExecCmd("powershell.exe", privCheck, false, false)
	if err != nil {
		err := host.ExecCmd("sudo bash -", "ps", false, false)
		if err != nil {
			return err
		}
	}
	return nil
}

(host.ExecCmd could be calling my winrm or ssh package and the function also serves as a check to see if passwordless sudo works / if the user has admin privileges on windows)

[kke]

I think my PR somewhat works but it fails in tests, I think it needs a bit different strategy, maybe a bool param for eof. I'm not using the RunWithInput in my own code so I'm not sure if that still works.

[kke]

Ok I changed it a bit, now it passes tests and it still works for me, don't know for others.

[kke]

Interestingly I can't find the documentation about @End in:
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-wsmv/af694324-e034-404d-a61d-de57c8cba74f
nor:
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-wsmv/2629bd02-7269-4d7a-9c6c-a0e5e8ade2bc

The description of #82 links to some 2006 pdf:

Fix this by sending an additional rsp:Stream block with no data and an
End="true" attribute after all stdin data has been sent. The End
attribute is specified in section 3.7.5 of "Remote Shell Web Services
Protocol" (http://xml.coverpages.org/MS-RemoteShell-200607.pdf).