Commit 4d053e15 authored by mathieufortin01's avatar mathieufortin01 Committed by Phillip Webb

Fix signed jar performance issues

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
parent 6bf1bd57
...@@ -85,16 +85,16 @@ class JarEntry extends java.util.jar.JarEntry implements FileHeader { ...@@ -85,16 +85,16 @@ class JarEntry extends java.util.jar.JarEntry implements FileHeader {
@Override @Override
public Certificate[] getCertificates() { public Certificate[] getCertificates() {
if (this.jarFile.isSigned() && this.certificates == null) { if (this.jarFile.isSigned() && this.certificates == null && isSignable()) {
this.jarFile.setupEntryCertificates(this); this.jarFile.setupEntryCertificates();
} }
return this.certificates; return this.certificates;
} }
@Override @Override
public CodeSigner[] getCodeSigners() { public CodeSigner[] getCodeSigners() {
if (this.jarFile.isSigned() && this.codeSigners == null) { if (this.jarFile.isSigned() && this.codeSigners == null && isSignable()) {
this.jarFile.setupEntryCertificates(this); this.jarFile.setupEntryCertificates();
} }
return this.codeSigners; return this.codeSigners;
} }
...@@ -109,4 +109,8 @@ class JarEntry extends java.util.jar.JarEntry implements FileHeader { ...@@ -109,4 +109,8 @@ class JarEntry extends java.util.jar.JarEntry implements FileHeader {
return this.localHeaderOffset; return this.localHeaderOffset;
} }
private boolean isSignable() {
return !isDirectory() && !getName().startsWith("META-INF");
}
} }
...@@ -387,19 +387,15 @@ public class JarFile extends java.util.jar.JarFile { ...@@ -387,19 +387,15 @@ public class JarFile extends java.util.jar.JarFile {
return this.signed; return this.signed;
} }
void setupEntryCertificates(JarEntry entry) { void setupEntryCertificates() {
// Fallback to JarInputStream to obtain certificates, not fast but hopefully not // Fallback to JarInputStream to obtain certificates, not fast but hopefully not
// happening that often. // happening that often.
try { try {
try (JarInputStream inputStream = new JarInputStream(getData().getInputStream())) { try (JarInputStream jarStream = new JarInputStream(getData().getInputStream())) {
java.util.jar.JarEntry certEntry = inputStream.getNextJarEntry(); java.util.jar.JarEntry certEntry = null;
while (certEntry != null) { while ((certEntry = jarStream.getNextJarEntry()) != null) {
inputStream.closeEntry(); jarStream.closeEntry();
if (entry.getName().equals(certEntry.getName())) {
setCertificates(entry, certEntry);
}
setCertificates(getJarEntry(certEntry.getName()), certEntry); setCertificates(getJarEntry(certEntry.getName()), certEntry);
certEntry = inputStream.getNextJarEntry();
} }
} }
} }
......
...@@ -363,7 +363,7 @@ class JarFileEntries implements CentralDirectoryVisitor, Iterable<JarEntry> { ...@@ -363,7 +363,7 @@ class JarFileEntries implements CentralDirectoryVisitor, Iterable<JarEntry> {
} }
int entryIndex = JarFileEntries.this.positions[this.index]; int entryIndex = JarFileEntries.this.positions[this.index];
this.index++; this.index++;
return getEntry(entryIndex, JarEntry.class, false, null); return getEntry(entryIndex, JarEntry.class, true, null);
} }
} }
......
...@@ -26,7 +26,9 @@ import java.net.URL; ...@@ -26,7 +26,9 @@ import java.net.URL;
import java.net.URLClassLoader; import java.net.URLClassLoader;
import java.nio.charset.Charset; import java.nio.charset.Charset;
import java.nio.file.attribute.FileTime; import java.nio.file.attribute.FileTime;
import java.security.cert.Certificate;
import java.time.Instant; import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.Enumeration; import java.util.Enumeration;
import java.util.jar.JarEntry; import java.util.jar.JarEntry;
import java.util.jar.JarInputStream; import java.util.jar.JarInputStream;
...@@ -387,17 +389,27 @@ public class JarFileTests { ...@@ -387,17 +389,27 @@ public class JarFileTests {
java.util.jar.JarFile jarFile = new JarFile(new File(signedJarFile)); java.util.jar.JarFile jarFile = new JarFile(new File(signedJarFile));
jarFile.getManifest(); jarFile.getManifest();
Enumeration<JarEntry> jarEntries = jarFile.entries(); Enumeration<JarEntry> jarEntries = jarFile.entries();
// Make sure this whole certificates routine runs in an acceptable time (few
// seconds at most)
// Some signed jars took from 30s to 5 min depending on the implementation.
Instant start = Instant.now();
while (jarEntries.hasMoreElements()) { while (jarEntries.hasMoreElements()) {
JarEntry jarEntry = jarEntries.nextElement(); JarEntry jarEntry = jarEntries.nextElement();
InputStream inputStream = jarFile.getInputStream(jarEntry); InputStream inputStream = jarFile.getInputStream(jarEntry);
inputStream.skip(Long.MAX_VALUE); inputStream.skip(Long.MAX_VALUE);
inputStream.close(); inputStream.close();
Certificate[] certs = jarEntry.getCertificates();
if (!jarEntry.getName().startsWith("META-INF") && !jarEntry.isDirectory() if (!jarEntry.getName().startsWith("META-INF") && !jarEntry.isDirectory()
&& !jarEntry.getName().endsWith("TigerDigest.class")) { && !jarEntry.getName().endsWith("TigerDigest.class")) {
assertThat(jarEntry.getCertificates()).isNotNull(); assertThat(certs).isNotNull();
} }
} }
jarFile.close(); jarFile.close();
// 3 seconds is still quite long, but low enough to catch most problems
assertThat(ChronoUnit.SECONDS.between(start, Instant.now())).isLessThanOrEqualTo(3L);
} }
@Test @Test
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment