Merge "[ssh known_hosts] Handle unknown keys better"

msohn · gerritforge-ltd · commit 3483bd76e9d5 · 2025-02-26T23:00:32.000Z
diff --git a/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/internal/transport/sshd/OpenSshServerKeyDatabaseTest.java b/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/internal/transport/sshd/OpenSshServerKeyDatabaseTest.java
@@ -478,6 +478,29 @@ public void testAddNewKey3() throws Exception {
 				new KnownHostsConfig(), null));
 	}
 
+	@Test
+	public void testUnknownKeyType() throws Exception {
+		List<String> initialKnownHosts = List.of(
+				"localhost,127.0.0.1 " + PublicKeyEntry.toString(ec384)
+						.replace("ecdsa", "foo"),
+				"some.other.com " + PublicKeyEntry.toString(ec384));
+		Files.write(knownHosts, initialKnownHosts);
+		TestCredentialsProvider ui = new TestCredentialsProvider(true, true);
+		assertTrue(database.accept("localhost", LOCAL, ec384,
+				new KnownHostsConfig(
+						ServerKeyDatabase.Configuration.StrictHostKeyChecking.ASK),
+				ui));
+		assertEquals(1, ui.invocations);
+		// The "modified key" dialog has two questions; whereas the "add new
+		// key" is just a simple question.
+		assertEquals(2, ui.questions);
+		List<String> expected = new ArrayList<>(initialKnownHosts);
+		expected.add("localhost,127.0.0.1 " + PublicKeyEntry.toString(ec384));
+		assertFile(knownHosts, expected);
+		assertTrue(database.accept("127.0.0.1:22", LOCAL, ec384,
+				new KnownHostsConfig(), null));
+	}
+
 	private void assertFile(Path path, List<String> lines) throws Exception {
 		assertEquals(lines, Files.readAllLines(path).stream()
 				.filter(s -> !s.isBlank()).toList());
@@ -489,6 +512,8 @@ private static class TestCredentialsProvider extends CredentialsProvider {
 
 		int invocations = 0;
 
+		int questions = 0;
+
 		TestCredentialsProvider(boolean accept, boolean store) {
 			values = new boolean[] { accept, store };
 		}
@@ -512,6 +537,7 @@ public boolean get(URIish uri, CredentialItem... items)
 				if (item instanceof CredentialItem.YesNoType) {
 					((CredentialItem.YesNoType) item)
 							.setValue(i < values.length && values[i++]);
+					questions++;
 				}
 			}
 			return true;
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/OpenSshServerKeyDatabase.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/OpenSshServerKeyDatabase.java
@@ -53,6 +53,7 @@
 import org.apache.sshd.common.config.keys.OpenSshCertificate;
 import org.apache.sshd.common.config.keys.PublicKeyEntry;
 import org.apache.sshd.common.config.keys.PublicKeyEntryResolver;
+import org.apache.sshd.common.config.keys.UnsupportedSshPublicKey;
 import org.apache.sshd.common.digest.BuiltinDigests;
 import org.apache.sshd.common.mac.Mac;
 import org.apache.sshd.common.util.io.ModifiableFileWatcher;
@@ -185,6 +186,9 @@ public List<PublicKey> lookup(@NonNull String connectAddress,
 		for (HostKeyFile file : filesToUse) {
 			for (HostEntryPair current : file.get()) {
 				KnownHostEntry entry = current.getHostEntry();
+				if (current.getServerKey() instanceof UnsupportedSshPublicKey) {
+					continue;
+				}
 				if (!isRevoked(entry) && !isCertificateAuthority(entry)) {
 					for (SshdSocketAddress host : candidates) {
 						if (entry.isHostMatch(host.getHostName(),
@@ -235,12 +239,20 @@ public boolean accept(@NonNull String connectAddress,
 					remoteAddress, modified[0].getServerKey(),
 					serverKey, path);
 			if (toDo == AskUser.ModifiedKeyHandling.ALLOW_AND_STORE) {
-				try {
-					updateModifiedServerKey(serverKey, modified[0], path);
-					knownHostsFiles.get(path).resetReloadAttributes();
-				} catch (IOException e) {
-					LOG.warn(format(SshdText.get().knownHostsCouldNotUpdate,
-							path));
+				if (modified[0]
+						.getServerKey() instanceof UnsupportedSshPublicKey) {
+					// Never update a line containing an unknown key type,
+					// always add.
+					addKeyToFile(filesToUse.get(0), candidates, serverKey, ask,
+							config);
+				} else {
+					try {
+						updateModifiedServerKey(serverKey, modified[0], path);
+						knownHostsFiles.get(path).resetReloadAttributes();
+					} catch (IOException e) {
+						LOG.warn(format(SshdText.get().knownHostsCouldNotUpdate,
+								path));
+					}
 				}
 			}
 			if (toDo == AskUser.ModifiedKeyHandling.DENY) {
@@ -253,19 +265,8 @@ public boolean accept(@NonNull String connectAddress,
 			return true;
 		} else if (ask.acceptUnknownKey(remoteAddress, serverKey)) {
 			if (!filesToUse.isEmpty()) {
-				HostKeyFile toUpdate = filesToUse.get(0);
-				path = toUpdate.getPath();
-				try {
-					if (Files.exists(path) || !askAboutNewFile
-							|| ask.createNewFile(path)) {
-						updateKnownHostsFile(candidates, serverKey, path,
-								config);
-						toUpdate.resetReloadAttributes();
-					}
-				} catch (Exception e) {
-					LOG.warn(format(SshdText.get().knownHostsCouldNotUpdate,
-							path), e);
-				}
+				addKeyToFile(filesToUse.get(0), candidates, serverKey, ask,
+						config);
 			}
 			return true;
 		}
@@ -379,6 +380,21 @@ private List<HostKeyFile> addUserHostKeyFiles(List<String> fileNames) {
 		return userFiles;
 	}
 
+	private void addKeyToFile(HostKeyFile file,
+			Collection<SshdSocketAddress> candidates, PublicKey serverKey,
+			AskUser ask, Configuration config) {
+		Path path = file.getPath();
+		try {
+			if (Files.exists(path) || !askAboutNewFile
+					|| ask.createNewFile(path)) {
+				updateKnownHostsFile(candidates, serverKey, path, config);
+				file.resetReloadAttributes();
+			}
+		} catch (Exception e) {
+			LOG.warn(format(SshdText.get().knownHostsCouldNotUpdate, path), e);
+		}
+	}
+
 	private void updateKnownHostsFile(Collection<SshdSocketAddress> candidates,
 			PublicKey serverKey, Path path, Configuration config)
 			throws Exception {
@@ -663,7 +679,7 @@ private List<HostEntryPair> reload(Path path) throws IOException {
 					}
 					try {
 						PublicKey serverKey = keyPart.resolvePublicKey(null,
-								PublicKeyEntryResolver.IGNORING);
+								PublicKeyEntryResolver.UNSUPPORTED);
 						if (serverKey == null) {
 							LOG.warn(format(
 									SshdText.get().knownHostsUnknownKeyType,
