go.crypto/ssh: fix certificate parsing/marshaling.

The change to add the PublicKey interface accidentally caused certificate handling to expect an extra copy of the private key algorithm name in the binary representation. This change adapts a suitable parsing API and adds a test to ensure that cert handling isn't easily broken in the future.

R=agl, hanwen, jmpittman
CC=golang-dev
https://golang.org/cl/13272055

ssh/agent.go

@@ -100,7 +100,7 @@ func (ak *AgentKey) String() string {
 
 // Key returns an agent's public key as one of the supported key or certificate types.
 func (ak *AgentKey) Key() (PublicKey, error) {
-	if key, _, ok := parsePubKey(ak.blob); ok {
+	if key, _, ok := ParsePublicKey(ak.blob); ok {
 		return key, nil
 	}
 	return nil, errors.New("ssh: failed to parse key blob")

ssh/certs.go

@@ -60,6 +60,17 @@ var certAlgoNames = map[string]string{
 	KeyAlgoECDSA521: CertAlgoECDSA521v01,
 }
 
+// certToPrivAlgo returns the underlying algorithm for a certificate algorithm.
+// Panics if a non-certificate algorithm is passed.
+func certToPrivAlgo(algo string) string {
+	for privAlgo, pubAlgo := range certAlgoNames {
+		if pubAlgo == algo {
+			return privAlgo
+		}
+	}
+	panic("unknown cert algorithm")
+}
+
 func (c *OpenSSHCertV01) PublicKeyAlgo() string {
 	algo, ok := certAlgoNames[c.Key.PublicKeyAlgo()]
 	if !ok {
@@ -83,12 +94,14 @@ func parseOpenSSHCertV01(in []byte, algo string) (out *OpenSSHCertV01, rest []by
 		return
 	}
 
-	cert.Key, in, ok = ParsePublicKey(in)
+	privAlgo := certToPrivAlgo(algo)
+	cert.Key, in, ok = parsePubKey(in, privAlgo)
 	if !ok {
 		return
 	}
 
-	if cert.Key.PrivateKeyAlgo() != algo {
+	// We test PublicKeyAlgo to make sure we don't use some weird sub-cert.
+	if cert.Key.PublicKeyAlgo() != privAlgo {
 		ok = false
 		return
 	}
@@ -139,7 +152,7 @@ func parseOpenSSHCertV01(in []byte, algo string) (out *OpenSSHCertV01, rest []by
 	if !ok {
 		return
 	}
-	if cert.SignatureKey, _, ok = parsePubKey(sigKey); !ok {
+	if cert.SignatureKey, _, ok = ParsePublicKey(sigKey); !ok {
 		return
 	}
 
@@ -152,8 +165,7 @@ func parseOpenSSHCertV01(in []byte, algo string) (out *OpenSSHCertV01, rest []by
 }
 
 func (cert *OpenSSHCertV01) Marshal() []byte {
-	pubKey := MarshalPublicKey(cert.Key)
-
+	pubKey := cert.Key.Marshal()
 	sigKey := MarshalPublicKey(cert.SignatureKey)
 
 	length := stringLength(len(cert.Nonce))

ssh/common.go

@@ -201,7 +201,7 @@ func serializeSignature(name string, sig []byte) []byte {
 // generating an authorized_keys or host_keys file.
 func MarshalPublicKey(key PublicKey) []byte {
 	// See also RFC 4253 6.6.
-	algoname := key.PrivateKeyAlgo()
+	algoname := key.PublicKeyAlgo()
 	blob := key.Marshal()
 
 	length := stringLength(len(algoname))

ssh/keys.go

@@ -31,14 +31,10 @@ const (
 	KeyAlgoECDSA521 = "ecdsa-sha2-nistp521"
 )
 
-// parsePubKey parses a public key according to RFC 4253, section 6.6.
-func parsePubKey(in []byte) (pubKey PublicKey, rest []byte, ok bool) {
-	algo, in, ok := parseString(in)
-	if !ok {
-		return
-	}
-
-	switch string(algo) {
+// parsePubKey parses a public key of the given algorithm.
+// Use ParsePublicKey for keys with prepended algorithm.
+func parsePubKey(in []byte, algo string) (pubKey PublicKey, rest []byte, ok bool) {
+	switch algo {
 	case KeyAlgoRSA:
 		return parseRSA(in)
 	case KeyAlgoDSA:
@@ -46,7 +42,7 @@ func parsePubKey(in []byte) (pubKey PublicKey, rest []byte, ok bool) {
 	case KeyAlgoECDSA256, KeyAlgoECDSA384, KeyAlgoECDSA521:
 		return parseECDSA(in)
 	case CertAlgoRSAv01, CertAlgoDSAv01, CertAlgoECDSA256v01, CertAlgoECDSA384v01, CertAlgoECDSA521v01:
-		return parseOpenSSHCertV01(in, string(algo))
+		return parseOpenSSHCertV01(in, algo)
 	}
 	return nil, nil, false
 }
@@ -54,7 +50,7 @@ func parsePubKey(in []byte) (pubKey PublicKey, rest []byte, ok bool) {
 // parseAuthorizedKey parses a public key in OpenSSH authorized_keys format
 // (see sshd(8) manual page) once the options and key type fields have been
 // removed.
-func parseAuthorizedKey(in []byte) (out interface{}, comment string, ok bool) {
+func parseAuthorizedKey(in []byte) (out PublicKey, comment string, ok bool) {
 	in = bytes.TrimSpace(in)
 
 	i := bytes.IndexAny(in, " \t")
@@ -69,7 +65,7 @@ func parseAuthorizedKey(in []byte) (out interface{}, comment string, ok bool) {
 		return
 	}
 	key = key[:n]
-	out, _, ok = parsePubKey(key)
+	out, _, ok = ParsePublicKey(key)
 	if !ok {
 		return nil, "", false
 	}
@@ -79,7 +75,7 @@ func parseAuthorizedKey(in []byte) (out interface{}, comment string, ok bool) {
 
 // ParseAuthorizedKeys parses a public key from an authorized_keys
 // file used in OpenSSH according to the sshd(8) manual page.
-func ParseAuthorizedKey(in []byte) (out interface{}, comment string, options []string, rest []byte, ok bool) {
+func ParseAuthorizedKey(in []byte) (out PublicKey, comment string, options []string, rest []byte, ok bool) {
 	for len(in) > 0 {
 		end := bytes.IndexByte(in, '\n')
 		if end != -1 {
@@ -179,9 +175,14 @@ func ParseAuthorizedKey(in []byte) (out interface{}, comment string, options []s
 }
 
 // ParsePublicKey parses an SSH public key formatted for use in
-// the SSH wire protocol.
+// the SSH wire protocol according to RFC 4253, section 6.6.
 func ParsePublicKey(in []byte) (out PublicKey, rest []byte, ok bool) {
-	return parsePubKey(in)
+	algo, in, ok := parseString(in)
+	if !ok {
+		return
+	}
+
+	return parsePubKey(in, string(algo))
 }
 
 // MarshalAuthorizedKey returns a byte stream suitable for inclusion

ssh/keys_test.go

@@ -1,11 +1,13 @@
 package ssh
 
 import (
+	"bytes"
 	"crypto/dsa"
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rand"
 	"crypto/rsa"
+	"encoding/base64"
 	"reflect"
 	"strings"
 	"testing"
@@ -163,6 +165,33 @@ func TestParseDSA(t *testing.T) {
 	}
 }
 
+func TestParseCert(t *testing.T) {
+	// Cert generated by ssh-keygen 6.0p1 Debian-4.
+	// % ssh-keygen -s ca-key -I test user-key
+	b64data := "AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8="
+
+	data, err := base64.StdEncoding.DecodeString(b64data)
+	if err != nil {
+		t.Fatal("base64.StdEncoding.DecodeString: ", err)
+	}
+	key, rest, ok := ParsePublicKey(data)
+	if !ok {
+		t.Fatalf("could not parse certificate")
+	}
+	if len(rest) > 0 {
+		t.Errorf("rest: got %q, want empty", rest)
+	}
+	_, ok = key.(*OpenSSHCertV01)
+	if !ok {
+		t.Fatalf("got %#v, want *OpenSSHCertV01", key)
+	}
+
+	marshaled := MarshalPublicKey(key)
+	if !bytes.Equal(data, marshaled) {
+		t.Errorf("marshaled certificate does not match original: got %q, want %q", marshaled, data)
+	}
+}
+
 func init() {
 	raw, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	ecdsaKey, _ = NewSignerFromKey(raw)

ssh/server.go

@@ -406,7 +406,7 @@ userAuthLoop:
 					break
 				}
 				signedData := buildDataSignedForAuth(H, userAuthReq, algoBytes, pubKey)
-				key, _, ok := parsePubKey(pubKey)
+				key, _, ok := ParsePublicKey(pubKey)
 				if !ok {
 					return ParseError{msgUserAuthRequest}
 				}