Issue131 #134

kimmerin · 2022-03-21T23:29:48Z

Hi, as announced I've put KnownHosts under test and implemented some of the issues discussed in #131:

The SHA1 instance can't be null now. If the instantiation of the configured class fails, the JSCH SHA1-class is used as fallback. This doesn't solve things because the SSH-handshake will fail later on (but the exception there is expected and should help troubleshoot). I haven't implemented a check if the configured SHA1-class is actually SHA1 as requested by @norrisjeremy
I fixed a bug in remove: There is a loop over the elements where entries are taken from the vector using getElementAt(i). If an element is removed, i isn't decreased, so one entry to be checked is skipped that might have been removed otherwise.
All debug-output to STDOUT/ERR have been replaced by log entries to the logger. For this I've added a default method in Logger that allows to log the causing log entry as well.
Created a unit test that put the whole class under test (93,4 % coverage). In order to be able to do tests, some methods were made package visible and some methods were extracted to reduce complexity.

fixed bugs that where found in the process - fixed bug in remove(String, String, String) leading to only every second host key being removed - ensure that there is a SHA1-hash-instance to prevent NullPointerExceptions - replaced all occurrences of STDOUT/ERR outputs in case of exceptions with log entries - added a default method to Logger to allow passing the exception to be logged with the message - put class under test

particular JVM

norrisjeremy · 2022-04-26T16:07:10Z

src/main/java/com/jcraft/jsch/Logger.java

+      try (PrintWriter pw = new PrintWriter(sw, true)) {
+        cause.printStackTrace(pw);
+      }
+      message += "\r\n" + sw.toString();


I think we should do this instead:

message += System.lineSeparator() + sw.toString();

That way it uses the correct line separator for whatever platform is in use.

I also wonder if we should override this default method in the concrete implementations I added to utilize their specialized methods for logging exceptions?

I can change the line separator to be platform dependent.

I also wonder if we should override this default method in the concrete implementations I added to utilize their specialized methods for logging exceptions?

I think this makes sense

I've changed the line break and added specific log-methods to framework-specific implementations. I've also added test cases for all the logging frameworks that allow logger configuration in a programmatic way, i.e. there is no test for Log4j2.

added log-framework-specific implementations of log(int, String, Throwable) to pass the cause to the framework in a fitting way added tests for all log frameworks that can be configured in a programmatic way.

…e131

norrisjeremy · 2022-04-27T19:28:57Z

It looks like there is a test failure with LoggerTest.testLogWithCause?

norrisjeremy

Can you also add specialization in the JplLogger class too (under src/main/java9)?

norrisjeremy · 2022-04-27T19:31:16Z

src/main/java/com/jcraft/jsch/Slf4jLogger.java

-      default:
-        return logger.isTraceEnabled();
-    }
+    return logger.isEnabledForLevel(getLevel(level));


I think we need to revert this part: I believe the isEnabledForLevel() method is only available with SLFJ4 2.x, but we should keep compatibility with the SLF4J 1.x API as well.

I assumed that slf4j 2.0 is used because it's in the pom.xml

I assumed that slf4j 2.0 is used because it's in the pom.xml

I switched the dependency to 2.x because the 1.x versions aren't modularized, so they caused warnings for module-info.java for JPMS support.
But I made sure that the Slf4jLogger was fully compatible with 1.x, allowing users to decide which version they want to use.

If backward compatibility to slf4j 1 is intended, the pom should be changed to use that library to make sure that the compiler always compiles against the correct methods.

If backward compatibility to slf4j 1 is intended, the pom should be changed to use that library to make sure that the compiler always compiles against the correct methods.

Except the project doesn't compile module-info.java correctly if it is built with the 1.x version of the library, and then we lose JPMS support.

It doesn't seem that difficult to just leave the code as it is with the existing switch statement (it works with both SLFJ4 1.x & 2.x), and the way I structured the project, users can make use of either version in their own projects.

Except the project doesn't compile module-info.java correctly if it is built with the 1.x version of the library, and then we lose JPMS support.

OK. I've just been bitten once already when compiling against a newer API while executing with an older one. It's been a while but a lecture for life ;-) In the good old times when a kilobyte weighted a kilogram, java.lang.BigDecimal got a new constructor BigDecimal(long). While I don't expect something like that took place in Slf4J, it's still dangerous to do so.

Yep, I realize that and I wouldn't have changed the pom.xml to the 2.x version, except that I was also trying to make JSch fully modularized & JPMS compatible, and this was the only way I found to accomplish that.
I think as long as we are careful about what SLF4J methods we utilize, we can accomplish everything:

SLF4j 1.x compability.

SLF4j 2.x compability.

Java 9+ JPMS support.

Hm, I'm not that Maven-savvy but isn't it possible to define a test-only dependency. Can we use slf4j 2 for compiling and slf4j 1 for testing? With the test cases I've added any incompatibility (and be it compiler-related) should show up during test that way.

I think the only way to do that would be to completely restructure as a multi-module project, with the library as one module and the tests as a different module, but that would be alot of work.

I've changed the implementation to only use the API of slf4j 1

actually is system dependent)

norrisjeremy · 2022-04-27T19:40:08Z

src/main/java/com/jcraft/jsch/JulLogger.java


 public class JulLogger implements com.jcraft.jsch.Logger {

-  private static final java.util.logging.Logger logger = java.util.logging.Logger.getLogger(JSch.class.getName());
+  private static final java.util.logging.Logger stlogger = java.util.logging.Logger.getLogger(JSch.class.getName());
+  private java.util.logging.Logger logger;


I think the can be a private final field?

norrisjeremy · 2022-04-27T19:40:29Z

src/main/java/com/jcraft/jsch/Slf4jLogger.java


 public class Slf4jLogger implements com.jcraft.jsch.Logger {

-  private static final org.slf4j.Logger logger = LoggerFactory.getLogger(JSch.class);
+  private static final Logger stlogger = LoggerFactory.getLogger(JSch.class);
+  private Logger logger;


I think the can be a private final field?

use slf4j 1.x API for logging to keep backward compatibility

kimmerin · 2022-04-27T21:41:54Z

Can you also add specialization in the JplLogger class too (under src/main/java9)?

done

norrisjeremy · 2022-05-06T12:35:24Z

src/main/java/com/jcraft/jsch/Slf4jLogger.java

+        case com.jcraft.jsch.Logger.INFO:
+            return logger.isInfoEnabled();
+        case com.jcraft.jsch.Logger.WARN:
+            return logger.isWarnEnabled();


The formatting for this no longer seems to follow Google Java Style Guide's guidelines in section 4.8.4.
Can we return the formatting back, since I created this file following their style guide?

Also, I had purposely ordered the levels in the switch statement in order by severity (lowest to highest), can we also restore that order?

norrisjeremy · 2022-05-06T12:47:37Z

src/main/java/com/jcraft/jsch/Slf4jLogger.java

    }
+    return logger.isTraceEnabled();


I think we should put this back as a default case in the switch statement above, to match the switch statement in the new specialization for Throwable's below where it is in its default case.

norrisjeremy · 2022-05-06T12:49:00Z

src/main/java/com/jcraft/jsch/Slf4jLogger.java

        break;
      default:
-        logger.trace(message);
+        logger.trace(message, cause);


Can we order the cases in this switch statement to match original, which was by severity?

norrisjeremy · 2022-05-06T13:08:14Z

src/main/java/com/jcraft/jsch/KnownHosts.java

+      if (fos != null) {
+        fos.close();
+      }
+    }


I wonder if we should change this to a try-with-resources statement?

norrisjeremy · 2022-05-06T13:11:18Z

src/main/java/com/jcraft/jsch/KnownHosts.java

-  private static final byte[] cr=Util.str2byte("\n");
+  private static final byte[] lf=Util.str2byte("\n");
+
+  @SuppressWarnings("unused") // to keep the method's signature without warning


Curious as to why this is needed since the out argument appears to be used in the call to dumpHostKey()?

I'm not sure I understand why this warning suppression is required?

out is used but no IOException is thrown. You can't remove it from the method signature without potentially breaking things, so my solution was to suppress the warning and keep the method signature as it was before.

Why not just remove the throws IOException then from the method signature?
It’s a package private method, so it’s not part of the public facing API.

I've checked and you're right. Except the testcase I've created this didn't break anything else, so I've changed it

norrisjeremy · 2022-05-06T13:14:47Z

src/main/java/com/jcraft/jsch/Slf4jLogger.java

@@ -27,22 +41,30 @@ public boolean isEnabled(int level) {

  @Override
  public void log(int level, String message) {
+    log (level, message, null);


Minor nit: there appears to be an extraneous space character between log and (.

norrisjeremy · 2022-05-06T13:28:11Z

src/main/java/com/jcraft/jsch/JulLogger.java


 public class JulLogger implements com.jcraft.jsch.Logger {

-  private static final java.util.logging.Logger logger = java.util.logging.Logger.getLogger(JSch.class.getName());
+  private static final java.util.logging.Logger stlogger = java.util.logging.Logger.getLogger(JSch.class.getName());
+  private final java.util.logging.Logger logger;


I think since you've added an import java.util.logging.Logger; statement, there is no longer a need to specify the fully qualified package for these?

Although I wonder if it would be better to drop the import java.util.logging.Logger statement instead and just always fully qualify the package everywhere? That would make it slightly easier to reason about which Logger class is being used.

norrisjeremy · 2022-05-06T13:32:58Z

src/main/java/com/jcraft/jsch/Slf4jLogger.java

@@ -1,12 +1,26 @@
 package com.jcraft.jsch;

+import org.slf4j.Logger;


I wonder if it would be better to drop this import statement and just always fully qualify the package for the Logger references below so that it is easier to reason about which type of Logger is being referred to?

I think, the class is simple enough that one can be done by import and the other fully qualified. IMHO the fully qualified way is less readable.

Well, it does lead to a bit of inconsistencies between various implementations, as some of the now have an import statement for the logging framework's Logger class and some don't.

We should make it consistent between them all whether they do or don't import it.

I've checked the loggers and haven't found any inconsistencies

norrisjeremy · 2022-05-06T13:58:17Z

src/main/java/com/jcraft/jsch/Log4j2Logger.java

+    }
+    logger.log(getLevel(level), message, cause);
+  }
+


Since we added an import statement for the Logger class for the other implementations, we should probably do the same thing for this class now.

I think I've changed that already without seeing your comment, haven't I?

norrisjeremy · 2022-05-09T10:31:57Z

src/main/java/com/jcraft/jsch/KnownHosts.java

+      jsch.getInstanceLogger().log(Logger.ERROR, "unable to instantiate HMAC-class " + hmacClassname + ", using SHA-1 as fallback", e);
+      return new HMACSHA1(); // the default in config
+    }
+  }


So I've been thinking about this some more: I think instead of falling back to the com.jcraft.jsch.jce.HMACSHA1 implementation, I think we should simply throw a JSchException if creating the hmacClassname fails.
I think this will work because:

The KnownHosts class is package private since the 0.2.1 release, so we have some API flexibility.

All the locations in the JSch and Session classes where we instantiate a KnownHosts instance all appear to be in methods that already allow throwing a JSchException.

I also think this makes more sense from a user perspective, since it would immediately inform a user that the class they configured as hmac-sha1 was not usable, instead of hiding it away in a way that they may not realize (especially if they don't have a logger setup).

I can do the change but a misconfigured hmac-class should lead to execption during SSH-handshakes as you've pointed out in the thread of the issue itself.

Yes, but I don't think we should rely on that behavior.

Also, whilst the KnownHosts class isn't public, it is a HostKeyRepository implementation, which a user can obtain access to via the JSch.getHostKeyRepository() or Session.getHostKeyRepository(), which they could then utilize standalone for their own purposes, before creating a session that would trigger the SSH-handshake issue.
Therefore, I think it's best that we not hide-away the potential hmac-sha1 misconfiguration issue inside the KnownHosts class, but rather fail immediately when it is encountered.

All the locations in the JSch and Session classes where we instantiate a KnownHosts instance all appear to be in methods that already allow throwing a JSchException.

That's actually not the case:

public HostKeyRepository getHostKeyRepository(){ if(known_hosts==null) known_hosts=new KnownHosts(this); return known_hosts; }

I can put it into a RuntimeException and throw that instead

Oops, you are correct. Throwing a RuntimeException is probably the best choice, thanks!

constructor) if the HMAC-SHA1-class can't be instantiated.

norrisjeremy · 2022-05-10T20:50:25Z

src/main/java/com/jcraft/jsch/KnownHosts.java

 import java.util.ArrayList;
 import java.util.List;
 import java.util.Vector;
+import com.jcraft.jsch.jce.HMACSHA1;


I believe this import can be removed?

It can. Missed that because there were other warnings of unused stuff. I've commented out the other lines containing unused stuff as well (not removed, maybe there is a reason why it's currently unused)

from seeing this earlier

norrisjeremy · 2022-05-12T10:23:27Z

src/main/java/com/jcraft/jsch/KnownHosts.java

 import java.util.ArrayList;
 import java.util.List;
 import java.util.Vector;

 class KnownHosts implements HostKeyRepository{
-  private static final String _known_hosts="known_hosts";
+//  private static final String _known_hosts="known_hosts";


If this isn't used, then I would say to remove it instead of commenting it out.

I can delete it (as with the others) but my usual workflow is commenting out first and if nobody complains within some given time, I delete it in a later step.

I think we can just safely remove it since it is a private field.

norrisjeremy · 2022-05-12T10:23:40Z

src/main/java/com/jcraft/jsch/KnownHosts.java

@@ -290,7 +295,7 @@ public int check(String host, byte[] key){
  public void add(HostKey hostkey, UserInfo userinfo){
    int type=hostkey.type;
    String host=hostkey.getHost();
-    byte[] key=hostkey.key;
+//    byte[] key=hostkey.key;


If this isn't used, then I would say to remove it instead of commenting it out.

key is used in a code block that currently is commented out a couple of lines later. So the person who commented out that block simply forgot to do it with this particular line. That's why I'd keep it there until it's decided what should happen with the larger code block. If it's "reactivated", the line can be brought back to life as well and if it's removed that line has to go, too.

Ok, you are correct on this one: it is best to just comment it out for now as you have done.

norrisjeremy

LGTM.
@mwiede I think we should do a squash merge of this so that's all contained with a single commit.

kimmerin added 3 commits March 22, 2022 00:18

added test class

f6478c4

create the exception we expect to get the exception message of that

a8457b3

particular JVM

norrisjeremy suggested changes Apr 26, 2022

View reviewed changes

kimmerin added 5 commits April 27, 2022 20:22

Merge branch 'mwiede:master' into issue131

4874e37

use platform dependent linebreak for logging the stack trace

f9ebecd

added log-framework-specific implementations of log(int, String, Throwable) to pass the cause to the framework in a fitting way added tests for all log frameworks that can be configured in a programmatic way.

added

7e3442f

Merge branch 'issue131' of https://github.com/kimmerin/jsch into issu…

07f89ac

…e131

"normalize" line breaks

fe48f29

enforce \r\n when printing linebreaks

3a6d708

norrisjeremy suggested changes Apr 27, 2022

View reviewed changes

fixed test to check the line break between message and stacktrace (that

a09f716

actually is system dependent)

norrisjeremy suggested changes Apr 27, 2022

View reviewed changes

kimmerin added 2 commits April 27, 2022 23:37

make members final

48e11e5

use slf4j 1.x API for logging to keep backward compatibility

use logger-method to log message with cause

c17fc10

removed unnecessary System.out.println

281054c

norrisjeremy suggested changes May 6, 2022

View reviewed changes

fixed formatting

7ef222a

norrisjeremy reviewed May 6, 2022

View reviewed changes

reordered switch statement

bed0768

norrisjeremy reviewed May 6, 2022

View reviewed changes

norrisjeremy suggested changes May 6, 2022

View reviewed changes

use try-with-resource

8db5889

norrisjeremy reviewed May 6, 2022

View reviewed changes

make use of the import

a7937a0

norrisjeremy reviewed May 6, 2022

View reviewed changes

use short form instead of FQN

ca27cdf

kimmerin added 2 commits May 9, 2022 10:07

removed unused exception from method signature

1f16716

removed unnecessary suppressWarning

4a8bd79

norrisjeremy suggested changes May 9, 2022

View reviewed changes

throw a RuntimeException (to keep the signature of KnownHost's

08c9a5c

constructor) if the HMAC-SHA1-class can't be instantiated.

norrisjeremy suggested changes May 10, 2022

View reviewed changes

removed unused import. Get rid off other compiler warnings that kept me

89d84ca

from seeing this earlier

norrisjeremy suggested changes May 12, 2022

View reviewed changes

removed unused member

d1d9867

norrisjeremy approved these changes May 13, 2022

View reviewed changes

mwiede added this to the 0.2.2 milestone Jul 7, 2022

mwiede merged commit 24c803d into mwiede:master Jul 8, 2022

		@@ -1,12 +1,26 @@
		package com.jcraft.jsch;

		import org.slf4j.Logger;

Issue131 #134

Issue131 #134

Conversation

kimmerin commented Mar 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norrisjeremy commented Apr 27, 2022

norrisjeremy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimmerin commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norrisjeremy left a comment

Choose a reason for hiding this comment