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