Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/crypto/ssh: provide Client.DialContext method #20288

Closed
ydnar opened this issue May 8, 2017 · 14 comments
Closed

x/crypto/ssh: provide Client.DialContext method #20288

ydnar opened this issue May 8, 2017 · 14 comments

Comments

@ydnar
Copy link

ydnar commented May 8, 2017

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.

@gopherbot gopherbot added this to the Unreleased milestone May 8, 2017
@hanwen
Copy link
Contributor

hanwen commented Jun 7, 2017

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

@glycerine
Copy link

glycerine commented Aug 11, 2017

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
Copy link

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

@denisvmedia
Copy link

denisvmedia commented Oct 12, 2020

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
Copy link

prochac commented May 4, 2021

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.

nemith added a commit to nemith/netconf that referenced this issue Jul 23, 2022
Context is a good way to handle cancelation so adding context to the Dial to
allow for connection timeout is  good thing.

Followed the method defined at: golang/go#20288 (comment)
ydnar added a commit to ydnar/crypto that referenced this issue Jun 21, 2023
ydnar added a commit to ydnar/crypto that referenced this issue Jun 21, 2023
@gopherbot
Copy link

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

ydnar added a commit to ydnar/crypto that referenced this issue Jun 21, 2023
ydnar added a commit to ydnar/crypto that referenced this issue Jun 22, 2023
@dmitshur dmitshur changed the title x/crypto/ssh: provide Client.DialContext method proposal: x/crypto/ssh: provide Client.DialContext method Jun 22, 2023
@drakkan
Copy link
Member

drakkan commented Oct 10, 2023

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
Copy link
Contributor

rsc commented Oct 27, 2023

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

@rsc
Copy link
Contributor

rsc commented Oct 27, 2023

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
Copy link
Contributor

rsc commented Nov 10, 2023

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) {

@rsc rsc changed the title proposal: x/crypto/ssh: provide Client.DialContext method x/crypto/ssh: provide Client.DialContext method Nov 10, 2023
HouseK added a commit to HouseK/crypto that referenced this issue Dec 4, 2023
This change adds DialContext to ssh.Client, which opens a TCP-IP
connection tunneled over the SSH connection. This is useful for
proxying network connections, e.g. setting
(net/http.Transport).DialContext.

Fixes golang/go#20288.

Change-Id: I110494c00962424ea803065535ebe2209364ac27
GitHub-Last-Rev: 3176984
GitHub-Pull-Request: golang#260
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/504735
Run-TryBot: Nicola Murino <nicola.murino@gmail.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
Auto-Submit: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Nicola Murino <nicola.murino@gmail.com>
Commit-Queue: Nicola Murino <nicola.murino@gmail.com>
@prochac
Copy link

prochac commented Dec 7, 2023

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
Copy link
Member

drakkan commented Dec 7, 2023

@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
Copy link

prochac commented Dec 12, 2023

@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
Copy link
Author

ydnar commented Dec 12, 2023

@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)
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.

9 participants