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.p...@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.p...@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.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
