RFR[8u252] - MR3 - ALPN & RSASSA-PSS in Java SE 8

[Bradford Wetmore]

On 2/5/2020 9:40 PM, Andrew John Hughes wrote:> First of all, thanks 
again for posting these patches and also for the
 > comprehensive list of issues for both of them. They pretty much matched
 > up with what I saw when reviewing the patches

Great.

 > The ALPN one looks pretty clean. My only concern there is the use of
 > "@since 8". Should this not be a reference to the maintenance release,
 > as these methods were not part of 8 at GA? Same applies to the usage in
 > the second patch too.

I originally had this as "@since 8 MR 3" but during internal review it 
was requested to use @since 8 instead.  This is what was presented/voted 
during the MR phase.

 > With the RSA-PSS patch, it's hard to tell what's going on with the
 > MSCAPI code, because they appear as new files in the patch. Why was it
 > necessary to bring in JDK-8213009? I don't see an obvious reason for
 > this.

JDK-8213009 was mainly a refactor of the MSCAPI code to enhance the code 
layout, and to a lesser degree to better support Windows-native PSS 
calls using CNG (MS Windows Crypto Next Generation).  The SunMSCAPI 
provider is a mix of mostly CAPI (Windows Crypto API) and a few CNG 
calls when CAPI couldn't support everything needed.

The SunMSCAPI changes that followed would take significant effort to 
extricate these reorganization changes when backporting later changes 
(e.g. jdk/jdk).  We could have forward ported/merged the 8u41 code into 
OpenJDK 8, but unfortunately I haven't been given a lot of cycles for 
OpenJDK code.

 > dependency of 8213010, but again, it's not obvious why that's required
 > here either. Some more details on why some of the less obvious bugs are
 > required would be helpful here.

Ok, I've added details to a comment in JDK-8230978.

 > If the refactoring is necessary, a
 > Git-style patch which shows the moves as renames (hg diff -g) would
 > help, so we can see what is actually being changed in these files.

I'm a bit confused, isn't the webrev already showing this:

http://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/PSS/webrev.01/
---begin---

...deleted...

*    src/windows/classes/sun/security/mscapi/CKey.java* /(was 
src/windows/classes/sun/security/mscapi/Key.java)/

///138 lines changed: 54 ins; 64 del; 20 mod; 81 unchg/

...deleted...

*    src/windows/classes/sun/security/mscapi/CSignature.java /(was 
src/windows/classes/sun/security/mscapi/RSASignature.java)/

///676 lines changed: 507 ins; 77 del; 92 mod; 355 unchg/
---end---

Looking at the index.html and subsequent "Frame" for this file, I can 
see both the rename and the old/new code side-by-side.

 > As you say, the RSA-PSS patch brings in truncated MessageDigests which
 > are part of "8051408: NIST SP 800-90A SecureRandom implementations". It
 > would be good if the summary field could be used to say "Contains
 > truncated MessageDigests from JDK-8051408", so that it then shows up if
 > someone is considering 8051408 at a later date

There are several comments to make which won't fit in the summary field 
which is supposed to only be one short line [0].

Instead, I have added several items into the comment for JDK-8230978, 
which is the main umbrella bug for the PSS work, and I will add:

     Summary: See comment in JDK-8230978 for details in Oracle JDK 8u 
and OpenJDK 8u

to the changeset, which currently reads:

---begin---
8230978: Add support for RSASSA-PSS Signature algorithm (Java SE 8)
8175029: StackOverflowError in X509CRL and 
X509Certificate.verify(PublicKey, Provider)
8146293: Add support for RSASSA-PSS Signature algorithm
8205445: Add RSASSA-PSS Signature support to SunMSCAPI
8205720: KeyFactory#getKeySpec and translateKey throws 
NullPointerException with Invalid key
8206171: Signature#getParameters for RSASSA-PSS throws ProviderException 
when not initialized
8213009: Refactoring existing SunMSCAPI classes
8213010: Supporting keys created with certmgr.exe
8214096: sun.security.util.SignatureUtil passes null parameter, so JCE 
validation fails
8215694: keytool cannot generate RSASSA-PSS certificates
8221407: Windows 32bit build error in libsunmscapi/security.cpp
8216039: TLS with BC and RSASSA-PSS breaks ECDHServerKeyExchange
8223003: SunMSCAPI keys are not cleaned up
8223063: Support CNG RSA keys
8225745: NoSuchAlgorithmException exception for SHA256withECDSA with 
RSASSA-PSS support
8225180: SignedObject with invalid Key not throwing the 
InvalidKeyException in Windows
8236470: Deal with ECDSA using ecdsa-with-SHA2 plus hash algorithm as 
AlgorithmId
8238502: sunmscapi.dll causing EXCEPTION_ACCESS_VIOLATION
Summary: See comment in JDK-8230978 for details in Oracle JDK 8u and 
OpenJDK 8u
Reviewed-by: valeriep, weijun, coffeys, pkoppula
---end---

Did you want me to add you as a reviewer?

 > Otherwise, looks good.
 >
 > Thanks,

Thanks,

Brad

[0] https://openjdk.java.net/guide/producingChangeset.html