Re: RFR[11] JDK-8199645: javax/net/ssl/SSLSession/TestEnabledProtocols.java failed with Connection reset
Hi Xuelei,
Thanks for your review!
The fix just was pushed.
I adjusted the following longer lines:
129 System.out.println("Client got UNEXPECTED
SSLHandshakeException:");
134 System.out.println("Client got expected
SSLHandshakeException:");
In addition, the parentheses, which encloses the new statement, also was
removed.
269 (new TestEnabledProtocols(
270 serverProtocols,
271 clientProtocols,
272 exceptionExpected,
273 selectedProtocol)).run();
Best regards,
John Jiang
On 2018/7/10 21:33, Xuelei Fan wrote:
Looks fine to me. Please limit each line in 80 characters.
Thanks,
Xuelei
On 7/10/2018 2:27 AM, sha.ji...@oracle.com wrote:
Hi,
javax/net/ssl/SSLSession/TestEnabledProtocols.java may have some
problem on sync between server and client.
And it would be better to refactor this test with
SSLSocketTemplate.java.
Webrev: http://cr.openjdk.java.net/~jjiang/8199645/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8199645
Best regards,
John Jiang
Re: RFR[11] JDK-8206171: Signature#getParameters for RSASSA-PSS throws ProviderException when not initialized
Hi Valerie About "it *may* return", do you mean it could also return null? My understanding is no. Is it better to clarify when the implementation "may also fail"? From the CSR, it's this method. Can you add a @throws spec to this method then? Also, I am a little confused by "default and randomly generated". Does this actually mean "default (might be randomly generated)"? The "it may" sentence mentions "default and randomly generated" but the "if there" only says "default", which sounds like the the "randomly generated" case could be different. Thanks Max > On Jul 11, 2018, at 5:12 AM, Valerie Peng wrote: > > Hi Brad, > > Would you have time to review the fix for JDK-8206171: > Signature#getParameters for RSASSA-PSS throws ProviderException when not > initialized? > No source code changes, but just updating javadoc to mention the possible > failure case. > Otherwise, JCK team expects a parameter object or null being returned. > I filed a CSR to track the javadoc clarification. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8206171 > Webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.00/ > CSR: https://bugs.openjdk.java.net/browse/JDK-8206864 > > Thanks, > Valerie > > >
Re: RFR[12] JDK-8179098 "Crypto AES/ECB encryption/decryption performance regression (introduced in jdk9b73)"
Thanks for the review~ Valerie On 7/10/2018 11:16 AM, Anthony Scarpino wrote: Ok, I'm fine with what you have here. thanks Tony On 07/09/2018 05:42 PM, Valerie Peng wrote: Hi Tony, The purpose of the if-block is to ensure that ArrayIndexOutOfBoundsException is thrown instead of IndexOutOfBoundsException. Objects.checkFromIndexSize(...) API is specified to throw IndexOutOfBoundsException which is more general and can be thrown when an index of some sort (such as to an array, to a string, or to a vector) is out of range. ArrayIndexOutOfBoundsException is a subclass of IndexOutOfBoundsException and is for array index out of bounds. Thanks, Valerie On 7/9/2018 3:14 PM, Anthony Scarpino wrote: On 07/03/2018 02:03 PM, Valerie Peng wrote: Hi Tony, Would you have time to review this? Instead of doing the bounds check per block, the changes move the bounds check up one level. Bug: https://bugs.openjdk.java.net/browse/JDK-8179098 Webrev: http://cr.openjdk.java.net/~valeriep/8179098/webrev.00/ Thanks, Valerie ArrayUtil.java: Line 48 - It seems like your not throwing any ArrayIndexOutOfBoundsExceptions, is that intentional? The method in the try-catch seems to only throw IndexOutOfBoundsException, whole if() seems confusing to me. thanks Tony
Re: (Open) RFR: 8205967: Remove sun/security/krb5/auto/UnboundSSL.java from ProblemList.txt
Looks fine to me. Thanks, Xuelei > On Jul 10, 2018, at 12:11 PM, Andrew Wong wrote: > > Dear Security Developer, > > Please review the following fix for bug id 8205967. The test associated with > UnboundSSL.java was removed in a previous fix but is still listed in > ProblemList.txt. ProblemList.txt has been updated accordingly by removing the > test. > > http://cr.openjdk.java.net/~rhalade/andrew/8205967/webrev.00/ > > Thank you, > > Andrew Wong
(Open) RFR: 8205967: Remove sun/security/krb5/auto/UnboundSSL.java from ProblemList.txt
Dear Security Developer, Please review the following fix for bug id 8205967. The test associated with UnboundSSL.java was removed in a previous fix but is still listed in ProblemList.txt. ProblemList.txt has been updated accordingly by removing the test. http://cr.openjdk.java.net/~rhalade/andrew/8205967/webrev.00/ Thank you, Andrew Wong
Re: JDK 11+21 SSLSocket.close() deadlock?
Hi Simone, Thank you for reporting this issue. Now it is tracked in JBS: https://bugs.openjdk.java.net/browse/JDK-8207004 In the following stacks, only one lock (on 0xac) can be observed. Can I understand that the read() is blocked, and then the close() is blocked as well? Did you have a test that we can use to reproduce this issue? Thanks, Xuelei On 7/10/2018 10:19 AM, Simone Bordet wrote: Hi, Please look at the stack traces below. The server code accept() a SSLSocket, then calls startHandshake() in a different thread. The client code sends the TLS handshake bytes very slowly. The server code waits for a bit for the handshake to finish, then attempts to close the socket, but it cannot due to the deadlock below. The client stops sending the TLS handshake bytes and now the server is stuck forever. I think invoking close() should wake up the read() with an exception, rather than trying to grab the same lock held by the read, as asynchronous closes from other threads happen all the times. The issue does not happen with JDK 9, 10, 11-ea+18, but does happen with 11-ea+21. Thanks! --- "main@1" prio=5 tid=0x1 nid=NA waiting for monitor entry java.lang.Thread.State: BLOCKED waiting for pool-1-thread-1@2224 to release lock on <0xac7> (a sun.security.ssl.SSLSocketImpl) at sun.security.ssl.SSLSocketImpl.close(SSLSocketImpl.java:447) "pool-1-thread-1@2224" prio=5 tid=0x19 nid=NA runnable java.lang.Thread.State: RUNNABLE blocks main@1 at java.net.SocketInputStream.socketRead0(SocketInputStream.java:-1) at java.net.SocketInputStream.socketRead(SocketInputStream.java:115) at java.net.SocketInputStream.read(SocketInputStream.java:168) at java.net.SocketInputStream.read(SocketInputStream.java:140) at sun.security.ssl.SSLSocketInputRecord.read(SSLSocketInputRecord.java:464) at sun.security.ssl.SSLSocketInputRecord.decode(SSLSocketInputRecord.java:167) at sun.security.ssl.SSLTransport.decode(SSLTransport.java:108) at sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:877) at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:810) - locked <0xac7> (a sun.security.ssl.SSLSocketImpl) at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:383)
Re: Unable to use custom SSLEngine with default TrustManagerFactory after updating to ea20 (and later)
Will do tomorrow latest. Thanks for the quick reply. Bye Norman > Am 10.07.2018 um 18:53 schrieb Xuelei Fan : > > Hi Norman, > > It's an interesting user case of the TrustManagerFactory. Please file a bug. > > Thanks, > Xuelei > >> On 7/10/2018 9:57 AM, Alan Bateman wrote: >> Forwarding to security-dev. >>> On 10/07/2018 17:47, Norman Maurer wrote: >>> Hi all, >>> >>> I just tried to run netty[1] testsuite with the latest jdk11 EA release >>> (21) and saw some class-cast-exception with our custom SSLEngine >>> implementation >>> >>> >>> Caused by: java.lang.ClassCastException: class >>> io.netty.handler.ssl.OpenSslEngine cannot be cast to class >>> sun.security.ssl.SSLEngineImpl (io.netty.handler.ssl.OpenSslEngine is in >>> unnamed module of loader 'app'; sun.security.ssl.SSLEngineImpl is in module >>> java.base of loader 'bootstrap') >>> at >>> java.base/sun.security.ssl.SSLAlgorithmConstraints.(SSLAlgorithmConstraints.java:93) >>> at >>> java.base/sun.security.ssl.X509TrustManagerImpl.checkTrusted(X509TrustManagerImpl.java:270) >>> at >>> java.base/sun.security.ssl.X509TrustManagerImpl.checkServerTrusted(X509TrustManagerImpl.java:141) >>> at >>> io.netty.handler.ssl.ReferenceCountedOpenSslClientContext$ExtendedTrustManagerVerifyCallback.verify(ReferenceCountedOpenSslClientContext.java:237) >>> at >>> io.netty.handler.ssl.ReferenceCountedOpenSslContext$AbstractCertificateVerifier.verify(ReferenceCountedOpenSslContext.java:621) >>> ... 27 more >>> >>> >>> This change seems to be related to: >>> http://hg.openjdk.java.net/jdk/jdk11/rev/68fa3d4026ea >>> >>> I think you miss an instanceof check here in SSLAlgorithmConstraints before >>> try to cast to SSLEngineImpl, as otherwise it will be impossible to use >>> custom implementations of SSLEngine (which we have in netty) with the >>> default TrustManagerFactory. >>> >>> Does this sound correct ? Should I open a bug-report ? >>> >>> Bye >>> Norman >>> >>> >>>
Re: RFR[12] JDK-8179098 "Crypto AES/ECB encryption/decryption performance regression (introduced in jdk9b73)"
Ok, I'm fine with what you have here. thanks Tony On 07/09/2018 05:42 PM, Valerie Peng wrote: Hi Tony, The purpose of the if-block is to ensure that ArrayIndexOutOfBoundsException is thrown instead of IndexOutOfBoundsException. Objects.checkFromIndexSize(...) API is specified to throw IndexOutOfBoundsException which is more general and can be thrown when an index of some sort (such as to an array, to a string, or to a vector) is out of range. ArrayIndexOutOfBoundsException is a subclass of IndexOutOfBoundsException and is for array index out of bounds. Thanks, Valerie On 7/9/2018 3:14 PM, Anthony Scarpino wrote: On 07/03/2018 02:03 PM, Valerie Peng wrote: Hi Tony, Would you have time to review this? Instead of doing the bounds check per block, the changes move the bounds check up one level. Bug: https://bugs.openjdk.java.net/browse/JDK-8179098 Webrev: http://cr.openjdk.java.net/~valeriep/8179098/webrev.00/ Thanks, Valerie ArrayUtil.java: Line 48 - It seems like your not throwing any ArrayIndexOutOfBoundsExceptions, is that intentional? The method in the try-catch seems to only throw IndexOutOfBoundsException, whole if() seems confusing to me. thanks Tony
Re: Unable to use custom SSLEngine with default TrustManagerFactory after updating to ea20 (and later)
Hi Norman, It's an interesting user case of the TrustManagerFactory. Please file a bug. Thanks, Xuelei On 7/10/2018 9:57 AM, Alan Bateman wrote: Forwarding to security-dev. On 10/07/2018 17:47, Norman Maurer wrote: Hi all, I just tried to run netty[1] testsuite with the latest jdk11 EA release (21) and saw some class-cast-exception with our custom SSLEngine implementation Caused by: java.lang.ClassCastException: class io.netty.handler.ssl.OpenSslEngine cannot be cast to class sun.security.ssl.SSLEngineImpl (io.netty.handler.ssl.OpenSslEngine is in unnamed module of loader 'app'; sun.security.ssl.SSLEngineImpl is in module java.base of loader 'bootstrap') at java.base/sun.security.ssl.SSLAlgorithmConstraints.(SSLAlgorithmConstraints.java:93) at java.base/sun.security.ssl.X509TrustManagerImpl.checkTrusted(X509TrustManagerImpl.java:270) at java.base/sun.security.ssl.X509TrustManagerImpl.checkServerTrusted(X509TrustManagerImpl.java:141) at io.netty.handler.ssl.ReferenceCountedOpenSslClientContext$ExtendedTrustManagerVerifyCallback.verify(ReferenceCountedOpenSslClientContext.java:237) at io.netty.handler.ssl.ReferenceCountedOpenSslContext$AbstractCertificateVerifier.verify(ReferenceCountedOpenSslContext.java:621) ... 27 more This change seems to be related to: http://hg.openjdk.java.net/jdk/jdk11/rev/68fa3d4026ea I think you miss an instanceof check here in SSLAlgorithmConstraints before try to cast to SSLEngineImpl, as otherwise it will be impossible to use custom implementations of SSLEngine (which we have in netty) with the default TrustManagerFactory. Does this sound correct ? Should I open a bug-report ? Bye Norman
JDK 11+21 SSLSocket.close() deadlock?
Hi, Please look at the stack traces below. The server code accept() a SSLSocket, then calls startHandshake() in a different thread. The client code sends the TLS handshake bytes very slowly. The server code waits for a bit for the handshake to finish, then attempts to close the socket, but it cannot due to the deadlock below. The client stops sending the TLS handshake bytes and now the server is stuck forever. I think invoking close() should wake up the read() with an exception, rather than trying to grab the same lock held by the read, as asynchronous closes from other threads happen all the times. The issue does not happen with JDK 9, 10, 11-ea+18, but does happen with 11-ea+21. Thanks! --- "main@1" prio=5 tid=0x1 nid=NA waiting for monitor entry java.lang.Thread.State: BLOCKED waiting for pool-1-thread-1@2224 to release lock on <0xac7> (a sun.security.ssl.SSLSocketImpl) at sun.security.ssl.SSLSocketImpl.close(SSLSocketImpl.java:447) "pool-1-thread-1@2224" prio=5 tid=0x19 nid=NA runnable java.lang.Thread.State: RUNNABLE blocks main@1 at java.net.SocketInputStream.socketRead0(SocketInputStream.java:-1) at java.net.SocketInputStream.socketRead(SocketInputStream.java:115) at java.net.SocketInputStream.read(SocketInputStream.java:168) at java.net.SocketInputStream.read(SocketInputStream.java:140) at sun.security.ssl.SSLSocketInputRecord.read(SSLSocketInputRecord.java:464) at sun.security.ssl.SSLSocketInputRecord.decode(SSLSocketInputRecord.java:167) at sun.security.ssl.SSLTransport.decode(SSLTransport.java:108) at sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:877) at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:810) - locked <0xac7> (a sun.security.ssl.SSLSocketImpl) at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:383) -- Simone Bordet --- 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: RFR JDK-8029661: JDK-Support TLS v1.2 algorithm in SunPKCS11 provider
Hi, Webrev 04 for JDK-8029661 is ready: * http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.04.zip * http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.04/ New: * Rebased to latest JDK revision (after TLS 1.3 merge) * Rev 1acfd2f56d72 * ProtocolVersion dependencies removed (raw TLS protocol version numbers are now used) * Code style improvements (tabs, trailing whitespaces, max lines length, etc.) Thanks, Martin.-
Re: Unable to use custom SSLEngine with default TrustManagerFactory after updating to ea20 (and later)
Forwarding to security-dev. On 10/07/2018 17:47, Norman Maurer wrote: Hi all, I just tried to run netty[1] testsuite with the latest jdk11 EA release (21) and saw some class-cast-exception with our custom SSLEngine implementation Caused by: java.lang.ClassCastException: class io.netty.handler.ssl.OpenSslEngine cannot be cast to class sun.security.ssl.SSLEngineImpl (io.netty.handler.ssl.OpenSslEngine is in unnamed module of loader 'app'; sun.security.ssl.SSLEngineImpl is in module java.base of loader 'bootstrap') at java.base/sun.security.ssl.SSLAlgorithmConstraints.(SSLAlgorithmConstraints.java:93) at java.base/sun.security.ssl.X509TrustManagerImpl.checkTrusted(X509TrustManagerImpl.java:270) at java.base/sun.security.ssl.X509TrustManagerImpl.checkServerTrusted(X509TrustManagerImpl.java:141) at io.netty.handler.ssl.ReferenceCountedOpenSslClientContext$ExtendedTrustManagerVerifyCallback.verify(ReferenceCountedOpenSslClientContext.java:237) at io.netty.handler.ssl.ReferenceCountedOpenSslContext$AbstractCertificateVerifier.verify(ReferenceCountedOpenSslContext.java:621) ... 27 more This change seems to be related to: http://hg.openjdk.java.net/jdk/jdk11/rev/68fa3d4026ea I think you miss an instanceof check here in SSLAlgorithmConstraints before try to cast to SSLEngineImpl, as otherwise it will be impossible to use custom implementations of SSLEngine (which we have in netty) with the default TrustManagerFactory. Does this sound correct ? Should I open a bug-report ? Bye Norman
Re: RFR[11] JDK-8199645: javax/net/ssl/SSLSession/TestEnabledProtocols.java failed with Connection reset
Looks fine to me. Please limit each line in 80 characters. Thanks, Xuelei On 7/10/2018 2:27 AM, sha.ji...@oracle.com wrote: Hi, javax/net/ssl/SSLSession/TestEnabledProtocols.java may have some problem on sync between server and client. And it would be better to refactor this test with SSLSocketTemplate.java. Webrev: http://cr.openjdk.java.net/~jjiang/8199645/webrev.00/ JBS: https://bugs.openjdk.java.net/browse/JDK-8199645 Best regards, John Jiang
Re: RFR: 8148188: Enhance the security libraries to record events of interest
Erik, After some trial edits, I'm not so sure if moving the event & logger commit code into the class where it's used works too well after all. In the code you suggested, there's an assumption that calls such as EventHelper.certificateChain(..) are low cost. While that might be the case here, it's something we can't always assume. It's called once for the JFR operation and once for the Logger operation. That pattern multiplies itself further in other Events. i.e. set up and assign the variables for a JFR event without knowing whether they'll be needed again for the Logger call. We could work around it by setting up some local variables and re-using them in the Logger code but then, we're back to where we were. The current EventHelper design eliminates this problem and also helps to abstract the recording/logging functionality away from the functionality of the class itself. It also becomes a problem where we record events in multiple different classes (e.g. SecurityPropertyEvent). While we could leave the code in EventHelper for this case, we then have a mixed design pattern. Are you ok with leaving as is for now? A future wish might be one where JFR would handle Logger via its own framework/API in a future JDK release. I've removed the variable names using underscore. Also optimized some variable assignments in X509Impl.commitEvent(..) http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/ regards, Sean. On 09/07/2018 18:01, Seán Coffey wrote: Erik, Thanks for reviewing. Comments inline.. On 09/07/18 17:21, Erik Gahlin wrote: Thanks Sean. Some feedback on the code in the security libraries. - We should use camel case naming convention for variables (not underscore). Sure. I see two offending variable names which I'll fix up. - Looking at sun/security/ssl/Finished.java, I wonder if it wouldn't be less code and more easy to read, if we would commit the event in a local private function and then use the EventHelper class for common functionality. I'm fine with your suggested approach. I figured that tucking the recording/logging logic away in a helper class also had benefits but I'll re-factor to your suggested style unless I hear objections. regards, Sean.
RFR[11] JDK-8199645: javax/net/ssl/SSLSession/TestEnabledProtocols.java failed with Connection reset
Hi, javax/net/ssl/SSLSession/TestEnabledProtocols.java may have some problem on sync between server and client. And it would be better to refactor this test with SSLSocketTemplate.java. Webrev: http://cr.openjdk.java.net/~jjiang/8199645/webrev.00/ JBS: https://bugs.openjdk.java.net/browse/JDK-8199645 Best regards, John Jiang
