<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Jamil,</p>
<p>Sure, I will add these two OIDs as well. If there is discussion
on oid registry, I'd like to join to explore potential ideas.</p>
<p>Will re-test w/ mach5 and update the webrev in place and
integrate once the mach5 job is finished.<br>
</p>
Thanks!<br>
Valerie<br>
<div class="moz-cite-prefix">On 6/10/2019 9:07 AM, Jamil Nimeh
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:3080e7c0-6e6e-4966-7eda-cf318757f52b@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
Hi Valerie,<br>
<br>
Sorry I didn't answer your question regarding where I found the <span
class="new">dsa-with-sha384 and dsa-with-sha512 OIDs. I usually
check OIDs from <a class="moz-txt-link-freetext"
href="http://www.oid-info.com" moz-do-not-send="true">http://www.oid-info.com</a>.
I grabbed the OID without the leaf node (</span><span
class="new">2.16.840.1.101.3.4.3) and checked your sha224 and
sha256, which were correct. Then I noticed the registry had 384
and 512 which were missing in your definitions and figured I'd
mention it.<br>
<br>
I agree with you about having an OID registry. In fact, working
on 8076999 I came to a similar conclusion and Weijun and I have
been throwing around some ideas on how to do that. We haven't
gone too far into just talk mostly about how best to set it up
and handle unknown/supported OIDs.<br>
<br>
Regardless of whether you wish to add those two OIDs or not, the
review as a whole looks good to me.<br>
<br>
--Jamil<br>
<br>
</span>
<div class="moz-cite-prefix">On 6/6/2019 7:50 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:f51f9333-ba66-e730-3802-15d60f460596@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
<p>Webrev updated: <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~valeriep/8080462/webrev.02/"
moz-do-not-send="true">http://cr.openjdk.java.net/~valeriep/8080462/webrev.02/</a></p>
<p>Mach5 run looks clean.</p>
Thanks,<br>
Valerie<br>
<div class="moz-cite-prefix">On 6/5/2019 7:42 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:24bd05f8-809b-df80-1551-74d732c2e4b4@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
<p>Hi Jamil,</p>
<p>Thanks much for reviewing this~<br>
</p>
<div class="moz-cite-prefix">On 6/5/2019 9:21 AM, Jamil Nimeh
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
Hi Valerie, on the whole it looks really good. I do have
some comments below:<br>
<br>
<ul>
<li>SunPKCS11.java</li>
<ul>
<li>728-738: I think you could add <span class="new">2.16.840.1.101.3.4.3.3
and .4 for dsa-with-sha384 and dsa-with-sha512,
respectively.</span></li>
</ul>
</ul>
</blockquote>
<p>Hmm, I didn't find the oids defined for DSA signature with
SHA384 and SHA512 digests off oid registry search. Where did
you find the oids, just curious?</p>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
<li><span class="new">790-792: Are you sure that's the
right OID? OID lookup shows it as PKCS10 and has </span><span
class="new">1.2.840.113549.1.1.10 as rsassa-pss.</span></li>
</ul>
</ul>
</blockquote>
<p>Good catch, I missed one component ".1". In hindsight, it'd
be nice to have a sun.security,util.OidMapping utility class
which handles the oid and string name aliasing. It's a pain
and error prone to repeat these inside the providers. Or,
maybe JCA can handle this instead of each provider.<br>
</p>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
</ul>
<li><span class="new">CK_MECHANISM.java</span></li>
<ul>
<li><span class="new">171: Can a CK_MECHANISM object be
reused after the action that calls freeHandle() is
completed? If so, would it be a good idea to return
the value of this.pHandle back to 0 (assuming it is
nonzero, of course)?</span></li>
</ul>
</ul>
</blockquote>
The CK_MECHANISM object itself may be reused. However, the
parameters it contained may change depending on whether the
engineSetParameter(...) is called again with different value.
So, the current model is to allocate/set the pHandle for every
init call and free it after sign/verify call. When it is
freed, it must be reset to 0. Otherwise it may lead to runtime
crash later. <br>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
</ul>
<li><span class="new">CK_RSA_PKCS_PSS_PARAMS.java</span></li>
<ul>
<li><span class="new">55-61: Seems like you could
replace all of removeDash() with just String's
replaceFirst("-", "") method.<br>
</span></li>
<li><span class="new">74: I see in a lot of equals
methods an identity check as well, like "if (this ==
o) { return true; }" maybe add that in before you
check the contents of "o"?</span></li>
</ul>
</ul>
</blockquote>
Sure, changed.<br>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
</ul>
<li><span class="new">Functions.java</span></li>
<ul>
<li><span class="new">412: Typo: Vender -> Vendor</span></li>
</ul>
</ul>
</blockquote>
Fixed.<br>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
</ul>
<li><span class="new">PKCS11.java</span></li>
<ul>
<li><span class="new">747, 825: Looks like there's a bit
of header comment rot. But I'm guessing that could
be said of other methods in this file that you have
not modified. Think it's worth updating the
comments?<br>
</span></li>
</ul>
</ul>
</blockquote>
Ok, I updated the comments for whose are modified by this
change.<br>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<li><span class="new">PKCS11Constants.java</span></li>
<ul>
<li><span class="new">362-363: [Nit] Looks like you've
been adding deprecated markings for other
attributes. According to the header file I'm
looking at CKA_SECONDARY_AUTH and CKA_AUTH_PIN_FLAGS
are deprecated.</span></li>
</ul>
</ul>
</blockquote>
Added.<br>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
</ul>
<li><span class="new">p11convert.c</span></li>
<ul>
<li><span class="new">594-613, 1562-1574, 1691, et al.:
For the cases where you are freeing certain fields
within the ckParamPtr before returning, what happens
to the CK_TLS_PRF_PARAMS structure once it has been
returned to the caller with those fields freed? Is
there any chance that the struct is reused? If so,
it might be a good idea to NULL those freed pointers
out. If the struct is done away with after this
function exits then it's fine as-is. It looks like
these branches happen on cases where an exception is
ultimately thrown, but I figured I'd ask to be sure.<br>
</span></li>
</ul>
</ul>
</blockquote>
When there is a pending exception, we should free natively
allocated memory inside the same method and then return. The
structure won't be used.<br>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
<li><span class="new"> </span><br>
</li>
<li><span class="new">797: You don't need a return here,
do you?</span></li>
</ul>
</ul>
</blockquote>
Removed.<br>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
</ul>
<li><span class="new">p11crypt.c</span></li>
<ul>
<li><span class="new">146-7: Still need these lines?</span></li>
</ul>
</ul>
</blockquote>
<p>I removed all the commented out debugging printf calls.
Hopefully we won't need these again. ;)</p>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
</ul>
<li><span class="new">p11digest.c</span></li>
<ul>
<li><span class="new">102: Style nit, can we get a
newline in here and break up the parameter list as
you've done in other .c files?</span></li>
</ul>
</ul>
</blockquote>
Fixed.<br>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
</ul>
<li><span class="new">p11sign.c</span></li>
<ul>
<li><span class="new">95: I notice in certain places
long/jlong values are referenced in the format
string as %X and sometimes as %lX. Should we
standardize on the latter? Maybe no big deal if you
aren't seeing compiler warnings.<br>
</span></li>
</ul>
</ul>
</blockquote>
Done.<br>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
<li><span class="new">136: You might want to make that
%u (or maybe %lu) so the data length prints as an
unsigned value. It's unlikely to see an overflow
here, but who knows?<br>
</span></li>
</ul>
</ul>
</blockquote>
Done.<br>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
<li><span class="new">530: Just curious: why do you need
a return here? Isn't this a void function? I don't
see it in some of the other void functions here.<br>
</span></li>
</ul>
</ul>
</blockquote>
Removed.<br>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<li><span class="new">GCMParameters.java</span></li>
<ul>
<li><span class="new">49: Is the @since 1.8 correct
here? Not sure you need @since for a sun.* family
class, but it's also not JDK 8.</span></li>
</ul>
</ul>
</blockquote>
<p>This file is mostly lifted from the one inside SunJCE
provider. I have changed the @since as well as the copyright
years. Was debating whether to remove the one in SunJCE
provider, but ended up just copy it over since this RFE is
for PKCS#11 provider and want to keep the scope of changes
on PKCS11 only.</p>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
</ul>
<li><span class="new">P11PSSSignature.java</span></li>
<ul>
<li><span class="new">isDigestEqual(): It seems like you
could simplify this a bit by "flattening" both the
stdAlg and givenAlg, removing the first instance of
the "-" and then do a case-ignore comparison.
Something like flatStdAlg = stdAlg.replaceFirst("-",
"") and the same with flatGivenAlg. Then just
"return flatStdAlg.equalsIgnoreCase(flatGivenAlg);"
Maybe I'm missing an edge case here, but it seems
like it could work for all the digest strings you
reference in the static initializer above.</span></li>
</ul>
</ul>
</blockquote>
<p>Well, this isDigestEqual() method is mostly for comparing
the edge case of "SHA-1"/"SHA"/"SHA1". For all other digest
algorithms, what you suggested would work.</p>
<p>Will re-test everything and update webrev once the testing
passes.</p>
Thanks,<br>
Valerie<br>
<blockquote type="cite"
cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
<ul>
<ul>
</ul>
</ul>
<p>--Jamil<br>
</p>
<br>
<div class="moz-cite-prefix">On 4/12/2019 5:05 PM, Valerie
Peng wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1de69fb8-6679-ca1e-3a1e-ef6f37f52ea6@oracle.com">
<br>
Anyone has time to review this? Besides the header files
update, I added support for AES/GCM/NoPadding support. Ran
into some strange NSS error with RSASSA-PSS signature
mechanism, so I have not included the PSS signature impl
here. <br>
<br>
RFE: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8080462"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8080462</a>
<br>
<br>
Webrev: <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~valeriep/8080462/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~valeriep/8080462/webrev.00/</a>
<br>
<br>
CSR: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8221442"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8221442</a>
<br>
<br>
Thanks, <br>
Valerie <br>
<br>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
</blockquote>
<br>
</blockquote>
</body>
</html>