RFR (12): 8207258: Distrust TLS server certificates anchored by Symantec Root CAs

[Weijun Wang]

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 <sean.mullan at oracle.com> 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 <sean.mullan at oracle.com> 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