[jdk8u-dev] RFR: 8339560: Unaddressed comments during code review of JDK-8337664

[Francisco Ferrari Bihurriet]

On Mon, 24 Feb 2025 20:02:57 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

> I'm backporting this test-only refactoring so that future distrust changes are easier to apply. In particular, this backport eases backporting [JDK-8346587](https://bugs.openjdk.org/browse/JDK-8346587). The patch was mostly done by hand as it didn't apply cleanly.
> 
> Notable changes:
> - `/test/lib` => `/lib/security` test library changes
> - Manual renames of cert chains due to changed paths in JDK 8u
> - Applied hunks manually to Distrust.java (rename from `Entrust/Distrust.java` as well).
> 
> Testing:
> - [x] Tests in `sun/security/ssl/X509TrustManagerImpl`

Hi @jerboaa, I gave it a look, although I'm not a Reviewer.

It looks good to me, I've only made a few completely minor comments.

jdk/test/sun/security/ssl/X509TrustManagerImpl/distrust/Distrust.java line 39:

> 37: 
> 38: public final class Distrust {
> 39: 

Nit: if we want to maximize the similarity with the `jdk11u-dev` version, we could remove this blank line (`Distrust.java:39`).

`Distrust.java` diff against `jdk11u-dev` (before openjdk/jdk11u-dev at 8322c66efa9da9210eca7d6081d2a8c2d65ba4e0):

diff --git a/jdk8u-dev at e8c86cc/jdk/test/sun/security/ssl/X509TrustManagerImpl/distrust/Distrust.java b/jdk11u-dev at 7dab5fe/test/jdk/sun/security/ssl/X509TrustManagerImpl/distrust/Distrust.java
index eb8abec..18178f6 100644
--- a/jdk8u-dev at e8c86cc/jdk/test/sun/security/ssl/X509TrustManagerImpl/distrust/Distrust.java
+++ b/jdk11u-dev at 7dab5fe/test/jdk/sun/security/ssl/X509TrustManagerImpl/distrust/Distrust.java
@@ -31,10 +31,11 @@ import javax.net.ssl.*;
 import sun.security.validator.Validator;
 import sun.security.validator.ValidatorException;
 
+import jdk.test.lib.security.SecurityUtils;
+
 /**
  * Helper class that provides methods to facilitate testing of distrusted roots.
  */
-
 public final class Distrust {
 
     private static final String TEST_SRC = System.getProperty("test.src", ".");
@@ -76,6 +77,7 @@ public final class Distrust {
         for (String test : tests) {
             System.err.println("Testing " + test);
             X509Certificate[] chain = loadCertificateChain(certPath, test);
+
             for (X509TrustManager tm : tms) {
                 testTM(tm, chain, notBefore, isValid);
             }

jdk/test/sun/security/ssl/X509TrustManagerImpl/distrust/Distrust.java line 78:

> 76:         for (String test : tests) {
> 77:             System.err.println("Testing " + test);
> 78:             X509Certificate[] chain = loadCertificateChain(certPath, test);

Nit: if we want to maximize the similarity with the `jdk11u-dev` version, we could add a blank line after this one (`Distrust.java:78`).

See `Distrust.java` diff against `jdk11u-dev` (before openjdk/jdk11u-dev at 8322c66efa9da9210eca7d6081d2a8c2d65ba4e0) in the previous comment.

jdk/test/sun/security/ssl/X509TrustManagerImpl/distrust/Entrust.java line 35:

> 33:  *          Entrust roots are invalid
> 34:  * @library /lib/security
> 35:  * @modules java.base/sun.security.validator

Nit: `@modules java.base/sun.security.validator` is not needed in 8u. However, I see `@modules` occurrences in other 94 tests, are we keeping them on purpose to minimize the differences with newer releases?

NOTE: the same question applies to the next file, `Symantec.java`.

-------------

Marked as reviewed by fferrari (no project role).

PR Review: https://git.openjdk.org/jdk8u-dev/pull/626#pullrequestreview-2641545931
PR Review Comment: https://git.openjdk.org/jdk8u-dev/pull/626#discussion_r1970036865
PR Review Comment: https://git.openjdk.org/jdk8u-dev/pull/626#discussion_r1970037169
PR Review Comment: https://git.openjdk.org/jdk8u-dev/pull/626#discussion_r1970037481