tomaswolf commented on code in PR #368:
URL: https://github.com/apache/mina-sshd/pull/368#discussion_r1186889041
##########
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) {
Review Comment:
This loop again goes over revoked entries. Those should probably never be
fed to `acceptModifiedServerKey()`. Because if we have only revoked keys, it's
not a "modified server key" case, but an "unknown server key".
##########
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;
+ }
+ } catch (Throwable t) {
+ warn("acceptKnownHostEntries({})[{}] failed ({}) to accept
modified server key: {}",
+ clientSession, remoteAddress,
t.getClass().getSimpleName(), t.getMessage(), t);
}
- } catch (Throwable t) {
- warn("acceptKnownHostEntries({})[{}] failed ({}) to accept
modified server key: {}",
- clientSession, remoteAddress,
t.getClass().getSimpleName(), t.getMessage(), t);
- return false;
}
+ return false;
Review Comment:
If the above loop skips revoked entries (or `acceptModifiedServerKey`
skipped them and returned `false`), then we can get here if we have for
instance only revoked keys, all different from the actual server key. So here
we should then call `return acceptUnknownHostKey(...);`.
##########
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:
I presume you can return false here after the `if`. No need to ask several
times whether to accept a modified key -- and we know that none of the entries
match the server 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
