RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

[Bradford Wetmore]

On 4/20/2016 4:30 PM, Wang Weijun wrote:

>>>> 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
>
> I'd keep the
>
>    "pr_and_reseed" | "reseed_only" | "none"
>
> line and put all descriptions into the comment, to make this still BNF style.

Ah...something like:

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

>>>> 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).
>
> java.security is better.

Are you going to at least list all the algorithms in 
SunProviders.html#SUNProvider too?

>>>> 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.
>
> A comment in java.security mentioning an internal class?

Other way around.  In the internal class where the defaults are set, you 
mention that if any of these change, you need to update the 
java.security accordingly.

Brad