Improving logging for KeyExchanger

[Andremoniy]

In the KeyExchanger class there is the following error logging:

log.error("Disconnecting because none of the configured Host key verifiers ({}) could verify '{}' host key with fingerprint {} for {}:{}", hostVerifiers, KeyType.fromKey(key), SecurityUtils.getFingerprint(key), transport.getRemoteHost(), transport.getRemotePort());

However the hostVerifiers's elements do not properly override the toString() method.

It would be nice to see actual contents of this array for debug purposes.

[hierynomus]

@Andremoniy Do you have any idea how many entries a typical known_hosts file contains? I have 96 in there. If we would add this logging, you would not have a readable log file.

[Andremoniy]

@Andremoniy Do you have any idea how many entries a typical known_hosts file contains? I have 96 in there. If we would add this logging, you would not have a readable log file.

Fair enough. However the error itself says about some critical situation which has to be investigated. Say we have this issue on a bamboo server, so we loose the actual docker container's contents but want to check what's going on based on the logs.

Would you approve a compromise solution with log.debug(...) printing out the contents of the hostVerifier's list?

[Andremoniy]

@Andremoniy Do you have any idea how many entries a typical known_hosts file contains? I have 96 in there. If we would add this logging, you would not have a readable log file.

P.S. I don't really think that a list of 96 default object identifiers is somehow better than their actual contents :p
With great respect to your great library 👍

[codecov-io]

Codecov Report

Merging #458 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #458      +/-   ##
============================================
+ Coverage     55.55%   55.59%   +0.03%     
- Complexity     1210     1212       +2     
============================================
  Files           192      192              
  Lines          7850     7852       +2     
  Branches        712      712              
============================================
+ Hits           4361     4365       +4     
+ Misses         3139     3137       -2     
  Partials        350      350

Impacted Files	Coverage Δ	Complexity Δ
...hj/transport/verification/FingerprintVerifier.java	74.19% <100%> (+0.86%)	9 <1> (+1)	⬆️
...sshj/transport/verification/OpenSSHKnownHosts.java	55.84% <100%> (+0.28%)	11 <1> (+1)	⬆️
.../main/java/net/schmizz/sshj/sftp/PacketReader.java	91.3% <0%> (-4.35%)	12% <0%> (-1%)
...zz/sshj/connection/channel/ChannelInputStream.java	76.11% <0%> (+2.98%)	16% <0%> (+1%)	⬆️
...t/schmizz/sshj/connection/ConnectionException.java	44.44% <0%> (+11.11%)	4% <0%> (+1%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17c368f...6af7ec2. Read the comment docs.

[hierynomus]

@Andremoniy Do you have any idea how many entries a typical known_hosts file contains? I have 96 in there. If we would add this logging, you would not have a readable log file.

P.S. I don't really think that a list of 96 default object identifiers is somehow better than their actual contents :p
With great respect to your great library 👍

You'd now only get a single OID ;) (OpenSshKnownHosts@129839...), whereas with the current change you get one very big string (with 96 lines)....

I agree that a toString for each HostKeyVerifier improves the error message. But I would suggest to only print the Classname and the filename that backs the OpenSSHKnownHosts verifier. So for instance OpenSSHKnownHosts[khFile=/home/user/.ssh/known_hosts], so not the actual entries in the known_hosts files.

[Andremoniy]

@Andremoniy Do you have any idea how many entries a typical known_hosts file contains? I have 96 in there. If we would add this logging, you would not have a readable log file.

P.S. I don't really think that a list of 96 default object identifiers is somehow better than their actual contents :p
With great respect to your great library 👍

You'd now only get a single OID ;) (OpenSshKnownHosts@129839...), whereas with the current change you get one very big string (with 96 lines)....

I agree that a toString for each HostKeyVerifier improves the error message. But I would suggest to only print the Classname and the filename that backs the OpenSSHKnownHosts verifier. So for instance OpenSSHKnownHosts[khFile=/home/user/.ssh/known_hosts], so not the actual entries in the known_hosts files.

Having a meaningful toString for the OpenSSHKnownHosts class as you proposed seems to be a certain improvement, no doubts!

Following your proposal I've updated my pull request. What do you think about that?

[hierynomus]

One line in there that doesn't belong to the PR I think. Otherwise great! If you remove that line then I'll merge it