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

[Bradford Wetmore]

On 2/12/2020 3:10 PM, Andrew John Hughes wrote:

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

I personally agree with you.


> but I guess we're stuck with this then.
 >>> 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.

Agreed, thanks.

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

Sure.

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

I'm a hardcore webrev/frames guys, so I wouldn't have thought to closely 
look for this.

I believe I've discovered a "bug" in webrev when specifying specific 
revisions.  I use Mercurial Queues to handle my patches.  With these 
revisions:

     r1 = current tip
     r2 = ALPN patch applied
     r3 = PSS patch applied

If I have applied ALPN + PSS and I use:

     % webrev -r r2 -o webrev

to generate the PSS-only changes, I don't get the git headers.  If I 
don't specify -r, it defaults to r1 (current tip):

     % webrev -o webrev

Then I do see the git changes, but unfortunately both ALPN+PSS show up 
in a unified webrev.

>> ---begin---
>> 8230978: Add support for RSASSA-PSS Signature algorithm (Java SE 8)
>> ...deleted...
>> 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.

Changed.

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

Done.

Brad