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

Bradford Wetmore bradford.wetmore at oracle.com
Tue Jul 24 23:59:30 UTC 2018


Returning to this briefly, looks good to me too.

Brad



On 7/19/2018 6:23 PM, Valerie Peng wrote:
> Thanks Max and Sean for the review and suggestions. I have updated the 
> webrev accordingly and finalized CSR.
> 
> Webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.04/
> CSR: https://bugs.openjdk.java.net/browse/JDK-8206864
> 
> Valerie
> 
> On 7/19/2018 3:13 PM, Wang Weijun wrote:
>>
>>> 在 2018年7月20日,05:18,Valerie Peng <valerie.peng at oracle.com> 写道:
>>>
>>> Hi Sean,
>>>
>>> Thanks for the suggestion, I like it.
>> Me too.
>>
>>> Max, any objection or concern before I update the webrev and CSR?
>> No.
>>
>> Thanks
>> Max
>>
>>> 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