Re: RFR: 8199018: Test crypto provider not registering
Thanks for pointing out where the review was, I don't check the update email list as often. Reviewed in jdk8u-dev. Looks good. Brad On 3/6/2018 1:03 AM, Seán Coffey wrote: Thanks Brad - yes, this review is for JDK 11 (jdk/jdk). I've another review request open for you in order to handle the jdk8u-dev edit : http://mail.openjdk.java.net/pipermail/jdk8u-dev/2018-March/007284.html regards, Sean. On 05/03/2018 23:41, Brad Wetmore wrote: Looks good. This looks like it still needs to be done in jdk/jdk. Are you planning on doing that? Thanks, Brad On 3/5/2018 6:37 AM, Seán Coffey wrote: Brad pointed out to me off-line that the test case from 8193892 needs correcting. Some late editing to the test broke functionality. I've corrected the testcase and added extra checks to ensure that "MyProvider" is being registered correctly. https://bugs.openjdk.java.net/browse/JDK-8199018 http://cr.openjdk.java.net/~coffeys/webrev.8199018/webrev/
Re: RFR: 8199018: Test crypto provider not registering
Looks good. This looks like it still needs to be done in jdk/jdk. Are you planning on doing that? Thanks, Brad On 3/5/2018 6:37 AM, Seán Coffey wrote: Brad pointed out to me off-line that the test case from 8193892 needs correcting. Some late editing to the test broke functionality. I've corrected the testcase and added extra checks to ensure that "MyProvider" is being registered correctly. https://bugs.openjdk.java.net/browse/JDK-8199018 http://cr.openjdk.java.net/~coffeys/webrev.8199018/webrev/
Re: provider registration
On 2/28/2018 8:36 AM, Bernd wrote: Hello, there was a thread on BouncyCastle's crypto-dev mailing list how to use a custom JCA provider with Java 9+. Since there is no alternative for the lib/ext extension mechanism this is a bit tricky (if you do want to make the extension in the java.security file permanent). There are multiple alternatives (adding to module path, to classpath, using service loader or programmatic registration). Those are described in the actual documentation. Hopefully it's clear. There was a lot of moving parts and sharp edges as JDK 9 came to a close. Comments welcome of course. However expanding the java.security list To be clear, when you say "expanding the java.security list", you mean adding an entry like: security.provider.14=MyProvider does not mention explicitely that without the extension mechanism this produces a java home which wont start without modifying the module path. The JDK should still start, but it wouldn't be able to find MyProvider if it's not in either the class or module path. Not sure if there is actually a default way to storesuch a "security provider module" Not aware of anything for JDK 9+, short of putting your provider into the module/classpath directory. As you note, the extension mechanism/directory was removed in the JDK 9 (see JEP 220). without using for example jlink to build a new image (or add the -mp argument). Please note from the Provider documentation: You can link a provider in a custom runtime image with the jlink command as long as it doesn't have a Cipher, KeyAgreement, or MAC implementation. Providers that don't have Cipher/KeyAgreement/MAC implementation can be jlinked. However, providers providing Cipher/KeyAgreement/MAC implementations must be signed using a valid certificate to pass the export tests in implementations like Oracle JDK. We only support signed modular jars (module-path) and signed jars (classpath). There is not a signed JMOD feature, and thus jlink can't be used to create an image that will pass the export tests. Maybe this should be stated explicite? "Starting with Java 9 there is no extension mechanism where you could install the provider JAR permanently. Therefore expanding the java.security leaves typically a incomplete java home and should be avoided. Permanently installing an additional module could be done with a custom jlink image." (I havent tested if JLink works, BCProv is not yet modularized or service loader enabled. Classpath and programmatic registration works fine). Is that correct? We could add some wording here to talk about the removal of the extension mechanism. Hope that helps. Brad
Re: RFR: 8193892: Impact of noncloneable MessageDigest implementation
Minor comments. I'm ok with leaving as is, but this seems cleaner.
MyProvider.java
---
Would prefer to use the non-deprecated super call.
*Digest.java
Would you consider making these 3 duplicate classes into a single class,
with the three public derived classes within? And then update the
MyProvider entries with DigestBase${MD5,SHA,SHA256}. i.e.
import java.security.*;
class DigestBase extends MessageDigestSpi {
private MessageDigest digest = null;
public DigestBase(String alg, String provider) throws Exception {
digest = MessageDigest.getInstance(alg, provider);
}
@Override
protected void engineUpdate(byte input) {
digest.update(input);
}
@Override
protected void engineUpdate(byte[] input, int offset, int len) {
digest.update(input, offset, len);
}
@Override
protected byte[] engineDigest() {
return digest.digest();
}
@Override
protected void engineReset() {
digest.reset();
}
public static final class MD5 extends DigestBase {
public MD5() throws Exception {
super("MD5", "SUN");
}
}
public static final class SHA extends DigestBase {
public SHA() throws Exception {
super("SHA", "SUN");
}
}
public static final class SHA256 extends DigestBase {
public SHA256() throws Exception {
super("SHA-256", "SUN");
}
}
}
Thanks, sorry for the delay.
Brad
On 2/15/2018 7:40 AM, Xuelei Fan wrote:
Looks fine to me. Thanks!
Xuelei
On 2/15/2018 1:04 AM, Seán Coffey wrote:
A reminder for this review..
regards,
Sean.
On 09/02/2018 16:25, Seán Coffey wrote:
Looking to push a new test which helps test CloneableDigest code.
It's something that arose during JDK-8193683 fix.
The test was contributed by Brad Wetmore. I've converted it to use
the SSLSocketTemplate.
JBS : https://bugs.openjdk.java.net/browse/JDK-8193892
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8193892/webrev/
Re: Code Review Request:8019544: Need to run ProviderTest.java in othervm mode
Looks good. brad On 7/25/2013 3:17 PM, Rajan Halade wrote: Could you please review the small fix for 8019544: http://cr.openjdk.java.net/~juh/rajan/8019544/webrev.00/ http://cr.openjdk.java.net/%7Ejuh/rajan/8019544/webrev.00/ This is a test only fix for test stabilization. -- http://www.oracle.com Rajan Halade, CISSP | Senior Member of Technical Staff Phone: +1 4082763359 tel:+1%204082763359 Oracle JAVA Platform Group 4220 Network Circle, Bldg 22, #2155 | Santa Clara, CA 94538 http://www.oracle.com/commitment Oracle is committed to developing practices and products that help protect the environment
Re: Code review request: 6755701 SecretKeySpec DES
It's not common to use this style: 74 throw new InvalidKeySpecException 75 (Inappropriate key specification); but rather: throw new InvalidKeySpecException( Inapp...); Also, what happens in the case that the size doesn't match up with what DESKey's constructor needs? For example, if you provide 7 bytes, won't that throw a InvalidKeyException and thus you get a null back from engineGenerateSecret? The SecretKeyFactory.generateSecret() API doesn't mention anything about possibly getting a null back. I know that's the existing behavior, but that seems fishy to me. Bug in API? Brad On 6/28/2013 5:33 PM, Xuelei Fan wrote: Looks fine to me. Xuelei On 6/29/2013 1:40 AM, Anthony Scarpino wrote: ping... On 06/13/2013 05:08 PM, Anthony Scarpino wrote: Hi all, I'm requesting a code review for the below bug 6755701 SunJCE DES/DESede SecretKeyFactory.generateSecret throws InvalidKeySpecExc if passed SecretKeySpec http://cr.openjdk.java.net/~ascarpino/6755701/webrev.00/ Thanks Tony
Re: Code review request, 7188658 Add possibility to disable client initiated renegotiation
Bernd, I also think this is an excellent suggestion. Glad that you made it and Xuelei is considering it. Sorry I didn't get a chance to respond before I left for the weekend. There will be a bit of work to do in the current code to support abandoning a handshake, but shouldn't be too bad. Brad On 6/27/2013 5:51 PM, Xuelei Fan wrote: On 6/28/2013 8:05 AM, Bernd Eckenfels wrote: Am 28.06.2013, 01:51 Uhr, schrieb Xuelei Fan xuelei@oracle.com: Please don't send a no_renegotiation warning alert. Warning message is not very useful because in general the sending party cannot know how the receiving party behave. The server side need to reject client initiated renegotiation proactively. Just for the record, I totally disagree. I would make the option a multi value like accept(default)|ignore|reject. Because you never can know how the other side reacts. Ignoring renego requests is totally safe in the spec and in a situation where you chose to turn off renogotiation by clients you can have only two things: a) clients continue to work when you ignore them b) clients break If you always terminate the connection there is no chance for some clients to keep working. Great suggestion. I will consider to add the new ignore option. Today you can already achieve the termination of connection (by disabling all ciphersuites after initial handshake). You dont need to add code if you dont offer more (i.e. ignore) options. You're right here. This new system property is just to make the work a little easier that developers won't need to touch the source code in applications. Greetings Bernd PS: and regarding the naming a question, is JSSE the name of the Sun implementaion or of the Specification? I intend to use it for specification. But the names in JSSE is a little confusing now (See Brad's response). We need to consolidate the naming style. Xuelei
Re: Review Request: JDK-8019227: JDK-8010325 broke the old build
The old build has not produced usable bits for several months, it may not have been failing but if you look closely (or run the tests) then you'll see that there are several things missing. On build-dev then you'll probably have seen a mail or two from me where I was trying to encourage the build group to set a date to finally retire and remove the old build - this is because the old build is just a tax on every change that needs to touch the build. Anyway, the change looks okay but I strongly encourage you to put in the time to resolve the usability or others ssues that arise when working on JCE. Alan, I tried to save you the effort of composing just such a reply. I've seen your mails and agree with your sentiment. You are preaching to the choir [1]. ;) Thank you Chris, for doing the operation! Brad [1] http://idioms.thefreedictionary.com/preach+to+the+choir
Re: Code review request, 7188658 Add possibility to disable client initiated renegotiation
Going back to this... On 6/18/2013 9:04 PM, Xuelei Fan wrote: On 6/19/2013 10:50 AM, Brad Wetmore wrote: Sorry for the delay. Two comments about the property name. I don't see the jdk.tls *System* Property prefix used anywhere else, except as a *Security* properties: jdk.tls.rejectClientInitializedRenego Otherwise, I think we should stick to one of the current namespace. jsse. (my preference since this is expected across other JDKs) sun.security.ssl. At the beginning, I use the prefix jsse.*. But it is rejected (CCC) because the system property has no effect on other providers. ...and... jsse prefix should not be used in provider level libraries (for example, Oracle JSSE provider). I'm not following the logic at all here. I think I'm hearing that: jsse.*:used by the javax.net.ssl.* code jdk.tls.*: used by the sun.security.ssl.* code But that doesn't quite make sense since we already have the following SunJSSE-specific properties: jsse.enableSNIExtension, jsse.SSLEngine.acceptLargeFragments, jsse.enableCBCProtection. For the javax.net.ssl.* code, we already have been using since very early on: ssl.*: used by the javax.net.ssl.* code. e.g. KeyManagerFactory, TrustManagerFactory, ServerSocketFactory.provider, SocketFactory.provider And then there are the other sun.security.ssl.* and javax.net.ssl.* for SunJSSE providers that we expect other providers will use: sun.security.ssl.* allowUnsafeRenegotiation, allowLegacyHelloMessages javax.net.ssl.* keyStore, keyStorePassword, etc. trustStore, trustStorePassword, etc. And now we're adding yet another naming scheme? If it weren't for: http://docs.oracle.com/javase/7/docs/technotes/guides/security/jsse/JSSERefGuide.html#Customization I'd be completely lost. Brad Now, two styles are acceptable. jdk.* is for JDK properties, and jsse.* is used when other parties also should follow it. sun.security.ssl is deprecated, I think. Regarding rejectClientInitializedRenego, I think the word Initiated more succinctly captures the intent of this property, rather than Initalized. And as long as the word Renegotiation is, it should probably be spelled out completely. AFAIK, we rarely abbreviate external names like this. Same initiated comment about the variable name in your codereview, but I don't care much about Renogo. I prefer Initiated to Initalized. Let's make the update in another update. Thanks for considering. ServerHandshaker.java = 283: My initial thought was a no_renegotiation(100) warning, but that allows the client to decide what to do, rather than the server terminating. No, we cannot. First of all, warning message is not very useful because in general the sending party cannot know how the receiving party behave. Secondly, it is the expected behavior to *reject client initiated renegotiation. It is the server who should make the decision, but not the client. This TLS logic decision is not straightforward, so this needs commenting. I think reject client initialized renegotiation has say all. ;-) I will add words about state != HandshakeMessage.ht_hello_request. 379: since you're making changes here. response-respond OK. Since you already have CCC approval and are ready to putback, I would suggest putting back now, and let's file another CCC to change the name. That should be a no-brainer. OK. Thanks, Xuelei Thanks! Brad On 5/29/2013 7:05 PM, Xuelei Fan wrote: Got it. Yes, this fix is addressing a different issue from you mentioned below. Thanks, Xuelei On 5/30/2013 9:53 AM, Bernd Eckenfels wrote: Am 30.05.2013, 02:18 Uhr, schrieb Xuelei Fan xuelei@oracle.com: 2381456 Would you mind send me the link of the bug, or the code review request mail? I may miss some mails about this direction. I am afraid I cant sent the link, the Bug is in review state and therefore not visible for me. It was acknowledged 2012-11-12, see attached. I guess the link would be http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=2381456 (not sure if the numbers are the same in the new bug tool). Good suggestion. Oracle provider of JSSE had addressed the TLS renegotiation issue in JDK 1.4.2 update 26, JDK 1.5.0 update 24 and JDK 6u 19 around the end of 2009 and the beginning of 2010. Here is the readme of the fix: http://www.oracle.com/technetwork/java/javase/documentation/tlsreadme2-176330.html. Thats a different problem, I was thinking about preventing execessive client initiated renegotiations. This is for example CVE-2011-1473 from THC. You mentioned industry will move to a secure handshake - are you aware of any initiative in that direction? See http://www.rfc.org/rfc/rfc5746.txt. As far as I know, nearly all major vendors of SSL protocols has support RFC5746. Ok, but thats a different issue. I was expecting 7188658
Re: Code review request, 7188658 Add possibility to disable client initiated renegotiation
continued, I forgot this next part. ServerHandshaker.java = 283: My initial thought was a no_renegotiation(100) warning, but that allows the client to decide what to do, rather than the server terminating. No, we cannot. First of all, warning message is not very useful because in general the sending party cannot know how the receiving party behave. Secondly, it is the expected behavior to *reject client initiated renegotiation. It is the server who should make the decision, but not the client. Exactly. This TLS logic decision is not straightforward, so this needs commenting. And the above is what I wanted to see in the comments. That is, why we don't send a no_renegotiation warning alert. It's a subtle but important enough point that should be documented. I think we should open a separate bug to handle this. Just a couple of lines are needed. I think reject client initialized renegotiation has say all. ;-) I will add words about state != HandshakeMessage.ht_hello_request. Different comment. Brad
Re: Code review request, 7188658 Add possibility to disable client initiated renegotiation
Rearranging and tweaking a bit. What do you think of: --- Reject client initiated renegotiation? If server side should reject client-initiated renegotiation, send an alert_handshake_failure fatal alert, not a no_renegotiation warning alert (no_renegotiation must be a warning: RFC 2246). no_renegotiation might seem more natural at first, but warnings are not appropriate because the sending party does not know how the receiving party will behave. This state must be treated as a fatal server condition. This will not have any impact on server initiated renegotiation. --- Brad On 6/27/2013 4:51 PM, Xuelei Fan wrote: On 6/28/2013 6:44 AM, Brad Wetmore wrote: continued, I forgot this next part. ServerHandshaker.java = 283: My initial thought was a no_renegotiation(100) warning, but that allows the client to decide what to do, rather than the server terminating. No, we cannot. First of all, warning message is not very useful because in general the sending party cannot know how the receiving party behave. Secondly, it is the expected behavior to *reject client initiated renegotiation. It is the server who should make the decision, but not the client. Exactly. This TLS logic decision is not straightforward, so this needs commenting. And the above is what I wanted to see in the comments. That is, why we don't send a no_renegotiation warning alert. It's a subtle but important enough point that should be documented. I think we should open a separate bug to handle this. Just a couple of lines are needed. What do you think these words: Please don't send a no_renegotiation warning alert. Warning message is not very useful because in general the sending party cannot know how the receiving party behave. The server side need to reject client initiated renegotiation proactively. Thanks, Xuelei
Re: Code review request, 7188658 Add possibility to disable client initiated renegotiation
On 6/27/2013 4:59 PM, Xuelei Fan wrote: I agree that the property name is pretty confusing now. We need to consolidate the property naming styles, at least in JSSE component. I will go back to this topic later. I've filed: JDK-8019346 Reconsider the namespace for JDK-7188658 to track for 8. Brad Xuelei On 6/28/2013 6:36 AM, Brad Wetmore wrote: Going back to this... On 6/18/2013 9:04 PM, Xuelei Fan wrote: On 6/19/2013 10:50 AM, Brad Wetmore wrote: Sorry for the delay. Two comments about the property name. I don't see the jdk.tls *System* Property prefix used anywhere else, except as a *Security* properties: jdk.tls.rejectClientInitializedRenego Otherwise, I think we should stick to one of the current namespace. jsse. (my preference since this is expected across other JDKs) sun.security.ssl. At the beginning, I use the prefix jsse.*. But it is rejected (CCC) because the system property has no effect on other providers. ...and... jsse prefix should not be used in provider level libraries (for example, Oracle JSSE provider). I'm not following the logic at all here. I think I'm hearing that: jsse.*:used by the javax.net.ssl.* code jdk.tls.*: used by the sun.security.ssl.* code But that doesn't quite make sense since we already have the following SunJSSE-specific properties: jsse.enableSNIExtension, jsse.SSLEngine.acceptLargeFragments, jsse.enableCBCProtection. For the javax.net.ssl.* code, we already have been using since very early on: ssl.*: used by the javax.net.ssl.* code. e.g. KeyManagerFactory, TrustManagerFactory, ServerSocketFactory.provider, SocketFactory.provider And then there are the other sun.security.ssl.* and javax.net.ssl.* for SunJSSE providers that we expect other providers will use: sun.security.ssl.* allowUnsafeRenegotiation, allowLegacyHelloMessages javax.net.ssl.* keyStore, keyStorePassword, etc. trustStore, trustStorePassword, etc. And now we're adding yet another naming scheme? If it weren't for: http://docs.oracle.com/javase/7/docs/technotes/guides/security/jsse/JSSERefGuide.html#Customization I'd be completely lost. Brad Now, two styles are acceptable. jdk.* is for JDK properties, and jsse.* is used when other parties also should follow it. sun.security.ssl is deprecated, I think. Regarding rejectClientInitializedRenego, I think the word Initiated more succinctly captures the intent of this property, rather than Initalized. And as long as the word Renegotiation is, it should probably be spelled out completely. AFAIK, we rarely abbreviate external names like this. Same initiated comment about the variable name in your codereview, but I don't care much about Renogo. I prefer Initiated to Initalized. Let's make the update in another update. Thanks for considering. ServerHandshaker.java = 283: My initial thought was a no_renegotiation(100) warning, but that allows the client to decide what to do, rather than the server terminating. No, we cannot. First of all, warning message is not very useful because in general the sending party cannot know how the receiving party behave. Secondly, it is the expected behavior to *reject client initiated renegotiation. It is the server who should make the decision, but not the client. This TLS logic decision is not straightforward, so this needs commenting. I think reject client initialized renegotiation has say all. ;-) I will add words about state != HandshakeMessage.ht_hello_request. 379: since you're making changes here. response-respond OK. Since you already have CCC approval and are ready to putback, I would suggest putting back now, and let's file another CCC to change the name. That should be a no-brainer. OK. Thanks, Xuelei Thanks! Brad On 5/29/2013 7:05 PM, Xuelei Fan wrote: Got it. Yes, this fix is addressing a different issue from you mentioned below. Thanks, Xuelei On 5/30/2013 9:53 AM, Bernd Eckenfels wrote: Am 30.05.2013, 02:18 Uhr, schrieb Xuelei Fan xuelei@oracle.com: 2381456 Would you mind send me the link of the bug, or the code review request mail? I may miss some mails about this direction. I am afraid I cant sent the link, the Bug is in review state and therefore not visible for me. It was acknowledged 2012-11-12, see attached. I guess the link would be http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=2381456 (not sure if the numbers are the same in the new bug tool). Good suggestion. Oracle provider of JSSE had addressed the TLS renegotiation issue in JDK 1.4.2 update 26, JDK 1.5.0 update 24 and JDK 6u 19 around the end of 2009 and the beginning of 2010. Here is the readme of the fix: http://www.oracle.com/technetwork/java/javase/documentation/tlsreadme2-176330.html. Thats a different problem, I was thinking about preventing
Review Request: JDK-8019227: JDK-8010325 broke the old build
Brent/Alan/Mike, Hashing.java was removed from the JDK workspace, but was not removed from the old java/java/FILES_java.gmk. Things that still depend on the old build (JCE/deploy) are currently broken. http://cr.openjdk.java.net/~wetmore/8019227/webrev.00/ Brad P.S. I'm very aware that we need to move off the old build soon and am getting closer to finally working on it with Erik, and that the old build isn't complete. But it's complete enough for the JCE dependencies. Unfortunately, this isn't a simple transition (JDK-8006350), and this is a quick fix to get the JCE bits built.
Re: Code review request, 8000456: Add programmatic deadlock detection in SSLEngineDeadlock
I enjoy your codereviews because I generally learn about new libraries. :) On 6/18/2013 2:47 AM, Xuelei Fan wrote: Hi, Please review this test update: http://cr.openjdk.java.net/~xuelei/8000456/webrev.00/ The test case SSLEngineDeadlock.java is used to detect the deadlock in SSLEngine. However, the result depends on time out. It is hard to tell whether the timeout is caused by deadlock or heavy loaded systems. Do you want to call: getThreadInfo(long id, int maxDepth) instead? That way you can output the deadlocked location which will help debugging in case this ever shows up again. Brad
Re: Code review request, 7188658 Add possibility to disable client initiated renegotiation
Sorry for the delay. Two comments about the property name. I don't see the jdk.tls *System* Property prefix used anywhere else, except as a *Security* properties: jdk.tls.rejectClientInitializedRenego Otherwise, I think we should stick to one of the current namespace. jsse. (my preference since this is expected across other JDKs) sun.security.ssl. Regarding rejectClientInitializedRenego, I think the word Initiated more succinctly captures the intent of this property, rather than Initalized. And as long as the word Renegotiation is, it should probably be spelled out completely. AFAIK, we rarely abbreviate external names like this. Same initiated comment about the variable name in your codereview, but I don't care much about Renogo. ServerHandshaker.java = 283: My initial thought was a no_renegotiation(100) warning, but that allows the client to decide what to do, rather than the server terminating. This TLS logic decision is not straightforward, so this needs commenting. 379: since you're making changes here. response-respond Since you already have CCC approval and are ready to putback, I would suggest putting back now, and let's file another CCC to change the name. That should be a no-brainer. Thanks! Brad On 5/29/2013 7:05 PM, Xuelei Fan wrote: Got it. Yes, this fix is addressing a different issue from you mentioned below. Thanks, Xuelei On 5/30/2013 9:53 AM, Bernd Eckenfels wrote: Am 30.05.2013, 02:18 Uhr, schrieb Xuelei Fan xuelei@oracle.com: 2381456 Would you mind send me the link of the bug, or the code review request mail? I may miss some mails about this direction. I am afraid I cant sent the link, the Bug is in review state and therefore not visible for me. It was acknowledged 2012-11-12, see attached. I guess the link would be http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=2381456 (not sure if the numbers are the same in the new bug tool). Good suggestion. Oracle provider of JSSE had addressed the TLS renegotiation issue in JDK 1.4.2 update 26, JDK 1.5.0 update 24 and JDK 6u 19 around the end of 2009 and the beginning of 2010. Here is the readme of the fix: http://www.oracle.com/technetwork/java/javase/documentation/tlsreadme2-176330.html. Thats a different problem, I was thinking about preventing execessive client initiated renegotiations. This is for example CVE-2011-1473 from THC. You mentioned industry will move to a secure handshake - are you aware of any initiative in that direction? See http://www.rfc.org/rfc/rfc5746.txt. As far as I know, nearly all major vendors of SSL protocols has support RFC5746. Ok, but thats a different issue. I was expecting 7188658 to address another point, but I might be wrong. I understand that as of Oracle policy we cannot discuss it. Even if this is a very well known issue. :-/ Greetings Bernd
Re: Signature.getAlogrithm return null in special case
Pushed. Thanks for the contribution. Brad On 5/27/2013 12:11 PM, Brad Wetmore wrote: Hi Deven, I just got back from a short break, hope to return back to this shortly. I tweaked your test a bit before I left, but the builds were failing (unrelated issue) so I couldn't check in. Still need to check the actual Signature code for a couple things, but I think it's ok. Brad On 5/21/2013 2:07 AM, Deven You wrote: Hi Brad, Thanks for your reply, is there any progress on this problem? I see the last comments on JDK-8014620 is six days before. Thanks a lot! On 05/15/2013 08:28 AM, Brad Wetmore wrote: Offhand, this seems reasonable. Since you are an OpenJDK author, I've filed: JDK-8014620: Signature.getAlogrithm return null in special case and stocked it with your patch. Thanks, Brad On 5/13/2013 9:46 PM, Deven You wrote: Hi All, I find in a special case: If you create a SignatureSpi service through extending Signature rather than SignatureSpi, the returned signature instance will lose its algortithm name. Though the fix[1] is simple I think it's valuable. Could anyone take a look? [1] http://cr.openjdk.java.net/~youdwei/ojdk-809/webrev/ http://cr.openjdk.java.net/%7Eyoudwei/ojdk-809/webrev/ Thanks a lot
Re: Signature.getAlogrithm return null in special case
On 6/17/2013 5:38 PM, Brad Wetmore wrote: Pushed. Thanks for the contribution. P.S. http://hg.openjdk.java.net/jdk8/tl/jdk/rev/116050227ee9 Brad Brad On 5/27/2013 12:11 PM, Brad Wetmore wrote: Hi Deven, I just got back from a short break, hope to return back to this shortly. I tweaked your test a bit before I left, but the builds were failing (unrelated issue) so I couldn't check in. Still need to check the actual Signature code for a couple things, but I think it's ok. Brad On 5/21/2013 2:07 AM, Deven You wrote: Hi Brad, Thanks for your reply, is there any progress on this problem? I see the last comments on JDK-8014620 is six days before. Thanks a lot! On 05/15/2013 08:28 AM, Brad Wetmore wrote: Offhand, this seems reasonable. Since you are an OpenJDK author, I've filed: JDK-8014620: Signature.getAlogrithm return null in special case and stocked it with your patch. Thanks, Brad On 5/13/2013 9:46 PM, Deven You wrote: Hi All, I find in a special case: If you create a SignatureSpi service through extending Signature rather than SignatureSpi, the returned signature instance will lose its algortithm name. Though the fix[1] is simple I think it's valuable. Could anyone take a look? [1] http://cr.openjdk.java.net/~youdwei/ojdk-809/webrev/ http://cr.openjdk.java.net/%7Eyoudwei/ojdk-809/webrev/ Thanks a lot
Re: RFR JDK8007636
Ditto. brad On 6/17/2013 5:16 PM, Xuelei Fan wrote: Looks fine to me. Thanks, Xuelei On 6/17/2013 10:29 PM, John Zavgren wrote: Greetings: I'm posting a fix for a memory leak. As you can see, the original code deallocated a structure, thereby rendering it's memory invalid, then it deallocated the memory that was allocated to one of its data members. I merely reversed the order of the free() operations. http://cr.openjdk.java.net/~jzavgren/8007636/webrev.01/ http://cr.openjdk.java.net/%7Ejzavgren/8007636/webrev.01/ Thanks! John -- John Zavgren john.zavg...@oracle.com 603-821-0904 US-Burlington-MA
Re: Code review: 8001284 8012971
Tony, NOTE: the diffs in webrev are hiding the change in indention for the if (found) change. Don't know why webrev is set this way, but looking at the non-diff links shows the proper indention. FYI: webrev's -b option will cause webrev to not ignore changes in the amount of white space. I'm guessing the concern is more for the actual code than whitespace, and listing whitespace changes just adds useless noise/overhead to the review. (But I do notice that kind of stuff! ;)) Brad On 04/22/13 14:18, Anthony Scarpino wrote: This code review is for the following two bugs: - 8001284 Buffer problems with SunPKCS11-Solaris and CKM_AES_CTR - 8012971 PKCS11Test hiding exception failures The first fix is simple as it prevents the method from enforcing AES block lengths on CTR, a stream cipher. The second fix is a test problem that is suppressing test failures. NOTE: the diffs in webrev are hiding the change in indention for the if (found) change. Don't know why webrev is set this way, but looking at the non-diff links shows the proper indention. http://cr.openjdk.java.net/~ascarpino/8001284/webrev.00/ thanks Tony
Re: Signature.getAlogrithm return null in special case
Hi Deven, I just got back from a short break, hope to return back to this shortly. I tweaked your test a bit before I left, but the builds were failing (unrelated issue) so I couldn't check in. Still need to check the actual Signature code for a couple things, but I think it's ok. Brad On 5/21/2013 2:07 AM, Deven You wrote: Hi Brad, Thanks for your reply, is there any progress on this problem? I see the last comments on JDK-8014620 is six days before. Thanks a lot! On 05/15/2013 08:28 AM, Brad Wetmore wrote: Offhand, this seems reasonable. Since you are an OpenJDK author, I've filed: JDK-8014620: Signature.getAlogrithm return null in special case and stocked it with your patch. Thanks, Brad On 5/13/2013 9:46 PM, Deven You wrote: Hi All, I find in a special case: If you create a SignatureSpi service through extending Signature rather than SignatureSpi, the returned signature instance will lose its algortithm name. Though the fix[1] is simple I think it's valuable. Could anyone take a look? [1] http://cr.openjdk.java.net/~youdwei/ojdk-809/webrev/ http://cr.openjdk.java.net/%7Eyoudwei/ojdk-809/webrev/ Thanks a lot
Re: Signature.getAlogrithm return null in special case
Offhand, this seems reasonable. Since you are an OpenJDK author, I've filed: JDK-8014620: Signature.getAlogrithm return null in special case and stocked it with your patch. Thanks, Brad On 5/13/2013 9:46 PM, Deven You wrote: Hi All, I find in a special case: If you create a SignatureSpi service through extending Signature rather than SignatureSpi, the returned signature instance will lose its algortithm name. Though the fix[1] is simple I think it's valuable. Could anyone take a look? [1] http://cr.openjdk.java.net/~youdwei/ojdk-809/webrev/ http://cr.openjdk.java.net/%7Eyoudwei/ojdk-809/webrev/ Thanks a lot
Reviewed: JDK-8012530 : test/sun/security/provider/SecureRandom/StrongSeedReader.java failing
This is a simple fix that Alan Bateman noticed during our nightly testing. On some Linux, the java.io.tmp directory does not include the trailing /, but does on others OS. Apparently, on my JPRT run, whatever Linux machine this ran on did include the slash, or else I was asleep at the switch. Anyway, I've reviewed and am integrating this on his behalf. http://cr.openjdk.java.net/~wetmore/8012530/webrev.00/ Thanks for pointing this out, Alan. Brad
Re: Update #5: JEP 123: SecureRandom Draft and Implementation.
On 4/11/2013 3:12 AM, Florian Weimer wrote:
On 04/11/2013 12:07 AM, Brad Wetmore wrote:
Hi Florian,
I wonder if this change to src/share/lib/security/java.security-linux
-securerandom.source=file:/dev/urandom
+securerandom.source=file:/dev/random
causes the return of the blocking behavior.
Welcome to my can of worms. [1] I hope everything I've said below is
correct, and haven't made any typos!
Yay for crypto pluggability. 8-/
If you were around pre-JDK 1.3.1_09/1.4, the SecureRandom performance is
much better compared to ThreadedSeedGenerator.
Rather than trying to find out what the code does, I tested your changes
on Fedora 18, and here is what I found:
new SecureRandom() does not block for seeding. It reads straight from
/dev/urandom, so it may have some impact on the kernel entropy pool.
I'm assuming you're using the default configuration, and using
nextBytes() and not generateSeed()? Then NativePRNG does read from
/dev/urandom.
The Linux entropy pool has been a bit of a black box for me. The latest
I've read on it was Analysis of the Linux Random Number Generator by
Gutterman, et.al. in 2006. From what I understand, reads of
/dev/urandom will indeed pull from the primary pool. But if that's
followed by a large of /dev/random, there's nothing in the primary pool
to refill it.
Maybe you would know the answer to this question. It used to be that
the Linux entropy pools was refilled by keyboard/mouse/interrupts/disk
activity. On a lights-out/headless system, the first two didn't
contribute. There were not many kernel modules that fed the interrupts,
so most of the entropy came from disk. I know some vendors are adding
network info, but from what I understand, that's not standard. Have
there been changes in more recent versions of Linux?
SecureRandom.getInstance(SHA1PRNG) may block for seeding, but only
once during startup. After that, it does not obtain entropy from the
kernel.
Correct. I omitted mentioning the details for fear of causing even more
confusion, but since you hinted at it: there is a system-wide seeder for
initializing future SHA1PRNGs
(sun.security.provider.SecureRandom$SeederHolder) that is itself a
SHA1PRNG and needs to be seeded via the system entropy and the
Native/URL/ThreadedSeedGenerator. Once that has been seeded (via
/dev/random by default), it generates seeds (using the
SHA1PRNG.nextBytes()) for future SHA1PRNGs (without going through
/dev/random). However, calls to SecureRandom.generateSeed() will still
always go to the Native/URL/ThreadedSeedGenerator.
When doing this project, there were many times where I said: Looks
like I picked a bad week to... [1]
This matches the behavior of OpenJDK 7 in that Fedora version.
However, I noticed that /dev/{u,}random is opened three times each (and
the file descriptors are kept open).
What was your application code? Any configuration I should be aware of?
Oops, just realized there's an optimization in NativePRNG/friends. If
the seedFile/nextfile are both pointing to the same file, we don't need
to open it twice. I've filed bug JDK-8012042. Might be able to
optimize it further.
Brad
[1] This quote is from a classic American comedy movie called
Airplane. http://www.youtube.com/watch?v=VmW-ScmGRMA
Re: Code review request, re-integrate JEP 115 into JDK 8
On 4/8/2013 7:24 AM, Xuelei Fan wrote: Hi, Because of conflicts, we backed out the implementation of JEP 115. This fix resolves the conflicts and re-integrates JEP 115 into JDK 8. The merge is straightforward except the updates in MAC.java, CipherBox.java, EngineInputRecord.java and InputRecord.java. As it looks more like a changeset merge, it's OK to me if you only want to review the above 4 files. old webrev: http://cr.openjdk.java.net./~xuelei/7030966/webrev.03/ new webrev: http://cr.openjdk.java.net./~xuelei/8011680/webrev.00/ There was a minor comment about a comment I made privately to Xuelei, but not important enough to mention here. I didn't notice anything else. Crossing fingers! ;) Brad
Re: Why cannot overwrite a KeyEntry with a TrustCertEntry?
On 4/11/2013 7:47 AM, Sean Mullan wrote: On 04/11/2013 04:36 AM, Weijun Wang wrote: Hi All The KeyStore::setCertificateEntry has * @exception KeyStoreException if the keystore has not been initialized, * or the given alias already exists and does not identify an * entry containing a trusted certificate, * or this operation fails for some other reason. which means you cannot overwrite a KeyEntry with a TrustCertEntry. While setKeyEntry allows a TrustCertEntry been overwritten by a KeyEntry. This has been true from the beginning, but why? I'm not sure, but the exact reason is probably now lost in the sands of time ;) On the other hand, setEntry mentions no restriction, although the current implementations (jks, pkcs12) fail when overwriting a KeyEntry with a TrustCertEntry. The only thing I can think of is that it protects against accidental overwriting of your private key, which might be a good thing, if you haven't backed it up. That was added in April 1998. 4129553: KeyStore should store any type of Key, not just PrivateKey I *THINK* what Sean states was the reason, but before my time. Brad
Update #5: JEP 123: SecureRandom Draft and Implementation.
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
Re: Update #5: JEP 123: SecureRandom Draft and Implementation.
Hi Florian, I wonder if this change to src/share/lib/security/java.security-linux -securerandom.source=file:/dev/urandom +securerandom.source=file:/dev/random causes the return of the blocking behavior. Welcome to my can of worms. [1] I hope everything I've said below is correct, and haven't made any typos! In the past, I saw /dev/random-related blocking during server start-up because too many SecureRandom instances needed seeding. If I follow the code correctly, If you came to this code fresh and understood it all in 7 hours, I would either give you a medal, or hand in my badge. :) It's really twisted stuff. seeding of non-strong generators now uses /dev/random again, which is subject to blocking. Given the long history of the JDK and concerns about backwards compatibility, our SecureRandom implementations have become *REALLY* hard to understand. Trying to make sense of all this, and now cleaning it up, was actually more than 1/2 of the project. It took me a couple weeks of intense code scrutiny/archeology, and then needed to write a wiki page just to keep my head wrapped around it. Likely the most confusing issue I've ever worked on. In a nutshell, the current implementation of SHA1PRNG does indeed read from /dev/random while seeding itself and supplying bytes via generateSeed(), which is in opposition to the documented behavior of the security properties: Remember the infamous file:/dev/./urandom workaround for SHA1PRNG? [2] (sigh...) I'll try to simplify as much as I can. I'll be including parts of this for the JDK 8 Oracle's Provider Impl page. In the current environment, here's our current list of providers, in default preference order. Solaris: PKCS11/NativePRNG/SHA1PRNG Linux/Mac: NativePRNG/SHA1PRNG Windows: SHA1PRNG/Windows-PRNG As you probably know, new SecureRandom() obtains the first implementation in the list, unless you call SecureRandom.getInstance(alg) or SecureRandom.getInstance(alg, provider), which gets a specific algorithm, either most preferred version or by a specific provider, respectively. The securerandom.source/java.security.egd currently only affects SHA1PRNG, but there's a slight wrinkle in that certain property values promote NativePRNG ahead of SHA1PRNG in the list of algorithms in the SUN provider. More on this later. Current behavior: = NativePRNG: --- does not use securerandom.source/java.security.egd at all engineGenerateSeed: always reads from /dev/random engineNextBytes:always reads from /dev/urandom SHA1PRNG: - Since this is a real PRNG [3], SHA1PRNG requires a very strong seed (initial) value, as strong as possible. If not initially seeded by the user via setSeed, SHA1PRNG will call SeedGenerator.getSystemEntropy + SeedGenerator.generateSeed() to generate a strong seed for itself. There are three seed generators which have SeedGenerator as their parent: ThreadedSeedGenerator was the original Seeder which uses system load to generate seed values, and is only used as a last resort. (SLOW!) URLSeedGenerator takes a URL and reads from it. NativeSeedGenerator uses /dev/random. When the SeedGenerator class initializes, if the properties (securerandom.source/java.security.egd) resolve to either the exact strings file:/dev/random or file:/dev/urandom, it will use NativeSeedGenerator (/dev/random). (Yeah, try explaining that to customers!) If the properties resolve to any other URL, then we use URLSeedGenerator with that URL. Note, this is why the infamous workaround file:///dev/urandom or file:/dev/./urandom works. [2] As someone once said, this has got to be the most confusing workaround in the history of the JDK. I would agree! :) If neither SeedGenerator is available, then we use ThreadedSeedGenerator as the last resort. Once seeded, then later calls to this SHA1PRNG SecureRandom instance are: engineGenerateSeed: always reads from SeedGenerator engineNextBytes:Number generation using the current state as input into SHA1. So even though the EGD properties point to file:/dev/urandom, it's not correct when it comes to the seeder. Back in JDK 5, adding NativePRNG was the workaround for this problem. When setting up the default order of implementations in the SUN provider, if the properties URL is file:/dev/urandom, then NativePRNG becomes the most preferred provider (over SHA1PRNG). So for those Linux implementations which just asked for the most preferred (i.e. new SecureRandom()), they get an instance of NativePRNG which uses /dev/urandom for nextBytes. (Note well: NativeSeedGenerator.generateSeed() still reads from /dev/random). So since you're not seeding a SHA1PRNG, using NativePRNG.nextBytes() makes it appear as though there is no more stalling, as long as you only call nextBytes() and not generateSeed(). But if you specifically asked for SHA1PRNG (which a lot of
Update #6: JEP 123: SecureRandom Draft and Implementation.
Fixed some stupidness in the tests. http://cr.openjdk.java.net/~wetmore/6425477/webrev.06/ Also, ignore the make/sun/splashscreen change. That will be fixed elsewhere, but I needed it to be able to build using the old system. Brad On 4/10/2013 3:07 PM, Brad Wetmore wrote: Hi Florian, I wonder if this change to src/share/lib/security/java.security-linux -securerandom.source=file:/dev/urandom +securerandom.source=file:/dev/random causes the return of the blocking behavior. Welcome to my can of worms. [1] I hope everything I've said below is correct, and haven't made any typos! In the past, I saw /dev/random-related blocking during server start-up because too many SecureRandom instances needed seeding. If I follow the code correctly, If you came to this code fresh and understood it all in 7 hours, I would either give you a medal, or hand in my badge. :) It's really twisted stuff. seeding of non-strong generators now uses /dev/random again, which is subject to blocking. Given the long history of the JDK and concerns about backwards compatibility, our SecureRandom implementations have become *REALLY* hard to understand. Trying to make sense of all this, and now cleaning it up, was actually more than 1/2 of the project. It took me a couple weeks of intense code scrutiny/archeology, and then needed to write a wiki page just to keep my head wrapped around it. Likely the most confusing issue I've ever worked on. In a nutshell, the current implementation of SHA1PRNG does indeed read from /dev/random while seeding itself and supplying bytes via generateSeed(), which is in opposition to the documented behavior of the security properties: Remember the infamous file:/dev/./urandom workaround for SHA1PRNG? [2] (sigh...) I'll try to simplify as much as I can. I'll be including parts of this for the JDK 8 Oracle's Provider Impl page. In the current environment, here's our current list of providers, in default preference order. Solaris: PKCS11/NativePRNG/SHA1PRNG Linux/Mac: NativePRNG/SHA1PRNG Windows: SHA1PRNG/Windows-PRNG As you probably know, new SecureRandom() obtains the first implementation in the list, unless you call SecureRandom.getInstance(alg) or SecureRandom.getInstance(alg, provider), which gets a specific algorithm, either most preferred version or by a specific provider, respectively. The securerandom.source/java.security.egd currently only affects SHA1PRNG, but there's a slight wrinkle in that certain property values promote NativePRNG ahead of SHA1PRNG in the list of algorithms in the SUN provider. More on this later. Current behavior: = NativePRNG: --- does not use securerandom.source/java.security.egd at all engineGenerateSeed: always reads from /dev/random engineNextBytes:always reads from /dev/urandom SHA1PRNG: - Since this is a real PRNG [3], SHA1PRNG requires a very strong seed (initial) value, as strong as possible. If not initially seeded by the user via setSeed, SHA1PRNG will call SeedGenerator.getSystemEntropy + SeedGenerator.generateSeed() to generate a strong seed for itself. There are three seed generators which have SeedGenerator as their parent: ThreadedSeedGenerator was the original Seeder which uses system load to generate seed values, and is only used as a last resort. (SLOW!) URLSeedGenerator takes a URL and reads from it. NativeSeedGenerator uses /dev/random. When the SeedGenerator class initializes, if the properties (securerandom.source/java.security.egd) resolve to either the exact strings file:/dev/random or file:/dev/urandom, it will use NativeSeedGenerator (/dev/random). (Yeah, try explaining that to customers!) If the properties resolve to any other URL, then we use URLSeedGenerator with that URL. Note, this is why the infamous workaround file:///dev/urandom or file:/dev/./urandom works. [2] As someone once said, this has got to be the most confusing workaround in the history of the JDK. I would agree! :) If neither SeedGenerator is available, then we use ThreadedSeedGenerator as the last resort. Once seeded, then later calls to this SHA1PRNG SecureRandom instance are: engineGenerateSeed: always reads from SeedGenerator engineNextBytes:Number generation using the current state as input into SHA1. So even though the EGD properties point to file:/dev/urandom, it's not correct when it comes to the seeder. Back in JDK 5, adding NativePRNG was the workaround for this problem. When setting up the default order of implementations in the SUN provider, if the properties URL is file:/dev/urandom, then NativePRNG becomes the most preferred provider (over SHA1PRNG). So for those Linux implementations which just asked for the most preferred (i.e. new SecureRandom()), they get an instance of NativePRNG which uses /dev/urandom for nextBytes. (Note well: NativeSeedGenerator.generateSeed() still reads from /dev/random). So
Re: Debuggability of failures in sun.security.rsa.RSASignature
Hi Matthew,
I've just taken a quick look, but yes, this seems to be a usability
issue that should somehow be addressed, either by adding some
logging/debugging or throwing a SignatureException. There's currently
no logging/debugging in this package.
We'd need to figure out why the original author made the original
decision to swallow the exception.
I've filed:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8011740
to track this.
Brad
On 4/1/2013 6:49 PM, Matthew Hall wrote:
Hi,
This code in RSASignature catches javax.crypto.BadPaddingException without
logging it, and some of the functions in try { ... } have detailed exceptions
which get lost when this exception is not properly propagated to calling code.
At minimum there should be a security logging debug flag which enables logging
the exceptions instead of silently suppressing them, otherwise it's impossible
to troubleshoot or even detect that issued were encountered here without using
a debugger on it.
Thoughts?
Matthew.
// verify the data and return the result. See JCA doc
protected boolean engineVerify(byte[] sigBytes) throws SignatureException {
byte[] digest = getDigestValue();
try {
byte[] decrypted = RSACore.rsa(sigBytes, publicKey);
byte[] unpadded = padding.unpad(decrypted);
byte[] decodedDigest = decodeSignature(digestOID, unpadded);
return Arrays.equals(digest, decodedDigest);
} catch (javax.crypto.BadPaddingException e) {
// occurs if the app has used the wrong RSA public key
// or if sigBytes is invalid
// return false rather than propagating the exception for
// compatibility/ease of use
return false; *** PROBLEM LINE ***
Re: code review request: 7171982 Cipher getParameters() throws RuntimeException: Cannot find SunJCE provider
(Vinnie, what do you think about the SunJCE item below?)
On 3/22/2013 11:57 AM, Anthony Scarpino wrote:
Hi all,
I need a code review for below webrev. The changes are to have SunJCE
call itself, using it's current instance, for checking such things as
parameters, instead of searching through the provider list or creating a
one time instance.
http://cr.openjdk.java.net/~mullan/webrevs/ascarpin/webrev.00/
PBES1Core.java
==
173: indention problem. Should be at the same level as (algo...)
PBES2Core.java:173
PKCS12PBECipherCore.java:147
SealedObjectForKeyProtector:50/57
Indention problem. Normally 4 spaces unless you're trying to line it up
with something.
SealedObjectForKeyProtector.java
54/57: In general, you should initCause() everywhere you possibly can.
This will help people (us) debug the real underlying root cause,
instead of just the top-level error message.
SunJCE.java
===
781: Your code could race during initialization and potentially have
many SunJCE instances active at once.
Either make instance a volatile (will reduce some of the race
opportunity), or instead, add locking around assignment/use. You may
still be creating multiple SunJCEs, but only one instance will ever be
obtained from getInstance:
synchronized (SunJCE.class) {
if (instance == null) {
instance = this;
}
}
and
static SunJCE getInstance() {
if (instance == null) {
new SunJCE();
}
synchronized (SunJCE.class) {
return instance;
}
}
Also, when you get ready to push, be sure to address also the closed
side: that is, please remember to build/integrate the signed
sunjce_provider.jar file in the closed repo.
HTH,
Brad
Re: simple code review request: 8001596
Just realized, there are no regression tests here. Simplest is to probably do as much setup as you can, then java.security.Security.removeProvider(SunJCE), then issue the calls that call into these changes. They should all pass in the new version, and fail in the old. Brad On 3/28/2013 2:29 PM, Anthony Scarpino wrote: Hi all, I have a very simple code review request. It's a typo bug. 8001596 Incorrect condition check in PBKDF2KeyImpl.JAVA https://jbs.oracle.com/bugs/browse/JDK-8001596 http://cr.openjdk.java.net/~mullan/webrevs/ascarpin/8001596/webrev.01/ thanks Tony
Re: code review request: 7171982 Cipher getParameters() throws RuntimeException: Cannot find SunJCE provider
(Whoops, was working on two reviews with two related comments, and
reversed the emails).
Just realized, there are no regression tests here.
Simplest is to probably do as much setup as you can, then
java.security.Security.removeProvider(SunJCE), then issue the calls
that call into these changes. They should all pass in the new version,
and fail in the old.
Brad
On 3/28/2013 2:34 PM, Brad Wetmore wrote:
(Vinnie, what do you think about the SunJCE item below?)
On 3/22/2013 11:57 AM, Anthony Scarpino wrote:
Hi all,
I need a code review for below webrev. The changes are to have SunJCE
call itself, using it's current instance, for checking such things as
parameters, instead of searching through the provider list or creating a
one time instance.
http://cr.openjdk.java.net/~mullan/webrevs/ascarpin/webrev.00/
PBES1Core.java
==
173: indention problem. Should be at the same level as (algo...)
PBES2Core.java:173
PKCS12PBECipherCore.java:147
SealedObjectForKeyProtector:50/57
Indention problem. Normally 4 spaces unless you're trying to line it up
with something.
SealedObjectForKeyProtector.java
54/57: In general, you should initCause() everywhere you possibly can.
This will help people (us) debug the real underlying root cause,
instead of just the top-level error message.
SunJCE.java
===
781: Your code could race during initialization and potentially have
many SunJCE instances active at once.
Either make instance a volatile (will reduce some of the race
opportunity), or instead, add locking around assignment/use. You may
still be creating multiple SunJCEs, but only one instance will ever be
obtained from getInstance:
synchronized (SunJCE.class) {
if (instance == null) {
instance = this;
}
}
and
static SunJCE getInstance() {
if (instance == null) {
new SunJCE();
}
synchronized (SunJCE.class) {
return instance;
}
}
Also, when you get ready to push, be sure to address also the closed
side: that is, please remember to build/integrate the signed
sunjce_provider.jar file in the closed repo.
HTH,
Brad
Re: simple code review request: 8001596
There is no regression test. I created one which relies on reflection, which is one way to test this problem. Feel free to create another, but that one is ready to go. Please see the attachment in the bug, and you'll probably want to update the copyright date. Brad On 3/28/2013 2:29 PM, Anthony Scarpino wrote: Hi all, I have a very simple code review request. It's a typo bug. 8001596 Incorrect condition check in PBKDF2KeyImpl.JAVA https://jbs.oracle.com/bugs/browse/JDK-8001596 http://cr.openjdk.java.net/~mullan/webrevs/ascarpin/8001596/webrev.01/ thanks Tony
Re: simple code review request: 8001596
Minor typos that don't affect program execution (comments/javadoc) are ok to not do unit tests, but even through this is a typo, I think this still needs a test. Brad On 3/28/2013 2:51 PM, Anthony Scarpino wrote: I had left the regression test out of this as it was a typo rather than a code logic issue or something someone could rebroken. Are you ok if this goes back without a test? Tony On 03/28/2013 02:46 PM, Brad Wetmore wrote: There is no regression test. I created one which relies on reflection, which is one way to test this problem. Feel free to create another, but that one is ready to go. Please see the attachment in the bug, and you'll probably want to update the copyright date. Brad On 3/28/2013 2:29 PM, Anthony Scarpino wrote: Hi all, I have a very simple code review request. It's a typo bug. 8001596 Incorrect condition check in PBKDF2KeyImpl.JAVA https://jbs.oracle.com/bugs/browse/JDK-8001596 http://cr.openjdk.java.net/~mullan/webrevs/ascarpin/8001596/webrev.01/ thanks Tony
Re: Next Protocol Negotiation TLS Extension
On 3/25/2013 6:59 AM, Simone Bordet wrote: Hi, On Fri, Mar 22, 2013 at 1:08 AM, Brad Wetmore bradford.wetm...@oracle.com wrote: Hi Simone, I haven't looked at the proposal yet, but just from a scheduling point of view, unfortunately we're finishing up the implementation of the last of the planned features for JDK 8, so getting this into 8 is likely not possible. Likely not possible means totally impossible or if you do this and that, then it's possible ? :) I would never say totally impossible, but closer to totally impossible than likely not possible. Do you know the status of JEP 114 ? Will it be shipped in JDK 8 ? That is still the expectation. We had to pull 114 temporarily because of a conflict in two projects, and we'll be working on resolving the merge when the engineer is back from vacation. http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8009925 We have an open bug for this (JDK-8007785) and it's on the radar for JDK 9. Ok, if impossible I will eventually pop up again when JDK 8 is released and JDK 9 work started. Planning for JDK 9's features will start well before that. Some of the dev work will already be underway by release. Brad Thanks ! -- Simone Bordet http://bordet.blogspot.com --- Finally, no matter how good the architecture and design are, to deliver bug-free software with optimal performance and reliability, the implementation technique must be flawless. Victoria Livschitz
Re: Next Protocol Negotiation TLS Extension
Hi Simone, I haven't looked at the proposal yet, but just from a scheduling point of view, unfortunately we're finishing up the implementation of the last of the planned features for JDK 8, so getting this into 8 is likely not possible. We have an open bug for this (JDK-8007785) and it's on the radar for JDK 9. I'll put in a link to your email in that bug. I know Xuelei will want to look at this more closely, and I hope to also. Brad On 3/21/2013 9:01 AM, Simone Bordet wrote: Hi all, [cross-posted to jdk8-dev and security-dev] I am a member of the Jetty servlet container (http://eclipse.org/jetty) team and the implementor of the Next Protocol Negotiation (NPN) TLS Extension used by Jetty to support the SPDY protocol (API at http://git.eclipse.org/c/jetty/org.eclipse.jetty.npn.git/ and implementation at https://github.com/jetty-project/jetty-npn). The SPDY protocol has been chosen as the basis for HTTP 2.0. I would like to ask for suggestions for what would be the best way to have NPN support in OpenJDK 8 rather than via the Jetty NPN implementation. Currently, the Jetty implementation is kind of hacky in that it is the smallest possible hack (in a positive meaning) to make NPN work in OpenJDK. It modifies 5 sun.security.ssl.* classes and introduces 5 new classes. These modifies classes must be put in the bootclasspath. The API of public classes like SSLEngine is not modified; instead the current implementation relies on a static class that maps SSLEngine (or SSLSocket) with application code that is invoked at the right time during the TLS handshake when NPN data is detected. Currently, the Jetty project maintains the NPN implementation locked with OpenJDK releases: every time the sun.security.ssl.* classes are modified, we pull in the changes from OpenJDK, re-patch these classes with NPN support and make a new release of the NPN jar. The NPN TLS extension requires an API exposed to applications (usually web servers, but they are applications for the Java runtime). In this sense, JEP 114 (http://openjdk.java.net/jeps/114, SNI TLS extension) is similar: I am guessing it also has to expose an API to applications. It seems to me that both NPN and SNI would require a standard way to access TLS extensions at the proper time during the TLS handshake. In light of this, it would be great if NPN could be piggybacked on JEP 114, exposing a standard TLS extensions API provided by OpenJDK that application can use to plug in their code for NPN and/or SNI. Now, I understand that designing a TLS extensions API is not as simple as including the current Jetty NPN implementation in OpenJDK, but I would rather see a generic solution in OpenJDK rather than a hacky solution like current Jetty NPN's included in OpenJDK. A private TLS extensions API already exists in the sun.security.ssl.* classes, but it's mostly package private and of course under sun.* packages. So perhaps the work to be done is not a from-scratch effort. I would like to get a discussion started on how NPN can be supported in OpenJDK 8. Ideally, my best plan would be: * NPN included in JEP 114. * JEP 114 designing a standard TLS extensions API that can serve for both NPN and SNI (and, generically, others TLS extensions) * JEP 114 shipped in OpenJDK 8. We're happy to keep Jetty NPN up-to-date for OpenJDK 7 releases, but we will really like to see NPN in OpenJDK 8. We are open to comply with any legal papers that needs to be in place for this contribution. I will be more than happy to provide detailed information about the implementation of Jetty NPN and have it discussed (or even reviewed) by security experts. Thanks ! -- Simone Bordet http://bordet.blogspot.com --- Finally, no matter how good the architecture and design are, to deliver bug-free software with optimal performance and reliability, the implementation technique must be flawless. Victoria Livschitz
Re: Allow configure to detect if EC implementation is present
CC'ing security-dev. Vinnie, As owner of ECC, you should probably look at this. Brad On 3/13/2013 7:02 PM, David Holmes wrote: On 14/03/2013 6:09 AM, Omair Majid wrote: Hi, jdk/makefiles/CompileNativeLibraries.gmk has a little note: TODO Set DISABLE_INTREE_EC in configure if src/share/native/sun/security/ec/impl is not present The webrev at http://cr.openjdk.java.net/~omajid/webrevs/intree-ec/00/ implements this. Does this look okay for jdk8/build ? Can I get a bug id? Bug ID: 8010030 I think it is more consistent to set the variable to yes/no and change: ifndef DISABLE_INTREE_EC to ifeq ($DISABLE_INTREE_EC), yes) Thanks, David Thanks, Omair
RFR 8009925: Back out AEAD CipherSuites temporarily
Valerie, Here is the codereview for the merge problem with the AEAD ciphersuites I told you about. The safest way to resolve is to pull the JSSE portion of AEAD until Xuelei is back to fix the merge problem. It will be easier to do a straight comparison of the pre-AEAD Ciphersuite directories/files than read the webrevs. More information about that in a separate email. But here's the webrev FYI: http://cr.openjdk.java.net/~wetmore/8009925/webrev.00/ The original changeset: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/def2e05299b7 changeset: 6635:def2e05299b7 user:xuelei date:Fri Mar 01 02:34:34 2013 -0800 summary: 7030966: Support AEAD CipherSuites Here's the previous changeset: changeset: 6634:7246a6e4dd5a user:juh date:Thu Feb 28 16:36:01 2013 -0800 summary: 8006853: OCSP timeout set to wrong value if \ com.sun.security.ocsp.timeout 0 I would suggest to clone/update to 6634:7246a6e4dd5a, and compare the files and file contents. I'll send you the closed information in a separate email. Thanks, Brad
Re: PKCS11 support on 64bit sun java
Are you talking about windows-64 bit? All the other platforms should have 64-bit support. JDK 8 does have windows-x64 support for PKCS11. http://openjdk.java.net/jeps/131 I don't know of any plans to backport. Brad On 3/6/2013 9:16 PM, mohankumar kanaka wrote: Hi, This is Mohankumar working based in Hyderabad India working for MNC Recently i have worked on PKCS11 support for 64 bit Sun/oracle java i found the SUNPKCS11.jar and j2pkcs11.dll are not present in jdk and which are core elements for PKCS11 support due to absence of this on*64bit java* we are unable work with tokens and HSMs So i tried to build sunpkcs11.jar and j2pkcs11.dll 1. Downloaded open jdk source code jdk version 6 2. figured out the classes for j2pkcs11.dll and sunpkcs11.jar respectively. 3. I was able to build the j2pkcs11.dll and sunpkcs11.jar and also tested primarily with safenet ikey 2032 token and i was successfully get provider by using sunpkcs11 class and also able to get the key store. But i do not know the where to submit the solution so that it would be helpful for others. Have a Great Day!! Regards Mohankumar kanaka
Re: code review request: 8009604, old make images failed: JarBASE64Encoder class not found
Looks good. brad On 3/6/2013 6:33 PM, Weijun Wang wrote: Please review the fix at http://cr.openjdk.java.net/~weijun/8009604/webrev.00/ The class was defined in jarsigner/Main.java and was removed in JDK-8006182, but it's still mentioned in jdk/make/common/Releases.gmk. Noreg-build. Thanks Max
Re: Code Review Request for 7030966, Support AEAD CipherSuites (JSSE part of JEP 115)
I've pulled out things that need no further discussion.
I will be doing a putback of the JCE providers, so I can do your SunJCE
signed provider putback for you. Let's coordinate when you are ready.
I will probably be ready early next week.
I will likely be putting back on Monday, maybe Tuesday. I'll try to
coordinate with you over the weekend.
CipherBox.java
==
312/564: I don't remember why we decided to use the RuntimeException
AIOOBE here. Ugh...anyway, can you also use exception chaining to help
debug any reported problems?
We don't have chaining constructor for ArrayIndexOutOfBoundsException class.
Well, that explains it. :) I wonder if there is already a RFE for this?
CipherSuite.java
510: I don't think this statement is true anymore. This is likley a
carryover from the 1.4 days when we used to have a crypto provider in
the JSSE jar file. Not for this review, but maybe we should actually
check that implementations are available? If we remove SunEC and
SunPKCS11 providers from the provider list, could we potentially
disable the EC suites?
Good catch.
EC algorithm will be checked in KeyExchange, rather than BulkCipher. If
EC algorithm is not available, EC cipher suites will be disabled.
I worry about the RC4 cipher. The AES/CBC/128 is the minimal
requirements of a crypto provider, so I don't worry about AES/CBC/128.
However, RC4 is not in the minimal requirements of a crypto provider, so
we should check the availability of RC4 here.
I will file a new for this issue.
Thanks.
522: Why are you also checking for CipherType.AEAD_CIPHER?
AEAD/GCM is not implemented in all providers, for example the SunPKCS11
provider. So we need to check the available of AEAD implementation.
Of course, now I see it. The extra check for AES_256_GCM threw me. If
you're also checking for AEAD, you probably don't need the separate
check for AES_256_GCM. Maybe:
if (this == B_AES_256 ||
(this.cipherType == CipherType.AEAD_CIPHER)) {
is sufficient?
970: before - while
It looks strange to me with two whiles:
// ..., we decrease the
// priority of cipher suites in GCM mode for a while while GCM
// technologies become mature in the industry.
I think before may be better word here.
Sorry, definitely not two whiles. I think I probably meant to say as
but obviously didn't.
Maybe:
// Placeholder for cipher suites in GCM mode.
//
// For better compatibility and interoperability, we decrease the
// priority of cipher suites in GCM mode for a while as GCM
// technologies mature in the industry. Eventually we'll move
// the GCM suites here.
EngineOutputRecord.java
===
294/296: Another great comment. I might suggest reversing the
comments so that the comment about AEAD is in the AEAD arm, and CBC is
outside.
I'm not sure I catch your ideas. ;-) Would you please show me the code?
Just a simple reversal of the lines so that the code you're talking
about is contained in the block that handles it:
if (!writeCipher.isAEADMode()) {
// DON'T encrypt the nonce_explicit for AEAD mode
dstBB.position(dstPos + headerSize);
} // The explicit IV in TLS 1.1 and later can be encrypted.
Hope that's clearer.
306: The original code was bad (double debug != null :) ), and I
realize the original code was lacking in parens (), but can you
please add parens to indicate exactly what expression order you intend
here. My head is spinning from parsing the various cases: I'm not sure
the logic here is correct. I think we should output if debug is on
and either handshake/record is active or we're outputing a CCS. That
is:
if ((debug ! null)
((Debug.isOn(record) || (Debug.isOn(handshake) ||
(contentType() == ct_change_cipher_spec) {
Is my thinking incorrect?
In the old code, there is no dump for handshake level log unless it is
a change_cipher_spec message. However, the log is dumped for record
level log. I think it was right because the log here is record
information, but not handshake message.
I think the update does not changes the behavior.
You are correct, the actual update does not change the observed
behavior, but I had to write an app to prove it to myself.
However, this code is not following the failfast paradigm of Debug,
though it does give the same answer because of the way the Debug.isOn()
code was written. debug!=null is supposed to be a lightweight check to
see if any Debug options are on, and if so, then do the more heavyweight
checks.
Abstracting a bit, test1 is the debug!=null test. The resulting code is:
if (test1() test2() || test3() test4()) {
System.out...
}
So if test1 fails, then we jump to the test3 test4 case, which then
potentially has to go through the isOn processing twice. Granted,
because of the way isOn was written (if args == null) it's not adding a
Update #3: JEP 123: SecureRandom First Draft and Implementation.
The third version is now out (minus test cases), and is ready in the
webrev.03 directory.
http://cr.openjdk.java.net/~wetmore/6425477/
The only change is the API as we discussed.
Brad
On 1/9/2013 7:21 PM, Brad Wetmore wrote:
Thanks for the feedback. I also received some privately which had
similar comments.
Wrapping up several emails into some bullet points:
1. I like Sean's suggested tweak to the API. I'm thinking of adjusting
it slightly.
2. Xuelei has a point about my fallback of most preferred
implementation may not actually be strong. And like Max, I've also had
concerns about which provider.
In my previous proposal laundry list for SecureRandom, I had something
like:
securerandom.strongAlgorithms=algname1,algname2.provname1,algname3
which addresses both issues. The property will contain a list of algs
or algs/providers, and we'll iterate through them. If we can't create
an instance of one of these, return a null.
public static SecureRandom getStrongSecureRandom()
Given these comments, I think I'm going to move forward on this. The
application will do:
SecureRandom sr = SecureRandom.getStrongSecureRandom();
if (sr == null) {
// Decide if this is a problem, and whether to recover.
// sr = new SecureRandom(); or return;
}
3. Sean wrote:
There's an assumption that the securerandom.strongAlgorithm has been
configured appropriately.
Exactly, we'll ship with default values for each platform, and
programs/deployers can add/subtract as needed.
4. Xuelei wrote:
The 2nd one is to define a SPI method (pros: the
admin won't need to set the property. The admin does not always know
what kind of providers will be used at runtime).
If I'm reading this comment right, given the pros of the current
approach, I hesitate letting implementations make comparative strength
decisions.
Thanks! I should have a new version out tonight.
Brad
Re: Fw: Update #2: JEP 123: SecureRandom First Draft and Implementation.
Thanks Bruce/Michael, FYI, I've created: 8006041: Create SecureRandom standard algorithm names. against JDK 8 to track this issue, and I had previously filed: 8003584: Consider adding a more modern SecureRandom implementation to add the SP800-90a algorithms in JDK. Brad On 1/10/2013 9:48 AM, Bruce Rich wrote: +1 IBM already has SP800-90a/SHA256/HASH, SP800-90a/SHA384/HASH, and SP800-90a/SHA512/HASH in our provider, but without standardized names, they are not very useable for the Java community as a whole. Bruce A Rich brich at-sign us dot ibm dot com - Forwarded by Bruce Rich/Austin/IBM on 01/10/2013 11:44 AM - From: Michael StJohns mstjo...@comcast.net To: Sean Mullan sean.mul...@oracle.com, Xuelei Fan xuelei@oracle.com Cc: OpenJDK Dev list security-dev@openjdk.java.net, Brad Wetmore bradford.wetm...@oracle.com Date: 01/09/2013 09:32 PM Subject: Re: Update #2: JEP 123: SecureRandom First Draft and Implementation. Sent by: security-dev-boun...@openjdk.java.net At 09:45 AM 1/9/2013, Sean Mullan wrote: think it is unlikely that 2 providers would implement the same SecureRandom algorithm, since the names are not standardized like other cryptographic algorithms such as SHA-256, RSA, etc. Can this be fixed? There really should be a flavor for this. E.g. SP800-90a/SHA256/HASH SP800-90A/SHA256/HMAC SP800-90A/AES/CTR NRBG/NoisyDiode[/implementation id] NRBG/RingOscillator[/Implementation id] There are about 6 classes of NIST approved deterministic random number generators. See http://csrc.nist.gov/publications/fips/fips140-2/fips1402annexc.pdf. I wouldn't be surprised to find that multiple providers implement the same RNGs, but don't have a common name for them. In fact, according to wikipedia, the underlying function for MSCAPI is the FIPS186-2 appendix 3.1 with SHA1 function. Mike
Update #4: JEP 123: SecureRandom Draft and Implementation.
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
Update #2: JEP 123: SecureRandom First Draft and Implementation.
Greetings, Thanks so much for all of the constructive feedback. I wasn't terribly happy with the previous API proposal, and the comments reflected that. Sean Mullan came up with a nice API idea which greatly simplifies the goal of helping applications/deployers select a strong SecureRandom implementation. I agree with the comments from Xuelei and Micheal StJohns (and others). As Xuelei mentioned, the original scoping a year ago included some of those larger configuration ideas, and Michael gave some great additional food for thought. With the JDK 8 M6 deadline quickly drawing near, we unfortunately don't have time to explore this further, but what I'm proposing should complement and not preclude such future work. As additional goals for this JEP, I wanted to address three problems in the current implementation: 1. Many customer escalations/complaints of slow SecureRandom performance because of the limited entropy collection problem on Linux boxes, and there's much confusion about how to workaround this problem. (e.g. file:/dev/./urandom) 2. The documentation/configuration in the java.security file does not match the implementations, and is very confusing when trying to figure out #1 above. 3. It's not clear what the four different Oracle JDK SecureRandom implementations do. (Solution: update the Oracle Security Providers page.) I think the current proposal addresses these issues. The highlights: A Security property called securerandom.strongAlgorithm. There are defaults for each supported platform, and deployers can change this value if they have access to better ones. static String SecureRandom.getStrongAlgorithm() which obtains the property. The expected usage: * SecureRandom sr = SecureRandom.getInstance( * SecureRandom.getStrongAlgorithm()); * ...deleted... * keyPairGenerator.initialize(2048, sr); Cleaned out the incorrect information in the java.security files. The default securerandom.source Security property is set to file:/dev/random to properly reflect the implementation. (Ideally, I'd like to push this back to earlier JDK's.) If the java.security.egd/securerandom.source properties are set to either file:/dev/random or file:/dev/urandom, NativePRNG will be preferred to SHA1PRNG. NativePRNG now respects the java.security.egd/securerandom.source properties. 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.strongAlgorithm property points to the blocking variant. I still have some cleanup work to do on the NativePRNG.java file, but the rest (minus test cases) is ready in the webrev.02 directory. http://cr.openjdk.java.net/~wetmore/6425477/ Thanks, Brad
Re: Update #2: JEP 123: SecureRandom First Draft and Implementation.
One minor omission. On 1/9/2013 12:44 AM, Brad Wetmore wrote: 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.strongAlgorithm property points to the blocking variant on Unix-like OS's (Solaris/Linux/MacOS), and Windows-PRNG on Windows. Brad
Re: Update #2: JEP 123: SecureRandom First Draft and Implementation.
Thanks for the feedback. I also received some privately which had
similar comments.
Wrapping up several emails into some bullet points:
1. I like Sean's suggested tweak to the API. I'm thinking of adjusting
it slightly.
2. Xuelei has a point about my fallback of most preferred
implementation may not actually be strong. And like Max, I've also had
concerns about which provider.
In my previous proposal laundry list for SecureRandom, I had something like:
securerandom.strongAlgorithms=algname1,algname2.provname1,algname3
which addresses both issues. The property will contain a list of algs
or algs/providers, and we'll iterate through them. If we can't create
an instance of one of these, return a null.
public static SecureRandom getStrongSecureRandom()
Given these comments, I think I'm going to move forward on this. The
application will do:
SecureRandom sr = SecureRandom.getStrongSecureRandom();
if (sr == null) {
// Decide if this is a problem, and whether to recover.
// sr = new SecureRandom(); or return;
}
3. Sean wrote:
There's an assumption that the securerandom.strongAlgorithm has been
configured appropriately.
Exactly, we'll ship with default values for each platform, and
programs/deployers can add/subtract as needed.
4. Xuelei wrote:
The 2nd one is to define a SPI method (pros: the
admin won't need to set the property. The admin does not always know
what kind of providers will be used at runtime).
If I'm reading this comment right, given the pros of the current
approach, I hesitate letting implementations make comparative strength
decisions.
Thanks! I should have a new version out tonight.
Brad
On 1/9/2013 5:47 PM, Xuelei Fan wrote:
On 1/9/2013 10:45 PM, Sean Mullan wrote:
On 01/09/2013 06:41 AM, Xuelei Fan wrote:
I like this new proposal. Some minor comments here.
1. The name of SecureRandom.getStrongAlgorithm()
In the specification of the method and java.security, the word
strongest is used to describe the algorithm. While the name use the
word strong. I think the method name and specification should use the
same word, strong or strongest. Using both may cause some
miss-understanding.
My very first feeling of the strongest is that it may depends on
both providers and algorithms. If two providers support the same
strongest algorithms, which one is the strongest?
I think it is unlikely that 2 providers would implement the same
SecureRandom algorithm, since the names are not standardized like other
cryptographic algorithms such as SHA-256, RSA, etc.
It's OK if the algorithm name is not standardized.
But then there is a close connection between provider and this security
property. The administrator must be aware of which providers are
supported and what is the provider-private algorithm name before he can
edit the security property. It would be better to describe the behavior
in the spec of the security property.
Not sure about whether it is possible that the provider that support the
provider-private algorithm are not loaded at runtime. If it happens,
the effect to get strong secure random may not work, a weak one may return.
It's a little bit
confusing to me to set security property. I would prefer to use strong.
Yes, but grammatically the term Returns the strong algorithm name
doesn't work. So I think using strongest instead is ok and not overly
confusing.
2. Do we really need the SecureRandom.getStrongAlgorithm()?
As the strong algorithm is specified by security property. I think it
should be enough to use Java.Security.getProperty(). We properly don't
need a new method here. We properly need to add some additional
description about the default value of strong algorithm that is
recommended to use when the security property is not set.
Look at the examples,
With this method, the application call looks like (1):
String strongAlg = SecureRandom.getStrongAlgorithm();
SecureRandom sr = SecureRandom.getInstance(strongAlg);
While using security property, the application call (2):
String strongAlg =
Security.getProperty(securerandom.strongAlgorithm);
SecureRandom sr = SecureRandom.getInstance(strongAlg);
if (strongAlg == null) {
strongAlg = new SecureRandom().
} else {
sr = SecureRandom.getInstance(strongAlg);
}
As we have defined security property, the (2) code style is always
useable. Looks like that the (1) style is not really necessary, because
(2) does the same thing.
Yes, but (2) contains a lot of boilerplate code. Also the app may not
have permission to read the security property, so you may have to deal
with that too. All of that is taken care of for you in the new method.
In my opinion, developers are more likely to use this new feature if we
make it as easy as possible.
In fact, I thought of an even simpler solution. I think we should
replace the new getStrongAlgorithm method with the
Re: Update #2: JEP 123: SecureRandom First Draft and Implementation.
I don't see any reason why not. We just need to come up with a good naming convention, and then we can add that into the Standard Algorithms document. The existing names were established years ago, based on functional implementations rather than a specific algorithmic basis. Brad On 1/9/2013 7:31 PM, Michael StJohns wrote: At 09:45 AM 1/9/2013, Sean Mullan wrote: think it is unlikely that 2 providers would implement the same SecureRandom algorithm, since the names are not standardized like other cryptographic algorithms such as SHA-256, RSA, etc. Can this be fixed? There really should be a flavor for this. E.g. SP800-90a/SHA256/HASH SP800-90A/SHA256/HMAC SP800-90A/AES/CTR NRBG/NoisyDiode[/implementation id] NRBG/RingOscillator[/Implementation id] There are about 6 classes of NIST approved deterministic random number generators. See http://csrc.nist.gov/publications/fips/fips140-2/fips1402annexc.pdf. I wouldn't be surprised to find that multiple providers implement the same RNGs, but don't have a common name for them. In fact, according to wikipedia, the underlying function for MSCAPI is the FIPS186-2 appendix 3.1 with SHA1 function. Mike
Re: JEP 123: SecureRandom First Draft and Implementation.
Forwarding some relevant comments: Brad Set #1 of 2: From weijun.wang (at) oracle (dot) com: SecureRandom.java: First you have If mode is set to true, successive calls... then you also says the return value may not necessarily be the same as the original object. Shall I use the return value or this? Also, what if I call the method with false? The spec says the strong mode may block. Does this imply that the weak mode never blocks? SecureRandomSpi.java: * Calls to codeengineSetStrongMode/code will return * the current codemode/code. You mean engineGetStrongMode? java.security: 100 # On Unix-like systems (for example, Solaris/Linux/MacOS), there is a 101 # separate NativePRNG implementation that obtains seed and random numbers 102 # from special device files. If a file is specified and does not exist, 103 # NativePRNG will not be available. file is the only currently 104 # supported protocol type. If a file is specified and it does exist, will NatievPRNG read from *this* specified file? Or still from some mysterious special devide file? 106 # In addition, if file:/dev/random or file:/dev/urandom is 107 # specified, the NativePRNG implementation will be more preferred than 108 # SHA1PRNG. Is more needed when preferred is used? Also, I haven't read the impl codes for a while, but by specifying one of the 2 sources above, is SHA1PRNG almost the same as NativePRNG? I'll read the code changes later. Thanks Max
Re: JEP 123: SecureRandom First Draft and Implementation.
Forwarding some relevant comments: Brad Set #2 of 2: From xuelei.fan (at) oracle (dot) com: From the specification, looks like that a provider would have to support both high-quality and normal-quality mode. Otherwise, SecureRandomSpi.engineSetStrongMode() should throw exception if the provider does not support high-quality mode. It does not look intuitive to me, I think a few (many be only a very few) providers may only support normal-quality or blocking mode. From the spec of SecureRandom.getStrongSecureRandom(), my understand is that the returned value may be not configured with the request mode. I mean that if application request to use high-quality secure random, the returned value may be not a high-quality secure random because the underlying provider does not support the SecureRandomSpi.engineSetStrongMode() or only supports normal-quality mode. It's confusing. Or am I miss-understanding something? BTW, the returned value of SecureRandom.getStrongSecureRandom() may not inherit the secure properties from the original caller instance, for example, the initialized seed. I think it would be better to call setSeed again on the returned value to add additional randomness. The code may look like: SecureRandom sr = ...; sr.setSeed(...); SecureRandom newSR = sr.getStrongSecureRandom(true); newSR.setSeed(...); // looks like useless, but it is useful // because the previous seed isn't inherited. I think the quality of a secure random depends on the provider. The quality may be a attribute of a provider. Personally, I like more to use block or non-block mode. I think you may have though about to use the algorithm characteristics, as the one proposed in JEP 123 description: sr = new SecureRandom( ..., SR_HIGHQUALITY|SR_NON_BLOCKING); It looks more intuitive to me, and the API may look like: // look for providers that supports the particular mode. + SecureRandom(boolean block); + SecureRandom(byte seed, boolean block); + SecureRandom.getInstance(String algorithm, boolean block); + SecureRandom.isBlockMode(); We may not need to update getInstance(String, Provider) because block or non-block is a character of a provider (get the value by isBlockMode()). Just for your consideration, may be too later. Xuelei On 1/2/2013 5:58 PM, Brad Wetmore wrote: Hi, Please review the API/impl for JEP 123: http://openjdk.java.net/jeps/123 http://cr.openjdk.java.net/~wetmore/6425477/webrev.00/ Oracle folks, there is also the internal CCC that needs review. The bug id is 6425477. There are several SecureRandom implementations in Oracle's JDK, and together with the configuration options in the java.security file, it can be very confusing for users to understand. As part of the work on JEP 123, I took a comprehensive look at the different SecureRandom implementations and how we got here. There are these implementations: PKCS11: Direct calls to the native PKCS11 library. Only enabled by default on Solaris, but available for any OS. No difference between seed/random. NativePRNG: uses /dev/random and /dev/urandom for seeds/random numbers respectively. Doesn't exist on Windows. SHA1PRNG: Available on all platforms. By default, uses a confusing mix of /dev/[u]random for internal seeding and external seed generation, along with a SHA1 MessageDigest for generating random numbers. The properties (below) control seeding, but in a confusing manner. Windows-PRNG: Direct calls to the MSCAPI library, only available for Windows. No difference between seed/random. There were two main points for this JEP: 1. Provide an API that allows applications to indicate whether they want the strongest-possible (possibly blocking) values, or if just regular values will do. 2. See if we can clarify the configuration model, and eliminate some of the confusion caused by the securerandom.source/java.security.egd variables. This second point has caused a lot of pain for developers/deployers/support. The workaround of specifying file:/dev/./urandom or file:///dev/urandom instead of file:/dev/urandom has to be one of the most unintuitive ever. [1] ;) The default value of the variable is changed to file:/dev/random to reflect the actual implementation we've been shipping since JDK 5, but will also install NativePRNG as more preferred over the SHA1PRNG. Otherwise, the part of the implementation stays the same, and is now better documented in the java.security file. We'll also be updating the Oracle Provider documentation to reflect the implementations, but that work will be done later. Thanks, Brad [1] https://forums.oracle.com/forums/thread.jspa?messageID=3793101
Re: JEP 123: SecureRandom First Draft and Implementation.
I had an ugly chicken/egg bug I just figured out, so didn't get a chance
to respond to your/Weijun/Xuelei's comments.
For the APIs, it's pretty clear I need some clearer explanations here,
and maybe some adjustments.
I wouldn't suggest you spend much more time on the internals as things
will change a bit and I just found another issue, but in case you want
to see what I had in mind, there's a new webrev:
http://cr.openjdk.java.net/~wetmore/6425477
See: webrev.01
addressed #4 below.
in NativePRNG, if the file URL isn't readable, it defaults to
/dev/random instead of disabling this implementation.
couple bug fixes.
BTW, on #3 (@code), the Oracle Javadoc comment page still says to use
code/code. I originally didn't change it because there were so many
instances of code.
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html
Thanks,
Brad
On 1/4/2013 2:03 PM, Sean Mullan wrote:
Just some initial comments on the API. I have not looked at the code yet.
1. getStrongSecureRandom says:
* If the underlying SPI implementation does not support the
* {@link SecureRandomSpi.engineSetStrongMode(boolean)
* SecureRandomSpi.engineSetStrongMode(boolean)} method,
* then a wrapper class will redirect codeSecureRandomSpi/code
* calls from codenextBytes()/code to codegenerateSeed()/code.
Can you explain in a bit more detail what this means? Is the
SecureRandom object that is returned the same whether mode is true or
false, even if the underlying implementation could be upgraded to
support a strong mode?
2. The name for the method getStrongMode seems a bit odd since it
returns a boolean. How about isStrong instead?
3. Nit: Use {@code} instead of code/code
4. Consider marking getStrongSecureRandom and getStrongMode final. I
think the other methods on SecureRandom are not final because the SPI
was added later, unlike other security SPI classes.
--Sean
On 01/02/2013 08:58 PM, Brad Wetmore wrote:
Hi,
Please review the API/impl for JEP 123:
http://openjdk.java.net/jeps/123
http://cr.openjdk.java.net/~wetmore/6425477/webrev.00/
Oracle folks, there is also the internal CCC that needs review. The bug
id is 6425477.
There are several SecureRandom implementations in Oracle's JDK, and
together with the configuration options in the java.security file, it
can be very confusing for users to understand. As part of the work on
JEP 123, I took a comprehensive look at the different SecureRandom
implementations and how we got here.
There are these implementations:
PKCS11:
Direct calls to the native PKCS11 library. Only enabled
by default on Solaris, but available for any OS. No difference
between seed/random.
NativePRNG:
uses /dev/random and /dev/urandom for seeds/random numbers
respectively. Doesn't exist on Windows.
SHA1PRNG:
Available on all platforms. By default, uses a confusing mix of
/dev/[u]random for internal seeding and external seed
generation, along with a SHA1 MessageDigest for
generating random numbers. The properties (below) control
seeding, but in a confusing manner.
Windows-PRNG:
Direct calls to the MSCAPI library, only available for Windows.
No difference between seed/random.
There were two main points for this JEP:
1. Provide an API that allows applications to indicate whether they
want the strongest-possible (possibly blocking) values, or if just
regular values will do.
2. See if we can clarify the configuration model, and eliminate some of
the confusion caused by the securerandom.source/java.security.egd
variables.
This second point has caused a lot of pain for
developers/deployers/support. The workaround of specifying
file:/dev/./urandom or file:///dev/urandom instead of
file:/dev/urandom has to be one of the most unintuitive ever. [1] ;)
The default value of the variable is changed to file:/dev/random to
reflect the actual implementation we've been shipping since JDK 5, but
will also install NativePRNG as more preferred over the SHA1PRNG.
Otherwise, the part of the implementation stays the same, and is now
better documented in the java.security file.
We'll also be updating the Oracle Provider documentation to reflect the
implementations, but that work will be done later.
Thanks,
Brad
[1] https://forums.oracle.com/forums/thread.jspa?messageID=3793101
JEP 123: SecureRandom First Draft and Implementation.
Hi, Please review the API/impl for JEP 123: http://openjdk.java.net/jeps/123 http://cr.openjdk.java.net/~wetmore/6425477/webrev.00/ Oracle folks, there is also the internal CCC that needs review. The bug id is 6425477. There are several SecureRandom implementations in Oracle's JDK, and together with the configuration options in the java.security file, it can be very confusing for users to understand. As part of the work on JEP 123, I took a comprehensive look at the different SecureRandom implementations and how we got here. There are these implementations: PKCS11: Direct calls to the native PKCS11 library. Only enabled by default on Solaris, but available for any OS. No difference between seed/random. NativePRNG: uses /dev/random and /dev/urandom for seeds/random numbers respectively. Doesn't exist on Windows. SHA1PRNG: Available on all platforms. By default, uses a confusing mix of /dev/[u]random for internal seeding and external seed generation, along with a SHA1 MessageDigest for generating random numbers. The properties (below) control seeding, but in a confusing manner. Windows-PRNG: Direct calls to the MSCAPI library, only available for Windows. No difference between seed/random. There were two main points for this JEP: 1. Provide an API that allows applications to indicate whether they want the strongest-possible (possibly blocking) values, or if just regular values will do. 2. See if we can clarify the configuration model, and eliminate some of the confusion caused by the securerandom.source/java.security.egd variables. This second point has caused a lot of pain for developers/deployers/support. The workaround of specifying file:/dev/./urandom or file:///dev/urandom instead of file:/dev/urandom has to be one of the most unintuitive ever. [1] ;) The default value of the variable is changed to file:/dev/random to reflect the actual implementation we've been shipping since JDK 5, but will also install NativePRNG as more preferred over the SHA1PRNG. Otherwise, the part of the implementation stays the same, and is now better documented in the java.security file. We'll also be updating the Oracle Provider documentation to reflect the implementations, but that work will be done later. Thanks, Brad [1] https://forums.oracle.com/forums/thread.jspa?messageID=3793101
Re: Request for Review: 7193792: sun/security/pkcs11/ec/TestECDSA.java failing intermittently
Ditto. Thanks for also removing from ProblemList. brad On 12/6/2012 1:05 AM, Vincent Ryan wrote: Fix looks good. On 6 Dec 2012, at 00:16, Jason Uh wrote: Could I please get a review of: http://cr.openjdk.java.net/~juh/7193792/webrev.00/ Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7193792 This change fixes sun/security/pkcs11/ec/TestECDSA.java, which fails on Solaris due to what seems to be an unintended back-to-back call to Signature.initSign(privateKey). On the second initSign, the test fails with CKR_OPERATION_ACTIVE, which means: There is already an active operation (or combination of active operations) which prevents Cryptoki from activating the specified operation. ... (http://www.cryptsoft.com/pkcs11doc/v220/pkcs11__all_8h.html#aCKR_OPERATION_ACTIVE) Thanks, Jason
Re: Code Review Request, 8004184, security tests leave JSSEServer running
Looks ok to me too. Brad On 12/2/2012 5:28 PM, Xuelei Fan wrote: On 12/2/2012 11:38 PM, Chris Hegarty wrote: Your description talks about releasing resources, but the comment in the source mentions initialising system properties. Are you using othervm mode to get around releasing resources or an issues with previously set system properties? JSSE cannot support agentvm mode because some system properties will be only called one time at every VM. Othervm mode is used to release resources. And it is also used to correct the previous system properties issues. I normally put the comments to all JSSE test, so that the code readers can understand why the test must be run in othervm mode. Thanks, Xuelei -Chris On 2 Dec 2012, at 02:49, Xuelei Fan xuelei@oracle.com wrote: Hi, Please review the test bug that does not release the server resources. bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004184webrev: http://cr.openjdk.java.net./~xuelei/8004184/webrev.00/ Thanks, Xuelei
Re: [8] Code Review Request for 8004044: Lazily instantiate SunJCE.RANDOM
Sean, Could you please provide a little more background for the motivation for this fix? I'm not quite following yet how we get into this situation. (Bear with me, I'm not familiar with ServiceLoader yet, so this may be a beginner's question.) In our current implementation, there's always been the assumption that the order of providers is specified by the java.security file, and possibly tweaked by Security.*provider() code, thus ensuring that requested algs will come from the highest ordered provider at the time of initialization. Is this going away? FYI, the current behavior when no providers have been registered as Security Providers is to directly create a SHA1PRNG. The code itself is ok, even without the upcoming change to ServiceLoader. Brad On 11/28/2012 8:53 AM, Sean Mullan wrote: Please review the following webrev which lazily initializes the SecureRandom object used by the SunJCE provider. In JDK 9, we want to load JCE providers with java.util.ServiceLoader as part of the transition to modules. This can cause potential recursive loading issues if the Sun provider for SecureRandom has not been loaded yet (since providers are loaded in no specific order using ServiceLoader). The fix is to lazily instantiate SunJCE.RANDOM. This is a proactive fix that will smooth the transition to modules. There is no regression test as this is a small refactoring that is covered by existing tests. The bug has been tagged with the noreg-cleanup label. The bug is not up on bugs.sun.com yet. bug : http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004044 webrev: http://cr.openjdk.java.net/~mullan/webrevs/8004044/webrev.00 Thanks, Sean
Re: Code review request, JDK-8001569 Regression test GetPeerHost uses static port number
I'm surprised we are still finding these! ;) I though we had fixed most of them already. Maybe whoever fixed them was looking for 443/80/8080, and missed ? Brad On 11/9/2012 1:20 AM, Chris Hegarty wrote: Looks fine Xuelei. -Chris. On 09/11/2012 05:27, Xuelei Fan wrote: webrev: http://cr.openjdk.java.net./~xuelei/8001569/webrev.00/ Test case, test/sun/security/ssl/com/sun/net/ssl/internal/ssl/ServerHandshaker/GetPeerHost.java, uses the static port number, and does not close the socket explicitly. As may result that the socket/port cannot be released in time in OS level. The fix will use dynamically allocated server port. Thanks, Xuelei
8001419: Build the JCE portion of JDK-8000970
I have stripped out the JCE portion of: http://mail.openjdk.java.net/pipermail/jdk8-dev/2012-October/001547.html and am integrating it for Fredrik, and doing the internal JCE builds. The work will be done under: 8001419: Build the JCE portion of JDK-8000970 Summary: Original code done by Fredrik Ohrstrom, separated/pushed by wetmore Reviewed-by: wetmore The codereview is: http://cr.openjdk.java.net/~wetmore/8001419/ Brad
Re: Memory leak fix for: src/solaris/native/com/sun/security/auth/module/Unix.c
Looks good, taken in isolation. Just to be sure, are there any other methods that might return some object that has to be freed. Brad On 10/19/2012 1:28 PM, John Zavgren wrote: Greetings: The following webrev image contains a fix for a memory leak that occurs in the procedure: Java_com_sun_security_auth_module_UnixSystem_getUnixInfo (JNIEnv *env, jobject obj) in the file: jdk/src/solaris/native/com/sun/security/auth/module/Unix.c http://cr.openjdk.java.net/~khazra/john/8000204/webrev/ The leaked memory is associated with the pointer named groups: gid_t *groups = (gid_t *)calloc(numSuppGroups, sizeof(gid_t)); The procedure in question exits in many places and in every case it's necessary to deallocate this memory. The leak occurred because returns were being made without freeing it. I fixed the leak by modifying the code so that there is a common exit point, that is reached from these same places via goto statements, that performs this common function, immediately before the return statement. Thanks! John Zavgren john.zavg...@oracle.com
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
BTW, both 7068321 and 8000954 are in Open, should be In Progress.
On 10/15/2012 6:27 PM, Xuelei Fan wrote:
On 10/16/2012 7:12 AM, Brad Wetmore wrote:
Looks good.
The main update is to add final keyword to the new methods in
SSLParamaters. I prefer to use the final because I don't see a
requirement to override them.
Let me know when the new CCC is ready, I'll approve it.
The CCC for this fix, 7068321 has been finalized. I submitted a new CCC
to add the final keyword:
https://jbs.oracle.com/bugs/browse/JDK-8000954
I will fill the new CCC after the push of 7068321.
Are you going to contact IETF?
Yes, I will.
Almost there.
I think we get all spec review, code review done.
I think so.
I will push the
changeset after the CCC get approved, and pay more time on CPU bugs.
You could push both together and put everything in just one changeset,
but of course up to you.
I don't think there's anything else for me to do, let me know if you're
waiting on something.
Brad
Thanks,
Xuelei
Brad
On 10/13/2012 6:26 AM, Xuelei Fan wrote:
New webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.14/
The main update is to add final keyword to the new methods in
SSLParamaters. I prefer to use the final because I don't see a
requirement to override them. But I can go with the version with or
without the final keyword. I think you properly don't need to review
other parts of the webrev any more, no other major update.
Thanks,
Xuelei
On 10/13/2012 10:17 AM, Xuelei Fan wrote:
I have no access to office network, just a quick reply. I want add
final keyword for the new methods in SSLParameters. See below.
Sent from my iPad
On Oct 13, 2012, at 8:00 AM, Brad Wetmore
bradford.wetm...@oracle.com wrote:
I guess you didn't need to have me as reviewer before going final
with
the CCC?
The CCC has been finalized. ;-) I though you have done with spec
review.
Anyway, we still can make updates on specification within new bugs.
That's fine, I just wasn't sure if you needed someone to approve
the final version of the spec to move it to the next state.
Just a comment:
459/468: Currently you have serverNames and sniMatchers as package
private variables, so the ClientHandshaker/ServerHandshaker *could*
inherit these variables rather than have a separate get methods.
Or you
could make them private and keep the get calls.
Good catch!
I can look at your latest if you like, but probably not necessary.
I will send the new webrev tonight my time.
I'm just not seeing why this implies that it requires *EVERY* name
must
match. This just says we can do one of two things upon receipt of an
unrecognized hostname: continue on, or alert/close. We can be very
restrictive (ALL/AND) or less so (at least one/OR), and still be
within
the RFC, I think.
I agree with your parser.
In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension.
Where? We do say in SNIMatcher:
Servers can use Server Name Indication (SNI) information to
decide if
specific {@link SSLSocket} or {@link SSLEngine} instances should
accept a connection.
But I don't think we have ever said that it MUST match or must
match all
or even what the implementation must do if there is a match
failure. Nor
should we specify that in the API, IMHO. That's implementation
behavior.
I may have different options on this point. I think, it must be a
specified behavior in Java. Otherwise, it is very confusing about
how to
use 1+ server names in server side.
I am going suggest we ask for guidance from IETF about this. It is
not clear from RFC 6066 how to handle multiple SNI types, even
reading very generously between the lines. See below.
Let's start from the logic of the design. If the specification is not
clear, I will rewrite the spec according to our agreement.
Let's start from the requirement. I think once a SNIMatcher is defined
in SSL parameters, it MUST be used to perform the match operations if
the corresponding server name appears in the SNI extension.
Otherwise,
what will happens? See the example:
1. SNI extension contains HostName, www.example.com.
2. Server side defines SNIMatcher for HostName, to accept
www.example.org. Server does not accept www.example.com.
3. What's the result of the handshake? Is the SNI extension is
accetable?
www.example.org != www.example.com. I would say fail handshake with
unknown_hostname.
If we do not define the spec about how to use SNIMatcher. The
answer to
#3 is unclear. Because if a SNIMatcher may be used to perform match
operation, and may be not used to perform match operation, then the
server may accept the SNI extension, may ignore the SNI extension, may
reject the SNI extension. It is useless to define SNIMatcher.
I think the requirement is clear that it is a must to use
SNIMatcher to
perform the match operation if the corresponding server name
appears in
the SNI
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
Approved. Brad On 10/16/2012 6:30 PM, Xuelei Fan wrote: On 10/17/2012 9:30 AM, Xuelei Fan wrote: On 10/17/2012 4:57 AM, Brad Wetmore wrote: You could push both together and put everything in just one changeset, but of course up to you. Good. Please review the CCC request, I will fast-track the request. http://ccc.us.oracle.com/8000954 I don't think there's anything else for me to do, let me know if you're waiting on something. I think you are done, ;-) except the above CCC review. I still need to wait for the approval of the CCCs. Thanks, Xuelei
Re: jsse.jar still needed?
On 10/15/2012 5:17 AM, Alan Bateman wrote: This might be a silly question but is jsse.jar needed any more? I think, but might be wrong, that it dates back to when JSSE was a extension. I'm curious if it would matter if the classes were moved to rt.jar? There are source licensee issues here. I'll talk to you about this offline. Brad I'm also interested to understand why jsse.jar has sun.security.provider.Sun and sun.security.rsa.SunRsaSign. This creates the odd situation where most of the types in those packages are in rt.jar but it's just these two classes that are in jsse.jar. Thanks, -Alan
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
Looks good.
The main update is to add final keyword to the new methods in
SSLParamaters. I prefer to use the final because I don't see a
requirement to override them.
Let me know when the new CCC is ready, I'll approve it.
Are you going to contact IETF?
Almost there.
Brad
On 10/13/2012 6:26 AM, Xuelei Fan wrote:
New webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.14/
The main update is to add final keyword to the new methods in
SSLParamaters. I prefer to use the final because I don't see a
requirement to override them. But I can go with the version with or
without the final keyword. I think you properly don't need to review
other parts of the webrev any more, no other major update.
Thanks,
Xuelei
On 10/13/2012 10:17 AM, Xuelei Fan wrote:
I have no access to office network, just a quick reply. I want add final
keyword for the new methods in SSLParameters. See below.
Sent from my iPad
On Oct 13, 2012, at 8:00 AM, Brad Wetmore bradford.wetm...@oracle.com wrote:
I guess you didn't need to have me as reviewer before going final with
the CCC?
The CCC has been finalized. ;-) I though you have done with spec review.
Anyway, we still can make updates on specification within new bugs.
That's fine, I just wasn't sure if you needed someone to approve the final
version of the spec to move it to the next state.
Just a comment:
459/468: Currently you have serverNames and sniMatchers as package
private variables, so the ClientHandshaker/ServerHandshaker *could*
inherit these variables rather than have a separate get methods. Or you
could make them private and keep the get calls.
Good catch!
I can look at your latest if you like, but probably not necessary.
I will send the new webrev tonight my time.
I'm just not seeing why this implies that it requires *EVERY* name must
match. This just says we can do one of two things upon receipt of an
unrecognized hostname: continue on, or alert/close. We can be very
restrictive (ALL/AND) or less so (at least one/OR), and still be within
the RFC, I think.
I agree with your parser.
In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension.
Where? We do say in SNIMatcher:
Servers can use Server Name Indication (SNI) information to decide if
specific {@link SSLSocket} or {@link SSLEngine} instances should
accept a connection.
But I don't think we have ever said that it MUST match or must match all
or even what the implementation must do if there is a match failure. Nor
should we specify that in the API, IMHO. That's implementation behavior.
I may have different options on this point. I think, it must be a
specified behavior in Java. Otherwise, it is very confusing about how to
use 1+ server names in server side.
I am going suggest we ask for guidance from IETF about this. It is not clear
from RFC 6066 how to handle multiple SNI types, even reading very generously
between the lines. See below.
Let's start from the logic of the design. If the specification is not
clear, I will rewrite the spec according to our agreement.
Let's start from the requirement. I think once a SNIMatcher is defined
in SSL parameters, it MUST be used to perform the match operations if
the corresponding server name appears in the SNI extension. Otherwise,
what will happens? See the example:
1. SNI extension contains HostName, www.example.com.
2. Server side defines SNIMatcher for HostName, to accept
www.example.org. Server does not accept www.example.com.
3. What's the result of the handshake? Is the SNI extension is accetable?
www.example.org != www.example.com. I would say fail handshake with
unknown_hostname.
If we do not define the spec about how to use SNIMatcher. The answer to
#3 is unclear. Because if a SNIMatcher may be used to perform match
operation, and may be not used to perform match operation, then the
server may accept the SNI extension, may ignore the SNI extension, may
reject the SNI extension. It is useless to define SNIMatcher.
I think the requirement is clear that it is a must to use SNIMatcher to
perform the match operation if the corresponding server name appears in
the SNI extension. I think it is true for the case when there is only
one server name type, at least.
I completely agree.
Let's look more about what happens when there is 1+ server name types in
the SNI extension. Let's use the example in your previous mail.
SNIHostname: example.com
SNINickName: www.example.com
SNIMatcher: example.com
SNINickNameMatcher: www1.example.com
Suppose that the server is able to understand both HostName and
NickName. In the above example, server is able to recognize HostName,
but not NickName. Then should the server includes an extension of type
server_name in the (extended) server hello?
If the server_name extension is included, it is unclear for client side
which one
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
I guess you didn't need to have me as reviewer before going final with
the CCC?
The CCC has been finalized. ;-) I though you have done with spec review.
Anyway, we still can make updates on specification within new bugs.
That's fine, I just wasn't sure if you needed someone to approve the
final version of the spec to move it to the next state.
Just a comment:
459/468: Currently you have serverNames and sniMatchers as package
private variables, so the ClientHandshaker/ServerHandshaker *could*
inherit these variables rather than have a separate get methods. Or you
could make them private and keep the get calls.
Good catch!
I can look at your latest if you like, but probably not necessary.
I'm just not seeing why this implies that it requires *EVERY* name must
match. This just says we can do one of two things upon receipt of an
unrecognized hostname: continue on, or alert/close. We can be very
restrictive (ALL/AND) or less so (at least one/OR), and still be within
the RFC, I think.
I agree with your parser.
In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension.
Where? We do say in SNIMatcher:
Servers can use Server Name Indication (SNI) information to decide if
specific {@link SSLSocket} or {@link SSLEngine} instances should
accept a connection.
But I don't think we have ever said that it MUST match or must match all
or even what the implementation must do if there is a match failure. Nor
should we specify that in the API, IMHO. That's implementation behavior.
I may have different options on this point. I think, it must be a
specified behavior in Java. Otherwise, it is very confusing about how to
use 1+ server names in server side.
I am going suggest we ask for guidance from IETF about this. It is not
clear from RFC 6066 how to handle multiple SNI types, even reading very
generously between the lines. See below.
Let's start from the logic of the design. If the specification is not
clear, I will rewrite the spec according to our agreement.
Let's start from the requirement. I think once a SNIMatcher is defined
in SSL parameters, it MUST be used to perform the match operations if
the corresponding server name appears in the SNI extension. Otherwise,
what will happens? See the example:
1. SNI extension contains HostName, www.example.com.
2. Server side defines SNIMatcher for HostName, to accept
www.example.org. Server does not accept www.example.com.
3. What's the result of the handshake? Is the SNI extension is accetable?
www.example.org != www.example.com. I would say fail handshake with
unknown_hostname.
If we do not define the spec about how to use SNIMatcher. The answer to
#3 is unclear. Because if a SNIMatcher may be used to perform match
operation, and may be not used to perform match operation, then the
server may accept the SNI extension, may ignore the SNI extension, may
reject the SNI extension. It is useless to define SNIMatcher.
I think the requirement is clear that it is a must to use SNIMatcher to
perform the match operation if the corresponding server name appears in
the SNI extension. I think it is true for the case when there is only
one server name type, at least.
I completely agree.
Let's look more about what happens when there is 1+ server name types in
the SNI extension. Let's use the example in your previous mail.
SNIHostname: example.com
SNINickName: www.example.com
SNIMatcher: example.com
SNINickNameMatcher: www1.example.com
Suppose that the server is able to understand both HostName and
NickName. In the above example, server is able to recognize HostName,
but not NickName. Then should the server includes an extension of type
server_name in the (extended) server hello?
If the server_name extension is included, it is unclear for client side
which one is accepted.
The presence of a server's server_name extension indicates that an SNI
value was used to guide the selection of an appropriate cert. It seems
ambiguous in RFC 6066 as to whether this extension indicated ALL or
AT LEAST ONE SNI value guided the selection. I might lean towards ALL
since RFC 6066 says The 'extension_data' field of this extension SHALL
be empty. If it AT LEAST ONE were meant, there is no way of indicating
*which* extension was used, or if multiple ones were used.
As an aside, we have no API for the KeyManagers's to indicate whether
they actually used a SNI extension, so I think you are currently sending
a server server_name extension if any cert was selected. I don't think
this is a major problem given our current APIs.
Because the client side may need to use the
server_name to do endpoint identification, it is not safe to use
www.example.com to perform endpoint identification because the server
side does not accept it.
If the server_name extension is not included, it does not follow the
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
Hi again, On 10/9/2012 8:56 AM, Xuelei Fan wrote: Thanks for the comments. The new comments for SSLEngine/SSLSocket/SSLServerSocket do make the spec much more simple and clear. Thanks, that looks so much cleaner, and I think developers will appreciate it. BTW, I removed the restriction on SSLSocketFactory.createSocket(), the socket parameter can be an instance of SSLSocket. The spec review is closed. Please file new bugs if you have other concerns. Very minor comments in the API which don't need CCC modification. javax/net/ssl/SNIHostName.java == Changes look good. Didn't review everything. javax/net/ssl/SNIMatcher.java = Changes look good. Didn't review everything. javax/net/ssl/SNIServerName.java Changes look good. Didn't review everything. javax/net/ssl/StandardConstants.java Didn't review. javax/net/ssl/ExtendedSSLSession.java = Didn't review. javax/net/ssl/SSLParameters.java 60: Wow, good catch! 314: Looks great, thanks! javax/net/ssl/SSLSocketFactory.java === Change look good. 216: I think you need a period at end of sentence here. 231: Might suggest some reason text for the UnsupportedOperationException. javax/net/ssl/SSLEngine.java Minor nit: Copyright. 1218/1220/1225: You are not closing the li properly, I think you are effectively adding another empty bullet. 1227: Copy/paste error. You said socket, but probably meant engine. :) javax/net/ssl/SSLServerSocket.java == Minor nit: Copyright. 488/490/495: You are not closing the li properly. 499: Similar comment to above. You could say server socket or leave as is, since it is a kind of a socket. javax/net/ssl/SSLSocket.java Minor nit: Copyright. sun/security/ssl/Utilities.java === 46: Minor comment on method name, you might want to use addToSNIServerNameList since you are adding. 84: Just wondering why you added this method here? This added a bit of overhead in extra methods calls (well, maybe not with hotspot unrolling) and added a few chars to the source. So given that you have added this, why not update the remainder also? EngineOutputRecord/InputRecord/OutputRecord. See the below comment in SSLSocketImpl.java. If you decide to accept it, you will want to remove the unmodifiable collection. sun/security/ssl/BaseSSLSocketImpl.java === OK sun/security/ssl/ClientHandshaker.java == OK sun/security/ssl/HandshakeInStream.java === OK sun/security/ssl/HandshakeMessage.java == OK sun/security/ssl/Handshaker.java 291: The change to allow for setting of SNI information has opened up a race condition. It's probably not too bad for SSLSocket, but might be more for SSLEngine. When we create ClientHandshaker/ServerHandshakers, we normally grab all the parameters necessary for the handshaker, and any future parameter modifications will apply to the next handshake. In the past, our SNI information would never change, so it wasn't necessary to pass it in on creation. That assumption no longer holds. So you could see something like this: sslEngine.beginHandshake(); SSLParameters sslp = new SSLParameters(); sslp.setHostnames(example.com); sslEngine.setSSLParameters(sslp); // do handshake... and you'll get the new value instead of old. sun/security/ssl/HelloExtensions.java = 35: Are these extra imports necessary with the package import at line 31? 307: Can you add the backwards compatibility comment about the size? For backward compatibility, all future data structures associated with new NameTypes MUST begin with a 16-bit length field. I'll forget it otherwise. 423: This behavior is currently underspecified. Right now we have one SNI match type, but eventually we might have more. Your current impl requires that *ALL* SNI matchers match. What if you have more than one, and more than one SNI hostnames are sent? SNIHostname: example.com SNINickName: www.example.com SNIMatcher: example.com SNINickNameMatcher: www1.example.com Should this fail or succeed? That is, should it be an AND or an OR? Whatever you decide, please document it at least here. Not sure if you want to make the doc at the API level. 438: Minor nit: snInOther - sniInOther? sun/security/ssl/ProtocolList.java == OK sun/security/ssl/SSLEngineImpl.java === 2084: See comments in SSLSocketImpl.java:2509. 2100/2107: See comments in
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 9/26/2012 8:05 AM, Xuelei Fan wrote:
Hi,
Please review the implementation of JEP 114/CR 7068321, Support TLS
Server Name Indication (SNI) Extension in JSSE Server.
JEP 114: http://openjdk.java.net/jeps/114
BuG: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7068321
webrev : http://cr.openjdk.java.net./~xuelei/7068321/webrev.10/
Not a lot of comments since we've already done so much over the last few
months. :)
If you modified anything else that we hadn't talked about previously,
please call it out. I didn't do a complete line/line review, just
looking at the main points and previous comments.
javax/net/ssl/SSLSocket.java
javax/net/ssl/SSLEngine.java
javax/net/ssl/SSLServerSocket.java
==
(Xuelei and I have been having internal discussions about how to rework
some of the lengthy verbage currently in the SSLParameters class, so
this comment builds on that discussion.)
I just noticed three classes missing from the webrev. We are updating
the SSLParameters class with two new data structures (Host
names/Matchers), but the actual discussion of what happens to the
underlying SSLSockets/SSLEngines/SSLServerSocket is missing. Note we
did that for the other values like protocols/cipherSuites.
Rather than the current discussion in SSLParameters about previously
configured/default, I would like to suggest we follow the simple
discussion already in the javadocs, and it can shorten the verbiage in
SSLParameters very significantly! Even though we don't have equivalent
methods (setEnabledHostNames()) in SSLSocket/SSLEngine/SSLServer, we
should still talk about what happens when called:
Add:
This means:
o ...deleted...
o if params.getServerNames() is non-null, the socket will configure
its server names with that value.
o if params.getSNIMatchers() is non-null, the socket will configure
its SNI matchers with that value.
This places the behavior explanation better where it should be, and not
in the configuration discussion.
Now in the SSLParameters we simply talk about the effects these methods
have on the SSLParameters class and what the values represent. We
already have the link to
SSLSocket/SSLServerSocket/SSLEngine.setSSLParameters(). So...
setServerNames() remains as is.
setSNIHostNames() remains as is.
getServerNames():
=
316: Add to the constructor
, or null if none has been set.
321-326: We can leave this discussion if desired, I think it sets up
the discussion for 327-341 well. I might suggest reordering the points
slightly:
* It is recommended that providers initialize default Server Name
* Indications when creating
* codeSSLSocket/code/codeSSLEngine/codes. In the following
* examples, the server name could be represented by an instance of
* {@link SNIHostName} which has been initialized with the hostname
* www.example.com and type {@link StandardConstants#SNI_HOST_NAME}.
*
* pre
* Socket socket =
* sslSocketFactory.createSocket(www.example.com, 443);
* /pre
* or
* pre
* SSLEngine engine =
* sslContext.createSSLEngine(www.example.com, 443);
* /pre
* P
342-367: Remove these lines. All the necessary info is in the
setSSLParameters methods.
getSNIHostNames():
==
435: Add to the constructor
, or null if none has been set.
440-471: Remove these lines.
Nice and clean. All the discussion about default/current goes away.
javax/net/ssl/SNIHostName.java
==
52: Do you want to remind folks here that this class is supposed to be
immutable?
64: Not sure if caching this hash value will actually add much to
performance vs. volatile adds overhead. There will only a few of these
(likely 1).
85-86: Should these fragments be separated , instead of . as you
did below in 143-149. You probably need an or before the last item.
147: You probably need an or before the last item.
javax/net/ssl/SNIMatcher.java
=
32: Do you want to spell out Server Name Indication before you use the
abbreviation? Or link to RFC 6066? Just a thought, not strictly necessary.
105: Empty line at end of file.
javax/net/ssl/SNIServerName.java
56: Same comment about hashCode/volatile.
136: Might want to add a p between sentences.
javax/net/ssl/StandardConstants.java
OK
javax/net/ssl/ExtendedSSLSession.java
=
OK
javax/net/ssl/SSLParameters.java
See above. No other comments.
javax/net/ssl/SSLSocketFactory.java
===
Ok.
So that you can update and submit to CCC, I'll stop here, but the
implementation review will be continued in next email.
Hope this helps,
Brad
Re: [PATCH FOR REVIEW] Allow OpenJDK to be built with the unlimited crypto policy
On 9/27/2012 9:50 AM, Andrew Hughes wrote: - Original Message - Will you be putting this back yourself? If so let me know when you go in, and I can update the bug once you're in. I will, though I'll need a bug ID for it. I presume tl is ok as the forest to use? This going into 8? Then yes. At first, yes. Do you have any objections to me proposing it for 7u too, in due course? None here, other than needing a separate code review. Brad 7201205: Add Makefile configuration option to build with unlimited crypto in OpenJDK. Great, thanks! I'll push it. Brad Mark wrote: The summary is that it was just easier to remove unused classes that made the code tricky to understand for no good reason except for some secret proprietary code. Unfortunately, Oracle and some of our commercial (non-OpenJDK) licensees still depend on that tricky code. :( I'd personally love to strip it all out, but we have to balance all of its consumers (Oracle SE and ME, commercial source/binary licensees, OpenJDK, etc.) Andrew wrote: I'm sure it would be easy enough to dump those classes if Oracle started producing OpenJDK binaries licensed under the GPL, rather than binaries from their proprietary fork. Unfortunately, not likely in our current export/import climate. Yes, this is what I thought. We just have to make sure to test well before shipping binaries. Brad
Re: Code review request, 7200295 CertificateRequest message is wrapping when using large numbers of Certs
On 9/24/2012 7:01 PM, Xuelei Fan wrote:
On 9/25/2012 9:23 AM, Brad Wetmore wrote:
Are there situations where we might overflow the int?
Yes, it is possible for many integer add operations. As 2^32 is a lot
bigger than 2^24 (the biggest number TLS protocol allows), I'm not
worried too much about int32 overflow.
Ah yes...
Integer overflow checking would make the code ugly.
Agreed!
For example, in CertificateRequest.messageLength()
for (int i = 0; i authorities.length; i++) {
len += authorities[i].length();
}
What if len overflows?
Also, all of these field's callers are overflow-1?
I'm not sure I get your point. In RFC5246, exception session ID, other
variable length is one of 2^8-1, 2^16-1 or 2^24 -1.
I was just wondering if there were any fields that were 2^8/2^16/2^24
that would fail with this check. I hadn't looked through the whole RFC,
just asking if you had checked to avoid an off-by-one error.
I have a recollection that we recently did add some checks for making
sure that we did not overflow other vector sizes.
Brad
On 9/23/2012 7:42 PM, Xuelei Fan wrote:
Hi,
Please review the update to check output filed length overflow in TLS
handshaking.
bug : http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7200295
webrev: http://cr.openjdk.java.net/~xuelei/7200295/webrev.00/
The cause of the bug is that for 8, 16, 24 bits length-variable fields,
before put the bytes into the fields, we do not check that the length of
the bytes is less than the capabilities of the field.
Thanks,
Xuelei
Re: [PATCH FOR REVIEW] Allow OpenJDK to be built with the unlimited crypto policy
Will you be putting this back yourself? If so let me know when you go in, and I can update the bug once you're in. I will, though I'll need a bug ID for it. I presume tl is ok as the forest to use? This going into 8? Then yes. 7201205: Add Makefile configuration option to build with unlimited crypto in OpenJDK. Brad Mark wrote: The summary is that it was just easier to remove unused classes that made the code tricky to understand for no good reason except for some secret proprietary code. Unfortunately, Oracle and some of our commercial (non-OpenJDK) licensees still depend on that tricky code. :( I'd personally love to strip it all out, but we have to balance all of its consumers (Oracle SE and ME, commercial source/binary licensees, OpenJDK, etc.) Andrew wrote: I'm sure it would be easy enough to dump those classes if Oracle started producing OpenJDK binaries licensed under the GPL, rather than binaries from their proprietary fork. Unfortunately, not likely in our current export/import climate. Yes, this is what I thought. We just have to make sure to test well before shipping binaries. Brad
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 9/26/2012 8:05 AM, Xuelei Fan wrote: Hi, Please review the implementation of JEP 114/CR 7068321, Support TLS Server Name Indication (SNI) Extension in JSSE Server. JEP 114: http://openjdk.java.net/jeps/114 BuG: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7068321 webrev : http://cr.openjdk.java.net./~xuelei/7068321/webrev.10/ I would prefer to get any comments by the end of next Monday (Oct 1th), if possible. Minor correction to the URL: http://cr.openjdk.java.net/~xuelei/7068321/webrev.10/ BTW, the openjdk.java.net domain is temporarily down, Ops is working on it. Brad
Re: [PATCH FOR REVIEW] Allow OpenJDK to be built with the unlimited crypto policy
On 9/18/2012 7:39 AM, Andrew Hughes wrote: The following simple webrev will achieve what I think is needed: http://cr.openjdk.java.net/~andrew/100062/webrev.01/ allowing OpenJDK to be built with the unlimited rather than limited crypto policy in place. I got a chance to talk to Valerie, and what you've done looks good. I'm wetmore if you need a reviewer, and I think Kelly has looked at it too. I just placed it within the OPENJDK ifdef so it won't interfere with the proprietary build at all, as obviously I can't test it Please leave your new code check within the ifdef OPENJDK. Will you be putting this back yourself? If so let me know when you go in, and I can update the bug once you're in. Mark wrote: The summary is that it was just easier to remove unused classes that made the code tricky to understand for no good reason except for some secret proprietary code. Unfortunately, Oracle and some of our commercial (non-OpenJDK) licensees still depend on that tricky code. :( I'd personally love to strip it all out, but we have to balance all of its consumers (Oracle SE and ME, commercial source/binary licensees, OpenJDK, etc.) Andrew wrote: I'm sure it would be easy enough to dump those classes if Oracle started producing OpenJDK binaries licensed under the GPL, rather than binaries from their proprietary fork. Unfortunately, not likely in our current export/import climate. Brad
Re: Code review request, 7200295 CertificateRequest message is wrapping when using large numbers of Certs
Are there situations where we might overflow the int?
For example, in CertificateRequest.messageLength()
for (int i = 0; i authorities.length; i++) {
len += authorities[i].length();
}
What if len overflows?
Also, all of these field's callers are overflow-1?
Brad
On 9/23/2012 7:42 PM, Xuelei Fan wrote:
Hi,
Please review the update to check output filed length overflow in TLS
handshaking.
bug : http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7200295
webrev: http://cr.openjdk.java.net/~xuelei/7200295/webrev.00/
The cause of the bug is that for 8, 16, 24 bits length-variable fields,
before put the bytes into the fields, we do not check that the length of
the bytes is less than the capabilities of the field.
Thanks,
Xuelei
Re: Quick Codereview
If you're interested, the final version is on: http://cr.openjdk.java.net/~wetmore/7197071/webrev.02/ I removed the UnsatisfiedLinkError check, and also added a check for jce.jar, and did some minor housecleaning. Putback pending a build from RE. Thanks, Brad On 9/12/2012 12:54 PM, Valerie (Yu-Ching) Peng wrote: The enhanced test is run for all platforms, right? However, the particular test for the PKCS11 using Solaris crypto library is platform specific. So, it seems kind of strange to have them in the same test which runs on all platforms. In addition, I don't see a need for testing this UnsatisfiedLinkError actually. You should never encounter this error when running against Solaris crypto impl since they define these PKCS11 entry points. Why testing something always pass? Thanks, Valerie On 09/12/12 11:41, Sean Mullan wrote: Looks good to me. --Sean On 09/12/2012 03:39 AM, Brad Wetmore wrote: Valerie/Sean, Here's the change for JDK8. 7197071: Makefiles for various security providers aren't including the default manifest. I've added the standard/regular rt.jar attributes (see common/Release.gmk) to the remainder of the JCE jars (see javax/crypto/Makefile and com/sun/crypto/provider/Makefile), and my regression test makes sure that the Specification version value lines up with the Implementation Version value. This does have the side effect that when we are transitioning major releases, this test will fail until the providers are rebuilt. This is an acceptable amount of noise (once per release), since that should is an actual failure. One minor addition from the previous version you saw, the reg test has been enhanced, and the Defs-jce.gmk file was updated to remove the -ea string, which was causing some confusion. Thanks, http://cr.openjdk.java.net/~wetmore/7197071/webrev.01/ Brad
Quick Codereview
Valerie/Sean, Here's the change for JDK8. 7197071: Makefiles for various security providers aren't including the default manifest. I've added the standard/regular rt.jar attributes (see common/Release.gmk) to the remainder of the JCE jars (see javax/crypto/Makefile and com/sun/crypto/provider/Makefile), and my regression test makes sure that the Specification version value lines up with the Implementation Version value. This does have the side effect that when we are transitioning major releases, this test will fail until the providers are rebuilt. This is an acceptable amount of noise (once per release), since that should is an actual failure. One minor addition from the previous version you saw, the reg test has been enhanced, and the Defs-jce.gmk file was updated to remove the -ea string, which was causing some confusion. Thanks, http://cr.openjdk.java.net/~wetmore/7197071/webrev.01/ Brad
Re: JDK8 code review request: 7196593: java.security.debug=help doesn't list certpath option
Codewise, it looks ok. You'll want Valerie and Sean to look over this as well for content. Brad On 9/11/2012 12:00 PM, Jason Uh wrote: Please review my fix for 7196593: java.security.debug=help doesn't list certpath option webrev: http://cr.openjdk.java.net/~juh/7196593/webrev.00/ CR: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7196593 Also added other undocumented debug options and fixed broken formatting for the gssloginconfig option. Thanks, Jason
Re: [PATCH] Review request for 7195733 TEST_BUG: sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/B6216082.java failing
On 9/3/2012 3:20 AM, Alan Bateman wrote: On 03/09/2012 11:02, Eric Wang wrote: Hi Chris and Xuelei, Thanks for your advice, I have updated the @run tag and copyright, please help to review. and Thank you to be my sponsor. http://dl.dropbox.com/u/90659131/fixes/7195733/webrev/index.html Looks fine to me too (note that you aren't required to change the copyright date, it doesn't matter if you do or don't but once it can cause clutter in patches. Didn't parse this. Once it can cause clutter in patches? Instead there is supposed to regular batch updating of the copyright dates, something that I don't see very often but is supposed to happen). -Alan
Re: test/java/security/spec/EllipticCurveMatch.java othervm?
On 6/15/2011 2:00 AM, Vincent Ryan wrote: On 06/15/11 09:45, Weijun Wang wrote: Hi Vinnie Why does this test run in /othervm mode? Thanks Max It was failing due to SecureRandom problems on some platforms when run in samevm mode. I think it was related to a lack of entropy. Really? That surprises me! Generally it's when you run in othervm that you you might run out of entropy in /dev/random. Can you reliably duplicate this? Brad
Re: OpenJDK 7 SNI Implementation
Cross-posting to security-dev. Hi Tim, On 8/21/2012 8:07 PM, Tim Gustafson wrote: I see that Java is supposed to support SNI, but it's not clear to me how this happens, or where it happens, or if support for SNI extends only to client SSLSocket object, or if it also applies to SSLServerSocket objects. I can't find any documentation to tell me exactly how Java supports SNI, nor can I find any examples of using SNI, even from the client side of things. We currently only support client side sending of the SNI extension. Our client handshakers look to see if the SNI Extension is enabled (System Property: jsse.enableSNIExtension=true). If so, then if the SSLSocket/SSLEngine was created with a Fully Qualified Domain Name hostname, then we will load that hostname into an RFC 6066 host_name extension [1] and send it as part of the ClientHello. We don't currently have APIs to specify alternate server names on the client side, or to observe the received SNI extensions on the server side. We are right in the middle of designing the APIs for that[2]. We will likely be posting a new version in the next week or so to the security-dev mailing list. I'd like my chooseServerAlias function in my X509KeyManager implementation to pick a server alias based on what server the client is attempting to connect to. But, I can't seem to find any properties that are available through the keyType, issuers or socket parameters that are passed to that method that would tell me which server the client is attempting to connect to. Earlier versions of the APIs are available via the security-dev mail archives[3], but I would suggest waiting for the next iteration. I thought perhaps that I could make my client SSLSocket specify which issuer/subject it was expecting to find on the server (and that information would find its way to the issuers parameter of the chooseServerAlias method), but I can't find any way to tell the client SSLSocket which certificate to expect or which local certificate to offer to the remote server. So, short version: where is Java's support for SNI actually documented in detail? And are there any sample code snippets that would show me how to use SNI? Or is Java's SNI implementation just based on the host name that you specify when creating your client SSLSocket? Yes. If so, where does that host name information show up in the chooseServerAlias function? Working on this for JDK 8. Thanks for any help in advance! Hope this helps, Brad [1] http://www.rfc-editor.org/rfc/rfc6066.txt [2] http://openjdk.java.net/jeps/114 [3] http://mail.openjdk.java.net/pipermail/security-dev/2012-August/005285.html
Re: (2nd round) Proposed API Changes for JEP 114: TLS Server Name Indication (SNI) Extension
On 8/15/2012 3:26 AM, Xuelei Fan wrote:
More comments about whether we are able to override the default value.
On 8/15/2012 10:45 AM, Xuelei Fan wrote:
Thought more about the design, I would have to say that we cannot return
the default value in sslParameters.getServerNames(). Otherwise, the
following two block of codes look very weird to me:
// case one:
1 SSLparameters sslParameters = sslSocket.getSSLParameters();
2 sslParameters.clearServerName(host_name);
3 MapString, String names = sslParameters.getServerNames();
4 sslSocket.setSSLParameters(sslParameters);
5 sslParameters = sslSocket.getSSLParameters();
6 names = sslParameters.getServerNames();
In line 3, the returned map does not contain host_name entry. But in
line 6, it may be expected that no host_name in the returned map. But
if we want to return default values, line 6 do need to return a map
containing host_name. The behavior is pretty confusing. We may want
to try avoid the confusion.
I'm not following your confusion, it seemed pretty straightforward to
me, it works much like CipherSuites. We have a set of ciphersuites
which are enabled by default. We can turn some off by using
SSLParameters.
Compatibility is my concerns here.
When the SSLParameters class was was introduced in JDK 6,
set/getCipherSuites() and set/getProtocols were new, so there was no
compatibility issue for these two pairs methods at that time, as they
were not used in old applications.
In JDK 7, we introduced two new methods, set/getAlgorithmConstraints().
We were luck that we cannot allow the override of default constraints
because of security consideration,
I didn't quite follow this sentence. We do allow the override, right?
Looking at SSLSocketImpl.setSSLParameters():
algorithmConstraints = params.getAlgorithmConstraints();
and the concept of algorithm
constraints was new in JDK 7. So we needed not to consider the
compatibility issue too much between JDK 6 and 7 for this pair of methods.
However, things get changed for the Server Name Inidication (SNI),
because in JDK 7 we have already support default SNI extension implicit.
We have to consider the compatibility issues more between JDK 7 and JDK
8, we need to make sure the behaviors are consistent whenever we call
the set/getServerName(). If we allow override of default value, the
application may run into corner trap as the description in my previous mail.
Not sure this is a problem, possibly I didn't explain my thought fully.
That's the background of my thoughts. Hope it helps you understand the
current design.
Here's what I was trying to propose:
SSLParameters:
// Local storage like the other variables
private MapString, String sniNames = null;
...deleted...
/**
* Set the SNI Names.
*
* A null parameter means don't set anything. Nothing is set.
*
* disallow invalid String-null entries.
*
* valid String-String entries will be added to SNI extension
* if there is a recognized Key type.
*/
public void setServerNames(MapString, String map)
/**
* Returns a copy of the array of servernames or null if
* none have been set.
*/
public MapString, String getServerNames()
SSLSocketImpl:
synchronized public void setSSLParameters(SSLParameters params) {
super.setSSLParameters();
...deleted...
MapString,String sniNames = params.getServerNames();
if (sniNames != null) {
sniNameStorage = sniNames.clone();
}
synchronized public SSLParameters getSSLParameters() {
SSLParameters params = super.getSSLParameters();
...deleted...
// sniNameStorage currently has one entry:
// host_name, www.example.com
params.setServerNames = sniNameStorage.clone();
So looking at the two use cases you pointed out:
1. SSLParameters sslp = new SSLParameters()
getServernames() would return null. When SSLSocketImpl.setSSLParameters
is called, the null indicates there is nothing to set, and the default
value remains. If app wants to set something here, it can, which would
override the existing default.
2. SSLParameters sslp = SSLSocket.getSSLParameters()
The SSLParameters will be populated with the SNI map value stored in
SSLSocketImpl/SSLEngineImpl, and apps can do whatever they choose with
the Map. When setSSLParameters is called, the potentially new values
are just pushed back to the SNI. In this case, the application has full
access to the default Map and can see what will be sent by default.
In both cases, the default remains unless the application takes steps to
change it. For applications, this is the same call model as
setCipherSuites()/setProtocols(), but the magic happens at different levels.
Am I missing something?
because in JDK 7 we have already support default SNI extension
implicit.
We are just providing API access to that functionality.
Brad
Thanks,
Xuelei
Re: (2nd round) Proposed API Changes for JEP 114: TLS Server Name Indication (SNI) Extension
On 8/12/2012 5:52 AM, Xuelei Fan wrote:
I revised the APIs to separate the differnt server name values and will
send it in another thread.
http://cr.openjdk.java.net./~xuelei/7068321/webrev_spec.04/
http://cr.openjdk.java.net./~xuelei/7068321/README_04.txt
Only reply on those items we may have different opinions.
I'll address the 3rd Round's CR in another email. This will contain
just responses to your comments to keep the message flow in the archives.
On 8/10/2012 8:37 AM, Brad Wetmore wrote:
SSLParameters.java
==
One general question before we get to specifics. Your current default
behavior of the SunJSSE is to add a SNI extension if we have the value
available. So if we call:
sslSocket = socketFactory.createSocket(www.example.com, 443);
sslp = sslParameters.getSSLParameters();
will this sslParameters ever contain a map with preinstalled host_name
set to www.example.com, or will it be empty? I think the answer will
be empty. This API is just a way to force setting the value if an
implementation select an unwanted value.
No, it is not empty. The default value will appear in the SSLParameters
in my prototype implementation.
That's good to know, that will help make things much easier to
understand, rather than a null value which is interpreted in a
undefined way underneath. I'll keep this in mind in the next review.
Although I just found your second email. Talk more there.
But that's an interesting topic to discuss as there are a few concerns I
have to consider when I design the APIs.
There are public SSLParameters constructors. As means that an instance
of SSLParameters is not always got from a SSLSocket or SSLEngine
instance. Then we are not always able to have the *default* value of an
instance of SSLSocket/SSLEngine into SSLParameters if it is not bound to
SSLSocket/SSLEngine. So we still need to discuss about how the user
specified values work with the default ones.
SSLParameters sslParameters = new SSLParameters();
...
sslSocket.setSSLParameters(sslParameters);
We have the same situation with
ciphersuites/protocols/endpointAlgs/AlgorithmConstraints. I was
expecting you'd do the same thing as the last two, that is, they're
interpreted at the SSLSocketImpl level. If they're null (not set), then
something similar needs to happen here. I'll also keep that in mind in
the next review.
76: Not sure why you want/need a LinkedHashMap with only one currently
defined NameType. Even if there were multiple types, I don't think that
SNI requires an ordering. You also mention this in
setAccessibleServerName, but not sure what purpose this serves. I'm not
strongly against it, just wondering.
I am also not sure about the strong desire that the SNI should be
ordered. But Weijun prefers it to be ordered because he think the SNI in
RFC6066 is defined as an ordered sequence.
struct {
ServerName server_name_list1..2^16-1
} ServerNameList;
I've gone through RFC6066 pretty carefully, and I'm not seeing any
indication that this should be ordered.
In RFC 2246, if there is an ordering required, such as
cipher_suites/compression/certs/cert_requests, it's specifically called
out. For any other lists, it is not specified.
Section 7.4.1.2
The CipherSuite list, passed from the client to the server in the
client hello message, contains the combinations of cryptographic
algorithms supported by the client in order of the client's
preference (favorite choice first).
...deleted...
The client hello includes a list of compression algorithms supported
by the client, ordered according to the client's preference.
...deleted...
cipher_suites
This is a list of the cryptographic options supported by the
client, with the client's first preference first.
...deleted...
compression_methods
This is a list of the compression methods supported by the
client, sorted by client preference.
Section 7.4.2
certificate_list
This is a sequence (chain) of X.509v3 certificates. The sender's
certificate must come first in the list.
Section 7.4.4
certificate_types
This field is a list of the types of certificates requested,
sorted in order of the server's preference.
Weijun, did you see something else in your read of the spec that
indicates an ordering? If not, maybe we should not put in the order
wording now. If it turns out we do need it, we can always add that
wording later in a later release, but it will be impossible to remove it
if we add it now.
295: So what does value = mean now? Will that send a SNI extension
with an empty string, or will no SNI entry be sent? If null is passed,
then it's up to the provider to decide what to do, but there doesn't
seem to be a way to shut off SNI like in your old proposal.
The APIs was revised. But even in the new APIs, it is still a concern
Re: (3rd Round) Proposed API Changes for JEP 114: TLS Server Name Indication (SNI) Extension
Still looking over the changes, but a few preliminary comments.
On 8/12/2012 5:50 AM, Xuelei Fan wrote:
Hi,
Please review the spec of JEP 114, TLS Server Name Indication (SNI)
Extension.
http://cr.openjdk.java.net./~xuelei/7068321/webrev_spec.04/
Please read the README to help you understanding the the specification:
http://cr.openjdk.java.net./~xuelei/7068321/README_04.txt
I forgot to mention this previously:
Oracle provider now only supports one server name type, host_name.
The value of name is encoded as UTF-8 string.
RFC 6066 requires ASCII, earlier versions (RFC 4366) have this as UTF-8.
Do you want to allow for UTF-8 here?
The major differences comparing with previous webrev are:
1. client mode and server mode will use separated API set.
For client, the related APIs are:
setServerName(String type, String value)
clearServerName(String type)
disableServerName(String type)
enableServerName(String type)
isDisabledServerName(String type)
getServerNames()
Please read my note on the 2nd round review before reading this,
specifically the section that starts I'm not following your confusion.
So wow, what happened here? I thought we were so very close on
finalizing the API, and it was just a matter of tweaking the wording to
have a null value disable the type from being sent.
setServerName(String type, String value) {
if type == value
throw IAE;
if value == null
map.remove(type);
return;
else if value.equals()
throw IAE;
else
map.put(value);
}
Then in the Handshakers, only those type/values that are present in the
map are be pulled out for constructing the SNI extensions.
If you want to go with this new API style with clear/disable/enable, I
can see where you are coming from, but that was unexpected. Before I
review for accuracy, I want to make sure you really want to tweak things
so extensively. I think what you had previously fit the bill pretty well.
For server side, the related APIs are:
setServerNamePattern(String type, Pattern pattern)
clearServerNamePattern(String type)
getServerNamePatterns()
and same for this one.
I'll look over the rest once I get your answer to the above.
2. close the door to use the generated socket in client mode.
SSLSocketFactory.createSocket(Socket s,
InputStream consumed, boolean autoClose)
The returned socket was set in server mode.
Brad
Re: Code review request: 7190945: pkcs11 problem loading NSS libs on Ubuntu
Vinnie, I'm cleaning out email from vacation, and saw the following new bug which sounds similar. 7189635 PKCS11 tests failed on Ubuntu due to nss libs path is not same as assumed in test Is this the same bug as 7189635? It's currently unassigned. Brad On 8/13/2012 3:39 AM, Vincent Ryan wrote: Please review these changes to correct a problem loading Mozilla NSS on multi-arch Ubuntu: http://cr.openjdk.java.net/~vinnie/7190945/webrev.00/ Thanks.
Re: (2nd round) Proposed API Changes for JEP 114: TLS Server Name Indication (SNI) Extension
Hi Xuelei, I like this style much better than your earlier versions. Thanks for considering them. We should still do a little more wordsmithing, some of the passages are a little awkward. The following are mainly content questions. ExtendedSSLSession.java === 91: Having the SNI acronym will help people who are specifically looking for this option. (i.e. find) Suggest updating to caps and include acronym. values of the Server Name Indication (SNI) capability. 94: to to-to 98: of enabled server name-of requested server name 99: SNI SSLParameters.java == One general question before we get to specifics. Your current default behavior of the SunJSSE is to add a SNI extension if we have the value available. So if we call: sslSocket = socketFactory.createSocket(www.example.com, 443); sslp = sslParameters.getSSLParameters(); will this sslParameters ever contain a map with preinstalled host_name set to www.example.com, or will it be empty? I think the answer will be empty. This API is just a way to force setting the value if an implementation select an unwanted value. But here's my point. These are brand new methods. Have you considered just having the SSLSocket.getSSLParameters() return the already fully populated SNI field, that is, the SSLSocketImpl's default values are already set in the SSLParameters? That way, if someone sets value to null, then we know they don't want the provider to send SNI fields of that type. e.g. sslp = sslParameters.getSSLParameters(); // sslp has host_name already set to www.example.com sslp.setAccessibleServerName(host_name, null); this makes it less confusing in that you get rid of all that ambigious default discussions in your API, and the user can easily find out exactly what will be sent. Something to consider. 72: Might suggest minor rewording: // As hostname is the only known server name indication type, to // As host_name is the only known Server Name Indication (SNI) type, RFC 6066 uses host_name and as previously mentioned, having the SNI acronym will help people who are specifically looking for this. (i.e. find) 76: Not sure why you want/need a LinkedHashMap with only one currently defined NameType. Even if there were multiple types, I don't think that SNI requires an ordering. You also mention this in setAccessibleServerName, but not sure what purpose this serves. I'm not strongly against it, just wondering. 271: Same comment in the APIs about SNI acronym. 295: So what does value = mean now? Will that send a SNI extension with an empty string, or will no SNI entry be sent? If null is passed, then it's up to the provider to decide what to do, but there doesn't seem to be a way to shut off SNI like in your old proposal. 308: What kinds of things are you thinking of writing in the Additional Standard Names doc for value? Would it be sufficient to just say this is the SNI hostname value? 365: alternatice - alternate 368: www\\.example\\.com|www\\.example\\.org - www\\.example\\.(com,org) This is a little more illuminating of the power of these patterns. Even though you have a link to java.util.regex.Pattern in the actual API, you might also mention here in textual form that these are java.util.regex patterns, and add a @link java.util.regex. Makes it more clear where to go to learn about how to specify patterns. 379: So what does mean now? Will an empty string SNI extension be accepted? Or we will 379: will not try to recognize the server name value... I'm thinking of IETF's WILL/SHALL/MAY here. What do you mean by will not try? Do you mean that if this is null, we will not use the SNI extension of this type? Or that we could possibly try? 387: This is a pattern and not a String, so only listing setAccessibleServerName() isn't quite complete. You might also list and also see the Pattern page for details on specifying regular expression patterns. SSLSocketFactory.java = 187: I would suggest following the style in the SocketFactory page which describes the functionality differences in the various createSocket methods. A new second paragraph really should give some motivation for this method. allow for inspection of inbound data, then provide that data to the SSLSocket's normal IO streams in the consumed variable, etc. Since you're leaving the door open for this socket being either client or server, shouldn't the API then be similar to the existing layered socket one, just including the additional InputStream here: public abstract Socket createSocket(Socket s, InputStream is, String host, int port, boolean autoClose) We might be able to glean SNI information from the host here. 171: Interesting, the existing layered createSocket doesn't mention anything about whether client or server mode, and that you must set the mode or suffer with the
Re: Please Review Test Fix of Bug 7177045
Sorry for the delay Dan. Please work with Kurchi to get this pushed. On 7/5/2012 1:38 PM, Dan Xu wrote: Hi Brad, Thanks for your good suggestions. I have fixed most of them and re-uploaded my changes at http://cr.openjdk.java.net/~dxu/7177045.01/. The reason that I chose ArrayDeque instead of LinkedList is that ArrayDequeseems have better performance. According to the java doc, most ArrayDeque operations run in amortized constant time and this class is likely to be faster then LinkedList when used as a queue. It is also very easy to remove last elements to back off memory allocation. Great, thanks for the pointer. In addition, I did not switch to diamond operator. Because old Jdk bundles, say jdk 1.7.0-ea-b23 and jdk 1.7.0-ea-b29 used in my testing, failed to compile diamond operator. Here are the compilation error messages, TestProviderLeak.java:62: illegal start of type Dequebyte [] data = new ArrayDeque(); ^ 1 error I guess those jdk might be too early to adopt the diamond operator changes. I am not sure whether we still take these old jdk bundles into consideration here. Thanks! Yes, it would be good to actually show that it failed on b23 and not on b29 without having to modify the source. Good point. Minor nit: line 38 has a space at the end of the line. Current jstyle guidelines state no indention with tabs and no whitespace at the end of the lines. Looks good. Suggest more liberal use of comments, either in the method's comments or inline. Good to explain your assumptions/approach in case things aren't obvious. For example, why do you backoff 3MB after allocating available memory? And at line 134: the operation could either time out or threw an exception. Nice to make that clear. Thanks! Line 114: consider adding a @overrides annotation on the call() method. Good. Line 139: I'm being paranoid here, but shutdownNow doesn't guarantee threads will be stopped. If we actually got into a situation where there was a timeout, executor.shutdownNow() *may* never return. One reason is it might be hanging somewhere waiting for memory. I would suggest as part of your finally block, you dequeue all the memory in dummyData, call System.gc(), then run executor.shutdownNow(). JTREG will timeout after two minutes, but if we can proactively help the situation, we might as well. Otherwise, looks good. We'll wait to see if anyone has other thoughts, and if not, we'll push when you're back from vacation. Thanks. Brad
Re: Code review request, CR 7180038 regression test failure, SSLEngineBadBufferArrayAccess.java
. I think it is a test issue, the current fix should has already addressed the issue. That's great. Since the reason is clear, can you reproduce this failure and then confirm the current fix does solve the problem? It is possible to reproduce this failure with a new test case. But it is pretty hard to reproduce it within this test. I was wondering as it is test bug, so we may not want a extra test case to prove that this test is correct. We also have a nested remind that, IF THIS FAILS, PLEASE REPORT THIS TO THE SECURITY TEAM. WE HAVE BEEN UNABLE TO RELIABLY DUPLICATE. I think it might be OK that we do not reproduce this issue at present. What do you think? Thanks, Xuelei Thanks Max Xuelei Thanks Max On 07/02/2012 10:39 AM, Xuelei Fan wrote: Hi Weijun, Would you please review the test update for CR 7180038? http://cr.openjdk.java.net./~xuelei/7180038/webrev.00/ We cannot reproduce the issue. However, from the test log, there is two possible issues exposed by this CR. 1. the improper test case senarios of un/wrap() In the test case, the scenarios is unwrap()-wrap()-serverOut.remaining()-serverIn.remaining() != clientMsg.length. After the wrap(), the next operation may need to be unwrap() to get more incoming data before comparing serverIn buffer with the expected client message. This fix is trying to do the comparing after the engine has closed. 2. From the log, the engine status and handshaking status move from CLOSED/NOT_HANDSHAKING to OK/FINISHED. FINISHED means the TLS handshaking just finished. As the handshaking should have completed for a while, it does not sound like a correct status change. However, I did not find why this happens. Need more info. So I added a line of log (suggested by Brad Wetmore) to collect the next failure: IF THIS FAILS, PLEASE REPORT THIS TO THE SECURITY TEAM. WE HAVE BEEN UNABLE TO RELIABLY DUPLICATE. Thanks, Xuelei
Re: Please Review Test Fix of Bug 7177045
Dan, congrats on assembling and posting your first webrev. Besides the big picture things, since you are new, I'll also be looking for minor things that you may or may not know yet. On 6/28/2012 1:49 PM, Dan Xu wrote: Security code reviewers, I have fixed a security test failure and posted my changes at http://cr.openjdk.java.net/~dxu/7177045/. Please help review it. Thanks! Minor nit: line 38 has a space at the end of the line. Current jstyle guidelines state no indention with tabs and no whitespace at the end of the lines. Lines 61/89: memroy-memory Just wondering why you chose a Deque instead of a simpler LinkedList? Suggest more liberal use of comments, either in the method's comments or inline. Good to explain your assumptions/approach in case things aren't obvious. For example, why do you backoff 3MB after allocating available memory? And at line 134: the operation could either time out or threw an exception. Nice to make that clear. dummyData could be a local variable. Line 64/113: consider using the JDK 7 diamond operator on your generics. Line 114: consider adding a @overrides annotation on the call() method. Line 139: I'm being paranoid here, but shutdownNow doesn't guarantee threads will be stopped. If we actually got into a situation where there was a timeout, executor.shutdownNow() *may* never return. One reason is it might be hanging somewhere waiting for memory. I would suggest as part of your finally block, you dequeue all the memory in dummyData, call System.gc(), then run executor.shutdownNow(). JTREG will timeout after two minutes, but if we can proactively help the situation, we might as well. Otherwise, looks good. We'll wait to see if anyone has other thoughts, and if not, we'll push when you're back from vacation. Brad
Codereview for 7177556
This test has started failing in JDK nightly. We were working on it this week, but test approach may need alteration. Putting this test on ProblemList.txt for the time being to quiet the test noise. 7177556: Put TestProviderLeak.java on the ProblemList until test can be reworked http://cr.openjdk.java.net/~wetmore/7177556/ Brad
Re: Code review request for 7172149 ArrayIndexOutOfBoundsException from Signature.verify
Looks good to me, however, we are still checking on the copyright notice in the test. I think what you have is ok, but want to run it by one other person. Hope to hear back soon. The bug presents an interesting and obvious problem when you think of it. Wonder how many things we have like that elsewhere. Another thing to keep in the back on my mind for future codereviews. Thanks, Brad On 5/31/2012 11:25 PM, Jonathan Lu wrote: Hello Xuelei, What do you think of the updated patch? any comments? http://cr.openjdk.java.net/~luchsh/7172149_2/ Thanks - Jonathan On 05/30/2012 07:26 AM, Xuelei Fan wrote: On 5/29/2012 3:11 PM, Jonathan Lu wrote: Hi Xuelei, Thanks for review! On 05/29/2012 02:45 PM, Xuelei Fan wrote: That's an interesting topic. From my understand, the length of an array is of type int. So normally, the (offset + length) should not be great than integer.max_value. Of course, Hostile or improper code are not of the case. What's interesting to me is that may be when we do additive operation for two int values, we may have to convert it to long in case of any overflow strictly. We are luck here because we have long type. But what about the additive operation for two long values I think this issue is special, since it is about index value of Java arrays, which is limited to smaller than Integer.MAX_VALUE according to Java language specification, not other general conditions of comparing integer or long values. Jonathan, do you run into the problem in real world? For now I am not quiet sure of whether it is from a real world problem, but this problem does exhibit some weakness or behavior differences, right? Yes, it is an improvement. Would you please add a comment about why convert it to long, and update the copyright year to 2012? Otherwise, looks fine to me for JDK 8. Thanks Regards, Xuelei Thanks regards -Jonathan Thanks Regards, Xuelei On 5/29/2012 1:53 PM, Jonathan Lu wrote: Hi Security-dev, Here's a patch for bug7172149, could anybody please help to take a look? http://cr.openjdk.java.net/~luchsh/7172149/ The problem is that the range check in Signature.verify(byte[], int, int) uses integer value to check whether (offset + length) is greater than signature.length, but if (offset + length) overflows the check will fail and ArrayIndexOutOfBoundsException will be thrown instead of IllegalArgumentException.My proposed solution is to make a conversion to long in the if block. Thanks! - Jonathan
Re: Code review request for 7172149 ArrayIndexOutOfBoundsException from Signature.verify
I think it is worth exploring what other parts of the code do in this case. It seems to me this is going to be a lot more involved than just Signatures. (InputStreams, etc) Brad On 5/29/2012 4:26 PM, Xuelei Fan wrote: On 5/29/2012 3:11 PM, Jonathan Lu wrote: Hi Xuelei, Thanks for review! On 05/29/2012 02:45 PM, Xuelei Fan wrote: That's an interesting topic. From my understand, the length of an array is of type int. So normally, the (offset + length) should not be great than integer.max_value. Of course, Hostile or improper code are not of the case. What's interesting to me is that may be when we do additive operation for two int values, we may have to convert it to long in case of any overflow strictly. We are luck here because we have long type. But what about the additive operation for two long values I think this issue is special, since it is about index value of Java arrays, which is limited to smaller than Integer.MAX_VALUE according to Java language specification, not other general conditions of comparing integer or long values. Jonathan, do you run into the problem in real world? For now I am not quiet sure of whether it is from a real world problem, but this problem does exhibit some weakness or behavior differences, right? Yes, it is an improvement. Would you please add a comment about why convert it to long, and update the copyright year to 2012? Otherwise, looks fine to me for JDK 8. Thanks Regards, Xuelei Thanks regards -Jonathan Thanks Regards, Xuelei On 5/29/2012 1:53 PM, Jonathan Lu wrote: Hi Security-dev, Here's a patch for bug7172149, could anybody please help to take a look? http://cr.openjdk.java.net/~luchsh/7172149/ The problem is that the range check in Signature.verify(byte[], int, int) uses integer value to check whether (offset + length) is greater than signature.length, but if (offset + length) overflows the check will fail and ArrayIndexOutOfBoundsException will be thrown instead of IllegalArgumentException.My proposed solution is to make a conversion to long in the if block. Thanks! - Jonathan
Minor Codereview.
Sherman, Can you please review: http://cr.openjdk.java.net/~wetmore/7167362/ This is for: 7167362: SecureRandom.init should be converted, amendment to 7084245 http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7084245 This is the bug where there were an instance of exception chaining that got missed in one spot in: 7084245: Update usages of InternalError to use exception chaining http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7084245 I was down in this code for something related and noticed it. Thanks, Brad
Re: code review request, CR 7167092 Need to put the return clause in the synchronized block
Looks fine. Thanks. Brad On 5/8/2012 7:45 AM, Xuelei Fan wrote: Hi, Please review the fix for a synchronization issue of JSSE. webrev: http://cr.openjdk.java.net/~xuelei/7167092/webrev.00/ Cause of the issue: need to put the return clause in the synchronized block. It is a regression of CR 7153184. Thanks, Xuelei
Re: code review request, CR 7166570: JSSE certificate validation has started to fail for certificate chains
Hi Xuelei, I walked through the code in the debugger, it looks good. Just a comment for the regression test, it might have been easier and likely faster performance-wise to simply create Simple and PKIX trustmanagers, and then directly call checkClientTrusted and passing a predefined chain, rather than incurring the overhead of SSL, much of what we cover in many other tests. Brad On 5/5/2012 8:06 AM, Xuelei Fan wrote: The webrev URL should be: http://javaweb.us.oracle.com/~xufan/bugbios/7166570/webrev.00/ Xuelei On 5/5/2012 9:38 PM, Xuelei Fan wrote: Hi, Please review the fix for JSSE certification path validation issue. webrev: http://javaweb.us.oracle.com/~xufan/bugbios/7166570/webrev/ Cause of the issue: in SimpleValiadtor, the count pathLenConstraint was not calculated properly, the non-intermediate certificate (end-entity certificate) was count in. Thanks, Xuelei
Re: Code review request, CR 7158688: Typo in SSLContext Spec
Looks fine. Thanks for fixing these obvious typos! Brad On 4/27/2012 2:53 AM, Xuelei Fan wrote: Hi, Please review the typo correction in SSLContext specification. Webrev: http://cr.openjdk.java.net/~xuelei/7158688/webrev.00/ Thanks, Xuelei
Re: 答复: Asynchronous IO in SSL
security-dev is not a general purpose mailing list for Java Programming/usage questions, it's for the discussion of development of the JDK. A couple pointers/suggestions: In the JDK distribution, there is some sample code which shows NIO/SSLEngine working together. sample/nio/server The majority of the SSLEngine code is ChannelIOSecure.java. It is not production quality, but gives some ideas of how it could be used. Please see the SSLEngine section in the JSSE documentation. http://docs.oracle.com/javase/7/docs/technotes/guides/security/jsse/JSSERefGuide.html#SSLENG Also, please check the user forums for JSSE: https://forums.oracle.com/forums/forum.jspa?forumID=965 Best wishes, Brad On 4/12/2012 2:54 AM, lixiangfeng wrote: Thanks your reply. I find some description in this PPT,it can help me to write ssl/tls implements use sslengine and aio. Is there some exsample and test code can show me how to use sslengine with aio? I think ,oracle (or openjdk group should test their api use java code. It is useable to me. Is their anyone can help me? *发件人:*Xuelei Fan [mailto:xuelei@oracle.com] *发送时间:*2012年3月31日16:49 *收件人:*lixiangfeng *抄送:* *主题:*Re: Asynchronous IO in SSL On Mar 31, 2012, at 4:26 PM, lixiangfeng lixiangf...@infosec.com.cn mailto:lixiangf...@infosec.com.cn wrote: I use JDK7,use SSL ServerSocket to accept a SSL Socket Connection,Can I use *Asynchronous http://docs.oracle.com/javase/7/docs/api/java/nio/channels/AsynchronousServerSocketChannel.html#AsynchronousServerSocketChannel%28java.nio.channels.spi.AsynchronousChannelProvider%29 IO?* I am afraid that It is not the normal approach to use asynchronous IO. The server socket may be the bottleneck. You may be interested in the blog: http://sim.ivi.co/2011/06/java-approach-to-lightweight-servers.html, which talked about how to use NIO2 and SSL together. Xuelei
Code review request: 7157903: JSSE client sockets are very slow
Hi Xuelei, Here's the webrev for for JDK7u and JDK8. http://cr.openjdk.java.net/~wetmore/7157903/ These should be identical to versions you've seen before. 7157903: JSSE client sockets are very slow http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7157903 Thanks, Brad
Re: 7142172: Custom TrustManagers that return null for getAcceptedIssuers will NPE
Reposted to same location under .01. Brad On 3/29/2012 7:42 PM, Brad Wetmore wrote: Thanks, I won't regenerate the webrev unless someone really wants to see it. Brad On 3/29/2012 7:30 PM, Xuelei Fan wrote: Forgot one thing about the regression test. JSSE test should run in othervm. Would you please add the following lines to the test: * SunJSSE does not support dynamic system properties, no way to * re-use system properties in samevm/agentvm mode. * @run main/othervm NullGetAcceptedIssuers * Thanks, Xuelei On 3/30/2012 10:26 AM, Xuelei Fan wrote: Looks fine to me. Thanks, Xuelei On 3/30/2012 10:20 AM, Brad Wetmore wrote: Hi Xuelei, Can you please rereview this bug, the 7u4 and 8 changes are here: http://cr.openjdk.java.net/~wetmore/7142172/ Thanks, Brad
Re: 7142172: Custom TrustManagers that return null for getAcceptedIssuers will NPE
Thanks, I won't regenerate the webrev unless someone really wants to see it. Brad On 3/29/2012 7:30 PM, Xuelei Fan wrote: Forgot one thing about the regression test. JSSE test should run in othervm. Would you please add the following lines to the test: * SunJSSE does not support dynamic system properties, no way to * re-use system properties in samevm/agentvm mode. * @run main/othervm NullGetAcceptedIssuers * Thanks, Xuelei On 3/30/2012 10:26 AM, Xuelei Fan wrote: Looks fine to me. Thanks, Xuelei On 3/30/2012 10:20 AM, Brad Wetmore wrote: Hi Xuelei, Can you please rereview this bug, the 7u4 and 8 changes are here: http://cr.openjdk.java.net/~wetmore/7142172/ Thanks, Brad
Re: Code Review Request for 7146728 and 7130959
Minor comment:
// Array too short, pad it w/ leading 0s including the sign bit
Can you pull off: including the sign bit
Looks good to me otherwise.
Brad
On 3/16/2012 1:42 PM, Valerie (Yu-Ching) Peng wrote:
Brad,
As I was preparing the files for putting back, I noticed some
now-redundant code in DHKeyAgreement2.java as well as one minor issue
about resetting the key agreement object in the
engineGenerateSecret(...) method of DHKeyAgreement.java.
Hope this to be the last version of the webrev:
http://cr.openjdk.java.net/~valeriep/7146728/webrev.04/
Please let me know if you have any more comment.
Thanks!
Valerie
On 03/14/12 16:54, Valerie (Yu-Ching) Peng wrote:
Sure, that's good clarification.
I'll add the suggested wording and put back the changes.
Thanks,
Valerie
On 03/14/12 16:01, Brad Wetmore wrote:
Here's the comment I was trying to suggest in your office yesterday:
/*
* NOTE: BigInteger.toByteArray() returns a byte array containing
* the two's-complement representation of this BigInteger with
* the most significant byte in the zero-th element. This
* contains the minimum number of bytes required to represent
* this BigInteger, including at least one sign bit whose value
* is always 0.
*
* Keys are always positive, and the above sign bit isn't
* actually used when representing keys. (i.e. key = new
* BigInteger(1, byteArray)) To obtain an array containing
* exactly expectedLen bytes of magnitude, we strip any extra
* leading 0's, or pad with 0's in case of a short secret.
*/
Hope this helps,
Brad
On 3/9/2012 12:57 PM, Valerie (Yu-Ching) Peng wrote:
Ok, more comments/changes added as suggested.
Webrev updated at:
http://cr.openjdk.java.net/~valeriep/7146728/webrev.02/
Thanks,
Valerie
On 03/08/12 17:15, Brad Wetmore wrote:
DHKeyAgreement:
===
I added more comments to both code blocks to explain what's going
on.
A couple more requested comments.
Around line 299, could please add that the sign bit is always 0.
Around line 303, could you put in a comment that since there is enough
room here for the sign bit in this array, it just becomes a leading
zero.
Good point, I modified the engineGenerateSecret() to leverage
engineGenerateSecret(byte[], int) method.
Great, thanks!
P11KeyAgreement.java
// Solaris
Shouldn't this be // Solaris/NSS, for the case in which there
isn't
a leading zero?
Well, I think putting NSS in both places seems somewhat confusing.
What
I tried to point out is that NSS (at least in the older versions,
I have
not tried the newer version to see if they switched behavior)
trims off
the leading 0s whenever possible. I've made some modification
hoping to
make it clearer.
Much better, thanks!
Minor nit (take or leave) you could adjust the logic around 208 to
only create newSecret if secret.length secretLen.
if (secret.length secretLen) {
throw new ProviderException(...
}
byte [] newSecret = new byte[secretLen];
System.arraycopy(...);
return newSecret;
TestShort.java
==
New bugid
@bug 4942494 7146728
Done.
Indention problem around the try.
Fixed.
Thanks for taking out the commented out code, I almost mentioned it
and then didn't.
Brad
Re: Code Review Request for 7146728 and 7130959
Here's the comment I was trying to suggest in your office yesterday:
/*
* NOTE: BigInteger.toByteArray() returns a byte array containing
* the two's-complement representation of this BigInteger with
* the most significant byte in the zero-th element. This
* contains the minimum number of bytes required to represent
* this BigInteger, including at least one sign bit whose value
* is always 0.
*
* Keys are always positive, and the above sign bit isn't
* actually used when representing keys. (i.e. key = new
* BigInteger(1, byteArray)) To obtain an array containing
* exactly expectedLen bytes of magnitude, we strip any extra
* leading 0's, or pad with 0's in case of a short secret.
*/
Hope this helps,
Brad
On 3/9/2012 12:57 PM, Valerie (Yu-Ching) Peng wrote:
Ok, more comments/changes added as suggested.
Webrev updated at:
http://cr.openjdk.java.net/~valeriep/7146728/webrev.02/
Thanks,
Valerie
On 03/08/12 17:15, Brad Wetmore wrote:
DHKeyAgreement:
===
I added more comments to both code blocks to explain what's going on.
A couple more requested comments.
Around line 299, could please add that the sign bit is always 0.
Around line 303, could you put in a comment that since there is enough
room here for the sign bit in this array, it just becomes a leading zero.
Good point, I modified the engineGenerateSecret() to leverage
engineGenerateSecret(byte[], int) method.
Great, thanks!
P11KeyAgreement.java
// Solaris
Shouldn't this be // Solaris/NSS, for the case in which there isn't
a leading zero?
Well, I think putting NSS in both places seems somewhat confusing. What
I tried to point out is that NSS (at least in the older versions, I have
not tried the newer version to see if they switched behavior) trims off
the leading 0s whenever possible. I've made some modification hoping to
make it clearer.
Much better, thanks!
Minor nit (take or leave) you could adjust the logic around 208 to
only create newSecret if secret.length secretLen.
if (secret.length secretLen) {
throw new ProviderException(...
}
byte [] newSecret = new byte[secretLen];
System.arraycopy(...);
return newSecret;
TestShort.java
==
New bugid
@bug 4942494 7146728
Done.
Indention problem around the try.
Fixed.
Thanks for taking out the commented out code, I almost mentioned it
and then didn't.
Brad
Re: Code Review Request for 7146728 and 7130959
DHKeyAgreement:
===
I added more comments to both code blocks to explain what's going on.
A couple more requested comments.
Around line 299, could please add that the sign bit is always 0.
Around line 303, could you put in a comment that since there is enough
room here for the sign bit in this array, it just becomes a leading zero.
Good point, I modified the engineGenerateSecret() to leverage
engineGenerateSecret(byte[], int) method.
Great, thanks!
P11KeyAgreement.java
// Solaris
Shouldn't this be // Solaris/NSS, for the case in which there isn't
a leading zero?
Well, I think putting NSS in both places seems somewhat confusing. What
I tried to point out is that NSS (at least in the older versions, I have
not tried the newer version to see if they switched behavior) trims off
the leading 0s whenever possible. I've made some modification hoping to
make it clearer.
Much better, thanks!
Minor nit (take or leave) you could adjust the logic around 208 to only
create newSecret if secret.length secretLen.
if (secret.length secretLen) {
throw new ProviderException(...
}
byte [] newSecret = new byte[secretLen];
System.arraycopy(...);
return newSecret;
TestShort.java
==
New bugid
@bug 4942494 7146728
Done.
Indention problem around the try.
Fixed.
Thanks for taking out the commented out code, I almost mentioned it and
then didn't.
Brad
Re: Code Review Request for 7146728 and 7130959
Looks good, better to be explicit about these values than prepend to an unknown list. Thanks, Brad On 3/6/2012 7:37 PM, Valerie (Yu-Ching) Peng wrote: BTW, have you looked at the fixes of 7130959? * 7130959: Tweak 7058133 fix for JDK 8 (javah makefile changes) o http://cr.openjdk.java.net/~valeriep/7130959/webrev.00/ Instead of using the -Xbootclasspath, switching over to use -boothclasspath for consistency with the backported changes in the update releases for earlier JDK, e.g. 7u. Thanks, Valerie On 03/06/12 19:19, Valerie (Yu-Ching) Peng wrote: Brad, Thanks for the review. Please find the update webrev at: http://cr.openjdk.java.net/~valeriep/7146728/webrev.01/ On 03/02/12 17:31, Brad Wetmore wrote: DHKeyAGreement.java === Mainly just a request for some additional commenting so the reader doesn't have to have a good understanding of the BigInteger logic to understand the assumptions being made here: For example, if the expected bits of the modulus is 1024 bits (128 bytes exactly) secret-magnitude secret.length action = == 1024 1025 bits (129 bytes) trim front byte 1023 1024 bits (128 bytes) return (sign bit (always 0) becomes leading 0) ... 1015 1016 bits (127 bytes) shift to end of array (implied leading 0's) ... I added more comments to both code blocks to explain what's going on. In the second bit of code, you have the new ProviderException that you don't have in the first. I'm not sure how you could get into this case, if you're % by modulus, but I guess it's good to be careful. That said, you should either make consistent with the above or remove. You might also combine the actual copy logic into a single function called from both. Good point, I modified the engineGenerateSecret() to leverage engineGenerateSecret(byte[], int) method. P11KeyAgreement.java // Solaris Shouldn't this be // Solaris/NSS, for the case in which there isn't a leading zero? Well, I think putting NSS in both places seems somewhat confusing. What I tried to point out is that NSS (at least in the older versions, I have not tried the newer version to see if they switched behavior) trims off the leading 0s whenever possible. I've made some modification hoping to make it clearer. For performance, it's a shame that engineGenerateSecret(byte[] sharedSecret, int offset) is doing a second copy here. Not a request to do anything, just an observation. Well, that's just how it is written from the very beginning. The current impl is more optimized for engineGenerateSecret(). Probably not worthwhile to re-write the code to get rid of the extra copying. DHKeyAgreement2.java Security.addProvider(jce) Good grief! ;) Still finding these! I think you should include the new bugid. @bug 7146728 Done. TestShort.java == New bugid @bug 4942494 7146728 Done. Indention problem around the try. Fixed. Was the change at 69 because the leading zeros were being trimmed? Yes, SunPKCS11 provider was explicitly trimming off the leading 0s before this 7146728 fix. Now that we decide to preserve the leading 0s, this test vector has to be updated. Thanks, Valerie Brad On 3/1/2012 2:03 PM, Valerie (Yu-Ching) Peng wrote: Yes, that's the section that mentioned the generated secret having the same length as p. I guess it depends on how it's interpreted. The way I look at it is that if the generated secrets aren't of the same length, then whoever using this variant for deriving the keying material will need to do the extra work of checking the size of generated secret assuming that they know the size of p to add back the leading 0s if needed. I can't find any specification dictating trimming off the leading 0s for the generated secret. Considering both models, preserving vs trimming, I feel the former makes more sense since it is simpler to use and provides more flexibility than the later. RFC 2631 has not be crystal clear on this I am afraid, that's why there has been some inconsistency in different vendors implementation since people interpret what they read differently. Valerie On 02/29/12 23:00, Brad Wetmore wrote: On 2/21/2012 5:33 PM, Valerie (Yu-Ching) Peng wrote: Brad, Can you please review the fixes for the following 2 bugs: * 7146728: Inconsistent length for the generated secret using DH key agreement impl from SunJCE and PKCS11 o http://cr.openjdk.java.net/~valeriep/7146728/webrev.00/ This impacts both SunJCE provider and SunPKCS11 provider. The implementations are inconsistent within SunJCE provider itself between the engineGenerateSecret() and engineGenerateSecret(byte[], int). Given that RFC 2631 specifies the leading 0s must be preserved so the generated secret has as many octets as the prime P, Just to be clear here, you're referring to Section 2.1.2 of 2631, which is just one of the DH Key agreement variants (based on X9.42) for generating Keying Material from secret keys
