RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params

[Weijun Wang-2]

This enhancement contains the following code changes:

1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` and remove the internal one.
2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` so it understands extra fields in `PSSParameterSpec` and is aware of the defaults in both directions.
3. Update `DOMSignedInfo` so that secure validation can restrict `DigestMethod` used inside `RSAPSSParameterSpec`
4. Tests

-------------

Commit messages:
 - 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params

Changes: https://git.openjdk.java.net/jdk/pull/3181/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3181&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8241306
  Stats: 1310 lines in 11 files changed: 1159 ins; 105 del; 46 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3181.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3181/head:pull/3181

PR: https://git.openjdk.java.net/jdk/pull/3181

[Weijun Wang-2]

On Wed, 24 Mar 2021 21:36:21 GMT, Weijun Wang <[hidden email]> wrote:

> This enhancement contains the following code changes:
>
> 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` and remove the internal one.
> 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` so it understands extra fields in `PSSParameterSpec` and is aware of the defaults in both directions.
> 3. Update `DOMSignedInfo` so that secure validation can restrict `DigestMethod` used inside `RSAPSSParameterSpec`
> 4. Tests

src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureBaseRSA.java line 588:

> 586:
> 587:         public enum DigestAlgorithm {
> 588: //            SHA1("SHA-1", DigestMethod.SHA1, 20),

Do we want to support "SHA-1"? It's considered weak and not the default but the RFC seems to have not disabled it at all.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Weijun Wang-2]

> This enhancement contains the following code changes:
>
> 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` and remove the internal one.
> 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` so it understands extra fields in `PSSParameterSpec` and is aware of the defaults in both directions.
> 3. Update `DOMSignedInfo` so that secure validation can restrict `DigestMethod` used inside `RSAPSSParameterSpec`
> 4. Tests

Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:

  update XMLUtils (not used by tests here)

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3181/files
  - new: https://git.openjdk.java.net/jdk/pull/3181/files/e459545a..211c491c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3181&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3181&range=00-01

  Stats: 9 lines in 1 file changed: 2 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3181.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3181/head:pull/3181

PR: https://git.openjdk.java.net/jdk/pull/3181

[Sean Mullan-2]

On Tue, 30 Mar 2021 02:07:06 GMT, Weijun Wang <[hidden email]> wrote:

>> This enhancement contains the following code changes:
>>
>> 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` and remove the internal one.
>> 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` so it understands extra fields in `PSSParameterSpec` and is aware of the defaults in both directions.
>> 3. Update `DOMSignedInfo` so that secure validation can restrict `DigestMethod` used inside `RSAPSSParameterSpec`
>> 4. Tests
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>
>   update XMLUtils (not used by tests here)

src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 36:

> 34:  * Parameters for the <a href="http://www.w3.org/2007/05/xmldsig-more#rsa-pss">
> 35:  * XML Signature RSASSA-PSS Algorithm</a>. The parameters are expressed as a
> 36:  * {@link PSSParameterSpec} object encapsulated.

I suggest removing "encapsulated", I found use of that word a little confusing. Maybe just "The parameters are represented as a {@link PSSParameterSpec} object." Similar comment about "encapsulated" in other methods.

src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 75:

> 73:  * {@code TrailerField}. This is equivalent to the parameter-less signature
> 74:  * method as defined by http://www.w3.org/2007/05/xmldsig-more#sha256-rsa-MGF1.
> 75:  *

Normally I don't like to hardcode defaults (in case they weaken later) but since this is specified by RFC 6931, I don't think we have a choice, and I think we need to use this default for interoperability.

src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 89:

> 87:      *
> 88:      * @param spec the input {@code PSSParameterSpec} to be encapsulated
> 89:      */

Should this throw NPE if spec is null?

src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 99:

> 97:      * @return the encapsulated {@code PSSParameterSpec} object
> 98:      */
> 99:     public PSSParameterSpec getPSSParameterSpec() {

If an XML Signature contained an RSAPSSParams with no DigestMethod, would this return a PSSParameterSpec with the defaults as specified in the @implSpec?

src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 103:

> 101:     }
> 102:
> 103:     @Override

Since you are overriding `Object.hashCode` and `equals`, I think you should document the specification for that.

src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 113:

> 111:
> 112:     @Override
> 113:     public boolean equals(Object obj) {

Add specification.

src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 139:

> 137:
> 138:     @Override
> 139:     public String toString() {

Add specification.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Weijun Wang-2]

On Tue, 30 Mar 2021 15:04:29 GMT, Sean Mullan <[hidden email]> wrote:

>> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   update XMLUtils (not used by tests here)
>
> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 36:
>
>> 34:  * Parameters for the <a href="http://www.w3.org/2007/05/xmldsig-more#rsa-pss">
>> 35:  * XML Signature RSASSA-PSS Algorithm</a>. The parameters are expressed as a
>> 36:  * {@link PSSParameterSpec} object encapsulated.
>
> I suggest removing "encapsulated", I found use of that word a little confusing. Maybe just "The parameters are represented as a {@link PSSParameterSpec} object." Similar comment about "encapsulated" in other methods.

OK.

> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 75:
>
>> 73:  * {@code TrailerField}. This is equivalent to the parameter-less signature
>> 74:  * method as defined by http://www.w3.org/2007/05/xmldsig-more#sha256-rsa-MGF1.
>> 75:  *
>
> Normally I don't like to hardcode defaults (in case they weaken later) but since this is specified by RFC 6931, I don't think we have a choice, and I think we need to use this default for interoperability.

Yes, we need to support `xMLSignatureFactory.newSignatureMethod(SignatureMethod.RSA_PSS, null)`.

> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 89:
>
>> 87:      *
>> 88:      * @param spec the input {@code PSSParameterSpec} to be encapsulated
>> 89:      */
>
> Should this throw NPE if spec is null?

I currently throw one. Do I need to mention it in the spec?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Weijun Wang-2]

On Tue, 30 Mar 2021 15:31:22 GMT, Sean Mullan <[hidden email]> wrote:

>> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   update XMLUtils (not used by tests here)
>
> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 99:
>
>> 97:      * @return the encapsulated {@code PSSParameterSpec} object
>> 98:      */
>> 99:     public PSSParameterSpec getPSSParameterSpec() {
>
> If an XML Signature contained an RSAPSSParams with no DigestMethod, would this return a PSSParameterSpec with the defaults as specified in the @implSpec?

Yes. The `DOMRSAPSSSignatureMethod::unmarshalParams` method will fill the blanks.

> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 103:
>
>> 101:     }
>> 102:
>> 103:     @Override
>
> Since you are overriding `Object.hashCode` and `equals`, I think you should document the specification for that.

OK.

> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 113:
>
>> 111:
>> 112:     @Override
>> 113:     public boolean equals(Object obj) {
>
> Add specification.

OK.

> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 139:
>
>> 137:
>> 138:     @Override
>> 139:     public String toString() {
>
> Add specification.

OK.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Weijun Wang-2]

On Tue, 30 Mar 2021 15:34:16 GMT, Sean Mullan <[hidden email]> wrote:

>> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   update XMLUtils (not used by tests here)
>
> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 103:
>
>> 101:     }
>> 102:
>> 103:     @Override
>
> Since you are overriding `Object.hashCode` and `equals`, I think you should document the specification for that.

For `equals` and `hashCode`, the removed internal `org/jcp/xml/dsig/internal/dom/RSAPSSParameterSpec.java` file had them so I kept them. But now I intend to remove both. `HMACParameterSpec` in the same package does not have them. In fact, I felt a little uncomfortable on using the result of `mgfParamsAsString` in `hashCode` and `equals` because even `PSSParameterSpec` and `MGF1ParameterSpec` have not implemented them.

As for `toString`, I'm not sure if there needs a spec (and I don't want to specify the output format). `PSSParameterSpec` and `MGF1ParameterSpec` implemented it with no spec. The documentation at https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/security/spec/MGF1ParameterSpec.html also only list it in the "Methods declared in class java.lang.Object" with all other `Object` methods no matter if it's overridden or not.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Sean Mullan-2]

On Tue, 30 Mar 2021 16:34:45 GMT, Weijun Wang <[hidden email]> wrote:

>> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 89:
>>
>>> 87:      *
>>> 88:      * @param spec the input {@code PSSParameterSpec} to be encapsulated
>>> 89:      */
>>
>> Should this throw NPE if spec is null?
>
> I currently throw one. Do I need to mention it in the spec?

Yes, unless it is specified more generally somewhere else in the API. (Ex: "all methods that take parameters with a `null` value throw `NullPointerException` unless otherwise specified").

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Sean Mullan-2]

On Tue, 30 Mar 2021 16:56:22 GMT, Weijun Wang <[hidden email]> wrote:

>> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 103:
>>
>>> 101:     }
>>> 102:
>>> 103:     @Override
>>
>> Since you are overriding `Object.hashCode` and `equals`, I think you should document the specification for that.
>
> For `equals` and `hashCode`, the removed internal `org/jcp/xml/dsig/internal/dom/RSAPSSParameterSpec.java` file had them so I kept them. But now I intend to remove both. `HMACParameterSpec` in the same package does not have them. In fact, I felt a little uncomfortable on using the result of `mgfParamsAsString` in `hashCode` and `equals` because even `PSSParameterSpec` and `MGF1ParameterSpec` have not implemented them.
>
> As for `toString`, I'm not sure if there needs a spec (and I don't want to specify the output format). `PSSParameterSpec` and `MGF1ParameterSpec` implemented it with no spec. The documentation at https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/security/spec/MGF1ParameterSpec.html also only list it in the "Methods declared in class java.lang.Object" with all other `Object` methods no matter if it's overridden or not.

I agree with removing `equals` and `hashCode`. For `toString` I think what you have implemented is compliant with Object.toString() so I'm ok with not adding a specific specification for that.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Sean Mullan-2]

On Tue, 30 Mar 2021 16:39:37 GMT, Weijun Wang <[hidden email]> wrote:

>> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 99:
>>
>>> 97:      * @return the encapsulated {@code PSSParameterSpec} object
>>> 98:      */
>>> 99:     public PSSParameterSpec getPSSParameterSpec() {
>>
>> If an XML Signature contained an RSAPSSParams with no DigestMethod, would this return a PSSParameterSpec with the defaults as specified in the @implSpec?
>
> There are other fields in `RSASSAParams`, so if there is no DigestMethod, it will be SHA-256 but the other fields (like SaltLength or TrailerField) will still be read if they exist.
>
> If there is no `RSASSAParams` at all or if it's empty, then the defaults will be returned.

I wonder if the @implSpec is clear enough that this will be returned. I might suggest adding a similar @implSpec in this method that basically states what you said above.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Weijun Wang-2]

On Tue, 30 Mar 2021 18:41:45 GMT, Sean Mullan <[hidden email]> wrote:

>> There are other fields in `RSASSAParams`, so if there is no DigestMethod, it will be SHA-256 but the other fields (like SaltLength or TrailerField) will still be read if they exist.
>>
>> If there is no `RSASSAParams` at all or if it's empty, then the defaults will be returned.
>
> I wonder if the @implSpec is clear enough that this will be returned. I might suggest adding a similar @implSpec in this method that basically states what you said above.

I'm not sure if it's appropriate to specify the default value in this method. As long as there is an `RSAPSSParameterSpec` object, there must be a non-null `PSSParameterSpec` inside and it is the one that was used to construct this object.

I am thinking if we can append the following to the `@implNote` in the class spec:
 * One can obtain this default value using the following expression:
 * <pre><code>
 * XMLSignatureFactory.getInstance()
 *      .newSignatureMethod(SignatureMethod.RSA_PSS, null)
 *      .getParameterSpec()
 * </code></pre>

but this makes it more like an `@implNote` instead of an `@implSpec`.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Weijun Wang-2]

> This enhancement contains the following code changes:
>
> 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` and remove the internal one.
> 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` so it understands extra fields in `PSSParameterSpec` and is aware of the defaults in both directions.
> 3. Update `DOMSignedInfo` so that secure validation can restrict `DigestMethod` used inside `RSAPSSParameterSpec`
> 4. Tests

Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:

  spec word change, no hashCode and equals, test change

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3181/files
  - new: https://git.openjdk.java.net/jdk/pull/3181/files/211c491c..5b88fff4

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3181&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3181&range=01-02

  Stats: 69 lines in 5 files changed: 18 ins; 38 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3181.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3181/head:pull/3181

PR: https://git.openjdk.java.net/jdk/pull/3181

[Weijun Wang-2]

On Tue, 30 Mar 2021 20:24:49 GMT, Weijun Wang <[hidden email]> wrote:

>> I wonder if the @implSpec is clear enough that this will be returned. I might suggest adding a similar @implSpec in this method that basically states what you said above.
>
> I'm not sure if it's appropriate to specify the default value in this method. As long as there is an `RSAPSSParameterSpec` object, there must be a non-null `PSSParameterSpec` inside and it is the one that was used to construct this object.
>
> I am thinking if we can append the following to the `@implSpec` in the class spec:
>  * One can obtain this default value using the following expression:
>  * <pre><code>
>  * XMLSignatureFactory.getInstance()
>  *      .newSignatureMethod(SignatureMethod.RSA_PSS, null)
>  *      .getParameterSpec()
>  * </code></pre>
>
> but this makes it more like an `@implNote` instead of an `@implSpec`.

I added the new lines as `@implNote` and kept the old `@implSpec` there (since it's still a requirement for implementations). New commit pushed. CSR updated as well.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Weijun Wang-2]

On Wed, 24 Mar 2021 21:36:21 GMT, Weijun Wang <[hidden email]> wrote:

> This enhancement contains the following code changes:
>
> 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` and remove the internal one.
> 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` so it understands extra fields in `PSSParameterSpec` and is aware of the defaults in both directions.
> 3. Update `DOMSignedInfo` so that secure validation can restrict `DigestMethod` used inside `RSAPSSParameterSpec`
> 4. Tests

This pull request has been closed without being integrated.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Sean Mullan-2]

On Thu, 1 Apr 2021 13:32:47 GMT, Weijun Wang <[hidden email]> wrote:

>> This enhancement contains the following code changes:
>>
>> 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` and remove the internal one.
>> 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` so it understands extra fields in `PSSParameterSpec` and is aware of the defaults in both directions.
>> 3. Update `DOMSignedInfo` so that secure validation can restrict `DigestMethod` used inside `RSAPSSParameterSpec`
>> 4. Tests
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>
>   spec word change, no hashCode and equals, test change

src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 74:

> 72:  * {@code MaskGenerationFunction}, 32 as {@code SaltLength}, and 1 as
> 73:  * {@code TrailerField}. This is equivalent to the parameter-less signature
> 74:  * method as defined by http://www.w3.org/2007/05/xmldsig-more#sha256-rsa-MGF1.

http://www.w3.org/2007/05/xmldsig-more#sha256-rsa-MGF1 is just a placeholder page for the namespace. I would instead link to `SignatureMethod.SHA256_RSA_MGF1` and also reference the RFC for more information. How about:

`This is equivalent to the {@link SignatureMethod#SHA256_RSA_MGF1 parameter-less signature method} as defined in <a href="https://www.ietf.org/rfc/rfc6931.txt#section-2.3.10">RFC 6931</a>.
`

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Sean Mullan-2]

On Wed, 24 Mar 2021 21:39:28 GMT, Weijun Wang <[hidden email]> wrote:

>> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   spec word change, no hashCode and equals, test change
>
> src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureBaseRSA.java line 588:
>
>> 586:
>> 587:         public enum DigestAlgorithm {
>> 588: //            SHA1("SHA-1", DigestMethod.SHA1, 20),
>
> Do we want to support "SHA-1"? It's considered weak and not the default but the RFC has not disabled it. Since we already have secure validation on by default, it does seem to be a security issue.
>
> The "RSASSA-PSS without Parameters" section at https://tools.ietf.org/html/rfc6931#section-2.3.10 also lists SHA-224 and SHA3-**. We should probably support them as well, or at least make sure we support the same algorithms in "without Parameters" and "with Parameters".

I'm ok with not supporting SHA-1, although adding it would not be a security issue. It is blocked by default now, but it can be re-enabled, and SHA-1 in general is still available in the JDK.

I'm fine with adding support for SHA-224 and SHA-3 as part of this issue. You can add support for all the algorithms that we have the underlying crypto support for.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Weijun Wang-2]

On Fri, 9 Apr 2021 16:44:07 GMT, Sean Mullan <[hidden email]> wrote:

>> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   spec word change, no hashCode and equals, test change
>
> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 74:
>
>> 72:  * {@code MaskGenerationFunction}, 32 as {@code SaltLength}, and 1 as
>> 73:  * {@code TrailerField}. This is equivalent to the parameter-less signature
>> 74:  * method as defined by http://www.w3.org/2007/05/xmldsig-more#sha256-rsa-MGF1.
>
> http://www.w3.org/2007/05/xmldsig-more#sha256-rsa-MGF1 is just a placeholder page for the namespace. I would instead link to `SignatureMethod.SHA256_RSA_MGF1` and also reference the RFC for more information. How about:
>
> `This is equivalent to the {@link SignatureMethod#SHA256_RSA_MGF1 parameter-less signature method} as defined in <a href="https://www.ietf.org/rfc/rfc6931.txt#section-2.3.10">RFC 6931</a>.
> `

Correct.

How about:
This is equivalent to the parameter-less signature
method {@link SignatureMethod#SHA256_RSA_MGF1 SHA256_RSA_MGF1} as defined
in <a href="https://tools.ietf.org/html/rfc6931#section-2.3.10">RFC 6931</a>.
 
SHA256_RSA_MGF1 is not the only parameter-less method so I prefer showing its name.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Sean Mullan-2]

On Fri, 9 Apr 2021 17:28:45 GMT, Weijun Wang <[hidden email]> wrote:

>> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/spec/RSAPSSParameterSpec.java line 74:
>>
>>> 72:  * {@code MaskGenerationFunction}, 32 as {@code SaltLength}, and 1 as
>>> 73:  * {@code TrailerField}. This is equivalent to the parameter-less signature
>>> 74:  * method as defined by http://www.w3.org/2007/05/xmldsig-more#sha256-rsa-MGF1.
>>
>> http://www.w3.org/2007/05/xmldsig-more#sha256-rsa-MGF1 is just a placeholder page for the namespace. I would instead link to `SignatureMethod.SHA256_RSA_MGF1` and also reference the RFC for more information. How about:
>>
>> `This is equivalent to the {@link SignatureMethod#SHA256_RSA_MGF1 parameter-less signature method} as defined in <a href="https://www.ietf.org/rfc/rfc6931.txt#section-2.3.10">RFC 6931</a>.
>> `
>
> Correct.
>
> How about:
> This is equivalent to the parameter-less signature
> method {@link SignatureMethod#SHA256_RSA_MGF1 SHA256_RSA_MGF1} as defined
> in <a href="https://tools.ietf.org/html/rfc6931#section-2.3.10">RFC 6931</a>.
>  
> SHA256_RSA_MGF1 is not the only parameter-less method so I prefer showing its name.

Works for me.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Weijun Wang-2]

On Fri, 9 Apr 2021 17:23:05 GMT, Sean Mullan <[hidden email]> wrote:

>> src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureBaseRSA.java line 588:
>>
>>> 586:
>>> 587:         public enum DigestAlgorithm {
>>> 588: //            SHA1("SHA-1", DigestMethod.SHA1, 20),
>>
>> Do we want to support "SHA-1"? It's considered weak and not the default but the RFC has not disabled it. Since we already have secure validation on by default, it does seem to be a security issue.
>>
>> The "RSASSA-PSS without Parameters" section at https://tools.ietf.org/html/rfc6931#section-2.3.10 also lists SHA-224 and SHA3-**. We should probably support them as well, or at least make sure we support the same algorithms in "without Parameters" and "with Parameters".
>
> I'm ok with not supporting SHA-1, although adding it would not be a security issue. It is blocked by default now, but it can be re-enabled, and SHA-1 in general is still available in the JDK.
>
> I'm fine with adding support for SHA-224 and SHA-3 as part of this issue. You can add support for all the algorithms that we have the underlying crypto support for.

Not sure if I got it, are you OK with adding SHA-1? It must be listed here to be supported.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181

[Sean Mullan-2]

On Fri, 9 Apr 2021 19:54:22 GMT, Weijun Wang <[hidden email]> wrote:

>> I'm ok with not supporting SHA-1, although adding it would not be a security issue. It is blocked by default now, but it can be re-enabled, and SHA-1 in general is still available in the JDK.
>>
>> I'm fine with adding support for SHA-224 and SHA-3 as part of this issue. You can add support for all the algorithms that we have the underlying crypto support for.
>
> Not sure if I got it, are you OK with adding SHA-1? It must be listed here to be supported.

Let's not add it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3181