x/crypto/ssh: ssh client hangs on waitting for the openConfirm message.

[ChiWu-Zero]

• overview

The hang problem arises when using the library, the ssh client key code in file ssh/mux.go:

open := channelOpenMsg{
	ChanType:         chanType,
	PeersWindow:      ch.myWindow,
	MaxPacketSize:    ch.maxIncomingPayload,
	TypeSpecificData: extra,
	PeersID:          ch.localId,
}
if err := m.sendMessage(open); err != nil {
	return nil, err
}

switch msg := (<-ch.msg).(type) {
case *channelOpenConfirmMsg:
	return ch, nil
case *channelOpenFailureMsg:
	return nil, &OpenChannelError{msg.Reason, msg.Message}
default:
	return nil, fmt.Errorf("ssh: unexpected packet in response to channel open: %T", msg)
}

The process hangs on the line of "switch msg := (<-ch.msg).(type) "，it may loss the OpenConfirm msg because of the sshd server or the net status.
Maybe we should add the timeout when waitting for the confirm msg.

select {
case data := <-ch.msg:
	switch msg := data.(type) {
	case *channelOpenConfirmMsg:
		return ch, nil
	case *channelOpenFailureMsg:
		return nil, &OpenChannelError{msg.Reason, msg.Message}
	default:
		return nil, fmt.Errorf("ssh: unexpected packet in response to channel open: %T", msg)
	}
case <-time.After(aliveTime):
	return nil, fmt.Errorf("ssh: the confirm message maybe lost")
}

[ianlancetaylor]

This doesn't seem to be a proposal, taking it out of the proposal process.

[ianlancetaylor]

CC @golang/security

[nemith]

Context support here would be greatly appreciated.

[schachmat]

Just encountered this issue as well. Was super annoying to debug where the hang came from. Proper context support seems like the right way forward. Slightly modified suggestion from first comment:

select {
case data := <-ch.msg:
	switch msg := data.(type) {
	case *channelOpenConfirmMsg:
		return ch, nil
	case *channelOpenFailureMsg:
		return nil, &OpenChannelError{msg.Reason, msg.Message}
	default:
		return nil, fmt.Errorf("ssh: unexpected packet in response to channel open: %T", msg)
	}
case <-ctx.Done():
	return nil, ctx.Err()
}

Just needs to be piped through the layers and callers need to adapt (or a second set of public functions which additionally accept the context is offered).