Update #4: JEP 123: SecureRandom Draft and Implementation.

[Xuelei Fan]

Hi Brad,

Please note the priorities for each comment. As the M6 is coming, you
can only take P1 comments.

P1: need to update
P2: suggested update, you can take it after M6.
P3: minor comments, it's OK to leave it unchanged.
P4: personal preference for your consideration, or my question.

java/security/SecureRandom.java
-------------------------------------
1-1. the performance of Pattern.compile [P2]
   Pattern.compile() is expensive. I would suggest to use private static
lazy-initialized class attribute the patterns.

   public class SecureRandom extends java.util.Random {
       private static final String regex = "...";
       private static final Pattern pattern;

       public static SecureRandom getStrongSecureRandom() {
           ...
           if (pattern == null) {
               pattern = Pattern.compile(regex);
           }
           ...
       }
   }

1-2. spaces are allowed between algorithm and provider [P4]
   According to the regex ("\\s*(\\S+)\\s*\\:\\s*(\\S+)\\s*"), spaces
are allowed around the tokens. For example, " NativePRNGBlocking : SUN
" is valid.  I would like to use a stricter syntax at the beginning in
case of any special requirement comes in the future.

1-3. may only need one regex for both "algorithm" and
"algorithm:provider" [P3]
  I think one regex is OK for both: "([\\S&&[^:]]+)(\\:([\\S&&[^:]]+))?".

  NativePRNGBlocking:Sun
      group 1: NativePRNGBlocking
      group 2: :Sun
      group 3: Sun

  NativePRNGBlocking
      group 1: NativePRNGBlocking
      group 2: null
      group 3: null

  If group 2 is non-null, it is of the "algorithm:provider" style.

1-4. a typo at line 614 [P1]:
- 614         // Pattern for "algorithm.provider"
+ 614         // Pattern for "algorithm:provider"


java.security-windows
-------------------------------------
2-1. Is it possible to enable "NativePRNGBlocking:SUN" in Windows? [P2]

+    securerandom.strongAlgorithms=Windows-PRNG:SunMSCAPI

I was wondering to enable "NativePRNGBlocking:SUN" here before I know
that the "NativePRNGBlocking:SUN" is not available on Windows:
+    securerandom.strongAlgorithms=Windows-PRNG:SunMSCAPI, \
+                                  NativePRNGBlocking:SUN

The availability of "securerandom.strongAlgorithms" property depends on
the enabled security providers, and the platform.  If "SunMSCAPI"
provider is not enabled, the "Windows-PRNG:SunMSCAPI" will not work.   I
think "NativePRNGBlocking:SUN" is not available on Windows system. It is
not as obviously as that the "Windows-PRNG:SunMSCAPI" is not available
on Unix/Linux/Mac OS systems. We need to documentation this behaviors
clear somewhere else.

P11SecureRandom.java
-------------------------------------
3-1: to support strong algorithm in PKCS11 [P4]
Is SHA1PRNG:SunPKCS11 a strong algorithm?  I think it would be nice to
add it as a backup in the strong algorithm property.


SeedGenerator.java
-------------------------------------
4-1: downgrade normative reference to java security property file [P3]
[line 57-60] As you have already there, I would suggest to use the new
style of security property. See
http://hg.openjdk.java.net/jdk8/tl/jdk/rev/346c0af4af41 and the
description from SeanM,
http://mail.openjdk.java.net/pipermail/security-dev/2012-December/006144.html.

line 57-60:
- * accomplished by setting the value of the "securerandom.source"
- * Security property (in the Java security properties file) to a URL
- * specifying the location of the entropy gathering device, or by setting
-  * the "java.security.egd" System property.

+ * accomplished by setting the value of the
+ * {@code securerandom.source} security property to a URL
+ * specifying the location of the entropy gathering device, or by setting
+  * the {@code java.security.egd} System property.


SunEntries.java
-------------------------------------
5-1: what's the usage of "NativePRNGNonBlocking"? [P2]

+        if (NativePRNG.NonBlocking.isAvailable()) {
+            map.put("SecureRandom.NativePRNGNonBlocking",
+                "sun.security.provider.NativePRNG$NonBlocking");
+        }

I did not find the description of this algorithm in the specification
(CCC) or other export documentation.  Do you want to add it to Oracle
provider names doc?  Otherwise, I would suggest to comment out this
algorithm. The above would set a external SecureRandom algorithm, I think.

sun/security/provider/NativePRNG.java
-------------------------------------
6-1: line 36-42, wordsmithing. [P3]

  "It obtains seed and random numbers by reading system files such as
   the special device files /dev/random and /dev/urandom.  This
   implementation respects the {@code securerandom.source} security
   property and {@code java.security.egd} system property
   for obtaining seed material.  If the file
   specified by the properties does not exist, /dev/random is the
   default seed source, and /dev/urandom is the default source of
   random numbers."

6-2: Do you want to put something here? [P4]

 321 // XXX change the urandom/random to seed/next


src/windows/classes/sun/security/provider/NativeSeedGenerator.java
-------------------------------------
7-1: Not about this fix, but the code looks strange to me. [P4]
The constructor calls:
   44         super();

The SeedGenerator static block will be called and SeedGenerator.instance
will be initialized.

According to the code in SeedGenerator.java:
 145     static public void generateSeed(byte[] result) {
 146         instance.getSeedBytes(result);
 147     }

The getSeedBytes() method of the initialized instance will be used.
However, in Windows platform, I think the
NativeSeedGenerator.getSeedBytes() should be called, I think.

I think the NativeSeedGenerator class should override the generateSeed()
method.  About the super() call in NativeSeedGenerator, I think the
initialization of instance (line 92-142, SeedGenerator.java) may be not
necessary.  The initialized instance in SeedGenerator is useless in
Windows from my understand. Am I missing something?

Otherwise, looks fine to me.

Xuelei


On 1/11/2013 8:24 AM, Brad Wetmore wrote:
> Minor tweak.  It occurred to me that people might use "." as separators
> (for example using some OIDs scheme), so I changed the syntax slightly
> of the system property to use ":" instead.
> 
> For example:
> 
> # This is a comma-separated list of algorithm and/or algorithm:provider
> # entries.
> #
> securerandom.strongAlgorithms=NativePRNGBlocking:SUN,
> SP800-90A/AES/CTR:IBMJDK
> 
> Latest is now webrev.04.
> 
> http://cr.openjdk.java.net/~wetmore/6425477/
> 
> Brad
> 
> 
>