x/crypto/ssh: provide Client.DialContext method

[ydnar]

Similar to #17759, it would be useful for ssh.Client to provide a DialContext method next to its Dial method. We use SSH tunnels for connecting to domain registries that use IP allowlists.

Note: per some confusion in the comments below, this does not refer to the top-level Dial func that opens an SSH connection. This proposal specifically refers to adding a (*ssh.Client).DialContext method that opens a TCP/IP tunnel, (returning a net.Conn), useful for proxying connections across an SSH connection.

An implementation can be found here: CL 504735

Original context from 2017:

We’ve been progressively consolidating our timeout and deadline code to use context.Context throughout, implementing workarounds for dialers in APIs that lack Context support.

[hanwen]

what exactly do you want controlled with deadline? The underlying TCP deadline? Or were you thinking of a SSH-level support for deadlines?

[glycerine]

My use case is not leaking goroutines. When my goroutine does a blocking read on an ssh.Channel it is always blocked, I can't signal that goroutine to shutdown. The usual way with a net.Conn is to set read and write deadlines, then poll for errors. So I was thinking of SSH channel level support for (both read and write) deadlines.

[glycerine]

My experimental fork https://github.com/glycerine/xcryptossh provides Dial with context and idle timers for Read/Write unblocking.

[denisvmedia]

Actually it's doable with user code without forking/modifying the lib.

func Dial(ctx context.Context, network, addr string, config *ssh.ClientConfig) (*ssh.Client, error) {
	d := net.Dialer{Timeout: config.Timeout}
	conn, err := d.DialContext(ctx, network, addr)
	if err != nil {
		return nil, err
	}
	c, chans, reqs, err := ssh.NewClientConn(conn, addr, config)
	if err != nil {
		return nil, err
	}
	return ssh.NewClient(c, chans, reqs), nil
}

[prochac]

Actually it's doable with user code without forking/modifying the lib.

func Dial(ctx context.Context, network, addr string, config *ssh.ClientConfig) (*ssh.Client, error) {
	d := net.Dialer{Timeout: config.Timeout}
	conn, err := d.DialContext(ctx, network, addr)
	if err != nil {
		return nil, err
	}
	c, chans, reqs, err := ssh.NewClientConn(conn, addr, config)
	if err != nil {
		return nil, err
	}
	return ssh.NewClient(c, chans, reqs), nil
}

This should be part of library as ssh.DialWithContext or similar.

[gopherbot]

Change https://go.dev/cl/504735 mentions this issue: ssh: add (*Client).DialContext method

[drakkan]

LGTM and the patch implementing this proposal is on Hold while the proposal is accepted.

Can we add this proposal to the active column of the proposals project? Thank you

cc @golang/proposal-review

[rsc]

Seems unobjectionable. Adding to minutes, can probably likely accept next week.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

// DialContext initiates a connection to the addr from the remote host.
//
// The provided Context must be non-nil. If the context expires before the
// connection is complete, an error is returned. Once successfully connected,
// any expiration of the context will not affect the connection.
//
// See [Dial] for additional information.
func (c *Client) DialContext(ctx context.Context, n, addr string) (net.Conn, error) {

[prochac]

I may be using it wrongly, but the usage is horrible. We have to stick with our wrapper for sane usability.

func DialWithContext(ctx context.Context, network, addr string, config *ssh.ClientConfig) (*ssh.Client, error) {
	ctx, cancel := context.WithTimeout(ctx, config.Timeout)
	defer cancel()
	sshClient, err := ssh.Dial(network, addr, config)
	if err != nil {
		return nil, errors.Wrap(err, "dialing SSH connection")
	}
	conn, err := sshClient.DialContext(ctx, network, addr)
	if err != nil {
		return nil, errors.Wrap(err, "dialing SSH connection")
	}
	c, chans, reqs, err := ssh.NewClientConn(conn, addr, config)
	if err != nil {
		return nil, errors.Wrap(err, "establishing SSH connection")
	}
	return ssh.NewClient(c, chans, reqs), nil
}

[drakkan]

@prochac , from the first comment

Note: per some confusion in the comments below, this does not refer to the top-level Dial func that opens an SSH connection. This proposal specifically refers to adding a (*ssh.Client).DialContext method that opens a TCP/IP tunnel, (returning a net.Conn), useful for proxying connections across an SSH connection.

Can you please share your wrapper? Maybe we can use it for a new proposal. Thank you.

[prochac]

@prochac , from the first comment

Note: per some confusion in the comments below, this does not refer to the top-level Dial func that opens an SSH connection. This proposal specifically refers to adding a (*ssh.Client).DialContext method that opens a TCP/IP tunnel, (returning a net.Conn), useful for proxying connections across an SSH connection.

Can you please share your wrapper? Maybe we can use it for a new proposal. Thank you.

I just did 😅

Secretly, I hope someone will tell me that I'm doing something horribly wrong in my DialWithContext func. That there is a nicer way.

[ydnar]

@prochac you’re not the first person to confuse the DialContext method with a top-level ssh.DialContext func. The former (the DialContext method) establishes a tunnel over the SSH connection to the remote host. The latter creates a new ssh.Client. Your DialWithContext creates a new SSH client within an SSH client, effectively double-wrapping the connection (if it works at all).

It’s relatively straightforward to add a top-level DialContext func to the crypto/ssh package, it’d look like this:

// DialContext starts a client connection to the given SSH server. It is a
// convenience function that connects to the given network address,
// initiates the SSH handshake, and then sets up a Client.
//
// The provided Context must be non-nil. If the context expires before the
// connection is complete, an error is returned. Once successfully connected,
// any expiration of the context will not affect the connection.
//
// See [Dial] for additional information.
func DialContext(ctx context.Context, network, addr string, config *ClientConfig) (*Client, error) {
	d := net.Dialer{
		Timeout: config.Timeout,
	}
	conn, err := d.DialContext(ctx, network, addr)
	if err != nil {
		return nil, err
	}
	type result struct {
		client *Client
		err    error
	}
	ch := make(chan result)
	go func() {
		var client *Client
		c, chans, reqs, err := NewClientConn(conn, addr, config)
		if err == nil {
			client = NewClient(c, chans, reqs)
		}
		select {
		case ch <- result{client, err}:
		case <-ctx.Done():
			if client != nil {
				client.Close()
			}
		}
	}()
	select {
	case res := <-ch:
		return res.client, res.err
	case <-ctx.Done():
		return nil, context.Cause(ctx)
	}
}