> On Jul 18, 2018, at 8:23 AM, Valerie Peng <valerie.p...@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.p...@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.p...@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.p...@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.p...@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
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>
