Gracefully load OpenSSH known_hosts without requiring BouncyCastle

[solind]

If BouncyCastle is not present, I want to be able to load whatever bits of the known_hosts file I can (i.e., the ssh-dss and ssh-rsa keys).

While making this change I noticed the KeyType.readPubKeyFromBuffer method didn't require passing in the String type name, so I cleaned that up too.

[solind]

Do you really want me to use four spaces to indent cases in a switch statement? :(

[hierynomus]

Well actually, that spacing is a pretty default and widely accepted setting ;)

[hierynomus]

I'm missing some tests that showcase the problem you had and that was fixed now. From the code I don't see much difference except for the type of exception that might now be thrown. You might've first gotten a SecurityException and now a BufferException.

[solind]

The BufferException is declared, the RuntimeException would generally be uncaught and terminate processing. That's the difference.

I thought the tests were all run with BouncyCastle in the classpath. How would I write a test case that requires BouncyCastle not be loaded? Is there an example you could point me to?

[hierynomus]

The whole problem with this is that there is also requests for being able
to use SpongyCastle, which is the Bouncy for Android. Adding this code
would now inadvertently throw an exception if Bouncy was not registered but
Spongy is. So I don't like this PR at the moment. The cleanup is nice
though, but the explicit checking for BC and switch outside of KeyType I
don't like.

2016-09-12 13:30 GMT+02:00 David Solin [email protected]:

The BufferException is declared, the RuntimeException would generally be
uncaught and terminate processing. That's the difference.

I thought the tests were all run with BouncyCastle in the classpath. How
would I write a test case that requires BouncyCastle not be loaded? Is
there an example you could point me to?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#271 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHLoyRjpt34LZqBDM-kvDEVm5FHV3o1ks5qpTfWgaJpZM4J5ZJx
.

[joval]

There are other places in the code that check for BouncyCastle -- the utility method is pre-existing. If SpongyCastle is a true replacement for BC, only that utility method would need to be changed to check for it to add SC support.

[solind]

Would you prefer it if I checked for BC in the individual KeyType implementations?

[solind]

Another possibility would be for me to modify the OpenSSHKnownHosts class, to catch SSHRuntimeExceptions in the constructor and log a different message.

Would that be preferable?

[solind]

Actually that won't work... this is the error I was aiming to avoid:

Exception in thread "main" java.lang.NoClassDefFoundError: org/bouncycastle/asn1/nist/NISTNamedCurves
    at net.schmizz.sshj.common.KeyType$3.readPubKeyFromBuffer(KeyType.java:146)
    at net.schmizz.sshj.common.Buffer.readPublicKey(Buffer.java:429)
    at net.schmizz.sshj.transport.verification.OpenSSHKnownHosts$EntryFactory.parseEntry(OpenSSHKnownHosts.java:215)
    at net.schmizz.sshj.transport.verification.OpenSSHKnownHosts.<init>(OpenSSHKnownHosts.java:62)
    at net.schmizz.sshj.transport.verification.OpenSSHKnownHosts.<init>(OpenSSHKnownHosts.java:47)

[hierynomus]

Hi David,

I think for the ecdsa-sha2-nistp256 key you need to check whether BC is registered right at the start of the readPubKeyFromBuffer method, as that really is hardwired against BC classes. However for ssh-ed25519 you would not need that check, as it does not use BC at all, but rather the ed25519 library.

[solind]

Hi Jeroen, you're quite right (of course)! I've verified that the last set of changes work, and avoid the ClassNotFoundException when BC is not available.

It would be nice if it would be possible to create automated test cases without the BouncyCastle dependency, to prevent future regressions. Do you have any ideas how that could be accomplished?

[solind]

(Note, I also re-formatted the switch statement in PKCS5KeyFile per the style in this project).

[hierynomus]

There are a number of scenarios that I could think of:

1. BC is not on the classpath
2. BC is on the classpath but is not registered (ie. Security.addProvider(new BouncyCastleProvider()) has not been called)
3. BC is on the classpath and is registered, but the unlimited strength crypto extensions have not been loaded
4. BC is on the classpath and is registered in unlimited strength mode

Currently only 4. is being tested against.

In order to test for 1 you would maybe need to spin up a new JVM with a classpath without BC present in a unit test. We could maybe do that as a separate test task in Gradle. That should not be all too hard.

[hierynomus]

Merged! Thanks again for the PR :)