Skip to content

Signed jar dependency performance problem when repackaged in a single jar #19041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

mathieufortin01
Copy link
Contributor

Issue

Some 3rd party jars verify their own signature and makes sure it is signed by a specific signer, calling JarEntry.getCertificates() in the process. When these jars are handled by the Spring Boot Loader (as is the case when the jar is repackaged into a single jar), a long time can be spent searching for the certificates in the original jar entry and setting them in the loader's own entries. We have a case where it takes up to 5 min, delaying the startup of the Spring boot application.

Workaround

The current workaround is to not use Spring Boot maven plugin to generate an executable jar, and use the classic way of launching the jar with a classpath entry.

Fix

The fix is to filter out the entries we know won't be signed, ie directory entries and META-INF entries. It seems that specifically processing a directory messes up the "find & set certificate" routines, ending up being what it seems to be quadratic.

The test for this (JarFileTests.verifySignedJar) hid this problem by pre-filtering unsigned entries. If the test is modified to call getCertificates on all entries, the current jar used in the test (bouncy castle) takes close to 7 sec, which is too much.

I fiexd the issue by filtering out the unsigned entries. I also modified the test to detect this issue by setting up a maximum amount of time it should take for a jar to load.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 18, 2019
@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Nov 19, 2019
@philwebb philwebb added type: enhancement A general enhancement and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Dec 13, 2019
@philwebb philwebb added this to the 2.3.x milestone Dec 13, 2019
@philwebb
Copy link
Member

We'd like to have another look at JarFile.setupEntryCertificates to see if there are better ways to deal with loading the certs.

@philwebb philwebb added the for: merge-with-amendments Needs some changes when we merge label Dec 13, 2019
@mathieufortin01
Copy link
Contributor Author

mathieufortin01 commented Dec 15, 2019

I changed JarFile.setupEntryCertificates(JarEntry) to JarFile.setupEntryCertificates(), as I didnt see the reasoning behind setting the certs for a single entry, and then for every entries. They now get set for all entries as soon as getCertificates is called on a signable entry in a signed jar.

I also enabled caching of the entries when JarFileEntries.EntryIterator.next is used, as it was set to false, leading to a bunch of unnecessary JarEntry creation (and it may have been related to the quadratic performance I was experiencing). I am not sure right now what triggers the cache clearing, but it looks like it gets cleared when running as a single jar.

I will update the pull request tomorrow.

@gjd6640
Copy link

gjd6640 commented Jan 9, 2020

Hello. I just ran into this same thing so am glad to see that a fix is in the works. Keep up the good work!

This OpenJDK bug fix is old news but I thought that its description of a lack of caching of metafiles and the resulting quadratic increase in read volumes sounded spookily similar to your diagnosis: https://bugs.openjdk.java.net/browse/JDK-6354728

I hope that you find this info helpful.

@philwebb philwebb self-assigned this Apr 30, 2020
@chrismathews
Copy link

chrismathews commented Aug 7, 2020

Any recent movement on this issue? We recently hit this problem when upgrading Bouncy Castle from 1.65 to 1.66 in our applications. When running the Spring Boot Plugin created uber jar we started to see a 20 - 30 second hit to our startup time when initializing Bouncy Castle. If we do a "classic" -cp style execution of the same application then there is no startup performance problems. We also noticed this only occurs on Java 11 and the problem did not exist if we ran against Java 8 (we didn't try any non-LTS releases of Java).

Nothing in the Bouncy Castle 1.66 release notes really jumped out at us as a potential cause and then we stumbled upon this PR. We did notice that the bcprov-jdk15on-1.65.jar was ~4.25MB in size vs the bcprov-jdk15on-1.66.jar which is ~5.61MB. Looking into a bit more and the META-INF for 1.66 is significantly larger than 1.65. So if this problem is anything like the bug referenced by @gjd6640 above then the steep increase in initialization time would make sense.

For the time being we are going to downgrade to Bouncy Castle 1.65 but that is obviously not a long-term solution. We are willing to help test or validate any proposed solution here.

@philwebb philwebb added type: bug A general bug and removed type: enhancement A general enhancement labels Sep 13, 2020
@philwebb philwebb modified the milestones: 2.3.x, 2.1.x Sep 13, 2020
philwebb pushed a commit that referenced this pull request Sep 14, 2020
philwebb pushed a commit that referenced this pull request Sep 14, 2020
Update Spring Boot nested JarFile support to improve the performance of
signed jars. Prior to this commit, `certificates` and `codeSigners`
were read by streaming the entire jar whenever the existing values
were `null`. Unfortunately, the contract for `getCertificates` and
get `getCodeSigners` states that `null` is a valid return value. This
meant that full jar streaming would occur whenever either method was
called on an entry that had no result. The problem was further
exacerbated by the fact that entries might not be cached.

See gh-19041
philwebb added a commit that referenced this pull request Sep 14, 2020
Update the performance improvements to push certificate loading
and storage into the `JarFileEntries` class. This allows us to
keep certificates without needing to cache all entry data. We
now also keep certificates and code signers in a dedicated class
which is set whenever the full jar stream as been read, even if
the contained values are `null`. The logic that assumes META-INF
entries are not signed has been removed in favor of delegating to
the streamed entry results.

See gh-19041
@philwebb philwebb modified the milestones: 2.1.x, 2.1.17 Sep 14, 2020
@philwebb philwebb closed this in 895ff9c Sep 14, 2020
@philwebb
Copy link
Member

@mathieufortin01 Thanks very much for the PR and for the detailed issue report. I'm sorry it has taken us so long to get to this one. I've revised the fix quite a bit in commit c6a9696 (the commit message has some more details). Since this is actually quite a nasty issue I've applied the changes to 2.1.x forwards.

@philwebb
Copy link
Member

@chrismathews Thanks for the additional info about Bouncy Castle 1.66.

We are willing to help test or validate any proposed solution here

Once CI build 871 has completed would you be able to try the latest SNAPSHOT of whatever branch you're on (assuming it's 2.1 or higher) to let us know if the fix has made a difference? We're planning to do a set of releases this week on Sep 17th.

@ShravanGottimukula
Copy link

ShravanGottimukula commented Sep 14, 2020

@philwebb I work with @chrismathews. Tested the above changes with Bouncy Castle 1.66 and Spring Boot 2.3.4.BUILD-SNAPSHOT and we are now running into the below ClassCastException when running the spring boot plugin generated uber jar using java -jar. Attached the sample project to replicate the issue and the output.

bouncycastle-spring-boot-test.zip
output.txt

Caused by: java.lang.ClassCastException: class org.springframework.boot.loader.jar.JarEntry cannot be cast to class org.springframework.boot.loader.jar.CentralDirectoryFileHeader (org.springframework.boot.loader.jar.JarEntry and org.springframework.boot.loader.jar.CentralDirectoryFileHeader are in unnamed module of loader 'app')
	at org.springframework.boot.loader.jar.JarFileEntries.getEntryIndex(JarFileEntries.java:380) ~[bouncycastle-spring-boot-test-0.0.1-SNAPSHOT.jar:0.0.1-SNAPSHOT]

Please let us know if you need any more info.

Environment:
Java 11.0.8
Maven 3.6.3

@philwebb philwebb reopened this Sep 14, 2020
@philwebb philwebb closed this in 4138e59 Sep 14, 2020
@philwebb
Copy link
Member

@ShravanGottimukula Thanks very much for testing the snapshot. I see the issue and I've pushed a fix but unfortunately I can't get the sample to fail in the same way on my local box. Perhaps it's due to a different Java build or a different OS.

When build 1384 has finished, could you please try the sample again on your side. If you run mvn with -U it should force a fresh of the SNAPSHOT dependencies.

@ShravanGottimukula
Copy link

ShravanGottimukula commented Sep 15, 2020

@philwebb Now running into the below error with the latest snapshot build

Environment:
Windows 10
Oracle Java 11.0.8
Maven 3.6.3
Spring Boot 2.3.4.BUILD-SNAPSHOT

Caused by: java.util.jar.JarException: jar:file:/C:/workspaces/bouncycastle-spring-boot-test/target/bouncycastle-spring-boot-test-0.0.1-SNAPSHOT.jar!/BOOT-INF/lib/bcprov-jdk15on-1.66.jar!/ has unsigned entries - META-INF/services/java.security.Provider at java.base/javax.crypto.JarVerifier.verifySingleJar(JarVerifier.java:459) ~[na:na] at java.base/javax.crypto.JarVerifier.verifyJars(JarVerifier.java:314) ~[na:na] at java.base/javax.crypto.JarVerifier.verify(JarVerifier.java:257) ~[na:na] at java.base/javax.crypto.ProviderVerifier.verify(ProviderVerifier.java:129) ~[na:na] at java.base/javax.crypto.JceSecurity.verifyProvider(JceSecurity.java:191) ~[na:na] at java.base/javax.crypto.JceSecurity.getVerificationResult(JceSecurity.java:217) ~[na:na] at java.base/javax.crypto.JceSecurity.getInstance(JceSecurity.java:141) ~[na:na] ... 43 common frames omitted

@wilkinsona wilkinsona reopened this Sep 15, 2020
@philwebb
Copy link
Member

Thanks @ShravanGottimukula. I've now managed to replicate this on a Windows box using the Oracle JVM

philwebb added a commit that referenced this pull request Sep 15, 2020
Ensure that the source jar entry is closed before reading
certificates and code signers from the entry.

gh-19041
@philwebb
Copy link
Member

Third time's a charm. I made a stupid mistake by not closing the entry before reading the certificates.

@ShravanGottimukula the sample now works for me, can you try it and your original application to see if things work and if the performance is better?

@ShravanGottimukula
Copy link

@philwebb Looks good now. Tested with 2.3.4.BUILD-SNAPSHOT, 2.2.10.BUILD-SNAPSHOT and 2.1.17.BUILD-SNAPSHOT and all of them work and the signed jar verification is much faster now with no noticeable slowness at all.

@philwebb
Copy link
Member

@ShravanGottimukula Thanks very much for testing it again. 👍

@philwebb philwebb closed this Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants