On Mon, 8 May 2023 17:22:53 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Update XML Security for Java to 3.0.2. Some change to tests:
>>
>> 1. No more Xalan. One test case is singled out to demonstrate how to use a
>> special configuration.
>> 2. EdDSA does not support `KeyValue`. Use X.509 certificate instead.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> cleanup commented out code and unnecessary bug id
src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/SignatureMethod.java
line 63:
> 61: public interface SignatureMethod extends XMLStructure, AlgorithmMethod {
> 62:
> 63: // All methods can be found in RFC 9231.
Only the ones with "xmldsig-more" in the URI, actually.
src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/SignatureMethod.java
line 260:
> 258: /**
> 259: * The <a
> href="http://www.w3.org/2021/04/xmldsig-more#eddsa-ed25519";>
> 260: * ED25519</a> signature method algorithm URI.
I think it would be beneficial to add a reference to the standards. Most of
these URIs are defined in RFC 9131, but a few are from the W3C Recommendation.
I suggest adding the following sentence to the class description:
"The signature method algorithm URIs defined in this class are specified in the
[W3C Recommendation for XML-Signature Syntax and
Processing](http://www.w3.org/TR/xmldsig-core/) and [RFC
9131](https://www.rfc-editor.org/info/rfc9131)."
test/jdk/javax/xml/crypto/dsig/GenerationTests.java line 105:
> 103: rsaSha1mgf1, rsaSha224mgf1, rsaSha256mgf1, rsaSha384mgf1,
> rsaSha512mgf1,
> 104: rsaShaPSS,
> 105: ed25519, ed448;
Nit: combine these lines.
test/jdk/javax/xml/crypto/dsig/XalanTest.java line 1:
> 1: /*
Can you add a comment to this test with more details, something like: "This
test used to be part of ValidationTests but was moved into its own test because
it tests a signature that contains the `here()` function which depends on Xalan
internals. The Xalan dependency has been removed from the DSig implementation
but can be optionally configured ..."
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13840#discussion_r1187870857
PR Review Comment: https://git.openjdk.org/jdk/pull/13840#discussion_r1187866459
PR Review Comment: https://git.openjdk.org/jdk/pull/13840#discussion_r1187845348
PR Review Comment: https://git.openjdk.org/jdk/pull/13840#discussion_r1187862788
