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
