Hi Max,
Please find comments in line.
On 11/22/2018 6:04 AM, Weijun Wang wrote:
On Nov 22, 2018, at 9:14 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
Hi Max,
Here are some comments:
<src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/CSignature.java>
- line 39, add PSS here as well.
- line 97, CSignature ctr, better to initialize all fields
Which particular ones are you thinking about? privateKey and publicKey will be
initialized later and messageDigest/messageDigestAlgorithm/needsReset will be
useless when digestName == null.
I was referring messageDigest/messageDigestAlgorithm/needsReset. My
general preference is to set them to null/false even when not used, may
help catch future coding problems.
- line 109, has key algorithm been checked by JCA already? If not, it should be
checked here. Same goes for line 414, and 560
Can I do this later? This sub-task is meant to be a cleanup.
My point for checking the key algorithm is to ensure that the specified
key object is RSA type. After this cleanup and refactoring, the
specified key should be checked to be of RSA type, i.e. line 111, 130.
Otherwise, the casting to various RSA public/private key interfaces,
e.g. java.security.interfaces.RSAPublicKey, may fail.
Updates look fine.
Regards,
Valerie
But I think I need to move more RSA related methods into the RSA subclass.
- with the class renaming, I think it's clearer to include "RSA" as part of the subclass names,
e.g. "MD2withRSA" instead of just "MD2"
OK.
- line 681: can be static, right? pkg-private?
Yes. Looks like it can be private.
<src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/CPublicKey.java>
- line 120: maybe a ProviderException instead of IllegalArgumentException as
the 'alg' is not from user but rather inside the provider.
Either is OK for me. Since it's only called inside the provider, it can even be
an AssertionError.
- line 127, add @Override
OK.
- line 140, must getPublicKeyBob be public? Maybe pkg private?
Correct.
<src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/CKeyStore.java>
- line 119: why not just use RSAPrivateCrtKey instead of Key? The caller has
already did an instanceof check before this method.
Correct. I think I should rename both setPrivateKey and generatePrivateKeyBlob
to setRSAPrivateKey and generateRSAPrivateKeyBlob. If one day I starts
supporting EC keys the methods will be quite different.
<test/jdk/sun/security/mscapi/KeyAlgorithms.java>
- would be nice to have a String constant for "RSA".
Sure.
Thanks
Max
Rest looks fine.
Thanks,
Valerie
On 11/15/2018 7:40 AM, Weijun Wang wrote:
Oops, my copy/paste sequence goes wrong.
On Nov 15, 2018, at 11:38 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
Webrev updated at
https://cr.openjdk.java.net/~weijun/8213009/webrev.01/
More refactorings:
- getEncoded and getFormat of CKey removed, implemented in CPublicKey and
CPrivateKey.
- CPublicKey has child class CRSAPublicKey, CKeyPairGenerator has child class
RSA.
- CPublicKey and CPrivateKey now have a static of() method that can return a
child instance.
- CCipher renamed to CRSACipher. I realized there won't be CECCipher.
Thanks
Max
On Nov 7, 2018, at 12:13 AM, Weijun Wang <weijun.w...@oracle.com> wrote:
Webrev updated at
https://cr.openjdk.java.net/~weijun/8213009/webrev.00/
The subtask id is now used.
The previous refactoring has removed the "RSA" algorithm info from some keys.
This update adds them back.
Thanks
Max
On Oct 25, 2018, at 4:38 PM, Weijun Wang <weijun.w...@oracle.com> 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
