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

[Sean Mullan]

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
>