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