Re: webrev request: JDK-6996377

[Xuelei Fan]

In general, the source code fix looks fine to me.  A few minor comments:

1. Copyright
PKIXValidator.java
--
It is not have to, I normally update the copyright date to the present
year when I update a file.

ctortest.sh:
ConstructorTest.java:
-
When create a new source, we need to use the current year as the
copyright date.
- # Copyright (c) 2010, 2013, Oracle and/or its affiliates. ...
+ # Copyright (c) 2014, Oracle and/or its affiliates. ...

- * Copyright (c) 2010, Oracle and/or its affiliates. ...
+ * Copyright (c) 2014, Oracle and/or its affiliates. ...

2. Clean up resources allocated in the test
There is a new keystore generated in ctortest.jks.  Need to remove the
file at the end of the test, even the test cannot pass.

Thanks,
Xuelei


On 5/8/2014 3:12 AM, Jamil Nimeh wrote:
> Please review the webrev for JDK-6996377 when you get a chance.
> 
> http://cr.openjdk.java.net/~ascarpino/6996377/webrev.01/
> 
> Thank you,
> --Jamil

Re: webrev request: JDK-6996377

[Sean Mullan]

On 05/07/2014 03:12 PM, Jamil Nimeh wrote:

Please review the webrev for JDK-6996377 when you get a chance.

http://cr.openjdk.java.net/~ascarpino/6996377/webrev.01/


- PKIXValidator[130]: you can use the diamond operator to make the code 
more concise:


new HashMap<>();

- shell script tests are somewhat discouraged going forward, since they 
are harder to debug and can have various cross-platform issues, etc. Do 
you think you could try to just create a Java test? One option is to 
hard-code the certs (base64-encoded) inside the Java source code and use 
CertificateFactory to instantiate them. If you do that, you should 
include the keytool commands that you used to create the certs in 
comments so that they can be re-created later on if necessary.


--Sean

Re: webrev request: JDK-6996377

[Jamil Nimeh]

Ah, didn't know that we were moving away from the scripts.  I had 
thought about hard-coding certs, but I liked the on-the-fly generation 
approach because it kept the validity periods always within current 
time.  But it's easy to just make really long lived certs so I'll make 
that change.


I'll make the change on line 130 as well and look for any other 
instances where I'm doing that.


Thanks!

--Jamil

On 05/08/2014 06:50 AM, Sean Mullan wrote:

On 05/07/2014 03:12 PM, Jamil Nimeh wrote:

Please review the webrev for JDK-6996377 when you get a chance.

http://cr.openjdk.java.net/~ascarpino/6996377/webrev.01/


- PKIXValidator[130]: you can use the diamond operator to make the 
code more concise:


new HashMap<>();

- shell script tests are somewhat discouraged going forward, since 
they are harder to debug and can have various cross-platform issues, 
etc. Do you think you could try to just create a Java test? One option 
is to hard-code the certs (base64-encoded) inside the Java source code 
and use CertificateFactory to instantiate them. If you do that, you 
should include the keytool commands that you used to create the certs 
in comments so that they can be re-created later on if necessary.


--Sean

Re: webrev request: JDK-6996377

[Sean Mullan]

On 05/08/2014 09:55 AM, Jamil Nimeh wrote:

Ah, didn't know that we were moving away from the scripts.


See https://blogs.oracle.com/jjg/entry/shelling_tests for some more 
rationale.



 I had
thought about hard-coding certs, but I liked the on-the-fly generation
approach because it kept the validity periods always within current
time.  But it's easy to just make really long lived certs so I'll make
that change.


You can also work around the expiration issue by setting the validity 
date to a time within the validity period of the certificate. See 
PKIXParameters.setDate(). We do that a lot in other tests.


--Sean

RFR: 8028192 : Use of PKCS11-NSS provider in FIPS mode broken

[Seán Coffey]

This is more or less a backport of the 8u20 code to the 7u-dev codebase.

This update fixes the SunJSSE problem that in FIPS mode, SunJSSE does
not work because keys cannot be extracted from crypto device.

Builds and tests are good.
http://cr.openjdk.java.net/~coffeys/webrev.8028192.jdk.7udev/webrev/

Regards,
Sean.

[Update]: webrev request: JDK-6996377

[Jamil Nimeh]

Hello all,

Updated webrev to account for Sean and Xuelei's comments is here:

http://cr.openjdk.java.net/~ascarpino/6996377/webrev.02/ 



Thank you,
--Jamil

On 05/07/2014 12:12 PM, Jamil Nimeh wrote:

Please review the webrev for JDK-6996377 when you get a chance.

http://cr.openjdk.java.net/~ascarpino/6996377/webrev.01/

Thank you,
--Jamil

Re: [Update]: webrev request: JDK-6996377

[Xuelei Fan]

test/sun/security/validator/ConstructorTest.java

Missed a "," in the copyright date line.  May only need 2014 as this is
a new test.

-  * Copyright (c) 2010, 2014 Oracle and/or its affiliates. ...
+  * Copyright (c) 2010, 2014, Oracle and/or its affiliates. ...
Or
+  * Copyright (c) 2014, Oracle and/or its affiliates. ...

I would prefer the later.

Otherwise, looks fine to me.

Xuelei

On 5/9/2014 9:23 AM, Jamil Nimeh wrote:
> Hello all,
> 
> Updated webrev to account for Sean and Xuelei's comments is here:
> 
> http://cr.openjdk.java.net/~ascarpino/6996377/webrev.02/
> 
> 
> Thank you,
> --Jamil
> 
> On 05/07/2014 12:12 PM, Jamil Nimeh wrote:
>> Please review the webrev for JDK-6996377 when you get a chance.
>>
>> http://cr.openjdk.java.net/~ascarpino/6996377/webrev.01/
>>
>> Thank you,
>> --Jamil
>