RFR 8213007: Update the link in test/jdk/sun/security/provider/SecureRandom/DrbgCavp.java
Oops, I should have updated the link in the actual CAVP test whiling fixing JDK-8212867. Here is it: diff --git a/test/jdk/sun/security/provider/SecureRandom/DrbgCavp.java b/test/jdk/sun/security/provider/SecureRandom/DrbgCavp.java --- a/test/jdk/sun/security/provider/SecureRandom/DrbgCavp.java +++ b/test/jdk/sun/security/provider/SecureRandom/DrbgCavp.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2016, 2018, 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 @@ -46,8 +46,9 @@ import static java.security.DrbgParameters.Capability.*; /** - * The Known-output DRBG test. The test vector can be obtained from - * http://csrc.nist.gov/groups/STM/cavp/documents/drbg/drbgtestvectors.zip. + * The Known-output DRBG test. The test vector can be downloaded from + * https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Algorithm-Validation-Program/documents/drbg/drbgtestvectors.zip. + * The test is described on https://csrc.nist.gov/Projects/Cryptographic-Algorithm-Validation-Program/Random-Number-Generators. * * Manually run this test with * Thanks Max
Re: RFR 8026953: Add support for MS Cryptography next generation (CNG) (step 1)
Hi Mike
Thanks for the feedback.
I understand the current SunMSCAPI implementation recognizes RSA keys only and
it's certainly incorrect to put something like getModulus() and
getPublicExponent() in a general CKey class. They will be fixed later. When I
have more sub class names, I'll definitely use them. You can see I've moved
some CSignature methods into CSignature$RSA. I just haven't done it everywhere.
We'll still need a base CKey for a CNG key, no matter what the underlying
algorithm is. Since CNG uses the same NCRYPT_KEY_HANDLE type for different
types of keys, we will do something similar on the Java side. Since the current
CPublicKey and CPrivateKey are very light, it will be easy to move the content
into algorithm-specific classes.
The main reason I want to take this first step is that after some study on CNG
I make some progress and also see some blockers. For example, I am able to
expose a EC CNG key stored in Windows-MY now and use it to sign/verify.
However, I still don't know how to import external keys inside there
(certmgr.exe can so it's possible). Until now, the most requested function is
to use existing keys in signatures and I want to start working on it. The first
thing I noticed then is that the current class names are unsuitable and I think
a refactoring will make them look better.
Once I start working on the next step, I'll need to have different sub classes
in CKey and CSignature. I even have an alternative plan to ditch CPublicKey and
use the PublicKey classes in SunEC and SunRsaSign. This was actually already
used in the RSASSA-PSS signature handling in SunMSCAPI we added into JDK 11 in
the last minute.
As for KeyFactory, we do not have an urgent requirement to use external keys in
a CNG Signature object or store them into Windows-MY. Also, we can use the one
in SunRsaSign.
Thanks again.
--Max
> On Oct 26, 2018, at 1:25 AM, Michael StJohns wrote:
>
> Hi Max -
>
> For the same reason I was pushing back on Adam's EC provider I think I need
> to push back here. You're recasting an RSAPublicKey to just a PublicKey and
> making it difficult to move key material in and out of the MSCAPI proivider.
> Same thing with the private key.
>
> For example, in the CPublicKey class you still have "getModulus()" and
> "getPublicExponent()", but to get at them you'd have to use CPublicKey rather
> than PublicKey.
>
> And looking forward, I'm not sure how you actually implement the EC classes
> here using this model.
>
> I'd suggest not making the change this way and overloading the existing
> classes, but instead adding the appropriate provider classes for new key
> types as you implement support for them. E.g. Keep CRSAKey, CRSAPublicKey
> and CRSAPrivateKey as distinct classes, add CECKey, CECPublicKey and
> CECPrivateKey when you get there.
>
> Are you missing a KeyFactory class as well?
>
> Lastly, you may want to change the subclass/methods for CSignature (and
> probably other classes) to reflect the type of Signature you're returning:
>
>> if (algo.equals("NONEwithRSA")) {
>>
>> -return new RSASignature.Raw();
>> +return new CSignature.Raw();
>
> Instead: "return new CSignature.RSARaw()"
>
> And this definitely isn't going to work if you have even one other Cipher
> you'll be returning:
>> if (algo.equals("RSA") ||
>> algo.equals("RSA/ECB/PKCS1Padding")) {
>>
>> -return new RSACipher();
>> +return new CCipher();
>>
>> }
>>
>
>
> Later, Mike
>
>
>
>
>
> On 10/25/2018 4:38 AM, Weijun Wang wrote:
>> Please review the change at
>>
>>
>> https://cr.openjdk.java.net/~weijun/8026953/webrev.00/
>>
>>
>> (I will use a sub-task id for this change but currently JBS is down).
>>
>> The major change is renaming classes. Since we are going to support
>> algorithms other than RSA, I've renamed the classes like RSAPrivateKey ->
>> CPrivateKey. Classes that have the same name as JCA classes (like Key,
>> KeyStore) are also renamed (to CKey, CKeyStore) so it's easy to tell them
>> apart.
>>
>> Others are not about renaming but they are also related to supporting other
>> algorithms, and there is no behavior change. They include:
>>
>> - CKey (plus its child classes CPublicKey and CPrivateKey) has a new field
>> "algorithm". This field is used by
>> CKeyStore::generateRSAKeyAndCertificateChain and its value is obtained from
>> the public key algorithm in a cert [1].
>>
>> - Child class named "RSA" of CKeyPairGenerator.
>>
>> - Child class named "RSA" of CSignature. I also moved some RSA-related
>> methods into this child class as overridden methods.
>>
>> - CKeyStore::setPrivateKey's key parameter has a new type Key, but it still
>> only accepts RSAPrivateCrtKey now.
>>
>> Noreg-cleanup.
>>
>> Thanks
>> Max
>>
>> [1]
>>
Re: RFR 8026953: Add support for MS Cryptography next generation (CNG) (step 1)
Hi Max -
For the same reason I was pushing back on Adam's EC provider I think I
need to push back here. You're recasting an RSAPublicKey to just a
PublicKey and making it difficult to move key material in and out of the
MSCAPI proivider. Same thing with the private key.
For example, in the CPublicKey class you still have "getModulus()" and
"getPublicExponent()", but to get at them you'd have to use CPublicKey
rather than PublicKey.
And looking forward, I'm not sure how you actually implement the EC
classes here using this model.
I'd suggest not making the change this way and overloading the existing
classes, but instead adding the appropriate provider classes for new key
types as you implement support for them. E.g. Keep CRSAKey,
CRSAPublicKey and CRSAPrivateKey as distinct classes, add CECKey,
CECPublicKey and CECPrivateKey when you get there.
Are you missing a KeyFactory class as well?
Lastly, you may want to change the subclass/methods for CSignature (and
probably other classes) to reflect the type of Signature you're returning:
if (algo.equals("NONEwithRSA")) {
- return new RSASignature.Raw();
+ return new CSignature.Raw();
Instead: "return new CSignature.RSARaw()"
And this definitely isn't going to work if you have even one other
Cipher you'll be returning:
if (algo.equals("RSA") ||
algo.equals("RSA/ECB/PKCS1Padding")) {
- return new RSACipher();
+ return new CCipher();
}
Later, Mike
On 10/25/2018 4:38 AM, Weijun Wang wrote:
Please review the change at
https://cr.openjdk.java.net/~weijun/8026953/webrev.00/
(I will use a sub-task id for this change but currently JBS is down).
The major change is renaming classes. Since we are going to support algorithms
other than RSA, I've renamed the classes like RSAPrivateKey -> CPrivateKey.
Classes that have the same name as JCA classes (like Key, KeyStore) are also
renamed (to CKey, CKeyStore) so it's easy to tell them apart.
Others are not about renaming but they are also related to supporting other
algorithms, and there is no behavior change. They include:
- CKey (plus its child classes CPublicKey and CPrivateKey) has a new field
"algorithm". This field is used by CKeyStore::generateRSAKeyAndCertificateChain
and its value is obtained from the public key algorithm in a cert [1].
- Child class named "RSA" of CKeyPairGenerator.
- Child class named "RSA" of CSignature. I also moved some RSA-related methods
into this child class as overridden methods.
- CKeyStore::setPrivateKey's key parameter has a new type Key, but it still
only accepts RSAPrivateCrtKey now.
Noreg-cleanup.
Thanks
Max
[1]
https://docs.microsoft.com/en-gb/windows/desktop/api/wincrypt/ns-wincrypt-_crypt_algorithm_identifier
Re: RFR 8212867: Link to DRBG test vectors is redirected to a broken link
On 10/24/18 9:55 PM, Weijun Wang wrote:
It's a part of @implNote, and I added it there to show this
implementation is compliant. I'm OK with removing it. Just like this:
Fine by me.
--Sean
*diff --git
a/src/java.base/share/classes/java/security/DrbgParameters.java
b/src/java.base/share/classes/java/security/DrbgParameters.java*
*--- a/src/java.base/share/classes/java/security/DrbgParameters.java*
*+++ b/src/java.base/share/classes/java/security/DrbgParameters.java*
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2016, 2017, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2016, 2018, 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
@@ -225,10 +225,6 @@
*
* Calling {@link SecureRandom#generateSeed(int)} will directly read
* from this system default entropy source.
- *
- * This implementation has passed all tests included in the 20151104
version of
- * href="http://csrc.nist.gov/groups/STM/cavp/documents/drbg/drbgtestvectors.zip;>
- * The DRBG Test Vectors.
*
* @since 9
*/
Thanks
Max
On Oct 25, 2018, at 1:54 AM, Sean Mullan > wrote:
IMHO, this is kind of an odd thing to include in the javadocs, the
fact that it passed a bunch of tests. I'd be more inclined to simply
remove this sentence and (maybe) instead include it in the JDK
Providers Guide, but even then I am not sure it is really necessary.
--Sean
On 10/23/18 10:27 PM, Weijun Wang wrote:
I'd like to refine the patch a little to
*diff --git
a/src/java.base/share/classes/java/security/DrbgParameters.java b/src/java.base/share/classes/java/security/DrbgParameters.java*
*--- a/src/java.base/share/classes/java/security/DrbgParameters.java*
*+++ b/src/java.base/share/classes/java/security/DrbgParameters.java*
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2016, 2017, Oracle and/or its affiliates. All
rights reserved.
+ * Copyright (c) 2016, 2018, 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
@@ -226,9 +226,9 @@
* Calling {@link SecureRandom#generateSeed(int)} will directly read
* from this system default entropy source.
*
- * This implementation has passed all tests included in the 20151104
version of
- * href="http://csrc.nist.gov/groups/STM/cavp/documents/drbg/drbgtestvectors.zip;>
- * The DRBG Test Vectors.
+ * This implementation has passed the DRBG Test Vectors (published
on 2015-11-04) in
+ * href="https://csrc.nist.gov/Projects/Cryptographic-Algorithm-Validation-Program/Random-Number-Generators;>
+ * CAVP Testing: Random Number Generators.
*
* @since 9
*/
While "Cryptographic Algorithm Validation Program" is the main title
shown on the page, it looks like the secondary header "CAVP Testing:
Random Number Generators" is more precise and matches the URL. I also
change the test vectors name to "DRBG Test Vectors" because the
section could contain more links to other RNG test vectors.
Sorry for the quick update.
Thanks
Max
On Oct 24, 2018, at 10:18 AM, Weijun Wang > wrote:
Please take a review at the fix below:
diff --git
a/src/java.base/share/classes/java/security/DrbgParameters.java b/src/java.base/share/classes/java/security/DrbgParameters.java
--- a/src/java.base/share/classes/java/security/DrbgParameters.java
+++ b/src/java.base/share/classes/java/security/DrbgParameters.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2016, 2017, Oracle and/or its affiliates. All
rights reserved.
+ * Copyright (c) 2016, 2018, 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
@@ -226,9 +226,9 @@
* Calling {@link SecureRandom#generateSeed(int)} will directly read
* from this system default entropy source.
*
- * This implementation has passed all tests included in the
20151104 version of
- * href="http://csrc.nist.gov/groups/STM/cavp/documents/drbg/drbgtestvectors.zip;>
- * The DRBG Test Vectors.
+ * This implementation has passed the test vectors (published on
2015-11-04) in
+ * href="https://csrc.nist.gov/Projects/Cryptographic-Algorithm-Validation-Program/Random-Number-Generators;>
+ * Cryptographic Algorithm Validation Program.
*
* @since 9
*/
Note that the link does not point to a zip file now. It might be a
little rude to link to a 13MB zip file, and a description page is
better. The page is quite concise and has a section named "Test
Vectors" containing the link to the zip file. The zip file has
actually no version info but the content shows the data was
generated on 2015-11-04. I'd like to keep the date info in case the
test vectors are
RFR 8026953: Add support for MS Cryptography next generation (CNG) (step 1)
Please review the change at https://cr.openjdk.java.net/~weijun/8026953/webrev.00/ (I will use a sub-task id for this change but currently JBS is down). The major change is renaming classes. Since we are going to support algorithms other than RSA, I've renamed the classes like RSAPrivateKey -> CPrivateKey. Classes that have the same name as JCA classes (like Key, KeyStore) are also renamed (to CKey, CKeyStore) so it's easy to tell them apart. Others are not about renaming but they are also related to supporting other algorithms, and there is no behavior change. They include: - CKey (plus its child classes CPublicKey and CPrivateKey) has a new field "algorithm". This field is used by CKeyStore::generateRSAKeyAndCertificateChain and its value is obtained from the public key algorithm in a cert [1]. - Child class named "RSA" of CKeyPairGenerator. - Child class named "RSA" of CSignature. I also moved some RSA-related methods into this child class as overridden methods. - CKeyStore::setPrivateKey's key parameter has a new type Key, but it still only accepts RSAPrivateCrtKey now. Noreg-cleanup. Thanks Max [1] https://docs.microsoft.com/en-gb/windows/desktop/api/wincrypt/ns-wincrypt-_crypt_algorithm_identifier
