You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by "tomaswolf (via GitHub)" <gi...@apache.org> on 2023/05/06 18:25:22 UTC
[GitHub] [mina-sshd] tomaswolf commented on a diff in pull request #368: [SSHD-1259] Match key type in known_hosts lookup
tomaswolf commented on code in PR #368:
URL: https://github.com/apache/mina-sshd/pull/368#discussion_r1186727613
##########
sshd-core/src/main/java/org/apache/sshd/common/kex/extension/DefaultClientKexExtensionHandler.java:
##########
@@ -20,12 +20,7 @@
package org.apache.sshd.common.kex.extension;
import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Set;
-import java.util.TreeSet;
+import java.util.*;
Review Comment:
There should be no changes in this file.
##########
sshd-core/src/main/java/org/apache/sshd/client/keyverifier/KnownHostsServerKeyVerifier.java:
##########
@@ -491,43 +486,68 @@ protected boolean acceptKnownHostEntry(
return true;
}
- protected HostEntryPair findKnownHostEntry(
- ClientSession clientSession, SocketAddress remoteAddress, Collection<HostEntryPair> knownHosts) {
+ protected List<HostEntryPair> getAllEntriesForHost(ClientSession clientSession, SocketAddress remoteAddress, Collection<HostEntryPair> knownHosts) {
Review Comment:
Return `Collections.emptyList()` instead of `null`.
##########
sshd-core/src/main/java/org/apache/sshd/client/keyverifier/KnownHostsServerKeyVerifier.java:
##########
@@ -491,43 +486,68 @@ protected boolean acceptKnownHostEntry(
return true;
}
- protected HostEntryPair findKnownHostEntry(
- ClientSession clientSession, SocketAddress remoteAddress, Collection<HostEntryPair> knownHosts) {
+ protected List<HostEntryPair> getAllEntriesForHost(ClientSession clientSession, SocketAddress remoteAddress, Collection<HostEntryPair> knownHosts) {
if (GenericUtils.isEmpty(knownHosts)) {
return null;
}
Collection<SshdSocketAddress> candidates = resolveHostNetworkIdentities(clientSession, remoteAddress);
boolean debugEnabled = log.isDebugEnabled();
if (debugEnabled) {
- log.debug("findKnownHostEntry({})[{}] host network identities: {}",
+ log.debug("getAllEntriesForHost({})[{}] host network identities: {}",
clientSession, remoteAddress, candidates);
}
if (GenericUtils.isEmpty(candidates)) {
return null;
}
- for (HostEntryPair match : knownHosts) {
- KnownHostEntry entry = match.getHostEntry();
+ List<HostEntryPair> matches = new ArrayList<>();
+ for (HostEntryPair line : knownHosts) {
+ KnownHostEntry entry = line.getHostEntry();
for (SshdSocketAddress host : candidates) {
try {
if (entry.isHostMatch(host.getHostName(), host.getPort())) {
- if (debugEnabled) {
- log.debug("findKnownHostEntry({})[{}] matched host={} for entry={}",
- clientSession, remoteAddress, host, entry);
- }
- return match;
+ log.debug("getAllEntriesForHost({})[{}] matched host={} for entry={}",
+ clientSession, remoteAddress, host, entry);
+ matches.add(line);
}
} catch (RuntimeException | Error e) {
- warn("findKnownHostEntry({})[{}] failed ({}) to check host={} for entry={}: {}",
+ warn("getAllEntriesForHost({})[{}] failed ({}) to check host={} for entry={}: {}",
clientSession, remoteAddress, e.getClass().getSimpleName(),
host, entry.getConfigLine(), e.getMessage(), e);
}
}
}
- return null; // no match found
+ return matches;
+ }
+
+ protected HostEntryPair findKnownHostEntry(
+ ClientSession clientSession, SocketAddress remoteAddress, Collection<HostEntryPair> knownHosts, String serverKeyType) {
+
+ List<HostEntryPair> filtered = getAllEntriesForHost(clientSession, remoteAddress, knownHosts);
+ if (filtered == null) {
+ return null;
+ }
Review Comment:
This `if` is not needed if the method returns an empty list, not `null`.
##########
sshd-core/src/test/java/org/apache/sshd/client/keyverifier/KnownHostsServerKeyVerifierTest.java:
##########
@@ -62,16 +46,33 @@
import org.junit.experimental.categories.Category;
import org.junit.runners.MethodSorters;
import org.mockito.Mockito;
+import org.testcontainers.shaded.com.google.common.collect.HashMultimap;
+import org.testcontainers.shaded.com.google.common.collect.Iterables;
+import org.testcontainers.shaded.com.google.common.collect.Multimap;
+
+import java.io.IOException;
+import java.net.SocketAddress;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.nio.file.StandardCopyOption;
+import java.security.KeyPair;
+import java.security.PublicKey;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
Review Comment:
Please make sure you have the same ordering as before.
##########
sshd-core/src/test/java/org/apache/sshd/client/keyverifier/KnownHostsServerKeyVerifierTest.java:
##########
@@ -62,16 +46,33 @@
import org.junit.experimental.categories.Category;
import org.junit.runners.MethodSorters;
import org.mockito.Mockito;
+import org.testcontainers.shaded.com.google.common.collect.HashMultimap;
+import org.testcontainers.shaded.com.google.common.collect.Iterables;
+import org.testcontainers.shaded.com.google.common.collect.Multimap;
Review Comment:
Please avoid dependencies on shaded things from org.testcontainers. I'd even avoid the Google collections altogether; It's not hard to write these tests with Map<SsshdSocketAddress, List<KnownHostEntry>>, especially using the pattern `map.computeIfAbsent(key, k -> new ArrayList<>()).add(value)`.
##########
sshd-core/src/main/java/org/apache/sshd/client/keyverifier/KnownHostsServerKeyVerifier.java:
##########
@@ -60,6 +39,22 @@
import org.apache.sshd.common.util.io.ModifiableFileWatcher;
import org.apache.sshd.common.util.net.SshdSocketAddress;
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.io.Writer;
+import java.net.SocketAddress;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.LinkOption;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+import java.security.GeneralSecurityException;
+import java.security.PublicKey;
+import java.util.*;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
Review Comment:
Please don't change the order of import blocks. If necessary, adjust your IDE's settings.
##########
sshd-core/src/main/java/org/apache/sshd/client/keyverifier/KnownHostsServerKeyVerifier.java:
##########
@@ -60,6 +39,22 @@
import org.apache.sshd.common.util.io.ModifiableFileWatcher;
import org.apache.sshd.common.util.net.SshdSocketAddress;
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.io.Writer;
+import java.net.SocketAddress;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.LinkOption;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+import java.security.GeneralSecurityException;
+import java.security.PublicKey;
+import java.util.*;
Review Comment:
We don't use wildcard imports.
--
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