> On Jan 6, 2016, at 1:21 AM, Sean Mullan <[email protected]> wrote:
>
> Here are some more comments on the API. I will send some comments on the impl
> next.
>
> * DrbgParameters
>
> 38 * A DRBG mechanism should extend this class.
>
> Is this sentence necessary? None of the builtin DRBG mechs extend this class.
I was wrong.
>
> 175 * If this method is not called, the implementation will select
> 176 * a default {@code EntropyInput}.
>
> I think you need to be more specific about what you mean by "the
> implementation" (and in other methods of this class). It is not the
> implementation of this class, it is the DRBG SecureRandom implementations.
OK.
>
> 236 * Requests that a derivation function to be used by the DRBG
> mechanism.
>
> s/to be used/is to be used/
>
> 280 * @return teh newly built {@code DrbgParameters} object
>
> s/teh/the/
>
> The get methods should state whether they can return null or not. Some of the
> methods should be more specific about whether they return or make copies of
> mutable parameters.
OK.
>
> * SecureRandomSpi
>
> 67 protected SecureRandomSpi() {
>
> This seems like an incompatible change to me. The previous version had a
> default public constructor. I think this needs to be public.
OK.
>
> 75 * This argument can be {@code null}.
>
> Why can't we call the SecureRandomSpi ctor that takes no args when the
> parameter is null? It seems it would be cleaner to do that, but there is
> probably a good reason.
SecureRandom.getInstance("HashDRBG") will call new HashDrbg(null), which calls
a series of super(null) until SecureRandomSpi. The @implSpec of the class has
more details.
I can revisit the change in Provider.java. Maybe the following will work:
if (param == null) {
findCtorWithNoArg().newInstance();
} else {
findCtorWithArg(argClazz).newInstance(param);
}
>
> 109 * @param bytes output.
>
> Too terse. How about "the array to be filled in with random bytes"
OK.
>
> 110 * @param additionalInput additional input. {@code null} if none.
>
> I think you should throw NPE if this is null because callers should call
> engineNextBytes(byte[]) in that case.
OK.
I'll need to implement engineNextBytes(1,2) and engineNextBytes(1) in all
implementations and probably let them call something like nextBytesInternal().
>
> 110, 134: both of these methods need an @throws UnsupportedOperationException
OK.
>
> 134 * @param additionalInput additional string, {@code null} if none.
>
> I think it would be better to add 2 engineReseed methods, one which takes no
> arguments and one with an additionalInput arg that throws NPE if it is null.
> This would also be consistent with the corresponding methods on SecureRandom.
OK.
>
> * SecureRandom
>
> 68 * {@link SecureRandomParameters} parameter. For example, a DRBG
> 69 * can be initialized with a {@link DrbgParameters} object.
>
> s/a DRBG/a DRBG implementation/
>
> 87 * <p> Except for the one created by {@link #SecureRandom(byte[])}, a newly
>
> s/one/instance/
>
> 88 * instantiated SecureRandom object is not seeded. Call one of {@code
> reseed}
> 89 * or {@code setSeed} methods to seed it. If none is called, the first
> call
>
> s/Call one of/Call the/
> s/methods/method/
> s/If none/If neither method/
>
> 92 * at instantiate time through a {@code SecureRandomParameters} object.
>
> s/instantiate/instantiation/ (or creation)
>
> 94 * This self-seeding will not occur if one of {@code reseed} or {@code
> setSeed}
> 95 * methods was previously called.
>
> s/one of/the/
> s/methods/method/
>
> 97 * <p> A SecureRandom can be reseeded at any time by calling one of the
>
> s/one of the/the/
> s/methods/method/
>
> 883 * {@code additionalInput} may contain entropy but its main usage is
> to
> 884 * provide diversity.
>
> s/entropy but/entropy, its/
OK.
Thanks
Max
>
> --Sean
>
> On 12/16/2015 02:20 AM, Wang Weijun wrote:
>> Webrev updated:
>>
>> http://cr.openjdk.java.net/~weijun/8051408/webrev.02/
>>
>> http://cr.openjdk.java.net/~weijun/8051408/webrev.02/specdiff/java/security/package-summary.html
>>
>> Changes:
>>
>> 1. DrbgParameters has a Builder now
>>
>> 2. No more default implementation for reseed()
>>
>> 3. Synchronization is now inside implementation, on engineXXX() methods
>>
>> 4. engineConfigure() now throws InvalidAlgorithmParameterException instead
>> of IllegalArgumentException
>>
>> 5. CtrDRBG uses up all input in engineSetSeed()
>>
>> Thanks
>> Max
