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