RFR[11] JDK-8206171: Signature#getParameters for RSASSA-PSS throws ProviderException when not initialized

Fri Jul 20 01:23:08 UTC 2018

Thanks Max and Sean for the review and suggestions. I have updated the 
webrev accordingly and finalized CSR.

Webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.04/
CSR: https://bugs.openjdk.java.net/browse/JDK-8206864

Valerie

On 7/19/2018 3:13 PM, Wang Weijun wrote:
>
>> 在 2018年7月20日，05:18，Valerie Peng <valerie.peng at oracle.com> 写道：
>>
>> Hi Sean,
>>
>> Thanks for the suggestion, I like it.
> Me too.
>
>> Max, any objection or concern before I update the webrev and CSR?
> No.
>
> Thanks
> Max
>
>> Valerie
>>
>>
>>> On 7/19/2018 7:28 AM, Sean Mullan wrote:
>>> Hi Valerie, Max -
>>>
>>> I took a fresh look at this issue this morning. I think we are getting bogged down by trying to adjust within the current wording, which is somewhat awkward and hard to understand. So, I think it might be better to break up the wording into multiple sentences. Here's a cut at the rewording:
>>>
>>> /**
>>>   * Returns the parameters used with this signature object.
>>>   *
>>>   * <p> If this signature has been previously initialized with parameters
>>>   * (by calling the {@code setParameter} method), this method returns
>>>   * the same parameters. If this signature has not been initialized with
>>>   * parameters, this method may return a combination of default and
>>>   * randomly generated parameter values if the underlying
>>>   * signature implementation supports it and can successfully generate
>>>   * them. Otherwise, {@code null} is returned.
>>>   *
>>>   * @return the parameters used with this signature, or {@code null}
>>>   */
>>>
>>> In the first sentence of the 2nd paragraph above, I wanted to first list the case where the signature is initialized with parameters, which is the most clear-cut case of what the behavior will be. Then I described the case where an implementation may return default/generated parameters -- and this is clearly marked "may" since it is optional. All other cases other than those two always return null, so I think this makes it easier to understand.
>>>
>>> --Sean
>>>
>>>> On 7/18/18 1:29 PM, Valerie Peng wrote:
>>>> Sean,
>>>>
>>>> Where do you think that we should add the part about "null must be returned ..." paragraph? At the end of first or second paragraph?
>>>>
>>>> I will go with majority.
>>>>
>>>> Valerie
>>>>
>>>>> On 7/17/2018 8:38 PM, Weijun Wang wrote:
>>>>> Is it better to append the new lines to the 2nd paragraph?
>>>>>
>>>>> Thanks
>>>>> Max
>>>>>
>>>>>> On Jul 18, 2018, at 9:46 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>>>>>
>>>>>>
>>>>>> Ok, let's use "must" then. I have also added the part about default parameters.
>>>>>> Hope that all is clear now.
>>>>>>
>>>>>> Latest webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.03/
>>>>>> Latest CSR: https://bugs.openjdk.java.net/browse/JDK-8206864
>>>>>>
>>>>>> Thanks,
>>>>>> Valerie
>>>>>>
>>>>>> On 7/17/2018 5:50 PM, Weijun Wang wrote:
>>>>>>>> On Jul 18, 2018, at 8:23 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>>>>>>>
>>>>>>>> Hi Max,
>>>>>>>>
>>>>>>>> Thanks for the suggestions. Please find comments inline.
>>>>>>>>
>>>>>>>>> On 7/16/2018 7:38 PM, Weijun Wang wrote:
>>>>>>>>> CSR at https://bugs.openjdk.java.net/browse/JDK-8206864.
>>>>>>>>>
>>>>>>>>> - At the end of the 1st paragraph, you have now
>>>>>>>>>
>>>>>>>>>>     However, for signature algorithm such as "RSASSA-PSS", it requires parameters and the one used for signing must be supplied for verification to succeed. It may be better to return null instead of returning provider-specific default parameters.
>>>>>>>>> I suggest we don't talk about the reason (params must be the same for signing and verification), we can just say something like "If there is no provider-specific default parameters, this method should return null before user sets one".
>>>>>>>> Alright, I initially didn't put down the reason. But since Brad asked about it, I add it to the CSR in case Joe raise the same question. I will update the CSR per your suggestion.
>>>>>>>>> - null may be returned
>>>>>>>>>
>>>>>>>>> How about "{@code null} must be returned"?
>>>>>>>> How about "should"? Is there a guideline on when to use "may/should/must"? Anyone knows?
>>>>>>> Even if there were guidelines on this for Java, I think we should just stick to https://tools.ietf.org/html/rfc2119, except that the capitalization is not necessary.
>>>>>>>
>>>>>>> "Must" is precise here.
>>>>>>>
>>>>>>>> I thought must is mostly used in mandating input arguments must not be null. But don't recall it being used much for return values.
>>>>>>> "must return" appears 39 times in java/ and "should return" 19 (simple grep, no line break).
>>>>>>>
>>>>>>> --Max
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Valerie
>>>>>>>>
>>>>>>>>> Everything else looks fine.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Max
>>>>>>>>>
>>>>>>>>>> On Jul 17, 2018, at 9:46 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Max,
>>>>>>>>>>
>>>>>>>>>> Good catch on the SignatureSpi class and SunMSCAPI provider, I will include them.
>>>>>>>>>>
>>>>>>>>>> As for the part about "randomly generated", I am leaning toward not having it as I don't see  a value of specifying this.
>>>>>>>>>>
>>>>>>>>>> Webrev and CSR has been updated.
>>>>>>>>>>
>>>>>>>>>> New webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.02/
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Valerie
>>>>>>>>>>
>>>>>>>>>>> On 7/16/2018 4:29 PM, Weijun Wang wrote:
>>>>>>>>>>> Valerie
>>>>>>>>>>>
>>>>>>>>>>> Would you like to retain the word "randomly generated" somewhere?
>>>>>>>>>>>
>>>>>>>>>>> Also, the SignatureSpi class needs to be updated in the same way.
>>>>>>>>>>>
>>>>>>>>>>> Can you also update the implementation in the MSCAPI Signature object?
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> Max
>>>>>>>>>>>
>>>>>>>>>>>> On Jul 17, 2018, at 6:16 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> No issues found and it seems ok to return null if no parameters is set. Thus, I have updated the webrev and CSR accordingly.
>>>>>>>>>>>>
>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8206171
>>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.01/
>>>>>>>>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8206864
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Valerie
>>>>>>>>>>>>
>>>>>>>>>>>>> On 7/13/2018 11:05 AM, Valerie Peng wrote:
>>>>>>>>>>>>> Hmm, I like the idea of expanding null to cover both cases.
>>>>>>>>>>>>> I will explore it more and see.
>>>>>>>>>>>>> Thanks for the feedback,
>>>>>>>>>>>>> Valerie
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 7/13/2018 6:56 AM, Sean Mullan wrote:
>>>>>>>>>>>>>> On 7/12/18 10:23 PM, Weijun Wang wrote:
>>>>>>>>>>>>>>>> On Jul 13, 2018, at 10:01 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hi Max,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The javadoc is for Signature.getParameters(), so null can be returned for signature algorithms which do not use parameters, e.g. SHA256withDSA. As Signature class covers all signature algorithms, I am not sure about mentioning specific algorithm names as it may be lengthy and even misleading unless we list out all applicable algorithms.
>>>>>>>>>>>>>>> Sure.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The part of "default and randomly generated" is inherited from existing javadoc. I think "default" in the aforementioned sentence means "hardcoded values". For example, something like salt length will likely have a fixed default value. Since we have no control over 3rd party providers, I think we may have to keep this for backward compatibility reason. For RSASSA-PSS sig algorithm, it errors out if the required parameter is not given. Thus, I added the sentence "If there are no provider-specific default values, the underlying signature implementation may also fail".
>>>>>>>>>>>>>>> OK, now I understand "a combination of default and randomly generated" means some part of the parameter is hardcoded and some is random. Anyway, let's keep it unchanged.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Then, how about simply "If there are no provider-specific values" which covers both hardcoded and random values?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> "the underlying signature implementation may also fail" is still not clear to me. If I had not read the CSR I would thought an exception will be thrown when update/sign is called.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> As for @throws, I debated about it. The main reason for not adding one is consistency. Many (or should I say most) security classes do not have @throws for ProviderException. If we were to add @throws ProviderException here, we should do it across the board. Besides, it is recommended to NOT document runtime exceptions which callers are not prepared to handle.
>>>>>>>>>>>>>>> I assume other getParameters() had not added it is because it happened they can return one.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> While people does not catch runtime exceptions but my understanding is that if you mentioned "fail" in the spec maybe it's better to add a @throws for it.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> For example, @throws SecurityException for File/Files operations.
>>>>>>>>>>>>>> Thinking more about this, I would be more inclined to recommend that you change the meaning of null as the return value to cover both cases:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> @return the parameters used with this signature, or null if this signature does not use any parameters or does not support default or randomly generated parameter values
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I don't think it is critical to make a distinction between these 2 cases, because if the programmer doesn't initialize it with parameters it will get a SignatureException anyway when it tries to call sign or verify.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> It's not perfect, but probably the best you can do working within the constraints of that API and minimizing compatibility risk.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --Sean
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>> Max
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>> Valerie
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 7/10/2018 7:16 PM, Weijun Wang wrote:
>>>>>>>>>>>>>>>>> Hi Valerie
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> About "it *may* return", do you mean it could also return null? My understanding is no.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Is it better to clarify when the implementation "may also fail"? From the CSR, it's this method. Can you add a @throws spec to this method then?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Also, I am a little confused by "default and randomly generated". Does this actually mean "default (might be randomly generated)"? The "it may" sentence mentions "default and randomly generated" but the "if there" only says "default", which sounds like the the "randomly generated" case could be different.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>> Max
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On Jul 11, 2018, at 5:12 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Hi Brad,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Would you have time to review the fix for JDK-8206171: Signature#getParameters for RSASSA-PSS throws ProviderException when not initialized?
>>>>>>>>>>>>>>>>>> No source code changes, but just updating javadoc to mention the possible failure case.
>>>>>>>>>>>>>>>>>> Otherwise, JCK team expects a parameter object or null being returned.
>>>>>>>>>>>>>>>>>> I filed a CSR to track the javadoc clarification.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8206171
>>>>>>>>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.00/
>>>>>>>>>>>>>>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8206864
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>> Valerie
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>