ssh/knownhosts: check more than one key

I believe this fixes https://github.com/golang/go/issues/36126 .

The problem was that it was keeping only the first known key of each
type found.
If you have a server advertising multiple keys of the same type,
you might get a missmatch key error.

Per sshd(8) man page, it should allow reapeatable hosts with
different host keys, although it don't specify anything about
hosts being from different types:

"It is permissible (but not recommended) to have several lines or
different host keys for the same names.  This will inevitably happen when
short forms of host names from different domains are put in the file.  It
is possible that the files contain conflicting information;
authentication is accepted if valid information can be found from either
file."

So, this changes knownhosts behavior to accept any of the keys for a
given host, regardless of type.

Fixes #36126

Change-Id: I3450ff954259a403f2471082d013a5f79def0e16
GitHub-Last-Rev: 361bd2bcd20348956aaf114ef159a5350397eaf4
GitHub-Pull-Request: golang/crypto#254
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/478535
Reviewed-by: Junyang Shao <shaojunyang@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Nicola Murino <nicola.murino@gmail.com>

2 files changed, 24 insertions, 36 deletions

diff --git a/ssh/knownhosts/knownhosts.go b/ssh/knownhosts/knownhosts.go
index 7376a8d..c022e41 100644
--- a/ssh/knownhosts/knownhosts.go
+++ b/ssh/knownhosts/knownhosts.go
@@ -302,8 +302,8 @@ func (k *KnownKey) String() string {
 // applications can offer an interactive prompt to the user.
 type KeyError struct {
 	// Want holds the accepted host keys. For each key algorithm,
-	// there can be one hostkey.  If Want is empty, the host is
-	// unknown. If Want is non-empty, there was a mismatch, which
+	// there can be multiple hostkeys.  If Want is empty, the host
+	// is unknown. If Want is non-empty, there was a mismatch, which
 	// can signify a MITM attack.
 	Want []KnownKey
 }
@@ -358,34 +358,20 @@ func (db *hostKeyDB) checkAddr(a addr, remoteKey ssh.PublicKey) error {
 	// is just a key for the IP address, but not for the
 	// hostname?
 
-	// Algorithm => key.
-	knownKeys := map[string]KnownKey{}
-	for _, l := range db.lines {
-		if l.match(a) {
-			typ := l.knownKey.Key.Type()
-			if _, ok := knownKeys[typ]; !ok {
-				knownKeys[typ] = l.knownKey
-			}
-		}
-	}
-
 	keyErr := &KeyError{}
-	for _, v := range knownKeys {
-		keyErr.Want = append(keyErr.Want, v)
-	}
 
-	// Unknown remote host.
-	if len(knownKeys) == 0 {
-		return keyErr
-	}
+	for _, l := range db.lines {
+		if !l.match(a) {
+			continue
+		}
 
-	// If the remote host starts using a different, unknown key type, we
-	// also interpret that as a mismatch.
-	if known, ok := knownKeys[remoteKey.Type()]; !ok || !keyEq(known.Key, remoteKey) {
-		return keyErr
+		keyErr.Want = append(keyErr.Want, l.knownKey)
+		if keyEq(l.knownKey.Key, remoteKey) {
+			return nil
+		}
 	}
 
-	return nil
+	return keyErr
 }
 
 // The Read function parses file contents.
diff --git a/ssh/knownhosts/knownhosts_test.go b/ssh/knownhosts/knownhosts_test.go
index 464dd59..156576a 100644
--- a/ssh/knownhosts/knownhosts_test.go
+++ b/ssh/knownhosts/knownhosts_test.go
@@ -201,17 +201,6 @@ func TestHostNamePrecedence(t *testing.T) {
 	}
 }
 
-func TestDBOrderingPrecedenceKeyType(t *testing.T) {
-	str := fmt.Sprintf("server.org,%s %s\nserver.org,%s %s", testAddr, edKeyStr, testAddr, alternateEdKeyStr)
-	db := testDB(t, str)
-
-	if err := db.check("server.org:22", testAddr, alternateEdKey); err == nil {
-		t.Errorf("check succeeded")
-	} else if _, ok := err.(*KeyError); !ok {
-		t.Errorf("got %T, want *KeyError", err)
-	}
-}
-
 func TestNegate(t *testing.T) {
 	str := fmt.Sprintf("%s,!server.org %s", testAddr, edKeyStr)
 	db := testDB(t, str)
@@ -354,3 +343,16 @@ func TestHashedHostkeyCheck(t *testing.T) {
 		t.Errorf("got error %v, want %v", got, want)
 	}
 }
+
+func TestIssue36126(t *testing.T) {
+	str := fmt.Sprintf("server.org,%s %s\nserver.org,%s %s", testAddr, edKeyStr, testAddr, alternateEdKeyStr)
+	db := testDB(t, str)
+
+	if err := db.check("server.org:22", testAddr, edKey); err != nil {
+		t.Errorf("should have passed the check, got %v", err)
+	}
+
+	if err := db.check("server.org:22", testAddr, alternateEdKey); err != nil {
+		t.Errorf("should have passed the check, got %v", err)
+	}
+}