RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v2]

[Weijun Wang]

On Tue, 13 Apr 2021 14:22:47 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> You are right that the overridable methods are elsewhere (`XMLSignatureFactory::newSignatureMethod` and `SignatureMethod::getParameterSpec`), but I still feel it a little strange to move the default parameter of one particular algorithm to the general interface `SignatureMethod` (this is similar to talking about EC curve names in `KeyPairGenerator`). It seems more natural to talk about this inside a class which is specific to the RSASSA-PSS algorithm and `RSAPSSParameterSpec` is the only public API we can find. We can add something like "specify null if you want a default spec" to `XMLSignatureFactory::newSignatureMethod` but it does not have enough space to describe "how default values are determined" for each algorithm.
>> 
>> I understand `@implSpec` is for implementations of a class or a method, but does not mean the words must be put inside that exact class and method? Maybe not necessarily.
>> 
>> As for making `@implSpec` more specific, at signing time, we can only either provide a `RSAPSSParameterSpec` or not one, so there is only one default value which is SHA256_RSA_MGF1. On the other hand, at validation time we might be parsing a partial-filled `RSAPSSParams` node and that's where default salt and default trailer field show up.
>> 
>> Also about the return value of `SignatureMethod::getParameterSpec`, are you suggesting that for RSASSA-PSS it must be non null? This can be specified somewhere but we need to find a place. For the existing `HMACParameterSpec`, the method returns null if none is set. Do we treat that as no spec at all (but for RSASSA-PSS there is always one)?
>> 
>> My current opinion is that we still document all these in `RSAPSSParameterSpec` but be very clear that this part is for `XMLSignatureFactory::newSignatureMethod` and that part is for `SignatureMethod::getParameterSpec`, etc, etc.
>
> I think it is worth asking the TCK team about this. After further thought, I am not sure `implSpec` is correct because the implementation is not in the classes, it is in the provider. I now think it needs to be part of the API contract, so that all implementations are compliant. `SignatureMethod` already defines the RSA_PSS algorithm constant, so it doesn't seem so out-of-place to put the specification there, something like:
> 
> 
> /**
>   * The <a href="http://www.w3.org/2007/05/xmldsig-more#rsa-pss">
>   * RSASSA-PSS</a> signature method algorithm URI.
>   *
>   * If the parameter is not specified when calling `XMLSignatureFactory.newSignatureMethod` with 
>   * RSA_PSS as the signature algorithm, the default parameter is used, which uses SHA-256 as the
>   * {@code DigestMethod}, MGF1 with SHA-256 as the
>   * {@code MaskGenerationFunction}, 32 as {@code SaltLength}, and 1 as
>   * {@code TrailerField}. 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. This default
>   * parameter is returned by the `getParameterSpec` method.
>   *
>   * @since 17
>   */
>  String RSA_PSS = "http://www.w3.org/2007/05/xmldsig-more#rsa-pss";
> 
> Forget my comment about making `implSpec` more specific, I think that is now implied by RFC 3161. I also think the above specification would take care of my returning null comment, as all implementations would need to comply. I just didn't want applications to have to have code that checks for null and then don't know whether that means it is the default parameters or something else since it was implementation specific.

Great, I forgot about this string. This *is* a proper place to put the text.

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

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