[GitHub] [mina-sshd] FliegenKLATSCH commented on a diff in pull request #368: [SSHD-1259] Match key type in known_hosts lookup

["FliegenKLATSCH (via GitHub)" <gi...@apache.org>]

FliegenKLATSCH commented on code in PR #368:
URL: https://github.com/apache/mina-sshd/pull/368#discussion_r1186893876


##########
sshd-core/src/main/java/org/apache/sshd/client/keyverifier/KnownHostsServerKeyVerifier.java:
##########
@@ -267,36 +268,55 @@ protected PublicKeyEntryResolver getFallbackPublicKeyEntryResolver() {
     protected boolean acceptKnownHostEntries(
             ClientSession clientSession, SocketAddress remoteAddress, PublicKey serverKey,
             Collection<HostEntryPair> knownHosts) {
-        // TODO allow for several candidates and check if ANY of them matches the key and has 'revoked' marker
-        HostEntryPair match = findKnownHostEntry(clientSession, remoteAddress, knownHosts);
-        if (match == null) {
+
+        List<HostEntryPair> hostMatches = findKnownHostEntries(clientSession, remoteAddress, knownHosts);
+        if (hostMatches.isEmpty()) {
             return acceptUnknownHostKey(clientSession, remoteAddress, serverKey);
         }
 
-        KnownHostEntry entry = match.getHostEntry();
-        PublicKey expected = match.getServerKey();
-        if (KeyUtils.compareKeys(expected, serverKey)) {
-            return acceptKnownHostEntry(clientSession, remoteAddress, serverKey, entry);
+        String serverKeyType = KeyUtils.getKeyType(serverKey);
+
+        List<HostEntryPair> keyMatches = hostMatches.stream()
+                .filter(entry -> serverKeyType.equals(entry.getHostEntry().getKeyEntry().getKeyType()))
+                .filter(k -> KeyUtils.compareKeys(k.getServerKey(), serverKey))
+                .collect(Collectors.toList());
+
+        if (keyMatches.stream()
+                .anyMatch(k -> !acceptKnownHostEntry(clientSession, remoteAddress, serverKey, k.getHostEntry()))) {
+            return false;
         }
 
-        try {
-            if (!acceptModifiedServerKey(clientSession, remoteAddress, entry, expected, serverKey)) {
-                return false;
+        if (!keyMatches.isEmpty()) {
+            return true;
+        }
+
+        for (HostEntryPair match : hostMatches) {
+            KnownHostEntry entry = match.getHostEntry();
+            PublicKey expected = match.getServerKey();
+
+            try {
+                if (acceptModifiedServerKey(clientSession, remoteAddress, entry, expected, serverKey)) {
+                    updateModifiedServerKey(clientSession, remoteAddress, serverKey, knownHosts, match);
+                    return true;
+                }

Review Comment:
   My first thought was to change the signature and pass a list (and adding another list of all names under which the real key is known, similar to openssh, which shows that with the warning), but I assume that "big" change is not ideal - assuming a lot of implementations overwrite this function.
   In the end, it may be (too) special cases where someone relies on an specific existing entry, so you are probably right - but on the other end, if someone accepts it right away, it will also not ask again, so it doesn't really hurt?
   I am wondering if some implementations would do an external lookup with the old key..



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org