RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

[Bradford Wetmore]

>> 175:  Should we add DRBG:SUN as a backup for non-windows?
>
> If NativePRNGBlocking:SUN is not always available, yes we can.

It should be available, unless someone decides to blow away 
/dev/(u)random.  But then DRBG will have the same problem.

One advantage about listing it here is that deployments know there is a 
good replacement if they don't like NativePRNG for some reason.

I'm fine with adding or leaving alone.

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

Yes, that works.  Or java-style comments /* */ or //.

Maybe expanding the values too?

#   /*
#    * Prediction Resistance options
#    *   default: "none"
#    */
#   pr:
#     "pr_and_reseed" | "reseed_only" | "none"
#
#         "pr_and_reseed" - Both Prediction Resistance and
#                           reseeding support requested
#         "reseed_only"   - Only reseeding support requested
#         "none"          - Neither prediction resistance nor
#                           reseeding support requested

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

StandardNames.html#SecureRandom
===============================
   NativePRNG Obtains...
   ...
+ DRBG Obtains from an 800-90A impl...

SunProviders.html#SUNProvider
=============================
SecureRandom: SHA1PRNG
               NativePRNG
               ...
+             DRBG

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

Valid point, since we haven't standardized these options, then we 
probably shouldn't mention them in the Standard Alg names document.  But 
we can still talk about this in the Sun Provider doc.

You can make a new table and put it at the end of the Sun provider 
section.  I suppose you could also put the expanded definitions here 
instead of java.security (or both).

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

Great, thanks.  The CCC will need an update with the new name, or else a 
new CCC.  There's been no votes on your "finalized" version yet, so you 
might be able to withdraw/resubmit.

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

Ok.  If you haven't already, please add a comment where the default 
value is stored to update the java.security file if this default value 
is ever changed.

Thanks,

Brad