x/crypto/ssh: SignWithAlgorithm for agentKeyringSigner fails for keys carrying cert algorithm name

[jgold-stripe]

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

$ go version
go version go1.17.6 darwin/amd64

Does this issue reproduce with the latest release?

Yes (as of this writing, latest is golang.org/x/crypto@2cf3adece1227c48e1673f1c37d70357e1a6b9d3

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

go env Output

$ go env
GO111MODULE=""

GOARCH="amd64"

GOBIN=""

GOCACHE="/Users/jgold/Library/Caches/go-build"

GOENV="/Users/jgold/Library/Application Support/go/env"

GOEXE=""

GOEXPERIMENT=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="darwin"

GOINSECURE=""

GOMODCACHE="/Users/jgold/go/pkg/mod"

GONOPROXY=""

GONOSUMDB=""

GOOS="darwin"

GOPATH="/Users/jgold/go"

GOPRIVATE=""

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/private/var/tmp/_bazel/4be5ed2556242c72b02c613ef1c4c2fa/external/go_sdk"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/private/var/tmp/_bazel/4be5ed2556242c72b02c613ef1c4c2fa/external/go_sdk/pkg/tool/darwin_amd64"

GOVCS=""

GOVERSION="go1.17.6"

GCCGO="gccgo"

AR="ar"

CC="clang"

CXX="clang++"

CGO_ENABLED="1"

GOMOD="REDACT"

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/11/02nsw83j2_3gcdzdxs193t300000gn/T/go-build3360806026=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Ran the sample program (see bottom of issue summary) with a host that is healthy and reachable, and to which I can connect via OpenSSH from my terminal, as well as via older versions of the golang.org/x/crypto/ssh package.

What did you expect to see?

I expect to see "connection ok"

What did you see instead?

The program fails to authenticate via public key with

2022/05/13 16:59:54 ssh: handshake failed: agent: unsupported algorithm "ecdsa-sha2-nistp256"

If the test program is used with -keys-sign-only, the connection is established as expected.

Other Observations

I added some print statements inside the ssh package and ran a git bisect and it seems that the "breaking" change was 5d542ad81a58c89581d596f49d0ba5d435481bcf. I'm quoting the word "breaking" since I'm only claiming it breaks from my perspective -- perhaps the current behavior (fail) was always supposed to be the way things work. That being said, the same keys are allowed by OpenSSH, so I suspect that the change really is breaking here.

While most of the work in that change was around supporting rsa-sha2-256/512, it seems from the commit comment and this discussion that the change also made it so that keys returned by the agentKeyringSigner implement ssh.AlgorithmSigner. From the commit summary:

The Signers returned by ssh/agent (when backed by an agent client)
didn't actually implement AlgorithmSigner but ParameterizedSigner, an
interface defined in an earlier version of CL 123955.

This breaks in cases like ours, when an agent presents a key of type ecdsa-sha2-nistp256-cert-v01@openssh.com, since:

• The function pickSignatureAlgorithm receives a signer for which keyFormat is ecdsa-sha2-nistp256-cert-v01@openssh.com. In our client, this assigned value is returned from line 234.
• The auth function then invokes agentKeyringSigner.SignWithAlgorithm (via the variable as) on line 278, passing the result of underlyingAlgo as the algorithm parameter. In our case, underlyingAlgo("ecdsa-sha2-nistp256-cert-v01@openssh.com") is ecdsa-sha2-nistp256.
• The agentKeyringSigner.SignWithAlgorithm function fails the test on line 774 (since s.pub.Type() == "ecdsa-sha2-nistp256-cert-v01@openssh.com") and subsequently fails on line 785.
• The test program above shows that we can sidestep the issue entirely in our own code by explicitly wrapping the ssh.Signer instances returned by the agentKeyringSigner so that they implement only Sign, and not SignWithAlgorithm. This causes our keys to be wrapped in client_auth.go in values of type algorithmSignerWrapper, which then signs successfully and completes the connection.

Sample Program

Here is my demo I used to repro and hack around:

package main

// This is a demo program of an unexpected handshake issue in
// golang.org/x/crypto/ssh

import (
	"flag"
	"fmt"
	"io"
	"log"
	"net"
	"os"
	"os/user"

	"golang.org/x/crypto/ssh"
	"golang.org/x/crypto/ssh/agent"
)

var (
	address      = flag.String("address", "", "host:port to which to connect")
	keysSignOnly = flag.Bool("keys-sign-only", false, "If set, wrap agent keys so that they implement nothing more than ssh.Signer")
)

func main() {
	if err := runMain(); err != nil {
		log.Fatal(err)
	}
}

func runMain() error {
	flag.Parse()
	if *address == "" {
		return fmt.Errorf("need address")
	}
	agentClient, closeAgentClient, err := openAgentClient()
	if err != nil {
		return err
	}
	user, err := user.Current()
	if err != nil {
		return err
	}
	defer closeAgentClient()
	clientConfig := ssh.ClientConfig{
		User:            user.Username,
		HostKeyCallback: ssh.InsecureIgnoreHostKey(),
		BannerCallback:  ssh.BannerDisplayStderr(),
		Auth: []ssh.AuthMethod{
			ssh.PublicKeysCallback(publicKeysCallback(agentClient.Signers)),
		},
	}
	log.Printf("opening connection")
	sshClient, err := ssh.Dial("tcp", *address, &clientConfig)
	if err != nil {
		return err
	}
	defer sshClient.Close()
	log.Printf("connection ok")
	return nil
}

func publicKeysCallback(getSigners func() ([]ssh.Signer, error)) func() ([]ssh.Signer, error) {
	if !*keysSignOnly {
		return getSigners
	}
	return func() ([]ssh.Signer, error) {
		signers, err := getSigners()
		if err != nil {
			return nil, err
		}
		var wrapped []ssh.Signer
		for _, signer := range signers {
			wrapped = append(wrapped, signWrapper{signer: signer})
		}
		return wrapped, nil
	}
}

// signWrapper is an ssh.Signer but *not* an ssh.AlgorithmSigner, even if the
// underlying signer is.
type signWrapper struct {
	signer ssh.Signer
}

func (w signWrapper) PublicKey() ssh.PublicKey { return w.signer.PublicKey() }

func (w signWrapper) Sign(rand io.Reader, data []byte) (*ssh.Signature, error) {
	return w.signer.Sign(rand, data)
}

func openAgentClient() (agent.Agent, func(), error) {
	// sample code copied mainly from https://pkg.go.dev/golang.org/x/crypto@v0.0.0-20210817164053-32db794688a5/ssh/agent#Agent
	socket := os.Getenv("SSH_AUTH_SOCK")
	conn, err := net.Dial("unix", socket)
	if err != nil {
		return nil, nil, err
	}
	closeFunc := func() {
		if err := conn.Close(); err != nil {
			log.Printf("could not close agent socket: %s", err)
		}
	}
	return agent.NewClient(conn), closeFunc, nil
}

[jgold-stripe]

Actually confirming this is a duplicate of #52185 and is resolved with https://go-review.googlesource.com/c/crypto/+/404614/ . The merge appears to have snuck in Friday 5/13 in the afternoon, after I had drafted this issue, but I didn't think to re-check this morning :)

[mknyszek]

Sounds like we're good here then? :) Closing optimistically. Let me know if I should reopen.

[jgold-stripe]

We are -- thanks and sorry for the churn :)