CADistrustPolicy.java:
81 if (property == null || property.length() == 0) {
82 return set;
83 }
There is an ongoing effort to use property.isEmpty(). Let's go with it.
Everything else looks fine to me.
Thanks,
Max
> On Dec 11, 2018, at 3:20 AM, Sean Mullan <[email protected]> wrote:
>
> Updated webrev: http://cr.openjdk.java.net/~mullan/webrevs/8207258/webrev.02/
>
> Specific comments below ...
>
> On 12/9/18 9:22 AM, Weijun Wang wrote:
>> Hi Sean,
>> CADistrustPolicy.java:
>> 80 String[] policies = (property != null) ? property.split(",") :
>> null;
>> 81 EnumSet<CADistrustPolicy> set =
>> EnumSet.noneOf(CADistrustPolicy.class);
>> 82 for (String policy : policies) {
>> If the property is not defined, the last line above would throw an NPE.
>
> Good catch.
>
>> test/Distrust.java:
>> 118 private static final Date APRIL_17_2019 =
>> 119 Date.from(LocalDate.of(2019, 4, 17)
>> 120 .atStartOfDay(ZoneId.systemDefault())
>> 121 .toInstant());
>> Should UTC be used?
>
> Yes, fixed.
>
>> 144 testTM(getTMF("PKIX", getParams(VERISIGN_UNIVERSAL_ROOTCA)),
>> 145 loadCertificateChain("verisignuniversalrootca"),
>> !distrust);
>> What's the purpose of the above test? To make sure that even if one day we
>> removed these root CAs from cacerts then the mechanism still works as
>> expected?
>
> No, it is simply to test the use case where an application passes its own
> PKIXBuilderParameters into a TrustManager. However, it is better to just pass
> in the cacerts KeyStore in that case to the PKIXBuilderParameters
> constructor. So, I have modified the test accordingly. I also created a new
> SecurityUtils class in the test library area and added a utility method for
> returning the cacerts KeyStore. We can change other tests over time to use
> this new utility method.
>
>> 191 X509Certificate tlsServerCert = chain[0];
>> 192 chain[0] = new DistrustedTLSServerCert(tlsServerCert,
>> APRIL_17_2019);
>> Is it necessary to introduce a local variable?
>
> No, fixed.
>
> Thanks,
> Sean
>
>> No other comment.
>> Thanks,
>> Max
>>> On Dec 8, 2018, at 12:02 AM, Sean Mullan <[email protected]> wrote:
>>>
>>> Please review this change to Distrust TLS server certificates anchored by
>>> Symantec Root CAs. Although the restrictions won't kick in until after 12
>>> GA, the fix touches code that validates certificate chains, so getting this
>>> into 12 now will provide more assurance that the chain validation code
>>> continues to work properly.
>>>
>>> webrev: http://cr.openjdk.java.net/~mullan/webrevs/8207258/webrev.01/
>>> issue: https://bugs.openjdk.java.net/browse/JDK-8207258
>>>
>>> Please see the recently posted blog for more information about the
>>> restrictions that are being imposed:
>>> https://blogs.oracle.com/java-platform-group/jdk-distrusting-symantec-tls-certificates
>>>
>>> Thanks,
>>> Sean
