Hi Valerie, on the whole it looks really good. I do have some comments
below:
* SunPKCS11.java
o 728-738: I think you could add 2.16.840.1.101.3.4.3.3 and .4 for
dsa-with-sha384 and dsa-with-sha512, respectively.
o 790-792: Are you sure that's the right OID? OID lookup shows it
as PKCS10 and has 1.2.840.113549.1.1.10 as rsassa-pss.
* CK_MECHANISM.java
o 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)?
* CK_RSA_PKCS_PSS_PARAMS.java
o 55-61: Seems like you could replace all of removeDash() with
just String's replaceFirst("-", "") method.
o 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"?
* Functions.java
o 412: Typo: Vender -> Vendor
* PKCS11.java
o 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?
* PKCS11Constants.java
o 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.
* p11convert.c
o 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.
o 797: You don't need a return here, do you?
* p11crypt.c
o 146-7: Still need these lines?
* p11digest.c
o 102: Style nit, can we get a newline in here and break up the
parameter list as you've done in other .c files?
* p11sign.c
o 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.
o 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?
o 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.
* GCMParameters.java
o 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.
* P11PSSSignature.java
o 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.
--Jamil
On 4/12/2019 5:05 PM, Valerie Peng wrote:
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.
RFE: https://bugs.openjdk.java.net/browse/JDK-8080462
Webrev: http://cr.openjdk.java.net/~valeriep/8080462/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8221442
Thanks,
Valerie
