Looks fine to me. Thanks for take the comments. Xuelei
On 4/10/2013 3:17 PM, Brad Wetmore wrote: > Hi Xuelei/Weijun, > > Thanks for the comments. > > The version I plan to putback is: > > http://cr.openjdk.java.net/~wetmore/6425477/webrev.05/ > > I probably won't have time to take any further comments before vacation, > but can address things when I'm back. I have a few more from Bernd that > I'd like to look at. > > On 1/17/2013 7:44 AM, Xuelei Fan wrote: >> 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. > > I think I've addressed all. > >> 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); >> } >> ... >> } >> } > > Good point. Other people mentioned this privately. > > BTW, as you have above, this Pattern can't be final (not a constructor), > and needs to be volatile. To avoid multiple initializations, I went > with the Effective Java 2nd Edition Item 71 (FileHolder) pattern instead. > >> 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. > > Changed. Weijun also mentioned this. I now allow: > > * Entries are alg:prov separated by , > * Allow for prepended/appended whitespace between entries. > >> 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. > > I modified this quite a bit and now the entire parsing logic is in one > regexp. I iterate over it until group(5) is null. > > * Capture groups: > * 1 - alg > * 2 - :prov (optional) > * 3 - prov (optional) > * 4 - ,nextEntry (optional) > * 5 - nextEntry (optional) > > \\s*([\\S&&[^:,]]*)(\\:([\\S&&[^,]]*))?\\s*(\\,(.*))? > >> 1-4. a typo at line 614 [P1]: >> - 614 // Pattern for "algorithm.provider" >> + 614 // Pattern for "algorithm:provider" > > Done. > >> java.security-windows >> ------------------------------------- >> 2-1. Is it possible to enable "NativePRNGBlocking:SUN" in Windows? [P2] > > To my knowledge, there is no concept of Blocking/NonBlocking or > quality-of-service levels in the SunMSCAPI CryptGenRandom API. There is > just one implementation (Vista SP1+: AES/NIST SP 800-90, earlier uses > FIPS 186-2.) > >> + 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. > > Yes. > >> If "SunMSCAPI" >> provider is not enabled, the "Windows-PRNG:SunMSCAPI" will not work. I >> think "NativePRNGBlocking:SUN" is not available on Windows system. > > Correct. > >> 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. > > I'm planning to have a new section in the Sun Providers document which > describes the various implementations available on which platforms, and > we'll use my internal SecureRandom wiki page as the base. > > I'm thinking seriously about adding a NativePRNG in the Sun Provider to > point to similar functionality that is in Windows-PRNG:SunMSCAPI. That > is, SHA1PRNG and NativePRNG would exist on all Oracle platforms. > > JDK-8011737: Consider adding a NativePRNG equivalent, > probably based on MSCAPI code. > > Of course, I'd also like to add > > JDK-8003584: Consider adding a more modern SecureRandom > implementation > >> P11SecureRandom.java >> ------------------------------------- >> 3-1: to support strong algorithm in PKCS11 [P4] >> Is SHA1PRNG:SunPKCS11 a strong algorithm? > > There is no SHA1PRNG in PKCS11. The PKCS11 SecureRandom impl reads > directly from the PKCS11 library. If setSeed was ever called, then use > a "mixer" SHA1PRNG to combine the seed with bytes from the PKCS11 impl. > >> I think it would be nice to >> add it as a backup in the strong algorithm property. > > One question which I'll answer but you didn't actually ask. :) > > AFAIK, SHA1PRNG:SUN has never undergone a formal evaluation for its > strength, so I hesitated including it in the list. All indications, > anecdotal reports, and some graduate level research have shown it is > quite strong, but it was a homebrew algorithm developed back in the days > when export control restricted what we could include in the JDK. > > For UNIXn, I'm thinking of sticking with the NonBlocking OS > implementations which return values only when "sufficient entropy has > been collected," and the MSCAPI which uses NIS 800-90. > >> 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. > > I also took out the following paragraph, which also talked specifically > about the file and its location. > >> 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? > > Yes, it will be added. > >> Otherwise, I would suggest to comment out this >> algorithm. The above would set a external SecureRandom algorithm, I > think. > > Yes, it does. > > I talked around this in the CCC, but I didn't actually spell out the new > names. That's an oversight that probably needs to be corrected. I've > sent Joe Darcy an email. Here's the current CCC description: > > NativePRNG reads seeds from /dev/random and nextBytes from > /dev/urandom. I added two new NativePRNG implementations which are > completely blocking or nonblocking. The > "securerandom.strongAlgorithms" property points to the blocking > variant. > > NativePRNG reads seeds from securerandom.source/java.security.egd > (default: /dev/random), and nextBytes from /dev/urandom > > NativePRNGBlocking reads both seed and nextBytes from /dev/random > > NativePRNGNonBlocking reads both seed and nextBytes from /dev/urandom. > >> 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." > > Thanks. > >> 6-2: Do you want to put something here? [P4] >> >> 321 // XXX change the urandom/random to seed/next > > Yes. XXX was my marker to not forget that. I have finished the > variable renaming and updated the comments. > >> 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(); > > This super() call could be left out, since there is just the > SeedGenerator default constructor. In the Solaris/UNIX equivalent, it > extends URLSeedGenerator, which does have a 1-arg constructor. > >> 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? > > Let's see if I can explain sufficiently. > > When SeedGenerator.generateSeed() is called from > sun.security.SecureRandom, a single class instance of the SeedGenerator > is created via the static initializer. There are three possible > variants, Native/URL/Threaded. Depending on the values of the > egdSource, we'll select one of the three, and set the global "instance" > variable accordingly. Calls to SeedGenerator.generateSeed() are then > rerouted to the underlying instance.getSeedBytes(). getSeedBytes is an > abstract method in SeedGenerator, but concrete implementations are > available in all three variants. > > Hope that helps. > > Brad > > > >> 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 >>> >>> >>> >>
