x/crypto/ssh: fix host certificate principal evaluation to check for hostname only

SSH host certificates are expected to contain hostnames only,
not "host:port" format.

This change allows Go clients to connect to OpenSSH servers that
use host certificates.

Note, this change will break any clients that use ssh.NewClientConn()
with an `addr` that is not in `host:port` format (they will see a
"missing port in address" error).

Fixes bug 20273.

Change-Id: I5a306c6b7b419a737e1f0f9c5ca8c585e21a45a4
Reviewed-on: https://go-review.googlesource.com/43475
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

5 files changed, 122 insertions, 27 deletions

diff --git a/ssh/certs.go b/ssh/certs.go
index 2fc8af1..b1f0220 100644
--- a/ssh/certs.go
+++ b/ssh/certs.go
@@ -298,8 +298,17 @@ func (c *CertChecker) CheckHostKey(addr string, remote net.Addr, key PublicKey)
 	if cert.CertType != HostCert {
 		return fmt.Errorf("ssh: certificate presented as a host key has type %d", cert.CertType)
 	}
+	if !c.IsHostAuthority(cert.SignatureKey, addr) {
+		return fmt.Errorf("ssh: no authorities for hostname: %v", addr)
+	}
+
+	hostname, _, err := net.SplitHostPort(addr)
+	if err != nil {
+		return err
+	}
 
-	return c.CheckCert(addr, cert)
+	// Pass hostname only as principal for host certificates (consistent with OpenSSH)
+	return c.CheckCert(hostname, cert)
 }
 
 // Authenticate checks a user certificate. Authenticate can be used as
@@ -316,6 +325,9 @@ func (c *CertChecker) Authenticate(conn ConnMetadata, pubKey PublicKey) (*Permis
 	if cert.CertType != UserCert {
 		return nil, fmt.Errorf("ssh: cert has type %d", cert.CertType)
 	}
+	if !c.IsUserAuthority(cert.SignatureKey) {
+		return nil, fmt.Errorf("ssh: certificate signed by unrecognized authority")
+	}
 
 	if err := c.CheckCert(conn.User(), cert); err != nil {
 		return nil, err
@@ -364,16 +376,6 @@ func (c *CertChecker) CheckCert(principal string, cert *Certificate) error {
 		}
 	}
 
-	// if this is a host cert, principal is the remote hostname as passed
-	// to CheckHostCert.
-	if cert.CertType == HostCert && !c.IsHostAuthority(cert.SignatureKey, principal) {
-		return fmt.Errorf("ssh: no authorities for hostname: %v", principal)
-	}
-
-	if cert.CertType == UserCert && !c.IsUserAuthority(cert.SignatureKey) {
-		return fmt.Errorf("ssh: certificate signed by unrecognized authority")
-	}
-
 	clock := c.Clock
 	if clock == nil {
 		clock = time.Now
diff --git a/ssh/certs_test.go b/ssh/certs_test.go
index fba6310..0200531 100644
--- a/ssh/certs_test.go
+++ b/ssh/certs_test.go
@@ -168,8 +168,8 @@ func TestHostKeyCert(t *testing.T) {
 	cert.SignCert(rand.Reader, testSigners["ecdsa"])
 
 	checker := &CertChecker{
-		IsHostAuthority: func(p PublicKey, h string) bool {
-			return h == "hostname" && bytes.Equal(testPublicKeys["ecdsa"].Marshal(), p.Marshal())
+		IsHostAuthority: func(p PublicKey, addr string) bool {
+			return addr == "hostname:22" && bytes.Equal(testPublicKeys["ecdsa"].Marshal(), p.Marshal())
 		},
 	}
 
@@ -178,7 +178,14 @@ func TestHostKeyCert(t *testing.T) {
 		t.Errorf("NewCertSigner: %v", err)
 	}
 
-	for _, name := range []string{"hostname", "otherhost", "lasthost"} {
+	for _, test := range []struct {
+		addr    string
+		succeed bool
+	}{
+		{addr: "hostname:22", succeed: true},
+		{addr: "otherhost:22", succeed: false}, // The certificate is valid for 'otherhost' as hostname, but we only recognize the authority of the signer for the address 'hostname:22'
+		{addr: "lasthost:22", succeed: false},
+	} {
 		c1, c2, err := netPipe()
 		if err != nil {
 			t.Fatalf("netPipe: %v", err)
@@ -201,16 +208,15 @@ func TestHostKeyCert(t *testing.T) {
 			User:            "user",
 			HostKeyCallback: checker.CheckHostKey,
 		}
-		_, _, _, err = NewClientConn(c2, name, config)
+		_, _, _, err = NewClientConn(c2, test.addr, config)
 
-		succeed := name == "hostname"
-		if (err == nil) != succeed {
-			t.Fatalf("NewClientConn(%q): %v", name, err)
+		if (err == nil) != test.succeed {
+			t.Fatalf("NewClientConn(%q): %v", test.addr, err)
 		}
 
 		err = <-errc
-		if (err == nil) != succeed {
-			t.Fatalf("NewServerConn(%q): %v", name, err)
+		if (err == nil) != test.succeed {
+			t.Fatalf("NewServerConn(%q): %v", test.addr, err)
 		}
 	}
 }
diff --git a/ssh/test/cert_test.go b/ssh/test/cert_test.go
index bc83e4f..b231dd8 100644
--- a/ssh/test/cert_test.go
+++ b/ssh/test/cert_test.go
@@ -7,12 +7,14 @@
 package test
 
 import (
+	"bytes"
 	"crypto/rand"
 	"testing"
 
 	"golang.org/x/crypto/ssh"
 )
 
+// Test both logging in with a cert, and also that the certificate presented by an OpenSSH host can be validated correctly
 func TestCertLogin(t *testing.T) {
 	s := newServer(t)
 	defer s.Shutdown()
@@ -36,13 +38,40 @@ func TestCertLogin(t *testing.T) {
 	}
 
 	conf := &ssh.ClientConfig{
-		User:            username(),
-		HostKeyCallback: ssh.InsecureIgnoreHostKey(),
+		User: username(),
+		HostKeyCallback: (&ssh.CertChecker{
+			IsHostAuthority: func(pk ssh.PublicKey, addr string) bool {
+				return bytes.Equal(pk.Marshal(), testPublicKeys["ca"].Marshal())
+			},
+		}).CheckHostKey,
 	}
 	conf.Auth = append(conf.Auth, ssh.PublicKeys(certSigner))
-	client, err := s.TryDial(conf)
-	if err != nil {
-		t.Fatalf("TryDial: %v", err)
+
+	for _, test := range []struct {
+		addr    string
+		succeed bool
+	}{
+		{addr: "host.example.com:22", succeed: true},
+		{addr: "host.example.com:10000", succeed: true}, // non-standard port must be OK
+		{addr: "host.example.com", succeed: false},      // port must be specified
+		{addr: "host.ex4mple.com:22", succeed: false},   // wrong host
+	} {
+		client, err := s.TryDialWithAddr(conf, test.addr)
+
+		// Always close client if opened successfully
+		if err == nil {
+			client.Close()
+		}
+
+		// Now evaluate whether the test failed or passed
+		if test.succeed {
+			if err != nil {
+				t.Fatalf("TryDialWithAddr: %v", err)
+			}
+		} else {
+			if err == nil {
+				t.Fatalf("TryDialWithAddr, unexpected success")
+			}
+		}
 	}
-	client.Close()
 }
diff --git a/ssh/test/test_unix_test.go b/ssh/test/test_unix_test.go
index dd9ff40..e673536 100644
--- a/ssh/test/test_unix_test.go
+++ b/ssh/test/test_unix_test.go
@@ -30,6 +30,7 @@ Protocol 2
 HostKey {{.Dir}}/id_rsa
 HostKey {{.Dir}}/id_dsa
 HostKey {{.Dir}}/id_ecdsa
+HostCertificate {{.Dir}}/id_rsa-cert.pub
 Pidfile {{.Dir}}/sshd.pid
 #UsePrivilegeSeparation no
 KeyRegenerationInterval 3600
@@ -119,6 +120,11 @@ func clientConfig() *ssh.ClientConfig {
 			ssh.PublicKeys(testSigners["user"]),
 		},
 		HostKeyCallback: hostKeyDB().Check,
+		HostKeyAlgorithms: []string{ // by default, don't allow certs as this affects the hostKeyDB checker
+			ssh.KeyAlgoECDSA256, ssh.KeyAlgoECDSA384, ssh.KeyAlgoECDSA521,
+			ssh.KeyAlgoRSA, ssh.KeyAlgoDSA,
+			ssh.KeyAlgoED25519,
+		},
 	}
 	return config
 }
@@ -154,6 +160,12 @@ func unixConnection() (*net.UnixConn, *net.UnixConn, error) {
 }
 
 func (s *server) TryDial(config *ssh.ClientConfig) (*ssh.Client, error) {
+	return s.TryDialWithAddr(config, "")
+}
+
+// addr is the user specified host:port. While we don't actually dial it,
+// we need to know this for host key matching
+func (s *server) TryDialWithAddr(config *ssh.ClientConfig, addr string) (*ssh.Client, error) {
 	sshd, err := exec.LookPath("sshd")
 	if err != nil {
 		s.t.Skipf("skipping test: %v", err)
@@ -179,7 +191,7 @@ func (s *server) TryDial(config *ssh.ClientConfig) (*ssh.Client, error) {
 		s.t.Fatalf("s.cmd.Start: %v", err)
 	}
 	s.clientConn = c1
-	conn, chans, reqs, err := ssh.NewClientConn(c1, "", config)
+	conn, chans, reqs, err := ssh.NewClientConn(c1, addr, config)
 	if err != nil {
 		return nil, err
 	}
@@ -250,6 +262,11 @@ func newServer(t *testing.T) *server {
 		writeFile(filepath.Join(dir, filename+".pub"), ssh.MarshalAuthorizedKey(testPublicKeys[k]))
 	}
 
+	for k, v := range testdata.SSHCertificates {
+		filename := "id_" + k + "-cert.pub"
+		writeFile(filepath.Join(dir, filename), v)
+	}
+
 	var authkeys bytes.Buffer
 	for k, _ := range testdata.PEMBytes {
 		authkeys.Write(ssh.MarshalAuthorizedKey(testPublicKeys[k]))
diff --git a/ssh/testdata/keys.go b/ssh/testdata/keys.go
index 0be2e7e..3b3d26c 100644
--- a/ssh/testdata/keys.go
+++ b/ssh/testdata/keys.go
@@ -70,6 +70,47 @@ AwEHoUQDQgAEYcO2xNKiRUYOLEHM7VYAp57HNyKbOdYtHD83Z4hzNPVC4tM5mdGD
 PLL8IEwvYu2wq+lpXfGQnNMbzYf9gspG0w==
 -----END EC PRIVATE KEY-----
 `),
+	"ca": []byte(`-----BEGIN RSA PRIVATE KEY-----
+MIIEpAIBAAKCAQEAvg9dQ9IRG59lYJb+GESfKWTch4yBpr7Ydw1jkK6vvtrx9jLo
+5hkA8X6+ElRPRqTAZSlN5cBm6YCAcQIOsmXDUn6Oj1lVPQAoOjTBTvsjM3NjGhvv
+52kHTY0nsMsBeY9q5DTtlzmlYkVUq2a6Htgf2mNi01dIw5fJ7uTTo8EbNf7O0i3u
+c9a8P19HaZl5NKiWN4EIZkfB2WdXYRJCVBsGgQj3dE/GrEmH9QINq1A+GkNvK96u
+vZm8H1jjmuqzHplWa7lFeXcx8FTVTbVb/iJrZ2Lc/JvIPitKZWhqbR59yrGjpwEp
+Id7bo4WhO5L3OB0fSIJYvfu+o4WYnt4f3UzecwIDAQABAoIBABRD9yHgKErVuC2Q
+bA+SYZY8VvdtF/X7q4EmQFORDNRA7EPgMc03JU6awRGbQ8i4kHs46EFzPoXvWcKz
+AXYsO6N0Myc900Tp22A5d9NAHATEbPC/wdje7hRq1KyZONMJY9BphFv3nZbY5apR
+Dc90JBFZP5RhXjTc3n9GjvqLAKfFEKVmPRCvqxCOZunw6XR+SgIQLJo36nsIsbhW
+QUXIVaCI6cXMN8bRPm8EITdBNZu06Fpu4ZHm6VaxlXN9smERCDkgBSNXNWHKxmmA
+c3Glo2DByUr2/JFBOrLEe9fkYgr24KNCQkHVcSaFxEcZvTggr7StjKISVHlCNEaB
+7Q+kPoECgYEA3zE9FmvFGoQCU4g4Nl3dpQHs6kaAW8vJlrmq3xsireIuaJoa2HMe
+wYdIvgCnK9DIjyxd5OWnE4jXtAEYPsyGD32B5rSLQrRO96lgb3f4bESCLUb3Bsn/
+sdgeE3p1xZMA0B59htqCrvVgN9k8WxyevBxYl3/gSBm/p8OVH1RTW/ECgYEA2f9Z
+95OLj0KQHQtxQXf+I3VjhCw3LkLW39QZOXVI0QrCJfqqP7uxsJXH9NYX0l0GFTcR
+kRrlyoaSU1EGQosZh+n1MvplGBTkTSV47/bPsTzFpgK2NfEZuFm9RoWgltS+nYeH
+Y2k4mnAN3PhReCMwuprmJz8GRLsO3Cs2s2YylKMCgYEA2UX+uO/q7jgqZ5UJW+ue
+1H5+W0aMuFA3i7JtZEnvRaUVFqFGlwXin/WJ2+WY1++k/rPrJ+Rk9IBXtBUIvEGw
+FC5TIfsKQsJyyWgqx/jbbtJ2g4s8+W/1qfTAuqeRNOg5d2DnRDs90wJuS4//0JaY
+9HkHyVwkQyxFxhSA/AHEMJECgYA2MvyFR1O9bIk0D3I7GsA+xKLXa77Ua53MzIjw
+9i4CezBGDQpjCiFli/fI8am+jY5DnAtsDknvjoG24UAzLy5L0mk6IXMdB6SzYYut
+7ak5oahqW+Y9hxIj+XvLmtGQbphtxhJtLu35x75KoBpxSh6FZpmuTEccs31AVCYn
+eFM/DQKBgQDOPUwbLKqVi6ddFGgrV9MrWw+SWsDa43bPuyvYppMM3oqesvyaX1Dt
+qDvN7owaNxNM4OnfKcZr91z8YPVCFo4RbBif3DXRzjNNBlxEjHBtuMOikwvsmucN
+vIrbeEpjTiUMTEAr6PoTiVHjsfS8WAM6MDlF5M+2PNswDsBpa2yLgA==
+-----END RSA PRIVATE KEY-----
+`),
+}
+
+var SSHCertificates = map[string][]byte{
+	// The following are corresponding certificates for the private keys above, signed by the CA key
+	// Generated by the following commands:
+	//
+	// 1. Assumes "rsa" key above in file named "rsa", write out the public key to "rsa.pub":
+	//    ssh-keygen -y -f rsa > rsa.pu
+	//
+	// 2. Assumes "ca" key above in file named "ca", sign a cert for "rsa.pub":
+	//    ssh-keygen -s ca -h -n host.example.com -V +500w -I host.example.com-key rsa.pub
+	"rsa": []byte(`ssh-rsa-cert-v01@openssh.com AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgLjYqmmuTSEmjVhSfLQphBSTJMLwIZhRgmpn8FHKLiEIAAAADAQABAAAAgQC8A6FGHDiWCSREAXCq6yBfNVr0xCVG2CzvktFNRpue+RXrGs/2a6ySEJQb3IYquw7HlJgu6fg3WIWhOmHCjfpG0PrL4CRwbqQ2LaPPXhJErWYejcD8Di00cF3677+G10KMZk9RXbmHtuBFZT98wxg8j+ZsBMqGM1+7yrWUvynswQAAAAAAAAAAAAAAAgAAABRob3N0LmV4YW1wbGUuY29tLWtleQAAABQAAAAQaG9zdC5leGFtcGxlLmNvbQAAAABZHN8UAAAAAGsjIYUAAAAAAAAAAAAAAAAAAAEXAAAAB3NzaC1yc2EAAAADAQABAAABAQC+D11D0hEbn2Vglv4YRJ8pZNyHjIGmvth3DWOQrq++2vH2MujmGQDxfr4SVE9GpMBlKU3lwGbpgIBxAg6yZcNSfo6PWVU9ACg6NMFO+yMzc2MaG+/naQdNjSewywF5j2rkNO2XOaViRVSrZroe2B/aY2LTV0jDl8nu5NOjwRs1/s7SLe5z1rw/X0dpmXk0qJY3gQhmR8HZZ1dhEkJUGwaBCPd0T8asSYf1Ag2rUD4aQ28r3q69mbwfWOOa6rMemVZruUV5dzHwVNVNtVv+ImtnYtz8m8g+K0plaGptHn3KsaOnASkh3tujhaE7kvc4HR9Igli9+76jhZie3h/dTN5zAAABDwAAAAdzc2gtcnNhAAABALeDea+60H6xJGhktAyosHaSY7AYzLocaqd8hJQjEIDifBwzoTlnBmcK9CxGhKuaoJFThdCLdaevCeOSuquh8HTkf+2ebZZc/G5T+2thPvPqmcuEcmMosWo+SIjYhbP3S6KD49aLC1X0kz8IBQeauFvURhkZ5ZjhA1L4aQYt9NjL73nqOl8PplRui+Ov5w8b4ldul4zOvYAFrzfcP6wnnXk3c1Zzwwf5wynD5jakO8GpYKBuhM7Z4crzkKSQjU3hla7xqgfomC5Gz4XbR2TNjcQiRrJQ0UlKtX3X3ObRCEhuvG0Kzjklhv+Ddw6txrhKjMjiSi/Yyius/AE8TmC1p4U= host.example.com
+`),
 }
 
 var PEMEncryptedKeys = []struct {