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

Valerie Peng valerie.peng at oracle.com
Thu Jul 19 21:18:45 UTC 2018


Hi Sean,

Thanks for the suggestion, I like it.

Max, any objection or concern before I update the webrev and CSR?

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



More information about the security-dev mailing list