> On Apr 20, 2016, at 8:54 AM, Bradford Wetmore <bradford.wetm...@oracle.com>
> wrote:
>
> > Webrev updated again at
> >
> > http://cr.openjdk.java.net/~weijun/8051408/webrev.10/
> > http://cr.openjdk.java.net/~weijun/8051408/webrev.10/spec
> > http://cr.openjdk.java.net/~weijun/8051408/webrev.10/specdiff
>
> Some initial comments.
>
> security/java.security
> ======================
> 123-133: Would you consider changing the ordering of the algorithm
> names to be consistent between the two sections? Say:
>
> NativePRNG, SHA1PRNG, and DRBG.
Sure.
>
> 175: Should we add DRBG:SUN as a backup for non-windows?
If NativePRNGBlocking:SUN is not always available, yes we can.
>
> 181:
> # NIST SP 800-90Ar1 lists several DRBG mechanisms, each can be configured with
> ->
> # NIST SP 800-90Ar1 lists several DRBG mechanisms. Each can be configured with
>
> 184:
> request of "DRBG" SecureRandom implemented in the SUN provider.
> ->
> request of "DRBG" SecureRandom implementations in the SUN provider.
> (Other DRBG implementations might also use this property.)
OK.
>
> 188: Can you adjust the line lengths so it looks polished, and not
> edited in place? I use a vi macro (using fmt) for this.
OK.
>
> 189: Do you want to add "currently", or leave as is?
> names are not configurable
> ->
> names are not currently configurable
Yes.
>
> 194: Am I missing something?
> is a slash-separated list
> ->
> is a comma-separated list
Oops, will correct.
>
> 198: Should we add a short 1-liner description for the fields? The
> variable meanings (esp pr/df) may not be obvious to a casual observer.
> For example, using these three fields as an example:
>
> mech_name: default "Hash_DRBG"
> "Hash_DRBG" | "HMAC_DRBG" | "CTR_DRBG"
>
> + The DRBG mechanism to use.
>
> pr: default "none"
> "pr_and_reseed" | "reseed_only" | "none"
>
> + Prediction resistance...
>
> df: default "use_df", only applicable to CTR_DRBG
> "use_df" | "no_df"
>
> + Derivation Function...
Not sure about the format, how about
# # The DRBG mechanism to use. default "Hash_DRBG"
# mech_name:
# "Hash_DRBG" | "HMAC_DRBG" | "CTR_DRBG"
So this is double comment. Please note I also put the default value into the
comment.
>
> 210: Instead of pointing to 800-90A here, you should instead point to
> the Sun Provider document. That document will then reference
> 800-90A/Section 10, and will use the Standard Algorithm names that
> you have defined for these algorithms. (I assume you'll be adding
> those to the standard algorithm name doc, right? I don't recall seeing
> that part of the review yet, but maybe haven't gotten that far.)
Yes, I will.
The standard algorithm name is only "DRBG", but here it's the DRBG algorithm
name (SHA-256 etc). Are we going to talk about this in Sun Provider doc?
>
> 214: In the past, we've used spaces as a field separator for names in
> a provider, I'm wondering if we should make this name something like
> "3KeyTDEA" instead?
OK.
>
> 229: I hadn't noticed this before, but the Security variable "drbg"
> doesn't match the style of the other variables, securerandom.* or
> otherwise. I think we should use something like either:
>
> securerandom.drbg
> securerandom.drbg.config
Will choose "securerandom.drbg.config".
>
> 229: Why an empty string here? Why not actually specify the default here
> instead of burying the default somewhere where it might changed without
> a corresponding update to this file?
There is a small technical issue here. The problem is that the default value of
algorithm_name depends on mech_name, and the default value of strength depends
on algorithm_name. If we set "Hash_DRBG,SHA-256,128" here and we only change
one of them, another one might be illegal.
For example, if I want to create a SecureRandom using MoreDrbgParameters (yes,
this is an internal class) with
SecureRandom.getInstance("drbg", new MoreDrbgParameters(
null, "CTR_DRBG", "3KeyTDEA", null, false,
DrbgParameters.instantiate(-1, NONE, null)))
it will fail, because with -1 in DrbgParameters.instantiate(), the new
configuration will look like
CTR_DRBG,3KeyTDEA,128
but CTR_DRBG,3KeyTDEA does not support 128 bit strength.
I will have to use DrbgParameters.instantiate(112, NONE, null) but that does
not looks comfortable because I thought the system can give me a default
working strength.
This won't be a problem if only public API is used, but I will have to redo
some implementation codes.
Thanks
Max
>
> Thanks,
> Brad
>
