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

[Weijun Wang]

On Mon, 12 Apr 2021 17:29:55 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> 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.
>
> Ok, I understand now. I think `@implSpec` (and probably the `@implNote`) are in the wrong class. `@implSpec`  means the implementation of this class. But this class is final and does not contain that logic. The logic of specifying/returning the defaults is in the JDK (XMLDSig provider) implementation of `SignatureMethod`. So I think it belongs there. In this class, you could add a sentence/link to `SignatureMethod`, something like "See SignatureMethod for how default values are determined when the parameters are not specified." 
> 
> Also, I think the `@implSpec` needs to be more specific, and also cover the cases where some, but not all of the parameters are specified and defaults are then used. For this, you will need to be more general, as the default salt length is based on what hash algorithm is being used. 
> 
> As for `@implNote`, this probably could use more discussion, but it might be better to make this part of the specification. If some implementations can return null, and others return defaults, it complicates the application's logic. The RFC has clearly specified what the defaults should be, so maybe the easiest thing to do is to make all implementations comply by also making it part of the API contract, and hiding the XML details as to whether the parameter was actually specified or not, which should not matter to applications.

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.

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

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