8305972: Update XML Security for Java to 3.0.2

[GoeLin]

I backport this for parity with 17.0.11-oracle.

The backport was almost clean, except for two trivial resolves due to differences in whitespace in the context.

The change comes with a CSR, which is already approved for 17.
But the CSR requires changes wrt. to the original change.
In 17, no EDDSA support is added.

The PR comes with two commits:

1. the almost clean backport. I already skipped two comments added in head but not needed in 17 (SignatureMethod, DigestMethod).
2. removing the eddsa support.

Tests pass, SAP nightly testing passed.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• JDK-8305972 needs maintainer approval
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8305972: Update XML Security for Java to 3.0.2 (Enhancement - P3)

Reviewers

• Matthias Baesken (@MBaesken - Reviewer) ⚠️ Review applies to 5b271378
• Martin Balao (@martinuy - Reviewer) ⚠️ Review applies to 3f4616cd

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/2006/head:pull/2006
$ git checkout pull/2006

Update a local copy of the PR:
$ git checkout pull/2006
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/2006/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2006

View PR using the GUI difftool:
$ git pr show -t 2006

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/2006.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back goetz! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

This backport pull request has now been updated with issue from the original commit.

[mlbridge]

Webrevs

• 05: Full - Incremental (45845757)
• 04: Full - Incremental (3f4616cd)
• 03: Full - Incremental (272a826e)
• 02: Full - Incremental (5b271378)
• 01: Full - Incremental (a41cd161)
• 00: Full (12d02610)

[MBaesken]

Looking at the files of your change, I find a few 'EDDSA' related ones.
But you said you remove it (" no EDDSA support is added.") in 17 , so why the occurences, is it intentional ?

[MBaesken]

...ava.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/resource/config.xml

that looks strange, is that really intended ?
Unless required by applicable law or agreed to in writing,ons.SignatureBaseRSA$SignatureRSASHA3_512MGF1" />

[openjdk]

@GoeLin Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[openjdk]

@GoeLin Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[MBaesken]

The deleted line in src/java.xml.crypto/share/classes/org/jcp/xml/dsig/internal/dom/DOMSignatureMethod.java
might be restored but otherwise looks okay to me.

[MBaesken]

Don#t think we need to delete this line, maybe it is a leftover from the removed EDDSA ?

[GoeLin]

Fixed, thanks

[openjdk]

⚠️ @GoeLin This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[bridgekeeper]

@GoeLin This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[martinuy]

Hi Goetz,

Thanks for proposing this backport.

One minor comment:

• test/jdk/javax/xml/crypto/dsig/GenerationTests.java
  • The import of X509Certificate and the definition of x5ks are dead code.

Otherwise, looks good to me. I'll give my approval anyways, as this is a minor comment.

On a final note, I agree with your changes of removing EdDSA code and aligning to the approved CSR. However, I have to say that doing this for 17u does not only prevent users from the enhancement but also increases the maintenance cost as there will be more chances of updates not applying cleanly for the years to come. I would have treated 17u differently than previous releases.

Martin.-

[GoeLin]

Hi @martinuy,
do you think it would be better to keep all the code and only remove the two String and the change to the comment of DigestMethod?
In the 21u change for the update to 3.0.3 I have done it that way. Probably that's also the better solution here.

[martinuy]

Hi @GoeLin ,

I assume that in 21u you kept all the (implementation) code except for the public members. If so, I understand the motivations but personally prefer what you proposed for 17u in this PR. It makes the code more clear in terms of what is supported. For example, it would be misleading for someone who looks for "EdDSA" references in the code, finds many —even beyond defines— and assumes that it is supported. This is, of course, at the expense of higher chances of non-clean updates. We have taken this approach in other libraries before such as when we removed the implementation of DTLS in the 8u backport of the TLS engine.

Martin.-

[GoeLin]

Hi @martinuy,
looking at the CSR differences: CSR for head CSR for 11 & 17 I still believe it is more correct to keep the coding.
At least for 17, which, as I understand, supports EdDSA in general. It was just not specified for XML Security (i.e. not contained in SignatureMethod.java). Probably changing this would require more than just a CSR so Oracle omitted it.

I prepared a minimal PR: #2116 that only removes the strings and replace their usage by the plain strings. The tests pass, our nightly testing passed for this PR, too.

[GoeLin]

Hi @martinuy,
are you fine with me driving #2116 and similarly #2455 for 11?

[martinuy]

Hi @GoeLin ,

I personally prefer the approach taken in PR #2006 (here), for the reasons in my previous comment. With that said, I see your motivation. I would be interested in hearing @jerboaa view on this issue.

[jerboaa]

From the 17, 11 and 8u CSR:

The port is largely the same in terms of implementation as that done for JDK 21.

The javax.xml.crypto.dsig.SignatureMethod and javax.xml.crypto.dsig.DigestMethod interfaces will not be updated.
Instead, end users would define the newly added EdDSA Signature methods locally in application code. Unlike JDK 17
and later, JDK 11 and 8 doesn't have EdDSA support by default. A 3rd party security provider which supports ed25519
and ed448 would be required.

This suggests that EdDSA support was kept as this bug is about updating a bundled in-tree library from a third party (Apache Santuario). So changes to that third party code should be kept to a minimum, IMO. For JDK 11 and JDK 8 even a third party provider would be needed in order to be able to use EdDSA there.

TLDR; Since this is a third-party library code upgrade, I'd suggest to go with #2116

HTH.

[GoeLin]

Thanks for your help! Closing this PR.