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

Sean Mullan sean.mullan at oracle.com
Thu Jul 19 14:28:37 UTC 2018


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
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
> 


More information about the security-dev mailing list