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-unsubscr...@mina.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
