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

[Valerie Peng]

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