On Mon, 12 Apr 2021 17:29:55 GMT, Sean Mullan <mul...@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
