x/crypto/ssh: expose negotiated algorithms

[hanwen]

Folks running production servers are under opposing pressures, to both

1. stop supporting obsolete or vulnerable algorithms and/or key sizes.
2. keep serving customers and migrate them away from obsolete algorithms

they can only make informed decisions on what to do (reach out to customers, switch off algorithms etc.) if they have data which algorithms are actually in use.

For the TLS library, I think this can be achieved by inspecting https://pkg.go.dev/crypto/tls#ConnectionState.

The SSH library doesn't expose this information, more in particular:

• Cipher, MAC, Compression in read/write directions
• host key algorithm
• key exchange algorithm.

this information is captured in type algorithms
https://cs.opensource.google/go/x/crypto/+/master:ssh/common.go;l=185;drc=6fad3dfc18918c2ac9c112e46b32473bd2e5e2f9

the proposal is to export type algorithms, and add

func Algorithms() Algorithms

to the ssh.Conn type.

This is technically an incompatible API change. However, the ssh.Conn (as well as ssh.Channel) are misdesigned, and should have been structs instead. They are not meant to be implemented by other package, so I think it's OK to add this method to the interface.

[ianlancetaylor]

CC @golang/security

[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]

/cc @FiloSottile and @rolandshoemaker for thoughts

[rsc]

The compatible way to do this would be to define

type AlgorithmsConn interface {
    Conn
    Algorithms() Algorithms
}

[rsc]

Talked to @FiloSottile. In general we are supportive of expanding things here. The algorithms struct right now has lots of other unexported fields. It is unclear to us what exactly the public API for Algorithms would be.

[hanwen]

my proposal is

type DirectionAlgorithms struct {
	Cipher      string
	MAC         string
	Compression string
}
type Algorithms struct {
	Kex     string
	HostKey string
	W       DirectionAlgorithms
	R       DirectionAlgorithms
}

Regarding

type AlgorithmsConn interface {
    Conn
    Algorithms() Algorithms
}

This is backward compatible, but runs the risk that we'd create a new interface for every bit of info we'd want to add down the line.

Maybe we could do func AlgorithmsOf(c Conn) *Algorithms instead?

[rsc]

What do W and R stand for and are those the right names?

[hanwen]

Oh, sure. How about:

type Algorithms struct {
	Kex     string
	HostKey string
	Write       DirectionAlgorithms
	Read       DirectionAlgorithms
}

We could also do

type Algorithms struct {
	Kex     string
	HostKey string
	ClientToServer       DirectionAlgorithms
	ServerToClient       DirectionAlgorithms
}

which is probably a bit more self-explanatory for callers, but it means slightly more complexity on the implementation side.

Also, I think callers will typically only be implementing either the client or the server, so Read and Write are sufficiently unambiguous.

[rolandshoemaker]

There does seem to be a bunch of stuff we'd like to expose down the road, and expanding the Conn interface each time seems unideal. I'd probably vote for something like func AlgorithmsFor(Conn) Algorithms instead.

In terms of the shape of Algorithms I think just exporting all of the currently private fields seems fine. I don't think the ClientToServer/ServerToClient names are particularly clearer. I think it'd probably be simpler to just expand w and r to Write and Read, which paired with the Direction part of DirectionAlgorithms I think makes it clear what they represent.

[rsc]

Re AlgorithmsConn vs AlgorithmsFor(Conn), we've established the AlgorithmsConn "extension interface" pattern in other contexts already, specifically the fs.FS interface and its extension interfaces, so I think that's probably okay. So it sounds like

type Algorithms struct {
	Kex     string
	HostKey string
	Read       DirectionAlgorithms
	Write       DirectionAlgorithms
}

type AlgorithmsConn interface {
	Conn
	Algorithms() Algorithms
}

type DirectionAlgorithms struct {
	Cipher      string
	MAC         string
	Compression string
}

Do I have that right?

[rolandshoemaker]

Yup, sounds right.

[hanwen]

Let me look around the bug tracker to see if there are other kind of things we could potentially expose (one thing that comes to mind is wiring Context through the SSH package) to see if we could do a combined update.

[neild]

This seems like a case where ssh.Conn should have been a concrete type to permit future expansion.

Perhaps we should add a concrete type, and say that the Conns returned by ssh package constructors can always be type asserted to *ssh.SSHConn? That would avoid the need to add a new interface every time we think of something that should be exposed.

[hanwen]

This seems like a case where ssh.Conn should have been a concrete type to permit future expansion.

yes, I was a young and innocent Go developer :-)

Perhaps we should add a concrete type, and say that the Conns returned by ssh package constructors can always be type asserted to *ssh.SSHConn? That would avoid the need to add a new interface every time we think of something that should be exposed.

I like that idea better than the extended interfaces, but it will be a bit of work to add the type. Maybe ssh.Connection (to avoid the stutter.)

[rsc]

If we use #58523 (comment), have all concerns been addressed?

[hanwen]

I wanted to do something like https://go-review.googlesource.com/c/crypto/+/507779

then we can add the algorithms as a normal method on Connection

[rsc]

Having both Conn and Connection doesn't sound right, even as a way to deal with legacy questions.

[hanwen]

Having both Conn and Connection doesn't sound right, even as a way to deal with legacy questions.

aside from naming details, are you saying it is better that we should add a new interface for every method we add?

[rsc]

Are there a bunch more methods that will need to be added?

[rsc]

/cc @drakkan and @FiloSottile for thoughts

[rsc]

Probably 61537 is wrong and we should drop Algorithms from the remaining struct fields in 61537. It is strange to have it in some fields but not others, and the overall struct is named Algorithms so having that word in the fields is redundant.

[drakkan]

Probably 61537 is wrong and we should drop Algorithms from the remaining struct fields in 61537. It is strange to have it in some fields but not others, and the overall struct is named Algorithms so having that word in the fields is redundant.

We already have HostKeyAlgorithms in ClientConfig and PublicKeyAuthAlgorithms in ServerConfig, keeping the same name may avoid confusion even if Algorithm is redundant, anyway we can also improve docs to avoid any confusion

[rsc]

I still think we should drop Algorithms inside the Algorithms struct. It is defensible to have them in ClientConfig and ServerConfig since that is not ClientConfigAlgorithms and ServerConfigAlgorithms.

[rsc]

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

The proposal is to add

type NegotiatedAlgorithms struct {
	KeyExchange string 
	HostKey string
	Read    DirectionAlgorithms
	Write   DirectionAlgorithms
}

type AlgorithmsConnMetadata interface {
	ConnMetadata
	Algorithms() NegotiatedAlgorithms
}

type DirectionAlgorithms struct {
	Cipher      string
	MAC         string
}

[drakkan]

Thanks for accepting this proposal!
I rebased and adapted the CL that implements it. It depends on #61537 and approval of #54743 should also be discussed to avoid to have KEX algorithms supported only on the client side

[onemorezero]

Hi, this feature will be super useful, any updates on when it will be merged?