On Tue, 13 Apr 2021 14:22:47 GMT, Sean Mullan <mul...@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

Reply via email to