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

[Andrew John Hughes]

On 08/02/2020 00:10, Bradford Wetmore wrote:
> 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.
> 

Ah ok. I personally think the original MR 3 version would have been
better, but I guess we're stuck with this then.

>> 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.
> 

Ok. Backporting 8213009 to make future backports easier is the route I
would prefer anyway (and we've done similar in other cases). It's just
hard to tell the motivation with everything in one patch.

>> 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.
> 

Thanks for this detailed list. It's very helpful.

>> 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.

I wasn't looking at the web pages, but just at the patch file
(https://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/PSS/webrev.01/jdk.patch)
and comparing with the changesets from 11u.

In the patch file, CKey.java (for example) appears as:

--- /dev/null   2020-01-30 10:24:41.746000000 -0800
+++ new/src/windows/classes/sun/security/mscapi/CKey.java
2020-02-04 15:16:49.338689705 -0800
@@ -0,0 +1,155 @@

and same in
https://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/PSS/webrev.01/src/windows/classes/sun/security/mscapi/CKey.java.patch

so it wasn't clear what actual changes were applied after the file was
moved.

It seems like webrev should be running hg diff with the --git option to
get a more useful diff in these cases.

In a git format patch, the same diff would appear as:

rename from src/windows/classes/sun/security/mscapi/Key.java
rename to src/windows/classes/sun/security/mscapi/CKey.java

and then the diff would be as if the move hadn't happened, just showing
the differences between the original Key.java and the final CKey.java.

> 
>> 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---
> 

I'd still prefer it was something like:

Summary: Contains elements of JDK-8051408 (see comments on JDK-8230978)

the reason for this being that this changeset will then show up if
someone searches the repository for 8051408, but won't trigger the
database to create a backport issue for it.

> Did you want me to add you as a reviewer?
> 

Please.

>> Otherwise, looks good.
>>
>> Thanks,
> 
> Thanks,
> 
> Brad
> 
> [0] https://openjdk.java.net/guide/producingChangeset.html
> 

Thanks again,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20200212/7bb216fe/signature.asc>