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".
- null may be returned
How about "{@code null} must be returned"?
Everything else looks fine.
Thanks
Max
> On Jul 17, 2018, at 9:46 AM, Valerie Peng <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>> 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 <[email protected]>
>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>
