Re: [11] RFR: JDK-8197441: Signature#initSign/initVerify for an invalid private/public key fails with ClassCastException for SunPKCS11 provider

[Valerie Peng]

Sure, I'll change it to "correct type".
Thanks,
Valerie

On 2/15/2018 4:46 PM, Anthony Scarpino wrote:

On 02/15/2018 04:40 PM, Valerie Peng wrote:


Anyone can help review this trivial fix? It'll just take a minute or so.
Essentially, the fix is to re-throw with InvalidKeyException when the 
casting failed.


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

Thanks,
Valerie




I think "correct type" is better, but the word choice is up to you. 
Consider it reviewed.


Tony

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

[Valerie Peng]

I am ok with the idea of putting the public API refactoring under a 
separate RFE.

Let me apply your updated patch below and run existing regressions again.
Thanks,
Valerie

On 2/5/2018 5:51 AM, Martin Balao wrote:

Hi Valerie,

Thanks for your review.

Here it's the new webrev updated to the new repository structure. I've 
also updated the testcase to use the new @module jtreg feature:


 * 
http://cr.openjdk.java.net/~akasko/mbalao/jdk_8029661_tls_12_sunpkcs11/2018_02_02/8029661.webrev.02 
 
(online)
 * 
http://cr.openjdk.java.net/~akasko/mbalao/jdk_8029661_tls_12_sunpkcs11/2018_02_02/8029661.webrev.02.zip 
 
(download)


A few comments in regards to creating public APIs from 
sun.security.internal.spec classes.


Classes in jdk/src/java.base/share/classes/sun/security/internal/spec:
 * TlsKeyMaterialParameterSpec.java
 * TlsMasterSecretParameterSpec.java
 * TlsRsaPremasterSecretParameterSpec.java
 * TlsKeyMaterialSpec.java
 * TlsPrfParameterSpec.java

Classes in jdk/src/java.base/share/classes/java/security/spec (which I 
assume would be the destination):

 * AlgorithmParameterSpec.java
 * ECGenParameterSpec.java
 * InvalidParameterSpecException.java
 * RSAOtherPrimeInfo.java
 * DSAGenParameterSpec.java
 * ECParameterSpec.java
 * KeySpec.java
 * ...

TlsRsaPremasterSecretParameterSpec class contains information about 
min and max SSL/TLS version and, optionally, the pre-master encoded 
key. This information may be useful to any 3rd party class that 
implements a KeyGenerator to generate RSA pre-master secrets.


TlsMasterSecretParameterSpec class contains information (client/server 
random, pre-master secret, hash algorithm, etc.) to generate a master 
secret from a pre-master secret.


TlsKeyMaterialParameterSpec class contains information (client/server 
random, master secret, hash algorithm, etc.) to generate keys for a 
session from a master secret.


TlsPrfParameterSpec class contains information (secret key, label, 
hash algorithm, etc.) to generate handshake authentication codes.


TlsKeyMaterialSpec class contains information about session keys. This 
class inherits from SecretKey class.


So, I agree with you: these parameters/specs may be used by a 3rd 
party and would be better to have them as public interfaces.


However, I suggest to address that in a new ticket because:
 * it is not strictly inherent to SunPKCS11 + TLS 1.2 support we are 
providing in the context of this ticket;
 * it would be more clear both in tickets documentation and repository 
changeset;
 * this refactoring is going to go through CSR, which SunPKCS11 + TLS 
1.2 support does not need; and,

 * we should also evaluate how TLS 1.3 changes going to impact into this.

Would splitting this into a new ticket work for you?

Kind regards,
Martin.-

Re: [11] RFR: JDK-8197441: Signature#initSign/initVerify for an invalid private/public key fails with ClassCastException for SunPKCS11 provider

[Anthony Scarpino]

On 02/15/2018 04:40 PM, Valerie Peng wrote:


Anyone can help review this trivial fix? It'll just take a minute or so.
Essentially, the fix is to re-throw with InvalidKeyException when the 
casting failed.


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

Thanks,
Valerie




I think "correct type" is better, but the word choice is up to you. 
Consider it reviewed.


Tony

[11] RFR: JDK-8197441: Signature#initSign/initVerify for an invalid private/public key fails with ClassCastException for SunPKCS11 provider

[Valerie Peng]

Anyone can help review this trivial fix? It'll just take a minute or so.
Essentially, the fix is to re-throw with InvalidKeyException when the 
casting failed.


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

Thanks,
Valerie

Re: PKCS#11 provider issues with min and max size

[Valerie Peng]

Yes, please go ahead and file a bug for this.
Thanks!
Valerie

On 2/13/2018 6:00 AM, Tomas Gustavsson wrote:

Thanks for taking a look.

I haven't created a bug for this (yet) Let me know if that would help.

Regards,
Tomas
On 2018-02-09 20:04, Valerie Peng wrote:

Hmm, seems reasonable to support this in the provider configuration file.
Or, on a similar note, but slightly different approach is to just add an
configuration option for disabling checking the supported key size range.
Regards,
Valerie

On 2/9/2018 2:16 AM, Tomas Gustavsson wrote:

I just realized that a natural place to configure provider behavior is
in the provider construction, which is also per provider, so you can
have multiple ones with different configuration. We are already using an
InputStream to construct SunPKCS11, and adding more parameters to
configure/override defaults would be natural.

I.e.:
-
name =  
library = ;
slot = slot
rsakeygenmech = CKM_RSA_X9_31_KEY_PAIR_GEN
rsakeygenmechminsize = 1024
rsakeygenmechmaxsize = 8192

attributes(*, CKO_PUBLIC_KEY, *) = {
    CKA_TOKEN = false
    CKA_ENCRYPT = true
    CKA_VERIFY = true
    CKA_WRAP = true
}
attributes(*, CKO_PRIVATE_KEY, *) = {
    CKA_DERIVE = false
    CKA_TOKEN = true
    CKA_PRIVATE = true
    CKA_SENSITIVE = true
    CKA_EXTRACTABLE = false
    CKA_DECRYPT = true
    CKA_SIGN = true
    CKA_UNWRAP = true
}
attributes(*, CKO_SECRET_KEY, *) = {
    CKA_SENSITIVE = true
    CKA_EXTRACTABLE = false
    CKA_ENCRYPT = true
    CKA_DECRYPT = true
    CKA_SIGN = true
    CKA_VERIFY = true
    CKA_WRAP = true
    CKA_UNWRAP = true
}
-

Cheers,
Tomas

On 2018-02-09 09:55, Tomas Gustavsson wrote:

Hi,

Thanks for the answer. (sorry I was out with the flu for a week)


I am not too keen to add an env var/system property to accommodate this
kind of PKCS11 library bugs since this should be rare I hope.
Valerie

Unfortunately I don't see it as rare and the impact is huge due to the
slow turnaround of HSM firmware. Due to FIPS certification and whatnot
HSM vendors do not fix simple things like this sometimes in years. This
puts customers to a complete halt in the meantime. These are heavily
certified environments where re-certification alone takes at least 6
months.

Having worked with all major HSM vendors for many years I know that
PKCS11 library bugs are not rare at all. If we'd be using PKCS#11
natively, you can hack around by ignoring bad values, but when using
SunPKCS#11 you are stuck with Java's rules unless you patch SunPKCS11
which is not for everyone.

Due to the strictness of using SunPKCS11 compared to native PKCS#11,
some more flexibility in configuration would help a lot.

For many years SunPKCS11 have worked great. But also the HSM world is
moving faster than they did 5 years ago, and unfortunately this means
that we're seeing a huge rise in PKCS#11 issues in the last year,
requiring quite a lot of hacking in SunPKCS11 to workaround. In theory
it should not be needed, but in practice it is. Faster evolution = more
bugs.

I just showed two real world use cases that you really need to be able
to work around. And these will not be the last. PKCS#11 is a complex
standard and implementors will rarely get it exactly right.

Increased flexibility and more control around PKCS#11 will be needed in
the future imho, or users will be forced to move to other solutions like
JackNJI11. We'd much rather use SunPKCS11 but customers (end users)
can't get stuck between Java and HSM vendors in a fight who does PKCS#11
right or wrong.

Cheers,
Tomas

On 2018-02-01 01:07, Valerie Peng wrote:

Thanks for the feedback. I suppose we can ignore values which obviously
don't make sense such as 0 or max being less than min key size.
However, if the underlying PKCS11 library vendors forgot to update the
max value as in your comment#2, supposedly they should fix it.
I am not too keen to add an env var/system property to accommodate this
kind of PKCS11 library bugs since this should be rare I hope.
Valerie

On 1/30/2018 12:22 AM, Tomas Gustavsson wrote:

Hi,

At some revision in the PKCS#11 provider there was introduced checking
of C_GetMechanismInfo min and max sizes.

This has turned out to be a bit fragile. Let me give two real world
examples:

1. Amazon Cloud HSM report minSize and maxSize for EC keys to 0. The
Java PKCS#11 provider will happily take 0 as maxSize and refuse to
generate any EC keys at all. Needless to say, without the Java
check it
would be no problem.

131: C_GetMechanismInfo
2018-01-30 07:52:20.740
[in] slotID = 0x1
    CKM_EC_KEY_PAIR_GEN
[out] pInfo:
CKM_EC_KEY_PAIR_GEN   : min:0 max:0 flags:0x10001 ( Hardware
KeyPair )
Returned:  0 CKR_OK

(we are reporting this to Amazon as well)

2. Thales HSMs (some?) report maxSize for RSA_PKCS key generation as
4096, but will happily generate 8192 bit keys. I.e. the reported
maxSize
is not true.
We have customers who used to generate 8192 bit RSA keys, but after a
Java update can not do so anymore, because Java 

Re: RFR: 8193892: Impact of noncloneable MessageDigest implementation

[Valerie Peng]

Looks fine to me.
Thanks,
Valerie

On 2/15/2018 1:04 AM, Seán Coffey wrote:

A reminder for this review..

regards,
Sean.


On 09/02/2018 16:25, Seán Coffey wrote:
Looking to push a new test which helps test CloneableDigest code. 
It's something that arose during JDK-8193683 fix.


The test was contributed by Brad Wetmore. I've converted it to use 
the SSLSocketTemplate.


JBS : https://bugs.openjdk.java.net/browse/JDK-8193892
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8193892/webrev/

Re: RFR: 8193892: Impact of noncloneable MessageDigest implementation

[Xuelei Fan]

Looks fine to me.  Thanks!

Xuelei

On 2/15/2018 1:04 AM, Seán Coffey wrote:

A reminder for this review..

regards,
Sean.


On 09/02/2018 16:25, Seán Coffey wrote:
Looking to push a new test which helps test CloneableDigest code. It's 
something that arose during JDK-8193683 fix.


The test was contributed by Brad Wetmore. I've converted it to use the 
SSLSocketTemplate.


JBS : https://bugs.openjdk.java.net/browse/JDK-8193892
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8193892/webrev/

Re: RFR: 8193892: Impact of noncloneable MessageDigest implementation

[Seán Coffey]

A reminder for this review..

regards,
Sean.


On 09/02/2018 16:25, Seán Coffey wrote:
Looking to push a new test which helps test CloneableDigest code. It's 
something that arose during JDK-8193683 fix.


The test was contributed by Brad Wetmore. I've converted it to use the 
SSLSocketTemplate.


JBS : https://bugs.openjdk.java.net/browse/JDK-8193892
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8193892/webrev/