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