support get new format fingerprint from HostKey

[eshizhan]

The PR make HostKey.getFingerPrint method support new format fingerprint, and keeping original method return old format.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[norrisjeremy]

Hi @eshizhan,

What exactly do you mean by "new format fingerprint"?

Thanks,
Jeremy

[eshizhan]

@norrisjeremy ref: http://www.openssh.com/txt/release-6.8

 Add FingerprintHash option to ssh(1) and sshd(8), and equivalent
 command-line flags to the other tools to control algorithm used
 for key fingerprints. The default changes from MD5 to SHA256 and
 format from hex to base64.

 Fingerprints now have the hash algorithm prepended. An example of
 the new format: SHA256:mVPwvezndPv/ARoIadVY98vAC0g+P/5633yTC4d/wXE
 Please note that visual host keys will also be different.

[eshizhan]

The old fingerprint format like: 9d:38:5b:83:a9:17:52:92:56:1a:5e:c4:d4:81:8e:0a:ca:51:a2:64:f1:74:20:11:2e:f8:8a:c3:a1:39:49:8f, the new fingerprint format like: SHA256:nThbg6kXUpJWGl7E1IGOCspRomTxdCARLviKw6E5SY8.
The Util.getFingerPrint in original code is support two format, just HostKey.getFingerPrint using old format before.

[norrisjeremy]

Hi @eshizhan,

I believe the only effect that your change has is to force the fingerprint has to prepend the MD5: or SHA256: string to the front of the string hash that it returns (depending upon which algorithm is configured): otherwise nothing else is different.

I'm not really sure that is thus worthwhile to introduce an unwieldy method overload (with a confusing boolean newFormat argument) to the public API.

Can you provide more details as to how this change will solve a functional issue you are having?

Thanks,
Jeremy

[norrisjeremy]

Hi @eshizhan,

What would you think of something like this instead, to just force HostKey.getFingerPrint() into always using the more modern format introduced in OpenSSH 6.8?

diff --git a/src/main/java/com/jcraft/jsch/HostKey.java b/src/main/java/com/jcraft/jsch/HostKey.java
index 0f9922b..112478a 100644
--- a/src/main/java/com/jcraft/jsch/HostKey.java
+++ b/src/main/java/com/jcraft/jsch/HostKey.java
@@ -128,7 +128,7 @@ public class HostKey {
         jsch.getInstanceLogger().log(Logger.ERROR, "getFingerPrint: " + e.getMessage(), e);
       }
     }
-    return Util.getFingerPrint(hash, key, false, true);
+    return Util.getFingerPrint(hash, key, true, false);
   }
 
   public String getComment() {
diff --git a/src/test/java/com/jcraft/jsch/KnownHostsTest.java b/src/test/java/com/jcraft/jsch/KnownHostsTest.java
index c74df31..fb7be14 100644
--- a/src/test/java/com/jcraft/jsch/KnownHostsTest.java
+++ b/src/test/java/com/jcraft/jsch/KnownHostsTest.java
@@ -454,9 +454,8 @@ class KnownHostsTest {
         "|1|AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=|mie6rcAf1aPGk6d+HxnkpvO4HaOAH/Y6YWegs+Xog/s=",
         hhk.getHost(), "host mismatch");
     assertEquals("", hhk.getMarker(), "marker mismatch");
-    assertEquals(
-        "9c:fb:7f:99:79:01:6d:46:68:87:39:15:4f:f5:cc:9d:71:7a:8b:5a:4a:c1:c7:4b:9c:20:a5:91:c2:6a:ff:5a",
-        hhk.getFingerPrint(jsch));
+    assertEquals("SHA256:nPt/mXkBbUZohzkVT/XMnXF6i1pKwcdLnCClkcJq/1o", hhk.getFingerPrint(jsch),
+        "fingerprint mismatch");
     assertEquals(null, hhk.getComment(), "comment mismatch");
     assertEquals("ICAgIHNzaC1yc2E=", hhk.getKey(), "key mismatch");
     assertEquals("ssh-rsa", hhk.getType(), "type mismatch");
@@ -1006,9 +1005,8 @@ class KnownHostsTest {
       String expectedType, String expectedComment) {
     assertEquals(expctedHost, hhk.getHost(), "host mismatch");
     assertEquals(expectedMarker, hhk.getMarker(), "marker mismatch");
-    assertEquals(
-        "1e:b5:70:92:65:6e:6a:f9:d6:7a:a9:43:00:40:a2:e7:c8:51:35:df:ee:60:19:b7:4b:18:1d:eb:46:48:28:4b",
-        hhk.getFingerPrint(jsch));
+    assertEquals("SHA256:HrVwkmVuavnWeqlDAECi58hRNd/uYBm3Sxgd60ZIKEs", hhk.getFingerPrint(jsch),
+        "fingerprint mismatch");
     assertEquals(expectedComment, hhk.getComment(), "comment mismatch");
     assertEquals("ICAgIHNzaC1kc2E=", hhk.getKey(), "key mismatch");
     assertEquals(expectedType, hhk.getType(), "type mismatch");
@@ -1022,9 +1020,8 @@ class KnownHostsTest {
     assertEquals("|1|AAECAwQFBgcICQoLDA0ODxAREhM=|/pE4peaossRYDRp6bEWa348eFLI=", hhk.getHost(),
         "host mismatch");
     assertEquals(expectedMarker, hhk.getMarker(), "marker mismatch");
-    assertEquals(
-        "1e:b5:70:92:65:6e:6a:f9:d6:7a:a9:43:00:40:a2:e7:c8:51:35:df:ee:60:19:b7:4b:18:1d:eb:46:48:28:4b",
-        hhk.getFingerPrint(jsch));
+    assertEquals("SHA256:HrVwkmVuavnWeqlDAECi58hRNd/uYBm3Sxgd60ZIKEs", hhk.getFingerPrint(jsch),
+        "fingerprint mismatch");
     assertEquals(expectedComment, hhk.getComment(), "comment mismatch");
     assertEquals("ICAgIHNzaC1kc2E=", hhk.getKey(), "key mismatch");
     assertEquals(expectedType, hhk.getType(), "type mismatch");
@@ -1054,9 +1051,8 @@ class KnownHostsTest {
     assertEquals(1, keys.length, "1 key expected");
     HostKey key = keys[0];
     assertEquals("some comment", key.getComment(), "comment mismatch");
-    assertEquals(
-        "9d:38:5b:83:a9:17:52:92:56:1a:5e:c4:d4:81:8e:0a:ca:51:a2:64:f1:74:20:11:2e:f8:8a:c3:a1:39:49:8f",
-        key.getFingerPrint(jsch), "fingerprint mismatch");
+    assertEquals("SHA256:nThbg6kXUpJWGl7E1IGOCspRomTxdCARLviKw6E5SY8", key.getFingerPrint(jsch),
+        "fingerprint mismatch");
     assertEquals(expectedHostResult, key.getHost(), "host mismatch");
     assertEquals(rsaKey, key.getKey(), "key mismatch");
     assertEquals(expectedMarker, key.getMarker(), "marker mismatch");

Thanks,
Jeremy

[eshizhan]

@norrisjeremy Thank you for the review.
I have considered two use cases: 1. Comparing whether the HostKey hash value is correct. 2. Logging to help with auditing and troubleshooting. These are also the functions of the fingerprint.

Initially, I had the same idea as you, to enforce the use of the new fingerprint format. But considering that this library is a well-known library in Java and is used by many other libraries and applications, I worry that forcing the transition to the new format will cause incompatibility. For example, someone might persist the HostKey hash to be compared in the old format.

This new format has actually been released for a long time. Switching to the new format is definitely a good choice. I chose the clumsy way of overloading, simply because of concerns about compatibility.

[norrisjeremy]

Hi @eshizhan,

Yes, considering how long ago OpenSSH switched to the new format (6.8 was released 9 years ago), I think I'd be more inclined to just change the format of the existing HostKey.getFingerPrint() method to match instead of introducing a method overload, but let me think on it some more.
@mwiede What's your opinion on this?

Thanks,
Jeremy

[DukeAstar]

I think the merged coud lead to regression if value is used as compare value.

Another option is to let user decide which format it prefers, while keeping new format as default.
This way users can keep access to old format if needed and it will be more flexible

See below my proposal.
@mwiede @norrisjeremy : do you think i can make a pull request ?

`
public String getFingerPrint(JSch jsch)
{
return getFingerPrint(jsch, true, false);
}

public String getFingerPrint(JSch jsch, boolean include_prefix, boolean force_hex)
{
	HASH hash = null;
	try
	{
		String _c = JSch.getConfig("FingerprintHash").toLowerCase();
		Class<? extends HASH> c = Class.forName(JSch.getConfig(_c)).asSubclass(HASH.class);
		hash = c.getDeclaredConstructor().newInstance();
	}
	catch (Exception e)
	{
		if (jsch.getInstanceLogger().isEnabled(Logger.ERROR))
		{
			jsch.getInstanceLogger().log(Logger.ERROR, "getFingerPrint: " + e.getMessage(), e);
		}
	}
	return Util.getFingerPrint(hash, key, include_prefix, force_hex);
}

`

[norrisjeremy]

Hi @DukeAstar,

No, I would not approve a pull request like that.
The new format was introduced by OpenSSH over 9 years ago, so I deliberately made this change to catch up with them.

Thanks,
Jeremy