tomaswolf commented on code in PR #368:
URL: https://github.com/apache/mina-sshd/pull/368#discussion_r1199814810
##########
sshd-core/src/main/java/org/apache/sshd/client/keyverifier/KnownHostsServerKeyVerifier.java:
##########
@@ -267,36 +268,63 @@ 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()))) {
Review Comment:
This determines whether any of the matching entries is revoked. It might be
clearer to use `.anyMatch(k -> "revoked".equals(k.getMarker()))` directly.
##########
sshd-core/src/main/java/org/apache/sshd/client/keyverifier/KnownHostsServerKeyVerifier.java:
##########
@@ -267,36 +268,63 @@ 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;
+ }
+
+ List<HostEntryPair> acceptedHostMatches = hostMatches.stream()
+ .filter(k -> acceptKnownHostEntry(clientSession,
remoteAddress, serverKey, k.getHostEntry()))
+ .collect(Collectors.toList());
Review Comment:
A `findAny` would be sufficient instead of collecting the list. If none is
found, call `acceptUnknownHostKey`, otherwise call `acceptModifiedServerKey()`
_once_ with the found entry.
The normal operation with `acceptModifiedServerKey()` is to ask the user.
And if the user says "no", it'll return false. There is no need for a loop here
going over all the other keys that we know and that the server didn't present.
This will only result in the user having to say "no" multiple times.
Also: we know here that `hostMatches` contains no matching key here. All
this does is filter out revoked keys. Please name this variable
`nonRevokedMatch`.
It might be even clearer to use `.filter(k ->
!"revoked".equals(k.getMarker()))` directly here.
--
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
