On Mon, 12 Apr 2021 20:53:21 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> 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. 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/3181
