From 6bf1bd5712230d6b1b238832dbb82e6ff986f600 Mon Sep 17 00:00:00 2001 From: mathieufortin01 Date: Sun, 13 Sep 2020 14:00:06 -0700 Subject: [PATCH 1/3] Ignore Visual Studio Code Files See gh-19041 --- .gitignore | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 0b836fe4ea..d01ecf75ca 100644 --- a/.gitignore +++ b/.gitignore @@ -19,6 +19,7 @@ .settings .springBeans /build +.vscode /code MANIFEST.MF _site/ @@ -38,4 +39,4 @@ transaction-logs secrets.yml .gradletasknamecache .sts4-cache -.mvn/.gradle-enterprise/gradle-enterprise-workspace-id \ No newline at end of file +.mvn/.gradle-enterprise/gradle-enterprise-workspace-id From 4d053e15d8fa58a87b1b1c5da55efb456e547e5e Mon Sep 17 00:00:00 2001 From: mathieufortin01 Date: Sun, 13 Sep 2020 14:01:11 -0700 Subject: [PATCH 2/3] 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 --- .../springframework/boot/loader/jar/JarEntry.java | 12 ++++++++---- .../springframework/boot/loader/jar/JarFile.java | 14 +++++--------- .../boot/loader/jar/JarFileEntries.java | 2 +- .../boot/loader/jar/JarFileTests.java | 14 +++++++++++++- 4 files changed, 27 insertions(+), 15 deletions(-) diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java index aed07d1bc7..77cc68d209 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java @@ -85,16 +85,16 @@ class JarEntry extends java.util.jar.JarEntry implements FileHeader { @Override public Certificate[] getCertificates() { - if (this.jarFile.isSigned() && this.certificates == null) { - this.jarFile.setupEntryCertificates(this); + if (this.jarFile.isSigned() && this.certificates == null && isSignable()) { + this.jarFile.setupEntryCertificates(); } return this.certificates; } @Override public CodeSigner[] getCodeSigners() { - if (this.jarFile.isSigned() && this.codeSigners == null) { - this.jarFile.setupEntryCertificates(this); + if (this.jarFile.isSigned() && this.codeSigners == null && isSignable()) { + this.jarFile.setupEntryCertificates(); } return this.codeSigners; } @@ -109,4 +109,8 @@ class JarEntry extends java.util.jar.JarEntry implements FileHeader { return this.localHeaderOffset; } + private boolean isSignable() { + return !isDirectory() && !getName().startsWith("META-INF"); + } + } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java index 2649588f6e..6d8d58b6bd 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java @@ -387,19 +387,15 @@ public class JarFile extends java.util.jar.JarFile { return this.signed; } - void setupEntryCertificates(JarEntry entry) { + void setupEntryCertificates() { // Fallback to JarInputStream to obtain certificates, not fast but hopefully not // happening that often. try { - try (JarInputStream inputStream = new JarInputStream(getData().getInputStream())) { - java.util.jar.JarEntry certEntry = inputStream.getNextJarEntry(); - while (certEntry != null) { - inputStream.closeEntry(); - if (entry.getName().equals(certEntry.getName())) { - setCertificates(entry, certEntry); - } + try (JarInputStream jarStream = new JarInputStream(getData().getInputStream())) { + java.util.jar.JarEntry certEntry = null; + while ((certEntry = jarStream.getNextJarEntry()) != null) { + jarStream.closeEntry(); setCertificates(getJarEntry(certEntry.getName()), certEntry); - certEntry = inputStream.getNextJarEntry(); } } } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java index 1c983663fd..152f837af1 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java @@ -363,7 +363,7 @@ class JarFileEntries implements CentralDirectoryVisitor, Iterable { } int entryIndex = JarFileEntries.this.positions[this.index]; this.index++; - return getEntry(entryIndex, JarEntry.class, false, null); + return getEntry(entryIndex, JarEntry.class, true, null); } } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java index 3459f76c18..74c39c7cea 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java @@ -26,7 +26,9 @@ import java.net.URL; import java.net.URLClassLoader; import java.nio.charset.Charset; import java.nio.file.attribute.FileTime; +import java.security.cert.Certificate; import java.time.Instant; +import java.time.temporal.ChronoUnit; import java.util.Enumeration; import java.util.jar.JarEntry; import java.util.jar.JarInputStream; @@ -387,17 +389,27 @@ public class JarFileTests { java.util.jar.JarFile jarFile = new JarFile(new File(signedJarFile)); jarFile.getManifest(); Enumeration 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()) { JarEntry jarEntry = jarEntries.nextElement(); InputStream inputStream = jarFile.getInputStream(jarEntry); inputStream.skip(Long.MAX_VALUE); inputStream.close(); + + Certificate[] certs = jarEntry.getCertificates(); if (!jarEntry.getName().startsWith("META-INF") && !jarEntry.isDirectory() && !jarEntry.getName().endsWith("TigerDigest.class")) { - assertThat(jarEntry.getCertificates()).isNotNull(); + assertThat(certs).isNotNull(); } } 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 From c6a9696dd197fa0f5087fbe9b662226a1b9d771e Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Sun, 13 Sep 2020 13:59:26 -0700 Subject: [PATCH 3/3] Polish 'Fix signed jar performance issues' 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 --- .../boot/loader/jar/JarEntry.java | 44 +++++++------- .../loader/jar/JarEntryCertification.java | 58 +++++++++++++++++++ .../boot/loader/jar/JarFile.java | 19 +----- .../boot/loader/jar/JarFileEntries.java | 47 +++++++++++++-- .../boot/loader/jar/JarFileTests.java | 55 ++++++++---------- 5 files changed, 148 insertions(+), 75 deletions(-) create mode 100644 spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntryCertification.java diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java index 77cc68d209..192b3a8788 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2020 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -32,20 +32,21 @@ import java.util.jar.Manifest; */ class JarEntry extends java.util.jar.JarEntry implements FileHeader { + private final int index; + private final AsciiBytes name; private final AsciiBytes headerName; - private Certificate[] certificates; - - private CodeSigner[] codeSigners; - private final JarFile jarFile; private long localHeaderOffset; - JarEntry(JarFile jarFile, CentralDirectoryFileHeader header, AsciiBytes nameAlias) { + private volatile JarEntryCertification certification; + + JarEntry(JarFile jarFile, int index, CentralDirectoryFileHeader header, AsciiBytes nameAlias) { super((nameAlias != null) ? nameAlias.toString() : header.getName().toString()); + this.index = index; this.name = (nameAlias != null) ? nameAlias : header.getName(); this.headerName = header.getName(); this.jarFile = jarFile; @@ -59,6 +60,10 @@ class JarEntry extends java.util.jar.JarEntry implements FileHeader { setExtra(header.getExtra()); } + int getIndex() { + return this.index; + } + AsciiBytes getAsciiBytesName() { return this.name; } @@ -85,23 +90,24 @@ class JarEntry extends java.util.jar.JarEntry implements FileHeader { @Override public Certificate[] getCertificates() { - if (this.jarFile.isSigned() && this.certificates == null && isSignable()) { - this.jarFile.setupEntryCertificates(); - } - return this.certificates; + return getCertification().getCertificates(); } @Override public CodeSigner[] getCodeSigners() { - if (this.jarFile.isSigned() && this.codeSigners == null && isSignable()) { - this.jarFile.setupEntryCertificates(); - } - return this.codeSigners; + return getCertification().getCodeSigners(); } - void setCertificates(java.util.jar.JarEntry entry) { - this.certificates = entry.getCertificates(); - this.codeSigners = entry.getCodeSigners(); + private JarEntryCertification getCertification() { + if (!this.jarFile.isSigned()) { + return JarEntryCertification.NONE; + } + JarEntryCertification certification = this.certification; + if (certification == null) { + certification = this.jarFile.getCertification(this); + this.certification = certification; + } + return certification; } @Override @@ -109,8 +115,4 @@ class JarEntry extends java.util.jar.JarEntry implements FileHeader { return this.localHeaderOffset; } - private boolean isSignable() { - return !isDirectory() && !getName().startsWith("META-INF"); - } - } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntryCertification.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntryCertification.java new file mode 100644 index 0000000000..7d8ff07904 --- /dev/null +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntryCertification.java @@ -0,0 +1,58 @@ +/* + * Copyright 2012-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.boot.loader.jar; + +import java.security.CodeSigner; +import java.security.cert.Certificate; + +/** + * {@link Certificate} and {@link CodeSigner} details for a {@link JarEntry} from a signed + * {@link JarFile}. + * + * @author Phillip Webb + */ +class JarEntryCertification { + + static final JarEntryCertification NONE = new JarEntryCertification(null, null); + + private final Certificate[] certificates; + + private final CodeSigner[] codeSigners; + + JarEntryCertification(Certificate[] certificates, CodeSigner[] codeSigners) { + this.certificates = certificates; + this.codeSigners = codeSigners; + } + + Certificate[] getCertificates() { + return (this.certificates != null) ? this.certificates.clone() : null; + } + + CodeSigner[] getCodeSigners() { + return (this.codeSigners != null) ? this.codeSigners.clone() : null; + } + + public static JarEntryCertification from(java.util.jar.JarEntry certifiedEntry) { + Certificate[] certificates = (certifiedEntry != null) ? certifiedEntry.getCertificates() : null; + CodeSigner[] codeSigners = (certifiedEntry != null) ? certifiedEntry.getCodeSigners() : null; + if (certificates == null && codeSigners == null) { + return NONE; + } + return new JarEntryCertification(certificates, codeSigners); + } + +} diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java index 6d8d58b6bd..eb4768543e 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java @@ -27,7 +27,6 @@ import java.net.URLStreamHandlerFactory; import java.util.Enumeration; import java.util.Iterator; import java.util.function.Supplier; -import java.util.jar.JarInputStream; import java.util.jar.Manifest; import java.util.zip.ZipEntry; @@ -387,29 +386,15 @@ public class JarFile extends java.util.jar.JarFile { return this.signed; } - void setupEntryCertificates() { - // Fallback to JarInputStream to obtain certificates, not fast but hopefully not - // happening that often. + JarEntryCertification getCertification(JarEntry entry) { try { - try (JarInputStream jarStream = new JarInputStream(getData().getInputStream())) { - java.util.jar.JarEntry certEntry = null; - while ((certEntry = jarStream.getNextJarEntry()) != null) { - jarStream.closeEntry(); - setCertificates(getJarEntry(certEntry.getName()), certEntry); - } - } + return this.entries.getCertification(entry); } catch (IOException ex) { throw new IllegalStateException(ex); } } - private void setCertificates(JarEntry entry, java.util.jar.JarEntry certEntry) { - if (entry != null) { - entry.setCertificates(certEntry); - } - } - public void clearCache() { this.entries.clearCache(); } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java index 152f837af1..5645abdd36 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2020 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -26,6 +26,7 @@ import java.util.Map; import java.util.NoSuchElementException; import java.util.jar.Attributes; import java.util.jar.Attributes.Name; +import java.util.jar.JarInputStream; import java.util.jar.Manifest; import java.util.zip.ZipEntry; @@ -91,14 +92,13 @@ class JarFileEntries implements CentralDirectoryVisitor, Iterable { private Boolean multiReleaseJar; + private JarEntryCertification[] certifications; + private final Map entriesCache = Collections .synchronizedMap(new LinkedHashMap(16, 0.75f, true) { @Override protected boolean removeEldestEntry(Map.Entry eldest) { - if (JarFileEntries.this.jarFile.isSigned()) { - return false; - } return size() >= ENTRY_CACHE_SIZE; } @@ -313,7 +313,7 @@ class JarFileEntries implements CentralDirectoryVisitor, Iterable { FileHeader entry = (cached != null) ? cached : CentralDirectoryFileHeader .fromRandomAccessData(this.centralDirectoryData, this.centralDirectoryOffsets[index], this.filter); if (CentralDirectoryFileHeader.class.equals(entry.getClass()) && type.equals(JarEntry.class)) { - entry = new JarEntry(this.jarFile, (CentralDirectoryFileHeader) entry, nameAlias); + entry = new JarEntry(this.jarFile, index, (CentralDirectoryFileHeader) entry, nameAlias); } if (cacheEntry && cached != entry) { this.entriesCache.put(index, entry); @@ -344,6 +344,41 @@ class JarFileEntries implements CentralDirectoryVisitor, Iterable { return (this.filter != null) ? this.filter.apply(name) : name; } + JarEntryCertification getCertification(JarEntry entry) throws IOException { + JarEntryCertification[] certifications = this.certifications; + if (certifications == null) { + certifications = new JarEntryCertification[this.size]; + // We fallback to use JarInputStream to obtain the certs. This isn't that + // fast, but hopefully doesn't happen too often. + try (JarInputStream certifiedJarStream = new JarInputStream(this.jarFile.getData().getInputStream())) { + java.util.jar.JarEntry certifiedEntry = null; + while ((certifiedEntry = certifiedJarStream.getNextJarEntry()) != null) { + int index = getEntryIndex(certifiedEntry.getName()); + if (index != -1) { + certifications[index] = JarEntryCertification.from(certifiedEntry); + } + certifiedJarStream.closeEntry(); + } + } + this.certifications = certifications; + } + JarEntryCertification certification = certifications[entry.getIndex()]; + return (certification != null) ? certification : JarEntryCertification.NONE; + } + + private int getEntryIndex(CharSequence name) { + int hashCode = AsciiBytes.hashCode(name); + int index = getFirstIndex(hashCode); + while (index >= 0 && index < this.size && this.hashCodes[index] == hashCode) { + CentralDirectoryFileHeader candidate = getEntry(index, CentralDirectoryFileHeader.class, false, null); + if (candidate.hasName(name, NO_SUFFIX)) { + return index; + } + index++; + } + return -1; + } + /** * Iterator for contained entries. */ @@ -363,7 +398,7 @@ class JarFileEntries implements CentralDirectoryVisitor, Iterable { } int entryIndex = JarFileEntries.this.positions[this.index]; this.index++; - return getEntry(entryIndex, JarEntry.class, true, null); + return getEntry(entryIndex, JarEntry.class, false, null); } } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java index 74c39c7cea..50daa7a0c4 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java @@ -26,9 +26,7 @@ import java.net.URL; import java.net.URLClassLoader; import java.nio.charset.Charset; import java.nio.file.attribute.FileTime; -import java.security.cert.Certificate; import java.time.Instant; -import java.time.temporal.ChronoUnit; import java.util.Enumeration; import java.util.jar.JarEntry; import java.util.jar.JarInputStream; @@ -46,6 +44,7 @@ import org.springframework.boot.loader.TestJarCreator; import org.springframework.boot.loader.data.RandomAccessDataFile; import org.springframework.test.util.ReflectionTestUtils; import org.springframework.util.FileCopyUtils; +import org.springframework.util.StopWatch; import org.springframework.util.StreamUtils; import static org.assertj.core.api.Assertions.assertThat; @@ -377,39 +376,33 @@ public class JarFileTests { @Test public void verifySignedJar() throws Exception { - String classpath = System.getProperty("java.class.path"); - String[] entries = classpath.split(System.getProperty("path.separator")); - String signedJarFile = null; - for (String entry : entries) { - if (entry.contains("bcprov")) { - signedJarFile = entry; + File signedJarFile = getSignedJarFile(); + assertThat(signedJarFile).exists(); + try (java.util.jar.JarFile expected = new java.util.jar.JarFile(signedJarFile)) { + try (JarFile actual = new JarFile(signedJarFile)) { + StopWatch stopWatch = new StopWatch(); + Enumeration actualEntries = actual.entries(); + while (actualEntries.hasMoreElements()) { + JarEntry actualEntry = actualEntries.nextElement(); + java.util.jar.JarEntry expectedEntry = expected.getJarEntry(actualEntry.getName()); + assertThat(actualEntry.getCertificates()).as(actualEntry.getName()) + .isEqualTo(expectedEntry.getCertificates()); + assertThat(actualEntry.getCodeSigners()).as(actualEntry.getName()) + .isEqualTo(expectedEntry.getCodeSigners()); + } + assertThat(stopWatch.getTotalTimeSeconds()).isLessThan(3.0); } } - assertThat(signedJarFile).isNotNull(); - java.util.jar.JarFile jarFile = new JarFile(new File(signedJarFile)); - jarFile.getManifest(); - Enumeration 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()) { - JarEntry jarEntry = jarEntries.nextElement(); - InputStream inputStream = jarFile.getInputStream(jarEntry); - inputStream.skip(Long.MAX_VALUE); - inputStream.close(); - - Certificate[] certs = jarEntry.getCertificates(); - if (!jarEntry.getName().startsWith("META-INF") && !jarEntry.isDirectory() - && !jarEntry.getName().endsWith("TigerDigest.class")) { - assertThat(certs).isNotNull(); + } + + private File getSignedJarFile() { + String[] entries = System.getProperty("java.class.path").split(System.getProperty("path.separator")); + for (String entry : entries) { + if (entry.contains("bcprov")) { + return new File(entry); } } - jarFile.close(); - - // 3 seconds is still quite long, but low enough to catch most problems - assertThat(ChronoUnit.SECONDS.between(start, Instant.now())).isLessThanOrEqualTo(3L); + return null; } @Test