[8u] RFR Backport 8177334: Update xmldsig implementation to Apache Santuario 2.1.1

[Elliott Baron]

Hi Andrew,

On 2020-06-15 12:52 p.m., Andrew Hughes wrote:
> 
> 
> On 25/05/2020 21:53, Elliott Baron wrote:
>> Hi,
>>
>> On 2020-05-08 2:58 p.m., Elliott Baron wrote:
>>> Hi,
>>>
>>> On 2020-04-16 7:54 p.m., Elliott Baron wrote:
>>>> Hi,
>>>>
>>>> I'd like to request a review to backport 8177334 to 8u.
>>>>
>>>> Original fix:
>>>> https://bugs.openjdk.java.net/browse/JDK-8177334
>>>> http://hg.openjdk.java.net/jdk/jdk/rev/3810c9a2efa1
>>>>
>>>> The JDK 11 fix did not apply cleanly. Below, I have detailed the
>>>> modifications I made in order to backport this fix to 8u. There are
>>>> some major changes that I believe may require some discussion, and
>>>> many minor changes outlined after those. The changes within each
>>>> section are listed roughly in order of the patch.
>>>>
>>>> Thank you to Martin Balao for his assistance in tracking down the
>>>> dependencies for this fix.
>>>>
>>>> I should point out that there are some known bugfixes that fix
>>>> problems introduced by this update. These should probably go into the
>>>> same 8u update as this fix. They are:
>>>> - 8217878: ENVELOPING XML signature no longer works
>>>> - 8218629: XML Digital Signature throws NAMESPACE_ERR exception on
>>>> OpenJDK 11, works 8/9/10 (I believe this is the same fix as above)
>>>> - 8236645: JDK 8u231 introduces a regression with incompatible
>>>> handling of XML messages
>>>>
>>>> 8u webrev:
>>>> https://cr.openjdk.java.net/~ebaron/jdk8u/JDK-8177334/webrev.00/
>>>
>>> Has anyone had a chance to take a look at this yet?
>>>
>>>>
>>>> [snip]
>>
>> Here is an updated webrev that fixes conflicts introduced by
>> 8231415: Better signatures in XML.
>>
>> 8u webrev:
>> https://cr.openjdk.java.net/~ebaron/jdk8u/JDK-8177334/webrev.01/
>>
>> Thanks,
>> Elliott
>>
> 
> Thanks for completing what is a huge backport.

Thanks for the review!

> 
> Can you comment more on what changes you believe require discussion?

Yes, of course. These were in my original patch email [1] along with all 
of the minor changes as well, but I truncated them when I sent out the 
ping. Most of them you've already discovered yourself:

> Major changes
> ---------------
> javax/xml/crypto/dsig/DigestMethod:
> - New string constants referencing new algorithms have been removed, in 
> order to not introduce new public API. I'm not sure if this would 
> technically be a breakage.
> 
> javax/xml/crypto/dsig/SignatureMethod:
> - New string constants have been removed, as in DigestMethod mentioned 
> above.
> 
> org/jcp/xml/dsig/internal/dom/DOMSignatureMethod:
> - There is no "8042967: Add variant of DSA Signature algorithms that do 
> not ASN.1 encode the signature bytes" in 8u. This was a messy backport 
> and it appears to add a new feature. To limit the amount of 
> modifications done to this class, I opted to backport the "getSignature" 
> method only from 8042967.
> - The class hierarchies of various nested DOMSignatureMethods have been 
> changed to exclude abstract classes that were introduced by 8042967.
> - I'm a bit on the fence about these modifications, perhaps it would be 
> better to backport 8042967 after all.
> 
> test/javax/xml/crypto/dsig/GenerationTests:
> - jtreg tag differs because of newer tests already in 8u, lack of 
> modules, and argument added by "8210736: 
> jdk/javax/xml/crypto/dsig/GenerationTests.java slow on linux".
> - Test cases using missing SHA-3 algorithm have been removed, since 
> there is no support for it in 8u. This would require a backport of 
> "8000415: Add support for SHA-3" and possibly others.
> - Constants in javax/xml/crypto/dsig/{Digest,Signature}Method that were 
> not backported are substituted with their String values.
> - List.of, which doesn't exist in JDK 8, has been replaced with a static 
> initializer.
> - For code added by "8206911: javax/xml/crypto/dsig/GenerationTests.java 
> fails in 8u-dev", rename call from "test_create_detached_signature" to 
> "test_create_detached_signature0". This reflects the method name change 
> introduced by this fix.
> - Some context also required changes due to 8206911.

Of these, I think the removal of new algorithm constants, and opting not 
to backport "8042967: Add variant of DSA Signature algorithms that do 
not ASN.1 encode the signature bytes" along with my workaround using 
"getSignature" were what I was most concerned about.

>  From comparing with the 11u version, most of it seems fine:
> 
> * Movement of checkRegisterPermission in JavaUtils.java is omitted as
> it's already been moved to its current location in 8u
> * Omission of new 11u algorithms in DigestMethod.java and
> SignatureMethod.java seems appropriate
> * Leaving out the type checking in e.g. DOMKeyInfo.java seems
> appropriate as these API methods did not previously throw ClassCastException
> 
> My main concern is that, although we don't add the new algorithms to
> DigestMethod & SignatureMethod, or their tests, we do seem to be adding
> support in e.g. JCEMapper for new algorithms like SHA3. I think we
> should be consistent here and if we're not going to put new algorithms
> in DigestMethod & SignatureMethod, and not include tests of them, they
> shouldn't be being implemented.

Makes sense to me. I've removed the code related to SHA-3 algorithms, 
and also Whirlpool, which also seems to not be unimplemented.

Revised 8u Webrev: 
https://cr.openjdk.java.net/~ebaron/jdk8u/JDK-8177334/webrev.02/

I noticed that RIPEMD-160 is already present, but not provided by any 
default crypto provider. I suppose this is to allow the XML-DSig code to 
work with third-party providers. We could follow suit and include the 
new algorithms after all, but as you said, there's no good way to test 
them as part of the JDK.

> 
> I also wonder why getSignature was pulled into DOMSignatureMethod.java
> from JDK-8042967. Why was this needed?
> 

This allowed a cleaner backport for the new RSASSA-PSS 
DOMSignatureMethods. They worked in the original by overriding the 
getSignature method to insert special handling for the PSSParameterSpec. 
Maybe I could add an instanceof special case to the original 8u code 
instead to do this?

Thanks,
Elliott

[1] https://mail.openjdk.java.net/pipermail/jdk8u-dev/2020-April/011571.html