On Thu, 10 Aug 2023 18:12:59 GMT, Rajan Halade <rhal...@openjdk.org> wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> address review feedbacks
>
> test/jdk/sun/security/pkcs11/PSSUtil.java line 24:
>
>> 22: */
>> 23: import java.security.*;
>> 24: import java.security.interfaces.*;
>
> Please remove unused imports
Sure
> test/jdk/sun/security/pkcs11/PSSUtil.java line 34:
>
>> 32: private static final String KEYALG = "RSA";
>> 33: private static final String SIGALG = "RSASSA-PSS";
>> 34: private static final String[] DIGESTS = {
>
> is this constant used anywhere or supposed to be used?
Removed.
> test/jdk/sun/security/pkcs11/PSSUtil.java line 59:
>
>> 57: for (String h : hashAlgs) {
>> 58: String sigAlg = (h.startsWith("SHA3-")?
>> 59: h : h.replace("-","")) + "withRSASSA-PSS";
>
> Suggestion:
>
> String sigAlg = (h.startsWith("SHA3-") ?
> h : h.replace("-", "")) + "withRSASSA-PSS";
Ok.
> test/jdk/sun/security/pkcs11/Signature/KeyAndParamCheckForPSS.java line 129:
>
>> 127: sig.setParameter(paramsGood);
>> 128: sig.initVerify(pub);
>> 129: System.out.println("test#2: good params pass");
>
> Either add a `test#1` message after `initSign` test or update this to remove
> `#2`
test#1 message matches with test#2 ones. Anyway, I removed the test output
messages with your comments.
> test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java line 66:
>
>> 64: public void main(Provider p) throws Exception {
>> 65: if (!PSSUtil.isSignatureSupported(p)) {
>> 66: System.out.println("Skip testing RSASSA-PSS" +
>
> throw SkippedException here.
Ok. I initially thought that the SkippedException only applies to library not
found and/or un-configured OS. With this extension of the meaning of
SkippedException, many of the existing tests have to be updated to match.
> test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java line 83:
>
>> 81: PSSUtil.isHashSupported(p, hash, mgfHash);
>> 82: if (s == PSSUtil.AlgoSupport.NO) {
>> 83: System.out.println(" => Skip; no support");
>
> Similar request here to mark test as skipped if all algorithms are not
> supported.
This is just one digest algorithm. To mark the test skipped if none of the
algorithms are supported will require different handling.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15217#discussion_r1290608992
PR Review Comment: https://git.openjdk.org/jdk/pull/15217#discussion_r1290607168
PR Review Comment: https://git.openjdk.org/jdk/pull/15217#discussion_r1290614205
PR Review Comment: https://git.openjdk.org/jdk/pull/15217#discussion_r1290598689
PR Review Comment: https://git.openjdk.org/jdk/pull/15217#discussion_r1290601153
PR Review Comment: https://git.openjdk.org/jdk/pull/15217#discussion_r1290602675
