Re: RFR 8199198: Remove unused functions in jdk.crypto.mscapi native code

[Vincent Ryan]

Hello Max,

Your change looks fine to me.
Thanks.

> On 7 Mar 2018, at 02:19, Weijun Wang  wrote:
> 
> Please take a review at
> 
>   http://cr.openjdk.java.net/~weijun/8199198/webrev.00
> 
> I just removed the 3 unused functions. They were there from the beginning 
> (2005) but had never existed in any Java class.
> 
> Thanks
> Max
> 

Re: hello, im a new contributor

[Vincent Ryan]

That all sounds fine. Let me know when your patch is ready to submit.
Thanks.


> On 1 Sep 2017, at 13:15, Philipp Kunz <philipp.k...@paratix.ch> wrote:
> 
> Hello Vincent
> 
> Thank you for sponsoring!
> So far, I have become a contributor by signing the OCA which has been 
> accepted. Dalibor Topic wrote that he can confirm and it's also here: 
> http://www.oracle.com/technetwork/community/oca-486395.html#p 
> <http://www.oracle.com/technetwork/community/oca-486395.html#p> -> Paratix 
> GmbH
> Therefore I think I have followed the steps in 
> http://openjdk.java.net/contribute/ <http://openjdk.java.net/contribute/> at 
> least the ones before actual patch submission.
> 
> Currently, I'm working on getting the existing test cases running locally. 
> Unfortunately, I started with jdk 9 and switched to 10 now. I figured these 
> commands run at least the relevant tests with 9 and hope this also applies to 
> 10:
> make run-test-tier1
> make run-test TEST="jdk/test"
> they report errors and failures but it hasn't been completed and released 
> which might explain it. If I run the tests I assume relevant for my patch
> make run-test TEST="jtreg:jdk/test/sun/security/tools/jarsigner 
> jtreg:jdk/test/java/util/jar"
> then it reports zero errors and failures which may be a good starting point.
> Do you think that sounds reasonable or do you have another suggestion how to 
> run the tests?
> 
> Next, I will add a test for JDK-6695402 before actually fixing it. As an 
> example, I'll try something in the style of 
> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java
>  
> <http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java>.
>  This way, I try to demonstrate the improvement.
> 
> I guess I have identified the following line as the cause: value = new 
> String(vb, 0, 0, vb.length);
> http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157
>  
> <http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157>
> So I'll try to remove it first including the whole four line if block.
> 
> Philipp
> 
> 
> On 01.09.2017 10:00, Vincent Ryan wrote:
>> Hello Philipp,
>> 
>> I’m happy to sponsor your fix for JDK 10. Have you followed these steps: 
>> http://openjdk.java.net/contribute/ <http://openjdk.java.net/contribute/> ?
>> 
>> Thanks.
>> 
>> 
>>> On 1 Sep 2017, at 08:58, Vincent Ryan <vincent.x.r...@oracle.com 
>>> <mailto:vincent.x.r...@oracle.com>> wrote:
>>> 
>>> Moved to security-dev
>>> 
>>> 
>>>> On 1 Sep 2017, at 08:28, Philipp Kunz <philipp.k...@paratix.ch 
>>>> <mailto:philipp.k...@paratix.ch>> wrote:
>>>> 
>>>> Hello everyone
>>>> 
>>>> I have been developing with Java for around 17 years now and when I 
>>>> encountered some bug I decided to attempt to fix it: 
>>>> https://bugs.openjdk.java.net/browse/JDK-6695402 
>>>> <https://bugs.openjdk.java.net/browse/JDK-6695402>. This also looks like 
>>>> it may not be too big a piece for a first contribution.
>>>> 
>>>> I read through quite some guides and all kinds of documents but could not 
>>>> yet help myself with the following questions:
>>>> 
>>>> May I login to jira to add comments to bugs? If so, how would I request or 
>>>> receive credentials? Or are mailing lists preferred?
>>>> 
>>>> Another question is whether I should apply it to jdk9, but it may be too 
>>>> late now, or to jdk10, and backporting can be considered later. Probably 
>>>> it wouldn't even make much a difference for the patch itself.
>>>> 
>>>> One more question I have is how or where to find the sources from before 
>>>> migration to mercurial. Because some lines of code I intend to change go 
>>>> back farther and in the history I find only 'initial commit'. With such a 
>>>> history I might be able better to understand why it's there and prevent to 
>>>> make the same mistake again.
>>>> 
>>>> I guess the appropriate mailing list for above mentioned bug is 
>>>> security-dev. Is it correct that I can send a patch there and just hope 
>>>> for some sponsor to pick it up? Of course I'd be glad if some sponsor 
>>>> would contact me and maybe provide some assistance or if someone would 
>>>> confirm that sending a patch to the mailing list is the right way to find 
>>>> a sponsor.
>>>> 
>>>> Philipp Kunz
>>> 
>> 
> 

Re: hello, im a new contributor

[Vincent Ryan]

Hello Philipp,

I’m happy to sponsor your fix for JDK 10. Have you followed these steps: 
http://openjdk.java.net/contribute/ <http://openjdk.java.net/contribute/> ?

Thanks.


> On 1 Sep 2017, at 08:58, Vincent Ryan <vincent.x.r...@oracle.com> wrote:
> 
> Moved to security-dev
> 
> 
>> On 1 Sep 2017, at 08:28, Philipp Kunz <philipp.k...@paratix.ch> wrote:
>> 
>> Hello everyone
>> 
>> I have been developing with Java for around 17 years now and when I 
>> encountered some bug I decided to attempt to fix it: 
>> https://bugs.openjdk.java.net/browse/JDK-6695402. This also looks like it 
>> may not be too big a piece for a first contribution.
>> 
>> I read through quite some guides and all kinds of documents but could not 
>> yet help myself with the following questions:
>> 
>> May I login to jira to add comments to bugs? If so, how would I request or 
>> receive credentials? Or are mailing lists preferred?
>> 
>> Another question is whether I should apply it to jdk9, but it may be too 
>> late now, or to jdk10, and backporting can be considered later. Probably it 
>> wouldn't even make much a difference for the patch itself.
>> 
>> One more question I have is how or where to find the sources from before 
>> migration to mercurial. Because some lines of code I intend to change go 
>> back farther and in the history I find only 'initial commit'. With such a 
>> history I might be able better to understand why it's there and prevent to 
>> make the same mistake again.
>> 
>> I guess the appropriate mailing list for above mentioned bug is 
>> security-dev. Is it correct that I can send a patch there and just hope for 
>> some sponsor to pick it up? Of course I'd be glad if some sponsor would 
>> contact me and maybe provide some assistance or if someone would confirm 
>> that sending a patch to the mailing list is the right way to find a sponsor.
>> 
>> Philipp Kunz
> 

Re: hello, im a new contributor

[Vincent Ryan]

Moved to security-dev


> On 1 Sep 2017, at 08:28, Philipp Kunz  wrote:
> 
> Hello everyone
> 
> I have been developing with Java for around 17 years now and when I 
> encountered some bug I decided to attempt to fix it: 
> https://bugs.openjdk.java.net/browse/JDK-6695402. This also looks like it may 
> not be too big a piece for a first contribution.
> 
> I read through quite some guides and all kinds of documents but could not yet 
> help myself with the following questions:
> 
> May I login to jira to add comments to bugs? If so, how would I request or 
> receive credentials? Or are mailing lists preferred?
> 
> Another question is whether I should apply it to jdk9, but it may be too late 
> now, or to jdk10, and backporting can be considered later. Probably it 
> wouldn't even make much a difference for the patch itself.
> 
> One more question I have is how or where to find the sources from before 
> migration to mercurial. Because some lines of code I intend to change go back 
> farther and in the history I find only 'initial commit'. With such a history 
> I might be able better to understand why it's there and prevent to make the 
> same mistake again.
> 
> I guess the appropriate mailing list for above mentioned bug is security-dev. 
> Is it correct that I can send a patch there and just hope for some sponsor to 
> pick it up? Of course I'd be glad if some sponsor would contact me and maybe 
> provide some assistance or if someone would confirm that sending a patch to 
> the mailing list is the right way to find a sponsor.
> 
> Philipp Kunz

[10] RFR 8173181: Empty string alias in KeyStore throws StringIndexOutOfBoundsException for getEntry()

[Vincent Ryan]

Please review this fix to correct support for an empty string alias in PKCS12 
keystores.
Thanks.

Bug: https://bugs.openjdk.java.net/browse/JDK-8173181 

Webrev: http://cr.openjdk.java.net/~vinnie/8173181/webrev.00/ 

Re: RFR: JDK-8159544: Remove deprecated classes in com.sun.security.auth.**

[Vincent Ryan]

Your changes look fine to me. Thanks.


> On 17 Aug 2017, at 20:08, Sean Mullan  wrote:
> 
> Please review this JDK 10 change to remove the deprecated classes in 
> com.sun.security.auth.** that have been previously marked with 
> forRemoval=true in JDK 9.
> 
> webrev: http://cr.openjdk.java.net/~mullan/webrevs/8159544/
> 
> I have also copied Jan for reviewing a change in langtools, and also 
> build-dev for a change to one of the JDK Makefiles.
> 
> Thanks,
> Sean

Re: RFR[10] 8185620: MSCAPI test leaves too many entries in keystore

[Vincent Ryan]

Your fix looks good to me. Thanks.

> On 14 Aug 2017, at 03:44, sha.ji...@oracle.com wrote:
> 
> Please review this fix.
> Thanks!
> 
> John Jiang
> 
> 
> On 07/08/2017 13:03, sha.ji...@oracle.com wrote:
>> Hi,
>> Test sun/security/mscapi/SmallPrimeExponentP.java adds some entries into 
>> Windows-MY keystore, but it doesn't delete them before exit.
>> Please review this patch for the above issue.
>> 
>> Webrev: http://cr.openjdk.java.net/~jjiang/8185620/webrev.00/
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8185620
>> 
>> Best regards,
>> John Jiang
>> 
>> 
> 

Re: RFR 8182999: SunEC throws ProviderException on invalid curves

[Vincent Ryan]

+1

> On 12 Jul 2017, at 15:51, Adam Petcher <adam.petc...@oracle.com> wrote:
> 
> I made a minor tweak to the test. I realized that the test will still pass if 
> the curve becomes supported in the future. I want the test to fail in this 
> case because it would no longer be testing an unsupported curve.
> 
> latest webrev: http://cr.openjdk.java.net/~apetcher/8182999/webrev.02/ 
> <http://cr.openjdk.java.net/~apetcher/8182999/webrev.02/>
> On 7/12/2017 10:42 AM, Vincent Ryan wrote:
>> Looks fine to me too.
>> 
>> We should investigate how best to support similar behaviour for the 
>> SunPKCS11 provider.
>> To track this issue I’ve filed a related bug 8184290 
>> <https://bugs.openjdk.java.net/browse/JDK-8184290>: SunPKCS11 throws 
>> ProviderException for unsupported curves
>> 
>> 
>> 
>>> On 10 Jul 2017, at 17:03, Seán Coffey <sean.cof...@oracle.com 
>>> <mailto:sean.cof...@oracle.com>> wrote:
>>> 
>>> Thanks for the update! Looks fine to me.
>>> 
>>> Regards,
>>> Sean.
>>> 
>>> On 10/07/17 16:13, Adam Petcher wrote:
>>>> New webrev: http://cr.openjdk.java.net/~apetcher/8182999/webrev.01/ 
>>>> <http://cr.openjdk.java.net/%7Eapetcher/8182999/webrev.01/>
>>>> 
>>>> Yes, this is a good idea. I made this work by printing out the value from 
>>>> AlgorithmParameters.toString(), so hopefully that means you should always 
>>>> get a useful string. At the moment (with SunEC AlgorithmParameters), the 
>>>> string prints the friendly name followed by the OID:
>>>> 
>>>> Unsupported curve: brainpoolP256r1 (1.3.36.3.3.2.8.1.1.7)
>>>> 
>>>> On 7/7/2017 4:12 PM, Seán Coffey wrote:
>>>>> Adam,
>>>>> 
>>>>> would it be useful to get the curve name in the new exception ? I think 
>>>>> it would help with future debugging. Line 96 already gets the curve name 
>>>>> if we're dealing with ECGenParameterSpec instance. I think the same 
>>>>> approach could be applied to your new code.
>>>>> 
>>>>> Regards,
>>>>> Sean.
>>>>> 
>>>>> 
>>>>> On 07/07/2017 19:59, Adam Petcher wrote:
>>>>>> This is a bug fix related to invalid curves in the SunEC provider. 
>>>>>> During ECKeyPairGenerator.initialize(), the provider only checks whether 
>>>>>> the curve is known, but it doesn't check whether the curve is actually 
>>>>>> supported by the native code. So the call to generateKeyPair() can fail 
>>>>>> in the native code and throw a ProviderException. This change adds a new 
>>>>>> native method to check whether the curve is supported. This method is 
>>>>>> called by initialize(), which will set the state to uninitialized and 
>>>>>> throw the expected exception when the curve is not supported.
>>>>>> 
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8182999 
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8182999>
>>>>>> Webrev: http://cr.openjdk.java.net/~apetcher/8182999/webrev.00/ 
>>>>>> <http://cr.openjdk.java.net/%7Eapetcher/8182999/webrev.00/>
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 

Re: RFR 8182999: SunEC throws ProviderException on invalid curves

[Vincent Ryan]

Looks fine to me too.

We should investigate how best to support similar behaviour for the SunPKCS11 
provider.
To track this issue I’ve filed a related bug 8184290 
: SunPKCS11 throws 
ProviderException for unsupported curves



> On 10 Jul 2017, at 17:03, Seán Coffey  wrote:
> 
> Thanks for the update! Looks fine to me.
> 
> Regards,
> Sean.
> 
> On 10/07/17 16:13, Adam Petcher wrote:
>> New webrev: http://cr.openjdk.java.net/~apetcher/8182999/webrev.01/
>> 
>> Yes, this is a good idea. I made this work by printing out the value from 
>> AlgorithmParameters.toString(), so hopefully that means you should always 
>> get a useful string. At the moment (with SunEC AlgorithmParameters), the 
>> string prints the friendly name followed by the OID:
>> 
>> Unsupported curve: brainpoolP256r1 (1.3.36.3.3.2.8.1.1.7)
>> 
>> On 7/7/2017 4:12 PM, Seán Coffey wrote:
>>> Adam,
>>> 
>>> would it be useful to get the curve name in the new exception ? I think it 
>>> would help with future debugging. Line 96 already gets the curve name if 
>>> we're dealing with ECGenParameterSpec instance. I think the same approach 
>>> could be applied to your new code.
>>> 
>>> Regards,
>>> Sean.
>>> 
>>> 
>>> On 07/07/2017 19:59, Adam Petcher wrote:
 This is a bug fix related to invalid curves in the SunEC provider. During 
 ECKeyPairGenerator.initialize(), the provider only checks whether the 
 curve is known, but it doesn't check whether the curve is actually 
 supported by the native code. So the call to generateKeyPair() can fail in 
 the native code and throw a ProviderException. This change adds a new 
 native method to check whether the curve is supported. This method is 
 called by initialize(), which will set the state to uninitialized and 
 throw the expected exception when the curve is not supported.
 
 JBS: https://bugs.openjdk.java.net/browse/JDK-8182999
 Webrev: http://cr.openjdk.java.net/~apetcher/8182999/webrev.00/
 
>>> 
>> 
> 

Re: [10] RFR 8183509: keytool should not allow multiple commands

[Vincent Ryan]

Looks fine to me.

> On 7 Jul 2017, at 15:55, Weijun Wang  wrote:
> 
> Updated at
> 
>   http://cr.openjdk.java.net/~weijun/8183509/webrev.01/
> 
> Changes:
> 
>   http://cr.openjdk.java.net/~weijun/8183509/webrev.01/interdiff.patch.html
> 
> Thanks
> Max
> 
>> On Jul 7, 2017, at 8:00 PM, Sean Mullan  wrote:
>> 
>> I would use "specified" instead of "provided" since that word is used more 
>> commonly in the keytool warnings.
>> 
>> Thanks,
>> Sean
>> 
>> On 7/7/17 3:58 AM, Weijun Wang wrote:
>>> Please take a look at
>>> http://cr.openjdk.java.net/~weijun/8183509/webrev.00/
>>> Multiple commands on the same keytool command line is now an error. 
>>> Specifying a single-valued option multiple times will trigger a warning.
>>> Thanks
>>> Max
> 

[9] RFR 8181978: Keystore probing mechanism fails for large PKCS12 keystores

[Vincent Ryan]

Martin has reported a serious regression involving PKCS12 keystores in JDK 9.
It affects large PKCS12 keystores loaded using the new 
KeyStore.getInstance(File, xxx) method.
The error is due to a typo in the masks used by the keystore type detection 
mechanism.

Bug: https://bugs.openjdk.java.net/browse/JDK-8181978
Webrev: http://cr.openjdk.java.net/~vinnie/8181978/webrev.00/

Re: RFR 8181841: A TSA server returns timestamp with precision higher than milliseconds

[Vincent Ryan]

This approach looks fine to me given the limitation on the precision of Date.
Just one issue: why remove the upper bound at l.277 in DerInputBuffer.java

Thanks.


> On 12 Jun 2017, at 05:22, Weijun Wang  wrote:
> 
> Please review this fix at
> 
>   http://cr.openjdk.java.net/~weijun/8181841/webrev.00
> 
> So I just ignore the extra digits. Do you think this is OK? It does mean 
> different encodings might equal to each other.
> 
> Thanks
> Max

Re: RFR 8172244: AIOOBE in KeyStore.getCertificateAlias on Windows

[Vincent Ryan]

Your fix looks fine to me.
Thanks.

> On 22 May 2017, at 20:57, Adam Petcher  wrote:
> 
> This is a bug fix related to keys without certificates in the Windows key 
> store. When a key doesn't have a certificate, the native code will set the 
> list of certificates to an empty list. Some of the Java code for the MSCAPI 
> provider doesn't handle this case correctly and throws an AIOOBE. The 
> regression test reproduces the same circumstances in the Java code by 
> bypassing some checks in the KeyStore interface, allowing us to test this 
> without upsetting the test environment.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8172244
> Webrev: http://cr.openjdk.java.net/~apetcher/8172244/webrev.00/
> 

Re: RFR 8176457: Add verbose option to java.security.debug

[Vincent Ryan]

Your change looks fine to me Tony.
Thanks.


> On 3 May 2017, at 01:13, Anthony Scarpino  wrote:
> 
> I need a review for adding the verbose option to debugging so that the output 
> is not overwhelmed with unnecessary info.
> 
> http://cr.openjdk.java.net/~ascarpino/8176457/webrev/
> 
> Tony
> 

Re: [10] RFR: 8175029: StackOverflowError in X509CRL and X509Certificate.verify(PublicKey, Provider)

[Vincent Ryan]

This fix looks fine to me.
Thanks.


> On 31 Mar 2017, at 16:34, Sean Mullan  wrote:
> 
> New webrev uploaded to address your comments: 
> http://cr.openjdk.java.net/~mullan/webrevs/8175029/webrev.01/
> 
> --Sean
> 
> On 3/31/17 9:11 AM, Sean Mullan wrote:
>> On 3/31/17 9:08 AM, Weijun Wang wrote:
>>> Can we just move the code you just added into the public API directly?
>>> Looks like it does not reference any internal classes.
>> 
>> Good point, that should work! I'll try that and send out an updated
>> webrev later.
>> 
>> --Sean
>> 
>>> 
>>> --Max
>>> 
>>> On 03/31/2017 08:06 PM, Sean Mullan wrote:
 Please review this fix for a StackOverflowError caused by the default
 implementation of verify(PublicKey, Provider).
 
 bug: https://bugs.openjdk.java.net/browse/JDK-8175029
 
 webrev: http://cr.openjdk.java.net/~mullan/webrevs/8175029/webrev.00/
 
 Thanks,
 Sean

Re: RFR: JDK-8176503: Disable SHA-1 TLS Server Certificates

[Vincent Ryan]

That change looks fine to me.
Thanks.


> On 13 Mar 2017, at 14:57, Sean Mullan  wrote:
> 
> Please review this configuration change to disable SHA-1 TLS server 
> certificates by default in JDK 9. In order to be disabled, the certificates 
> must chain back to trusted root certificate in the cacerts keystore that has 
> a " [jdk]" attribute appended to their alias name.
> 
> --Sean
> 
> diff --git a/src/java.base/share/conf/security/java.security 
> b/src/java.base/share/conf/security/java.security
> --- a/src/java.base/share/conf/security/java.security
> +++ b/src/java.base/share/conf/security/java.security
> @@ -598,8 +598,8 @@
> #   jdk.certpath.disabledAlgorithms=MD2, DSA, RSA keySize < 2048
> #
> #
> -jdk.certpath.disabledAlgorithms=MD2, MD5, RSA keySize < 1024, \
> -DSA keySize < 1024, EC keySize < 224
> +jdk.certpath.disabledAlgorithms=MD2, MD5, SHA1 jdkCA & usage TLSServer, \
> +RSA keySize < 1024, DSA keySize < 1024, EC keySize < 224
> 
> #
> # Algorithm restrictions for signed JAR files

Re: RFR 8175846: Provide javadoc descriptions for jdk.policytool and jdk.crypto.* modules

[Vincent Ryan]

Your comments look fine to me.


> On 7 Mar 2017, at 14:44, Weijun Wang  wrote:
> 
> Ping again.
> 
> On 03/01/2017 09:02 AM, Weijun Wang wrote:
>> Please review the patch below. I don't know if all those jdk.crypto.*
>> modules will be listed in the doc, but it's nice to include a comment.
>> 
>> Thanks
>> Max
>> 
>> diff --git a/src/jdk.policytool/share/classes/module-info.java
>> b/src/jdk.policytool/share/classes/module-info.java
>> --- a/src/jdk.policytool/share/classes/module-info.java
>> +++ b/src/jdk.policytool/share/classes/module-info.java
>> @@ -23,6 +23,14 @@
>>  * questions.
>>  */
>> 
>> +/**
>> + * GUI tool for managing policy files.
>> + *
>> + * @since 9
>> + * @deprecated The policytool tool has been deprecated and
>> + * is planned to be removed in a future release.
>> + */
>> +@Deprecated
>> module jdk.policytool {
>> requires java.desktop;
>> requires java.logging;
>> 
>> diff --git a/src/jdk.crypto.cryptoki/share/classes/module-info.java
>> b/src/jdk.crypto.cryptoki/share/classes/module-info.java
>> --- a/src/jdk.crypto.cryptoki/share/classes/module-info.java
>> +++ b/src/jdk.crypto.cryptoki/share/classes/module-info.java
>> @@ -23,6 +23,11 @@
>>  * questions.
>>  */
>> 
>> +/**
>> + * The SunPKCS11 security provider.
>> + *
>> + * @since 9
>> + */
>> module jdk.crypto.cryptoki {
>> // Depends on SunEC provider for EC related functionality
>> requires jdk.crypto.ec;
>> diff --git a/src/jdk.crypto.ec/share/classes/module-info.java
>> b/src/jdk.crypto.ec/share/classes/module-info.java
>> --- a/src/jdk.crypto.ec/share/classes/module-info.java
>> +++ b/src/jdk.crypto.ec/share/classes/module-info.java
>> @@ -23,6 +23,11 @@
>>  * questions.
>>  */
>> 
>> +/**
>> + * The SunEC security provider.
>> + *
>> + * @since 9
>> + */
>> module jdk.crypto.ec {
>> provides java.security.Provider with sun.security.ec.SunEC;
>> }
>> diff --git a/src/jdk.crypto.mscapi/windows/classes/module-info.java
>> b/src/jdk.crypto.mscapi/windows/classes/module-info.java
>> --- a/src/jdk.crypto.mscapi/windows/classes/module-info.java
>> +++ b/src/jdk.crypto.mscapi/windows/classes/module-info.java
>> @@ -23,6 +23,11 @@
>>  * questions.
>>  */
>> 
>> +/**
>> + * The SunMSCAPI security provider.
>> + *
>> + * @since 9
>> + */
>> module jdk.crypto.mscapi {
>> provides java.security.Provider with sun.security.mscapi.SunMSCAPI;
>> }
>> diff --git a/src/jdk.crypto.ucrypto/solaris/classes/module-info.java
>> b/src/jdk.crypto.ucrypto/solaris/classes/module-info.java
>> --- a/src/jdk.crypto.ucrypto/solaris/classes/module-info.java
>> +++ b/src/jdk.crypto.ucrypto/solaris/classes/module-info.java
>> @@ -23,6 +23,11 @@
>>  * questions.
>>  */
>> 
>> +/**
>> + * The OracleUCrypto security provider.
>> + *
>> + * @since 9
>> + */
>> module jdk.crypto.ucrypto {
>> provides java.security.Provider with
>> com.oracle.security.ucrypto.UcryptoProvider;
>> }
>> 
>> Thanks
>> Max

Re: RFR: 8174837: Add "since=9" to deprecated ContentSigner and ContentSignerParameters classes

[Vincent Ryan]

Your fix looks fine. Minor nit with the copyright header in ContentSigner.java: 
it’s missing a comma after ‘2017’.
Thanks.


> On 13 Feb 2017, at 16:25, Sean Mullan <sean.mul...@oracle.com> wrote:
> 
> Could I get a quick code review for this simple fix for 
> https://bugs.openjdk.java.net/browse/JDK-8174837?:
> 
> diff --git 
> a/src/jdk.jartool/share/classes/com/sun/jarsigner/ContentSigner.java 
> b/src/jdk.jartool/share/classes/com/sun/jarsigner/ContentSigner.java
> --- a/src/jdk.jartool/share/classes/com/sun/jarsigner/ContentSigner.java
> +++ b/src/jdk.jartool/share/classes/com/sun/jarsigner/ContentSigner.java
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2015, 2017 Oracle and/or its affiliates. All rights 
> reserved.
>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>  *
>  * This code is free software; you can redistribute it and/or modify it
> @@ -38,7 +38,7 @@
>  * @deprecated This class has been deprecated.
>  */
> 
> -@Deprecated
> +@Deprecated(since="9")
> public abstract class ContentSigner {
> 
> /**
> diff --git 
> a/src/jdk.jartool/share/classes/com/sun/jarsigner/ContentSignerParameters.java
>  
> b/src/jdk.jartool/share/classes/com/sun/jarsigner/ContentSignerParameters.java
> --- 
> a/src/jdk.jartool/share/classes/com/sun/jarsigner/ContentSignerParameters.java
> +++ 
> b/src/jdk.jartool/share/classes/com/sun/jarsigner/ContentSignerParameters.java
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2003, 2015, Oracle and/or its affiliates. All rights 
> reserved.
> + * Copyright (c) 2003, 2017, Oracle and/or its affiliates. All rights 
> reserved.
>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>  *
>  * This code is free software; you can redistribute it and/or modify it
> @@ -36,7 +36,7 @@
>  * @author Vincent Ryan
>  * @deprecated This class has been deprecated.
>  */
> -@Deprecated
> +@Deprecated(since="9")
> public interface ContentSignerParameters {

[9] 8173956: KeyStore regression due to default keystore being changed to PKCS12

[Vincent Ryan]

Please review this fix to correct support for mixed-case aliases in PKCS12 
keystores.
Thanks.

Webrev: http://cr.openjdk.java.net/~vinnie/8173956/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8173956

Re: Feedback on SSLEngine.setHandshakeApplicationProtocolSelector()

[Vincent Ryan]

That’s good news. Thanks for validating the new mechanism in Jetty.


> On 12 Jan 2017, at 19:04, Simone Bordet  wrote:
> 
> Hi,
> 
> On Wed, Jan 11, 2017 at 5:57 PM, Simone Bordet  
> wrote:
>> Hi,
>> 
>> I just wanted to report that I have implemented the new mechanism
>> provided by SSLEngine.setHandshakeApplicationProtocolSelector() in
>> Jetty, and it works well in a much much simpler way.
>> 
>> The amount of code required now is trivial (a couple of lines), there
>> is no need to parse the ClientHello, and we basically tap into the
>> existing mechanism that Jetty had.
>> No big surprises here since the new JDK mechanism is very similar to
>> what Jetty has for JDK 8.
> 
> For the record, I have implemented also the client-side mechanism
> using the JDK 9 APIs.
> Looks simple enough and works fine.
> 
> -- 
> Simone Bordet
> http://bordet.blogspot.com
> ---
> Finally, no matter how good the architecture and design are,
> to deliver bug-free software with optimal performance and reliability,
> the implementation technique must be flawless.   Victoria Livschitz

Re: [9] RFR 8171443: (spec) An ALPN callback function may also ignore ALPN

[Vincent Ryan]

Hello Simone,

Throwing an exception is certainly another option but we chose these 3 specific 
return values for the callback
so that they match the existing behaviour of SSLEngine.getApplicationProtocol / 
SSLEngine.getHandshakeApplicationProtocol
and the SSLSocket equivalents.

http://download.java.net/java/jdk9/docs/api/javax/net/ssl/SSLEngine.html#getApplicationProtocol--
http://download.java.net/java/jdk9/docs/api/javax/net/ssl/SSLEngine.html#getHandshakeApplicationProtocol--
http://download.java.net/java/jdk9/docs/api/javax/net/ssl/SSLSocket.html#getApplicationProtocol--
http://download.java.net/java/jdk9/docs/api/javax/net/ssl/SSLSocket.html#getHandshakeApplicationProtocol--

BTW if a runtime exception does get thrown from the callback then it gets 
handled further up the call stack
where a TLS fatal alert will be returned.

I don’t think it is worth changing all these methods to throw an exception 
instead.
I’d prefer to keep the current behaviour.

Thanks.


> On 22 Dec 2016, at 11:06, Simone Bordet <simone.bor...@gmail.com> wrote:
> 
> Vincent,
> 
> On Thu, Dec 22, 2016 at 11:38 AM, Vincent Ryan
> <vincent.x.r...@oracle.com> wrote:
>> Please review this spec change to allow an ALPN callback function to also 
>> disable ALPN usage
>> and return no ALPN extension value during a TLS handshake.
> 
> As I understand it, the callback needs to convey 3 results:
> 1. a success -> non empty string
> 2. a failure -> null
> 3. no action -> empty string
> 
> I wonder if it is better to convey the failure by throwing an exception ?
> This would also match the case where the implementation of the
> function, for any reason, throws an unexpected exception such as NPE
> or ClassCastException, etc.
> I expect the SSLEngine implementation to catch Throwable from the
> invocation of the callback and send back a TLS close message with code
> 120.
> 
> If the failure case is conveyed with an exception, then only 2 cases
> remain, and then null can be used to signal "no action" ?
> 
> The SSLEngine implementation should also check that the String
> returned is among those sent by the other peer, so an empty string is
> as invalid as the string "no_proto" - hence the choice of null to
> signal "no action".
> 
> Makes sense ?
> 
> Thanks !
> 
> -- 
> Simone Bordet
> http://bordet.blogspot.com
> ---
> Finally, no matter how good the architecture and design are,
> to deliver bug-free software with optimal performance and reliability,
> the implementation technique must be flawless.   Victoria Livschitz

[9] RFR 8171443: (spec) An ALPN callback function may also ignore ALPN

[Vincent Ryan]

Please review this spec change to allow an ALPN callback function to also 
disable ALPN usage
and return no ALPN extension value during a TLS handshake.

Thanks.


Bug: https://bugs.openjdk.java.net/browse/JDK-8171443
Webrev:  http://cr.openjdk.java.net/~vinnie/8171443/webrev.01/

Re: [9] RFR 8170282: Enable ALPN parameters to be supplied during the TLS handshake

[Vincent Ryan]

I’ve made the change to use activated() rather than started() in SSLEngineImpl 
and SSLSocketImpl
and also the suggestions for ServerHandshaker.

I’ve also updated the tests to check 
SSLEngine/SSLSocket.getHandshakeApplicationProtocolSelector.
All tests pass.

Thanks for all the review comments.



> On 16 Dec 2016, at 01:15, Bradford Wetmore <bradford.wetm...@oracle.com> 
> wrote:
> 
> (Xuelei, question for you below)
> 
> Hi Vinnie,
> 
> There is no test yet for SSLEngine/SSLSocket:
> 
>public BiFunction<SSLEngine, List, String>
>getHandshakeApplicationProtocolSelector() {
> 
> Tests are passing at 100% with latest jdk.patch.
> 
> 
> SSLSocketImpl.java
> ==
> 2672:  Sorry, but I think what you want here is:
> 
> if ((handshaker != null) && !handshaker.started()) {
> ->
> if ((handshaker != null) && !handshaker.activated()) {
> 
> Xuelei can confirm.  BTW, I just filed:
> 
>8171337: Check for
> correct SSLEngineImpl/SSLSocketImpl.setSSLParameters
> handshaker update method
> 
> as I think setSSLParameters may be using the wrong method.
> 
> 
> SSLEngineImpl.java
> ==
> 2275:  I misspoke earlier today.  Please add a similar change that you made 
> in SSLSocket (2671-2674).
> 
>if ((handshaker != null) && !handshaker.activated()) {
> 
> 
> ServerHandshaker.java
> =
> 536:  Minor nit:  suggest renaming hasCallback to something a little more 
> descriptive.  By the time you drop 400 lines, you may have forgotten the 
> variable meaning.  hasAPCallback?
> 
> 535:  A comments describing your current logic would be nice.
> 
>   if (there is a callback) {
>  use the callback
>   } else {
>  use the list
>   }
> 
> Rest looks good!  Thanks.
> 
> Brad
> 
> 
> 
> On 12/15/2016 4:39 PM, Vincent Ryan wrote:
>> Thanks Brad for those review comments.
>> 
>> I’ve make some implementation changes and updated the existing ALPN tests.
>> No public API changes.
>> 
>> A new webrev is available at:
>> http://cr.openjdk.java.net/~vinnie/8170282/webrev.02/
>> 
>> 
>> 
>>> On 9 Dec 2016, at 23:27, Bradford Wetmore <bradford.wetm...@oracle.com
>>> <mailto:bradford.wetm...@oracle.com>> wrote:
>>> 
>>> API looks good.
>>> 
>>> SSLEngineImpl/SSLSocketImpl.java
>>> 
>>> 212/229:  I might suggest a more descriptive variable name, like
>>> applicationSelector.  "selector" is a bit ambiguous.
>>> 
>>> 450/1379:
>>> getHandshakeApplicationProtocolSelector());
>>> ->
>>> selector);
>>> 
>>> Xuelei wrote:
>>> 
>>> > This method would work in server side only.  You mention this point
>>> > in the apiNote part.  I'd like to spec this point in the beginning
>>> > part.
>>> 
>>> If you do something like this, then you need to be careful to mention
>>> something like "application protocols such as ALPN would do this on
>>> the server side..."  NPN is the reverse, where the server offers the
>>> strings, and the client selects.
>>> 
>>> > and application developer know what kind of information can be
>>> > retrieved from the handshake session reliably.
>>> 
>>> Whatever you put in here, make sure it can be changed later in case we
>>> are able revisit the selection mechanism.
>>> 
>>> > The current application protocol selection scenarios looks like:
>>> 
>>> In my previous response, I suggested a different approach where
>>> everything ALPN is done together.  I think it may be similar to your N1-4.
>>> 
>>> I look forward to the ServerHandshaker change next week.
>>> 
>>> Brad
>>> 
>>> 
>>> On 12/9/2016 1:08 PM, Vincent Ryan wrote:
>>>> Thanks for your detailed review Brad. I’ve generated a new webrev:
>>>>   http://cr.openjdk.java.net/~vinnie/8170282/webrev.01/
>>>> 
>>>> 
>>>> 
>>>>> On 9 Dec 2016, at 01:34, Bradford Wetmore
>>>>> <bradford.wetm...@oracle.com <mailto:bradford.wetm...@oracle.com>>
>>>>> wrote:
>>>>> 
>>>>> 
>>>>> Hi Vinnie,
>>>>> 
>>>>> On 12/8/2016 2:18 PM, Vincent Ryan wrote:
>>>>>> The Java Servlet Expect Group reported that they have identified a
>>>

Re: [9] RFR 8170282: Enable ALPN parameters to be supplied during the TLS handshake

[Vincent Ryan]

Thanks Brad for those review comments.

I’ve make some implementation changes and updated the existing ALPN tests.
No public API changes.

A new webrev is available at:
http://cr.openjdk.java.net/~vinnie/8170282/webrev.02/ 
<http://cr.openjdk.java.net/~vinnie/8170282/webrev.02/>



> On 9 Dec 2016, at 23:27, Bradford Wetmore <bradford.wetm...@oracle.com> wrote:
> 
> API looks good.
> 
> SSLEngineImpl/SSLSocketImpl.java
> 
> 212/229:  I might suggest a more descriptive variable name, like 
> applicationSelector.  "selector" is a bit ambiguous.
> 
> 450/1379:
> getHandshakeApplicationProtocolSelector());
> ->
> selector);
> 
> Xuelei wrote:
> 
> > This method would work in server side only.  You mention this point
> > in the apiNote part.  I'd like to spec this point in the beginning
> > part.
> 
> If you do something like this, then you need to be careful to mention 
> something like "application protocols such as ALPN would do this on the 
> server side..."  NPN is the reverse, where the server offers the strings, and 
> the client selects.
> 
> > and application developer know what kind of information can be
> > retrieved from the handshake session reliably.
> 
> Whatever you put in here, make sure it can be changed later in case we are 
> able revisit the selection mechanism.
> 
> > The current application protocol selection scenarios looks like:
> 
> In my previous response, I suggested a different approach where everything 
> ALPN is done together.  I think it may be similar to your N1-4.
> 
> I look forward to the ServerHandshaker change next week.
> 
> Brad
> 
> 
> On 12/9/2016 1:08 PM, Vincent Ryan wrote:
>> Thanks for your detailed review Brad. I’ve generated a new webrev:
>>http://cr.openjdk.java.net/~vinnie/8170282/webrev.01/
>> 
>> 
>> 
>>> On 9 Dec 2016, at 01:34, Bradford Wetmore <bradford.wetm...@oracle.com> 
>>> wrote:
>>> 
>>> 
>>> Hi Vinnie,
>>> 
>>> On 12/8/2016 2:18 PM, Vincent Ryan wrote:
>>>> The Java Servlet Expect Group reported that they have identified a 
>>>> specific HTTP2 server use-case that cannot
>>>> be easily addressed using the existing ALPN APIs.
>>>> 
>>>> This changeset fixes that problem. It supports a new callback mechanism to 
>>>> allow TLS server applications
>>>> to set an application protocol during the TLS handshake. Specifically it 
>>>> allows the cipher suite chosen by the
>>>> TLS protocol implementation to be examined by the TLS server application 
>>>> before it sets the application protocol.
>>>> Additional TLS parameters are also available for inspection in the 
>>>> callback function.
>>>> 
>>>> This new mechanism is available only to TLS server applications. TLS 
>>>> clients will continue to use the existing ALPN APIs.
>>> 
>>> Technically, the API could be used for NPN (though it's pretty much dead), 
>>> so that would be a listing the options on the server side, and selection on 
>>> client side.
>>> 
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170282
>>>> Webrev: http://cr.openjdk.java.net/~vinnie/8170282/webrev.00/
>>> 
>>> SSLEngineImpl.java/SSLSocketImpl.java
>>> =
>>> Please use the standard handshaker initialization pattern where the 
>>> Handshaker is initialized with all of the data/mechanisms needed for a 
>>> complete handshake.  This will will ensure consistent handshake results and 
>>> avoid potential strange race conditions.  (There's some corresponding API 
>>> comments below.)
>>> 
>>> You would register your callback after the 
>>> handshaker.setApplicationProtocols() calls at (currently) line 444 in the 
>>> SSLEngineImpl code.
>>> 
>>> 
>>> SSLEngine.java/SSLSocket.java
>>> =
>>> I would suggest putting an introduction to this addition in the class 
>>> description section, that application values can be set using 
>>> SSLParameters.setApplication...() and selected with the default algorithm, 
>>> or that a more accurate selection mechanism can be created by registering 
>>> the callback that could look at any Handshake in progress and make 
>>> appropriate decisions.
>>> 
>>> 1339:
>>> Registers the callback function that selects an application protocol
>>> value during the 

Re: [9] RFR 8170282: Enable ALPN parameters to be supplied during the TLS handshake

[Vincent Ryan]

Thanks for your detailed review Brad. I’ve generated a new webrev:
http://cr.openjdk.java.net/~vinnie/8170282/webrev.01/



> On 9 Dec 2016, at 01:34, Bradford Wetmore <bradford.wetm...@oracle.com> wrote:
> 
> 
> Hi Vinnie,
> 
> On 12/8/2016 2:18 PM, Vincent Ryan wrote:
>> The Java Servlet Expect Group reported that they have identified a specific 
>> HTTP2 server use-case that cannot
>> be easily addressed using the existing ALPN APIs.
>> 
>> This changeset fixes that problem. It supports a new callback mechanism to 
>> allow TLS server applications
>> to set an application protocol during the TLS handshake. Specifically it 
>> allows the cipher suite chosen by the
>> TLS protocol implementation to be examined by the TLS server application 
>> before it sets the application protocol.
>> Additional TLS parameters are also available for inspection in the callback 
>> function.
>> 
>> This new mechanism is available only to TLS server applications. TLS clients 
>> will continue to use the existing ALPN APIs.
> 
> Technically, the API could be used for NPN (though it's pretty much dead), so 
> that would be a listing the options on the server side, and selection on 
> client side.
> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170282
>> Webrev: http://cr.openjdk.java.net/~vinnie/8170282/webrev.00/
> 
> SSLEngineImpl.java/SSLSocketImpl.java
> =
> Please use the standard handshaker initialization pattern where the 
> Handshaker is initialized with all of the data/mechanisms needed for a 
> complete handshake.  This will will ensure consistent handshake results and 
> avoid potential strange race conditions.  (There's some corresponding API 
> comments below.)
> 
> You would register your callback after the 
> handshaker.setApplicationProtocols() calls at (currently) line 444 in the 
> SSLEngineImpl code.
> 
> 
> SSLEngine.java/SSLSocket.java
> =
> I would suggest putting an introduction to this addition in the class 
> description section, that application values can be set using 
> SSLParameters.setApplication...() and selected with the default algorithm, or 
> that a more accurate selection mechanism can be created by registering the 
> callback that could look at any Handshake in progress and make appropriate 
> decisions.
> 
> 1339:
> Registers the callback function that selects an application protocol
> value during the SSL/TLS/DTLS handshake.
> ->
> Registers a callback function that selects an application protocol
> value for a SSL/TLS/DTLS handshake.  The function overrides any values set 
> using {@link SSLParameters#setApplicationProtocols 
> SSLParameters.setApplicationProtocols}.
> 
> and remove the corresponding section from the return docs (in the {@code 
> String section}).
> 
> the function's first argument enables the current
> handshake settings to be inspected.
> ->
> the function's first argument allows the current SSLEngine(SSLSocket) to be 
> inspected, including the handshake session and configuration settings.
> 
> If null is returned, or a value that was not advertised
> then the underlying protocol will determine what action
> to take.
> (For example, ALPN will send a "no_application_protocol"
> alert and terminate the connection.)
> ->
> If the return value is null (no value chosen) or is a value that was not 
> advertised by the peer, the underlying protocol will determine what action to 
> take.  (For example, ALPN will send a "no_application_protocol" alert and 
> terminate the connection.)
> 
> Also, TLS should be configured with parameters that
> ->
> Also, this SSLEngine(SSLSocket) should be configured with parameters that
> 
> @param selector the callback function, or null to de-register.
> ->
> @param selector the callback function, or null to disable the callback 
> functionality.
> 
> Retrieves the callback function that selects an application protocol
> value during the SSL/TLS/DTLS handshake.
> ->
> Retrieves the callback function that selects an application protocol
> value during a SSL/TLS/DTLS handshake.
> 
>This method should be called by TLS protocol implementations during
>the TLS handshake. The callback function should not be called until
>after the cipher suite has been selected.
> 
> I would suggest removing this apiNote entirely.  I expect only applications 
> will call this method, so the first sentence is not necessary since it's up 
> to the implementation how it wants to store the BiFunction.  I expect that 
> when the handshaker is initialized, the current BiFunction w

[9] RFR 8170282: Enable ALPN parameters to be supplied during the TLS handshake

[Vincent Ryan]

The Java Servlet Expect Group reported that they have identified a specific 
HTTP2 server use-case that cannot
be easily addressed using the existing ALPN APIs.

This changeset fixes that problem. It supports a new callback mechanism to 
allow TLS server applications
to set an application protocol during the TLS handshake. Specifically it allows 
the cipher suite chosen by the
TLS protocol implementation to be examined by the TLS server application before 
it sets the application protocol.
Additional TLS parameters are also available for inspection in the callback 
function.

This new mechanism is available only to TLS server applications. TLS clients 
will continue to use the existing ALPN APIs.

Thanks


Bug: https://bugs.openjdk.java.net/browse/JDK-8170282
Webrev: http://cr.openjdk.java.net/~vinnie/8170282/webrev.00/

Re: RFR 8170035: When determining the ciphersuite lists, there is no debug output for disabled suites

[Vincent Ryan]

Looks fine to me.
Thanks.


> On 22 Nov 2016, at 18:45, Jamil Nimeh  wrote:
> 
> Hello all,
> 
> This is a short webrev that adds extra debug output that will show users 
> which ciphers are not in the enabled list due to being disabled by things 
> like jdk.tls.disabledAlgorithms, etc.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8170035
> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8170035/webrev.01
> 
> Thanks,
> --Jamil

Re: RFR: 8168861 AnchorCertificates uses hardcoded password for cacerts keystore

[Vincent Ryan]

Your fix looks good to me Tony.
Thanks.


> On 10 Nov 2016, at 21:55, Anthony Scarpino  
> wrote:
> 
> Hi,
> 
> I need a one line review to null out the password field in the keystore load 
> op..
> 
> http://cr.openjdk.java.net/~ascarpino/8168861/webrev/
> 
> thanks
> 
> Tony

Re: Code Review Request, JDK-8166103 Allow certs with unknown critical extension in SunX509 validator

[Vincent Ryan]

Your changes look fine to me.
Just a minor language correction: ‘to use’ -> ‘using’ (2 instances)

> On 11 Nov 2016, at 05:11, Xuelei Fan  wrote:
> 
> Hi,
> 
> Please review this bug fix:
> 
>   http://cr.openjdk.java.net/~xuelei/8166103/webrev.00/
> 
> The current validator implementations only allow white listed critical 
> certificate extensions, and not all JDK supported extensions are known to the 
> validator.  As may result in some issues that the cert is valid, but cannot 
> pass the validation because there is a critical extension that is not white 
> listed in the validator implementation.
> 
> This fix will only check the validity of the white listed critical 
> certificate extensions, and ignore the critical certificate extensions if 
> they can be parsed with X509Certificate.
> 
> Thanks,
> Xuelei

Re: RFR : (XS) 8166432:Bad 8u112 merge of sun/security/tools/jarsigner/warnings/Test.java

[Vincent Ryan]

Looks fine to me.
Thanks.


> On 9 Nov 2016, at 11:26, Seán Coffey  wrote:
> 
> A bad test code merge occurred when 8u112 and CPU code was being merged a few 
> weeks ago. Some extra functionality added to a helper test was lost. This fix 
> restores it. 8u applicable only.
> 
> diff --git a/test/sun/security/tools/jarsigner/warnings/Test.java 
> b/test/sun/security/tools/jarsigner/warnings/Test.java
> --- a/test/sun/security/tools/jarsigner/warnings/Test.java
> +++ b/test/sun/security/tools/jarsigner/warnings/Test.java
> @@ -22,6 +22,11 @@
>  */
> 
> import jdk.testlibrary.OutputAnalyzer;
> +import jdk.testlibrary.ProcessTools;
> +
> +import java.util.ArrayList;
> +import java.util.Arrays;
> +import java.util.List;
> 
> /**
>  * Base class.
> @@ -175,4 +180,21 @@
> }
> analyzer.shouldContain(JAR_SIGNED);
> }
> +
> +protected OutputAnalyzer keytool(String... cmd) throws Throwable {
> +return tool(KEYTOOL, cmd);
> }
> +
> +protected OutputAnalyzer jarsigner(String... cmd) throws Throwable {
> +return tool(JARSIGNER, cmd);
> +}
> +
> +private OutputAnalyzer tool(String tool, String... args) throws 
> Throwable {
> +List cmd = new ArrayList<>();
> +cmd.add(tool);
> +cmd.add("-J-Duser.language=en");
> +cmd.add("-J-Duser.country=US");
> +cmd.addAll(Arrays.asList(args));
> +return ProcessTools.executeCommand(cmd.toArray(new 
> String[cmd.size()]));
> +}
> +}
> 
> -- 
> Regards,
> Sean.
> 

Re: RFR: 8168851: Tighten permissions granted to the java.smartcardio module

[Vincent Ryan]

Your fix still looks fine to me.


> On 27 Oct 2016, at 18:00, Sean Mullan <sean.mul...@oracle.com> wrote:
> 
> I accidentally used the wrong bugid. I have updated the Subject line and 
> links to reflect that this fix is for the java.smartcardio module, not the 
> jdk.crypto.ucrypto module:
> 
> https://bugs.openjdk.java.net/browse/JDK-8168851
> http://cr.openjdk.java.net/~mullan/webrevs/8168851/webrev.00/
> 
> --Sean
> 
> On 10/27/2016 12:28 PM, Vincent Ryan wrote:
>> Your fix looks fine to me Sean.
>> 
>> 
>>> On 27 Oct 2016, at 17:09, Sean Mullan <sean.mul...@oracle.com> wrote:
>>> 
>>> Please review this change to tighten or remove unnecessary permissions
>>> granted to the jdk.crypto.ucrypto module:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8167512
>>> http://cr.openjdk.java.net/~mullan/webrevs/8167512/webrev.00/
>>> 
>>> Thanks,
>>> Sean
>> 

Re: RFR: 8167512: Tighten permissions granted to the jdk.crypto.ucrypto module

[Vincent Ryan]

Your fix looks fine to me Sean.


> On 27 Oct 2016, at 17:09, Sean Mullan  wrote:
> 
> Please review this change to tighten or remove unnecessary permissions
> granted to the jdk.crypto.ucrypto module:
> 
> https://bugs.openjdk.java.net/browse/JDK-8167512
> http://cr.openjdk.java.net/~mullan/webrevs/8167512/webrev.00/
> 
> Thanks,
> Sean

Re: PING : Re: [jdk9] (S) RFR: 8165463: Native implementation of sunmscapi should use operator new (nothrow) for allocations

[Vincent Ryan]

Your fix looks good Ivan.
Thanks.


> On 21 Oct 2016, at 22:06, Ivan Gerasimov  wrote:
> 
> Hello!
> 
> Could someone with the Reviewer's rights please help review and approve this 
> fix?
> 
> The latest webrev can be found here:
> http://cr.openjdk.java.net/~igerasim/8165463/01/webrev/
> 
> Thanks in advance,
> Ivan
> 
> 

Re: RFR 8168374: TsacertOptionTest.java fails on all platforms

[Vincent Ryan]

Your fix looks fine to me.
Thanks.

> On 20 Oct 2016, at 05:21, Wang Weijun  wrote:
> 
> Please review this test change:
> 
> diff --git a/test/sun/security/tools/jarsigner/TsacertOptionTest.java 
> b/test/sun/security/tools/jarsigner/TsacertOptionTest.java
> --- a/test/sun/security/tools/jarsigner/TsacertOptionTest.java
> +++ b/test/sun/security/tools/jarsigner/TsacertOptionTest.java
> @@ -31,6 +31,7 @@
>  * @library /lib/testlibrary warnings
>  * @modules java.base/sun.security.pkcs
>  *  java.base/sun.security.timestamp
> + *  java.base/sun.security.tools.keytool
>  *  java.base/sun.security.util
>  *  java.base/sun.security.x509
>  *  java.management
> 
> TsacertOptionTest.java references another file TimestampCheck.java which uses 
> a class in this private package, and compilation fails.
> 
> Unfortunately JPRT has not caught this because TimestampCheck.java was 
> executed earlier (which itself is also a @test and includes the property 
> @modules line).
> 
> Noreg-self.
> 
> Thanks
> Max
> 

[9] RFR 8167371: KeyStoreSpi.engineSetEntry should throw an Exception if password protection alg is specified

[Vincent Ryan]

Please review this fix to add a check to the default implementation of KeyStore 
setEntry and getEntry (in KeyStoreSpi).
An exception is thrown if a password protection algorithm is specified. An 
existing test has been updated to validate the fix.

Keystore implementations that support a user-supplied password protection 
algorithm override the default implementation
of these setEntry/getEntry methods and are unaffected by this fix.

Thanks.


Bug: https://bugs.openjdk.java.net/browse/JDK-8167371
Webrev: http://cr.openjdk.java.net/~vinnie/8167371/webrev.00/

Re: RFR[9] JDK-8077138: Some PKCS11 tests fail because NSS library is not initialized

[Vincent Ryan]

The code changes look fine (but your webrev shows zero lines changed 
throughout).


> On 13 Sep 2016, at 10:43, John Jiang  wrote:
> 
> Hi,
> Please review this patch for fixing JDK-8077138.
> The solution is re-building NSS libraries with VS2013, and then the new NSS 
> DLLs can depend on msvcr120.dll, which is already distributed with JDK 9.
> 
> And please note that, this patch also removes the PKCS11 tests from 
> ProblemList.txt, though these tests have another issue JDK-8023434.
> JDK-8023434 is related to Solaris, but the PKCS11 tests are marked with 
> windows-all. So, I think it's no meaning to keep such items. Then, these 
> tests will really be executed on Windows platforms.
> 
> Webrev: http://cr.openjdk.java.net/~jjiang/8077138/webrev.00/
> Issue: https://bugs.openjdk.java.net/browse/JDK-8077138
> 
> Best regards,
> John Jiang
> 

Re: RFR: 8024714: In java.security file, ocsp.responderCertSubjectName should not contain quotes

[Vincent Ryan]

Looks fine to me.

> On 26 Aug 2016, at 13:13, Sean Mullan  wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8024714
> 
> A trivial fix to remove the quotes from the ocsp property name examples in 
> the java.security file as they should not be included and will cause a 
> parsing exception if included:
> 
> $ hg diff -U 1 --nodates
> diff -r 8c0d4a9bfea1 src/java.base/share/conf/security/java.security
> --- a/src/java.base/share/conf/security/java.security
> +++ b/src/java.base/share/conf/security/java.security
> @@ -492,3 +492,3 @@
> # Example,
> -#   ocsp.responderCertSubjectName="CN=OCSP Responder, O=XYZ Corp"
> +#   ocsp.responderCertSubjectName=CN=OCSP Responder, O=XYZ Corp
> 
> @@ -507,3 +507,3 @@
> # Example,
> -#   ocsp.responderCertIssuerName="CN=Enterprise CA, O=XYZ Corp"
> +#   ocsp.responderCertIssuerName=CN=Enterprise CA, O=XYZ Corp
> 
> Thanks,
> Sean

Re: [9] RFR 8164494: SunPKCS11-Solaris requires a non-empty PBE password

[Vincent Ryan]

I did consider the approach below, especially since it saves an extra call to 
Mac.getInstance.
However one motivation for fixing the original issue was to better facilitate 
third-party JCE providers
and I know of one provider that does support empty passwords which will never 
get selected if that approach is used.


> On 20 Aug 2016, at 01:30, Weijun Wang <weijun.w...@oracle.com> wrote:
> 
> How about
> 
>  this.prf = (passwdBytes.length == 0) ?
>  Mac.getInstance(prfAlgo, SunJCE.getInstance()) :
>  Mac.getInstance(prfAlgo);
> 
> Mac is only implemented in SunPKCS11 and SunJCE out-of-box, so this saves an 
> extra getInstance() call.
> 
> If a 3rd party provider is involved, I'm not sure it supports an empty 
> password (I have a feeling that except for Java everyone else are using the 
> same C codes. Maybe not Microsoft), and this call is safer.
> 
> --Max
> 
> On 8/20/2016 5:18, Valerie Peng wrote:
>> Looks fine to me.
>> Thanks,
>> Valerie
>> 
>> On 8/19/2016 9:57 AM, Vincent Ryan wrote:
>>> Please review this fix to PBE key derivation function which detects
>>> when a non-empty password
>>> is supplied to the SunPKCS11-Solaris JCE provider and fails over to
>>> the SunJCE provider instead.
>>> Thanks.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8164494
>>> Webrev: http://cr.openjdk.java.net/~vinnie/8164494/webrev.00/
>>> <http://cr.openjdk.java.net/%7Evinnie/8164494/webrev.00/>
>>> 
>> 

[9] RFR 8164494: SunPKCS11-Solaris requires a non-empty PBE password

[Vincent Ryan]

Please review this fix to PBE key derivation function which detects when a 
non-empty password
is supplied to the SunPKCS11-Solaris JCE provider and fails over to the SunJCE 
provider instead.
Thanks.

Bug: https://bugs.openjdk.java.net/browse/JDK-8164494
Webrev: http://cr.openjdk.java.net/~vinnie/8164494/webrev.00/ 

[9] RFR 8164398: Add test sun/security/krb5/auto/EmptyPassword.java to ProblemList

[Vincent Ryan]

Please approve this change to add a failing test to jdk/test/ProblemList.txt so 
we can investigate further.
Thanks.


diff --git a/test/ProblemList.txt b/test/ProblemList.txt
--- a/test/ProblemList.txt
+++ b/test/ProblemList.txt
@@ -289,6 +289,8 @@

 sun/security/ssl/SSLSocketImpl/AsyncSSLSocketClose.java 8161232 
macosx-all

+sun/security/krb5/auto/EmptyPassword.java   8164307 
solaris-all
+
 

 # jdk_sound

Re: PING - [jdk9] RFR: 8153438: Avoid repeated "Please insert a smart card" popup windows

[Vincent Ryan]

That fix looks fine. Is there any significant performance impact due to calling 
CryptAcquireCertificatePrivateKey twice?
Thanks.

> On 16 Aug 2016, at 13:56, Ivan Gerasimov  wrote:
> 
> A gentle reminder.
> 
> Would you please help review at your convenience.
> 
> With kind regards,
> Ivan
> 
> 
> On 09.08.2016 12:27, Ivan Gerasimov wrote:
>> Hello!
>> 
>> In order to reduce the number of popup dialog windows during accessing the 
>> smartcard, it is proposed to first do a silent "probe" step.
>> Only if this probe succeeded, or if it failed due to that SILENT flag, we'll 
>> try to re-acquire the key normally (i.e. not silently).
>> 
>> Would you please help review this proposal?
>> 
>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8153438
>> WEBREV: http://cr.openjdk.java.net/~igerasim/8153438/00/webrev/
>> 
>> With kind regards,
>> Ivan
>> 
>> 
> 

Re: [jdk9] RFR: 8163896: Finalizing one key of a KeyPair invalidates the other key

[Vincent Ryan]

Thanks Ivan. Your fix looks good.


> On 13 Aug 2016, at 02:15, Ivan Gerasimov  wrote:
> 
> Hello!
> 
> It was noticed, that SunMSCAPI implementation of a KeyPair has a problem: The 
> public and the private keys share the same native handles.
> 
> As the result, when one of the keys is finalized, and its resources are 
> freed, the second key becomes invalid.
> 
> Would you please help review the fix?
> 
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8163896
> WEBREV: http://cr.openjdk.java.net/~igerasim/8163896/00/webrev/
> 
> With kind regards,
> Ivan
> 

Re: [9] RFR 8163503: PKCS12 keystore cannot store non-X.509 certificates

[Vincent Ryan]

I’ve made that change at l.970 and updated webrev.02 in-place.
Thanks.

> On 12 Aug 2016, at 19:53, Sean Mullan <sean.mul...@oracle.com> wrote:
> 
> Looks fine, although I would probably avoid calling checkX509Certs on line 
> 970 and just checking the cert right there to avoid creating the array which 
> is not needed.
> 
> --Sean
> 
> On 08/12/2016 11:07 AM, Vincent Ryan wrote:
>> I’ve moved the X.509 check to earlier in the code and reverted the changes 
>> to the validateChain method.
>> Updated webrev is at:
>>   http://cr.openjdk.java.net/~vinnie/8163503/webrev.02/
>> 
>> 
>> 
>>> On 10 Aug 2016, at 21:15, Sean Mullan <sean.mul...@oracle.com> wrote:
>>> 
>>> On 08/10/2016 12:39 PM, Vincent Ryan wrote:
>>>> Yes they could be merged but the first loop iterates over all the certs 
>>>> and the second one iterates over all but the final cert.
>>>> And the special case of a 1-cert chain also needs to be handled. I think 
>>>> it’s a little clearer to leave them separate.
>>> 
>>> Agreed, but it's probably better to check these earlier and bail out if 
>>> they are not all X509Certificates, for example, right at the beginning of 
>>> engineSetKeyEntry. There is no need to proceed with decoding keys, etc 
>>> since it is already a violation of the supported API parameters.
>>> 
>>> --Sean
>>> 
>>>> 
>>>> An updated webrev is at:
>>>> http://cr.openjdk.java.net/~vinnie/8163503/webrev.01/
>>>> 
>>>> Thanks.
>>>> 
>>>>> On 10 Aug 2016, at 02:04, Xuelei Fan <xuelei@oracle.com> wrote:
>>>>> 
>>>>> The for loop at line 1507 and 1520 may be merged together.
>>>>> 
>>>>> Xuelei
>>>>> 
>>>>> On 8/10/2016 8:38 AM, Weijun Wang wrote:
>>>>>> I thought I've seen this webrev before.
>>>>>> 
>>>>>> Why not just throw a KeyStoreException in validateChain()?
>>>>>> 
>>>>>> --Max
>>>>>> 
>>>>>> On 8/10/2016 2:14, Vincent Ryan wrote:
>>>>>>> Please review this fix to improve the error handling for attempts to
>>>>>>> store a Certificate object in PKCS12 keystore.
>>>>>>> The PKCS12 keystore implementation supports storing only
>>>>>>> X509Certificate objects but the KeyStore API allows Certificate objects.
>>>>>>> This fix rejects attempts to store non-X.509 certificates and throws a
>>>>>>> KeyStoreException.
>>>>>>> 
>>>>>>> Thanks.
>>>>>>> 
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8163503
>>>>>>> Webrev: http://cr.openjdk.java.net/~vinnie/8163503/webrev.00/
>>>>>>> 
>>>>>>> 
>>>>> 
>>>> 
>> 

Re: [9] RFR 8163503: PKCS12 keystore cannot store non-X.509 certificates

[Vincent Ryan]

I’ve moved the X.509 check to earlier in the code and reverted the changes to 
the validateChain method.
Updated webrev is at:
   http://cr.openjdk.java.net/~vinnie/8163503/webrev.02/



> On 10 Aug 2016, at 21:15, Sean Mullan <sean.mul...@oracle.com> wrote:
> 
> On 08/10/2016 12:39 PM, Vincent Ryan wrote:
>> Yes they could be merged but the first loop iterates over all the certs and 
>> the second one iterates over all but the final cert.
>> And the special case of a 1-cert chain also needs to be handled. I think 
>> it’s a little clearer to leave them separate.
> 
> Agreed, but it's probably better to check these earlier and bail out if they 
> are not all X509Certificates, for example, right at the beginning of 
> engineSetKeyEntry. There is no need to proceed with decoding keys, etc since 
> it is already a violation of the supported API parameters.
> 
> --Sean
> 
>> 
>> An updated webrev is at:
>>  http://cr.openjdk.java.net/~vinnie/8163503/webrev.01/
>> 
>> Thanks.
>> 
>>> On 10 Aug 2016, at 02:04, Xuelei Fan <xuelei@oracle.com> wrote:
>>> 
>>> The for loop at line 1507 and 1520 may be merged together.
>>> 
>>> Xuelei
>>> 
>>> On 8/10/2016 8:38 AM, Weijun Wang wrote:
>>>> I thought I've seen this webrev before.
>>>> 
>>>> Why not just throw a KeyStoreException in validateChain()?
>>>> 
>>>> --Max
>>>> 
>>>> On 8/10/2016 2:14, Vincent Ryan wrote:
>>>>> Please review this fix to improve the error handling for attempts to
>>>>> store a Certificate object in PKCS12 keystore.
>>>>> The PKCS12 keystore implementation supports storing only
>>>>> X509Certificate objects but the KeyStore API allows Certificate objects.
>>>>> This fix rejects attempts to store non-X.509 certificates and throws a
>>>>> KeyStoreException.
>>>>> 
>>>>> Thanks.
>>>>> 
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8163503
>>>>> Webrev: http://cr.openjdk.java.net/~vinnie/8163503/webrev.00/
>>>>> 
>>>>> 
>>> 
>> 

[9] RFR 6877937: The SunJCE PBKDF2KeyImpl is requiring the MAC instance also be from SunJCE.

[Vincent Ryan]

Please review this change to unpin the Mac implementation from the SunJCE 
provider.
Since the Mac is a private field there are no issues regarding Clonable 
implementations for Mac or its MessageDigest.
Thanks.

Bug: https://bugs.openjdk.java.net/browse/JDK-6977937

diff --git 
a/src/java.base/share/classes/com/sun/crypto/provider/PBKDF2KeyImpl.java 
b/src/java.base/share/classes/com/sun/crypto/provider/PBKDF2KeyImpl.java
--- a/src/java.base/share/classes/com/sun/crypto/provider/PBKDF2KeyImpl.java
+++ b/src/java.base/share/classes/com/sun/crypto/provider/PBKDF2KeyImpl.java
@@ -107,7 +107,7 @@
 throw new InvalidKeySpecException("Key length is negative");
 }
 try {
-this.prf = Mac.getInstance(prfAlgo, SunJCE.getInstance());
+this.prf = Mac.getInstance(prfAlgo);
 } catch (NoSuchAlgorithmException nsae) {
 // not gonna happen; re-throw just in case
 InvalidKeySpecException ike = new InvalidKeySpecException();

Re: [9] RFR 8163503: PKCS12 keystore cannot store non-X.509 certificates

[Vincent Ryan]

Yes they could be merged but the first loop iterates over all the certs and the 
second one iterates over all but the final cert.
And the special case of a 1-cert chain also needs to be handled. I think it’s a 
little clearer to leave them separate.

An updated webrev is at:
  http://cr.openjdk.java.net/~vinnie/8163503/webrev.01/

Thanks.

> On 10 Aug 2016, at 02:04, Xuelei Fan <xuelei@oracle.com> wrote:
> 
> The for loop at line 1507 and 1520 may be merged together.
> 
> Xuelei
> 
> On 8/10/2016 8:38 AM, Weijun Wang wrote:
>> I thought I've seen this webrev before.
>> 
>> Why not just throw a KeyStoreException in validateChain()?
>> 
>> --Max
>> 
>> On 8/10/2016 2:14, Vincent Ryan wrote:
>>> Please review this fix to improve the error handling for attempts to
>>> store a Certificate object in PKCS12 keystore.
>>> The PKCS12 keystore implementation supports storing only
>>> X509Certificate objects but the KeyStore API allows Certificate objects.
>>> This fix rejects attempts to store non-X.509 certificates and throws a
>>> KeyStoreException.
>>> 
>>> Thanks.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8163503
>>> Webrev: http://cr.openjdk.java.net/~vinnie/8163503/webrev.00/
>>> 
>>> 
> 

Re: [9] RFR 8163503: PKCS12 keystore cannot store non-X.509 certificates

[Vincent Ryan]

You’re right. This same issue had been reported as an obscure JCK test failure.
I created this new bug to clarify the issue.

I’ve updated the webrev to include your suggestion:
  http://cr.openjdk.java.net/~vinnie/8163503/webrev.01/

Thanks.


> On 10 Aug 2016, at 01:38, Weijun Wang <weijun.w...@oracle.com> wrote:
> 
> I thought I've seen this webrev before.
> 
> Why not just throw a KeyStoreException in validateChain()?
> 
> --Max
> 
> On 8/10/2016 2:14, Vincent Ryan wrote:
>> Please review this fix to improve the error handling for attempts to store a 
>> Certificate object in PKCS12 keystore.
>> The PKCS12 keystore implementation supports storing only X509Certificate 
>> objects but the KeyStore API allows Certificate objects.
>> This fix rejects attempts to store non-X.509 certificates and throws a 
>> KeyStoreException.
>> 
>> Thanks.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8163503
>> Webrev: http://cr.openjdk.java.net/~vinnie/8163503/webrev.00/
>> 
>> 

Re: [9] RFR 8163503: PKCS12 keystore cannot store non-X.509 certificates

[Vincent Ryan]

I’ve updated the webrev to include your suggestion:
  http://cr.openjdk.java.net/~vinnie/8163503/webrev.01/

Thanks.

> On 10 Aug 2016, at 10:59, Seán Coffey <sean.cof...@oracle.com> wrote:
> 
> It would be good if we can print the cert class type in the new exception if 
> the instanceof check fails.
> 
> Regards,
> Sean.
> 
> On 09/08/16 19:14, Vincent Ryan wrote:
>> Please review this fix to improve the error handling for attempts to store a 
>> Certificate object in PKCS12 keystore.
>> The PKCS12 keystore implementation supports storing only X509Certificate 
>> objects but the KeyStore API allows Certificate objects.
>> This fix rejects attempts to store non-X.509 certificates and throws a 
>> KeyStoreException.
>> 
>> Thanks.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8163503
>> Webrev: http://cr.openjdk.java.net/~vinnie/8163503/webrev.00/
>> 
>> 
> 

[9] RFR 8163503: PKCS12 keystore cannot store non-X.509 certificates

[Vincent Ryan]

Please review this fix to improve the error handling for attempts to store a 
Certificate object in PKCS12 keystore.
The PKCS12 keystore implementation supports storing only X509Certificate 
objects but the KeyStore API allows Certificate objects.
This fix rejects attempts to store non-X.509 certificates and throws a 
KeyStoreException.

Thanks.

Bug: https://bugs.openjdk.java.net/browse/JDK-8163503
Webrev: http://cr.openjdk.java.net/~vinnie/8163503/webrev.00/

Re: RFR 8162739: Create new keytool option to access cacerts file

[Vincent Ryan]

Your changes look good to me.
Thanks.


> On 8 Aug 2016, at 14:46, Weijun Wang  wrote:
> 
> Please review the code changes at
> 
>   http://cr.openjdk.java.net/~weijun/8162739/webrev.00/
> 
> A new -cacerts option is added to keytool so there is no need to provide the 
> full cacerts path on the command line. lib/security/cacerts is considered an 
> implementation detail and subject to change.
> 
> The conf/security/cacerts file in src/ is moved to lib/security/cacerts, 
> which matches its destination path. A small change in make/ is needed.
> 
> Thanks
> Max

Re: [9] RFR 8157579: com/sun/crypto/provider/Mac/MacClone.java failed on solaris12(sparcv9 and x86)

[Vincent Ryan]

Your fix looks good. One comment is that you could trim the Provider[] to 
exclude the most-preferred provider (at index=0).
Thanks.


> On 5 Aug 2016, at 03:01, Valerie Peng  wrote:
> 
> 
> Anyone has time to review this fix? The code change is in only one file and 
> straightforward if you agree with the approach.
> Starting S11.3 and S12, OracleUcrypto provider switched to new Ucrypto APIs 
> for message digest operations and there is no clone support.
> This affects the MAC impls of SunJCE provider which delegates the digest 
> operation to the most preferred provider.
> To ensure the clone support, I will switch to getting the digest impl from 
> SUN provider if the most preferred one does not.
> In the case of SUN provider is not available, it will then goes through 
> provider list.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8157579
> Webrev: http://cr.openjdk.java.net/~valeriep/8157579/webrev.00/
> 
> No new test as it's covered by existing regression test.
> 
> Thanks,
> Valerie
> 
> 

Re: [9] RFR 8161571: Verifying ECDSA signatures permits trailing bytes

[Vincent Ryan]

Thanks for the review.

The PKCS11 implementation is a little peculiar in that it is configured 
out-of-the-box only for Solaris
and that implementation doesn’t support DSA. So I’ve added only the first of 
your additional lines below.

 (NOTE the update to the Ucrypto provider)

Updated webrev at: 
  http://cr.openjdk.java.net/~vinnie/8161571/webrev.01/




> On 21 Jul 2016, at 15:46, Xuelei Fan <xuelei@oracle.com> wrote:
> 
> Looks fine to me.
> 
> Just two minor comments.  The run tag in the test may be not necessary.
> Like EC algorithm, maybe the PKCS11 implementation of RSA and DSA
> algorithms can also be checked on some platform if not using provider
> option.
> 
> +  main0("RSA", 2048, "SHA256withRSA", null);
> +  main0("DSA", 2048, "SHA256withDSA", null);
> 
> Xuelei
> 
> On 7/20/2016 3:10 AM, Vincent Ryan wrote:
>> Please review this fix to apply stricter length checks when verifying public 
>> key signatures.
>> Thanks.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8161571
>> Webrev: http://cr.openjdk.java.net/~vinnie/8161571/webrev.00/
>> 
> 

Re: Code Review Request JDK-8161898 Mark the use of deprecated javax.security.cert APIs with forRemoval=true

[Vincent Ryan]

Looks fine to me.
Thanks.


> On 21 Jul 2016, at 03:32, Xuelei Fan  wrote:
> 
> Hi,
> 
> javax.security.cert was deprecated and marked with forRemoval=true in
> JDK 9.  The use of javax.security.cert APIs should be marked with
> forRemoval=true too.
> 
> Please review the update:
> 
>   http://cr.openjdk.java.net/~xuelei/8161898/webrev.00/
> 
> Thanks,
> Xuelei

[9] RFR 8161571: Verifying ECDSA signatures permits trailing bytes

[Vincent Ryan]

Please review this fix to apply stricter length checks when verifying public 
key signatures.
Thanks.

Bug: https://bugs.openjdk.java.net/browse/JDK-8161571
Webrev: http://cr.openjdk.java.net/~vinnie/8161571/webrev.00/

Re: RFR: 9: 8144559: sun/security/mscapi/SignUsingNONEwithRSA.sh failed intermittently

[Vincent Ryan]

Your change looks fine to me - and another shell script zapped.
Thanks.


> On 7 Jul 2016, at 23:43, Rajan Halade  wrote:
> 
> May I request you to review this small patch. I am not able to reproduce the 
> failure so I enhance the test itself to get rid of shell script. Since 
> temporary key generation is now moved to java source, if it fails, exception 
> will be raised and test will not fail with "Cannot load keys". I also 
> removed "intermittent" key from this test as it has never failed since this 
> one instance back in 2015 and there are no other open bugs affecting this 
> test.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8144559
> Webrev: http://cr.openjdk.java.net/~rhalade/8144559/webrev.00/
> 
> Thanks,
> Rajan
> 

Re: [9] RFR 8068290: JCK: 2 api/java_security/cert/ tests fail

[Vincent Ryan]

I’ve updated the webrev to also check for X509Certificate in both 
engineSetKeyEntry methods and in engineSetEntry:
  http://cr.openjdk.java.net/~vinnie/8068290/webrev.01/


> On 4 Jul 2016, at 10:25, Vincent Ryan <vincent.x.r...@oracle.com> wrote:
> 
> Good catch Max. I’ll add the check there too.
> 
> 
>> On 2 Jul 2016, at 02:04, Wang Weijun <weijun.w...@oracle.com> wrote:
>> 
>> Should engineSetKeyEntry make the same check?
>> 
>> --Max
>> 
>>> On Jul 2, 2016, at 5:52 AM, Vincent Ryan <vincent.x.r...@oracle.com> wrote:
>>> 
>>> Please review this change to correct two failing JCK tests.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8068290
>>> Webrev: http://cr.openjdk.java.net/~vinnie/8068290/webrev.00/
>>> 
>>> PKCS12 is the default keystore type in JDK 9 and its implementation stores 
>>> only X.509 certificates
>>> although the KeyStore API allows any Certificate object to be stored.
>>> This fix checks that the supplied certificate is an X509Certificate object 
>>> before storing it.
>>> 
>>> Thanks.
>>> 
>> 
> 

Re: [9] RFR 8068290: JCK: 2 api/java_security/cert/ tests fail

[Vincent Ryan]

Good catch Max. I’ll add the check there too.


> On 2 Jul 2016, at 02:04, Wang Weijun <weijun.w...@oracle.com> wrote:
> 
> Should engineSetKeyEntry make the same check?
> 
> --Max
> 
>> On Jul 2, 2016, at 5:52 AM, Vincent Ryan <vincent.x.r...@oracle.com> wrote:
>> 
>> Please review this change to correct two failing JCK tests.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8068290
>> Webrev: http://cr.openjdk.java.net/~vinnie/8068290/webrev.00/
>> 
>> PKCS12 is the default keystore type in JDK 9 and its implementation stores 
>> only X.509 certificates
>> although the KeyStore API allows any Certificate object to be stored.
>> This fix checks that the supplied certificate is an X509Certificate object 
>> before storing it.
>> 
>> Thanks.
>> 
> 

[9] RFR 8068290: JCK: 2 api/java_security/cert/ tests fail

[Vincent Ryan]

Please review this change to correct two failing JCK tests.

Bug: https://bugs.openjdk.java.net/browse/JDK-8068290
Webrev: http://cr.openjdk.java.net/~vinnie/8068290/webrev.00/

PKCS12 is the default keystore type in JDK 9 and its implementation stores only 
X.509 certificates
although the KeyStore API allows any Certificate object to be stored.
This fix checks that the supplied certificate is an X509Certificate object 
before storing it.

Thanks.

Re: RFR 8157730: Mark deprecated java.security.{Identity, IdentityScope, Signer} APIs with forRemoval=true

[Vincent Ryan]

Sure. (It works for packages too.)

> On 17 Jun 2016, at 18:54, e...@zusammenkunft.net wrote:
> 
> Hello,
> 
> While you are at that code, can the JavaDoc  @code classnames be changed into 
> @link as well? (Not sure if it works for packages).
> 
> Gruss
> Bernd
> 
> -- 
> http://bernd.eckenfels.net
> 
> -Original Message-
> From: Sean Mullan <sean.mul...@oracle.com>
> To: Vincent Ryan <vincent.x.r...@oracle.com>, OpenJDK 
> <security-dev@openjdk.java.net>
> Sent: Fr., 17 Juni 2016 16:41
> Subject: Re: RFR 8157730: Mark deprecated java.security.{Identity, 
> IdentityScope, Signer} APIs with forRemoval=true
> 
> Looks fine to me.
> 
> --Sean
> 
> On 06/17/2016 08:52 AM, Vincent Ryan wrote:
>> Three Identity-related classes were deprecated in JDK 1.2.
>> Please review the patch below that marks them as candidates for removal
>> in a future JDK release.
>> 
>> See http://openjdk.java.net/jeps/277 for details of the enhanced
>> deprecation annotation.
>> 
>> Thanks.
>> 
>> 
>> diff --git a/src/java.base/share/classes/java/security/Identity.java
>> b/src/java.base/share/classes/java/security/Identity.java
>> --- a/src/java.base/share/classes/java/security/Identity.java
>> +++ b/src/java.base/share/classes/java/security/Identity.java
>> @@ -55,8 +55,9 @@
>>   * replaced by {@code java.security.KeyStore}, the
>>   * {@code java.security.cert} package, and
>>   * {@code java.security.Principal}.
>> + * This class is subject to removal in a future version of Java SE.
>>   */
>> -@Deprecated
>> +@Deprecated(since="1.2", forRemoval=true)
>>  public abstract class Identity implements Principal, Serializable {
>> 
>>  /** use serialVersionUID from JDK 1.1.x for interoperability */
>> 
>> 
>> diff --git
>> a/src/java.base/share/classes/java/security/IdentityScope.java
>> b/src/java.base/share/classes/java/security/IdentityScope.java
>> --- a/src/java.base/share/classes/java/security/IdentityScope.java
>> +++ b/src/java.base/share/classes/java/security/IdentityScope.java
>> @@ -60,8 +60,12 @@
>>   * replaced by {@code java.security.KeyStore}, the
>>   * {@code java.security.cert} package, and
>>   * {@code java.security.Principal}.
>> + * This class is subject to removal in a future version of Java SE.
>> + *
>> + * Note that the security property {@code policy.ignoreIdentityScope}
>> + * is only applicable to these APIs and is also a candidate for removal.
>>   */
>> -@Deprecated
>> +@Deprecated(since="1.2", forRemoval=true)
>>  public abstract
>>  class IdentityScope extends Identity {
>> 
>> 
>> diff --git a/src/java.base/share/classes/java/security/Signer.java
>> b/src/java.base/share/classes/java/security/Signer.java
>> --- a/src/java.base/share/classes/java/security/Signer.java
>> +++ b/src/java.base/share/classes/java/security/Signer.java
>> @@ -43,8 +43,9 @@
>>   * replaced by {@code java.security.KeyStore}, the
>>   * {@code java.security.cert} package, and
>>   * {@code java.security.Principal}.
>> + * This class is subject to removal in a future version of Java SE.
>>   */
>> -@Deprecated
>> +@Deprecated(since="1.2", forRemoval=true)
>>  public abstract class Signer extends Identity {
>> 
>>  private static final long serialVersionUID = -1763464102261361480L;
>> 

RFR 8157730: Mark deprecated java.security.{Identity, IdentityScope, Signer} APIs with forRemoval=true

[Vincent Ryan]

Three Identity-related classes were deprecated in JDK 1.2.
Please review the patch below that marks them as candidates for removal in a 
future JDK release.

See http://openjdk.java.net/jeps/277  for 
details of the enhanced deprecation annotation.

Thanks.


diff --git a/src/java.base/share/classes/java/security/Identity.java 
b/src/java.base/share/classes/java/security/Identity.java
--- a/src/java.base/share/classes/java/security/Identity.java
+++ b/src/java.base/share/classes/java/security/Identity.java
@@ -55,8 +55,9 @@
  * replaced by {@code java.security.KeyStore}, the
  * {@code java.security.cert} package, and
  * {@code java.security.Principal}.
+ * This class is subject to removal in a future version of Java SE.
  */
-@Deprecated
+@Deprecated(since="1.2", forRemoval=true)
 public abstract class Identity implements Principal, Serializable {

 /** use serialVersionUID from JDK 1.1.x for interoperability */


diff --git a/src/java.base/share/classes/java/security/IdentityScope.java 
b/src/java.base/share/classes/java/security/IdentityScope.java
--- a/src/java.base/share/classes/java/security/IdentityScope.java
+++ b/src/java.base/share/classes/java/security/IdentityScope.java
@@ -60,8 +60,12 @@
  * replaced by {@code java.security.KeyStore}, the
  * {@code java.security.cert} package, and
  * {@code java.security.Principal}.
+ * This class is subject to removal in a future version of Java SE.
+ *
+ * Note that the security property {@code policy.ignoreIdentityScope}
+ * is only applicable to these APIs and is also a candidate for removal.
  */
-@Deprecated
+@Deprecated(since="1.2", forRemoval=true)
 public abstract
 class IdentityScope extends Identity {


diff --git a/src/java.base/share/classes/java/security/Signer.java 
b/src/java.base/share/classes/java/security/Signer.java
--- a/src/java.base/share/classes/java/security/Signer.java
+++ b/src/java.base/share/classes/java/security/Signer.java
@@ -43,8 +43,9 @@
  * replaced by {@code java.security.KeyStore}, the
  * {@code java.security.cert} package, and
  * {@code java.security.Principal}.
+ * This class is subject to removal in a future version of Java SE.
  */
-@Deprecated
+@Deprecated(since="1.2", forRemoval=true)
 public abstract class Signer extends Identity {

 private static final long serialVersionUID = -1763464102261361480L;

Re: RFR 8149521: automatic discovery of LDAP servers with Kerberos authentication

[Vincent Ryan]

Looks fine to me Max.
Thanks.

> On 10 May 2016, at 14:34, Wang Weijun  wrote:
> 
> Hi All
> 
> Please take a review at 
> 
>   http://cr.openjdk.java.net/~weijun/8149521/webrev.00/
> 
> While the bug report [1] suggests we can fix com.sun.jndi.ldap.ServiceLocator 
> to emit a trail-dot-less hostname, I am not sure if it's safe to do so. 
> Anyway, the failure is on the Kerberos side and I believe this is a 
> regression since we stop canonicalizing the hostname. Therefore I prefer to 
> do a small "normalization" inside PrincipalName.
> 
> Thanks
> Max
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8149521

Re: Code Review Request: 8156502, Use short name of SupportedEllipticCurvesExtension.java

[Vincent Ryan]

Your change looks fine to me.
Thanks.


> On 8 May 2016, at 16:12, Xuelei Fan  wrote:
> 
> Hi,
> 
> Please review this class name update:
> 
>http://cr.openjdk.java.net/~xuelei/8156502/webrev.00/
> 
> The class names of SupportedEllipticCurvesExtension and
> SupportedEllipticPointFormatsExtension are very long. The source codes
> using the class names are likely to exceed 80 characters each line.
> Better to use short names.
> 
> Thanks,
> Xuelei

Re: CFV: New Security Group Member: Jamil Nimeh

[Vincent Ryan]

Vote: yes


> On 29 Apr 2016, at 15:40, Sean Mullan  wrote:
> 
> I hereby nominate Jamil Nimeh to Membership in the Security Group.
> 
> Jamil is a member of the Java Security Libraries team at Oracle and  has been 
> an active contributor to the OpenJDK Security Group for approximately two 
> years. Jamil was voted in as a JDK 9 committer in April, 2015, and has 
> continued to play an active role working on the security components of the 
> JDK. Jamil has integrated many significant bug fixes and enhancements 
> especially in the PKI and TLS areas. He is also the owner of JEP 249 ("OCSP 
> Stapling for TLS"), which has successfully reached Completed status for JDK 9.
> 
> Votes are due by May 13, 2016, 3:00 PM UTC.
> 
> Only current Members of the Security Group [1] are eligible to vote on this 
> nomination. Votes must be cast in the open by replying to this mailing list.
> 
> For Lazy Consensus voting instructions, see [2].
> 
> Sean Mullan
> 
> [1] http://openjdk.java.net/census#security
> [2] http://openjdk.java.net/groups/#member-vote
> 

Re: JDK 9 RFR of JDK-8153695: Problem list sun/security/pkcs11/Provider/Login.sh for linux-all

[Vincent Ryan]

That looks fine to me.
Thanks.

> On 7 Apr 2016, at 04:42, Amy Lu  wrote:
> 
> sun/security/pkcs11/Provider/Login.sh 
> This test is known to fail at Linux platform (JDK-8153545). 
> 
> Please review the patch to put the test to ProblemList.txt for linux-all 
> until mentioned issue (JDK-8153545) is resolved.
> Tested with jtreg -listtests
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8153695 
> 
> webrev: http://cr.openjdk.java.net/~amlu/8153695/webrev.00/ 
> 
> 
> Thanks,
> Amy
> 
> 
>  --- old/test/ProblemList.txt 2016-04-07 11:36:18.0 +0800
> +++ new/test/ProblemList.txt  2016-04-07 11:36:18.0 +0800
> @@ -238,7 +238,7 @@
>  sun/security/pkcs11/MessageDigest/ReinitDigest.java 
> 8077138,8023434 windows-all
>  sun/security/pkcs11/MessageDigest/TestCloning.java  
> 8077138,8023434 windows-all
>  sun/security/pkcs11/Provider/ConfigQuotedString.sh  
> 8077138,8023434 windows-all
> -sun/security/pkcs11/Provider/Login.sh   
> 8077138,8023434 windows-all
> +sun/security/pkcs11/Provider/Login.sh   
> 8077138,8023434,8153545 windows-all,linux-all
>  sun/security/pkcs11/SampleTest.java 
> 8077138,8023434 windows-all
>  sun/security/pkcs11/Secmod/AddPrivateKey.java   
> 8077138,8023434 windows-all
>  sun/security/pkcs11/Secmod/AddTrustedCert.java  
> 8077138,8023434 windows-all
> 
> 

Re: RFR 6483657: MSCAPI provider does not create unique alias names

[Vincent Ryan]

Your fix looks good.
Thanks.

> On 31 Mar 2016, at 16:57, Ivan Gerasimov  wrote:
> 
> Hello!
> 
> Could you please help review the fix for this long standing issue?
> Windows-MY allows non-unique aliases, but our implementation of KeyStore does 
> not take it into account.
> 
> To help to deal with such keystores with multiple same-named aliases it is 
> proposed to internally remap the keystore entries to aliases that 
> artificially made unique.
> 
> I was meaning to create a regression test, but found no easy way to test this 
> behavior, as the keytool program doesn't allow creating same-named aliases.
> All existing regression tests pass well however.
> 
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-6483657
> WEBREV: http://cr.openjdk.java.net/~igerasim/6483657/00/webrev/
> 
> With kind regards,
> Ivan
> 

Re: JDK 9 RFR of JDK-8151835: Mark SmallPrimeExponentP.java as intermittently failing

[Vincent Ryan]

Looks fine to me.
Thanks.


> On 14 Mar 2016, at 17:31, joe darcy  wrote:
> 
> Hello,
> 
> The test
> 
> sun/security/mscapi/SmallPrimeExponentP.java
> 
> has been seen to timeout intermittently, bug JDK-8151834.
> 
> The sources of the test should be marked accordingly; please review the 
> corresponding patch below.
> 
> Thanks,
> 
> -Joe
> 
> --- a/test/sun/security/mscapi/SmallPrimeExponentP.javaFri Mar 11 
> 17:07:57 2016 -0800
> +++ b/test/sun/security/mscapi/SmallPrimeExponentP.javaMon Mar 14 
> 10:27:59 2016 -0700
> @@ -32,6 +32,7 @@
> /*
>  * @test
>  * @bug 8023546
> + * @key intermittent
>  * @modules java.base/sun.security.x509
>  *  java.base/sun.security.tools.keytool
>  * @summary sun/security/mscapi/ShortRSAKey1024.sh fails intermittently
> 

Re: [9] review request 8151149: CipherSpi implementation of PBEWithSHA1AndDESede returns key size in bytes

[Vincent Ryan]

Thanks Xuelei.

> On 7 Mar 2016, at 14:14, Xuelei Fan <xuelei@oracle.com> wrote:
> 
> On 3/7/2016 10:05 PM, Vincent Ryan wrote:
>> Please review this fix to PKCS#12 PBE CipherSpi classes to return the cipher 
>> key size in bits.
>> An effective key size of 112 bits is used for Triple DES.
>> Thanks.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8151149
>> Webrev: http://cr.openjdk.java.net/~vinnie/8151149/webrev.00/
>> 
> Nice!  Looks fine to me.
> 
> Thanks,
> Xuelei

Re: RFR 8138653: Default key sizes for the AlgorithmParameterGenerator and KeyPairGenerator implementations should be upgraded

[Vincent Ryan]

Your fix looks fine.
Thanks.


> On 1 Mar 2016, at 19:21, Sean Mullan  wrote:
> 
> Updated webrev: http://cr.openjdk.java.net/~mullan/webrevs/8138653/webrev.01/
> 
> The following changes have been made:
> 
> - The default key size for DSA has not been changed (stays at 1024) due to 
> the high risk of breaking compatibility with applications still using 
> SHA1withDSA (key sizes larger than 1024 may be incompatible and rejected). We 
> will wait on this one for now.
> 
> - The SunPKCS11 default size for RSA keys has been increased to 2048.
> 
> - A bug in the PKCS11 tests was fixed which caused the version of newer NSS 
> libraries to be unrecognized.
> 
> --Sean
> 
> On 02/24/2016 09:54 AM, Sean Mullan wrote:
>> Please review this fix to improve security defaults by increasing the
>> default keysize of the RSA, DSA, and DiffieHellman implementations of
>> AlgorithmParameterGenerator and KeyPairGenerator from 1024 to 2048 bits:
>> 
>> http://cr.openjdk.java.net/~mullan/webrevs/8138653/webrev.00/
>> 
>> Thanks,
>> Sean
>> 

Re: [9] request for review 8149411: PKCS12KeyStore cannot extract AES Secret Keys

[Vincent Ryan]

Thanks.

> On 15 Feb 2016, at 15:20, Xuelei Fan <xuelei@oracle.com> wrote:
> 
> Looks fine to me.
> 
> Xuelei
> 
> On 2/15/2016 10:54 PM, Vincent Ryan wrote:
>> Please review this fix to PKCS12 keystore implementation that removes an 
>> unnecessary dependency on SecretKeyFactory when extracting a secret key.
>> SecretKeySpec is used instead of SecretKeyFactory because it is JCE 
>> provider-independent.
>> 
>> Webrev: http://cr.openjdk.java.net/~vinnie/8149411/webrev.00/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8149411
>> 
> 

[9] request for review 8149411: PKCS12KeyStore cannot extract AES Secret Keys

[Vincent Ryan]

Please review this fix to PKCS12 keystore implementation that removes an 
unnecessary dependency on SecretKeyFactory when extracting a secret key.
SecretKeySpec is used instead of SecretKeyFactory because it is JCE 
provider-independent.
 
Webrev: http://cr.openjdk.java.net/~vinnie/8149411/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8149411

Re: Request for review: 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation Extension

[Vincent Ryan]

Hello Sean,

An empty array is allowed: it means do not use ALPN.
I’ve updated the exception messages to display the offending length in each 
case.

--- ALPNExtension.java  Tue Dec  1 15:22:02 2015
+++ ALPNExtension.java  Tue Dec  1 14:56:12 2015
@@ -97,11 +97,13 @@
 listLength = s.getInt16(); // list length
 if (listLength < 2 || listLength + 2 != len) {
 throw new SSLProtocolException(
-"Invalid " + type + " extension: incorrect list length");
+"Invalid " + type + " extension: incorrect list length " +
+"(length=" + listLength + ")");
 }
 } else {
 throw new SSLProtocolException(
-"Invalid " + type + " extension: too short");
+"Invalid " + type + " extension: insufficient data " +
+"(length=" + len + ")");
 }

 int remaining = listLength;
@@ -121,7 +123,8 @@

 if (remaining != 0) {
 throw new SSLProtocolException(
-"Invalid " + type + " extension: extra data");
+"Invalid " + type + " extension: extra data " +
+"(length=" + remaining + ")");
 }
 }




> On 1 Dec 2015, at 13:53, Seán Coffey <sean.cof...@oracle.com> wrote:
> 
> Hey Vinnie,
> 
> question on SSLParameters.setApplicationProtocols(String[] protocols) method
> 
> What happens if you pass an empty array into this method. Shouldn't it throw 
> an IllegalArgumentException  ?
> 
> 
> 
> In ALPNExtension.java :
>> +if (listLength < 2 || listLength + 2 != len) {
>> +throw new SSLProtocolException(
>> +"Invalid " + type + " extension: incorrect list 
>> length");
> Can you print the listLength value in exception message ?
> 
>> +throw new SSLProtocolException(
>> +"Invalid " + type + " extension: too short");
> Can you print the len value here ?
> 
>> +if (remaining != 0) {
>> +throw new SSLProtocolException(
>> +"Invalid " + type + " extension: extra data");
> Can you print remaining value here ?
> 
> Regards,
> Sean.
> 
> On 01/12/2015 01:53, Xuelei Fan wrote:
>> Looks fine to me.  Thanks for the update.
>> 
>> Xuelei
>> 
>> On 12/1/2015 7:08 AM, Vincent Ryan wrote:
>>> Thanks for your review. Comments in-line.
>>> 
>>>> On 30 Nov 2015, at 06:30, Xuelei Fan <xuelei@oracle.com> wrote:
>>>> 
>>>> Looks fine to me.  Just a few minor comments.
>>>> 
>>>> ServerHandshaker.java
>>>> =
>>>> There is a typo of the first line.
>>>> - /* Copyright (c) 1996, 2015, ...
>>>> + /*
>>>> +  * Copyright (c) 1996, 2015 …
>>>> 
>>> Done.
>>> 
>>>> line 538-546
>>>> 
>>>>   String negotiatedValue = null;
>>>>   List protocols = clientHelloALPN.getPeerAPs();
>>>> 
>>>>   for (String ap : localApl) {
>>>>   if (protocols.contains(ap)) {
>>>>   negotiatedValue = ap;
>>>>   break;
>>>>   }
>>>>   }
>>>> 
>>>> I think the above order prefer the server preference order of
>>>> application protocols.  It's OK per Section 3.2 of RFC 7301.
>>>> 
>>>> However RFC 7301 also defines the client preference order.  Looks like
>>>> the client preference order is not necessary.  It's a little bit
>>>> confusing, and may result in different behaviors for different TLS vendors.
>>>> 
>>>> Maybe, we can add an option to honor server preference order in the future.
>>> I added a comment to show that server-preference is used.
>>> If necessary I think we can add support for controlling this via a 
>>> property, later.
>>> 
>>> 
>>>> ALPNExtension.java
>>>> ==
>>>> 96listLength = s.getInt16(); // list length
>>>> 
>>>> The code would throws SSLException if the remaining input is not
>>>> sufficient.  I was wondering, SSLProtocolException may be more suitable
>>>> here.  May be nice to check the "len" argument value if sufficient for
>>>> the list length.  Otherwise, a SS

Re: Request for review: 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation Extension

[Vincent Ryan]

Thanks for your review. Comments in-line.

> On 30 Nov 2015, at 06:30, Xuelei Fan <xuelei@oracle.com> wrote:
> 
> Looks fine to me.  Just a few minor comments.
> 
> ServerHandshaker.java
> =
> There is a typo of the first line.
> - /* Copyright (c) 1996, 2015, ...
> + /*
> +  * Copyright (c) 1996, 2015 …
> 
Done.

> 
> line 538-546
> 
>   String negotiatedValue = null;
>   List protocols = clientHelloALPN.getPeerAPs();
> 
>   for (String ap : localApl) {
>   if (protocols.contains(ap)) {
>   negotiatedValue = ap;
>   break;
>   }
>   }
> 
> I think the above order prefer the server preference order of
> application protocols.  It's OK per Section 3.2 of RFC 7301.
> 
> However RFC 7301 also defines the client preference order.  Looks like
> the client preference order is not necessary.  It's a little bit
> confusing, and may result in different behaviors for different TLS vendors.
> 
> Maybe, we can add an option to honor server preference order in the future.

I added a comment to show that server-preference is used.
If necessary I think we can add support for controlling this via a property, 
later.


> 
> ALPNExtension.java
> ==
> 96listLength = s.getInt16(); // list length
> 
> The code would throws SSLException if the remaining input is not
> sufficient.  I was wondering, SSLProtocolException may be more suitable
> here.  May be nice to check the "len" argument value if sufficient for
> the list length.  Otherwise, a SSLProtocolException would be thrown.
> 
>if (len >= 2) {
>listLength = s.getInt16(); // list length
>...
>} else {
>throw new SSLProtocolException(...);
>}

I added the exception and more detailed message.

> 
> ---
> 104   byte[] bytes = s.getBytes8();
> 105   String p =
> 106   new String(bytes, StandardCharsets.UTF_8); // app protocol
> 
> Do you want to check that the app protocol cannot be empty?
> 
> // opaque ProtocolName<1..2^8-1>;  // RFC 7301
> byte[] bytes = s.getBytes8();
> +if (bytes.length == 0) {
> +throw new SSLProtocolException("Empty ...");
> +}

Done.


> 
> -
> Personally, I would prefer to use unmodifiableList for the protocolNames
> variable.

It is accessible only to classes in this package.


> 
> 
> HelloExtensions.java
> 
> line 34-37:
> * This file contains all the classes relevant to TLS Extensions for the
> * ClientHello and ServerHello messages. The extension mechanism and
> * several extensions are defined in RFC 3546. Additional extensions are
> * defined in the ECC RFC 4492.The ALPN extension is defined in RFC7301.
> 
> I may put the "Additional extensions ..." at the end.  BTW, as you are
> here already, would you mind update RFC 3546 to RFC 6066?  RFC 6066 is
> the newest revision of RFC 3546.

I concatenated the final sentence.


> 
> * This file contains all the classes relevant to TLS Extensions for the
> * ClientHello and ServerHello messages. The extension mechanism and
> * several extensions are defined in RFC 6066. The ALPN extension is
> * defined in RFC 7301.  Additional extensions are defined in the ECC
> * RFC 4492.
> 
> SSLEngineImpl.java
> ==
> 
> line 1411-1412:
> private Ciphertext writeRecord(ByteBuffer[] appData,
>int offset, int length, ByteBuffer netData) throws IOException {
>...
>  if (...) {
>...
>// set connection ALPN value
>applicationProtocol =
>handshaker.getHandshakeApplicationProtocol();
>...
>  }
> }
> 
> Is it redundant to set applicationProtocol here.  I was wondering, the
> value should set in the processInputRecord() method.

My understanding is the the wrapping is performed here and the unwrapping 
performed in processInputRecord(). Isn’t the assignment required in both places?



> 
> Cheers,
> Xuelei
> 
> On 11/30/2015 8:08 AM, Vincent Ryan wrote:
>> Hello,
>> 
>> Following on from Brad’s recent email, here is the full webrev of the
>> API and the implementation classes for ALPN:
>>  http://cr.openjdk.java.net/~vinnie/8144093/webrev.00/
>> 
>> In adds the implementation classes (sun/security/ssl) to the public API
>> classes (javax/net/ssl) which have already been agreed.
>> Some basic tests (test/javax/net/ssl) are also included.
>> 
>> Please send any code review comments by close-of-business on Tuesday 1
>> December.
>> Thanks.
> 

Request for review: 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation Extension

[Vincent Ryan]

Hello,

Following on from Brad’s recent email, here is the full webrev of the API and 
the implementation classes for ALPN:
  http://cr.openjdk.java.net/~vinnie/8144093/webrev.00/ 


In adds the implementation classes (sun/security/ssl) to the public API classes 
(javax/net/ssl) which have already been agreed.
Some basic tests (test/javax/net/ssl) are also included.

Please send any code review comments by close-of-business on Tuesday 1 December.
Thanks.

Re: RFR 8143913: MSCAPI keystore should accept Certificate[] in setEntry()

[Vincent Ryan]

Your fix looks good.
Thanks.


> On 24 Nov 2015, at 14:32, Weijun Wang  wrote:
> 
> Please review the fix at
> 
>   http://cr.openjdk.java.net/~weijun/8143913/webrev.00/
> 
> Not everyone passes a X509Certificate[] into KeyStore::setEntry().
> 
> Thanks
> Max

Re: JEP260 -- Impact on SunPKCS11?

[Vincent Ryan]

Hello Glen,

JCE providers are always accessed via the Java SE public APIs and not directly 
via sun.* implementation classes.
In JDK 9, the SunPKCS11 provider continues to be accessible via those APIs. 
It’s implementation classes are present
in the  jdk.crypto.pkcs11 module.

Thanks.


> On 16 Nov 2015, at 15:21, Chris Hegarty  wrote:
> 
> Including the security-dev mailing list.
> 
> -Chris.
> 
> On 16/11/15 12:13, glen.vermey...@telenet.be wrote:
>> In the Devoxx presentation "Prepare for JDK9", the strategy for
>> encapsulating "sun.* " packages is discussed.
>> The class sun.security.SunPkcs11 is not listed on slide 16 ("Uses of
>> JDK-internal APIs"), but as the rest of sun.security.* is listed as
>> "Non-critical, no replacement planned", will this also be case for
>> SunPKCS11?
>> As far as I know there is no alternative security Provider for
>> integrating with PKCS11 aside from rolling your own jni code or using
>> vendor-specific apis.
>> 
>> We rely on SunPKCS for interfacing with an HSM and belgian e-id
>> smartcard. And even though we are aware that touching sun.* is frowned
>> upon, first search hit on "java pkcs11" gives following page:
>> https://docs.oracle.com/javase/7/docs/technotes/guides/security/p11guide.html
>> . With such elaborate documentation, you can't really blame devs to
>> actually use this functionality :) .
>> 
>> Is there an alternative to SunPKCS11 or am I overlooking something?
>> 
>> Thanks for your response,
>> Glen Vermeylen

Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete data

[Vincent Ryan]

Yes the source changes look fine.
Last week I ran the tests using webrev.4 and all passed.

Thanks Christoph for the comprehensive fix.


> On 16 Nov 2015, at 00:32, Wang Weijun  wrote:
> 
> All tests look fine.
> 
> I suppose Vincent already agreed with the src change. Right?
> 
> For safety, I am running all related tests on windows-i586 and windows-x64 
> now.
> 
> Thanks
> Max
> 
>> On 2015年11月16日, at 上午7:27, Langer, Christoph  
>> wrote:
>> 
>> Hi Max,
>> 
>> thanks for that hint about correctly referencing the inner class. Now I 
>> could do it.
>> 
>> As for PrngSlow.java: By having a closer look, I discovered that the test 
>> was always running into an exception, as the algorithm name "PRNG" is not 
>> available for MSCAPI. I renamed it to the correct value of "Windows-PRNG" 
>> and now the test would run. I also cleaned the class and removed the 
>> exception handler so real exceptions should pop up now.
>> 
>> Also, I completed adding the @requires tag for each test.
>> 
>> This is the result: http://cr.openjdk.java.net/~clanger/webrevs/8139436.6/
>> 
>> Best regards
>> Christoph
> 

Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete data

[Vincent Ryan]

I don’t see your testcase in the webrev can you add it in and re-generate.
Thanks.


> On 11 Nov 2015, at 12:08, Langer, Christoph <christoph.lan...@sap.com> wrote:
> 
> Hi Vincent,
>  
> so this is the final version which should be pushed:
> http://cr.openjdk.java.net/~clanger/webrevs/8139436.2/ 
> <http://cr.openjdk.java.net/~clanger/webrevs/8139436.2/>
>  
> I removed an unnecessary blank in a method call and updated the copyright 
> information. I also ran another build and verified with my test case.
>  
> Thanks in advance for taking care.
> Christoph
>  
> From: Vincent Ryan [mailto:vincent.x.r...@oracle.com 
> <mailto:vincent.x.r...@oracle.com>] 
> Sent: Mittwoch, 11. November 2015 08:46
> To: Mike StJohns <mstjo...@comcast.net <mailto:mstjo...@comcast.net>>; 
> Langer, Christoph <christoph.lan...@sap.com <mailto:christoph.lan...@sap.com>>
> Cc: security-dev@openjdk.java.net <mailto:security-dev@openjdk.java.net>
> Subject: Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete 
> data
>  
> Explicitly referencing the Sun JCE provider makes the jdk.crypto.mscapi 
> module depend on the java.base module.
> And that’s OK.
>  
> I can sponsor this changeset.
>  
>  
> On 10 Nov 2015, at 22:39, Mike StJohns <mstjo...@comcast.net 
> <mailto:mstjo...@comcast.net>> wrote:
>  
> On 11/10/2015 4:12 PM, Langer, Christoph wrote:
> Hi folks,
>  
> is there any feedback/review for this change?
> 
> I missed the 4 Nov message when it came past.  No further objections.  But 
> still unclear if modularization will permit this.
> 
> Mike
> 
> 
>  
> Thanks
> Christoph
>  
> From: Langer, Christoph 
> Sent: Mittwoch, 4. November 2015 12:11
> To: 'Michael StJohns' <mstjo...@comcast.net> <mailto:mstjo...@comcast.net>
> Cc: security-dev@openjdk.java.net <mailto:security-dev@openjdk.java.net>
> Subject: RE: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete 
> data
>  
> Hi Mike,
>  
> eventually I’ve made a new webrev implementing your suggestion for a simpler 
> patch:
>  
> http://cr.openjdk.java.net/~clanger/webrevs/8139436.1/ 
> <http://cr.openjdk.java.net/~clanger/webrevs/8139436.1/>
>  
> @All: Is that a valid fix for this issue? Would anyone mind to review/sponsor 
> this?
>  
> Thanks
> Christoph
>  
>  
> From: Michael StJohns [mailto:mstjo...@comcast.net 
> <mailto:mstjo...@comcast.net>] 
> Sent: Donnerstag, 15. Oktober 2015 02:09
> To: Langer, Christoph <christoph.lan...@sap.com 
> <mailto:christoph.lan...@sap.com>>
> Cc: security-dev@openjdk.java.net <mailto:security-dev@openjdk.java.net>
> Subject: Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete 
> data
>  
> Sorry for the top post, I'm wring this on an iPad on a plane.
>  
>  
> It's perfectly acceptable for a provider to prefer its version of stuff over 
> even something early in the provider list.   Usually, that has more to do 
> with key material ( for example pkcs11 instantiations), but there's no reason 
> why it shouldn't apply to certificates.   
>  
> Mike
>  
>  
> 
> Sent from my iPad
> 
> On Oct 14, 2015, at 18:35, Langer, Christoph <christoph.lan...@sap.com 
> <mailto:christoph.lan...@sap.com>> wrote:
> 
> Hi Mike,
>  
> thanks for your comments on this.
>  
> I agree that the change is quite critical and my approach might not be good. 
> However, as for your suggestion to specify the sun provider for the 
> certificate factory, I’d say this is not quite right, too. Because, if you 
> have loaded a different provider on top of the providers list, you’ll intend 
> to use its facilities by default. Maybe the idea would be to use the sun 
> provider as fallback in case of an exception?
>  
> The other suggestion to check for certChain.length in KeyStore.getCertificate 
> and return null would be an improvement compared to throwing an 
> ArrayIndexOutOfBoundsException – however, it would still be difficult to find 
> the root cause why a certificate could not be loaded.
>  
> What do you think?
>  
> @all: Are there other opinions?
>  
> Thanks and best regards
> Christoph
>  
> From: security-dev [mailto:security-dev-boun...@openjdk.java.net 
> <mailto:security-dev-boun...@openjdk.java.net>] On Behalf Of Mike StJohns
> Sent: Mittwoch, 14. Oktober 2015 02:17
> To: security-dev@openjdk.java.net <mailto:security-dev@openjdk.java.net>
> Subject: Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete 
> data
>  
> Hi - 
> 
> I took a look and this probably isn't the correct way 

Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete data

[Vincent Ryan]

Explicitly referencing the Sun JCE provider makes the jdk.crypto.mscapi module 
depend on the java.base module.
And that’s OK.

I can sponsor this changeset.


> On 10 Nov 2015, at 22:39, Mike StJohns  wrote:
> 
> On 11/10/2015 4:12 PM, Langer, Christoph wrote:
>> Hi folks,
>>  
>> is there any feedback/review for this change?
> 
> I missed the 4 Nov message when it came past.  No further objections.  But 
> still unclear if modularization will permit this.
> 
> Mike
> 
>>  
>> Thanks
>> Christoph
>>  
>> From: Langer, Christoph 
>> Sent: Mittwoch, 4. November 2015 12:11
>> To: 'Michael StJohns'  
>> Cc: security-dev@openjdk.java.net 
>> Subject: RE: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete 
>> data
>>  
>> Hi Mike,
>>  
>> eventually I’ve made a new webrev implementing your suggestion for a simpler 
>> patch:
>>  
>>  
>> http://cr.openjdk.java.net/~clanger/webrevs/8139436.1/
>>  
>>  
>> @All: Is that a valid fix for this issue? Would anyone mind to 
>> review/sponsor this?
>>  
>> Thanks
>> Christoph
>>  
>>  
>> From: Michael StJohns [mailto:mstjo...@comcast.net 
>> ] 
>> Sent: Donnerstag, 15. Oktober 2015 02:09
>> To: Langer, Christoph < 
>> christoph.lan...@sap.com 
>> >
>> Cc: security-dev@openjdk.java.net 
>> Subject: Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete 
>> data
>>  
>> Sorry for the top post, I'm wring this on an iPad on a plane.
>>  
>>  
>> It's perfectly acceptable for a provider to prefer its version of stuff over 
>> even something early in the provider list.   Usually, that has more to do 
>> with key material ( for example pkcs11 instantiations), but there's no 
>> reason why it shouldn't apply to certificates.   
>>  
>> Mike
>>  
>>  
>> 
>> Sent from my iPad
>> 
>> On Oct 14, 2015, at 18:35, Langer, Christoph < 
>> christoph.lan...@sap.com 
>> > wrote:
>> 
>> Hi Mike,
>>  
>> thanks for your comments on this.
>>  
>> I agree that the change is quite critical and my approach might not be good. 
>> However, as for your suggestion to specify the sun provider for the 
>> certificate factory, I’d say this is not quite right, too. Because, if you 
>> have loaded a different provider on top of the providers list, you’ll intend 
>> to use its facilities by default. Maybe the idea would be to use the sun 
>> provider as fallback in case of an exception?
>>  
>> The other suggestion to check for certChain.length in 
>> KeyStore.getCertificate and return null would be an improvement compared to 
>> throwing an ArrayIndexOutOfBoundsException – however, it would still be 
>> difficult to find the root cause why a certificate could not be loaded.
>>  
>> What do you think?
>>  
>> @all: Are there other opinions?
>>  
>> Thanks and best regards
>> Christoph
>>  
>> From: security-dev [ 
>> mailto:security-dev-boun...@openjdk.java.net
>>  ] On Behalf Of Mike StJohns
>> Sent: Mittwoch, 14. Oktober 2015 02:17
>> To: security-dev@openjdk.java.net 
>> Subject: Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete 
>> data
>>  
>> Hi - 
>> 
>> I took a look and this probably isn't the correct way to fix this. 
>> 
>> A simpler change might be to specify the sun provider when requesting the 
>> certificate factory.I hesitate to say that definitively as 
>> modularization guidance may restrict that approach?
>> 
>> The belt and suspenders approach is to catch the bad certificate exception 
>> and return null.  That appears to be the correct contract for 
>> KeyStore.getCertificate(String alias).   (e.g. "if (certChain.length == 0) 
>> return null;")
>> 
>> Mike
>> 
>> 
>> On 10/12/2015 5:04 PM, Langer, Christoph wrote:
>> Hi,
>>  
>> please review a change proposal regarding an issue in the Microsoft Security 
>> API (mscapi).
>>  
>> Bug:  
>> https://bugs.openjdk.java.net/browse/JDK-8139436
>>  
>> Webrev:  
>> http://cr.openjdk.java.net/~clanger/webrevs/8139436.0/
>>  
>>  
>> I stumbled over the issue when using an old IAIK security provider. It would 
>> throw java.security.cert.CertificateException upon parsing ECC based 
>> certificates when generating Certificate objects. The way it is right now, 
>> such exceptions are silently caught and the Windows Keystore is created with 

Re: RFR :8136534: Loading JKS keystore using non-null InputStream results in closed stream

[Vincent Ryan]

Looks good to me.

> On 14 Oct 2015, at 15:59, Seán Coffey  wrote:
> 
> Backporting this fix to jdk8u-dev sources.
> 
> The jdk9 changeset didn't apply cleanly but the fix in essence is the removal 
> of a try-with-resources block. Tests are green.
> 
> bug report : https://bugs.openjdk.java.net/browse/JDK-8136534
> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8136534.8u/webrev/
> 
> -- 
> Regards,
> Sean.
> 

[9] request for review: 8136534: Loading JKS keystore using non-null InputStream results in closed stream

[Vincent Ryan]

Please review this JDK 9 fix to stop KeyStore.load from closing the input 
stream passed to it.
Thanks.

Bug: https://bugs.openjdk.java.net/browse/JDK-8136534
Webrev: http://cr.openjdk.java.net/~vinnie/8136534/webrev.00/

Re: [9] RFR: 8134232: KeyStore.load() throws an IOException with a wrong cause in case of wrong password

[Vincent Ryan]

Your fix looks fine Artem.
Thanks.


> On 11 Sep 2015, at 12:46, Artem Smotrakov  wrote:
> 
> Hello,
> 
> Please review this for 9.
> 
> According to [1], KeyStore.load(InputStream, char[]) method should throw an 
> IOException, and the cause of the IOException should be an 
> UnrecoverableKeyException:
> 
> ...
> Throws:
> IOException - if there is an I/O or format problem with the keystore data, if 
> a password is required but not given, or if the given password was incorrect. 
> If the error is due to a wrong password, the cause of the IOException should 
> be an UnrecoverableKeyException
> ...
> 
> But in case of PKCS11, PKCS12 and JCEKS keystores it throws an IOException, 
> but the cause is not UnrecoverableKeyException.
> 
> This fix updates PKCS11, PKCS12 and JCEKS keystore implementations to use 
> UnrecoverableKeyException in case of wrong password. Also updated a couple of 
> existing tests. Please note that an additional test will be added in 
> https://bugs.openjdk.java.net/browse/JDK-8048622 (review pending).
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8134232
> Webrev: http://cr.openjdk.java.net/~asmotrak/8134232/webrev.00/
> 
> [1] 
> http://docs.oracle.com/javase/8/docs/api/java/security/KeyStore.html#load-java.io.InputStream-char:A-
> 
> Artem

Re: [9] RFR: 8048622: Enhance tests for PKCS11 keystores with NSS

[Vincent Ryan]

Your fix looks fine.
Thanks.


> On 21 Aug 2015, at 21:34, Artem Smotrakov  wrote:
> 
> Hello,
> 
> Please review a couple of changes for PKCS11 tests:
> - Added a new test which checks that a keystore can't be loaded with 
> incorrect password,
>  found https://bugs.openjdk.java.net/browse/JDK-8134232
> - Added a check that a trusted entry can be removedfrom keystore
> - Updated existing tests not to use random data
> - Updated existing tests to use try-with-resources for I/O operationswith 
> files
> - Small cosmetic updates
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8048622
> Webrev: http://cr.openjdk.java.net/~asmotrak/8048622/webrev.00/
> 
> Artem

Re: RFR 8136425: KeystoreImpl.m using wrong type for cert format

[Vincent Ryan]

Your fix looks fine to me.
Thanks.


> On 12 Sep 2015, at 10:23, Wang Weijun  wrote:
> 
> Please take a look at
> 
>http://cr.openjdk.java.net/~weijun/8136425/webrev.00/ 
> 
> 
> It looks like a wrong type is used, kSecFormatX509Cert and 
> kSecFormatWrappedPKCS8 are of SecExternalFormat and the SecKeychainItemImport 
> function also uses this type for its 3rd argument. Constants and function are 
> all defined in
> 
>
> http://www.opensource.apple.com/source/libsecurity_keychain/libsecurity_keychain-40768/lib/SecImportExport.h
>  
> 
> 
> It is an enum, so still works.
> 
> Thanks
> Max
> 

Re: [9] Urgent RFR 8135099 "9-dev solaris builds failed on 2015-09-04"

[Vincent Ryan]

Your fix looks fine to me.
Thanks.

> On 8 Sep 2015, at 21:33, Valerie Peng  wrote:
> 
> Hi,
> 
> I need someone to help reviewing for this bug ASAP:
> http://cr.openjdk.java.net/~valeriep/8135099/webrev.00/
> 
> The build is broken due to the compilation warning introduced in 8130875 
> which slipped through un-detected due to the "--disable-warnings-as-errors" 
> setting in my local workspace.
> 
> Thanks,
> Valerie
> 

8130800: KeyStore.getInstance(File, char[]) does not throw IOE for null password

[Vincent Ryan]

Please review this spec change in java.security.KeyStore to clarify that a 
keystore integrity check is not performed when a null password is supplied.
Thanks.

Webrev: http://cr.openjdk.java.net/~vinnie/8130800/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8130800

[9] Request for review 8134112: api/java_security/KeyStore/index.html#GetInstance2Tests[getInstanceIOE2, getInstanceIOE3] still fail after JDK-8130850 is fixed in b77

[Vincent Ryan]

Please review this change to the KeyStore.getInstance(File,LoadStoreParameter) 
method to correctly handle its LoadStoreParameter argument.
It now extracts the embedded password before calling the 
KeyStore.load(InputStream,char[]) method.

This fix corrects a JCK test that had been failing.
Thanks.


Webrev: http://cr.openjdk.java.net/~vinnie/8134112/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8134112

Re: [9] code review request for 8130799: KeyStoreSpi.engineProbe does not throw the expected NullPointerException

[Vincent Ryan]

Thanks Xuelei, I’ll add that message.

 On 19 Aug 2015, at 16:56, Xuelei Fan xuelei@oracle.com wrote:
 
 Looks fine to me.  Don't you want to add a exception message to indicate
 the input stream is null?
 
 Xuelei
 
 On 8/18/2015 7:23 PM, Vincent Ryan wrote:
 Please review this trivial change to the KeyStoreSpi.engineProbe method
 so that it matches its specification wrt NPE.
 Thanks.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8130799
 Webrev: http://cr.openjdk.java.net/~vinnie/8130799/webrev.00/
 

[9] review request for 8132786: java/security/cert/CertPathValidator/OCSP/AIACheck.java fails intermittently

[Vincent Ryan]

Please review this fix for an intermittently failing test. It now expects a 
SocketException or a SocketTimeoutException.
Thanks.

Bug: https://bugs.openjdk.java.net/browse/JDK-8132786
Webrev: http://cr.openjdk.java.net/~vinnie/8132786/webrev.00/

[9] code review request for 8130799: KeyStoreSpi.engineProbe does not throw the expected NullPointerException

[Vincent Ryan]

Please review this trivial change to the KeyStoreSpi.engineProbe method so that 
it matches its specification wrt NPE.
Thanks.

Bug: https://bugs.openjdk.java.net/browse/JDK-8130799 
https://bugs.openjdk.java.net/browse/JDK-8130799
Webrev: http://cr.openjdk.java.net/~vinnie/8130799/webrev.00/ 
http://cr.openjdk.java.net/~vinnie/8130799/webrev.00/

Re: [9] review request for 8133318: Exclude intermittent failing PKCS11 tests on Solaris SPARC 11.1 and earlier

[Vincent Ryan]

The process exec for uname is only made when the test is run on Solaris SPARC 
earlier than 11.2 but I take your point.
I’ll look at caching its value and re-using the util methods.



 On 11 Aug 2015, at 13:28, Sean Mullan sean.mul...@oracle.com wrote:
 
 There are bunch of methods for determining os.version, etc in 
 jdk/testlibrary/Platform that might be better to reuse.
 
 Did you consider caching the value of uname -v so it can be reused by other 
 tests? It seems expensive to exec a process just to get that value every time 
 one of these tests is run.
 
 --Sean
 
 On 08/11/2015 07:34 AM, Vincent Ryan wrote:
 Please review this change to omit several PKCS11 tests from test runs on 
 Solaris SPARC 11.1 and earlier.
 The tests had been failing intermittently.
 
 Webrev: http://cr.openjdk.java.net/~vinnie/8133318/webrev.00/
 Bug: https://bugs.openjdk.java.net/browse/JDK-8133318
 
 Thanks.
 

[9] review request for 8133318: Exclude intermittent failing PKCS11 tests on Solaris SPARC 11.1 and earlier

[Vincent Ryan]

Please review this change to omit several PKCS11 tests from test runs on 
Solaris SPARC 11.1 and earlier.
The tests had been failing intermittently.

Webrev: http://cr.openjdk.java.net/~vinnie/8133318/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8133318

Thanks.

Re: [9] RFR: 8129306: Some new tests developed for JDK-8085979 fail in jdk9/cpu

[Vincent Ryan]

Looks fine to me.

 On 16 Jul 2015, at 19:08, Konstantin Shefov konstantin.she...@oracle.com 
 wrote:
 
 Hello,
 
 Please review a test bug fix.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8129306
 Webrev: http://cr.openjdk.java.net/~kshefov/8129306/webrev.00/
 
 -Konstantin

[9] request for review 8131359 Correct the JTREG tags in java/security/KeyStore/PKCS12/MetadataStoreLoadTest.java test

[Vincent Ryan]

Please approve this correction to the 
java/security/KeyStore/PKCS12/MetadataStoreLoadTest.java test was failing to 
compile.
Thanks.

Bug: https://bugs.openjdk.java.net/browse/JDK-8131359
Webrev: http://cr.openjdk.java.net/~vinnie/8131359/webrev.00/

Re: [9] request for review 8131184: Add test sun/security/pkcs11/rsa/TestKeyPairGenerator.java to the problem list

[Vincent Ryan]

Thanks Sean.

 On 14 Jul 2015, at 20:09, Sean Mullan sean.mul...@oracle.com wrote:
 
 Looks fine to me.
 
 --Sean
 
 On 07/14/2015 02:43 PM, Vincent Ryan wrote:
 Adding the intermittently failing test 
 sun/security/pkcs11/rsa/TestKeyPairGenerator.java to the problem list.
 
 
 Webrev: http://cr.openjdk.java.net/~vinnie/8131184/webrev.00/
 Bug: https://bugs.openjdk.java.net/browse/JDK-8131184
 

[9] request for review 8131184: Add test sun/security/pkcs11/rsa/TestKeyPairGenerator.java to the problem list

[Vincent Ryan]

Adding the intermittently failing test 
sun/security/pkcs11/rsa/TestKeyPairGenerator.java to the problem list.


Webrev: http://cr.openjdk.java.net/~vinnie/8131184/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8131184

Re: [9] RFR: 8130041: TsacertOptionTest.java intermittently fails on Mac

[Vincent Ryan]

Your fix looks fine.
Thanks.


 On 11 Jul 2015, at 17:36, Artem Smotrakov artem.smotra...@oracle.com wrote:
 
 Hello,
 
 Please review this fix for TsacertOptionTest.java test.
 
 The test fails intermittently on some Macs. It starts a local TSA service 
 which is actually an HTTP server. Then, it runs jarsigner which uses this 
 local TSA service. The failure happens on some Macs due to system network 
 settings. It seems that on such machines jarsigner tries to connect to local 
 TSA via proxy that can't redirect a request to TSA. The fix updates the test 
 to reset a couple of system properties for proxy settings.
 
 The test passes with this patch. There are some other tests which use a local 
 TSA in sun/security/tools/jarsigner, but they don't seem to be affected by 
 the problem.
 
 Webrev: http://cr.openjdk.java.net/~asmotrak/8130041/webrev.00/
 Bug: https://bugs.openjdk.java.net/browse/JDK-8130041
 
 Artem

[9] code review request for 8130850: Support loading a keystore with a custom KeyStore.LoadStoreParameter class

[Vincent Ryan]

Please review this change to the implementation of the KeyStoreSpi.engineLoad 
method to accept custom KeyStore.LoadStoreParameter classes.
Thanks.

Bug: https://bugs.openjdk.java.net/browse/JDK-8130850
Webrev: http://cr.openjdk.java.net/~vinnie/8130850/webrev.00/

Re: [9] review request for 8130151: Exclude sun/security/provider/SecureRandom/StrongSecureRandom.java from testruns on MacOSX 10.10

[Vincent Ryan]

Thanks Xuelei.

 On 1 Jul 2015, at 01:44, Xuelei Fan xuelei@oracle.com wrote:
 
 Looks fine to me.
 
 Xuelei
 
 On 7/1/2015 1:34 AM, Vincent Ryan wrote:
 Please approve this temporary addition to the ProblemList to exclude a
 single test on Mac OS X 10.10 only.
 It is related to the issue being tracked
 by https://bugs.openjdk.java.net/browse/JDK-8051770
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8130151
 Webrev: http://cr.openjdk.java.net/~vinnie/8130151/webrev.00/
 
 Thanks.
 

Re: RFR 8098854: Do cleanup in a proper order in mscapi

[Vincent Ryan]

Your fix looks fine to me.
Thanks.


 On 1 Jul 2015, at 18:04, Ivan Gerasimov ivan.gerasi...@oracle.com wrote:
 
 Hello!
 
 Would someone please help review this small cleanup fix?
 I'll go on vacation in a week, and I would like to push it before then.
 
 Sincerely yours,
 Ivan
 
 On 19.06.2015 22:17, Ivan Gerasimov wrote:
 Hello everyone!
 
 The changeset consists of two parts:
 - checking systematically for NULL result of FindClass() and GetMethodID(),
 - calling CryptReleaseContext() after CryptDestroyHash(), as it is suggested 
 in MSDN [1].
 
 BUGURL: https://bugs.openjdk.java.net/browse/JDK-8098854
 WEBREV: http://cr.openjdk.java.net/~igerasim/8098854/00/webrev/
 
 [1] 
 https://msdn.microsoft.com/en-us/library/windows/desktop/aa380268(v=vs.85).aspx
 
 Sincerely yours,
 Ivan
 
 
 
 

[9] review request for 8130151: Exclude sun/security/provider/SecureRandom/StrongSecureRandom.java from testruns on MacOSX 10.10

[Vincent Ryan]

Please approve this temporary addition to the ProblemList to exclude a single 
test on Mac OS X 10.10 only.
It is related to the issue being tracked by 
https://bugs.openjdk.java.net/browse/JDK-8051770 
https://bugs.openjdk.java.net/browse/JDK-8051770

Bug: https://bugs.openjdk.java.net/browse/JDK-8130151
Webrev: http://cr.openjdk.java.net/~vinnie/8130151/webrev.00/ 
http://cr.openjdk.java.net/~vinnie/8130151/webrev.00/

Thanks.