Re: RFR JDK-8029661: JDK-Support TLS v1.2 algorithm in SunPKCS11 provider

[Valerie Peng]

Hi Martin,

Sorry for the delay, I am able to resume on reviewing this now.

Here are some initial comments.

What about CKM_TLS12_MAC? I see that it's defined under TLS 1.2 
mechanisms, but not much other details for it.



- Is this change still necessary? I didn't notice any new import 
statement with sun.security.ssl package in the rest of changes.



- Why removing the SENSITIVE_FALSE attribute on line 829 and 854?


- As my comments for the SunPKCS11.java (see below), I think we should 
not assume the support for CKM_TLS12_KEY_AND_MAC_DERIVE. The existing 
impl and how it's registered in SunPKCS11 class is already somewhat 
problematic when CKM_TLS_KEY_AND_MAC_DERIVE is not supported. We should 
avoid assuming CKM_TLS12_KEY_AND_MAC_DERIVE is supported which may 
create even more problems. Overriding this.mechanism based on the TLS 
version specified in the parameters from the engineInit(...) call will 
lead to failure when the mechanism is not supported by underlying PKCS11 
library



- Please see above comments for P11TlsKeyMaterialGenerator.java


- Retrieve label outside of the new code block for TLS 1.2, i.e. line 
133- 167, as it's used by all mechanisms.



- Line 131, Is it really necessary to use SunTls12RsaPremasterSecret? 
SunJCE provider uses SunTlsRsaPremasterSecret for all cases. If 
different algorithm is not needed, then no need to save tlsVersion as a 
field



- Hmm, for TLS 12 specific mechanisms, some PKCS11 libraries may not 
support them. Instead of registering 
SunTls12MasterSecret/SunTls12KeyMaterial as aliases, they should be 
registered separately so that if the native TLS 12 mechanisms were not 
supported, the new entry will be skipped. The corresponding 
implementation classes such as P11TlsMasterSecretGenerator and 
P11TlsKeyMaterialGenerator will have to check the specified parameter 
spec in their engineInit(..) calls to make sure things line up, e.g. 
error out if the TLS version in the spec does not match the native 
mechanism.
- Lines 528 - 532, the mapping is stored without checking for support. 
May become an issue when the underlying PKCS11 library does not support 
all of these hash mechanisms. Probably not a big deal as these are 
fairly common hash algorithms, but I think it'd be good to add a comment 
on line 527 with something like "// assuming all hash mechanisms below 
are supported".



- Miss the mapping for CKM_TLS12_KEY_AND_MAC_DERIVE? Move these new 
entries to be after the existing SSL3/TLS entries (from line 711-721)


I still have a few files left and will send you comments on them later.
Thanks,
Valerie



On 7/13/2018 2:08 PM, Valerie Peng wrote:


Thanks for updating the webrev, I will take a look.

Valerie


On 7/10/2018 10:18 AM, Martin Balao wrote:

Hi,

Webrev 04 for JDK-8029661 is ready:

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.04.zip 

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.04/ 



New:

 * Rebased to latest JDK revision (after TLS 1.3 merge)
  * Rev 1acfd2f56d72
 * ProtocolVersion dependencies removed (raw TLS protocol version 
numbers are now used)
 * Code style improvements (tabs, trailing whitespaces, max lines 
length, etc.)


Thanks,
Martin.-

Re: RFR[11] JDK-8206171: Signature#getParameters for RSASSA-PSS throws ProviderException when not initialized

[Weijun Wang]

Something related.

Cipher has a similar init(..,params) and getParameters() structure and the spec 
is also similar.

* The returned parameters may be the same that were used to initialize
* this cipher, or may contain a combination of default and random
* parameter values used by the underlying cipher implementation if this
* cipher requires algorithm parameters but was not initialized with any.

However, one can supply an incomplete parameters object in init() and 
getParameters() will fill in default/random values to make it complete.

For example, in PBE-based Cipher, one can only include salt and iteration count 
in the init params, and init() will add in a random IV, and the IV can be 
retrieved with getParameters().

Is this something we need to clarify?

Thanks
Max

> On Jul 25, 2018, at 7:59 AM, Bradford Wetmore  
> wrote:
> 
> Returning to this briefly, looks good to me too.
> 
> Brad
> 
> 
> 
> On 7/19/2018 6:23 PM, Valerie Peng wrote:
>> Thanks Max and Sean for the review and suggestions. I have updated the 
>> webrev accordingly and finalized CSR.
>> Webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.04/
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8206864
>> Valerie
>> On 7/19/2018 3:13 PM, Wang Weijun wrote:
>>> 
 在 2018年7月20日，05:18，Valerie Peng  写道：
 
 Hi Sean,
 
 Thanks for the suggestion, I like it.
>>> Me too.
>>> 
 Max, any objection or concern before I update the webrev and CSR?
>>> No.
>>> 
>>> Thanks
>>> Max
>>> 
 Valerie
 
 
> On 7/19/2018 7:28 AM, Sean Mullan wrote:
> Hi Valerie, Max -
> 
> I took a fresh look at this issue this morning. I think we are getting 
> bogged down by trying to adjust within the current wording, which is 
> somewhat awkward and hard to understand. So, I think it might be better 
> to break up the wording into multiple sentences. Here's a cut at the 
> rewording:
> 
> /**
>   * Returns the parameters used with this signature object.
>   *
>   *  If this signature has been previously initialized with parameters
>   * (by calling the {@code setParameter} method), this method returns
>   * the same parameters. If this signature has not been initialized with
>   * parameters, this method may return a combination of default and
>   * randomly generated parameter values if the underlying
>   * signature implementation supports it and can successfully generate
>   * them. Otherwise, {@code null} is returned.
>   *
>   * @return the parameters used with this signature, or {@code null}
>   */
> 
> In the first sentence of the 2nd paragraph above, I wanted to first list 
> the case where the signature is initialized with parameters, which is the 
> most clear-cut case of what the behavior will be. Then I described the 
> case where an implementation may return default/generated parameters -- 
> and this is clearly marked "may" since it is optional. All other cases 
> other than those two always return null, so I think this makes it easier 
> to understand.
> 
> --Sean
> 
>> On 7/18/18 1:29 PM, Valerie Peng wrote:
>> Sean,
>> 
>> Where do you think that we should add the part about "null must be 
>> returned ..." paragraph? At the end of first or second paragraph?
>> 
>> I will go with majority.
>> 
>> Valerie
>> 
>>> On 7/17/2018 8:38 PM, Weijun Wang wrote:
>>> Is it better to append the new lines to the 2nd paragraph?
>>> 
>>> Thanks
>>> Max
>>> 
 On Jul 18, 2018, at 9:46 AM, Valerie Peng  
 wrote:
 
 
 Ok, let's use "must" then. I have also added the part about default 
 parameters.
 Hope that all is clear now.
 
 Latest webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.03/
 Latest CSR: https://bugs.openjdk.java.net/browse/JDK-8206864
 
 Thanks,
 Valerie
 
 On 7/17/2018 5:50 PM, Weijun Wang wrote:
>> On Jul 18, 2018, at 8:23 AM, Valerie Peng  
>> wrote:
>> 
>> Hi Max,
>> 
>> Thanks for the suggestions. Please find comments inline.
>> 
>>> On 7/16/2018 7:38 PM, Weijun Wang wrote:
>>> CSR at https://bugs.openjdk.java.net/browse/JDK-8206864.
>>> 
>>> - At the end of the 1st paragraph, you have now
>>> 
 However, for signature algorithm such as "RSASSA-PSS", it 
 requires parameters and the one used for signing must be supplied 
 for verification to succeed. It may be better to return null 
 instead of returning provider-specific default parameters.
>>> I suggest we don't talk about the reason (params must be the same 
>>> for signing and verification), we can just say something like "If 
>>> there is no provider-specific default 

Re: RFR[11] JDK-8206171: Signature#getParameters for RSASSA-PSS throws ProviderException when not initialized

[Bradford Wetmore]

Returning to this briefly, looks good to me too.

Brad



On 7/19/2018 6:23 PM, Valerie Peng wrote:
Thanks Max and Sean for the review and suggestions. I have updated the 
webrev accordingly and finalized CSR.


Webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.04/
CSR: https://bugs.openjdk.java.net/browse/JDK-8206864

Valerie

On 7/19/2018 3:13 PM, Wang Weijun wrote:



在 2018年7月20日，05:18，Valerie Peng  写道：

Hi Sean,

Thanks for the suggestion, I like it.

Me too.


Max, any objection or concern before I update the webrev and CSR?

No.

Thanks
Max


Valerie



On 7/19/2018 7:28 AM, Sean Mullan wrote:
Hi Valerie, Max -

I took a fresh look at this issue this morning. I think we are 
getting bogged down by trying to adjust within the current wording, 
which is somewhat awkward and hard to understand. So, I think it 
might be better to break up the wording into multiple sentences. 
Here's a cut at the rewording:


/**
  * Returns the parameters used with this signature object.
  *
  *  If this signature has been previously initialized with 
parameters

  * (by calling the {@code setParameter} method), this method returns
  * the same parameters. If this signature has not been initialized 
with

  * parameters, this method may return a combination of default and
  * randomly generated parameter values if the underlying
  * signature implementation supports it and can successfully generate
  * them. Otherwise, {@code null} is returned.
  *
  * @return the parameters used with this signature, or {@code null}
  */

In the first sentence of the 2nd paragraph above, I wanted to first 
list the case where the signature is initialized with parameters, 
which is the most clear-cut case of what the behavior will be. Then 
I described the case where an implementation may return 
default/generated parameters -- and this is clearly marked "may" 
since it is optional. All other cases other than those two always 
return null, so I think this makes it easier to understand.


--Sean


On 7/18/18 1:29 PM, Valerie Peng wrote:
Sean,

Where do you think that we should add the part about "null must be 
returned ..." paragraph? At the end of first or second paragraph?


I will go with majority.

Valerie


On 7/17/2018 8:38 PM, Weijun Wang wrote:
Is it better to append the new lines to the 2nd paragraph?

Thanks
Max

On Jul 18, 2018, at 9:46 AM, Valerie Peng 
 wrote:



Ok, let's use "must" then. I have also added the part about 
default parameters.

Hope that all is clear now.

Latest webrev: 
http://cr.openjdk.java.net/~valeriep/8206171/webrev.03/

Latest CSR: https://bugs.openjdk.java.net/browse/JDK-8206864

Thanks,
Valerie

On 7/17/2018 5:50 PM, Weijun Wang wrote:
On Jul 18, 2018, at 8:23 AM, Valerie Peng 
 wrote:


Hi Max,

Thanks for the suggestions. Please find comments inline.


On 7/16/2018 7:38 PM, Weijun Wang wrote:
CSR at https://bugs.openjdk.java.net/browse/JDK-8206864.

- At the end of the 1st paragraph, you have now

    However, for signature algorithm such as "RSASSA-PSS", it 
requires parameters and the one used for signing must be 
supplied for verification to succeed. It may be better to 
return null instead of returning provider-specific default 
parameters.
I suggest we don't talk about the reason (params must be the 
same for signing and verification), we can just say something 
like "If there is no provider-specific default parameters, 
this method should return null before user sets one".
Alright, I initially didn't put down the reason. But since Brad 
asked about it, I add it to the CSR in case Joe raise the same 
question. I will update the CSR per your suggestion.

- null may be returned

How about "{@code null} must be returned"?
How about "should"? Is there a guideline on when to use 
"may/should/must"? Anyone knows?
Even if there were guidelines on this for Java, I think we 
should just stick to https://tools.ietf.org/html/rfc2119, except 
that the capitalization is not necessary.


"Must" is precise here.

I thought must is mostly used in mandating input arguments must 
not be null. But don't recall it being used much for return 
values.
"must return" appears 39 times in java/ and "should return" 19 
(simple grep, no line break).


--Max


Thanks,
Valerie


Everything else looks fine.

Thanks
Max

On Jul 17, 2018, at 9:46 AM, Valerie Peng 
 wrote:


Hi Max,

Good catch on the SignatureSpi class and SunMSCAPI provider, 
I will include them.


As for the part about "randomly generated", I am leaning 
toward not having it as I don't see  a value of specifying this.


Webrev and CSR has been updated.

New webrev: 
http://cr.openjdk.java.net/~valeriep/8206171/webrev.02/


Thanks,
Valerie


On 7/16/2018 4:29 PM, Weijun Wang wrote:
Valerie

Would you like to retain the word "randomly generated" 
somewhere?


Also, the SignatureSpi class needs to be updated in the same 
way.


Can you also update the implementation in the MSCAPI 
Signature object?


Thanks
Max

On Jul 17, 2018, at 6:16 AM, Valerie 

RFR 8076190: Support passwordless access to PKCS12 keystores

[Weijun Wang]

Please review the code change and CSR at

   webrev: http://cr.openjdk.java.net/~weijun/8076190/webrev.01/
   CSR: https://bugs.openjdk.java.net/browse/JDK-8202590

The bug is at

   https://bugs.openjdk.java.net/browse/JDK-8076190

This is the 1st part of the process to make cacerts using pkcs12:

1. Support passwordless access to PKCS12 keystores
2. Update default algorithms and params when creating a PKCS12 keystore
3. Update keytool to support passwordless pkcs12 keystores
4. Migrate cacerts to pkcs12

Thanks
Max

Re: RFR[11] JDK-8206258: [Test Error] sun/security/pkcs11 tests fail if NSS libs not found

[sha . jiang]

Hi Valerie,
Thanks for your review!
Please take a look at this new webrev: 
http://cr.openjdk.java.net/~jjiang/8206258/webrev.02


On 2018/7/24 06:18, Valerie Peng wrote:

Hi John,

Changes look fine.

I just have one nit, perhaps add more information reporting when 
skipping tests, e.g.


PKCS11Test: line 163

Different tests may have different reasons for skipping testing.
So, it would be better to output the info in PKCS11Test's child classes.
In fact, the tests overriding the method PKCS11Test::skipTest already 
report the reasons respectively.



TestNssDbSqlite.java: line 68.

Add the below line
120 System.out.println("Cannot init security module 
database, skipping");


Best regards,
John Jiang


Thanks,
Valerie

On 7/9/2018 12:38 AM, sha.ji...@oracle.com wrote:

Hi Thomas,
Thanks for your testing.

I'm not sure that's a reasonable case.
From my view, PKCS11Test.java simply checks if the NSS library 
directory exists.

But it looks unnecessary to check every library file.
In fact, if removing libnss3 or libsoftokn3's dependencies, like 
libnssutil3, the test also fails.


However, I still re-checked my previous solution, and made a new 
webrev [1].

The constant badNSSVersion in PKCS11Test.java may not be fine.
The static field nss_library in PKCS11Test.java can be softokn3 or 
nss3 for different tests.
badNSSVersion should be checked after the target nss library is 
determined.
And this checking should happen before the real testing, especially 
before security manager is enabled.
So, a new extension method, exactly PKCS11Test::skipTest, was 
introduced, and the affected tests were modified accordingly.


[1] http://cr.openjdk.java.net/~jjiang/8206258/webrev.01/

Best regards,
John Jiang

On 2018/7/4 14:15, Thomas Stüfe wrote:

Hi,

Unfortunately this is not enough.

Running tests with your patch and NSS libs disabled (I renamed
libsoftokn3.so) yields the following errors:

sun/security/pkcs11/Secmod/AddPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/AddTrustedCert.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/Crypto.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/GetPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/JksSetPrivateKey.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/LoadKeystore.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/TestNssDbSqlite.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS
sun/security/pkcs11/Secmod/TrustAnchors.java
  Failed. Execution
failed: `main' threw exception: java.security.ProviderException: Could
not initialize NSS


Excerpt from TestNssDbSqlite.jtr:

--messages:(3/98)--
command: build TestNssDbSqlite
reason: Named class compiled on demand
elapsed time (seconds): 0.0
result: Passed. All files up to date

#section:main
--messages:(5/721)--
command: main TestNssDbSqlite
reason: User specified action: run main/othervm/timeout=120 
TestNssDbSqlite

Mode: othervm [/othervm specified]
Additional options from @modules: --add-modules
java.base,jdk.crypto.cryptoki --add-exports
java.base/sun.security.rsa=ALL-UNNAMED --add-exports
java.base/sun.security.provider=ALL-UNNAMED --add-exports
java.base/sun.security.jca=ALL-UNNAMED --add-exports
java.base/sun.security.tools.keytool=ALL-UNNAMED --add-exports
java.base/sun.security.x509=ALL-UNNAMED --add-exports
java.base/com.sun.crypto.provider=ALL-UNNAMED --add-exports
jdk.crypto.cryptoki/sun.security.pkcs11=ALL-UNNAMED --add-opens
jdk.crypto.cryptoki/sun.security.pkcs11=ALL-UNNAMED
elapsed time (seconds): 0.445
--configuration:(11/604)--
Boot Layer
   add modules: java.base jdk.crypto.cryptoki
   add exports: java.base/com.sun.crypto.provider ALL-UNNAMED
    java.base/sun.security.jca ALL-UNNAMED
    java.base/sun.security.provider ALL-UNNAMED
    java.base/sun.security.rsa ALL-UNNAMED
    java.base/sun.security.tools.keytool ALL-UNNAMED
    java.base/sun.security.x509 ALL-UNNAMED