ssh/knownhosts: compare only public key portions for revocation

Revocation matching compared full marshaled key blobs, so a @revoked
line containing a plain host key did not revoke a certificate
certifying that key: when the server presented such a certificate,
IsRevoked only matched the whole certificate blob or the CA key, and
verification succeeded even though the underlying host key was revoked.

OpenSSH's check_key_not_revoked compares keys with sshkey_equal_public,
which considers only the public portions and explicitly allows
comparisons between certificates and plain keys. Match that behavior by
normalizing both @revoked entries and lookups to the underlying public
key: for a certificate, the certified key rather than the certificate
blob. This also makes a @revoked line containing a certificate revoke
the plain key it certifies

Change-Id: I3029d59e68fb01f2340763e2eae25cac23c6193d
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/816841
Reviewed-by: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>

ssh/knownhosts/knownhosts.go

diff --git a/ssh/knownhosts/knownhosts.go b/ssh/knownhosts/knownhosts.go
index cf520ad..017c696 100644
--- a/ssh/knownhosts/knownhosts.go
+++ b/ssh/knownhosts/knownhosts.go
@@ -142,6 +142,15 @@
 	return bytes.Equal(a.Marshal(), b.Marshal())
 }
 
+// plainKeyBlob returns the serialized public portion of key: for a
+// certificate this is the certified key, otherwise the key itself.
+func plainKeyBlob(key ssh.PublicKey) string {
+	if cert, ok := key.(*ssh.Certificate); ok {
+		return string(cert.Key.Marshal())
+	}
+	return string(key.Marshal())
+}
+
 // IsHostAuthority can be used as a callback in ssh.CertChecker
 func (db *hostKeyDB) IsHostAuthority(remote ssh.PublicKey, address string) bool {
 	h, p, err := net.SplitHostPort(address)
@@ -160,10 +169,10 @@
 
 // IsRevoked can be used as a callback in ssh.CertChecker
 func (db *hostKeyDB) IsRevoked(key *ssh.Certificate) bool {
-	if _, ok := db.revoked[string(key.Marshal())]; ok {
+	if _, ok := db.revoked[plainKeyBlob(key)]; ok {
 		return true
 	}
-	if _, ok := db.revoked[string(key.SignatureKey.Marshal())]; ok {
+	if _, ok := db.revoked[plainKeyBlob(key.SignatureKey)]; ok {
 		return true
 	}
 	return false
@@ -228,7 +237,7 @@
 	}
 
 	if marker == markerRevoked {
-		db.revoked[string(key.Marshal())] = &KnownKey{
+		db.revoked[plainKeyBlob(key)] = &KnownKey{
 			Key:      key,
 			Filename: filename,
 			Line:     linenum,
@@ -341,7 +350,7 @@
 // check checks a key against the host database. This should not be
 // used for verifying certificates.
 func (db *hostKeyDB) check(address string, remote net.Addr, remoteKey ssh.PublicKey) error {
-	if revoked := db.revoked[string(remoteKey.Marshal())]; revoked != nil {
+	if revoked := db.revoked[plainKeyBlob(remoteKey)]; revoked != nil {
 		return &RevokedError{Revoked: *revoked}
 	}
 

ssh/knownhosts/knownhosts_test.go

diff --git a/ssh/knownhosts/knownhosts_test.go b/ssh/knownhosts/knownhosts_test.go
index 5e51447..947cf70 100644
--- a/ssh/knownhosts/knownhosts_test.go
+++ b/ssh/knownhosts/knownhosts_test.go
@@ -316,6 +316,68 @@
 	}
 }
 
+func TestRevokedCertifiedKey(t *testing.T) {
+	_, caPriv, err := ed25519.GenerateKey(rand.Reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	caSigner, err := ssh.NewSignerFromKey(caPriv)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	_, hostPriv, err := ed25519.GenerateKey(rand.Reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	hostKey, err := ssh.NewPublicKey(hostPriv.Public())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	signCert := func(serial uint64, principal string) *ssh.Certificate {
+		cert := &ssh.Certificate{
+			CertType:        ssh.HostCert,
+			Key:             hostKey,
+			Serial:          serial,
+			ValidBefore:     ssh.CertTimeInfinity,
+			ValidPrincipals: []string{principal},
+		}
+		if err := cert.SignCert(rand.Reader, caSigner); err != nil {
+			t.Fatal(err)
+		}
+		return cert
+	}
+	cert := signCert(1, "server.org")
+
+	// Revoking the host key revokes any certificate for it, even when
+	// the CA is trusted, matching OpenSSH's check_key_not_revoked.
+	db := testDB(t, "@revoked server.org "+serialize(hostKey)+"\n"+
+		"@cert-authority server.org "+serialize(caSigner.PublicKey())+"\n")
+	if !db.IsRevoked(cert) {
+		t.Error("IsRevoked returned false for certificate with revoked certified key")
+	}
+
+	// Conversely, a @revoked line containing a certificate revokes the
+	// plain key it certifies.
+	db = testDB(t, "@revoked server.org "+serialize(cert)+"\n")
+	if err := db.check("server.org:22", testAddr, hostKey); err == nil {
+		t.Error("no error for plain key revoked via certificate")
+	} else if _, ok := err.(*RevokedError); !ok {
+		t.Errorf("got %T (%v), want *RevokedError", err, err)
+	}
+
+	// Since only public portions are compared, revoking a certificate
+	// also revokes any sibling certificate certifying the same key,
+	// whatever its serial number and principals.
+	sibling := signCert(2, "other.org")
+	db = testDB(t, "@revoked * "+serialize(cert)+"\n"+
+		"@cert-authority * "+serialize(caSigner.PublicKey())+"\n")
+	if !db.IsRevoked(sibling) {
+		t.Error("IsRevoked returned false for sibling certificate of a revoked certificate")
+	}
+}
+
 const testHostname = "hostname"
 
 // generated with keygen -H -f