Re: RFR: 8199018: Test crypto provider not registering

2018-03-06 Thread Brad Wetmore
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:3

Re: RFR: 8199018: Test crypto provider not registering

2018-03-05 Thread Brad Wetmore
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

Re: provider registration

2018-03-01 Thread Brad Wetmore
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.secu

Re: RFR: 8193892: Impact of noncloneable MessageDigest implementation

2018-02-23 Thread Brad Wetmore
g 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, 8013809 deadlock in SSLSocketImpl between between write and close

2013-08-01 Thread Brad Wetmore
On 7/25/2013 2:11 AM, Xuelei Fan wrote: Hi Brad, Are you available to review this fix? Webrev: http://cr.openjdk.java.net/~xuelei/8013809/webrev.00/ No new regression test, hard to reproduce the issue. Your immediate fix looks good, however, IIRC, the reason for having getConnectionState(

Re: Code Review Request:8019544: Need to run ProviderTest.java in othervm mode

2013-07-25 Thread Brad Wetmore
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/ This is a test only fix for test stabilization. --

Re: Code review request: 6755701 SecretKeySpec & DES

2013-07-02 Thread Brad Wetmore
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 D

Re: Code review request, 7188658 Add possibility to disable client initiated renegotiation

2013-06-30 Thread Brad Wetmore
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.

Re: Code review request, 7188658 Add possibility to disable client initiated renegotiation

2013-06-27 Thread Brad Wetmore
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 use

Re: Code review request, 7188658 Add possibility to disable client initiated renegotiation

2013-06-27 Thread Brad Wetmore
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

Re: Code review request, 7188658 Add possibility to disable client initiated renegotiation

2013-06-27 Thread Brad Wetmore
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 be

Re: Code review request, 7188658 Add possibility to disable client initiated renegotiation

2013-06-27 Thread Brad Wetmore
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* prope

Re: Review Request: JDK-8019227: JDK-8010325 broke the old build

2013-06-27 Thread Brad Wetmore
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 buil

Review Request: JDK-8019227: JDK-8010325 broke the old build

2013-06-26 Thread Brad Wetmore
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

Re: Code review request, 7188658 Add possibility to disable client initiated renegotiation

2013-06-18 Thread Brad Wetmore
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.

Re: Code review request, 8000456: Add programmatic deadlock detection in SSLEngineDeadlock

2013-06-18 Thread Brad Wetmore
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. Howe

Re: RFR JDK8007636

2013-06-17 Thread Brad Wetmore
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 dealloc

Re: Signature.getAlogrithm return null in special case

2013-06-17 Thread Brad Wetmore
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

Re: Signature.getAlogrithm return null in special case

2013-06-17 Thread Brad Wetmore
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

Re: Code review: 8001284 & 8012971

2013-05-30 Thread Brad Wetmore
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 s

Re: Signature.getAlogrithm return null in special case

2013-05-27 Thread Brad Wetmore
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 Open

Re: Signature.getAlogrithm return null in special case

2013-05-14 Thread Brad Wetmore
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 SignatureS

Reviewed: JDK-8012530 : test/sun/security/provider/SecureRandom/StrongSeedReader.java failing

2013-04-25 Thread Brad Wetmore
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 switc

Re: code review request: 7171982 Cipher getParameters() throws RuntimeException: Cannot find SunJCE provider

2013-04-23 Thread Brad Wetmore
=6578538 I might suggest changing to the Holder pattern (Effective Java 2nd Edition: Item 71) Thanks, Brad On 3/28/2013 2:45 PM, Brad Wetmore wrote: (Whoops, was working on two reviews with two related comments, and reversed the emails). Just realized, there are no regression tests here. Simp

Re: Why cannot overwrite a KeyEntry with a TrustCertEntry?

2013-04-11 Thread Brad Wetmore
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

Re: Code review request, re-integrate JEP 115 into JDK 8

2013-04-11 Thread Brad Wetmore
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 InputReco

Re: Update #5: JEP 123: SecureRandom Draft and Implementation.

2013-04-11 Thread Brad Wetmore
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

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

2013-04-10 Thread Brad Wetmore
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

Re: Update #5: JEP 123: SecureRandom Draft and Implementation.

2013-04-10 Thread Brad Wetmore
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,

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

2013-04-10 Thread Brad Wetmore
nce (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

Re: Debuggability of failures in sun.security.rsa.RSASignature

2013-04-08 Thread Brad Wetmore
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

Re: simple code review request: 8001596

2013-04-03 Thread Brad Wetmore
Looks good. Be sure to check in the closed version when you get your changes finished. Also, check to see if anyone else will be checking in signed jar files. I should have a changeset coming within the next week or so. Thanks, Brad On 4/3/2013 4:31 PM, Anthony Scarpino wrote: Updated

Re: simple code review request: 8001596

2013-03-28 Thread Brad Wetmore
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

Re: simple code review request: 8001596

2013-03-28 Thread Brad Wetmore
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,

Re: code review request: 7171982 Cipher getParameters() throws RuntimeException: Cannot find SunJCE provider

2013-03-28 Thread Brad Wetmore
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 chan

Re: simple code review request: 8001596

2013-03-28 Thread Brad Wetmore
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

Re: code review request: 7171982 Cipher getParameters() throws RuntimeException: Cannot find SunJCE provider

2013-03-28 Thread Brad Wetmore
(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 t

Re: Next Protocol Negotiation TLS Extension

2013-03-25 Thread Brad Wetmore
On 3/25/2013 6:59 AM, Simone Bordet wrote: Hi, On Fri, Mar 22, 2013 at 1:08 AM, Brad Wetmore 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

Re: Next Protocol Negotiation TLS Extension

2013-03-21 Thread Brad Wetmore
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

Re: Allow configure to detect if EC implementation is present

2013-03-13 Thread Brad Wetmore
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/nat

RFR 8009925: Back out AEAD CipherSuites temporarily

2013-03-12 Thread Brad Wetmore
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 director

Re: PKCS11 support on 64bit sun java

2013-03-07 Thread Brad Wetmore
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 Mohankum

Re: code review request: 8009604, old make images failed: JarBASE64Encoder class not found

2013-03-06 Thread Brad Wetmore
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. Tha

Re: Code Review Request for 7030966, Support AEAD CipherSuites (JSSE part of JEP 115)

2013-01-18 Thread Brad Wetmore
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 Tu

Re: Code Review Request for 7030966, Support AEAD CipherSuites (JSSE part of JEP 115)

2013-01-17 Thread Brad Wetmore
Hi Xuelei, Minor stuff. A couple things to check. Xuelei wrote: Hi Valerie, Max or Brad, Can you review the update for JDK-7030966? It is the JSSE part of JEP 115. webrev: http://cr.openjdk.java.net./~xuelei/7030966/webrev.00/ JEP 115: http://openjdk.java.net/jeps/115 I looked at webrev.0

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

2013-01-10 Thread Brad Wetmore
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. # securerand

Re: Fw: Update #2: JEP 123: SecureRandom First Draft and Implementation.

2013-01-10 Thread Brad Wetmore
arded by Bruce Rich/Austin/IBM on 01/10/2013 11:44 AM - From: Michael StJohns To: Sean Mullan , Xuelei Fan Cc: OpenJDK Dev list , Brad Wetmore Date: 01/09/2013 09:32 PM Subject: Re: Update #2: JEP 123: SecureRandom First Draft and Implementation. Sent by: security-dev-boun...@openjdk.jav

Update #3: JEP 123: SecureRandom First Draft and Implementation.

2013-01-10 Thread Brad Wetmore
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

Re: Update #2: JEP 123: SecureRandom First Draft and Implementation.

2013-01-09 Thread Brad Wetmore
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/

Re: Update #2: JEP 123: SecureRandom First Draft and Implementation.

2013-01-09 Thread Brad Wetmore
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" impleme

Re: Update #2: JEP 123: SecureRandom First Draft and Implementation.

2013-01-09 Thread Brad Wetmore
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 th

Update #2: JEP 123: SecureRandom First Draft and Implementation.

2013-01-09 Thread Brad Wetmore
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

Re: JEP 123: SecureRandom First Draft and Implementation.

2013-01-04 Thread Brad Wetmore
nd 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://c

Re: JEP 123: SecureRandom First Draft and Implementation.

2013-01-04 Thread Brad Wetmore
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

Re: JEP 123: SecureRandom First Draft and Implementation.

2013-01-04 Thread Brad Wetmore
lean 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 th

JEP 123: SecureRandom First Draft and Implementation.

2013-01-02 Thread Brad Wetmore
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,

Re: Request for Review: 7193792: sun/security/pkcs11/ec/TestECDSA.java failing intermittently

2012-12-06 Thread Brad Wetmore
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

Re: Code Review Request, 8004184, security tests leave JSSEServer running

2012-12-03 Thread Brad Wetmore
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 o

Re: [8] Code Review Request for 8004044: Lazily instantiate SunJCE.RANDOM

2012-11-28 Thread Brad Wetmore
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 b

Re: Code review request, JDK-8001569 Regression test GetPeerHost uses static port number

2012-11-09 Thread Brad Wetmore
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

8001419: Build the JCE portion of JDK-8000970

2012-10-23 Thread Brad Wetmore
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 Fred

Re: Memory leak fix for: src/solaris/native/com/sun/security/auth/module/Unix.c

2012-10-19 Thread Brad Wetmore
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

Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server

2012-10-16 Thread Brad Wetmore
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

Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server

2012-10-16 Thread Brad Wetmore
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 b

Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server

2012-10-15 Thread Brad Wetmore
ick 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 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 ha

Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server

2012-10-15 Thread Brad Wetmore
A server name indication extension will be sent whenever the name is > recognizable by the matches. I did not check for the types of cipher suites. I think it is the proper approach because although for anonymous cipher cuties, there is not certificates, but the ssl context may be different,

Re: jsse.jar still needed?

2012-10-15 Thread Brad Wetmore
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.

Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server

2012-10-12 Thread Brad Wetmore
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" th

Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server

2012-10-10 Thread Brad Wetmore
On 10/10/2012 5:47 AM, Xuelei Fan wrote: new webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.13/ I guess you didn't need to have me as reviewer before going final with the CCC? javax/net/ssl/SSLSocketFactory.java === Change look good. 216: I th

Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server

2012-10-09 Thread Brad Wetmore
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

Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server

2012-10-08 Thread Brad Wetmore
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://c

Re: [PATCH FOR REVIEW] Allow OpenJDK to be built with the unlimited crypto policy

2012-09-27 Thread Brad Wetmore
It's now marked as resolved. Thanks, Brad On 9/27/2012 9:58 AM, Andrew Hughes wrote: - Original Message - - 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

Re: [PATCH FOR REVIEW] Allow OpenJDK to be built with the unlimited crypto policy

2012-09-27 Thread Brad Wetmore
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

Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server

2012-09-26 Thread Brad Wetmore
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://

Re: [PATCH FOR REVIEW] Allow OpenJDK to be built with the unlimited crypto policy

2012-09-26 Thread Brad Wetmore
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 unl

Re: Code review request, 7200295 CertificateRequest message is wrapping when using large numbers of Certs

2012-09-25 Thread Brad Wetmore
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

Re: [PATCH FOR REVIEW] Allow OpenJDK to be built with the unlimited crypto policy

2012-09-25 Thread Brad Wetmore
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

Re: Code review request, 7200295 CertificateRequest message is wrapping when using large numbers of Certs

2012-09-24 Thread Brad Wetmore
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

Re: [PATCH FOR REVIEW] Allow OpenJDK to be built with the unlimited crypto policy

2012-09-19 Thread Brad Wetmore
> But I think someone from the security team should chime in on this. I plan to look closer at this. On the surface, it looks acceptable to me, but I've been heads down in the SNI code: likely for one more day. Wanted to also run this by one of my other colleagues. One thought: I'm wonderin

Re: Code review request, 7199066 Typo in method name

2012-09-18 Thread Brad Wetmore
Looks ok to me too. Brad On 9/18/2012 5:49 AM, Sean Mullan wrote: Looks good to me. Don't forget to add the noreg-trivial keyword to the CR before pushing. --Sean On 9/17/12 9:38 PM, Xuelei Fan wrote: Hi, Please review the trivial fix of a typo in internal method name. The bug has not bee

Re: Quick Codereview

2012-09-13 Thread Brad Wetmore
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 J

Quick Codereview

2012-09-12 Thread Brad Wetmore
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/

Re: JDK8 code review request: 7196593: java.security.debug=help doesn't list certpath option

2012-09-11 Thread Brad Wetmore
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

Re: [PATCH] Review request for 7195733 TEST_BUG: sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/B6216082.java failing

2012-09-04 Thread Brad Wetmore
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

Re: test/java/security/spec/EllipticCurveMatch.java othervm?

2012-08-30 Thread Brad Wetmore
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

Re: OpenJDK 7 SNI Implementation

2012-08-22 Thread Brad Wetmore
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

Re: (2nd round) Proposed API Changes for JEP 114: TLS Server Name Indication (SNI) Extension

2012-08-15 Thread Brad Wetmore
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

Re: (3rd Round) Proposed API Changes for JEP 114: TLS Server Name Indication (SNI) Extension

2012-08-14 Thread Brad Wetmore
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 unders

Re: (2nd round) Proposed API Changes for JEP 114: TLS Server Name Indication (SNI) Extension

2012-08-14 Thread Brad Wetmore
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

Re: Code review request: 7190945: pkcs11 problem loading NSS libs on Ubuntu

2012-08-13 Thread Brad Wetmore
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

Re: (2nd round) Proposed API Changes for JEP 114: TLS Server Name Indication (SNI) Extension

2012-08-09 Thread Brad Wetmore
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: Hav

Re: Please Review Test Fix of Bug 7177045

2012-07-12 Thread Brad Wetmore
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 o

Re: Code review request, CR 7180038 regression test failure, SSLEngineBadBufferArrayAccess.java

2012-07-06 Thread Brad Wetmore
age. 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 d

Re: Please Review Test Fix of Bug 7177045

2012-06-28 Thread Brad Wetmore
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 m

Codereview for 7177556

2012-06-15 Thread Brad Wetmore
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.

Re: Code review request for 7172149 ArrayIndexOutOfBoundsException from Signature.verify

2012-06-04 Thread Brad Wetmore
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 th

Re: Code review request for 7172149 ArrayIndexOutOfBoundsException from Signature.verify

2012-05-29 Thread Brad Wetmore
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

Minor Codereview.

2012-05-09 Thread Brad Wetmore
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 go

Re: code review request, CR 7166570: JSSE certificate validation has started to fail for certificate chains

2012-05-08 Thread Brad Wetmore
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, r

Re: code review request, CR 7167092 Need to put the return clause in the synchronized block

2012-05-08 Thread Brad Wetmore
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 C

  1   2   3   >