On Tue, 7 Apr 2026 14:06:23 GMT, Jaikiran Pai <[email protected]> wrote:
> Can I please get a review of this test-only change which updates the > `test/jdk/java/util/jar/JarEntry/GetMethodsReturnClones.java` test to use a > dynamically generated signed JAR file to run its tests? This addresses > https://bugs.openjdk.org/browse/JDK-8378291. > > This test exercises the `JarEntry.getCertificates()` and > `JarEntry.getCodeSigners()` method to verify that those methods return a copy > of the original array thus preventing any updates to the returned array from > being propagated to the original array. This test was using a pre-generated > `test.jar` file which was signed using a (now) weak key and the JAR was being > treated as unsigned: > > > jarsigner -certs -verbose -verify test/jdk/java/util/jar/JarEntry/test.jar > > 140 Sun Feb 08 17:22:36 IST 2004 META-INF/MANIFEST.MF > 202 Sun Feb 08 17:22:36 IST 2004 META-INF/TEST.SF > 1595 Sun Feb 08 17:22:36 IST 2004 META-INF/TEST.RSA > 0 Tue Sep 17 13:07:14 IST 2002 META-INF/ > m ? 1770 Tue Sep 17 13:04:40 IST 2002 WriteFileTest.class > > s = signature was verified > m = entry is listed in manifest > k = at least one certificate was found in keystore > ? = unsigned entry > > ... > WARNING: The jar will be treated as unsigned, because it is signed with a > weak algorithm that is now disabled by the security property: > > jdk.jar.disabledAlgorithms=MD2, MD5, RSA keySize < 1024, DSA keySize < > 1024, SHA1 denyAfter 2019-01-01 > > > The test has now been updated to dynamically generate the JAR file and verify > these methods. Existing tests in `test/jdk/java/util/jar` continue to pass > and tier1 testing is currently in progress. Good to see this taken care of. Left some review comments. test/jdk/java/util/jar/JarEntry/GetMethodsReturnClones.java line 74: > 72: */ > 73: @BeforeAll() > 74: static void beforeAll() throws Exception { I'm mostly used to this such methods being named `setup`. The current name does not provide anything other than what the annotation does. Maybe a matter of personal preference. test/jdk/java/util/jar/JarEntry/GetMethodsReturnClones.java line 116: > 114: assertNotNull(certs, "JarEntry.getCertificates() returned > null for " + je.getName()); > 115: // verify that the newly returned array doesn't have the > overwrittent value > 116: assertNotNull(certs[0], "Internal certiticates array was > modified"); Typo: Suggestion: assertNotNull(certs[0], "Internal certificates array was modified"); test/jdk/java/util/jar/JarEntry/GetMethodsReturnClones.java line 149: > 147: try (JarOutputStream out = new > JarOutputStream(Files.newOutputStream(unsigned))) { > 148: out.putNextEntry(new JarEntry(ENTRY_NAME)); > 149: out.write(new byte[]{(byte) 0XCA, (byte) 0XFE, (byte) 0XBA, > (byte) 0XBE}); Although this is a nice tip of the hat to the Java class file format and its magic bytes header, I think a more "boring" "hello".getBytes(StandardCharsets.UTF_8) would also work fine here and reduce parsing on future maintainers trying to understand if the content is significant or not. test/jdk/java/util/jar/JarEntry/GetMethodsReturnClones.java line 170: > 168: private static KeyStore.PrivateKeyEntry privateKeyEntry() throws > Exception { > 169: final CertAndKeyGen gen = new CertAndKeyGen(KEY_TYPE, > SIGNATURE_ALGORITHM); > 170: gen.generate(4096); I'm curious if the algorithm / key size choices here and in similar signed jar tests have meaningful impact on test duration? EC keys are smaller and should be faster? I could test this myself, just wondering if the choices here were deliberate. test/jdk/java/util/jar/JarEntry/GetMethodsReturnClones.java line 176: > 174: } > 175: > 176: private static boolean isSignatureFileEntry(final JarEntry entry) { I think there's precedence in the JDK to call this predicate "isSignatureRelated", see `ZipFile.Source.isSignatureRelated`. Using that name may help recognition. ------------- PR Review: https://git.openjdk.org/jdk/pull/30602#pullrequestreview-4069089651 PR Review Comment: https://git.openjdk.org/jdk/pull/30602#discussion_r3045934708 PR Review Comment: https://git.openjdk.org/jdk/pull/30602#discussion_r3045918932 PR Review Comment: https://git.openjdk.org/jdk/pull/30602#discussion_r3045881034 PR Review Comment: https://git.openjdk.org/jdk/pull/30602#discussion_r3045913348 PR Review Comment: https://git.openjdk.org/jdk/pull/30602#discussion_r3045897610
