go: x/crypto/ssh: publicKeyCallback cannot handshake using ssh-rsa keys signed using the ssh-rsa-sha2-256 algorithm

Hi, I know it’s annoying having someone nagging at an issue, but it’s been a while and having some kind of solution to this is a blocker for some people using Packer. Is there any way @hanwen or @FiloSottile could take a look at the patch I’ve submitted?

Would love to see this reviewed, it is a blocker for me with packer. Thanks!

Fundamentally this is an issue with the ssh.Signer type and/or its use in handshake.go.

The issue is that the type ssh.Signer:

type Signer interface {
	// PublicKey returns an associated PublicKey instance.
	PublicKey() PublicKey

	// Sign returns raw signature for the given data. This method
	// will apply the hash specified for the keytype to the data.
	Sign(rand io.Reader, data []byte) (*Signature, error)
}

Doesn’t provide a mechanism to say which type of algorithm is supported by this Signer - and its assumed incorrectly that that is the PublicKey type and although the AlgorithmSigner does provide a different signing method it’s still not saying which type of Signer it is:

type AlgorithmSigner interface {
	Signer

	// SignWithAlgorithm is like Signer.Sign, but allows specification of a
	// non-default signing algorithm. See the SigAlgo* constants in this
	// package for signature algorithms supported by this package. Callers may
	// pass an empty string for the algorithm in which case the AlgorithmSigner
	// will use its default algorithm.
	SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*Signature, error)
}

And so it is not wired into handshake.go.

The Signer interface would actually need to be something like:

type TypedSigner interface {
	Signer

	// Type returns the algorithm type for this signer
	Type() String
}

This would then allow backwards compatibility with the current system and allow handshake.go to be changed to:

		for _, k := range t.hostKeys {
			if typed, ok := k.(TypeSigner); ok {
				msg.ServerHostKeyAlgos = append(
					msg.ServerHostKeyAlgos, typed.Type())
				continue
			}

			msg.ServerHostKeyAlgos = append(
				msg.ServerHostKeyAlgos, k.PublicKey().Type())
		}

A couple of utility types and associated functions would need to be provided - the most simple being one that takes an AlgorithmSigner and creates a TypeSigner for a given algorithm.

A MultipleAlgorithmSigner could be created as below but it would be more complex to wire in:

type MultipleAlgorithmSigner interface {
	AlgorithmSigner

	// Algorithms returns the available algorithms 
	Algorithms() []string

	// SignWithAlgorithm is like Signer.Sign, but allows specification of a
	// non-default signing algorithm. See the SigAlgo* constants in this
	// package for signature algorithms supported by this package. Callers may
	// pass an empty string for the algorithm in which case the AlgorithmSigner
	// will use its default algorithm.
	SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*Signature, error)
}

My suspicion is that this would be better used as a way of an AlgorithmSigner advertising which TypeSigner they could become rather than being used directly in handshake.go. The idea being that it would be good if there was some programmatic way of listing all of the things could be supported. This could at first be used with some utility function to provide Signers for everything but later cut down to only support specific Signers.

At present there is no programmatic way of determining which algorithms are supported.

Edit: This is only part of the problem – we also need RFC8308 supported. (see #49269)

The linked PR only appears to implement handshake for the client.

Is it intended to do this for the server part?

Yeah I think that’s a better approach too. This patchset was a very timid “I don’t want to muck around too much inside anything or change any current behaviors without explicit interaction” approach, which I’d hoped would help get it adopted faster since it seemed pretty low risk to current users.

That said, I bet we could simply have the validateKey check determine whether the PublicKey.Type() is SigAlgoRSA (“ssh-rsa”), and if the validation fails try again with SigAlgoRSASHA2256 (“rsa-sha2-256”) instead. It would be a lot less code and prevent users from having to implement this.

Thanks! The test code in https://github.com/golang/go/issues/39885#issuecomment-835233610 now works after upgrading to: golang.org/x/crypto v0.0.0-20220314234724-5d542ad81a58.

This still affects golang 1.17 and we’ve recently noticed this on Gitea https://github.com/go-gitea/gitea/issues/17175

The underlying problem is that:

https://github.com/golang/crypto/blob/089bfa5675191fd96a44247682f76ebca03d7916/ssh/handshake.go#L459-L462

		for _, k := range t.hostKeys {
			msg.ServerHostKeyAlgos = append(
				msg.ServerHostKeyAlgos, k.PublicKey().Type())
		}

Makes the assumption that a public key type is the same as the key algorithm that that key can produce. There’s not really any way of defining what the algorithms are separate from these keys - and in fact once we start needing to completely deprecate the ssh-rsa method there doesn’t appear to be a way of defining which Algorithms we support separate from the keys.

In Gitea the workaround I’ve come up with is to simply wrap around the Signer and its PublicKey to shadow the Type() and switch the algorithm to the alternative as per the Type().

See https://github.com/go-gitea/gitea/pull/17281

OpenSSH 8.8 was released on 2021-09-26. RSA + SHA-1 has now been disabled by default (release notes):

This release disables RSA signatures using the SHA-1 hash algorithm by default. This change has been made as the SHA-1 hash algorithm is cryptographically broken, and it is possible to create chosen-prefix hash collisions for <USD$50K

SHA-256 and SHA-512 have been supported by SSH servers for many years now. The Go SSH package should default to using SHA-256 to avoid ongoing connectivity failures with well maintained servers.

Getting back up to speed, as RFC8332 and RFC8308 are much newer than my previous ssh work. It looks like the correct negotiation here is to implement server-sig-algs to get the supported algorithms before polling for an accepted key algorithm, to avoid auth penalties with certain server configurations. Agent usage often encounters these limits already (which is a primary use for the IdentityFile openssh option with an agent), and tripling the requests is likely to exacerbate the problem significantly.

The key types should however still be decoupled from the algorithm name, as the public key format and algorithm were only the same by coincidence and AlgorithmSigner alone can’t break that association.

If I understand correctly, I think in this case the ssh client should be responsible for the signing algorithm negotiation. Because the user may not know if the server configuration will accept the legacy format or not, I don’t think think we can place the burden on the user to determine the signing algorithm, plus it’s kind of cumbersome since changing the algorithm for most keys isn’t valid anyway. The AlgorithmSignerWithAlgo also has the drawback of not working with agent signers, which have been the most common in my experience.

It looks to me like we should be able to easily negotiate the signing algorithm while verifying the allowed public keys with the server, and pass the discovered algorithm along. We probably need to add SignWithAlgorithm to the agent signer, and have some special handling of Certificates to deal with the nist names they have wrapped around the public key types, but I think that might be a better approach to solve the issue transparently for users.