Change https://golang.org/cl/193117 mentions this issue: ssh: export a transport interface

@FiloSottile, if you have some time to investigate this proposal it would be much appreciated.

The change looks good to me, and I would appreciate this interface.

Hello, just checking to see where this is at in the proposal process. I am still maintaining a fork of this library for an environment where openssh control master sockets are in use.

This was accidentally left off the list when it was created, and now it is waiting to get onto the list of proposals under active consideration. Sorry for the delay.

My immediate use case for this is to implement an application that uses an OpenSSH ControlMaster socket as the transport.

Any code on how this would look like?

Example usage:

package main

import (
	"bytes"
	"encoding/binary"
	"fmt"
	"io"
	"net"
	xssh "golang.org/x/crypto/ssh"
)

func check(err error) {
	if err != nil {
		panic(err)
	}
}

func main() {
	unixAddr := "/tmp/ssh-control-cy@ssh-bastion.acme-corp.com:22"
	c, err := net.Dial("unix", unixAddr)

	if err != nil {
		check(fmt.Errorf("dial failed: %v", err))
	}

	trans, err := handshakeControlProxy(c)
	check(err)

	conn, nchan, reqs, err := xssh.NewClientConnFromTransport(trans)

	cli := xssh.NewClient(conn, nchan, reqs)
	sess, err := cli.NewSession()
	check(err)

	// Print out the hostname on the other end.
	b, err := sess.CombinedOutput("hostname")
	check(err)
	fmt.Println(string(b))
}

const (
	MUX_MSG_HELLO = 0x00000001
	MUX_C_PROXY   = 0x1000000f
	MUX_S_PROXY   = 0x8000000f
	MUX_S_FAILURE = 0x80000003
)

// handshakeControlProxy attempts to establish a transport connection with an
// ssh ControlMaster socket in proxy mode. For details see:
// https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.mux
func handshakeControlProxy(rw io.ReadWriteCloser) (*controlProxyTransport, error) {
	b := &controlBuffer{}
	b.WriteUint32(MUX_MSG_HELLO)
	b.WriteUint32(4) // Protocol Version
	if _, err := rw.Write(b.LengthPrefixedBytes()); err != nil {
		return nil, fmt.Errorf("mux hello write failed: %v", err)
	}

	b.Reset()
	b.WriteUint32(MUX_C_PROXY)
	b.WriteUint32(0) // Request ID
	if _, err := rw.Write(b.LengthPrefixedBytes()); err != nil {
		return nil, fmt.Errorf("mux client proxy write failed: %v", err)
	}

	r := controlReader{rw}
	m, err := r.Next()
	if err != nil {
		return nil, fmt.Errorf("mux hello read failed: %v", err)
	}
	if m.messageType != MUX_MSG_HELLO {
		return nil, fmt.Errorf("mux reply not hello")
	}
	if v, err := m.ReadUint32(); err != nil || v != 4 {
		return nil, fmt.Errorf("mux reply hello has bad protocol version")
	}
	m, err = r.Next()
	if err != nil {
		return nil, fmt.Errorf("error reading mux server proxy: %v", err)
	}
	if m.messageType != MUX_S_PROXY {
		return nil, fmt.Errorf("expected server proxy response got %d", m.messageType)
	}
	return &controlProxyTransport{rw}, nil
}

// controlProxyTransport implements the connTransport interface for
// ControlMaster connections. Each controlMessage has zero length padding and
// no MAC.
type controlProxyTransport struct {
	rw io.ReadWriteCloser
}

func (p *controlProxyTransport) Close() error {
	return p.Close()
}

func (p *controlProxyTransport) GetSessionID() []byte {
	return nil
}

func (p *controlProxyTransport) ReadPacket() ([]byte, error) {
	var l uint32
	err := binary.Read(p.rw, binary.BigEndian, &l)
	if err == nil {
		buf := &bytes.Buffer{}
		_, err = io.CopyN(buf, p.rw, int64(l))
		if err == nil {
			// Discard the padding byte.
			buf.ReadByte()
			return buf.Bytes(), nil
		}
	}
	return nil, err
}

func (p *controlProxyTransport) WritePacket(controlMessage []byte) error {
	l := uint32(len(controlMessage)) + 1
	b := &bytes.Buffer{}
	binary.Write(b, binary.BigEndian, &l) // controlMessage Length.
	b.WriteByte(0)                        // Padding Length.
	b.Write(controlMessage)
	_, err := p.rw.Write(b.Bytes())
	return err
}

func (p *controlProxyTransport) WaitSession() error {
	return nil
}

type controlBuffer struct {
	bytes.Buffer
}

func (b *controlBuffer) WriteUint32(i uint32) {
	binary.Write(b, binary.BigEndian, i)
}

func (b *controlBuffer) LengthPrefixedBytes() []byte {
	b2 := &bytes.Buffer{}
	binary.Write(b2, binary.BigEndian, uint32(b.Len()))
	b2.Write(b.Bytes())
	return b2.Bytes()
}

type controlMessage struct {
	body        bytes.Buffer
	messageType uint32
}

func (p controlMessage) ReadUint32() (uint32, error) {
	var u uint32
	err := binary.Read(&p.body, binary.BigEndian, &u)
	return u, err
}

func (p controlMessage) ReadString() (string, error) {
	var l uint32
	err := binary.Read(&p.body, binary.BigEndian, &l)
	if err != nil {
		return "", fmt.Errorf("error reading string length: %v", err)
	}
	b := p.body.Next(int(l))
	if len(b) != int(l) {
		return string(b), fmt.Errorf("EOF on string read")
	}
	return string(b), nil
}

type controlReader struct {
	r io.Reader
}

func (r controlReader) Next() (*controlMessage, error) {
	p := &controlMessage{}
	var len uint32
	err := binary.Read(r.r, binary.BigEndian, &len)
	if err != nil {
		return nil, fmt.Errorf("error reading message length: %v", err)
	}
	_, err = io.CopyN(&p.body, r.r, int64(len))
	if err != nil {
		return nil, fmt.Errorf("error reading message payload: %v", err)
	}
	err = binary.Read(&p.body, binary.BigEndian, &p.messageType)
	if err != nil {
		return nil, fmt.Errorf("error reading message type: %v", err)
	}
	if p.messageType == MUX_S_FAILURE {
		reason, _ := p.ReadString()
		return nil, fmt.Errorf("server failure: '%s'", reason)
	}
	return p, nil
}

Hi @ianlancetaylor, do you know if this proposal was ever reviewed? A colleague just reached out to see if there was a resolution for this, and figured I would ask...

/cc @FiloSottile

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

Thank you for the proposal. We'd like to support ControlMaster, but this might be too low level an interface, as it leaves a lot of implementation of the ControlMaster protocol to the application. Are there other use cases for a Transport-level interface?

What if we added NewClientConnFromControl that takes a net.Conn dialed to the UNIX socket? (We can't put it in ClientConfig as the socket changes between remotes.)

By the way, I haven't looked at the protocol in depth yet, the reason we can't use NewClientConn for ControlMaster is that transport encryption happens at the transport level, right?

Thanks for looking at this!

Are there other use cases for a Transport-level interface?

I can imagine a few (custom transports / proxy protocols), and will also note that this seems like a logical boundary to unit test RFC 4254 in isolation of 4253. All that said; I don't have a strong opinion on the matter and my only planned usage is a ControlMaster transport.

What if we added NewClientConnFromControl that takes a net.Conn dialed to the UNIX socket?

Seems fine to me. This was actually my original proposal and patch, which i closed out in favor of this one:
#31874
golang/crypto@6af5693

My reasoning for assuming y'all might prefer the generic transport approach was:

• ControlMaster is not part of any IETF SSH specification, it's OpenSSH specific.
• Having a logical boundary between RFC 4254 and 4253 seemed desirable for both extensibility and testing.

the reason we can't use NewClientConn for ControlMaster is that transport encryption happens at the transport level, right?

I believe so, see RFC 4253.

Are there other use cases for a Transport-level interface?

The application/product I was working on is a host access management solution using ssh certificates. We implemented a custom server to handle looking up the correct CA cert from our DB based on info in the client cert. This all worked fine but requires lots of ssh_config and dns check host gymnastics on the client side. We wanted to do improve this experience with a custom ssh client for users who were interesting in adopting it. IIR idea was that our client and server would overlay some logic prior to the actual ssh authentication handshake, do happy path setup, and then proceed to traditional ssh auth.

I don’t remember all the details but the use case is pretty similar to a proxy. Having a transport interface supported here would be very convenient as opposed to some heavier solution with a separate proxy. And it would allow transports to be manage and swapped out independently from the ssh protocol and application implementations as future requirements may or may not dictate.

@FiloSottile and I discussed this and suggest to add NewSharedClientConn that takes as an argument the net.Conn already opened to the control socket. Then the package doesn't need to figure out how to find a control socket. It also avoids "master" in ControlMaster, and the docs for OpenSSH refer to these as shared sockets.

Thoughts?

Can you confirm the approach in golang/crypto@6af5693 is along the lines of what you are suggesting, and that the OpenSSH specific handshake bits should be included?

I chose "NewControlProxyClientConn" because the protocol comes in two flavors; "passenger" and "proxy". I only planned on adding support for "proxy" in my change. I don't have a strong opinion on naming, and "NewSharedClientConn" seems fine.

ping @FiloSottile

cc @golang/security

Can you confirm the approach in golang/crypto@6af5693 is along the lines of what you are suggesting, and that the OpenSSH specific handshake bits should be included?

Yeah, that looks like it, an implementation of proxy mode from PROTOCOL.mux.

I chose "NewControlProxyClientConn" because the protocol comes in two flavors; "passenger" and "proxy". I only planned on adding support for "proxy" in my change. I don't have a strong opinion on naming, and "NewSharedClientConn" seems fine.

Agreed on only supporting proxy mode, but I wouldn't expose that name in the API, because nothing user-facing mentions "proxy" in relation to muxing. "Proxy" makes me think about ProxyJump which is a similarly shaped but very different mechanism. How about NewControlClientConn?

It sounds like the API being proposed is

// NewControlClientConn establishes an SSH connection over a ControlMaster
// socket c. The Request and NewChannel channels must be serviced or the
// connection will hang.
func NewControlClientConn(c net.Conn) (Conn, <-chan NewChannel, <-chan *Request, error)

Is that correct? Is the channel part right? Can we say more about what "serviced" means?

Yes, that should be the extent of the newly exposed API. I think the channel part is correct? I tested this code was working as expected using OpenSSH version 8.2p1 and the following code:

import (
        "fmt"
        "golang.org/x/crypto/ssh"
        "net"
)

func check(err error) {
        if err != nil {
                panic(err)
        }
}

func main() {
        c, err := net.Dial("unix", "/tmp/ssh-control-acme-corp@ssh-bastion.acme-corp.com:22")
        check(err)
        cc, nc, rc, err := ssh.NewControlClientConn(c)
        check(err)
        cli := ssh.NewClient(cc, nc, rc)
        s, err := cli.NewSession()
        check(err)
        o, err := s.CombinedOutput("hostname")
        check(err)
        fmt.Println(string(o))
}

The comment was copied from the NewClientConn method, and quite honestly I don't know exactly what "serviced" was intended to mean. Happy to update this comment, or both comments, if you can help me figure it out.

Ah, well, if that's what NewClientConn does then I suppose it is fine.
Does anyone object to the API in #32958 (comment)?

@rsc could you please disambiguate between NewControlClientConn and NewControlProxyClientConn before moving forward?

NewControlClientConn is the newly exposed API name per previous discussions here about naming. This is the name being used in the open PR. I think @rsc's reference to NewControlProxyClientConn is in error.

I've fixed the name in my summary, added a link to the summary to the top post, and retitled. Any objections to this NewControlClientConn?

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

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

Hello, do you recommend any further action on my part to advance the review of https://go-review.googlesource.com/c/crypto/+/383374 ?

Thanks.

Hello, do you recommend any further action on my part to advance the review of https://go-review.googlesource.com/c/crypto/+/383374 ?

Thanks.

I've added myself as a reviewer to https://go.dev/cl/383374 and left some comments.

Amazing work @y3llowcake, would love to see this merged!

Is there a way to create a unix socket same as openssh? Without this, do we need to start openssh first? Then we connect to the unix socket created by openssh.

what about introducing x/crypto/ssh-transport if this cannot be merged

@y3llowcake Are there some comments need to be resolved?
https://go-review.googlesource.com/c/crypto/+/383374?tab=comments

Love to see it merged!

For anyone stuck on this, before merging https://go-review.googlesource.com/c/crypto/+/383374.

You can try this. But it's unsafe, and you do it at your own risk.

package main

import (
	"log"
	"net"

	"github.com/trzsz/trzsz-ssh/tssh"
	"golang.org/x/crypto/ssh"
)

func main() {
	conn, err := net.Dial("unix", "/path/to/ssh_control_path")
	if err != nil {
		log.Fatal(err)
	}

	ncc, chans, reqs, err := tssh.NewControlClientConn(conn)
	if err != nil {
		log.Fatal(err)
	}

	client := ssh.NewClient(ncc, chans, reqs)
	session, err := client.NewSession()
	if err != nil {
		log.Fatal(err)
	}

	output, err := session.CombinedOutput("hostname")
	if err != nil {
		log.Fatal(err)
	}

	log.Printf("hostname: %s", string(output))
}

You can also do it without tssh. Feel free to copy the whole file into your project:
https://github.com/trzsz/trzsz-ssh/blob/4fc7fe4490dbbb7387d225a38cd79242407e1b22/tssh/control.go