RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

[Wang Weijun]

Thanks for the review. All comments accepted.

--Max

> On Apr 5, 2016, at 9:13 PM, Seán Coffey <sean.coffey at oracle.com> wrote:
> 
> A few comments from supportability side of the table.
> 
> =============
> sun/security/provider/AbstractDrbg.java
> 
>> +            if (dp.getStrength() > strength) {
>> +                throw new IllegalArgumentException("strength too high");
>> +            }
>> +            if (result.length > maxNbLength) {
>> +                throw new IllegalArgumentException("result too long");
>> +            }
> Please print these bad strengths / results in the exception.
> 
> Similar corrections are needed in the engineConfigure method :
> 
>>  608                 if (config.getStrength() > highestSecurity) {
>>  609                     throw new IllegalArgumentException("strength too big");
>>  610                 }
>>  611                 if (config.getPersonalizationString() != null && config.getPersonalizationString().length > maxPsLength) {
>>  612                     throw new IllegalArgumentException("ps too long");
>>  613                 }
> 
>> throw new IllegalArgumentException("unknown params type");
> can you print the type of params that was passed in ? (X 2 calls)
> 
> 
>> +            if (dp.getAdditionalInput() != null && dp.getAdditionalInput().length > maxAiLength) {
>> +                throw new IllegalArgumentException("ai too long");
> Please print ai value.
> 
> 
>> +                    // This SEI does not support pr
>> +                    throw new IllegalArgumentException();
> Cou you put your comment in the body of the IllegalArgumentException ?
> 
> =============
> sun/security/provider/CtrDrbg.java
> 
> +        try {
> +            aesLimit = Cipher.getMaxAllowedKeyLength("AES");
> +        } catch (Exception e) {
> +            // should not happen
> +            throw new AssertionError("Cannot detect AES");
> +        }
> 
> Just to be safe, can you add e as Throwable variable for AssertionError ?
> 
> 
>> +        if (input.length != seedLen) {
>> +            // Should not happen
>> +            throw new IllegalArgumentException("input must be of seedlen bytes");
> 
> can you print the lengths expected ?
> =============
> sun/security/provider/DRBG.java
> 
>> +                            if (strength < 0) {
>> +                                throw new IllegalArgumentException(
>> +                                        "strength in drbg cannot be negative");
>> +                            }
> Let's print the value of the 'part' string in this exception.
> 
> +            } else {
> +                throw new IllegalArgumentException("Unsupported params");
> +            }
> 
> can you print the type of params that were passed in ?
> 
> +            default:
> +                throw new IllegalArgumentException("Unsupported mech");
> +        }
> 
> can yuo print the mech value encoutered ?
> =============
> sun/security/provider/HashDrbg.java
> 
>> +            } catch (DigestException e) {
>> +                throw new AssertionError("will not happen");
>> +            }
> 
> Famous last words ;)
> Can you add e as Throwable cause to AssertionError  ? (happens in two areas)
> 
> Regards,
> Sean.
> 
> On 05/04/2016 03:34, Wang Weijun wrote:
>> Updated webrev again at
>> 
>>  http://cr.openjdk.java.net/~weijun/8051408/webrev.09/
>>  http://cr.openjdk.java.net/~weijun/8051408/webrev.09/spec
>>  http://cr.openjdk.java.net/~weijun/8051408/webrev.09/specdiff
>> 
>> The only change is that SecureRandomInstantiateParameters, SecureRandomNextBytesParameters and SecureRandomReseedParameters are removed and only a single SecureRandomParameters is added. There seems no reason to introduce 3 marker interfaces.
>> 
>> Thanks
>> Max