x/crypto/ssh: export supported algorithms #61537

drakkan · 2023-07-23T12:37:48Z

Currently KEXs, MACs and ciphers are private, some of them are defined as constants and others as simple strings, for example take a look at the supported ciphers list

// supportedCiphers lists ciphers we support but might not recommend.
var supportedCiphers = []string{
	"aes128-ctr", "aes192-ctr", "aes256-ctr",
	"aes128-gcm@openssh.com", gcm256CipherID,
	chacha20Poly1305ID,
	"arcfour256", "arcfour128", "arcfour",
	aes128cbcID,
	tripledescbcID,
}

I propose defining all supported algorithms as constants and exporting them for better discoverability.
We should also export the list of supported ciphers, KEXs, MACs, host key, public key algorithms and so on, so an application using the library can simply check if an algorithm is supported.

cc @golang/security

The text was updated successfully, but these errors were encountered:

FiloSottile · 2023-07-23T13:31:43Z

I definitely agree the supported algorithms should be somewhere on the godoc, either in the constants block or in the package description, but I am mildly opposed to exposing a list of "supported" algorithms as a slice or the like. First, it encourages enabling all supported algorithms by making it easy, and we have a lot of bad supported algorithms; second, "supported" is a complex state that can depend on when and how they are used. See #46232.

Can we see the list of proposed constants?

drakkan · 2023-07-23T14:19:28Z

Here is the list of proposed constants

// Supported ciphers. Ciphers based on AES CBC and RC4 are no longer considered
// secure.
const (
	CipherAlgoAES128GCM        = "aes128-gcm@openssh.com"
	CipherAlgoAES256GCM        = "aes256-gcm@openssh.com"
	CipherAlgoChacha20Poly1305 = "chacha20-poly1305@openssh.com"
	CipherAlgoAES128CTR        = "aes128-ctr"
	CipherAlgoAES192CTR        = "aes192-ctr"
	CipherAlgoAES256CTR        = "aes256-ctr"
	CipherAlgoAES128CBC        = "aes128-cbc"
	CipherAlgoTripleDESCBC     = "3des-cbc"
	CipherAlgoRC4              = "arcfour"
	CipherAlgoRC4128           = "arcfour128"
	CipherAlgoRC4256           = "arcfour256"
)

// Supported key exchanges algorithms. SHA-1 based KEXs are no longer considered
// secure.
const (
	KEXAlgoDH1SHA1    = "diffie-hellman-group1-sha1"
	KEXAlgoDH14SHA1   = "diffie-hellman-group14-sha1"
	KEXAlgoDH14SHA256 = "diffie-hellman-group14-sha256"
	KEXAlgoECDH256    = "ecdh-sha2-nistp256"
	KEXAlgoECDH384    = "ecdh-sha2-nistp384"
	KEXAlgoECDH521    = "ecdh-sha2-nistp521"
	// This KEX enables both curve25519-sha256 and curve25519-sha256@libssh.org
	KEXAlgoCurve25519SHA256 = "curve25519-sha256"
	// For the following kex only the client half contains a production ready
	// implementation. The server half only consists of a minimal implementation
	// to satisfy the automated tests and it is not accepted.
	KEXAlgoDHGEXSHA1   = "diffie-hellman-group-exchange-sha1"
	KEXAlgoDHGEXSHA256 = "diffie-hellman-group-exchange-sha256"
)

// Supported message authentication code (MAC) algorithms. SHA-1 based MACs are
// no longer considered secure.
const (
	MACAlgoHMACSHA256ETM = "hmac-sha2-256-etm@openssh.com"
	MACAlgoHMACSHA512ETM = "hmac-sha2-512-etm@openssh.com"
	MACAlgoHMACSHA256    = "hmac-sha2-256"
	MACAlgoHMACSHA512    = "hmac-sha2-512"
	MACAlgoHMACSHA1      = "hmac-sha1"
	MACAlgoHMACSHA196    = "hmac-sha1-96"
)

FiloSottile · 2023-07-25T15:37:47Z

Do we need both KEXAlgoCurve25519SHA256LibSSH and KEXAlgoCurve25519SHA256? Can we just have KEXAlgoCurve25519SHA256 instead and always negotiate both if enabled, assuming the wire protocol is the same or equivalent?

drakkan · 2023-07-26T10:07:25Z

Do we need both KEXAlgoCurve25519SHA256LibSSH and KEXAlgoCurve25519SHA256? Can we just have KEXAlgoCurve25519SHA256 instead and always negotiate both if enabled, assuming the wire protocol is the same or equivalent?

curve25519-sha256 is just the new name for curve25519-sha256@libssh.org, they already share the same implementation

https://github.com/golang/crypto/blob/d08e19beaccde615f2f1458b1b0df8fe75e20c8a/ssh/kex.go#L436

kexAlgoMap[kexAlgoCurve25519SHA256] = &curve25519sha256{}
kexAlgoMap[kexAlgoCurve25519SHA256LibSSH] = &curve25519sha256{}

we can expose only KEXAlgoCurve25519SHA256 and document that it will also enable the other variant.
I'll update the proposed constants. Thank you

rsc · 2023-07-26T18:26:20Z

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

hanwen · 2023-07-27T09:57:20Z

First, it encourages enabling all supported algorithms by making it easy, and we have a lot of bad supported algorithms

We could counter this by selecting a sensible default configuration for both client and server.

If library users decide to override this default, we should provide them information to let them make informed choices.

Maybe the TLS package is a good example: see https://pkg.go.dev/crypto/tls#CipherSuites and https://pkg.go.dev/crypto/tls#InsecureCipherSuites. We could do something like

type Algorithms {
  KEX []string
  Cipher []string
  MAC []string
  PublicKey []string
  PublicKeyAlgo []string
}

// Algorithms implemented, excluding ones with security problems.
func SupportedAlgorithms() Algorithms

// Algorithms implemented with security problems.
func InsecureAlgorithms() Algorithms

second, "supported" is a complex state that can depend on when and how they are used. See #46232.

if we have a clear distinction between secure/insecure algorithms, we could move the server implementation of 'client-only' algorithms out of the test files.

rsc · 2023-08-09T17:50:34Z

Any more thoughts on this? What is the proposal we've converged on?

drakkan · 2023-08-17T11:14:18Z

I think we agree on exporting all supported algorithms as constants but we are not yet sure if exporting or not a list of supported algorithms.

Internally we have a list of supported and preferred algorithms and use the preferred ones if no algorithm is explicitly configured.

The weird thing for me the first time I used x/crypto as a user was that I had to look at the code to figure out which algorithms are supported and preferred.
I would favour exporting also the lists of algorithms I would do only a minor change to the @hanwen proposal

type Algorithms {
  KEX []string
  Cipher []string
  MAC []string
  PublicKey []string
  PublicKeyAlgo []string
}

// SupportedAlgorithms returns algorithms we support but might not recommend.
func SupportedAlgorithms() Algorithms

// PreferredAlgorithms returns the default preference for algorithms.
func PreferredAlgorithms() Algorithms

We could also change the preferred algorithms so that sha-1 based algorithms are disabled by default

hanwen · 2023-08-17T12:51:47Z

re. Supported/Preferred vs. Supported/Insecure: following the pattern set by the TLS package will be less cognitive overhead in the long run, for folks that use both TLS and SSH package.

I am assuming that the way the TLS package does it hasn't been a problem so far.

drakkan · 2023-08-17T13:29:58Z

re. Supported/Preferred vs. Supported/Insecure: following the pattern set by the TLS package will be less cognitive overhead in the long run, for folks that use both TLS and SSH package.

I am assuming that the way the TLS package does it hasn't been a problem so far.

That's fine with me too, the only difference is that if an application wants to check if a user-supplied algorithm is acceptable (crypto/ssh ignores unknown algorithms), it has to look at two lists. But at the same time this approach makes more difficult to enable "insecure" algorithms, which is perhaps what we want to achieve

hanwen · 2023-08-17T14:01:37Z

nit: []string feels more straightforward, but does the ordering mean something? In practice, map[string]struct{} may be slightly more practical to use.

drakkan · 2023-08-17T14:40:12Z

nit: []string feels more straightforward, but does the ordering mean something? In practice, map[string]struct{} may be slightly more practical to use.

order matters. The first common algorithm between client and server is selected, see findCommon

hanwen · 2023-08-17T15:15:45Z

order matters. The first common algorithm between client and server is selected, see findCommon

order matters if you set it in ssh.Config, but maybe not for

an application using the library can simply check if an algorithm is supported.

drakkan · 2023-08-17T15:49:06Z

order matters. The first common algorithm between client and server is selected, see findCommon

order matters if you set it in ssh.Config, but maybe not for

an application using the library can simply check if an algorithm is supported.

ah ok, do you mean something like CipherSuites() []*CipherSuite in crypto/tls. This is useful if we want to add some additional info, for example if the cipher is supported only on the client side

hanwen · 2023-09-28T13:08:22Z

nit: []string feels more straightforward, but does the ordering mean something? In practice, map[string]struct{} may be slightly more practical to use.

we can probably address that in your proposal as follows

type Algorithms {
  KEX []string
  Cipher []string
  MAC []string
  PublicKey []string
  PublicKeyAlgo []string
}

func (a *Algorithms) IsSupportedCipher(alg string) bool
func (a *Algorithms) IsSupportedKEX(alg string) bool
...

so your proposal sounds fine.

hanwen · 2023-09-28T13:12:21Z

note, #58523 should be addressed together (ie in 2 reviews on top of each other) so we don't paint ourselves in an awkward corner with naming. other than that, I think we are in agreement on the general shape of things here.

drakkan · 2023-09-29T17:30:27Z

@hanwen thank you. I'll start working on this, so we can also see a test implementation before accepting the proposal.

Since #61244 has been accepted, it would be useful to also review CL 510775 so that Supported/InsecureAlgorithms can be implemented for public keys as well. thanks

gopherbot · 2023-10-01T13:26:35Z

Change https://go.dev/cl/531935 mentions this issue: ssh: export supported algorithms

drakkan · 2023-10-01T13:30:31Z

Hello,

I tried a first implementation. Here are the exported constants

// Supported ciphers. Ciphers based on AES CBC and RC4 are no longer considered
// secure.
const (
	CipherAlgoAES128GCM        = "aes128-gcm@openssh.com"
	CipherAlgoAES256GCM        = "aes256-gcm@openssh.com"
	CipherAlgoChacha20Poly1305 = "chacha20-poly1305@openssh.com"
	CipherAlgoAES128CTR        = "aes128-ctr"
	CipherAlgoAES192CTR        = "aes192-ctr"
	CipherAlgoAES256CTR        = "aes256-ctr"
	CipherAlgoAES128CBC        = "aes128-cbc"
	CipherAlgoTripleDESCBC     = "3des-cbc"
	CipherAlgoRC4              = "arcfour"
	CipherAlgoRC4128           = "arcfour128"
	CipherAlgoRC4256           = "arcfour256"
)

// Supported key exchanges algorithms. SHA-1 based KEXs are no longer considered
// secure.
const (
	KexAlgoDH1SHA1    = "diffie-hellman-group1-sha1"
	KexAlgoDH14SHA1   = "diffie-hellman-group14-sha1"
	KexAlgoDH14SHA256 = "diffie-hellman-group14-sha256"
	KexAlgoDH16SHA512 = "diffie-hellman-group16-sha512"
	KexAlgoECDH256    = "ecdh-sha2-nistp256"
	KexAlgoECDH384    = "ecdh-sha2-nistp384"
	KexAlgoECDH521    = "ecdh-sha2-nistp521"
	// This KEX enables both curve25519-sha256 and curve25519-sha256@libssh.org
	KexAlgoCurve25519SHA256       = "curve25519-sha256"
	// For the following KEXs only the client half contains a production ready
	// implementation. The server half only consists of a minimal implementation
	// to satisfy the automated tests and it is not accepted.
	KexAlgoDHGEXSHA1   = "diffie-hellman-group-exchange-sha1"
	KexAlgoDHGEXSHA256 = "diffie-hellman-group-exchange-sha256"
)

// Supported message authentication code (MAC) algorithms. SHA-1 based MACs are
// no longer considered secure.
const (
	MACAlgoHMACSHA256ETM = "hmac-sha2-256-etm@openssh.com"
	MACAlgoHMACSHA512ETM = "hmac-sha2-512-etm@openssh.com"
	MACAlgoHMACSHA256    = "hmac-sha2-256"
	MACAlgoHMACSHA512    = "hmac-sha2-512"
	MACAlgoHMACSHA1      = "hmac-sha1"
	MACAlgoHMACSHA196    = "hmac-sha1-96"
)

// Supported compression algorithms.
const (
	CompressionNone = "none"
)

and here the exported struct and methods

// Algorithms defines the algorithms for an SSH connection.
type Algorithms struct {
	KEXs         []string
	Ciphers      []string
	MACs         []string
	HostKeys     []string
	PublicKeys   []string
	Compressions []string
}

// SupportedAlgorithms returns algorithms currently implemented by this package,
// excluding those with security issues, which are returned by
// InsecureAlgorithms. The algorithms listed here are in preference order.
// Please note that the algorithms used by default may not match these ones for
// backward compatibility reasons.
func SupportedAlgorithms(isServer bool) Algorithms {
	algos := Algorithms{
		Ciphers:      supportedCiphers,
		MACs:         supportedMACs,
		HostKeys:     supportedHostKeyAlgos,
		PublicKeys:   supportedPubKeyAuthAlgos,
		Compressions: supportedCompressions,
	}
	if isServer {
		algos.KEXs = supportedServerKexAlgos
	} else {
		algos.KEXs = supportedClientKexAlgos
	}
	return algos
}

// InsecureAlgorithms returns algorithms currently implemented by this package
// and which have security issues.
func InsecureAlgorithms(isServer bool) Algorithms {
	kexs := []string{KexAlgoDH14SHA1, KexAlgoDH1SHA1}
	if !isServer {
		kexs = append(kexs, KexAlgoDHGEXSHA1)
	}
	return Algorithms{
		KEXs: kexs,
		Ciphers: []string{
			CipherAlgoAES128CBC,
			CipherAlgoTripleDESCBC,
			CipherAlgoRC4256, CipherAlgoRC4128, CipherAlgoRC4,
		},
		MACs:         []string{MACAlgoHMACSHA196, MACAlgoHMACSHA1},
		HostKeys:     []string{CertAlgoRSAv01, CertAlgoDSAv01, KeyAlgoRSA, KeyAlgoDSA},
		PublicKeys:   []string{KeyAlgoRSA, KeyAlgoDSA},
		Compressions: nil,
	}
}

rsc · 2023-11-01T17:44:36Z

The bool argument is odd; no one is going to remember at a call site what InsecureAlgorithms(true) does versus InsecureAlgorithms(false). Should we have four methods instead? (InsecureServerAlgorithms, ...)?

drakkan · 2023-11-01T17:53:15Z

The bool argument is odd; no one is going to remember at a call site what InsecureAlgorithms(true) does versus InsecureAlgorithms(false). Should we have four methods instead? (InsecureServerAlgorithms, ...)?

The bool argument is not required, we are also trying to add server-side DHGEX (see CL 532415).

I forgot to update this proposal, sorry. Here is the new proposal as implemented in CL 531935

// Implemented ciphers algorithms. Ciphers based on AES CBC and RC4 are no
// longer considered secure.
const (
	CipherAlgoAES128GCM        = "aes128-gcm@openssh.com"
	CipherAlgoAES256GCM        = "aes256-gcm@openssh.com"
	CipherAlgoChacha20Poly1305 = "chacha20-poly1305@openssh.com"
	CipherAlgoAES128CTR        = "aes128-ctr"
	CipherAlgoAES192CTR        = "aes192-ctr"
	CipherAlgoAES256CTR        = "aes256-ctr"
	CipherAlgoAES128CBC        = "aes128-cbc"
	CipherAlgoTripleDESCBC     = "3des-cbc"
	CipherAlgoRC4              = "arcfour"
	CipherAlgoRC4128           = "arcfour128"
	CipherAlgoRC4256           = "arcfour256"
)

// Implemented key exchanges algorithms. SHA-1 based KEXs are no longer
// considered secure.
const (
	KexAlgoDH1SHA1          = "diffie-hellman-group1-sha1"
	KexAlgoDH14SHA1         = "diffie-hellman-group14-sha1"
	KexAlgoDH14SHA256       = "diffie-hellman-group14-sha256"
	KexAlgoDH16SHA512       = "diffie-hellman-group16-sha512"
	KexAlgoECDH256          = "ecdh-sha2-nistp256"
	KexAlgoECDH384          = "ecdh-sha2-nistp384"
	KexAlgoECDH521          = "ecdh-sha2-nistp521"
	KexAlgoCurve25519SHA256 = "curve25519-sha256"
	KexAlgoDHGEXSHA1        = "diffie-hellman-group-exchange-sha1"
	KexAlgoDHGEXSHA256      = "diffie-hellman-group-exchange-sha256"

	// An alias for KexAlgoCurve25519SHA256. This kex ID will be added if
	// KexAlgoCurve25519SHA256 is requested for backward compatibility with
	// OpenSSH versions up to 7.2.
	kexAlgoCurve25519SHA256LibSSH = "curve25519-sha256@libssh.org"
)

// Implemented message authentication code (MAC) algorithms. SHA-1 based MACs are
// no longer considered secure.
const (
	MACAlgoHMACSHA256ETM = "hmac-sha2-256-etm@openssh.com"
	MACAlgoHMACSHA512ETM = "hmac-sha2-512-etm@openssh.com"
	MACAlgoHMACSHA256    = "hmac-sha2-256"
	MACAlgoHMACSHA512    = "hmac-sha2-512"
	MACAlgoHMACSHA1      = "hmac-sha1"
	MACAlgoHMACSHA196    = "hmac-sha1-96"
)

// Implemented compression algorithms.
const (
	CompressionNone = "none"
)

// Algorithms defines a set of algorithms that can be configured in the client
// or server config for negotiation during a handshake.
type Algorithms struct {
	KEXs         []string
	Ciphers      []string
	MACs         []string
	HostKeys     []string
	PublicKeys   []string
	Compressions []string
}

// SupportedAlgorithms returns algorithms currently implemented by this package,
// excluding those with security issues, which are returned by
// InsecureAlgorithms. The algorithms listed here are in preference order.
func SupportedAlgorithms() Algorithms {
	return Algorithms{
		Ciphers:      supportedCiphers,
		MACs:         supportedMACs,
		KEXs:         supportedKexAlgos,
		HostKeys:     supportedHostKeyAlgos,
		PublicKeys:   supportedPubKeyAuthAlgos,
		Compressions: supportedCompressions,
	}
}

// InsecureAlgorithms returns algorithms currently implemented by this package
// and which have security issues.
func InsecureAlgorithms() Algorithms {
	return Algorithms{
		KEXs: []string{KexAlgoDH14SHA1, KexAlgoDH1SHA1, KexAlgoDHGEXSHA1},
		Ciphers: []string{
			CipherAlgoAES128CBC,
			CipherAlgoTripleDESCBC,
			CipherAlgoRC4256, CipherAlgoRC4128, CipherAlgoRC4,
		},
		MACs:         []string{MACAlgoHMACSHA196, MACAlgoHMACSHA1},
		HostKeys:     []string{CertAlgoRSAv01, CertAlgoDSAv01, KeyAlgoRSA, KeyAlgoDSA},
		PublicKeys:   []string{KeyAlgoRSA, KeyAlgoDSA},
		Compressions: nil,
	}
}

rsc · 2023-11-02T14:33:40Z

There is a casing inconsistency that should be fixed: some names use "Kex" and others use "KEX". They should all use the same casing.

drakkan · 2023-11-02T15:00:20Z

There is a casing inconsistency that should be fixed: some names use "Kex" and others use "KEX". They should all use the same casing.

You're right, do we agree on "KEX"?

hanwen · 2023-11-02T16:06:00Z

https://pkg.go.dev/golang.org/x/crypto/ssh#Config says "KeyExchange", which is probably better than either KEX or Kex. It also avoids the dilemma of kexes vs. kexs

hanwen · 2023-11-02T16:13:45Z

in the review I also suggested Insecure as prefix for known insecure algorithms, eg. InsecureCipherAlgoRC4

drakkan · 2023-11-02T16:14:30Z

Thank you @hanwenI would prefer to keep KexAlgoCurve25519SHA256 etc. to avoid too long names. Here is the updated proposal

// Implemented ciphers algorithms. Ciphers based on AES CBC and RC4 are no
// longer considered secure.
const (
	CipherAlgoAES128GCM        = "aes128-gcm@openssh.com"
	CipherAlgoAES256GCM        = "aes256-gcm@openssh.com"
	CipherAlgoChacha20Poly1305 = "chacha20-poly1305@openssh.com"
	CipherAlgoAES128CTR        = "aes128-ctr"
	CipherAlgoAES192CTR        = "aes192-ctr"
	CipherAlgoAES256CTR        = "aes256-ctr"
	CipherAlgoAES128CBC        = "aes128-cbc"
	CipherAlgoTripleDESCBC     = "3des-cbc"
	CipherAlgoRC4              = "arcfour"
	CipherAlgoRC4128           = "arcfour128"
	CipherAlgoRC4256           = "arcfour256"
)

// Implemented key exchanges algorithms. SHA-1 based KEXs are no longer
// considered secure.
const (
	KexAlgoDH1SHA1          = "diffie-hellman-group1-sha1"
	KexAlgoDH14SHA1         = "diffie-hellman-group14-sha1"
	KexAlgoDH14SHA256       = "diffie-hellman-group14-sha256"
	KexAlgoDH16SHA512       = "diffie-hellman-group16-sha512"
	KexAlgoECDH256          = "ecdh-sha2-nistp256"
	KexAlgoECDH384          = "ecdh-sha2-nistp384"
	KexAlgoECDH521          = "ecdh-sha2-nistp521"
	KexAlgoCurve25519SHA256 = "curve25519-sha256"
	KexAlgoDHGEXSHA1        = "diffie-hellman-group-exchange-sha1"
	KexAlgoDHGEXSHA256      = "diffie-hellman-group-exchange-sha256"

	// An alias for KexAlgoCurve25519SHA256. This kex ID will be added if
	// KexAlgoCurve25519SHA256 is requested for backward compatibility with
	// OpenSSH versions up to 7.2.
	kexAlgoCurve25519SHA256LibSSH = "curve25519-sha256@libssh.org"
)

// Implemented message authentication code (MAC) algorithms. SHA-1 based MACs are
// no longer considered secure.
const (
	MACAlgoHMACSHA256ETM = "hmac-sha2-256-etm@openssh.com"
	MACAlgoHMACSHA512ETM = "hmac-sha2-512-etm@openssh.com"
	MACAlgoHMACSHA256    = "hmac-sha2-256"
	MACAlgoHMACSHA512    = "hmac-sha2-512"
	MACAlgoHMACSHA1      = "hmac-sha1"
	MACAlgoHMACSHA196    = "hmac-sha1-96"
)

// Implemented compression algorithms.
const (
	CompressionNone = "none"
)

type Algorithms struct {
	KeyExchanges []string
	Ciphers      []string
	MACs         []string
	HostKeys     []string
	PublicKeys   []string
	Compressions []string
}

// SupportedAlgorithms returns algorithms currently implemented by this package,
// excluding those with security issues, which are returned by
// InsecureAlgorithms. The algorithms listed here are in preference order.
func SupportedAlgorithms() Algorithms {
	return Algorithms{
		Ciphers:      supportedCiphers,
		MACs:         supportedMACs,
		KeyExchanges: supportedKexAlgos,
		HostKeys:     supportedHostKeyAlgos,
		PublicKeys:   supportedPubKeyAuthAlgos,
		Compressions: supportedCompressions,
	}
}

// InsecureAlgorithms returns algorithms currently implemented by this package
// and which have security issues.
func InsecureAlgorithms() Algorithms {
	return Algorithms{
		KeyExchanges: []string{KexAlgoDH14SHA1, KexAlgoDH1SHA1, KexAlgoDHGEXSHA1},
		Ciphers: []string{
			CipherAlgoAES128CBC,
			CipherAlgoTripleDESCBC,
			CipherAlgoRC4256, CipherAlgoRC4128, CipherAlgoRC4,
		},
		MACs:         []string{MACAlgoHMACSHA196, MACAlgoHMACSHA1},
		HostKeys:     []string{CertAlgoRSAv01, CertAlgoDSAv01, KeyAlgoRSA, KeyAlgoDSA},
		PublicKeys:   []string{KeyAlgoRSA, KeyAlgoDSA},
		Compressions: nil,
	}
}

hanwen · 2023-11-02T16:48:31Z

I was (more simply minded) thinking of breaking clients in the name of security (eg. #19767).

If we already have annotations for 'deprecated', can't we simply add those to the constants of insecure algorithms?

rsc · 2023-11-02T16:53:04Z

I don't think we should break people in the name of security anymore.
I am fine with just adding Deprecated tags and not including the Insecure-prefixed name.
The Insecure-prefixed name gives people a way to "silence" the deprecation.

drakkan · 2023-11-02T16:59:58Z

Thank you both. So we should also deprecate things like KeyAlgoRSA, KeyAlgoDSA etc. in this proposal/related CL and add InsecureKeyAlgoRSA, InsecureKeyAlgoDSA, etc.

rsc · 2023-11-02T17:16:39Z

Have all remaining concerns about this proposal been addressed?

The proposal details are in #61537 (comment).

drakkan · 2023-11-02T17:23:57Z

Have all remaining concerns about this proposal been addressed?

The proposal details are in #61537 (comment).

My ony remaining concern is if we have to deprecate the following algos

HostKeys:     []string{CertAlgoRSAv01, CertAlgoDSAv01, KeyAlgoRSA, KeyAlgoDSA},
PublicKeys:   []string{KeyAlgoRSA, KeyAlgoDSA},

and add InsecureKeyAlgoRSA as you proposed above so that func InsecureAlgorithms() Algorithms { returns only algorithms with the Insecure prefix

hanwen · 2023-11-02T18:33:08Z

If we are going to leave the constants without Insecure prefix alone, we don't have to add any right now, so that makes #61537 (comment) moot? Or do we also want to settle this in this proposal/change?

drakkan · 2023-11-02T18:43:27Z

If we are going to leave the constants without Insecure prefix alone, we don't have to add any right now,

yes

Or do we also want to settle this in this proposal/change?

Adding the Insecure prefix and deprecating insecure algorithms is a smart idea, maybe we can do it here too.

drakkan · 2023-11-02T19:03:03Z

We may do this, but I'm also fine without the Insecure variants

const (
	// Deprecated: this algorithm is insecure.
	CertAlgoRSAv01         = InsecureCertAlgoRSAv01
	InsecureCertAlgoRSAv01 = "ssh-rsa-cert-v01@openssh.com"
	// Deprecated: this algorithm is insecure.
	CertAlgoDSAv01         = InsecureCertAlgoDSAv01
	InsecureCertAlgoDSAv01 = "ssh-dss-cert-v01@openssh.com"
       ...
)

const (
	// Deprecated: this algorithm is insecure.
	KeyAlgoRSA         = InsecureKeyAlgoRSA
	InsecureKeyAlgoRSA = "ssh-rsa"
	// Deprecated: this algorithm is insecure.
	KeyAlgoDSA         = InsecureKeyAlgoDSA
        InsecureKeyAlgoDSA = "ssh-dss"
        ....
)

// Implemented ciphers algorithms.
const (
	CipherAES128GCM            = "aes128-gcm@openssh.com"
	CipherAES256GCM            = "aes256-gcm@openssh.com"
	CipherChacha20Poly1305     = "chacha20-poly1305@openssh.com"
	CipherAES128CTR            = "aes128-ctr"
	CipherAES192CTR            = "aes192-ctr"
	CipherAES256CTR            = "aes256-ctr"
	InsecureCipherAES128CBC    = "aes128-cbc"
	InsecureCipherTripleDESCBC = "3des-cbc"
	InsecureCipherRC4          = "arcfour"
	InsecureCipherRC4128       = "arcfour128"
	InsecureCipherRC4256       = "arcfour256"
)

// Implemented key exchanges algorithms.
const (
	InsecureKeyExchangeDH1SHA1   = "diffie-hellman-group1-sha1"
	InsecureKeyExchangeDH14SHA1  = "diffie-hellman-group14-sha1"
	KeyExchangeDH14SHA256        = "diffie-hellman-group14-sha256"
	KeyExchangeDH16SHA512        = "diffie-hellman-group16-sha512"
	KeyExchangeECDH256           = "ecdh-sha2-nistp256"
	KeyExchangeECDH384           = "ecdh-sha2-nistp384"
	KeyExchangeECDH521           = "ecdh-sha2-nistp521"
	KeyExchangeCurve25519SHA256  = "curve25519-sha256"
	InsecureKeyExchangeDHGEXSHA1 = "diffie-hellman-group-exchange-sha1"
	KeyExchangeDHGEXSHA256       = "diffie-hellman-group-exchange-sha256"

	// An alias for KeyExchangeCurve25519SHA256. This kex ID will be added if
	// KeyExchangeCurve25519SHA256 is requested for backward compatibility with
	// OpenSSH versions up to 7.2.
	keyExchangeCurve25519SHA256LibSSH = "curve25519-sha256@libssh.org"
)

// Implemented message authentication code (MAC) algorithms. SHA-1 based MACs are
// no longer considered secure.
const (
	HMACSHA256ETM      = "hmac-sha2-256-etm@openssh.com"
	HMACSHA512ETM      = "hmac-sha2-512-etm@openssh.com"
	HMACSHA256         = "hmac-sha2-256"
	HMACSHA512         = "hmac-sha2-512"
	InsecureHMACSHA1   = "hmac-sha1"
	InsecureHMACSHA196 = "hmac-sha1-96"
)

// Implemented compression algorithms.
const (
	CompressionNone = "none"
)

// Algorithms defines a set of algorithms that can be configured in the client
// or server config for negotiation during a handshake.
type Algorithms struct {
	KeyExchanges []string
	Ciphers      []string
	MACs         []string
	HostKeys     []string
	PublicKeys   []string
	Compressions []string
}

// SupportedAlgorithms returns algorithms currently implemented by this package,
// excluding those with security issues, which are returned by
// InsecureAlgorithms. The algorithms listed here are in preference order.
func SupportedAlgorithms() Algorithms {
	return Algorithms{
		Ciphers:      supportedCiphers,
		MACs:         supportedMACs,
		KeyExchanges: supportedKexAlgos,
		HostKeys:     supportedHostKeyAlgos,
		PublicKeys:   supportedPubKeyAuthAlgos,
		Compressions: supportedCompressions,
	}
}

// InsecureAlgorithms returns algorithms currently implemented by this package
// and which have security issues.
func InsecureAlgorithms() Algorithms {
	return Algorithms{
		KeyExchanges: []string{InsecureKeyExchangeDH14SHA1, InsecureKeyExchangeDH1SHA1,
			InsecureKeyExchangeDHGEXSHA1},
		Ciphers: []string{
			InsecureCipherAES128CBC,
			InsecureCipherTripleDESCBC,
			InsecureCipherRC4256, InsecureCipherRC4128, InsecureCipherRC4,
		},
		MACs:         []string{InsecureHMACSHA196, InsecureHMACSHA1},
		HostKeys:     []string{InsecureCertAlgoRSAv01, InsecureCertAlgoDSAv01, InsecureKeyAlgoRSA, InsecureKeyAlgoDSA},
		PublicKeys:   []string{InsecureKeyAlgoRSA, InsecureKeyAlgoDSA},
		Compressions: nil,
	}
}

hanwen · 2023-11-03T10:12:38Z

it makes no sense to introduce variables/constants that are already deprecated on introduction. If we do this, we don't need to have the constants without Insecure prefix.

drakkan · 2023-11-03T10:30:49Z

it makes no sense to introduce variables/constants that are already deprecated on introduction. If we do this, we don't need to have the constants without Insecure prefix.

New constants introduced for insecure algorithms are prefixed with Insecure (e.g. InsecureHMACSHA1). CertAlgoRSAv01, CertAlgoDSAv01, KeyAlgoRSA, KeyAlgoDSA were already exported before this patch so I proposed to deprecate them and add the non-deprecated variant with the Insecure prefix. Sorry if this is not clear in my previous comment

hanwen · 2023-11-03T12:50:21Z

Sorry if this is not clear in my previous comment

sorry on my side: I didn't read your comment in detail. What you propose SGTM.

FiloSottile · 2023-11-07T14:03:34Z

I would drop Algorithms.Compressions and CompressionNone.

The names of the Algorithms fields should probably match the field names where they'd be used. Let's call HostKeys HostKeyAlgorithms (after the ClientConfig field) and PublicKeys PublicKeyAuthAlgorithms (after the ServerConfig field in #61244). This also helps making it clear they are algorithms (so they include rsa-sha2-256) instead of key types.

Unfortunately, it looks like ClientConfig.HostKeyAlgorithms includes certificate algorithms, while ServerConfig.PublicKeyAuthAlgorithms includes only underlying algorithms. This is awful, but it seems to match the on-the-wire semantics, so not something we can change. Do we also make the two Algorithms fields have different semantics, or do we make an Algorithms field that can't be used as a Config field?

Finally, we can't deprecate KeyAlgoRSA because while it is insecure as an algorithm name it is not insecure as key type name. (This was probably my fault for merging SigType and KeyType, but it's done now.)

hanwen · 2023-11-07T15:20:54Z

I would drop Algorithms.Compressions and CompressionNone.

sgtm. We can add it later once we support more than "None"

rsc · 2023-11-08T18:28:55Z

@drakkan can you please post an updated API?

drakkan · 2023-11-08T19:02:52Z

Thanks to all, here is the updated API

const (
        ...
        // Deprecated: this algorithm is insecure.
        CertAlgoDSAv01         = InsecureCertAlgoDSAv01
        InsecureCertAlgoDSAv01 = "ssh-dss-cert-v01@openssh.com"
        ...
)

const (
        ...
        // Deprecated: this algorithm is insecure.
       KeyAlgoDSA         = InsecureKeyAlgoDSA
       InsecureKeyAlgoDSA = "ssh-dss"
       ....
)

// Implemented ciphers algorithms.
const (
	CipherAES128GCM            = "aes128-gcm@openssh.com"
	CipherAES256GCM            = "aes256-gcm@openssh.com"
	CipherChacha20Poly1305     = "chacha20-poly1305@openssh.com"
	CipherAES128CTR            = "aes128-ctr"
	CipherAES192CTR            = "aes192-ctr"
	CipherAES256CTR            = "aes256-ctr"
	InsecureCipherAES128CBC    = "aes128-cbc"
	InsecureCipherTripleDESCBC = "3des-cbc"
	InsecureCipherRC4          = "arcfour"
	InsecureCipherRC4128       = "arcfour128"
	InsecureCipherRC4256       = "arcfour256"
)

// Implemented key exchanges algorithms.
const (
	InsecureKeyExchangeDH1SHA1   = "diffie-hellman-group1-sha1"
	InsecureKeyExchangeDH14SHA1  = "diffie-hellman-group14-sha1"
	KeyExchangeDH14SHA256        = "diffie-hellman-group14-sha256"
	KeyExchangeDH16SHA512        = "diffie-hellman-group16-sha512"
	KeyExchangeECDH256           = "ecdh-sha2-nistp256"
	KeyExchangeECDH384           = "ecdh-sha2-nistp384"
	KeyExchangeECDH521           = "ecdh-sha2-nistp521"
	KeyExchangeCurve25519SHA256  = "curve25519-sha256"
	InsecureKeyExchangeDHGEXSHA1 = "diffie-hellman-group-exchange-sha1"
	KeyExchangeDHGEXSHA256       = "diffie-hellman-group-exchange-sha256"

	// An alias for KeyExchangeCurve25519SHA256. This kex ID will be added if
	// KeyExchangeCurve25519SHA256 is requested for backward compatibility with
	// OpenSSH versions up to 7.2.
	keyExchangeCurve25519SHA256LibSSH = "curve25519-sha256@libssh.org"
)

// Implemented message authentication code (MAC) algorithms. SHA-1 based MACs are
// no longer considered secure.
const (
	HMACSHA256ETM      = "hmac-sha2-256-etm@openssh.com"
	HMACSHA512ETM      = "hmac-sha2-512-etm@openssh.com"
	HMACSHA256         = "hmac-sha2-256"
	HMACSHA512         = "hmac-sha2-512"
	InsecureHMACSHA1   = "hmac-sha1"
	InsecureHMACSHA196 = "hmac-sha1-96"
)

// Algorithms defines a set of algorithms that can be configured in the client
// or server config for negotiation during a handshake.
type Algorithms struct {
	KeyExchanges            []string
	Ciphers                 []string
	MACs                    []string
	HostKeyAlgorithms       []string
	PublicKeyAuthAlgorithms []string
}

// SupportedAlgorithms returns algorithms currently implemented by this package,
// excluding those with security issues, which are returned by
// InsecureAlgorithms. The algorithms listed here are in preference order.
func SupportedAlgorithms() Algorithms {
	return Algorithms{
		Ciphers:                 supportedCiphers,
		MACs:                    supportedMACs,
		KeyExchanges:            supportedKexAlgos,
		HostKeyAlgorithms:       supportedHostKeyAlgos,
		PublicKeyAuthAlgorithms: supportedPubKeyAuthAlgos,
	}
}

// InsecureAlgorithms returns algorithms currently implemented by this package
// and which have security issues.
func InsecureAlgorithms() Algorithms {
	return Algorithms{
		KeyExchanges: []string{InsecureKeyExchangeDH14SHA1, InsecureKeyExchangeDH1SHA1,
			InsecureKeyExchangeDHGEXSHA1},
		Ciphers: []string{
			InsecureCipherAES128CBC,
			InsecureCipherTripleDESCBC,
			InsecureCipherRC4256, InsecureCipherRC4128, InsecureCipherRC4,
		},
		MACs:                    []string{InsecureHMACSHA196, InsecureHMACSHA1},
		HostKeyAlgorithms:       []string{KeyAlgoRSA, InsecureKeyAlgoDSA, CertAlgoRSAv01, InsecureCertAlgoDSAv01},
		PublicKeyAuthAlgorithms: []string{KeyAlgoRSA, InsecureKeyAlgoDSA},
	}
}

HostKeyAlgorithms will include certificate algorithms while PublicKeyAuthAlgorithms will include only underlying algorithms.

drakkan · 2023-11-16T16:57:36Z

Update proposal, removed the redundat Algorithms from struct fields

const (
        ...
        // Deprecated: this algorithm is insecure.
        CertAlgoDSAv01         = InsecureCertAlgoDSAv01
        InsecureCertAlgoDSAv01 = "ssh-dss-cert-v01@openssh.com"
        ...
)

const (
        ...
        // Deprecated: this algorithm is insecure.
       KeyAlgoDSA         = InsecureKeyAlgoDSA
       InsecureKeyAlgoDSA = "ssh-dss"
       ....
)

// Implemented ciphers algorithms.
const (
	CipherAES128GCM            = "aes128-gcm@openssh.com"
	CipherAES256GCM            = "aes256-gcm@openssh.com"
	CipherChacha20Poly1305     = "chacha20-poly1305@openssh.com"
	CipherAES128CTR            = "aes128-ctr"
	CipherAES192CTR            = "aes192-ctr"
	CipherAES256CTR            = "aes256-ctr"
	InsecureCipherAES128CBC    = "aes128-cbc"
	InsecureCipherTripleDESCBC = "3des-cbc"
	InsecureCipherRC4          = "arcfour"
	InsecureCipherRC4128       = "arcfour128"
	InsecureCipherRC4256       = "arcfour256"
)

// Implemented key exchanges algorithms.
const (
	InsecureKeyExchangeDH1SHA1   = "diffie-hellman-group1-sha1"
	InsecureKeyExchangeDH14SHA1  = "diffie-hellman-group14-sha1"
	KeyExchangeDH14SHA256        = "diffie-hellman-group14-sha256"
	KeyExchangeDH16SHA512        = "diffie-hellman-group16-sha512"
	KeyExchangeECDH256           = "ecdh-sha2-nistp256"
	KeyExchangeECDH384           = "ecdh-sha2-nistp384"
	KeyExchangeECDH521           = "ecdh-sha2-nistp521"
	KeyExchangeCurve25519SHA256  = "curve25519-sha256"
	InsecureKeyExchangeDHGEXSHA1 = "diffie-hellman-group-exchange-sha1"
	KeyExchangeDHGEXSHA256       = "diffie-hellman-group-exchange-sha256"

	// An alias for KeyExchangeCurve25519SHA256. This kex ID will be added if
	// KeyExchangeCurve25519SHA256 is requested for backward compatibility with
	// OpenSSH versions up to 7.2.
	keyExchangeCurve25519SHA256LibSSH = "curve25519-sha256@libssh.org"
)

// Implemented message authentication code (MAC) algorithms. SHA-1 based MACs are
// no longer considered secure.
const (
	HMACSHA256ETM      = "hmac-sha2-256-etm@openssh.com"
	HMACSHA512ETM      = "hmac-sha2-512-etm@openssh.com"
	HMACSHA256         = "hmac-sha2-256"
	HMACSHA512         = "hmac-sha2-512"
	InsecureHMACSHA1   = "hmac-sha1"
	InsecureHMACSHA196 = "hmac-sha1-96"
)

// Algorithms defines a set of algorithms that can be configured in the client
// or server config for negotiation during a handshake.
type Algorithms struct {
	KeyExchanges            []string
	Ciphers                 []string
	MACs                    []string
	HostKeys       []string
	PublicKeyAuths []string
}

// SupportedAlgorithms returns algorithms currently implemented by this package,
// excluding those with security issues, which are returned by
// InsecureAlgorithms. The algorithms listed here are in preference order.
func SupportedAlgorithms() Algorithms {
	return Algorithms{
		Ciphers:                 supportedCiphers,
		MACs:                    supportedMACs,
		KeyExchanges:            supportedKexAlgos,
		HostKeys:       supportedHostKeyAlgos,
		PublicKeyAuths: supportedPubKeyAuthAlgos,
	}
}

// InsecureAlgorithms returns algorithms currently implemented by this package
// and which have security issues.
func InsecureAlgorithms() Algorithms {
	return Algorithms{
		KeyExchanges: []string{InsecureKeyExchangeDH14SHA1, InsecureKeyExchangeDH1SHA1,
			InsecureKeyExchangeDHGEXSHA1},
		Ciphers: []string{
			InsecureCipherAES128CBC,
			InsecureCipherTripleDESCBC,
			InsecureCipherRC4256, InsecureCipherRC4128, InsecureCipherRC4,
		},
		MACs:                    []string{InsecureHMACSHA196, InsecureHMACSHA1},
		HostKeys:       []string{KeyAlgoRSA, InsecureKeyAlgoDSA, CertAlgoRSAv01, InsecureCertAlgoDSAv01},
		PublicKeyAuths: []string{KeyAlgoRSA, InsecureKeyAlgoDSA},
	}
}

rsc · 2023-12-09T18:15:18Z

Have all remaining concerns about this proposal been addressed?

The proposal details are in #61537 (comment).

rsc · 2023-12-14T04:43:42Z

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

The proposal details are in #61537 (comment).

rsc · 2023-12-21T04:13:56Z

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

The proposal details are in #61537 (comment).

FiloSottile · 2024-01-12T10:44:51Z

Sorry for catching this late, but KeyExchangeECDH256 and similar really need to be KeyExchangeECDHP256. "ECDH256" is not specific enough, there are many 256-bit curves for ECDH.

drakkan · 2024-01-13T10:26:56Z

Sorry for catching this late, but KeyExchangeECDH256 and similar really need to be KeyExchangeECDHP256. "ECDH256" is not specific enough, there are many 256-bit curves for ECDH.

CL updated, thanks

Fixes golang/go#61537 Change-Id: If3478121e3ae445391e3faeceeb889d75e9e3214

drakkan added the Proposal label Jul 23, 2023

gopherbot added this to the Proposal milestone Jul 23, 2023

ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jul 25, 2023

hanwen mentioned this issue Jul 27, 2023

x/crypto/ssh: expose negotiated algorithms #58523

Open

rsc added the Proposal-FinalCommentPeriod label Dec 14, 2023

rsc changed the title ~~proposal: x/crypto/ssh: export supported algorithms~~ x/crypto/ssh: export supported algorithms Dec 21, 2023

rsc modified the milestones: Proposal, Backlog Dec 21, 2023

rsc added Proposal-Accepted and removed Proposal-FinalCommentPeriod labels Dec 21, 2023

drakkan added a commit to drakkan/crypto that referenced this issue Feb 24, 2024

ssh: export supported algorithms

7236471

Fixes golang/go#61537 Change-Id: If3478121e3ae445391e3faeceeb889d75e9e3214

drakkan added a commit to drakkan/crypto that referenced this issue Mar 7, 2024

ssh: export supported algorithms

37b7d33

Fixes golang/go#61537 Change-Id: If3478121e3ae445391e3faeceeb889d75e9e3214

drakkan added a commit to drakkan/crypto that referenced this issue Apr 9, 2024

ssh: export supported algorithms

8010f07

Fixes golang/go#61537 Change-Id: If3478121e3ae445391e3faeceeb889d75e9e3214

x/crypto/ssh: export supported algorithms #61537

x/crypto/ssh: export supported algorithms #61537

Comments

drakkan commented Jul 23, 2023

FiloSottile commented Jul 23, 2023

drakkan commented Jul 23, 2023 • edited

FiloSottile commented Jul 25, 2023

drakkan commented Jul 26, 2023

rsc commented Jul 26, 2023

hanwen commented Jul 27, 2023

rsc commented Aug 9, 2023

drakkan commented Aug 17, 2023

hanwen commented Aug 17, 2023

drakkan commented Aug 17, 2023

hanwen commented Aug 17, 2023

drakkan commented Aug 17, 2023

hanwen commented Aug 17, 2023

drakkan commented Aug 17, 2023

hanwen commented Sep 28, 2023

hanwen commented Sep 28, 2023

drakkan commented Sep 29, 2023

gopherbot commented Oct 1, 2023

drakkan commented Oct 1, 2023

rsc commented Nov 1, 2023

drakkan commented Nov 1, 2023

rsc commented Nov 2, 2023

drakkan commented Nov 2, 2023

hanwen commented Nov 2, 2023

hanwen commented Nov 2, 2023

drakkan commented Nov 2, 2023 • edited

hanwen commented Nov 2, 2023

rsc commented Nov 2, 2023

drakkan commented Nov 2, 2023

rsc commented Nov 2, 2023

drakkan commented Nov 2, 2023

hanwen commented Nov 2, 2023

drakkan commented Nov 2, 2023

drakkan commented Nov 2, 2023 • edited

hanwen commented Nov 3, 2023

drakkan commented Nov 3, 2023 • edited

hanwen commented Nov 3, 2023

FiloSottile commented Nov 7, 2023 • edited

hanwen commented Nov 7, 2023

rsc commented Nov 8, 2023

drakkan commented Nov 8, 2023

drakkan commented Nov 16, 2023

rsc commented Dec 9, 2023

rsc commented Dec 14, 2023

rsc commented Dec 21, 2023

FiloSottile commented Jan 12, 2024

drakkan commented Jan 13, 2024

drakkan commented Jul 23, 2023 •

edited

drakkan commented Nov 2, 2023 •

edited

drakkan commented Nov 2, 2023 •

edited

drakkan commented Nov 3, 2023 •

edited

FiloSottile commented Nov 7, 2023 •

edited