x/crypto/ssh: error from ssh.HostKeyCallback is not wrapped during handshake

[paxan]

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

$ go version
go version go1.20.5 darwin/arm64

Does this issue reproduce with the latest release?

Yes.

Root cause

https://github.com/golang/crypto/blob/2e82bdd17/ssh/client.go#L85

return nil, nil, nil, fmt.Errorf("ssh: handshake failed: %v", err)

It does not wrap the causing err using %w.

What did you do?

In my program, there is a mechanism that, for a given host, maintains []ssh.PublicKey with known host keys.

With this slice in hands, we proceed to call ssh.NewClientConn with:

ssh.ClientConfig{
    HostKeyCallback: validateHostKeys(known),
    ...
}

During ssh handshake if the host key doesn't match any keys from that slice, we get an error:

ssh: handshake failed: unknown host key: ecdsa-sha2-nistp256 AAAAElided...

The "unknown host key: ..." part comes from function validateHostKeys:

var ErrUnknownHostKey = errors.New("unknown host key")

func validateHostKeys(knownHostKeys []ssh.PublicKey) ssh.HostKeyCallback {
	return func(hostname string, remote net.Addr, hostKey ssh.PublicKey) error {
		if hostKey == nil {
			return fmt.Errorf("got a nil host key")
		}

		for _, knownHostKey := range knownHostKeys {
			if bytes.Equal(hostKey.Marshal(), knownHostKey.Marshal()) {
				return nil
			}
		}

		return fmt.Errorf("%w: %s",
			ErrUnknownHostKey, bytes.TrimSpace(ssh.MarshalAuthorizedKey(hostKey)))
	}
}

What did you expect to see?

In my host validation error handler, I am using:

errors.Is(err, ErrUnknownHostKey)

... and expect it to return true when such error indeed arises.

What did you see instead?

It returns false.

[gopherbot]

Change https://go.dev/cl/508876 mentions this issue: ssh: wrap errors from client handshake

[cherrymui]

cc @FiloSottile @golang/security

[drakkan]

Hello,

this is a good addition but I would like to see something more generic.

For example I'm exposing ErrNoCommonAlgo in my branch. Tailscale exposes other errors in their branch and so on.

I think we should expose an error struct with an error code or something similar so that we can let users easily identify the various types of errors. Obviously this struct should support errors.Is/As

[paxan]

Having a dedicated ssh error struct would be very valuable, agreed!
But it doesn’t clash with this use case: the package user is supplying a callback that returns a user-supplied error, so it is natural to expect that this error should be wrapped correctly, isn’t it?