Wrap IllegalArgumentException thrown by Base64 decoder #936
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some time ago, there had been `net.schmizz.sshj.common.Base64`. This class used to throw `IOException` in case of any problem. Although `IOException` isn't an appropriate class for indicating on parsing issues, a lot of code has been expecting `IOException` from Base64. Once, the old Base64 decoder was replaced with the one, bundled into Java 14 (see f35c2bd). Copy-paste elimination and switching to standard implementations is undoubtedly a good decision. Unfortunately, `java.util.Base64.Decoder` brought a pesky issue. It throws `IllegalArgumentException` in case of any problem. Since it is an unchecked exception, it was quite challenging to notice it. It's especially challenging because the error appears during processing malformed base64 strings. So, a lot of places in the code kept expecting `IOException`. Sudden `IllegalArgumentException` led to authentication termination in cases where everything used to work perfectly. One of such issues is already found and fixed: 03f8b22 This commit represents a work, based on revising every change made in f35c2bd. It should fix all other similar issues.
There was a problem hiding this comment.
Thanks for highlighting this issue and providing improvements @vladimirlagunov! The general approach makes sense, wrapping the Java Base64 Decoder in an SSHJ class that throws a checked exception seems like a good way to surface potential problems. I made several comments about class naming and message wording.
src/main/java/com/hierynomus/sshj/transport/verification/KnownHostMatchers.java
Outdated
Show resolved
Hide resolved
src/main/java/com/hierynomus/sshj/userauth/keyprovider/OpenSSHKeyFileUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/com/hierynomus/sshj/userauth/keyprovider/OpenSSHKeyV1KeyFile.java
Outdated
Show resolved
Hide resolved
src/main/java/net/schmizz/sshj/userauth/keyprovider/PuTTYKeyFile.java
Outdated
Show resolved
Hide resolved
Rename Base64DecodeError -> Base64DecodingException
A better warning message in KnownHostMatchers
A better error message in OpenSSHKeyFileUtil
A better error message in OpenSSHKeyV1KeyFile
Get rid of unnecessary `throws IOException` in Base64Decoder
Better error messages in OpenSSHKeyFileUtil and PuTTYKeyFile
exceptionfactory
approved these changes
Apr 12, 2024
There was a problem hiding this comment.
Thanks for making the adjustments @vladimirlagunov, the latest version looks good.
I prefer explicit imports versus star imports, but I don't believe that is a consistent practice throughout SSHJ.
Otherwise, hopefully @hierynomus can review soon.
Thanks again for addressing this issue!
|
Going to look at this now... Just got back from Google Cloud Next, so didn't have time before. |
hierynomus
approved these changes
Apr 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some time ago, there had been
net.schmizz.sshj.common.Base64. This class used to throwIOExceptionin case of any problem. AlthoughIOExceptionisn't an appropriate class for indicating on parsing issues, a lot of code has been expectingIOExceptionfrom Base64.Once, the old Base64 decoder was replaced with the one, bundled into Java 14 (see f35c2bd). Copy-paste elimination and switching to standard implementations is undoubtedly a good decision.
Unfortunately,
java.util.Base64.Decoderbrought a pesky issue. It throwsIllegalArgumentExceptionin case of any problem. Since it is an unchecked exception, it was quite challenging to notice it. It's especially challenging because the error appears during processing malformed base64 strings. So, a lot of places in the code kept expectingIOException. SuddenIllegalArgumentExceptionled to authentication termination in cases where everything used to work perfectly.One of such issues is already found and fixed: 03f8b22
This commit represents a work, based on revising every change made in f35c2bd. It should fix all other similar issues.