sshj
sshj copied to clipboard
Remove jzlib dependency
The JRE already contains a gzip implementation in java.util.zip.*
, so we should be able to use that rather than relying on the external jCraft library.
Codecov Report
Merging #772 (ec74015) into master (dcfa183) will increase coverage by
0.01%
. The diff coverage is0.00%
.
@@ Coverage Diff @@
## master #772 +/- ##
============================================
+ Coverage 64.88% 64.89% +0.01%
+ Complexity 1469 1465 -4
============================================
Files 210 210
Lines 8537 8523 -14
Branches 781 780 -1
============================================
- Hits 5539 5531 -8
+ Misses 2591 2582 -9
- Partials 407 410 +3
Impacted Files | Coverage Δ | |
---|---|---|
...zz/sshj/transport/compression/ZlibCompression.java | 0.00% <0.00%> (ø) |
|
...net/schmizz/sshj/transport/TransportException.java | 44.44% <0.00%> (-11.12%) |
:arrow_down: |
...ain/java/net/schmizz/sshj/common/SSHException.java | 67.74% <0.00%> (-6.46%) |
:arrow_down: |
src/main/java/net/schmizz/sshj/SSHClient.java | 58.52% <0.00%> (-1.14%) |
:arrow_down: |
...java/net/schmizz/sshj/transport/TransportImpl.java | 64.87% <0.00%> (-0.83%) |
:arrow_down: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
Unfortunately I do not think this will work. Zip is different from zlib and both algorithms are unable to read eachother's data.
Yes, a PKZip file is a different format from a Gzip file. But technically both use the "deflate" algorithm internally. Long ago, the JRE didn't contain tools for dealing directly with gzip/zlib style "deflate" streams (Java of course has supported access to PKZIP files since the beginning, since that's what .JARs are). As of Java 6 or so, (apparently as part of adding PNG support), classes were made available inside java.util.zip
for handling "deflate" streams in a general-purpose way, compatible with gzip/zlib. See the docs here relating to the specific classes I used: https://docs.oracle.com/javase/6/docs/api/index.html?java/util/zip/package-summary.html
Note it says "compression using the popular ZLIB compression library".
Additionally, I have tested this myself against an OpenSSH Docker container by doing this to force the use of my Zlib implementation (note the absence of the "NoneCompression.Factory"):
ssh.getTransport().getConfig().setCompressionFactories(Arrays.asList(
new DelayedZlibCompression.Factory()
));
Give it a try -- it works for me 😁
I dug into the history a little more - it actually looks like zlib deflate/inflate support has been available in java.util.zip
since all the way back in Java 1.1, when the JAR format was first introduced!
https://www.cs.princeton.edu/courses/archive/fall97/cs461/jdkdocs/api/java.util.zip.Deflater.html
The only reason the jCraft jzlib project was even created was because the JRE implementation didn't yet support partial-flush mode, as explained here: http://www.jcraft.com/jzlib/
This deficiency was eventually fixed in Java 7 with the introduction of the 4-argument deflate(byte[] b, int length, int off, int flush)
, allowing the use of the SYNC_FLUSH
flag:
https://docs.oracle.com/javase/7/docs/api/java/util/zip/Deflater.html#deflate(byte[],%20int,%20int,%20int)
The Java bug report about this is here (and confirms that it was fixed as of JDK 7): https://bugs.java.com/bugdatabase/view_bug.do?bug_id=4206909
So I suppose if you are intending to still support Java 6 for now, maybe you can't merge this yet, but as soon as you are targeting Java 7 and above, this PR should be usable.
Is there a chance that this PR moves forward? I think that would be great! Just like jsch, jzlib seems to be unmaintained. And if jzlib can even be replaced by JDK internals, I think it's a good idea to do so.
Apache Mina Sshd did the same a while ago: https://github.com/apache/mina-sshd/commit/ea72a9eefa9872f6d41bd6c60a9a929824328051
It looks like Java 7 may have already been a requirement since this commit: https://github.com/hierynomus/sshj/commit/35266945585b440ca75040d0af07181598531139#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R88
So I'm not aware of any reason this shouldn't be mergeable at this point. @hierynomus , do you have any remaining concerns?