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: add NewControlClientConn #32958

Open
y3llowcake opened this issue Jul 5, 2019 · 28 comments · May be fixed by golang/crypto#100
Open

x/crypto/ssh: add NewControlClientConn #32958

y3llowcake opened this issue Jul 5, 2019 · 28 comments · May be fixed by golang/crypto#100
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal-FinalCommentPeriod
Milestone

Comments

@y3llowcake
Copy link

@y3llowcake y3llowcake commented Jul 5, 2019

Update, 23 Feb 2022: The new API is #32958 (comment).


What version of Go are you using (go version)?

$ go version
go version go1.12.6 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"                         
GOBIN=""                               
GOCACHE="/tmp/bazel_shared/gocache"    
GOEXE=""                               
GOFLAGS="-mod=readonly"                
GOHOSTARCH="amd64"                     
GOHOSTOS="linux"                       
GOOS="linux"                           
GOPATH="/tmp/bazel_shared/gopath"      
GOPROXY="direct"                       
GORACE=""                              
GOROOT="/home/cy/.cache/bazel/_bazel_cy/80b33c882b53d18473a4d1179c663f81/external/go_sdk"                                                                     
GOTMPDIR=""                            
GOTOOLDIR="/home/cy/.cache/bazel/_bazel_cy/80b33c882b53d18473a4d1179c663f81/external/go_sdk/pkg/tool/linux_amd64"                                             
GCCGO="gccgo"                          
CC="gcc"                               
CXX="g++"                              
CGO_ENABLED="0"                        
GOMOD="/home/cy/.cache/bazel/_bazel_cy/80b33c882b53d18473a4d1179c663f81/execroot/goslackgo/bazel-out/k8-fastbuild/bin/go.runfiles/goslackgo/go_gocmd_execroot/go.mod"
CGO_CFLAGS="-g -O2"                    
CGO_CPPFLAGS=""                        
CGO_CXXFLAGS="-g -O2"                  
CGO_FFLAGS="-g -O2"                    
CGO_LDFLAGS="-g -O2"                   
PKG_CONFIG="pkg-config"                
GOGCCFLAGS="-fPIC -m64 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build807263343=/tmp/go-build -gno-record-gcc-switches" 

Proposal: Feature Request

I propose a modification to the x/crypto/ssh library that would allow an application to use this library's implementation of the SSH Connection Protocol (RFC 4254), but provide a separate implementation of the SSH Transport Protocol (RFC 4253).

Concretely this would require adapting the existing packetConn interface and exporting it, and then adding a Client constructor that operated on this interface:

type Transport interface {
  WritePacket([]byte) error
  ReadPacket() ([]byte, error)
  Close() error
}

func NewClientFromTransport(t Transport) (Conn, <-chan NewChannel, <-chan *Request, error) 

My immediate use case for this is to implement an application that uses an OpenSSH ControlMaster socket as the transport. However, I can imagine other use cases for this. This proposal is an alternative to #31874.

@gopherbot gopherbot added this to the Proposal milestone Jul 5, 2019
@agnivade agnivade added the Proposal-Crypto label Jul 6, 2019
@y3llowcake y3llowcake linked a pull request Sep 3, 2019 that will close this issue
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 30, 2019

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

@y3llowcake
Copy link
Author

@y3llowcake y3llowcake commented Mar 5, 2020

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

@dcow
Copy link

@dcow dcow commented Jun 8, 2020

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

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Feb 17, 2021
@y3llowcake
Copy link
Author

@y3llowcake y3llowcake commented Mar 14, 2021

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.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 16, 2021

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.

@marvinthepa
Copy link

@marvinthepa marvinthepa commented Mar 30, 2021

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?

@y3llowcake
Copy link
Author

@y3llowcake y3llowcake commented Mar 30, 2021

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
}

@y3llowcake
Copy link
Author

@y3llowcake y3llowcake commented Aug 31, 2021

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...

@rsc
Copy link
Contributor

@rsc rsc commented Sep 15, 2021

/cc @FiloSottile

@rsc rsc moved this from Incoming to Active in Proposals Sep 15, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Sep 15, 2021

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

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Oct 13, 2021

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?

@y3llowcake
Copy link
Author

@y3llowcake y3llowcake commented Oct 13, 2021

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.

@dcow
Copy link

@dcow dcow commented Oct 13, 2021

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.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 27, 2021

@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?

@rsc rsc changed the title proposal: x/crypto/ssh: export a transport interface proposal: x/crypto/ssh: add NewSharedClientConn Oct 27, 2021
@y3llowcake
Copy link
Author

@y3llowcake y3llowcake commented Oct 29, 2021

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.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 3, 2021

ping @FiloSottile

1 similar comment
@rsc
Copy link
Contributor

@rsc rsc commented Dec 1, 2021

ping @FiloSottile

@rsc
Copy link
Contributor

@rsc rsc commented Jan 5, 2022

cc @golang/security

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Jan 28, 2022

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?

@rsc
Copy link
Contributor

@rsc rsc commented Feb 9, 2022

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?

@y3llowcake
Copy link
Author

@y3llowcake y3llowcake commented Feb 10, 2022

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.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 16, 2022

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

@ucirello
Copy link
Contributor

@ucirello ucirello commented Feb 17, 2022

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

@y3llowcake
Copy link
Author

@y3llowcake y3llowcake commented Feb 23, 2022

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.

@rsc rsc changed the title proposal: x/crypto/ssh: add NewSharedClientConn proposal: x/crypto/ssh: add NewControlClientConn Feb 23, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Feb 23, 2022

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?

@rsc
Copy link
Contributor

@rsc rsc commented Mar 2, 2022

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

@rsc rsc moved this from Active to Likely Accept in Proposals Mar 2, 2022
@rsc rsc moved this from Likely Accept to Accepted in Proposals Mar 9, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Mar 9, 2022

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

@rsc rsc changed the title proposal: x/crypto/ssh: add NewControlClientConn x/crypto/ssh: add NewControlClientConn Mar 9, 2022
@rsc rsc removed this from the Proposal milestone Mar 9, 2022
@rsc rsc added this to the Backlog milestone Mar 9, 2022
@y3llowcake
Copy link
Author

@y3llowcake y3llowcake commented Apr 22, 2022

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

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal-FinalCommentPeriod
Projects
Proposals
Accepted
Development

Successfully merging a pull request may close this issue.

9 participants