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
