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
signature.asc
Description: OpenPGP digital signature
