<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Max,</p>
<p>Please find comments in line.<br>
</p>
<div class="moz-cite-prefix">On 11/22/2018 6:04 AM, Weijun Wang
wrote:
</div>
<blockquote type="cite"
cite="mid:D39CD181-893A-456A-A389-E47F48B95C96@oracle.com">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On Nov 22, 2018, at 9:14 AM, Valerie Peng <a class="moz-txt-link-rfc2396E" href="mailto:valerie.peng@oracle.com"><valerie.peng@oracle.com></a> 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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Which particular ones are you thinking about? privateKey and publicKey will be initialized later and messageDigest/messageDigestAlgorithm/needsReset will be useless when digestName == null.
</pre>
</blockquote>
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. <br>
<blockquote type="cite"
cite="mid:D39CD181-893A-456A-A389-E47F48B95C96@oracle.com">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">- line 109, has key algorithm been checked by JCA already? If not, it should be checked here. Same goes for line 414, and 560
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Can I do this later? This sub-task is meant to be a cleanup.</pre>
</blockquote>
<p>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 <span
class="new">RSA public/private key interfaces, e.g.
java.security.interfaces.RSAPublicKey, may fail.</span></p>
<p><span class="new">Updates look fine.</span></p>
<span class="new">Regards,</span><br>
<span class="new"></span><br>
<span class="new">Valerie</span><br>
<span class="new"></span>
<p><span class="new"></span>
</p>
<blockquote type="cite"
cite="mid:D39CD181-893A-456A-A389-E47F48B95C96@oracle.com">
<pre class="moz-quote-pre" wrap="">But I think I need to move more RSA related methods into the RSA subclass.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">- 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"
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
OK.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">- line 681: can be static, right? pkg-private?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Yes. Looks like it can be private.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
<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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Either is OK for me. Since it's only called inside the provider, it can even be an AssertionError.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">- line 127, add @Override
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
OK.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">- line 140, must getPublicKeyBob be public? Maybe pkg private?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Correct.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
<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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
<test/jdk/sun/security/mscapi/KeyAlgorithms.java>
- would be nice to have a String constant for "RSA".
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Sure.
Thanks
Max
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Rest looks fine.
Thanks,
Valerie
On 11/15/2018 7:40 AM, Weijun Wang wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Oops, my copy/paste sequence goes wrong.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On Nov 15, 2018, at 11:38 PM, Weijun Wang <a class="moz-txt-link-rfc2396E" href="mailto:weijun.wang@oracle.com"><weijun.wang@oracle.com></a> wrote:
Webrev updated at
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""> <a class="moz-txt-link-freetext" href="https://cr.openjdk.java.net/~weijun/8213009/webrev.01/">https://cr.openjdk.java.net/~weijun/8213009/webrev.01/</a>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On Nov 7, 2018, at 12:13 AM, Weijun Wang <a class="moz-txt-link-rfc2396E" href="mailto:weijun.wang@oracle.com"><weijun.wang@oracle.com></a> wrote:
Webrev updated at
<a class="moz-txt-link-freetext" href="https://cr.openjdk.java.net/~weijun/8213009/webrev.00/">https://cr.openjdk.java.net/~weijun/8213009/webrev.00/</a>
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
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On Oct 25, 2018, at 4:38 PM, Weijun Wang <a class="moz-txt-link-rfc2396E" href="mailto:weijun.wang@oracle.com"><weijun.wang@oracle.com></a> wrote:
Please review the change at
<a class="moz-txt-link-freetext" href="https://cr.openjdk.java.net/~weijun/8026953/webrev.00/">https://cr.openjdk.java.net/~weijun/8026953/webrev.00/</a>
(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] <a class="moz-txt-link-freetext" href="https://docs.microsoft.com/en-gb/windows/desktop/api/wincrypt/ns-wincrypt-_crypt_algorithm_identifier">https://docs.microsoft.com/en-gb/windows/desktop/api/wincrypt/ns-wincrypt-_crypt_algorithm_identifier</a>
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
</html>