proposal: x/crypto/ssh: expose supported Kex algorithms

[jannfis]

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

$ go version
go version go1.16.3 linux/amd64

Does this issue reproduce with the latest release?

Probably.

With x/crypto/ssh as of latest commit golang/crypto@c07d793

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/vagrant/.cache/go-build"
GOENV="/home/vagrant/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/vagrant/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/vagrant/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/vagrant/sdk/go1.16.3"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/vagrant/sdk/go1.16.3/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.3"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/vagrant/go/src/github.com/argoproj/argo-cd/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 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3756356103=/tmp/go-build -gno-record-gcc-switches"

What did you do?

We're using the x/crypto/ssh package to implement an SSH client and we want to make allowed Key Exchange (kex) algorithms configurable by the user.

Alas, the package doesn't provide us with a method to retrieve the supported algorithms, so we have to copy&paste the constants over to our own package. From a maintenance perspective, this might not be the best approach, because the supported algorithms in x/crypto/ssh may change over time and we'd have to track these changes.

What did you expect to see?

I did expect a method in the x/crypto/ssh package that makes the list of available algorithms available to consumers of the package, e.g. KexAlgorithms() ([]string). As far as I understood from the code, client and server implementations support a different set of Kex algorithms, so maybe KexAlgorithmsClient() ([]string) and KexAlgorithmsServer() ([]string) could also make sense here.

For example, with OpenSSH you can run ssh -Q kex to get a list of supported Kex algorithms in the client.

What did you see instead?

Private constants in x/crypto/ssh package here

[seankhliao]

cc @FiloSottile

[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

[0xjac]

It would be nice if—for similar reasons—supported host key algorithms, ciphers and MACs could be exposed too, either as functions as suggested in the issue description, or simply by making those variables public.

Supported MACs is especially important until #39397 is resolved to validate the supported algorithms.

(Happy to open a separate issue, if you deem it necessary. I felt it was similar enough to just comment here.)

[rsc]

/cc @FiloSottile

[FiloSottile]

I’m a little reluctant to add half a dozen SupportedX functions because we don’t really know if support will depend on client/server role or other logic in the future. For example the client-only key exchanges were only added recently.

Maybe we can simply embrace the fact that the lists can be a superset, and silently ignore unsupported entries in configs, documenting which ones are special and how. This should still let applications tell their users an approximate list of (potentially) available options.

This would mean adding the functions SupportedKeyExchanges, SupportedCiphers, SupportedMACs, and SupportedHostKeyAlgorithms (using Config field names), all returning []string. I’d be ok with that.

In any case, since all values are pretty self-describing strings and not opaque IDs, we don’t need to expose all the constants.

[rsc]

@jannfis and @0xjac, would the API in the previous comment work for you?

[0xjac]

@rsc Yes a function returning a list of algorithms supported by the package would be work for me.
I feel like declaring individual constants (one per supported algorithm) would be more idiomatic and follow the current design of the package, but that's personal preferences so I am fine with either.

There is one thing which is not clear to me with @FiloSottile's comment:

we don’t really know if support will depend on client/server role or other logic in the future.

From my understanding, this is not about which algorithms are supported by a client, server or other future logic, but the algorithms supported (i.e. implemented) by x/crypto/ssh.
We know exactly what those are. If a new cipher is added (or an old one removed) then the corresponding "supported" value should be amended accordingly. This should already be the case, whether the value is public or not.

[beoran]

Why not just have a function Supported that returns a struct {
KeyExchanges []string
Ciphers []string
MACs []string
HostKeyAlgorithms []string
}

Makes the API a lot simpler and it is easy to extend later with extra fields if needed.

[rsc]

@beoran one reason is that we typically want to return copies of these lists, so that they cannot be edited by clients, and (computing and) returning copies of lots of lists when the caller only cares about one is inefficient.

[beoran]

@rsc hmm, that's a good point.

In that case, the Supported function could in stead return a struct that has the methods KeyExchanges() []string, Ciphers() []string, MACs() []string, HostKeyAlgorithms() []string. Then you could write Supported().KeyExchanges(). Although it is a matter of taste if this is better than SupportedKeyExchanges(). I tend to like 'fluid" APIs but in this case, probably not.

[FiloSottile]

we don’t really know if support will depend on client/server role or other logic in the future.

From my understanding, this is not about which algorithms are supported by a client, server or other future logic, but the algorithms supported (i.e. implemented) by x/crypto/ssh.
We know exactly what those are. If a new cipher is added (or an old one removed) then the corresponding "supported" value should be amended accordingly. This should already be the case, whether the value is public or not.

The thing is that we already have some algorithm (the negotiated DH kex) that are only supported on the client side. They are technically implemented for the server too, but only for use in test files, and you're not (supposed to be?) able to enable them from an application.

[rsc]

This is starting to sound like maybe we should decline this proposal? Thoughts?

[rsc]

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

[rsc]

No change in consensus, so declined.
— rsc for the proposal review group