hg: jdk8/tl/jdk: 8025123: SNI support in Kerberos cipher suites
Changeset: 3fca37c636be Author:xuelei Date: 2013-10-01 20:25 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3fca37c636be 8025123: SNI support in Kerberos cipher suites Reviewed-by: weijun, xuelei Contributed-by: Artem Smotrakov ! src/share/classes/sun/security/ssl/ClientHandshaker.java ! src/share/classes/sun/security/ssl/Handshaker.java ! src/share/classes/sun/security/ssl/KerberosClientKeyExchange.java ! src/share/classes/sun/security/ssl/krb5/KerberosClientKeyExchangeImpl.java ! test/sun/security/krb5/auto/SSL.java
hg: jdk8/tl/jdk: 6956398: make ephemeral DH key match the length of the certificate key
Changeset: 0d5f4f1782e8 Author:xuelei Date: 2013-10-07 18:46 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0d5f4f1782e8 6956398: make ephemeral DH key match the length of the certificate key Reviewed-by: weijun ! src/share/classes/sun/security/ssl/ServerHandshaker.java + test/sun/security/ssl/com/sun/net/ssl/internal/ssl/DHKeyExchange/DHEKeySizing.java
hg: jdk8/tl/jdk: 8026119: Regression test DHEKeySizing.java failing intermittently
Changeset: fb202a8e83c9 Author:xuelei Date: 2013-10-13 21:10 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/fb202a8e83c9 8026119: Regression test DHEKeySizing.java failing intermittently Reviewed-by: weijun ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/DHKeyExchange/DHEKeySizing.java
hg: jdk8/tl/jdk: 8023147: Test DisabledShortRSAKeys.java intermittent failed
Changeset: 1158d504e39e Author:xuelei Date: 2013-11-13 01:14 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1158d504e39e 8023147: Test DisabledShortRSAKeys.java intermittent failed Reviewed-by: mullan ! test/sun/security/ssl/javax/net/ssl/TLSv12/DisabledShortRSAKeys.java
hg: jdk8/tl/jdk: 8014266: regression test AsyncSSLSocketClose.java time out.
Changeset: 40d0ccd00f87 Author:xuelei Date: 2013-11-14 16:08 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/40d0ccd00f87 8014266: regression test AsyncSSLSocketClose.java time out. Reviewed-by: xuelei Contributed-by: Rajan Halade ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLSocketImpl/AsyncSSLSocketClose.java
hg: jdk8/tl/jdk: 7093640: Enable client-side TLS 1.2 by default
Changeset: 8d35f0985dd7 Author:xuelei Date: 2013-12-18 16:46 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8d35f0985dd7 7093640: Enable client-side TLS 1.2 by default Reviewed-by: weijun, mullan, wetmore ! src/share/classes/sun/security/ssl/ProtocolVersion.java ! src/share/classes/sun/security/ssl/SSLContextImpl.java ! src/share/classes/sun/security/ssl/SunJSSE.java ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/DHKeyExchange/DHEKeySizing.java ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/EngineArgs/DebugReportsOneExtraByte.java + test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/CustomizedDefaultProtocols.java + test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/DefaultEnabledProtocols.java + test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/IllegalProtocolProperty.java + test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/NoOldVersionContext.java + test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/SSLContextVersion.java - test/sun/security/ssl/javax/net/ssl/SSLContextVersion.java
Re: AES GCM slow
What's the platform are you using for the testing? Windows, Linux,
Solaris or Mac OS? GCM are now only implemented in SunJCE provider. I
want to make sure the crypto provider for AES-CBC, which is different
for different platforms by default, is not the major cause of the
performance impact.
Thanks for the performance measure.
Regards,
Xuelei
On 1/27/2014 5:34 PM, Chris Hegarty wrote:
> Cross posting to security-dev, since the question cipher related.
>
> -Chris.
>
> On 27/01/14 09:28, Mark Christiaens wrote:
>> I wrote a little test client/server setup that transfers 100 MB of data
>> over an SSL socket configured to use TLS 1.2 AES GCM
>> (TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256). On my i7-4770 CPU @ 3.40GHz
>> with OpenJDK 1.8.0-ea-b124 I get a transfer rate of around 5.2
>> MiB/second. I expected a higher speed. Using
>> TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256 I reach 100 MiB/s. Is this to
>> be expected?
>>
>> For reference, here is my code:
>>
>> / Client.java
>>
>> package ssl;
>>
>> import javax.net.ssl.*;
>> import java.io.*;
>> import java.util.Arrays;
>>
>> public class Client {
>>
>> public static void main(String[] arstring) {
>> try {
>> SSLSocketFactory sslsocketfactory = (SSLSocketFactory)
>> SSLSocketFactory.getDefault();
>> SSLSocket sslsocket = (SSLSocket)
>> sslsocketfactory.createSocket("localhost", );
>> Helper.requireAESCipherSuites(sslsocket);
>> sslsocket.setEnabledProtocols(new String[]{"TLSv1.2"});
>>
>> try (OutputStream outputstream =
>> sslsocket.getOutputStream()) {
>> byte[] buf = new byte[Helper.BUF_SIZE];
>> Arrays.fill(buf, (byte) 1);
>> for (int i = 0; i < Helper.BUF_COUNT; ++i) {
>> outputstream.write(buf);
>> }
>>
>> System.out.println("Using cipher suite: " +
>> (sslsocket.getSession()).getCipherSuite());
>>
>> outputstream.flush();
>> }
>>
>> } catch (IOException exception) {
>> exception.printStackTrace();
>> }
>> }
>> }
>>
>> / Server.java
>>
>> package ssl;
>>
>> import javax.net.ssl.*;
>> import java.io.*;
>>
>> public class Server {
>>
>> public static void main(String[] arstring) {
>> try {
>> SSLServerSocketFactory sslserversocketfactory =
>> (SSLServerSocketFactory) SSLServerSocketFactory.getDefault();
>> SSLServerSocket sslserversocket = (SSLServerSocket)
>> sslserversocketfactory.createServerSocket();
>> SSLSocket sslsocket = (SSLSocket) sslserversocket.accept();
>>
>> InputStream inputstream = sslsocket.getInputStream();
>>
>> byte[] buf = new byte[Helper.BUF_SIZE];
>> long bytesToRead = BYTES_TO_READ;
>>
>> long startTime = System.currentTimeMillis();
>>
>> while (bytesToRead > 0) {
>> bytesToRead -= inputstream.read(buf);
>> }
>>
>> long stopTime = System.currentTimeMillis();
>> long totalTimeMs = stopTime - startTime;
>> double mbRead = BYTES_TO_READ / (1024.0 * 1024);
>> double totalTimeSeconds = totalTimeMs / 1000.0;
>> double mibPerSecond = mbRead / totalTimeSeconds;
>>
>> System.out.println("Using cipher suite: " +
>> (sslsocket.getSession()).getCipherSuite());
>> System.out.println("Read " + mbRead + "MiB in " +
>> totalTimeSeconds + "s");
>> System.out.println("Bandwidth: " + mibPerSecond + "MiB/s");
>>
>> } catch (IOException exception) {
>> exception.printStackTrace();
>> }
>> }
>>
>> private static final int BYTES_TO_READ = Helper.BUF_COUNT *
>> Helper.BUF_SIZE;
>> }
>>
>> / Helper.java
>>
>> package ssl;
>>
>> import java.util.*;
>> import java.util.regex.*;
>> import javax.net.ssl.*;
>>
>> public class Helper {
>>
>> static int BUF_SIZE = 1024 * 1024;
>> static int BUF_COUNT = 100;
>>
>> static SSLSocket requireAESCipherSuites(SSLSocket socket) {
>> String supportedCipherSuites[] =
>> socket.getSupportedCipherSuites();
>>
>> System.out.println("Supported cipher suite: " +
>> Arrays.toString(supportedCipherSuites));
>>
>> List selectedCipherSuites = new ArrayList<>();
>>
>> //String patternString = ".*";
>> String patternString = "TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256";
>> //String patternString =
>> "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256";
>>
>> Pattern pattern = Pattern.compile(patternString);
>>
>> for (String cipherSuite : supportedCipherSuites) {
>> Matcher matcher = pattern.matcher(cipherSuite);
>> if (matcher.find()) {
>> selectedCipherSuites.add(cipherSuite);
>> }
>> }
>>
>> System.out.println("Selected cipher s
Re: Review Request of JDK Enhancement Proposal: DTLS
Networking experts, any suggestion? Xuelei On 3/21/2014 8:28 AM, Matthew Hall wrote: > On Fri, Mar 21, 2014 at 06:58:50AM +0800, Xuelei Fan wrote: >> here. Although MTU is not PMTU, but it is normally "correct". > > I would state, not "normally correct", but "frequently correct". > > In case of IPSEC, SSL VPN, IPv6, GRE, etc. this will not be true. Many of > these are used for Site-to-Site VPN, which will appear often in the context > of > RTP packets and SRTP packets, which happen to travel over VPNs. > >> It would be great if there is PMTU discovery API in Java, which can >> simplify the implementation of DTLS. > > Without it, I think there will be a lot of odd bugs occurring. > > Matthew. > --- Begin Message --- PMTU is a key point of the design. I was wondering to expose this application layer as a configurable parameter. If it is too big (or not configured), DTLSEngine(let call it temporarily) will downgrade the size automatically, just as the previous messages get lost. It's good point that need a separate spec to determine the PMTU. I will see what we can do here. Thanks, Xuelei On 3/20/2014 8:31 AM, Matthew Hall wrote: > Xuelei, > > Is there an existing method for determining valid PMTU from inside of Java? > If > not then supplying correct segment size to whatever DTLSEngine (or however > it's named) class would be non-trivial and could require native code. > > If there is not such support, then a separate spec would be needed to add > that > support, before it would be possible to get the new DTLS support to work very > reliably. > > Matthew. > > On Thu, Mar 20, 2014 at 07:19:06AM +0800, Xuelei Fan wrote: >> Hi, >> >> Please review the JDK Enhancement Proposal, Support Datagram Transport >> Layer Security (DTLS) version 1.0 (RFC 4347) and 1.2 (RFC 6347) in the >> JSSE API and the SunJSSE security provider. Detailed, please refer to >> the draft JEP: >> >> http://cr.openjdk.java.net/~xuelei/7093601/jep-dtls-v00.txt >> >> Feel free to make comment and send your feedback to the alias. >> >> Thanks, >> Xuelei --- End Message ---
Code review request, JDK-8040062 Need to add new methods in BaseSSLSocketImpl
Hi, Please review the fix for JDK-8040062: http://cr.openjdk.java.net/~xuelei/8040062/webrev.00/ In JDK-8036979, there are news methods are added to java.net.Socket. These methods need to be overridden in SSL socket implementation, sun/security/ssl/BaseSSLSocketImpl.java. No new regression test. The existing test: sun/security/ssl/SSLSocketImpl/CheckMethods.java can be used to verify this fix. Thanks, Xuelei
Re: Code review request, JDK-8040062 Need to add new methods in BaseSSLSocketImpl
On 4/14/2014 8:59 PM, Sean Mullan wrote: > Looks fine to me. Can you add a relates to link to JDK-8036979 and an > appropriate noreg label? > Made the update in JBS. Thanks for the review. Xuelei > --Sean > > On 04/14/2014 06:04 AM, Xuelei Fan wrote: >> Hi, >> >> Please review the fix for JDK-8040062: >> >> http://cr.openjdk.java.net/~xuelei/8040062/webrev.00/ >> >> In JDK-8036979, there are news methods are added to java.net.Socket. >> These methods need to be overridden in SSL socket implementation, >> sun/security/ssl/BaseSSLSocketImpl.java. >> >> No new regression test. The existing test: >> >> sun/security/ssl/SSLSocketImpl/CheckMethods.java >> >> can be used to verify this fix. >> >> Thanks, >> Xuelei >>
Re: [8u20] Request for approval/Review for CR 8041931: test/sun/net/www/http/HttpClient/B8025710.java fails in 8 Update
The change looks fine to me.
Xuelei
On 4/26/2014 1:22 AM, Chris Hegarty wrote:
> Hi 8u maintainers,
>
> Stupidly I mixed up testing results, and consequently pushed a new test to
> 8u-dev [1] that has a minor issue, that causes it to fail on all platforms,
> all of the time. Root cause, the keystore used by the test has moved location
> in JDK 9, and the test should be updated to refer to the keystore location in
> JDK 8. Clearly this is not an issue in JDK 9.
>
> Since the change is trivial, it should be possible to do the approval and
> review here?
>
> diff --git a/test/sun/net/www/http/HttpClient/B8025710.java
> b/test/sun/net/www/http/HttpClient/B8025710.java
> --- a/test/sun/net/www/http/HttpClient/B8025710.java
> +++ b/test/sun/net/www/http/HttpClient/B8025710.java
> @@ -43,7 +43,7 @@
> private final static AtomicBoolean connectInServer = new
> AtomicBoolean();
> private static final String keystorefile =
> System.getProperty("test.src", "./")
> - + "/../../../../../javax/net/ssl/etc/keystore";
> + + "/../../../../../sun/security/ssl/etc/keystore";
> private static final String passphrase = "passphrase";
>
> public static void main(String[] args) throws Exception {
>
> -Chris.
>
> [1] http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/47a5f3d6aeac
>
Re: JDK-8041679 Replace uses of StringBuffer with StringBuilder within the JDK
> - security
>
http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-security/webrev/
Looks fine to me. Thanks for making this update.
Xuelei
On 5/12/2014 6:03 PM, Paul Sandoz wrote:
> Hi,
>
> This is a request for review of Otavio's patch replacing StringBuffer
> with StringBuilder within OpenJDK. (I also need to review it.)
>
> It covers many areas and i have grouped the patches into such areas to
> aid reviewing. When commenting please including core-libs.
>
> Jtreg tests showed no regressions, but when reviewing we need to be
> mindful of the context e.g. if the buffer escapes and can cross thread
> boundaries.
>
> This is also an experiment to see if we can review the whole thing under
> one bug :-) if things prove problematic and slow we can split it
> out. Many files are touched but there are not many changes to each file
> and changes are very formulaic.
>
> I have also included ASM related changes, but i suspect we may have to
> leave those alone since such changes will make it more difficult to pull
> in ASM from upstream.
>
> -
>
> Otavio, for the record can you reply to this thread posting your single
> ("uber") patch as textual attachment? (IIUC such attachments should now
> be supported by the email server).
>
> Paul.
>
> - core
> http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-core/webrev/
>
> - io
> http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-io/webrev/
>
> - management
> http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-management/webrev/
>
> - rmi
> http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-rmi/webrev/
>
> - security
> http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-security/webrev/
>
>
> - tools
> http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-tools/webrev/
>
>
> - graphics/media
> http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-media/webrev/
>
>
> - asm
> http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-asm/webrev/
Re: RFR 8014870: Faster KDC availability check in Kerberos
I have not read the fix. I was just wondering that this fix save the wait time, but increase the networking traffics, and increase the workload of KDC servers. I think the KDC timeout should be corner cases for TCP, and it is tolerable for UDP connections. I'm not confident that this is a cost-effective update if we considering the overall system of Kerberos. Xuelei On 6/24/2014 4:17 PM, Wang Weijun wrote: > Hi All > > Please review the code change at > >http://cr.openjdk.java.net/~weijun/8014870/webrev.00/ > > In Kerberos, when trying to request for a ticket, we tried multiple KDC > servers for multiple times. Before this fix, we connect to a server, wait for > 30 seconds (the default kdc_timeout). If there is no answer, go to the next > KDC, wait for another 30 seconds, and so on. If none of the KDCs replies. We > do more rounds (max_retries, default 3) of connections, and fail at last. > Altogether with 3 KDCs we will wait at most 3*30*3=270 seconds. > > After this fix, connections are non-blocking and made every second, so they > can wait at the same time. The kdc_timeout default is also reduced to 10 > seconds (the same as other vendors). At the worst case, we will wait > 3*3+10=19 seconds. > > You might say that changing kdc_timeout to 10 seconds matters a lot here but > actually the wait-together style is much more helpful. Suppose only the 3rd > KDC is alive, the old code needs to wait for 60 seconds to be able to connect > to it, while the new one only needs to wait for 2 seconds, and this has > nothing to do with kdc_timeout. > > Because of this, I've thrown away the old krb5.kdc.bad.policy security > property. It's now not worth remembering which KDC is alive and which is not. > > All changes are inside the KdcComm.java file. Others are test and removal of > useless things. > > I've included net-dev@ because these are all NIO calls, which I was no > familiar with. > > Thanks > Max >
Re: RFR 8014870: Faster KDC availability check in Kerberos
Missed the security-dev list. On 7/7/2014 10:39 AM, Xuelei Fan wrote: > I have not read the fix. I was just wondering that this fix save the > wait time, but increase the networking traffics, and increase the > workload of KDC servers. I think the KDC timeout should be corner cases > for TCP, and it is tolerable for UDP connections. I'm not confident > that this is a cost-effective update if we considering the overall > system of Kerberos. > > Xuelei > > On 6/24/2014 4:17 PM, Wang Weijun wrote: >> Hi All >> >> Please review the code change at >> >>http://cr.openjdk.java.net/~weijun/8014870/webrev.00/ >> >> In Kerberos, when trying to request for a ticket, we tried multiple KDC >> servers for multiple times. Before this fix, we connect to a server, wait >> for 30 seconds (the default kdc_timeout). If there is no answer, go to the >> next KDC, wait for another 30 seconds, and so on. If none of the KDCs >> replies. We do more rounds (max_retries, default 3) of connections, and fail >> at last. Altogether with 3 KDCs we will wait at most 3*30*3=270 seconds. >> >> After this fix, connections are non-blocking and made every second, so they >> can wait at the same time. The kdc_timeout default is also reduced to 10 >> seconds (the same as other vendors). At the worst case, we will wait >> 3*3+10=19 seconds. >> >> You might say that changing kdc_timeout to 10 seconds matters a lot here but >> actually the wait-together style is much more helpful. Suppose only the 3rd >> KDC is alive, the old code needs to wait for 60 seconds to be able to >> connect to it, while the new one only needs to wait for 2 seconds, and this >> has nothing to do with kdc_timeout. >> >> Because of this, I've thrown away the old krb5.kdc.bad.policy security >> property. It's now not worth remembering which KDC is alive and which is not. >> >> All changes are inside the KdcComm.java file. Others are test and removal of >> useless things. >> >> I've included net-dev@ because these are all NIO calls, which I was no >> familiar with. >> >> Thanks >> Max >> >
Re: RFR: 8055299 HttpsURLConnection.equals() broken
Looks fine to me. Thanks for the update. Xuelei On 8/20/2014 9:02 PM, Michael McMahon wrote: > This is the recently reported fix to HttpsURLConnection.equals > > http://cr.openjdk.java.net/~michaelm/8055299/webrev.1/ > > The fix includes a test. Not shown in the webrev is java key store > file called testkeys, which is copied from another location > in the test tree and is to be installed in the same directory > as the new test. > > Thanks > Michael
Re: RFR 8072615: test/lib/testlibrary/jdk/testlibrary/SimpleSSLContext.java wrong on Windows
Good catch!
Looks fine to me.
Xuelei
On 2/5/2015 7:54 PM, Weijun Wang wrote:
> Hi All
>
> A test helper tries to parse the "test.src.path" system property using
> delimiter ":". This is not correct on Windows.
>
> The fix is simply
>
> diff --git a/test/lib/testlibrary/jdk/testlibrary/SimpleSSLContext.java
> b/test/lib/testlibrary/jdk/testlibrary/SimpleSSLContext.java
> --- a/test/lib/testlibrary/jdk/testlibrary/SimpleSSLContext.java
> +++ b/test/lib/testlibrary/jdk/testlibrary/SimpleSSLContext.java
> @@ -56,7 +56,7 @@
> */
> public SimpleSSLContext () throws IOException {
> String paths = System.getProperty("test.src.path");
> -StringTokenizer st = new StringTokenizer(paths,":");
> +StringTokenizer st = new StringTokenizer(paths,
> File.pathSeparator);
> boolean securityExceptions = false;
> while (st.hasMoreTokens()) {
> String path = st.nextToken();
>
> The test still runs fine now, because when "C:\x;c:\y" is divided into
> "C", "\x;c" and "\y". The useful part "\y" still somehow works. If
> test.src and jtreg working directory are on different drives, the test
> would fail.
>
> Thanks
> Max
Re: TLS ALPN Proposal v5
On 9/25/2015 7:45 AM, Bradford Wetmore wrote:
>
> On 9/23/2015 2:33 AM, Simone Bordet wrote:
>> Hi,
>>
>> On Wed, Sep 23, 2015 at 7:04 AM, Bradford Wetmore
>> wrote:
>>>
This new proposal still requires that ciphers are sorted in a way that
matches the ApplicationProtocol order.
Would be nice if, along with the HTTP/2 blacklist, there is a HTTP/2
comparator that sorts ciphers putting the blacklisted ones at the end.
>>>
>>> Hm...is the sample code at the end of the initial class description
>>> insufficient? Adding a comparator seems a little heavyweight in that it
>>> could require access to the ciphersuite internals and would add a lot of
>>> complexity for this one known use case. When TLSv1.3 is done, the
>>> blacklist
>>> stuff in HTTP/2 goes away.
>>
>> Sure, but until TLS 1.3 widely deployed, applications will have to
>> sort the ciphers to put HTTP/2 ones before the blacklisted ones.
>> Providing this comparator is as trivial as providing
>> ApplicationProtocol.HTTP2BLACKLIST, so I thought to mention it.
>
> I learned something today: Collections/Arrays.sort()/others are stable.
> (i.e. equal elements will not be reordered as a result of the sort) My
> expectation of "equals" was .equals(), not return value == 0.
>
> I was concerned that such a Comparator might reorder valid H2 suites
> from what was passed in. Thankfully, that's not the case, so I've added
> this Comparator. There is a warning now about "consistency with
> equals()", as the Strings obvioulsy won't be equals() and thus sorted
> sets/maps just won't work. (See the Comparator pages for further
> discussion.)
It might be not customers expected behavior to re-order/sort their
preference of cipher suites or preference.
For example, a client wants to negotiate {HTTP2, HTTP1.1} or {HTTP1.1,
HTTP2} and {TLS_RSA_WITH_AES_128_CBC_SHA,
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384}.
HTTP1.1/TLS_RSA_WITH_AES_128_CBC_SHA should be negotiated per the TLS
and HTTP2 specification. If the cipher suites are sorted,
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 would be negotiated, this is not
what the customer really expected. The customer preference should be
respected.
I don't think we really need to re-order the cipher suites.
Let's consider the following client requests (prefer cipher suite more
than application protocol; blacklisted_CS are HTTP2 blacklisted cipher
suite):
1. {HTTP2, HTTP1.1} {strong_cipher_site, blacklisted_CS}
HTTP2 and strong_cipher_site should be negotiated. Need not to re-order
cipher suites.
2. {HTTP1.1, HTTP2} {strong_cipher_site, blacklisted_CS}
HTTP1.1 and strong_cipher_site should be negotiated. Need not to
re-order cipher suites.
3. {HTTP2, HTTP1.1} {blacklisted_CS, strong_cipher_site}
HTTP1.1 and blacklisted_CS should be negotiated. Need not to re-order
cipher suites.
4. {HTTP1.1, HTTP2} {blacklisted_CS, strong_cipher_site}
HTTP1.1 and blacklisted_CS should be negotiated. Need not to re-order
cipher suites.
5. {HTTP2} {strong_cipher_site, blacklisted_CS}
HTTP2 and strong_cipher_site should be negotiated. Need not to re-order
cipher suites.
6. {HTTP1.1} {strong_cipher_site, blacklisted_CS}
HTTP1.1 and strong_cipher_site should be negotiated. Need not to
re-order cipher suites.
7. {HTTP2} {blacklisted_CS, strong_cipher_site}
blacklisted_CS would be filtered out as it does not appy to HTTP2. Only
strong_cipher_site presents in ClientHello message.
HTTP2 and strong_cipher_site should be negotiated. Need not to re-order
cipher suites.
8. {HTTP1.1} {blacklisted_CS, strong_cipher_site}
HTTP1.1 and blacklisted_CS should be negotiated. Need not to re-order
cipher suites.
One concern may be that, the customer is not intent to negotiate HTTP2
blacklisted cipher suite. The customer just don't know which are the
strong cipher suites among many cipher suites. I think we may need a
handy tool to order the cipher suites before configuration.
// there are a few cipher suites are available
String[] cipherSuites = ... // a array of cipher suites.
// Q: Don't know the strength of them
// A: OK, there is a handy tool
cipherSuites = cipherSuiteReorder.sort(cipherSuites);
// configure the cipher suites
SSLParameters.setCipherSuites(cipherSuites);
The order also apply to the normally cipher suites configuration, not
only to application protocols. The re-order should be called by
customers explicitly. JSSE would better not sort them automatically.
I think, the handy sort tool cannot be simply bind to application
protocol. For example, HTTP2 has a blacklisted cipher suites. OK,
ApplicationProtocol.H2BLACKLISTCOMPARATOR is expected to make the sort.
If, in the future, a new application protocol (AP_NEW) has a different
blacklist cipher suites, a new
ApplicationProtocol.APNEWBLACKLISTCOMPARATOR would be defined. If both
{HTTP2, AP_NEW} would be requested, which comparator for the sorting
would be used? None of them can sort the cipher suite properly. The
comparator des
Re: TLS ALPN Proposal v5
On 9/25/2015 4:11 PM, Simone Bordet wrote:
> Hi,
>
> On Fri, Sep 25, 2015 at 3:44 AM, Xuelei Fan wrote:
>> For example, a client wants to negotiate {HTTP2, HTTP1.1} or {HTTP1.1,
>> HTTP2} and {TLS_RSA_WITH_AES_128_CBC_SHA,
>> TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384}.
>> HTTP1.1/TLS_RSA_WITH_AES_128_CBC_SHA should be negotiated per the TLS
>> and HTTP2 specification. If the cipher suites are sorted,
>> TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 would be negotiated, this is not
>> what the customer really expected. The customer preference should be
>> respected.
>>
>> I don't think we really need to re-order the cipher suites.
>
> "We" as in the OpenJDK implementation *must not* reorder the cipher suites.
Yes. I agree with this point, OpenJDK MUST NOT reorder the cipher suites.
> "We" as in an application that wants to use HTTP/2 *must* reorder the
> cipher suites.
In an application, the cipher suites preference MUST be used properly,
but the "reorder" behavior may be not the application expected order.
For some reasons, applications (or customers) may want the preference of
{TLS_RSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384},
rather than reordered to {TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,
TLS_RSA_WITH_AES_128_CBC_SHA}. Customers preference should be
respected, application and OpenJDK MUST NOT reorder the cipher suites.
If there are three roles, OpenJDK, application, customers, there are
three result:
1. OpenJDK MUST NOT reorder the cipher suites if no application request.
2. Applications MUST NOT reorder the cipher suites if no customer request.
3. Customers can set the cipher suites order according to they requirements.
> The comparator provided is only a help to the application to perform
> this reordering.
>
>> Let's consider the following client requests (prefer cipher suite more
>> than application protocol; blacklisted_CS are HTTP2 blacklisted cipher
>> suite):
>>
>> 1. {HTTP2, HTTP1.1} {strong_cipher_site, blacklisted_CS}
>> HTTP2 and strong_cipher_site should be negotiated. Need not to re-order
>> cipher suites.
>>
>> 2. {HTTP1.1, HTTP2} {strong_cipher_site, blacklisted_CS}
>> HTTP1.1 and strong_cipher_site should be negotiated. Need not to
>> re-order cipher suites.
>>
>> 3. {HTTP2, HTTP1.1} {blacklisted_CS, strong_cipher_site}
>> HTTP1.1 and blacklisted_CS should be negotiated. Need not to re-order
>> cipher suites.
>
> Of course you need to re-order in this case.
> The application has just provided a preference for HTTP/2 in the
> application protocol list, it actually happens that HTTP/2 could be
> negotiated because strong ciphers are present, but instead HTTP/1.1 is
> chosen.
>
Here is the question to answer, which preference should be respected
firstly between cipher suite and application protocol? If application
protocol are preferred at first, of course, application preference
should be respected at first; otherwise, cipher suite preference should
be respected at first.
Let's re-write the example, among {application_protocol_1,
application_protocol_2} and {cipher_suite_for_AP2,
cipher_suite_for_AP1}. If application protocol preference get
respected, application_protocol_1/cipher_suite_for_AP1 would be
negotiated; If cipher suite preference get respected,
application_protocol_2/cipher_suite_for_AP2 should be negotiated.
>From my understand, cipher suite preference should be preferred over
application protocols.
>> 4. {HTTP1.1, HTTP2} {blacklisted_CS, strong_cipher_site}
>> HTTP1.1 and blacklisted_CS should be negotiated. Need not to re-order
>> cipher suites.
>>
>> 5. {HTTP2} {strong_cipher_site, blacklisted_CS}
>> HTTP2 and strong_cipher_site should be negotiated. Need not to re-order
>> cipher suites.
>>
>> 6. {HTTP1.1} {strong_cipher_site, blacklisted_CS}
>> HTTP1.1 and strong_cipher_site should be negotiated. Need not to
>> re-order cipher suites.
>>
>> 7. {HTTP2} {blacklisted_CS, strong_cipher_site}
>> blacklisted_CS would be filtered out as it does not appy to HTTP2. Only
>> strong_cipher_site presents in ClientHello message.
>>
>> HTTP2 and strong_cipher_site should be negotiated. Need not to re-order
>> cipher suites.
>>
>> 8. {HTTP1.1} {blacklisted_CS, strong_cipher_site}
>> HTTP1.1 and blacklisted_CS should be negotiated. Need not to re-order
>> cipher suites.
>>
>> One concern may be that, the customer is not intent to negotiate HTTP2
>> blacklisted cipher suite. The customer just don't know which are the
>> strong cipher suites among many cipher suites. I think we may need a
>> handy tool to order the cipher suites before configuratio
Re: TLS ALPN Proposal v5
On 9/25/2015 7:42 PM, Simone Bordet wrote:
> Hi,
>
> On Fri, Sep 25, 2015 at 11:47 AM, Xuelei Fan wrote:
>> Here is the question to answer, which preference should be respected
>> firstly between cipher suite and application protocol? If application
>> protocol are preferred at first, of course, application preference
>> should be respected at first; otherwise, cipher suite preference should
>> be respected at first.
>
> The answer to this question has been decided when the algorithm has
> been chosen to be:
>
> for each cipher
> for each application protocol
> end
> end
>
> All the rest being equal, ciphers dominate application protocol selection.
>
> Are you suggesting to change this to:
>
> for each application protocol
> for each cipher
> end
> end
>
> ?
>
No, I would prefer the first approach. Good, we are at the same page at
this point, I think.
> It's in the hands of the role that configures application protocols
> and ciphers to decide whether it's more important to prefer a protocol
> or a cipher.
>
> Put it in a different way:
>
> If the role prefers application protocols, it has to sort the ciphers
> to influence that.
> If the role prefers ciphers, it has to sort the ciphers.
>
I did not catch the idea. if the tole prefers ciphers, why still need
to sort the ciphers? Sort for what?
> No matter what, it has to sort the ciphers.
>
I did not get your ideas. Who would sort the preferences? and when the
preferences should be sorted?
Maybe, we are not on the same page, I think.
>From the viewpoint of application or library, if an administrator had
made his decision about the preference of application protocols and
cipher suites, applications and OpenJDK may be better to respect the
decision.
// the customers' decision//(1)
String[] cipherSuites = {}; // customer preference, sorted.
List appProtocols =
{...}; // customer preference, sorted.
// configure the parameters // (2)
//
// customer preferences should be respected.
sslParameters.setCipherSuites(cipherSuites);
sslParameters.setApplicationProtocols(appProtocols);
There might be concerns, customers preference may be not right and need
to adjusted. Maybe, a re-order tool is needed before (not after) the
configuration of the parameters.
// the customers' decision // (1)
String[] cipherSuites = {}; // customer preference
List appProtocols =
{...}; // customer preference
+
+ // reorde the cipher suites // (1.1)
+ cipherSuites = ... // the actual customer preference
+
+ // reorder application protocols preference
+ appProtocols = ... // the actual customer preference
// configure the parameters // (2)
//
// customer preferences should be respected.
sslParameters.setCipherSuites(cipherSuites);
sslParameters.setApplicationProtocols(appProtocols);
>> Therefore, personally, I think application may want a handy tool to sort
>> the cipher suite for the strength for general purpose, but not for
>> application protocol.
>
> Because HTTP/2 would probably be popular given the success of its
> predecessor, it would be handy to have a HTTP/2 comparator to
> influence the selection of the HTTP/2 protocol.
>
> Nothing forbids to offer a comparator by cipher strength too.
>
I think a general cipher strength comparator is sufficient for HTTP/2
preference too. What do you think?
Maybe, need no extra comparator for HTTP2. Extra comparator would make
behaviors pretty complicated and hard to get expected, as I described in
the previous mail.
Xuelei
Re: TLS ALPN Proposal v5
On 9/25/2015 8:48 PM, Simone Bordet wrote:
> Hi,
>
> On Fri, Sep 25, 2015 at 2:26 PM, Xuelei Fan wrote:
>> Maybe, we are not on the same page, I think.
>
> We are.
>
>> I think a general cipher strength comparator is sufficient for HTTP/2
>> preference too. What do you think?
>
> I don't know if all the blacklisted ciphers are actually lower
> strength of all the remaining ciphers, nor what is the exact
> definition of "strength" that you can use in a comparator.
> But because the HTTP/2 specification bothered to say what's
> blacklisted, I would just use that.
>
HTTP/2 blacklists them because they are no so strong because of various
reasons.
The definition of "strength" can be customized by the customers.
Therefore, I don't worry too much because of this flexibility.
>> Maybe, need no extra comparator for HTTP2. Extra comparator would make
>> behaviors pretty complicated and hard to get expected, as I described in
>> the previous mail.
>
> There is no complication, really. Two comparators would compose, not exclude.
>
For the complication, I posted the comments in previous mail here:
-
> In case you have [HTTP/2, AP_NEW, HTTP/1.1], then you can simply
> compose the comparators to sort first with the H2.CIPHER_COMPARATOR,
> then with AP_NEW.CIPHER_COMPARATOR.
>
> cipherSuites = Arrays.sort(cipherSuites,
>ApplicationProtocol.H2.CIPHER_COMPARATOR.
>thenComparing(AP_NEW.CIPHER_COMPARATOR));
>
Let's look at an example. application_protocol_1 prefer cipher_suite_1,
and application_protocol_1 prefer cipher_suite_2.
The comparator for application_protocol_1 would set the preference as
{cipher_suite_1, cipher_suite_2}. and the comparator for
application_protocol_2} would set the preference as {cipher_suite_2,
cipher_suite_1}.
The result to sort 1 and then 2, and the result to sort 2 and then 1 are
different.
The call sequence to the comparators, and the call to each comparator
would result in difference result. That's may be not the expected behavior.
-
It's not easy to use application comparator, I think.
> For example, let's say that you want to sort by "bit" strength in a
> way that you want to prefer 128 bits or lower to favor performance,
> but also do HTTP/2.
>
> The H2 comparator will sort the blacklisted at the end.
> Among the good ones, they all compare == 0 for the H2 comparator.
> That is where the second comparator will kick in, grep the cipher name
> for the bits number and sort them accordingly.
>
It's flexible to meet the requirement by customer's customization.
Maybe, extra comparator is not necessary here. I think the comparator
can be a handy tool, but not belong to ALPN.
Xuelei
Re: TLS ALPN Proposal v5
On 9/25/2015 9:14 PM, Simone Bordet wrote: > On Fri, Sep 25, 2015 at 2:46 PM, David M. Lloyd > wrote: >> > Well, SNI already basically works this way, so it's not so great a stretch. > I don't follow ? > SNI has APIs in JDK 8, you don't use SSLExplorer at all. There are two typical cases for SNI and ALPN. One is that the same server is used for difference SNI/ALPN. Another one is that different server is used for different SNI/ALPN. For example, there is a TLS server 101.101.1.1 for delegation. If SNI www.example.com get requested, the delegation server may redirect the connection to 192.168.1.100 (provide the service for www.example.com). If SNI www.another.com get requested, the delegation server may redirect the connection to 192.168.1.101 (provide the service for www.another.com). Similarly, ALPN need to support the case as above. Another example, there is a server 101.101.1.1. If SNI www.example.com get requested, the server would act as the service for www.example.com. If SNI www.another.com get requested, the server would act as the service for www.another.com. Similarly, ALPN also need to support the case as above. Actually, it was a puzzle to me: whether a concrete server can support both HTTP/2 and HTTP/1.1, or not. If HTTP/2 mode of the server does not work, is it OK to fall-over to use HTTP/1.1 mode? I did not get the answer from the HTTP/2 spec. The current design is based on the idea that a concrete server may support both HTTP/2 and HTTP/1.1 at the same time, and fall-over mode is necessary. Otherwise, the API and implementation can be simplified a lot. Xuelei
Re: TLS ALPN Proposal v5
On 9/25/2015 10:20 PM, Simone Bordet wrote:
> Hi,
>
> On Fri, Sep 25, 2015 at 3:20 PM, Xuelei Fan wrote:
>> For the complication, I posted the comments in previous mail here:
>>
>> -
>>> In case you have [HTTP/2, AP_NEW, HTTP/1.1], then you can simply
>>> compose the comparators to sort first with the H2.CIPHER_COMPARATOR,
>>> then with AP_NEW.CIPHER_COMPARATOR.
>>>
>>> cipherSuites = Arrays.sort(cipherSuites,
>>>ApplicationProtocol.H2.CIPHER_COMPARATOR.
>>>thenComparing(AP_NEW.CIPHER_COMPARATOR));
>>>
>> Let's look at an example. application_protocol_1 prefer cipher_suite_1,
>> and application_protocol_1 prefer cipher_suite_2.
>>
>> The comparator for application_protocol_1 would set the preference as
>> {cipher_suite_1, cipher_suite_2}. and the comparator for
>> application_protocol_2} would set the preference as {cipher_suite_2,
>> cipher_suite_1}.
>>
>> The result to sort 1 and then 2, and the result to sort 2 and then 1 are
>> different.
>>
>> The call sequence to the comparators, and the call to each comparator
>> would result in difference result. That's may be not the expected behavior.
>
> The example is malformed, since it does not specify which ciphers are
> good for which application protocol, and neither the order of the
> application protocols.
>
> Let me rewrite it:
>
> application protocols: [ap1, ap2]
> ciphers: [c1, c2]
>
> ap1 requires c1, does not work with c2
> ap2 requires c2, does not work with c1
>
OK, as make the case more simple.
> Now the question is: you have to configure your system, what you want to do ?
>
> If you want to favor ap1, then you sort [c1, c2]
> If you want to favor ap2, then you sort [c2, c1]
> If you want to favor c1, then you sort [c1, c2]
> If you want to favor c2, then you sort [c2, c1]
>
> If you want to favor ap1 *and* c2, you have to decide what is more
> important between the two, because you cannot have both.
>
> I don't see any problem, really.
>
If "you" as the customer, I don't see any problem. If "you" as OpenJDK,
the problem is that OpenJDK know nothing about the conditions (if you
want to favor ...), and therefore cannot make the decision (sort)
internally.
I think we should be on the same page: customers can make any sort as
they want. SSLParameters.setCipherSuite() and
SSLParameters.setAppliationProtocols() can be used for any decision they
made.
> That the results are different, sure, but they are predictable.
> When the configuration uses one comparator, it will always be that
> result, and same for the other comparator.
>
> But you configure the comparators in base of what you want to do.
>
Need a confirmation. I think you agree that the sort happens before the
calls to SSLParameters.setCipherSuite() and
SSLParameters.setAppliationProtocols(), right? As the case I commented
in previous mail:
// the customers' decision // (1)
String[] cipherSuites = {}; // customer preference
List appProtocols =
{...}; // customer preference
+
+ // reorde the cipher suites // (1.1)
+ cipherSuites = ... // the actual customer preference
+
+ // reorder application protocols preference
+ appProtocols = ... // the actual customer preference
// configure the parameters // (2)
//
// customer preferences should be respected.
sslParameters.setCipherSuites(cipherSuites);
sslParameters.setApplicationProtocols(appProtocols);
Xuelei
Re: TLS ALPN Proposal v5
CC security-dev. On 9/25/2015 9:14 PM, Simone Bordet wrote: > Hi, > > On Fri, Sep 25, 2015 at 2:46 PM, David M. Lloyd > wrote: >> Well, SNI already basically works this way, so it's not so great a stretch. > > I don't follow ? > SNI has APIs in JDK 8, you don't use SSLExplorer at all. > > Also, SNI is a client-to-server information only, while with ALPN you > have to reply to the client, so you have to modify the ServerHello. > I don't see how you can do this without support from the JDK via APIs ? > >> I imagine the client code simply specifying a list of protocols along with >> today's list of cipher suites. >> >> The user-space server side logic would go like this: >> >> * Receive SSL ServerHello >> * Examine the packet for ALPN and SNI information >> * Read the list of cipher suites >> * Evaluate >> * Select an SSLContext based on protocol and/or server name >> * Construct an SSLSocket or SSLEngine as appropriate >> * Set a property on the SSLSocket/SSLEngine to indicate ALPN protocol name >> * (optional) Change/sort the cipher suite list on the SSLSocket/SSLEngine as >> appropriate >> * Resume negotation by passing the ServerHello in to the SSLSocket/SSLEngine >> as initial data >> >> It's not super elegant but it should work just as well as SNI works, and it >> would cover 100% of use cases since the user has complete flexibility to >> make a decision based on any combination of cipher suite selection, protocol >> name, and host name, even potentially with the option to pretend that ALPN >> wasn't recognized. > > Are you saying that every application has to write its own TLS parser ? > Would not that be overkill and full of potential security issues if > one does not get the implementation strictly correct ? > > Also, what if the JDK implementation refuses to use the cipher you > chose along with the application protocol, for whatever reason ? > > Thanks ! >
Re: whether a concrete server can support,both HTTP/2 and HTTP/1.1, or not?
On 9/25/2015 10:28 PM, Simone Bordet wrote: > Hi, > > On Fri, Sep 25, 2015 at 4:19 PM, Xuelei Fan wrote: >> Actually, it was a puzzle to me: whether a concrete server can support >> both HTTP/2 and HTTP/1.1, or not. If HTTP/2 mode of the server does not >> work, is it OK to fall-over to use HTTP/1.1 mode? I did not get the >> answer from the HTTP/2 spec. > > Yes, there was an answer to Bradford's email: > https://lists.w3.org/Archives/Public/ietf-http-wg/2015JulSep/0160.html. > The answer is "yes". > Thanks! It would be nice to document the specification officially that HTTP2 can be fail-over to HTTP 1.1 (HTTP protocol version negotiation). Xuelei
Re: TLS ALPN Proposal v5
I would second David's proposal based on the #1/#A ideas.
Here are some background and options.
1. a HTTP2 server should support HTTP2
If a HTTP2 server declare to support HTTP2, it should support HTTP2
protocol. What happens if corner cases happen that the security is not
sufficient (client requested cipher suites are all blacklisted)? Two
approaches, refuse the connection or complete the connection.
It is easy to understand if refusing the connection for corner cases.
But why complete the connection with insufficient cipher suite? It's
because for HTTP2, the client side need to check the security strength
outside of SSL/TLS layer in case the SSL/TLS server uses insufficient
security parameters. If the security is insufficient, the client side
can request abbreviate handshaking (Section 3.1, RFC 7301) for a low
level application protocols (application protocol downgrade), or close
the connection.
Here is the scenarios:
A.1. client requests {HTTP2, HTTP1.1)
A.2. server negotiates HTTP2 and HTTP2 blacklisted cipher suite.
A.3. client checks the security sufficiency. The negotiated cipher suite
is insufficient.
A.4. client requests an abbreviated handshaking for {HTTP 1.1}
A.5. server negotiates HTTP1.1
A.6. client checks the security sufficiency. The negotiated cipher suite
is sufficient for HTTP 1.1.
There are might be one question? Should client check the security
sufficiency even if OpenJDK can do it pretty well. I would think client
need to do so because the client may also want to connect to other TLS
vendors, known or unknown. Client need to make sure the negotiated TLS
strength is sufficient for the specified application protocol.
If the story happens in this way, everything get more simple.
2. a HTTP2 server should support both HTTP2 and HTTP1.1
If an HTTP2 server support both HTTP2 and HTTP1.1, AND it is expected to
do HTTP protocol version negotiated by itself internally (fail-over to
HTTP 1.1) (I did not find the spec in HTTP2 RFC, but HTTP experts said a
server may support both HTTP2 and HTTP1.1), what happens if corner cases
happen that the security is not sufficient for HTTP 2? Three approaches:
refuse the connection, complete the connection with HTTP2, fail-over to
HTTP1.1 in server side.
The first two approaches are the same as #1. For the 3rd approach, the
server implementation and APIs design get much more complicated.
For the 2nd approach, the scenarios is similar to #1.1. The server
implementation part may looks like:
B.1. receive a ClientHello message.
B.2. negotiate the TLS protocol version, independently
B.3. negotiate the TLS cipher suite, independently
B.4. negotiate the application protocols, independently
B.5. sending the ServerHello message.
For the 3rd approach, the server implementation may looks like:
C.1. receive a ClientHello message.
C.2. negotiate the TLS protocol version, independently, get
negotiated_protocol
C.3. try to negotiate an TLS cipher suite, candidate_cipher_suite
C.4. try to negotiate an application protocol with negotiated_protocol
and candidate_cipher_suite. If the application protocol does not work
for the TLS protocol and cipher suites, goto #3.3 and try again.
Otherwise, move forward.
C.5. if application protocol can be negotiated, sending the ServerHello
message; otherwise, terminated the connection immediately.
For the case above, looks like client may not need to do application
protocol renegotiation. But actually, it may be able to avoid the coding
job because the client may not know whether the server would behavior
like #B, or #C.
#1/#A do the application protocol version negotiation in client side,
and #2/#C do the application protocol version negotiation in server
side. And there is no spec about which behavior should be expected as
far as I know. #1/#A is simple and straightforward, and #2/#C makes a
smart (and complicated) server.
OpenJDK now is trying to support both #1 and #2.
Thanks,
Xuelei
On 9/26/2015 4:22 AM, David M. Lloyd wrote:
> On 09/25/2015 02:11 PM, Simone Bordet wrote:
>> Hi,
>>
>> On Fri, Sep 25, 2015 at 7:23 PM, David M. Lloyd
>> wrote:
>>> The application protocol implementation chooses only valid cipher
>>> suites for
>>> the protocol. Why would it choose one that is not valid, considering
>>> that
>>> the protocol implementation itself is the only thing that "knows"
>>> what is
>>> valid or not?
>>
>> The cipher could fail for the number of reasons it fails in
>> trySetCipherSuite(), even if the application has chosen the right
>> combination of (application protocol, cipher, whatever else).
>> At that point you have to try another application protocol.
>
> From my reading of that code, it can only fail if you specifically set
> up invalid combinations of cipher suite, protocol, and credentials. The
> application code should have all the information it needs to set up a
> correct configuration though.
>
>>> This validation should have happened before the JDK ever has a chance
>>> to be
>>> involved.
>>
>>
Re: TLS ALPN Proposal v5
On 9/26/2015 8:47 AM, Bradford Wetmore wrote:
>> {TLS_RSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384}
>
> BTW, in case anyone was wondering, both of these suites are on the RFC
> 7540 blacklist.
Ooops, I meant to use "TLS_ECDHE_" as HTTP2 non-blacklisted cipher suite.
Xuelei
Re: TLS ALPN Proposal v5
On 9/26/2015 8:47 AM, Bradford Wetmore wrote: >> It might be not customers expected behavior to re-order/sort their >> preference of cipher suites or preference. > > Are we are clear that the intention was never for the JDK to internally > resort the ciphersuites, but rather to provide an external helper > function (H2BLACKLISTCOMPARATOR) with which applications can do their > own sorting and pass the results to setEnabledCiphersuite()? My points: 1. OpenJDK should not do the re-sort internally. The preference decision should be made before the call to setEnabledCiphersuite(). I think Simone agreed with this point. 2. A handy function to resort the cipher suite is useful. But it is out of the scope of ALPN, or even out of the scope of OpenJDK. Application can do whatever resorting, H2BLACKLISTCOMPARATOR does not belong to OpenJDK. Xuelei
Re: TLS ALPN Proposal v6
On 10/2/2015 9:03 AM, Bradford Wetmore wrote: > Major changes: > > 1. ApplicationProtocols is gone. The H2 black list and comparator were > moved to StandardConstants. > > 2. StandardConstants. Strings for "h2" and "http/1.1" are back. And > now that you are parsing the raw network bytes, I added a convenience > mapping between the two byte ciphersuite IANA-assigned value and the > Java Standard Name. > There is no SSLExplorer in OpenJDK. I think, maybe, the map is not belong to OpenJDK either. I think, the constants for HTTP2 is also belong to application protocol (HTTP2) layer. Application (HTTP2) implementation would take care of them. Maybe, they are not a part of JSSE framework either. I would like to have "h2" and "http/1.1" defined as Standard Algorithms Docs as we usually did for other standard constants. > 3. SSLParameter (set/get) are moved to SSLSocket/SSLEngine. Even > though these could go into SSLParameters, this change makes backporting > much easier. The helper code simply has to reflectively look for the > four methods in the implementation classes, and call if they are there. > > Otherwise, there would have to be reflection both in the user code > (above) and implementation (to see if the passed SSLParameters had the > new methods via a subclass). But, looking forward, per JSSE framework, SSLParameters should be the central place to define SSL/TLS configuration parameters. We'd better follow the conventions so that application developers won't get confused about where SSL/TLS parameters should be configured. Maybe, we cannot add public APIs for backporting. I think backporting is another history, and would better not impact too much of the design for JDK 9 and future releases. Hope it helps! Xuelei
Re: TLS ALPN Proposal v7
Thanks! Xuelei On 10/3/2015 8:19 AM, Bradford Wetmore wrote: > > > On 10/1/2015 7:35 PM, Xuelei Fan wrote: >> On 10/2/2015 9:03 AM, Bradford Wetmore wrote: >>> Major changes: >>> >>> 1. ApplicationProtocols is gone. The H2 black list and comparator were >>> moved to StandardConstants. >>> >>> 2. StandardConstants. Strings for "h2" and "http/1.1" are back. And >>> now that you are parsing the raw network bytes, I added a convenience >>> mapping between the two byte ciphersuite IANA-assigned value and the >>> Java Standard Name. >>> >> There is no SSLExplorer in OpenJDK. I think, maybe, the map is not >> belong to OpenJDK either. >> >> I think, the constants for HTTP2 is also belong to application protocol >> (HTTP2) layer. Application (HTTP2) implementation would take care of >> them. Maybe, they are not a part of JSSE framework either. > > Ok, I've removed all of the H2 constants from StandardConstants. I've > mentioned to the the H2/network team that this is something they will > need to handle/include (e.g. Blacklist/Comparator) in their code, and > they might want to consider APIs similar to what I had. > > I personally found it very useful to have a proper mapping to get the > SSL_ vs. TLS_ prefix correct in the blacklist. > >> I would like to have "h2" and "http/1.1" defined as Standard Algorithms >> Docs as we usually did for other standard constants. > > Of course, they will be added as Standard Algorithm names. > >>> 3. SSLParameter (set/get) are moved to SSLSocket/SSLEngine. Even >>> though these could go into SSLParameters, this change makes backporting >>> much easier. The helper code simply has to reflectively look for the >>> four methods in the implementation classes, and call if they are there. >>> >>> Otherwise, there would have to be reflection both in the user code >>> (above) and implementation (to see if the passed SSLParameters had the >>> new methods via a subclass). >> But, looking forward, per JSSE framework, SSLParameters should be the >> central place to define SSL/TLS configuration parameters. We'd better >> follow the conventions so that application developers won't get confused >> about where SSL/TLS parameters should be configured. > > I went back and forth on this many, many times yesterday. > >> Maybe, we cannot add public APIs for backporting. > > I figured we'd have to use reflection on user-derived classes to see if > the methods were there, but apparently implementation-specifc APIs can > be added in an update release, just not Java APIs. So if we really can > do that, then we can add a jdk.SSLParameters/ > org.openjdk.jsse.SSLParameters extends SSLParameters, and in the > implementation, look for instanceof. > >> I think backporting is >> another history, and would better not impact too much of the design for >> JDK 9 and future releases. > > Ok, so get/setApplicationProtocols() is back in SSLParameters. > > Thanks for the comments everyone. I'm submitting the following to the > CCC (internal review board): > > http://cr.openjdk.java.net/~wetmore/8051498/webrev.17/ > > Changes: > > 1. No H2 Blacklist/Comparator > > 2. set/getApplicationProtocols() back to SSLParameters. > > Thanks, > > Brad >
Re: Request for review: 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation Extension
Looks fine to me. Just a few minor comments.
ServerHandshaker.java
=
There is a typo of the first line.
- /* Copyright (c) 1996, 2015, ...
+ /*
+ * Copyright (c) 1996, 2015 ...
line 538-546
String negotiatedValue = null;
List protocols = clientHelloALPN.getPeerAPs();
for (String ap : localApl) {
if (protocols.contains(ap)) {
negotiatedValue = ap;
break;
}
}
I think the above order prefer the server preference order of
application protocols. It's OK per Section 3.2 of RFC 7301.
However RFC 7301 also defines the client preference order. Looks like
the client preference order is not necessary. It's a little bit
confusing, and may result in different behaviors for different TLS vendors.
Maybe, we can add an option to honor server preference order in the future.
ALPNExtension.java
==
96listLength = s.getInt16(); // list length
The code would throws SSLException if the remaining input is not
sufficient. I was wondering, SSLProtocolException may be more suitable
here. May be nice to check the "len" argument value if sufficient for
the list length. Otherwise, a SSLProtocolException would be thrown.
if (len >= 2) {
listLength = s.getInt16(); // list length
...
} else {
throw new SSLProtocolException(...);
}
---
104 byte[] bytes = s.getBytes8();
105 String p =
106 new String(bytes, StandardCharsets.UTF_8); // app protocol
Do you want to check that the app protocol cannot be empty?
// opaque ProtocolName<1..2^8-1>; // RFC 7301
byte[] bytes = s.getBytes8();
+if (bytes.length == 0) {
+throw new SSLProtocolException("Empty ...");
+}
-
Personally, I would prefer to use unmodifiableList for the protocolNames
variable.
HelloExtensions.java
line 34-37:
* This file contains all the classes relevant to TLS Extensions for the
* ClientHello and ServerHello messages. The extension mechanism and
* several extensions are defined in RFC 3546. Additional extensions are
* defined in the ECC RFC 4492.The ALPN extension is defined in RFC7301.
I may put the "Additional extensions ..." at the end. BTW, as you are
here already, would you mind update RFC 3546 to RFC 6066? RFC 6066 is
the newest revision of RFC 3546.
* This file contains all the classes relevant to TLS Extensions for the
* ClientHello and ServerHello messages. The extension mechanism and
* several extensions are defined in RFC 6066. The ALPN extension is
* defined in RFC 7301. Additional extensions are defined in the ECC
* RFC 4492.
SSLEngineImpl.java
==
line 1411-1412:
private Ciphertext writeRecord(ByteBuffer[] appData,
int offset, int length, ByteBuffer netData) throws IOException {
...
if (...) {
...
// set connection ALPN value
applicationProtocol =
handshaker.getHandshakeApplicationProtocol();
...
}
}
Is it redundant to set applicationProtocol here. I was wondering, the
value should set in the processInputRecord() method.
Cheers,
Xuelei
On 11/30/2015 8:08 AM, Vincent Ryan wrote:
> Hello,
>
> Following on from Brad’s recent email, here is the full webrev of the
> API and the implementation classes for ALPN:
> http://cr.openjdk.java.net/~vinnie/8144093/webrev.00/
>
> In adds the implementation classes (sun/security/ssl) to the public API
> classes (javax/net/ssl) which have already been agreed.
> Some basic tests (test/javax/net/ssl) are also included.
>
> Please send any code review comments by close-of-business on Tuesday 1
> December.
> Thanks.
Re: Request for review: 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation Extension
Looks fine to me. Thanks for the update.
Xuelei
On 12/1/2015 7:08 AM, Vincent Ryan wrote:
> Thanks for your review. Comments in-line.
>
>> On 30 Nov 2015, at 06:30, Xuelei Fan wrote:
>>
>> Looks fine to me. Just a few minor comments.
>>
>> ServerHandshaker.java
>> =
>> There is a typo of the first line.
>> - /* Copyright (c) 1996, 2015, ...
>> + /*
>> + * Copyright (c) 1996, 2015 …
>>
> Done.
>
>>
>> line 538-546
>>
>> String negotiatedValue = null;
>> List protocols = clientHelloALPN.getPeerAPs();
>>
>> for (String ap : localApl) {
>> if (protocols.contains(ap)) {
>> negotiatedValue = ap;
>> break;
>> }
>> }
>>
>> I think the above order prefer the server preference order of
>> application protocols. It's OK per Section 3.2 of RFC 7301.
>>
>> However RFC 7301 also defines the client preference order. Looks like
>> the client preference order is not necessary. It's a little bit
>> confusing, and may result in different behaviors for different TLS vendors.
>>
>> Maybe, we can add an option to honor server preference order in the future.
>
> I added a comment to show that server-preference is used.
> If necessary I think we can add support for controlling this via a property,
> later.
>
>
>>
>> ALPNExtension.java
>> ==
>> 96listLength = s.getInt16(); // list length
>>
>> The code would throws SSLException if the remaining input is not
>> sufficient. I was wondering, SSLProtocolException may be more suitable
>> here. May be nice to check the "len" argument value if sufficient for
>> the list length. Otherwise, a SSLProtocolException would be thrown.
>>
>>if (len >= 2) {
>>listLength = s.getInt16(); // list length
>>...
>>} else {
>>throw new SSLProtocolException(...);
>>}
>
> I added the exception and more detailed message.
>
>>
>> ---
>> 104 byte[] bytes = s.getBytes8();
>> 105 String p =
>> 106 new String(bytes, StandardCharsets.UTF_8); // app protocol
>>
>> Do you want to check that the app protocol cannot be empty?
>>
>> // opaque ProtocolName<1..2^8-1>; // RFC 7301
>> byte[] bytes = s.getBytes8();
>> +if (bytes.length == 0) {
>> +throw new SSLProtocolException("Empty ...");
>> +}
>
> Done.
>
>
>>
>> -
>> Personally, I would prefer to use unmodifiableList for the protocolNames
>> variable.
>
> It is accessible only to classes in this package.
>
>
>>
>>
>> HelloExtensions.java
>>
>> line 34-37:
>> * This file contains all the classes relevant to TLS Extensions for the
>> * ClientHello and ServerHello messages. The extension mechanism and
>> * several extensions are defined in RFC 3546. Additional extensions are
>> * defined in the ECC RFC 4492.The ALPN extension is defined in RFC7301.
>>
>> I may put the "Additional extensions ..." at the end. BTW, as you are
>> here already, would you mind update RFC 3546 to RFC 6066? RFC 6066 is
>> the newest revision of RFC 3546.
>
> I concatenated the final sentence.
>
>
>>
>> * This file contains all the classes relevant to TLS Extensions for the
>> * ClientHello and ServerHello messages. The extension mechanism and
>> * several extensions are defined in RFC 6066. The ALPN extension is
>> * defined in RFC 7301. Additional extensions are defined in the ECC
>> * RFC 4492.
>>
>> SSLEngineImpl.java
>> ==
>>
>> line 1411-1412:
>> private Ciphertext writeRecord(ByteBuffer[] appData,
>>int offset, int length, ByteBuffer netData) throws IOException {
>>...
>> if (...) {
>>...
>>// set connection ALPN value
>>applicationProtocol =
>>handshaker.getHandshakeApplicationProtocol();
>>...
>> }
>> }
>>
>> Is it redundant to set applicationProtocol here. I was wondering, the
>> value should set in the processInputRecord() method.
>
> My understanding is the the wrapping is performed here and the unwrapping
> performed in processInputRecord(). Isn’t the assignment required in both
> places?
>
>
>
>>
>> Cheers,
>> Xuelei
>>
>> On 11/30/2015 8:08 AM, Vincent Ryan wrote:
>>> Hello,
>>>
>>> Following on from Brad’s recent email, here is the full webrev of the
>>> API and the implementation classes for ALPN:
>>> http://cr.openjdk.java.net/~vinnie/8144093/webrev.00/
>>>
>>> In adds the implementation classes (sun/security/ssl) to the public API
>>> classes (javax/net/ssl) which have already been agreed.
>>> Some basic tests (test/javax/net/ssl) are also included.
>>>
>>> Please send any code review comments by close-of-business on Tuesday 1
>>> December.
>>> Thanks.
>>
>
Re: 8143397: It looks like InetAddress.isReachable(timeout) works incorrectly
Is it nice to say in the spec that it is not reliable if the timeout is too small? At least 1000ms timeout by default may be not acceptable in some circumstances. Xuelei On 12/9/2015 12:31 AM, Rob McKenna wrote: > Testing has shown that when a timeout < 1000ms is specified the > IcmpSendEcho calls fail (apparently) randomly. Once the timeout is > 1000ms or greater it works as expected. Therefore I've updated the fix > to use 1000ms as a minimum. The existing logic ensures that the ttl is > less than the specified timeout in any case: > > http://cr.openjdk.java.net/~robm/8143397/webrev.02/ > > -Rob > > On 01/12/15 14:59, Rob McKenna wrote: >> It appears that there is an undocumented minimum timeout in the >> IcmpSendEcho* functions. If the timeout parameter is set to a number >> below this minimum timeout it is effectively ignored. Thus if you wanted >> to ensure that you could ping a particular host within a certain timeout >> less than the undocumented minimum, you could potentially receive a >> false positive. (i.e. if you set the timeout to 20ms but the ping takes >> 30ms, IcmpSendEcho will still succeed) >> >> The following fix checks the icmp reply packet and compares the round >> trip time to the requested timeout parameter before deciding whether the >> call was successful or not: >> >> http://cr.openjdk.java.net/~robm/8143397/webrev.01/ >> >> -Rob
Re: 8143397: It looks like InetAddress.isReachable(timeout) works incorrectly
On 12/9/2015 9:44 AM, Rob McKenna wrote: > The intention of the 2nd revision of the fix is to make the undocumented > 1000ms problem a non issue. > > If a user calls this function with a timeout of 200ms that timeout is > automatically substituted for 1000ms in the IcmpSendEcho call. Once the > response is received its RTT is checked to make sure it is lower than > 200ms. If not the method returns false. > OK. It makes sense to me. Thanks, Xuelei > That being the case though the call could potentially take up to 1000ms > to complete where a host is not reachable even though a smaller timeout > is specified. The return value of isReachable() will correctly say > whether the host was reachable or not within the actual timeout > specified by the user however. > > In order to avoid the older (and less reliable) way of testing > reachability I feel that this small corner case is a worthwhile tradeoff. > > -Rob > > On 09/12/15 01:31, Xuelei Fan wrote: >> Is it nice to say in the spec that it is not reliable if the timeout is >> too small? At least 1000ms timeout by default may be not acceptable in >> some circumstances. >> >> Xuelei >> >> On 12/9/2015 12:31 AM, Rob McKenna wrote: >>> Testing has shown that when a timeout < 1000ms is specified the >>> IcmpSendEcho calls fail (apparently) randomly. Once the timeout is >>> 1000ms or greater it works as expected. Therefore I've updated the fix >>> to use 1000ms as a minimum. The existing logic ensures that the ttl is >>> less than the specified timeout in any case: >>> >>> http://cr.openjdk.java.net/~robm/8143397/webrev.02/ >>> >>> -Rob >>> >>> On 01/12/15 14:59, Rob McKenna wrote: >>>> It appears that there is an undocumented minimum timeout in the >>>> IcmpSendEcho* functions. If the timeout parameter is set to a number >>>> below this minimum timeout it is effectively ignored. Thus if you >>>> wanted >>>> to ensure that you could ping a particular host within a certain >>>> timeout >>>> less than the undocumented minimum, you could potentially receive a >>>> false positive. (i.e. if you set the timeout to 20ms but the ping takes >>>> 30ms, IcmpSendEcho will still succeed) >>>> >>>> The following fix checks the icmp reply packet and compares the round >>>> trip time to the requested timeout parameter before deciding whether >>>> the >>>> call was successful or not: >>>> >>>> http://cr.openjdk.java.net/~robm/8143397/webrev.01/ >>>> >>>> -Rob >>
Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException
As you were already there, I would suggest to consider the SSLSocketSample.java template as the comment in JDK-8163924 review thread. Thanks, Xuelei On 9/15/2016 9:13 AM, Artem Smotrakov wrote: Not urgent, but I would appreciate if someone can get a chance to look at this. Artem On 09/07/2016 03:17 PM, Artem Smotrakov wrote: Sending to net-dev@openjdk.java.net as well. Artem On 09/07/2016 12:28 PM, Artem Smotrakov wrote: Hello, Please review the following patch for sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java The test has been observed to fail a couple of times, but it's still not clear why it failed because there is not much info in logs. The patch updates the test to enable additional debug output, so that we have more info if it fails next time. While looking at the test, I notices a couple of issues, but they don't seem to cause these intermittent failures: - The test sets system properties for JSSE in a loop, but JSSE provider reads them only once while initialization. As a result, only values which were set in the first iteration are actually used. - The test doesn't close files and sockets sometimes. The patch also fixed the issues above, and there are a couple cosmetic changes. Bug: https://bugs.openjdk.java.net/browse/JDK-8164591 Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/ Artem
Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException
On 9/15/2016 9:23 AM, Artem Smotrakov wrote: Hi Xuelei, For this one, I am not sure that it would help here since the test failed after it already had started handshaking. It has the same issue as a free-port is used. We don't actually know the handshake is coming from the right client. Xuelei I would prefer to have it as a separate enhancement. Artem On 09/14/2016 06:19 PM, Xuelei Fan wrote: As you were already there, I would suggest to consider the SSLSocketSample.java template as the comment in JDK-8163924 review thread. Thanks, Xuelei On 9/15/2016 9:13 AM, Artem Smotrakov wrote: Not urgent, but I would appreciate if someone can get a chance to look at this. Artem On 09/07/2016 03:17 PM, Artem Smotrakov wrote: Sending to net-dev@openjdk.java.net as well. Artem On 09/07/2016 12:28 PM, Artem Smotrakov wrote: Hello, Please review the following patch for sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java The test has been observed to fail a couple of times, but it's still not clear why it failed because there is not much info in logs. The patch updates the test to enable additional debug output, so that we have more info if it fails next time. While looking at the test, I notices a couple of issues, but they don't seem to cause these intermittent failures: - The test sets system properties for JSSE in a loop, but JSSE provider reads them only once while initialization. As a result, only values which were set in the first iteration are actually used. - The test doesn't close files and sockets sometimes. The patch also fixed the issues above, and there are a couple cosmetic changes. Bug: https://bugs.openjdk.java.net/browse/JDK-8164591 Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/ Artem
Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException
On 9/15/2016 9:45 AM, Artem Smotrakov wrote: Well, in this particular case it's not clear that it has the same issue with free port (at least to me). The exception occurred on client side, so it's not the case where we don't know where the handshake came from. ;-) Yeh, you catch the point. But there is a free-port issue although the exception stack in the bug description may be not that case. Let's look at a scenarios: 1. server open a server socket and listen. 2. other test case connect to the server socket. 3. this test case try to connect to server socket. 4. this test case would fail as the server only accept one connections. I did not check it very carefully, I think for #4, the exception stack can be similar to the one in the bug description. Anyway, as a free port is used, there are free-port issues. Please consider to make the enhancement in the fix. Otherwise, you cannot avoid the intermittent failure for this test case in the current testing environment. Xuelei I can make this enhancement, but like I said I don't think it's going to help, so I would like to keep debug output on. Artem On 09/14/2016 06:39 PM, Xuelei Fan wrote: On 9/15/2016 9:23 AM, Artem Smotrakov wrote: Hi Xuelei, For this one, I am not sure that it would help here since the test failed after it already had started handshaking. It has the same issue as a free-port is used. We don't actually know the handshake is coming from the right client. Xuelei I would prefer to have it as a separate enhancement. Artem On 09/14/2016 06:19 PM, Xuelei Fan wrote: As you were already there, I would suggest to consider the SSLSocketSample.java template as the comment in JDK-8163924 review thread. Thanks, Xuelei On 9/15/2016 9:13 AM, Artem Smotrakov wrote: Not urgent, but I would appreciate if someone can get a chance to look at this. Artem On 09/07/2016 03:17 PM, Artem Smotrakov wrote: Sending to net-dev@openjdk.java.net as well. Artem On 09/07/2016 12:28 PM, Artem Smotrakov wrote: Hello, Please review the following patch for sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java The test has been observed to fail a couple of times, but it's still not clear why it failed because there is not much info in logs. The patch updates the test to enable additional debug output, so that we have more info if it fails next time. While looking at the test, I notices a couple of issues, but they don't seem to cause these intermittent failures: - The test sets system properties for JSSE in a loop, but JSSE provider reads them only once while initialization. As a result, only values which were set in the first iteration are actually used. - The test doesn't close files and sockets sometimes. The patch also fixed the issues above, and there are a couple cosmetic changes. Bug: https://bugs.openjdk.java.net/browse/JDK-8164591 Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/ Artem
Re: RFR 8085575_8130657, misc fixes for a few of java.net intermittent failures
Just FYI. The intermittent failures look like similar to some anti-free-port using issues. In the current testing environment, for the InheritHandle test case, this anti-free-port using issue may looks like: 1. InheritHandle creates a server socket on a free port, and gets the port (PORT-A). (line 60-61) 2. InheritHandle close the server socket. (line 87) 3. Another test (TEST-B) creates a server socket on a free port. PORT-A may be used by the TEST-B. 4. InheritHandle create new server socket on PORT-A (line 88), it is expected to fail as PORT-A has been used by TEST-B. The InheritHandle.java may run into intermittent failure unless it is run single alone. Xuelei On 9/26/2016 3:56 PM, Felix Yang wrote: Hi there, please review following patch to a few of java.net tests. Bug: https://bugs.openjdk.java.net/browse/JDK-8085575 https://bugs.openjdk.java.net/browse/JDK-8130657 Webrev: http://cr.openjdk.java.net/~xiaofeya/8085575_8130657/webrev.00/ Add retry for java/net/Socket/InheritHandle.java. Though it may be unable to resolve all BindException ( I suppose it is the nature of such close-reuse scenarios), it will be helpful to avoid failures from asynchronized close, which has been observed especially on Windows. Also some misc changes on other two tests. Thanks, Felix
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
In theory, there are intermittent compatibility problems as this update may not close the SSL connection over the existing socket layer gracefully, even for high speed networking environments, while the underlying socket is alive. The impact could be serious in some environment. For safe, I may suggest turn this countermeasure off by default. And providing options to turn on this countermeasure: 1. Close the SSL connection gracefully by default; or 2. Close the SSL connection after a timeout. It's hardly to say 5 times receiving timeout is better/safer than timeout once in this context. As you have already had a system property to control, you may be able to use options other than the customized socket receiving timeout, so that the closing timeout is not mixed/confused/dependent on/with the receiving timeout. Put all together: 1. define a closing timeout, for example "jdk.tls.waitForClose". 2. the property default value is zero, no behavior changes. 3. applications can set positive milliseconds value for the property. The SSL connection will be closed in the set milliseconds (or about the maximum value between SO_TIMEOUT and closing timeout), the connection is not grant to be gracefully. What do you think? BTW, please file a CSR as this update is introducing an external system property. Thanks, Xuelei On 9/11/2017 3:29 PM, Rob McKenna wrote: Hi folks, In high latency environments a client SSLSocket with autoClose set to false can hang indefinitely if it does not correctly recieve a close_notify from the server. In order to rectify this situation I would like to suggest that we implement an integer JDK property (jdk.tls.closeRetries) which instructs waitForClose to attempt the close no more times than the value of the property. I would also suggest that 5 is a reasonable default. Note: each attempt times out based on the value of Socket.setSoTimeout(int timeout). Also, the behaviour here is similar to that of waitForClose() when autoClose is set to true, less the retries. http://cr.openjdk.java.net/~robm/8184328/webrev.01/ -Rob
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
On 9/13/2017 5:52 AM, Xuelei Fan wrote: In theory, there are intermittent compatibility problems as this update may not close the SSL connection over the existing socket layer gracefully, even for high speed networking environments, while the underlying socket is alive. The impact could be serious in some environment. For safe, I may suggest turn this countermeasure off by default. And providing options to turn on this countermeasure: 1. Close the SSL connection gracefully by default; or 2. Close the SSL connection after a timeout. It's hardly to say 5 times receiving timeout is better/safer than timeout once in this context. As you have already had a system property to control, you may be able to use options other than the customized socket receiving timeout, so that the closing timeout is not mixed/confused/dependent on/with the receiving timeout. Put all together: 1. define a closing timeout, for example "jdk.tls.waitForClose". 2. the property default value is zero, no behavior changes. 3. applications can set positive milliseconds value for the property. The SSL connection will be closed in the set milliseconds (or about the maximum value between SO_TIMEOUT and closing timeout), the connection is not grant to be gracefully. typo and missed, "not granted to be closed gracefully". What do you think? BTW, please file a CSR as this update is introducing an external system property. Thanks, Xuelei On 9/11/2017 3:29 PM, Rob McKenna wrote: Hi folks, In high latency environments a client SSLSocket with autoClose set to false can hang indefinitely if it does not correctly recieve a close_notify from the server. In order to rectify this situation I would like to suggest that we implement an integer JDK property (jdk.tls.closeRetries) which instructs waitForClose to attempt the close no more times than the value of the property. I would also suggest that 5 is a reasonable default. Note: each attempt times out based on the value of Socket.setSoTimeout(int timeout). Also, the behaviour here is similar to that of waitForClose() when autoClose is set to true, less the retries. http://cr.openjdk.java.net/~robm/8184328/webrev.01/ -Rob
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
On 9/13/2017 8:52 AM, Rob McKenna wrote: Hi Xuelei, This behaviour is already exposed via the autoclose boolean in: https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLSocketFactory.html#createSocket-java.net.Socket-java.io.InputStream-boolean- I did not get the point. What do you mean by this behavior is already exposed? My position would be that allowing 5 retries allows us to say with some confidence that we're not going to get a close_notify from the server. You have more chance to get the close_notify, but it does not mean you can always get the close_notify in 5 retries. When you cannot get it, something bad happens. If this is the case I think its reasonable to close the connection. W.r.t. a separate timeout, the underlying mechanics of a close already depend on the readTimeout in this situation. (waiting on a close_notify requires performing a read so the read timeout makes sense in this context) I'm happy to alter that but I think that the combination of a timeout and a retry count is straightforward and lower impact. In my opinion the default behaviour of potentially hanging indefinitely is worse than the alternative here. (bearing in mind that we are closing the underlying socket) I did not get the point, are we really closing the underlying socket (or the layered ssl connection?) for the context of you update? Xuelei I'll file a CSR as soon as we settle on the direction this fix will take. -Rob On 13/09/17 05:52, Xuelei Fan wrote: In theory, there are intermittent compatibility problems as this update may not close the SSL connection over the existing socket layer gracefully, even for high speed networking environments, while the underlying socket is alive. The impact could be serious in some environment. For safe, I may suggest turn this countermeasure off by default. And providing options to turn on this countermeasure: 1. Close the SSL connection gracefully by default; or 2. Close the SSL connection after a timeout. It's hardly to say 5 times receiving timeout is better/safer than timeout once in this context. As you have already had a system property to control, you may be able to use options other than the customized socket receiving timeout, so that the closing timeout is not mixed/confused/dependent on/with the receiving timeout. Put all together: 1. define a closing timeout, for example "jdk.tls.waitForClose". 2. the property default value is zero, no behavior changes. 3. applications can set positive milliseconds value for the property. The SSL connection will be closed in the set milliseconds (or about the maximum value between SO_TIMEOUT and closing timeout), the connection is not grant to be gracefully. What do you think? BTW, please file a CSR as this update is introducing an external system property. Thanks, Xuelei On 9/11/2017 3:29 PM, Rob McKenna wrote: Hi folks, In high latency environments a client SSLSocket with autoClose set to false can hang indefinitely if it does not correctly recieve a close_notify >from the server. In order to rectify this situation I would like to suggest that we implement an integer JDK property (jdk.tls.closeRetries) which instructs waitForClose to attempt the close no more times than the value of the property. I would also suggest that 5 is a reasonable default. Note: each attempt times out based on the value of Socket.setSoTimeout(int timeout). Also, the behaviour here is similar to that of waitForClose() when autoClose is set to true, less the retries. http://cr.openjdk.java.net/~robm/8184328/webrev.01/ -Rob
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
On 9/13/2017 8:52 AM, Rob McKenna wrote: W.r.t. a separate timeout, the underlying mechanics of a close already depend on the readTimeout in this situation. That's a concerns of mine. In order to work for your countermeasure, applications have to set receiving timeout, and take care of the closing timeout when evaluate what's a right timeout value. The mixing could be misleading and not easy to use. Xuelei
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
It's a little bit complicated for layered SSL connections. Application can build a SSL connection on existing socket (we call it layered SSL connections). The problem scenarios make look like: 1. open a socket for applications. 2. established a SSL connection on the existing socket. 3. close the SSL connection, but leaving data in the socket. 4. establish another SSL connection on the socket, as the existing data in the socket, the connection cannot be established. 5. establish another app connection on the socket, as the existing data in the socket, the connection cannot be established. Timeout happens even on very high speed network. If a timeout happens and the SSL connection is not closed gracefully, and then the following applications breaks. IMHO, we need to take care of the case. Xuelei On 9/13/2017 1:06 PM, Chris Hegarty wrote: Xuelei, Without diving deeper into this issue, Rob’s suggested approach seems reasonable to me, and better than existing out-of-the-box behaviour. I’m not sure what issues you are thinking of, with using the read timeout in combination with a retry mechanism, in this manner? If the network is so slow, surely there will be other issues with connecting and reading, why is closing any different. -Chris. On 13 Sep 2017, at 16:52, Rob McKenna wrote: Hi Xuelei, This behaviour is already exposed via the autoclose boolean in: https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLSocketFactory.html#createSocket-java.net.Socket-java.io.InputStream-boolean- My position would be that allowing 5 retries allows us to say with some confidence that we're not going to get a close_notify from the server. If this is the case I think its reasonable to close the connection. W.r.t. a separate timeout, the underlying mechanics of a close already depend on the readTimeout in this situation. (waiting on a close_notify requires performing a read so the read timeout makes sense in this context) I'm happy to alter that but I think that the combination of a timeout and a retry count is straightforward and lower impact. In my opinion the default behaviour of potentially hanging indefinitely is worse than the alternative here. (bearing in mind that we are closing the underlying socket) I'll file a CSR as soon as we settle on the direction this fix will take. -Rob On 13/09/17 05:52, Xuelei Fan wrote: In theory, there are intermittent compatibility problems as this update may not close the SSL connection over the existing socket layer gracefully, even for high speed networking environments, while the underlying socket is alive. The impact could be serious in some environment. For safe, I may suggest turn this countermeasure off by default. And providing options to turn on this countermeasure: 1. Close the SSL connection gracefully by default; or 2. Close the SSL connection after a timeout. It's hardly to say 5 times receiving timeout is better/safer than timeout once in this context. As you have already had a system property to control, you may be able to use options other than the customized socket receiving timeout, so that the closing timeout is not mixed/confused/dependent on/with the receiving timeout. Put all together: 1. define a closing timeout, for example "jdk.tls.waitForClose". 2. the property default value is zero, no behavior changes. 3. applications can set positive milliseconds value for the property. The SSL connection will be closed in the set milliseconds (or about the maximum value between SO_TIMEOUT and closing timeout), the connection is not grant to be gracefully. What do you think? BTW, please file a CSR as this update is introducing an external system property. Thanks, Xuelei On 9/11/2017 3:29 PM, Rob McKenna wrote: Hi folks, In high latency environments a client SSLSocket with autoClose set to false can hang indefinitely if it does not correctly recieve a close_notify from the server. In order to rectify this situation I would like to suggest that we implement an integer JDK property (jdk.tls.closeRetries) which instructs waitForClose to attempt the close no more times than the value of the property. I would also suggest that 5 is a reasonable default. Note: each attempt times out based on the value of Socket.setSoTimeout(int timeout). Also, the behaviour here is similar to that of waitForClose() when autoClose is set to true, less the retries. http://cr.openjdk.java.net/~robm/8184328/webrev.01/ -Rob
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
On 9/15/2017 7:00 AM, Rob McKenna wrote: When we call close() on the SSLSocket that calls close() on the underlying java Socket which closes the native socket. Sorry, I did not get the point. Please see the close() implementation of SSLSocket (sun.security.ssl.SSLSocketImpl.close()) about the details. Xuelei -Rob On 13/09/17 04:09, Xuelei Fan wrote: It's a little bit complicated for layered SSL connections. Application can build a SSL connection on existing socket (we call it layered SSL connections). The problem scenarios make look like: 1. open a socket for applications. 2. established a SSL connection on the existing socket. 3. close the SSL connection, but leaving data in the socket. 4. establish another SSL connection on the socket, as the existing data in the socket, the connection cannot be established. 5. establish another app connection on the socket, as the existing data in the socket, the connection cannot be established. Timeout happens even on very high speed network. If a timeout happens and the SSL connection is not closed gracefully, and then the following applications breaks. IMHO, we need to take care of the case. Xuelei On 9/13/2017 1:06 PM, Chris Hegarty wrote: Xuelei, Without diving deeper into this issue, Rob’s suggested approach seems reasonable to me, and better than existing out-of-the-box behaviour. I’m not sure what issues you are thinking of, with using the read timeout in combination with a retry mechanism, in this manner? If the network is so slow, surely there will be other issues with connecting and reading, why is closing any different. -Chris. On 13 Sep 2017, at 16:52, Rob McKenna wrote: Hi Xuelei, This behaviour is already exposed via the autoclose boolean in: https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLSocketFactory.html#createSocket-java.net.Socket-java.io.InputStream-boolean- My position would be that allowing 5 retries allows us to say with some confidence that we're not going to get a close_notify from the server. If this is the case I think its reasonable to close the connection. W.r.t. a separate timeout, the underlying mechanics of a close already depend on the readTimeout in this situation. (waiting on a close_notify requires performing a read so the read timeout makes sense in this context) I'm happy to alter that but I think that the combination of a timeout and a retry count is straightforward and lower impact. In my opinion the default behaviour of potentially hanging indefinitely is worse than the alternative here. (bearing in mind that we are closing the underlying socket) I'll file a CSR as soon as we settle on the direction this fix will take. -Rob On 13/09/17 05:52, Xuelei Fan wrote: In theory, there are intermittent compatibility problems as this update may not close the SSL connection over the existing socket layer gracefully, even for high speed networking environments, while the underlying socket is alive. The impact could be serious in some environment. For safe, I may suggest turn this countermeasure off by default. And providing options to turn on this countermeasure: 1. Close the SSL connection gracefully by default; or 2. Close the SSL connection after a timeout. It's hardly to say 5 times receiving timeout is better/safer than timeout once in this context. As you have already had a system property to control, you may be able to use options other than the customized socket receiving timeout, so that the closing timeout is not mixed/confused/dependent on/with the receiving timeout. Put all together: 1. define a closing timeout, for example "jdk.tls.waitForClose". 2. the property default value is zero, no behavior changes. 3. applications can set positive milliseconds value for the property. The SSL connection will be closed in the set milliseconds (or about the maximum value between SO_TIMEOUT and closing timeout), the connection is not grant to be gracefully. What do you think? BTW, please file a CSR as this update is introducing an external system property. Thanks, Xuelei On 9/11/2017 3:29 PM, Rob McKenna wrote: Hi folks, In high latency environments a client SSLSocket with autoClose set to false can hang indefinitely if it does not correctly recieve a close_notify >from the server. In order to rectify this situation I would like to suggest that we implement an integer JDK property (jdk.tls.closeRetries) which instructs waitForClose to attempt the close no more times than the value of the property. I would also suggest that 5 is a reasonable default. Note: each attempt times out based on the value of Socket.setSoTimeout(int timeout). Also, the behaviour here is similar to that of waitForClose() when autoClose is set to true, less the retries. http://cr.openjdk.java.net/~robm/8184328/webrev.01/ -Rob
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
On 9/15/2017 7:07 AM, Rob McKenna wrote: But they are inextricably linked regardless. When we close an SSLSocket it performs a readReply which is subject to the read timeout. So if no read timeout is specified, the call to readReply will hang indefinitely. That's one of what I worried about. Applications have to set receiving timeout in your proposal. I don't want closing timeout binding to receiving timeout. It's doable and the impact is minimal. Xuelei If a read timeout is specified we would need to maintain two separate timeouts and take each into account when polling. What you are suggesting would effectively necessitate a reimplementation of the close mechanics discarding the read timeout completely. (which would be a significant enough change in terms of compatibility) -Rob On 13/09/17 03:56, Xuelei Fan wrote: On 9/13/2017 8:52 AM, Rob McKenna wrote: W.r.t. a separate timeout, the underlying mechanics of a close already depend on the readTimeout in this situation. That's a concerns of mine. In order to work for your countermeasure, applications have to set receiving timeout, and take care of the closing timeout when evaluate what's a right timeout value. The mixing could be misleading and not easy to use. Xuelei
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
On 9/15/2017 7:16 AM, Rob McKenna wrote: On 13/09/17 03:52, Xuelei Fan wrote: On 9/13/2017 8:52 AM, Rob McKenna wrote: Hi Xuelei, This behaviour is already exposed via the autoclose boolean in: https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLSocketFactory.html#createSocket-java.net.Socket-java.io.InputStream-boolean- I did not get the point. What do you mean by this behavior is already exposed? In SSLSocketImpl.closeSocket() waitForClose is only called if autoclose is true. If not the SSLSocket simply calls super.close(). Did you get something different? I think waitForClose is only called if autoclose is false. No matter the autoclose is true or false, I'm not sure what do you mean by this behavior is already exposed. Can you describe more about the point. My position would be that allowing 5 retries allows us to say with some confidence that we're not going to get a close_notify from the server. You have more chance to get the close_notify, but it does not mean you can always get the close_notify in 5 retries. When you cannot get it, something bad happens. No, the property would need to be tuned to suit the networking environment in which the application is deployed. Much the same as a timeout would be. If this is the case I think its reasonable to close the connection. W.r.t. a separate timeout, the underlying mechanics of a close already depend on the readTimeout in this situation. (waiting on a close_notify requires performing a read so the read timeout makes sense in this context) I'm happy to alter that but I think that the combination of a timeout and a retry count is straightforward and lower impact. In my opinion the default behaviour of potentially hanging indefinitely is worse than the alternative here. (bearing in mind that we are closing the underlying socket) I did not get the point, are we really closing the underlying socket (or the layered ssl connection?) for the context of you update? We're calling fatal which calls closeSocket which in turn calls super.close(). (this calls Socket.close() via BaseSSLSocketImpl / SSLSocket) As noted in an earlier reply, this will close the underlying native socket. (I'll perform more testing to verify this) When the fatal get called? I may miss something. Could you describe the scenarios in more details? Xuelei -Rob Xuelei I'll file a CSR as soon as we settle on the direction this fix will take. -Rob On 13/09/17 05:52, Xuelei Fan wrote: In theory, there are intermittent compatibility problems as this update may not close the SSL connection over the existing socket layer gracefully, even for high speed networking environments, while the underlying socket is alive. The impact could be serious in some environment. For safe, I may suggest turn this countermeasure off by default. And providing options to turn on this countermeasure: 1. Close the SSL connection gracefully by default; or 2. Close the SSL connection after a timeout. It's hardly to say 5 times receiving timeout is better/safer than timeout once in this context. As you have already had a system property to control, you may be able to use options other than the customized socket receiving timeout, so that the closing timeout is not mixed/confused/dependent on/with the receiving timeout. Put all together: 1. define a closing timeout, for example "jdk.tls.waitForClose". 2. the property default value is zero, no behavior changes. 3. applications can set positive milliseconds value for the property. The SSL connection will be closed in the set milliseconds (or about the maximum value between SO_TIMEOUT and closing timeout), the connection is not grant to be gracefully. What do you think? BTW, please file a CSR as this update is introducing an external system property. Thanks, Xuelei On 9/11/2017 3:29 PM, Rob McKenna wrote: Hi folks, In high latency environments a client SSLSocket with autoClose set to false can hang indefinitely if it does not correctly recieve a close_notify >from the server. In order to rectify this situation I would like to suggest that we implement an integer JDK property (jdk.tls.closeRetries) which instructs waitForClose to attempt the close no more times than the value of the property. I would also suggest that 5 is a reasonable default. Note: each attempt times out based on the value of Socket.setSoTimeout(int timeout). Also, the behaviour here is similar to that of waitForClose() when autoClose is set to true, less the retries. http://cr.openjdk.java.net/~robm/8184328/webrev.01/ -Rob
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
On 9/15/2017 7:41 AM, Rob McKenna wrote: On 15/09/17 07:07, Xuelei Fan wrote: On 9/15/2017 7:00 AM, Rob McKenna wrote: When we call close() on the SSLSocket that calls close() on the underlying java Socket which closes the native socket. Sorry, I did not get the point. Please see the close() implementation of SSLSocket (sun.security.ssl.SSLSocketImpl.close()) about the details. Running my original test against an instrumented 8u-dev produces the following: java.lang.Exception: Stack trace at java.lang.Thread.dumpStack(Thread.java:1336) at java.net.Socket.close(Socket.java:1491) at sun.security.ssl.BaseSSLSocketImpl.close(BaseSSLSocketImpl.java:624) at sun.security.ssl.SSLSocketImpl.closeSocket(SSLSocketImpl.java:1579) at sun.security.ssl.SSLSocketImpl.fatal(SSLSocketImpl.java:1980) at sun.security.ssl.SSLSocketImpl.waitForClose(SSLSocketImpl.java:1793) at sun.security.ssl.SSLSocketImpl.closeSocket(SSLSocketImpl.java:1592) at sun.security.ssl.SSLSocketImpl.closeInternal(SSLSocketImpl.java:1726) at sun.security.ssl.SSLSocketImpl.close(SSLSocketImpl.java:1615) at ssl.SSLClient.close(SSLClient.java:143) at ssl.SocketTimeoutCloseHang.ReadHang.testSSLServer(ReadHang.java:77) It is just one possible stacks of many. There are cases where no fatal() get called. For example, application call close() method directly. Xuelei
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
On 9/15/2017 7:44 AM, Rob McKenna wrote:
Perhaps I'm misunderstanding you here. Can you illustrate this a bit
further?
The basic point is simple: removing the closing blocking even receiving
timeout is not set.
Applications already have to set a read timeout
I did not get the point. Applications don't have to set a read timeout.
Xuelei
, my proposal doesn't
alter this fact. (i.e. if the read timeout isn't set applications which
call close could potentially get stuck in readReply indefinitely)
-Rob
On 15/09/17 07:23, Xuelei Fan wrote:
On 9/15/2017 7:07 AM, Rob McKenna wrote:
But they are inextricably linked regardless.
When we close an SSLSocket it performs a readReply which is subject to
the read timeout. So if no read timeout is specified, the call to
readReply will hang indefinitely.
That's one of what I worried about. Applications have to set receiving
timeout in your proposal. I don't want closing timeout binding to receiving
timeout. It's doable and the impact is minimal.
Xuelei
If a read timeout is specified we
would need to maintain two separate timeouts and take each into account
when polling.
What you are suggesting would effectively necessitate a reimplementation
of the close mechanics discarding the read timeout completely. (which
would be a significant enough change in terms of compatibility)
-Rob
On 13/09/17 03:56, Xuelei Fan wrote:
On 9/13/2017 8:52 AM, Rob McKenna wrote:
W.r.t. a separate timeout, the underlying mechanics of a close already
depend on the readTimeout in this situation.
That's a concerns of mine. In order to work for your countermeasure,
applications have to set receiving timeout, and take care of the closing
timeout when evaluate what's a right timeout value. The mixing could be
misleading and not easy to use.
Xuelei
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
On 9/15/2017 8:22 AM, Rob McKenna wrote: This test calls close directly. (3rd last line in the stack) I believe this is the only possible stack (with the new parameter) once autoclose is set to false. If autoclose is true we'd skip the call to waitForClose and just go directly to Socket.close() unless I'm mistaken. I did not find the call to fatal() in the current implementation. I think you mean you added the call to fatal() in your update so that when timeout, a fatal() will always get called? Thinking about two things: 1. application have to set receiving timeout in order to get receiving timeout. I have a concern about it, as described in other comments. 2. can we close the super socket? It is a surprise to me to close super socket even we don't allocate it. It does not feel right to me, but this is the current behavior. All right, I get your point. Xuelei -Rob On 15/09/17 07:55, Xuelei Fan wrote: On 9/15/2017 7:41 AM, Rob McKenna wrote: On 15/09/17 07:07, Xuelei Fan wrote: On 9/15/2017 7:00 AM, Rob McKenna wrote: When we call close() on the SSLSocket that calls close() on the underlying java Socket which closes the native socket. Sorry, I did not get the point. Please see the close() implementation of SSLSocket (sun.security.ssl.SSLSocketImpl.close()) about the details. Running my original test against an instrumented 8u-dev produces the following: java.lang.Exception: Stack trace at java.lang.Thread.dumpStack(Thread.java:1336) at java.net.Socket.close(Socket.java:1491) at sun.security.ssl.BaseSSLSocketImpl.close(BaseSSLSocketImpl.java:624) at sun.security.ssl.SSLSocketImpl.closeSocket(SSLSocketImpl.java:1579) at sun.security.ssl.SSLSocketImpl.fatal(SSLSocketImpl.java:1980) at sun.security.ssl.SSLSocketImpl.waitForClose(SSLSocketImpl.java:1793) at sun.security.ssl.SSLSocketImpl.closeSocket(SSLSocketImpl.java:1592) at sun.security.ssl.SSLSocketImpl.closeInternal(SSLSocketImpl.java:1726) at sun.security.ssl.SSLSocketImpl.close(SSLSocketImpl.java:1615) at ssl.SSLClient.close(SSLClient.java:143) at ssl.SocketTimeoutCloseHang.ReadHang.testSSLServer(ReadHang.java:77) It is just one possible stacks of many. There are cases where no fatal() get called. For example, application call close() method directly. Xuelei
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
I still prefer to not-depends on socket receiving timeout. But I'm fine if you want to move on with it. As we can close the super socket in the current implementation, it implies that application can handle it already. So you may not need the system property and 5 times retries. I think it's fine just call fatal() for the first timeout. Xuelei On 9/15/2017 4:32 PM, Xuelei Fan wrote: On 9/15/2017 8:22 AM, Rob McKenna wrote: This test calls close directly. (3rd last line in the stack) I believe this is the only possible stack (with the new parameter) once autoclose is set to false. If autoclose is true we'd skip the call to waitForClose and just go directly to Socket.close() unless I'm mistaken. I did not find the call to fatal() in the current implementation. I think you mean you added the call to fatal() in your update so that when timeout, a fatal() will always get called? Thinking about two things: 1. application have to set receiving timeout in order to get receiving timeout. I have a concern about it, as described in other comments. 2. can we close the super socket? It is a surprise to me to close super socket even we don't allocate it. It does not feel right to me, but this is the current behavior. All right, I get your point. Xuelei -Rob On 15/09/17 07:55, Xuelei Fan wrote: On 9/15/2017 7:41 AM, Rob McKenna wrote: On 15/09/17 07:07, Xuelei Fan wrote: On 9/15/2017 7:00 AM, Rob McKenna wrote: When we call close() on the SSLSocket that calls close() on the underlying java Socket which closes the native socket. Sorry, I did not get the point. Please see the close() implementation of SSLSocket (sun.security.ssl.SSLSocketImpl.close()) about the details. Running my original test against an instrumented 8u-dev produces the following: java.lang.Exception: Stack trace at java.lang.Thread.dumpStack(Thread.java:1336) at java.net.Socket.close(Socket.java:1491) at sun.security.ssl.BaseSSLSocketImpl.close(BaseSSLSocketImpl.java:624) at sun.security.ssl.SSLSocketImpl.closeSocket(SSLSocketImpl.java:1579) at sun.security.ssl.SSLSocketImpl.fatal(SSLSocketImpl.java:1980) at sun.security.ssl.SSLSocketImpl.waitForClose(SSLSocketImpl.java:1793) at sun.security.ssl.SSLSocketImpl.closeSocket(SSLSocketImpl.java:1592) at sun.security.ssl.SSLSocketImpl.closeInternal(SSLSocketImpl.java:1726) at sun.security.ssl.SSLSocketImpl.close(SSLSocketImpl.java:1615) at ssl.SSLClient.close(SSLClient.java:143) at ssl.SocketTimeoutCloseHang.ReadHang.testSSLServer(ReadHang.java:77) It is just one possible stacks of many. There are cases where no fatal() get called. For example, application call close() method directly. Xuelei
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
> On Sep 22, 2017, at 6:06 AM, Rob McKenna wrote: > > Thanks Xuelei, webrev below: > > http://cr.openjdk.java.net/~robm/8184328/webrev.02/ > Looks fine to me. > Presumably this won't require a CSR? > Agreed. Xuelei > -Rob > >> On 15/09/17 04:38, Xuelei Fan wrote: >> I still prefer to not-depends on socket receiving timeout. But I'm fine if >> you want to move on with it. >> >> As we can close the super socket in the current implementation, it implies >> that application can handle it already. So you may not need the system >> property and 5 times retries. I think it's fine just call fatal() for the >> first timeout. >> >> Xuelei >> >>> On 9/15/2017 4:32 PM, Xuelei Fan wrote: >>>> On 9/15/2017 8:22 AM, Rob McKenna wrote: >>>> This test calls close directly. (3rd last line in the stack) >>>> >>>> I believe this is the only possible stack (with the new parameter) once >>>> autoclose is set to false. If autoclose is true we'd skip the call to >>>> waitForClose and just go directly to Socket.close() unless I'm mistaken. >>>> >>> I did not find the call to fatal() in the current implementation. I think >>> you mean you added the call to fatal() in your update so that when >>> timeout, a fatal() will always get called? >>> >>> Thinking about two things: >>> 1. application have to set receiving timeout in order to get receiving >>> timeout. >>> I have a concern about it, as described in other comments. >>> >>> 2. can we close the super socket? >>> It is a surprise to me to close super socket even we don't allocate it. It >>> does not feel right to me, but this is the current behavior. All right, I >>> get your point. >>> >>> Xuelei >>> >>>> -Rob >>>> >>>>> On 15/09/17 07:55, Xuelei Fan wrote: >>>>>> On 9/15/2017 7:41 AM, Rob McKenna wrote: >>>>>>> On 15/09/17 07:07, Xuelei Fan wrote: >>>>>>>> On 9/15/2017 7:00 AM, Rob McKenna wrote: >>>>>>>> When we call close() on the SSLSocket that calls close() on the >>>>>>>> underlying java Socket which closes the native socket. >>>>>>>> >>>>>>> Sorry, I did not get the point. Please see the close() >>>>>>> implementation of >>>>>>> SSLSocket (sun.security.ssl.SSLSocketImpl.close()) about the details. >>>>>> >>>>>> Running my original test against an instrumented 8u-dev produces the >>>>>> following: >>>>>> >>>>>> java.lang.Exception: Stack trace >>>>>> at java.lang.Thread.dumpStack(Thread.java:1336) >>>>>> at java.net.Socket.close(Socket.java:1491) >>>>>> at >>>>>> sun.security.ssl.BaseSSLSocketImpl.close(BaseSSLSocketImpl.java:624) >>>>>> at >>>>>> sun.security.ssl.SSLSocketImpl.closeSocket(SSLSocketImpl.java:1579) >>>>>> at sun.security.ssl.SSLSocketImpl.fatal(SSLSocketImpl.java:1980) >>>>>> at >>>>>> sun.security.ssl.SSLSocketImpl.waitForClose(SSLSocketImpl.java:1793) >>>>>> at >>>>>> sun.security.ssl.SSLSocketImpl.closeSocket(SSLSocketImpl.java:1592) >>>>>> at >>>>>> sun.security.ssl.SSLSocketImpl.closeInternal(SSLSocketImpl.java:1726) >>>>>> at sun.security.ssl.SSLSocketImpl.close(SSLSocketImpl.java:1615) >>>>>> at ssl.SSLClient.close(SSLClient.java:143) >>>>>> at >>>>>> ssl.SocketTimeoutCloseHang.ReadHang.testSSLServer(ReadHang.java:77) >>>>>> >>>>> It is just one possible stacks of many. There are cases where no >>>>> fatal() >>>>> get called. For example, application call close() method directly. >>>>> >>>>> Xuelei
Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received
Please let me know your concerns by the end of August 1st, 2018. Thanks, Xuelei On 7/30/2018 9:59 AM, Xuelei Fan wrote: Hi, Please review the update for the TLS 1.3 half-close and synchronization implementation: http://cr.openjdk.java.net/~xuelei/8207009/webrev.00/ Unlike TLS 1.2 and prior versions, for TLS 1.3, the close_notify is use to close the local write side and peer read side only. After the close_notify get handles, the local read side and peer write side may still be open. In this update, if an application calls SSLEngine.closeInbound/Outbound() or SSLSocket.shutdownInput/Output(), half-close will be used. For compatibility, if SSLSocket.close() get called, a duplex close will be tried. In order to support duplex close, JDK will use the user_canceled warning alert even the handshake complete. In practice, an application may only close outbound even it is intended to close the inbound as well, or close the connection completely. It works for TLS 1.2 and prior versions. But no more for TLS 1.3 because of the close_notify behavior change in the TLS 1.3 specification. The application may be hung and dead-waiting for read/close. It could be solved by closing the inbound explicitly. In order to mitigate the impact, a new System Property is introduced, "jdk.tls.acknowledgeCloseNotify" if source code update is not available. If the System Property is set to "true", if receiving the close_notify, a close_notify alert will be responded. It is a countermeasure of the TLS 1.3 half-close issues. Thanks, Xuelei
Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received
Update: http://cr.openjdk.java.net/~xuelei/8207009/webrev.01/ Integrated the fix for JDK-8208642, "Server initiated TLSv1.2 renegotiation fails if Java client allows TLSv1.3". SSLHandshake.java is updated to use negotiated version so that TLS 1.2 HelloRequest is acceptable in TLS 1.3 client side. Thanks, Xuelei On 7/30/2018 10:24 AM, Xuelei Fan wrote: Please let me know your concerns by the end of August 1st, 2018. Thanks, Xuelei On 7/30/2018 9:59 AM, Xuelei Fan wrote: Hi, Please review the update for the TLS 1.3 half-close and synchronization implementation: http://cr.openjdk.java.net/~xuelei/8207009/webrev.00/ Unlike TLS 1.2 and prior versions, for TLS 1.3, the close_notify is use to close the local write side and peer read side only. After the close_notify get handles, the local read side and peer write side may still be open. In this update, if an application calls SSLEngine.closeInbound/Outbound() or SSLSocket.shutdownInput/Output(), half-close will be used. For compatibility, if SSLSocket.close() get called, a duplex close will be tried. In order to support duplex close, JDK will use the user_canceled warning alert even the handshake complete. In practice, an application may only close outbound even it is intended to close the inbound as well, or close the connection completely. It works for TLS 1.2 and prior versions. But no more for TLS 1.3 because of the close_notify behavior change in the TLS 1.3 specification. The application may be hung and dead-waiting for read/close. It could be solved by closing the inbound explicitly. In order to mitigate the impact, a new System Property is introduced, "jdk.tls.acknowledgeCloseNotify" if source code update is not available. If the System Property is set to "true", if receiving the close_notify, a close_notify alert will be responded. It is a countermeasure of the TLS 1.3 half-close issues. Thanks, Xuelei
Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received
Update: http://cr.openjdk.java.net/~xuelei/8207009/webrev.02/ In webrev.01, the socket close may be blocked by super class close synchronization. Updated the SSLSocketImpl.java to use handshake only lock in the startHandshake() implementation. Thanks, Xuelei On 8/1/2018 7:27 PM, Xuelei Fan wrote: Update: http://cr.openjdk.java.net/~xuelei/8207009/webrev.01/ Integrated the fix for JDK-8208642, "Server initiated TLSv1.2 renegotiation fails if Java client allows TLSv1.3". SSLHandshake.java is updated to use negotiated version so that TLS 1.2 HelloRequest is acceptable in TLS 1.3 client side. Thanks, Xuelei On 7/30/2018 10:24 AM, Xuelei Fan wrote: Please let me know your concerns by the end of August 1st, 2018. Thanks, Xuelei On 7/30/2018 9:59 AM, Xuelei Fan wrote: Hi, Please review the update for the TLS 1.3 half-close and synchronization implementation: http://cr.openjdk.java.net/~xuelei/8207009/webrev.00/ Unlike TLS 1.2 and prior versions, for TLS 1.3, the close_notify is use to close the local write side and peer read side only. After the close_notify get handles, the local read side and peer write side may still be open. In this update, if an application calls SSLEngine.closeInbound/Outbound() or SSLSocket.shutdownInput/Output(), half-close will be used. For compatibility, if SSLSocket.close() get called, a duplex close will be tried. In order to support duplex close, JDK will use the user_canceled warning alert even the handshake complete. In practice, an application may only close outbound even it is intended to close the inbound as well, or close the connection completely. It works for TLS 1.2 and prior versions. But no more for TLS 1.3 because of the close_notify behavior change in the TLS 1.3 specification. The application may be hung and dead-waiting for read/close. It could be solved by closing the inbound explicitly. In order to mitigate the impact, a new System Property is introduced, "jdk.tls.acknowledgeCloseNotify" if source code update is not available. If the System Property is set to "true", if receiving the close_notify, a close_notify alert will be responded. It is a countermeasure of the TLS 1.3 half-close issues. Thanks, Xuelei
Re: Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received
New webrev: http://cr.openjdk.java.net/~xuelei/8207009/webrev.03/ Thanks for a find of Tim Brooks, that the SSLEngine inbound/outbound status is incorrect if closing during handshake. The above webrev is trying to fix the problems. See more in the OpenJDK thread: http://mail.openjdk.java.net/pipermail/security-dev/2018-August/017778.html Please let me know your concerns before this Wednesday. Thanks, Xuelei On 8/3/2018 1:55 PM, Xuelei Fan wrote: Update: http://cr.openjdk.java.net/~xuelei/8207009/webrev.02/ In webrev.01, the socket close may be blocked by super class close synchronization. Updated the SSLSocketImpl.java to use handshake only lock in the startHandshake() implementation. Thanks, Xuelei On 8/1/2018 7:27 PM, Xuelei Fan wrote: Update: http://cr.openjdk.java.net/~xuelei/8207009/webrev.01/ Integrated the fix for JDK-8208642, "Server initiated TLSv1.2 renegotiation fails if Java client allows TLSv1.3". SSLHandshake.java is updated to use negotiated version so that TLS 1.2 HelloRequest is acceptable in TLS 1.3 client side. Thanks, Xuelei On 7/30/2018 10:24 AM, Xuelei Fan wrote: Please let me know your concerns by the end of August 1st, 2018. Thanks, Xuelei On 7/30/2018 9:59 AM, Xuelei Fan wrote: Hi, Please review the update for the TLS 1.3 half-close and synchronization implementation: http://cr.openjdk.java.net/~xuelei/8207009/webrev.00/ Unlike TLS 1.2 and prior versions, for TLS 1.3, the close_notify is use to close the local write side and peer read side only. After the close_notify get handles, the local read side and peer write side may still be open. In this update, if an application calls SSLEngine.closeInbound/Outbound() or SSLSocket.shutdownInput/Output(), half-close will be used. For compatibility, if SSLSocket.close() get called, a duplex close will be tried. In order to support duplex close, JDK will use the user_canceled warning alert even the handshake complete. In practice, an application may only close outbound even it is intended to close the inbound as well, or close the connection completely. It works for TLS 1.2 and prior versions. But no more for TLS 1.3 because of the close_notify behavior change in the TLS 1.3 specification. The application may be hung and dead-waiting for read/close. It could be solved by closing the inbound explicitly. In order to mitigate the impact, a new System Property is introduced, "jdk.tls.acknowledgeCloseNotify" if source code update is not available. If the System Property is set to "true", if receiving the close_notify, a close_notify alert will be responded. It is a countermeasure of the TLS 1.3 half-close issues. Thanks, Xuelei
A new proposal to add methods to HttpsURLConnection to access SSLSession
Hi, For the current HttpsURLConnection, there is not much security parameters exposed in the public APIs. An application may need richer information for the underlying TLS connections, for example the negotiated TLS protocol version. Please let me know if you have concerns to add a new method HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, by the end of Nov. 2, 2018. Here is the proposal: https://bugs.openjdk.java.net/browse/JDK-8213161 Thanks, Xuelei
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
On 10/31/2018 8:52 AM, Chris Hegarty wrote: Xuelei, On 30/10/18 20:55, Xuelei Fan wrote: Hi, For the current HttpsURLConnection, there is not much security parameters exposed in the public APIs. An application may need richer information for the underlying TLS connections, for example the negotiated TLS protocol version. Please let me know if you have concerns to add a new method HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, by the end of Nov. 2, 2018. Here is the proposal: https://bugs.openjdk.java.net/browse/JDK-8213161 Thanks, Xuelei The new method looks fine. On the deprecation, minimally the annotation should contain the "since" element, which will have a value of `12`. Hm, it looks better now if using the "since" element. Thanks! Also, I wonder, now that I see the spec, whether or not it is actually a good idea to deprecate these methods. The reason I ask this is that the new method, getSSLSession, can throw UOE, which effectively makes it an optional method. Access to the specific security parameters provided by the existing methods is non-optional. This is a clear difference, and possibly a reason, for folk not interested in the "new" parameters, to continue to use the existing methods. Yes. I had the same concern about the optional operation. However, I came to a different conclusion if I'm from viewpoint of these users that need to use this new operation. If an application have to use this new operation, for example to access the negotiated TLS version, this operation is essential to it. Unfortunately, because of compatibility impact, we cannot force all implementation supports this new added operation. It is a potential problem to those applications that need it. It would be nice if an implementation can support this operation as soon as possible. If we just add the operation, providers may not aware there is a need to update their implementation. Unfortunately. there is not much we can do to notify the provider. If we deprecated the duplicated methods, it is easier for providers to catch up with this new added operation, and move forward to support it. As the deprecating will show up building warnings, or even error if run in strict building mode. To make it more clear, I added a implNote in the getSSLSession() method, and recommend to support it in all implementations. I prefer to deprecate these methods a little bit more, but not very strong. If there is a strong preference, I can live with it. BTW, I also updated the java.net.SecureCacheResponse accordingly. I'm not sure if SecureCacheResponse is really used in practice. I did not find the implementation of it in JDK. Here is the webrev if you want to look at the implementation details: http://cr.openjdk.java.net/~xuelei/8212261/webrev.00/ Thanks, Xuelei
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
I removed the deprecation parts in the update. Here is the new webrev: http://cr.openjdk.java.net/~xuelei/8212261/webrev.01/ And the CSR was updated accordingly. https://bugs.openjdk.java.net/browse/JDK-8213161 Thanks, Xuelei On 11/1/2018 4:57 AM, Chris Hegarty wrote: On 31 Oct 2018, at 20:03, Xuelei Fan <mailto:xuelei@oracle.com>> wrote: ... Yes. I had the same concern about the optional operation. However, I came to a different conclusion if I'm from viewpoint of these users that need to use this new operation. If an application have to use this new operation, for example to access the negotiated TLS version, this operation is essential to it. Unfortunately, because of compatibility impact, we cannot force all implementation supports this new added operation. It is a potential problem to those applications that need it. It would be nice if an implementation can support this operation as soon as possible. If we just add the operation, providers may not aware there is a need to update their implementation. Unfortunately. there is not much we can do to notify the provider. If we deprecated the duplicated methods, it is easier for providers to catch up with this new added operation, and move forward to support it. As the deprecating will show up building warnings, or even error if run in strict building mode. I have no issue with the addition of the new method to access the SSLSession. Unfortunately, due to the evolutionary constraints of this API, the `getSSLSession` method will likely need to be specified to throw UOE. The actual JDK's HTTPS protocol handler implementation will not throw UOE. Clearly there is a desire for non-JDK HTTPS protocol handler implementations to quickly determine the need to update their code and override the default implementation of `getSSLSession` ( to do something useful ), hence the request to deprecate the the existing individual security parameter accessor methods. I don't believe that deprecating these individual security parameter accessors is a good idea for the following reasons: 1) Deprecated warnings are only issued at compile-time, so only HTTPS protocol handler implementations being recompiled may encounter the warnings. Existing binary artifacts will not. 2) More importantly. Forever more, developers wanting access to say, the peer principle or the server's certificate chain, will be encouraged to get them through an optional API ( rather than a non-optional API ). This increases the risk that the code will encounter an UOE. I don't think that this increased risk is justified by the desire to not-have-two-ways-to-do-the-same-thing. I would agree if this were a new API, but it is not. HTTPS protocol handler implementations actively being maintained will likely quickly get requests from their users to provide a better implementation of this new method, as folk move towards TLS 1.3, etc. Maybe such requests will be sufficient to help adoption, at which point the deprecation of these methods could then be re-considered. -Chris.
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
On 11/1/2018 11:24 AM, Sean Mullan wrote: On 10/31/18 11:52 AM, Chris Hegarty wrote: Xuelei, On 30/10/18 20:55, Xuelei Fan wrote: Hi, For the current HttpsURLConnection, there is not much security parameters exposed in the public APIs. An application may need richer information for the underlying TLS connections, for example the negotiated TLS protocol version. Please let me know if you have concerns to add a new method HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, by the end of Nov. 2, 2018. Here is the proposal: https://bugs.openjdk.java.net/browse/JDK-8213161 Are there any security issues associated with returning the SSLSession, since it is mutable? It should be fine. The update APIs of the session (invalidating, bind values) does not impact the connection. + * SHOULD override this method with appropriate implementation. s/appropriate/an appropriate/ I would probably not capitalize "SHOULD" and just say "should". "SHOULD" is more common in RFCs. I don't see that much in javadocs. + * @implNote The JDK Reference Implementation supports this operation. + * As an application may have to use this operation for more + * security parameters, it is recommended to support this + * operation in all implementations. I think it should be obvious that the JDK implementation would override this method so not sure that first sentence is necessary. The other sentence seems like it could be combined with the previous sentence, ex: "Subclasses should override this method with an appropriate implementation since an application may need to access additional parameters associated with the SSL session." Updated accordingly, in the CSR and webrev: https://bugs.openjdk.java.net/browse/JDK-8213161 http://cr.openjdk.java.net/~xuelei/8212261/webrev.02/ Thanks, Xuelei
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
Thanks for the review and suggestions, Chris and Sean. I just finalized the CSR. Thanks, Xuelei On 11/2/2018 5:58 AM, Chris Hegarty wrote: Thanks for the updates Xuelei, some minor comments inline.. On 1 Nov 2018, at 23:42, Xuelei Fan <mailto:xuelei@oracle.com>> wrote: On 11/1/2018 11:24 AM, Sean Mullan wrote: On 10/31/18 11:52 AM, Chris Hegarty wrote: Xuelei, On 30/10/18 20:55, Xuelei Fan wrote: Hi, For the current HttpsURLConnection, there is not much security parameters exposed in the public APIs. An application may need richer information for the underlying TLS connections, for example the negotiated TLS protocol version. Please let me know if you have concerns to add a new method HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, by the end of Nov. 2, 2018. Here is the proposal: https://bugs.openjdk.java.net/browse/JDK-8213161 Are there any security issues associated with returning the SSLSession, since it is mutable? It should be fine. The update APIs of the session (invalidating, bind values) does not impact the connection. Alternatively, as is done in the new HTTP Client, an immutable SSLSession instance can be returned. + * SHOULD override this method with appropriate implementation. s/appropriate/an appropriate/ I would probably not capitalize "SHOULD" and just say "should". "SHOULD" is more common in RFCs. I don't see that much in javadocs. + * @implNote The JDK Reference Implementation supports this operation. + * As an application may have to use this operation for more + * security parameters, it is recommended to support this + * operation in all implementations. I think it should be obvious that the JDK implementation would override this method so not sure that first sentence is necessary. The other sentence seems like it could be combined with the previous sentence, ex: "Subclasses should override this method with an appropriate implementation since an application may need to access additional parameters associated with the SSL session." Updated accordingly, in the CSR and webrev: https://bugs.openjdk.java.net/browse/JDK-8213161 The CSR looks good. I made a few minor edits to the verbiage and added myself as reviewer. The title will need to be updated to reflect the addition of the new method in SecureCacheResponse. Maybe: "Add SSLSession accessors to HttpsURLConnection and SecureCacheResponse" -Chris.
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
Hi Chris and Sean, There are a few feedback for the CSR approval. I updated to use Optional for the returned value. For more details, please refer to: https://bugs.openjdk.java.net/browse/JDK-8213161 http://cr.openjdk.java.net/~xuelei/8212261/webrev.03/ Please let me know if you are OK with this change. Thanks, Xuelei On 11/2/2018 11:42 AM, Xuelei Fan wrote: Thanks for the review and suggestions, Chris and Sean. I just finalized the CSR. Thanks, Xuelei On 11/2/2018 5:58 AM, Chris Hegarty wrote: Thanks for the updates Xuelei, some minor comments inline.. On 1 Nov 2018, at 23:42, Xuelei Fan <mailto:xuelei@oracle.com>> wrote: On 11/1/2018 11:24 AM, Sean Mullan wrote: On 10/31/18 11:52 AM, Chris Hegarty wrote: Xuelei, On 30/10/18 20:55, Xuelei Fan wrote: Hi, For the current HttpsURLConnection, there is not much security parameters exposed in the public APIs. An application may need richer information for the underlying TLS connections, for example the negotiated TLS protocol version. Please let me know if you have concerns to add a new method HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, by the end of Nov. 2, 2018. Here is the proposal: https://bugs.openjdk.java.net/browse/JDK-8213161 Are there any security issues associated with returning the SSLSession, since it is mutable? It should be fine. The update APIs of the session (invalidating, bind values) does not impact the connection. Alternatively, as is done in the new HTTP Client, an immutable SSLSession instance can be returned. + * SHOULD override this method with appropriate implementation. s/appropriate/an appropriate/ I would probably not capitalize "SHOULD" and just say "should". "SHOULD" is more common in RFCs. I don't see that much in javadocs. + * @implNote The JDK Reference Implementation supports this operation. + * As an application may have to use this operation for more + * security parameters, it is recommended to support this + * operation in all implementations. I think it should be obvious that the JDK implementation would override this method so not sure that first sentence is necessary. The other sentence seems like it could be combined with the previous sentence, ex: "Subclasses should override this method with an appropriate implementation since an application may need to access additional parameters associated with the SSL session." Updated accordingly, in the CSR and webrev: https://bugs.openjdk.java.net/browse/JDK-8213161 The CSR looks good. I made a few minor edits to the verbiage and added myself as reviewer. The title will need to be updated to reflect the addition of the new method in SecureCacheResponse. Maybe: "Add SSLSession accessors to HttpsURLConnection and SecureCacheResponse" -Chris.
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
On 11/7/2018 1:30 PM, Sean Mullan wrote: https://bugs.openjdk.java.net/browse/JDK-8213161 http://cr.openjdk.java.net/~xuelei/8212261/webrev.03/ I didn't see a test for SecureCacheResponse - is it possible? JDK does not have the reference implementation of SecureCacheResponse. You could also add a test case for IllegalStateExc. Added in the same test file. lines 108-113 of HttpsSession.java should be indented 4 more spaces. Otherwise looks ok to me. Updated. New webrev: http://cr.openjdk.java.net/~xuelei/8212261/webrev.04/ Thanks, Xuelei
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
To make sure the SecureCacheResponse class work, two new tests are added (DefaultCacheResponse for default implementation, DummyCacheResponse for a test implementation): http://cr.openjdk.java.net/~xuelei/8212261/webrev.04/ Thanks, Xuelei On 11/8/2018 7:03 AM, Sean Mullan wrote: On 11/7/18 7:22 PM, Xuelei Fan wrote: On 11/7/2018 1:30 PM, Sean Mullan wrote: https://bugs.openjdk.java.net/browse/JDK-8213161 http://cr.openjdk.java.net/~xuelei/8212261/webrev.03/ I didn't see a test for SecureCacheResponse - is it possible? JDK does not have the reference implementation of SecureCacheResponse. Ah, I see. I am sure there is a good reason, but why doesn't it have an implementation? You could also add a test case for IllegalStateExc. Added in the same test file. lines 108-113 of HttpsSession.java should be indented 4 more spaces. Otherwise looks ok to me. Updated. New webrev: http://cr.openjdk.java.net/~xuelei/8212261/webrev.04/ Looks good. --Sean
Re: RFR [13] JDK-8215430: Remove the internal package com.sun.net.ssl
It makes sense to me to leave the AbstractDelegateHttpsURLConnection methods as public. We may need to re-org the code later, but it is out of the scope of this update. Here is the new webrev (only AbstractDelegateHttpsURLConnection updated): http://cr.openjdk.java.net/~xuelei/8215430/webrev.01/ Thanks, Xuelei On 2/25/2019 1:55 PM, Sean Mullan wrote: (I'd suggest cross-posting to net-dev since some classes in the networking area are also changed). - AbstractDelegateHttpsURLConnection It might be less risky to leave the methods as public just in case some code out there is using them (even though they are not supposed to). The rest of this looks ok, pretty straightforward. --Sean On 2/21/19 10:45 PM, Xuelei Fan wrote: Hi, Could I get the following update reviewed: http://cr.openjdk.java.net/~xuelei/8215430/webrev.00/ The internal package com.sun.net.ssl had been deprecated since JDK 1.4, and was not exported in the java.base module since JDK 9. It should be safe to remove them in JDK 13. For more details, please refer to CSR: https://bugs.openjdk.java.net/browse/JDK-8218932 Thanks, Xuelei
Re: RFR [13] JDK-8215430: Remove the internal package com.sun.net.ssl
On 2/28/2019 4:26 PM, Bradford Wetmore wrote: AbstractDelegateHttpsURLConnection.java --- 74/88/etc. I haven't checked for sure, but IIRC we made setNewClient public from protected and other such changes in order for these classes to use the delegation model and allow calls to both sun/net/www/protocol and com/sun/net/ssl/internal/www/protocol... Should the delegation code be stripped out, and these methods be set back to protected? It should be safe to use "protected", but it's less risky to keep it unchanged. I agree that the delegation code should be stripped out. The HttpsURLConnection implementation could be re-designed. I filed a new bug to track the enhancement (JDK-8219987). ProviderConfig.java --- I think we're over-rotating on this one, I'd be very surprised if anyone is still using com.sun.net.ssl.internal.ssl.Provider. My personal preference is to kill it now, but if you really want to keep it, please file a bug and commit to a near release so we can finally kill this thing off. I would like do it later as the old java.security use this string, and I'm not sure if someone really use it in their application. It is safer to fix in a separate bug. I filled a new bug to tack the update (JDK-8219989). test/jdk/com/sun/net/ssl/SSLSecurity/ComTrustManagerFactoryImpl.java test/jdk/com/sun/net/ssl/SSLSecurity/JavaxTrustManagerFactoryImpl.java When we opensourced the JSSE code, we left some of the javax and com tests in the com/sun/net/ssl test directory. The test names are very similar, with the exception of the Com/Javax prefix. The tests were identical, with the exception of whether they called into the com or javax APIs. Here you are deleting the ComTrustManagerFactoryImpl test which is probably correct, but should you also be deleting the JavaxTrustManagerFactoryImpl test as well? Please check that we didn't get too carried away and are deleting still valid tests. They are helper classes and used by ProviderTest, which is used to test the com.sun.net.ssl wrappers. Thanks, Xuelei Thanks, Brad On 2/26/2019 7:33 AM, Chris Hegarty wrote: On 26 Feb 2019, at 03:53, Xuelei Fan wrote: It makes sense to me to leave the AbstractDelegateHttpsURLConnection methods as public. We may need to re-org the code later, but it is out of the scope of this update. Here is the new webrev (only AbstractDelegateHttpsURLConnection updated): http://cr.openjdk.java.net/~xuelei/8215430/webrev.01/ I think that this is probably ok ( since the package is not exported ), but it seems a little distasteful to not have the API move through the terminal deprecation route before being removed. -Chris.
Re: (teststabilization) RFR 8230858: Replace wildcard address with loopback or local host in tests - part 23
+1. Xuelei On 9/12/2019 4:43 AM, Chris Hegarty wrote: On 11 Sep 2019, at 16:24, Daniel Fuchs wrote: Hi, Please find below a patch for: 8230858: Replace wildcard address with loopback or local host in tests - part 23 https://bugs.openjdk.java.net/browse/JDK-8230858 webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8230858/webrev.00/index.html Looks fine. -Chris.
RFR JDK-8241039, Retire the deprecated SSLSession.getPeerCertificateChain() method
Hi, Could I get the following update reviewed? Bug: https://bugs.openjdk.java.net/browse/JDK-8241039 CSR: https://bugs.openjdk.java.net/browse/JDK-8241047 webrev: http://cr.openjdk.java.net/~xuelei/8241039/webrev.00/ In a preview review thread, https://mail.openjdk.java.net/pipermail/security-dev/2020-March/021401.html I requested to remove the deprecated javax.security.cert APIs in JDK 15. Be part of the removal, the deprecated interface method javax.net.ssl.SSLSession.getPeerCertificateChain() is also involved. As SSLSession.getPeerCertificateChain() is an interface method, third party's implementation must override this method. If it is removed, there are compiler errors unless the override implementation get removed in third party's source code. Maybe, we could retire SSLSession.getPeerCertificateChain() first, and then come back to remove the deprecated javax.security.cert package in a few years. In this update, I'm trying to change SSLSession.getPeerCertificateChain() to default method , throwing exception in the default implementation, and removing the real implementation in the SunJSSE provider and related code (Httpclient). Thanks, Xuelei
Re: RFR JDK-8241039, Retire the deprecated SSLSession.getPeerCertificateChain() method
Hi Alan, Thanks for the review. All comments look good to me. Here is the update webrev: http://cr.openjdk.java.net/~xuelei/8241039/webrev.01/ HandshakeCompletedEvent.java and SSLSession.java are updated. Thanks, Xuelei On 3/16/2020 12:36 AM, Alan Bateman wrote: On 16/03/2020 04:25, Xuelei Fan wrote: Hi, Could I get the following update reviewed? Bug: https://bugs.openjdk.java.net/browse/JDK-8241039 CSR: https://bugs.openjdk.java.net/browse/JDK-8241047 webrev: http://cr.openjdk.java.net/~xuelei/8241039/webrev.00/ I see you've created a new issue (and sub-tasks), in which JDK-8227024, CSR and other tasks need to be closed to avoid any confusion. SSLSession - the method is already terminally deprecated so there is a big note in the javadoc. No need to duplicate the text in an @implNote. For @implSpec then I think you want "The default method throws ". HandshakeCompletedEvent - you can drop the @implNote here too. Catching UOE and then re-throwing it with the UOE from SSLSession as the cause is confusing. I think you can leave the implementation as-is. > -Alan.
Re: RFR JDK-8241039, Retire the deprecated SSLSession.getPeerCertificateChain() method
On 3/16/2020 3:37 AM, Daniel Fuchs wrote: Hi Xuelei, HandshakeCompletedEvent.java: typo: 186 "This method has retired, pleaase use the " + Same in SSLSession.java: 303 "This method has retired, pleaase use the " + I removed the sections per Alan's comment. WRT to the HttpClient code I wonder whether the deprecated method should be kept. On the one hand I'd welcome the removal of the implemenatation of terminally deprecated methods. On the other hand the two classes in HttpClient implement simple delegation over an SSLSession object. Unless we can guarantee that this object is our own implementation, maybe the delegation should be kept - and the throwing of UnsupportedOperationException left up to the delegate object? I am not sure what's the best course here. There are compiler error if SSLSession.getPeerCertificateChain() get removed, while the implementation override it. As one of the goals, the implementation, especially third party provider that is intended to support multiple releases, should remove override implementation as soon as possible, without waiting for the removal of the SSLSession.getPeerCertificateChain() method. Otherwise, there are still compiler error when we want to remove this interface method in the future. It should be fine to keep the HttpClient implementation as it only ship with the current JDK release. But if you don't mind, I would like to remove it to show an example about how to handle with the method in third party's source code. Thanks, Xuelei best regards -- daniel On 16/03/2020 04:25, Xuelei Fan wrote: Hi, Could I get the following update reviewed? Bug: https://bugs.openjdk.java.net/browse/JDK-8241039 CSR: https://bugs.openjdk.java.net/browse/JDK-8241047 webrev: http://cr.openjdk.java.net/~xuelei/8241039/webrev.00/ In a preview review thread, https://mail.openjdk.java.net/pipermail/security-dev/2020-March/021401.html I requested to remove the deprecated javax.security.cert APIs in JDK 15. Be part of the removal, the deprecated interface method javax.net.ssl.SSLSession.getPeerCertificateChain() is also involved. As SSLSession.getPeerCertificateChain() is an interface method, third party's implementation must override this method. If it is removed, there are compiler errors unless the override implementation get removed in third party's source code. Maybe, we could retire SSLSession.getPeerCertificateChain() first, and then come back to remove the deprecated javax.security.cert package in a few years. In this update, I'm trying to change SSLSession.getPeerCertificateChain() to default method , throwing exception in the default implementation, and removing the real implementation in the SunJSSE provider and related code (Httpclient). Thanks, Xuelei
Re: RFR JDK-8241039, Retire the deprecated SSLSession.getPeerCertificateChain() method
I updated the CSR and local code. Thanks, Xuelei On 3/16/2020 10:23 AM, Alan Bateman wrote: On 16/03/2020 16:00, Xuelei Fan wrote: Hi Alan, Thanks for the review. All comments look good to me. Here is the update webrev: http://cr.openjdk.java.net/~xuelei/8241039/webrev.01/ HandshakeCompletedEvent.java and SSLSession.java are updated. Thanks. One suggestion is to use "The default implementation ..." rather than "This default method ..." in the @implSpec (no need for a new webrev). -Alan.
Re: RFR JDK-8239595/JDK-8239594 : ssl context version is not respected/jdk.tls.client.protocols is not respected
With this update, the logic looks like: if TLSv1.3 is not enabled in the
SSLContext, use TLSv1.2 instead; Otherwise, use TLSv1.3 and TLSv1.2.
There may be a couple of issues:
1. TLSv1.2 may be not enabled, although TLSv1.3 is enabled.
For example:
System.setProperty("jdk.tls.client.protocols", "TLSv1.3")
System.setProperty("jdk.tls.client.protocols", "TLSv1.1, TLSv1.0")
2. TLSv1.2 may be not supported in the SSLContext.
For example:
SSLContext context = SSLContext.getInstance("DTLS");
HttpClient.newBuilder().sslContext(context)...
3. The application may not want to use TLS 1.2.
For example:
System.setProperty("jdk.tls.client.protocols", "TLSv1.1, TLSv1.0")
The System property may be shared by code other than httpclient. So the
setting may not consider the impact on httpclient.
I may use enabled protocols only. If no TLSv1.2/TLSv1.3, I may use an
empty protocol array, and test to see what happens in the httpclient
implementation stack.
Xuelei
On 3/26/2020 9:28 AM, Sean Mullan wrote:
Cross-posting to security-dev as this involves TLS/SSL configuration.
--Sean
On 3/26/20 10:02 AM, rahul.r.ya...@oracle.com wrote:
Hello,
Request to have my fix reviewed for issues:
JDK-8239595 : ssl context version is not respected
JDK-8239594 : jdk.tls.client.protocols is not respected
The fix updates
jdk.internal.net.http.HttpClientImpl.getDefaultParams(SSLContext ctx)
to use ctx.getDefaultSSLParameters()instead of
ctx.getSupportedSSLParameters(),
as the latter does not respect the context parameters set by the user.
Issue: https://bugs.openjdk.java.net/browse/JDK-8239595
Issue: https://bugs.openjdk.java.net/browse/JDK-8239594
Webrev:
http://cr.openjdk.java.net/~jboes/rayayada/webrevs/8239595/webrev.00/
-- Rahul
Re: RFR JDK-8239595/JDK-8239594 : ssl context version is not respected/jdk.tls.client.protocols is not respected
On 3/27/2020 5:52 AM, Chris Hegarty wrote:
Xuelei,
Before commenting further on the interaction of the HTTP Client with various
contorted configurations, I would like to get a better understanding of the
`jdk.tls.client.protocols` property.
Is there a specification or other documentation describing
`jdk.tls.client.protocols` ?
See the jdk.tls.client.protocols line in table 'Table 8-3 System
Properties and Customized Items" in JSSE Reference Guides:
"https://docs.oracle.com/en/java/javase/14/security/java-secure-socket-extension-jsse-reference-guide.html#GUID-A41282C3-19A3-400A-A40F-86F4DA22ABA9
For your quick reference, I copied the note here:
---
Customized Item:
Default handshaking protocols for TLS/DTLS clients.
Notes:
To enable specific SunJSSE protocols on the client, specify them in a
comma-separated list within quotation marks; all other supported
protocols are not enabled on the client
For example,
If jdk.tls.client.protocols="TLSv1,TLSv1.1", then the default
protocol settings on the client for TLSv1 and TLSv1.1 are enabled, while
SSLv3, TLSv1.2, TLSv1.3, and SSLv2Hello are not enabled
If jdk.tls.client.protocols="DTLSv1.2" , then the protocol setting
on the client for DTLS1.2 is enabled, while DTLS1.0 is not enabled
---
It is my understanding that the property only affects the *default* protocol’s ( not
the supported protocols ) of the *default* context. That is, the context returned by
`SSLContext.getInstance("Default”)`,
It is correct that the property impact the default SSLContext only. The
default SSLContext instance could get from:
SSLContext.getInstance("Default");
SSLContext.getInstance("TLS");
SSLContext.getInstance("DTLS");
and the protocol values returned by the following invocation on that context
`getDefaultSSLParameters().getProtocols()`. Is this correct? If not, what does
it do?
Yes.
Xuelei
-Chris.
On 26 Mar 2020, at 16:58, Xuelei Fan wrote:
With this update, the logic looks like: if TLSv1.3 is not enabled in the
SSLContext, use TLSv1.2 instead; Otherwise, use TLSv1.3 and TLSv1.2.
There may be a couple of issues:
1. TLSv1.2 may be not enabled, although TLSv1.3 is enabled.
For example:
System.setProperty("jdk.tls.client.protocols", "TLSv1.3")
System.setProperty("jdk.tls.client.protocols", "TLSv1.1, TLSv1.0")
2. TLSv1.2 may be not supported in the SSLContext.
For example:
SSLContext context = SSLContext.getInstance("DTLS");
HttpClient.newBuilder().sslContext(context)...
3. The application may not want to use TLS 1.2.
For example:
System.setProperty("jdk.tls.client.protocols", "TLSv1.1, TLSv1.0")
The System property may be shared by code other than httpclient. So the
setting may not consider the impact on httpclient.
I may use enabled protocols only. If no TLSv1.2/TLSv1.3, I may use an empty
protocol array, and test to see what happens in the httpclient implementation
stack.
Xuelei
On 3/26/2020 9:28 AM, Sean Mullan wrote:
Cross-posting to security-dev as this involves TLS/SSL configuration.
--Sean
On 3/26/20 10:02 AM, rahul.r.ya...@oracle.com wrote:
Hello,
Request to have my fix reviewed for issues:
JDK-8239595 : ssl context version is not respected
JDK-8239594 : jdk.tls.client.protocols is not respected
The fix updates
jdk.internal.net.http.HttpClientImpl.getDefaultParams(SSLContext ctx)
to use ctx.getDefaultSSLParameters()instead of ctx.getSupportedSSLParameters(),
as the latter does not respect the context parameters set by the user.
Issue: https://bugs.openjdk.java.net/browse/JDK-8239595
Issue: https://bugs.openjdk.java.net/browse/JDK-8239594
Webrev: http://cr.openjdk.java.net/~jboes/rayayada/webrevs/8239595/webrev.00/
-- Rahul
Re: RFR JDK-8239595/JDK-8239594 : ssl context version is not respected/jdk.tls.client.protocols is not respected
On 3/27/2020 10:36 AM, Chris Hegarty wrote:
Thank you Xuelei, this very helpful.
Sorry, but I am going to ask just a few more clarifying questions to make sure
that we’re on the same page.
On 27 Mar 2020, at 16:23, Xuelei Fan wrote:
On 3/27/2020 5:52 AM, Chris Hegarty wrote:
Xuelei,
Before commenting further on the interaction of the HTTP Client with various
contorted configurations, I would like to get a better understanding of the
`jdk.tls.client.protocols` property.
Is there a specification or other documentation describing
`jdk.tls.client.protocols` ?
See the jdk.tls.client.protocols line in table 'Table 8-3 System Properties and
Customized Items" in JSSE Reference Guides:
"https://docs.oracle.com/en/java/javase/14/security/java-secure-socket-extension-jsse-reference-guide.html#GUID-A41282C3-19A3-400A-A40F-86F4DA22ABA9
For your quick reference, I copied the note here:
---
Customized Item:
Default handshaking protocols for TLS/DTLS clients.
Notes:
To enable specific SunJSSE protocols on the client, specify them in a
comma-separated list within quotation marks; all other supported protocols are
not enabled on the client
“supported” here means protocols that are supported by the provider, and may be
used within a specific context. This translates, for the default SSLContext, to
the API call getSupportedSSLParameters().getProtocols(), right?
Yes.
getSupportedSSLParameters().getProtocols() returns a superset of
getDefaultSSLParameters().getProtocols(). Conversely,
getDefaultSSLParameters().getProtocols() is a strict subset of
getSupportedSSLParameters().getProtocols(), right?
Yes.
The `jdk.tls.client.protocols` property has no affect on
getSupportedSSLParameters().getProtocols() only
getDefaultSSLParameters().getProtocols(), right?
Yes.
In which case, getDefaultSSLParameters().getProtocols() returns the value of
`jdk.tls.client.protocols`.
For example,
If jdk.tls.client.protocols="TLSv1,TLSv1.1", then the default protocol
settings on the client for TLSv1 and TLSv1.1 are enabled, while SSLv3, TLSv1.2, TLSv1.3,
and SSLv2Hello are not enabled
If jdk.tls.client.protocols="DTLSv1.2" , then the protocol setting on the
client for DTLS1.2 is enabled, while DTLS1.0 is not enabled
---
Seems that the term “client” here is referring to client-initiated exchanges,
rather than any specific technology.
The assumption, which is reasonable, is that “clients” will use the default
context. Again, this is reasonable default out-of-the-box behavior.
The client refer to the client side SSLSocket or SSLEngine created with
the default SSLContext. or example:
SSLContext sslContext = SSLContext.getInstance("TLS");
SSLEngine sslEngine = sslContext.createSSLEngine();
sslEngine.setUseClientMode(true);
The sslEngine object is a client that impacted by the property.
While if
sslEngine.setUseClientMode(false);
then the object should not be impacted by the property.
Xuelei
It is my understanding that the property only affects the *default* protocol’s ( not
the supported protocols ) of the *default* context. That is, the context returned by
`SSLContext.getInstance("Default”)`,
It is correct that the property impact the default SSLContext only. The
default SSLContext instance could get from:
SSLContext.getInstance("Default");
SSLContext.getInstance("TLS");
SSLContext.getInstance("DTLS”);
Thanks for this clarification.
and the protocol values returned by the following invocation on that context
`getDefaultSSLParameters().getProtocols()`. Is this correct? If not, what does
it do?
Yes.
Thanks,
-Chris.
hg: jdk7/jsn/jdk: 6546639: (spec)javax.net.ssl.SSLContext.getInstance(null) throws undocumented NPE
Changeset: c0eb84957bea Author:xuelei Date: 2008-04-11 03:33 -0400 URL: http://hg.openjdk.java.net/jdk7/jsn/jdk/rev/c0eb84957bea 6546639: (spec)javax.net.ssl.SSLContext.getInstance(null) throws undocumented NPE Summary: add NullPointerException description to those methods. Reviewed-by: weijun ! src/share/classes/javax/net/ssl/SSLContext.java
hg: jdk7/jsn/jdk: 6546671: (spec)javax.net.ssl.TrustManagerFactory.getInstance() throws undocumented NP; ...
Changeset: da9fa1fa9b95 Author:xuelei Date: 2008-04-11 03:43 -0400 URL: http://hg.openjdk.java.net/jdk7/jsn/jdk/rev/da9fa1fa9b95 6546671: (spec)javax.net.ssl.TrustManagerFactory.getInstance() throws undocumented NP 5053895: (spec) Unspecified IllegalStateException in TrustManagerFactory Summary: add NullPointerException/IllegalStateException description Reviewed-by: weijun ! src/share/classes/javax/net/ssl/TrustManagerFactory.java ! src/share/classes/javax/net/ssl/TrustManagerFactorySpi.java
hg: jdk7/jsn/jdk: 6571950: SSLSocket(raddr, rport, laddr, lport) allows null as laddr that spec doesn't reflect
Changeset: 143e1a9b51a9 Author:xuelei Date: 2008-04-11 03:50 -0400 URL: http://hg.openjdk.java.net/jdk7/jsn/jdk/rev/143e1a9b51a9 6571950: SSLSocket(raddr, rport, laddr, lport) allows null as laddr that spec doesn't reflect Summary: add the description that while the local address parameter is null, anyLocalAddress will be used instead. Reviewed-by: weijun ! src/share/classes/java/net/Socket.java ! src/share/classes/javax/net/ssl/SSLSocket.java
Re: Where to File Bug Reports?
Refer to: http://bugreport.sun.com/bugreport/, at the bottom of the page, there is a start a new report button, report a bug from here, please. - xuelei Ole Ersoy wrote: Hi, Could someone please tell the URL for filing an openjdk bug report? Thanks, - Ole
hg: jdk7/tl/jdk: 6728126: Parsing Extensions in Client Hello message is done in a wrong way
Changeset: 16efbe49c725 Author:xuelei Date: 2008-11-13 23:08 -0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/16efbe49c725 6728126: Parsing Extensions in Client Hello message is done in a wrong way Summary: the inputStream.read(byte[], int, 0) is not always return zero. Reviewed-by: wetmore, weijun ! src/share/classes/sun/security/ssl/HelloExtensions.java + test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/EmptyExtensionData.java
hg: jdk7/tl/jdk: 6745052: SLServerSocket file descriptor leak
Changeset: dcb8d806d731 Author:xuelei Date: 2008-11-13 23:25 -0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/dcb8d806d731 6745052: SLServerSocket file descriptor leak Summary: SSLServerSocketImpl.checkEnabledSuites() does not release the temporary socket properly Reviewed-by: wetmore, weijun ! src/share/classes/sun/security/ssl/BaseSSLSocketImpl.java ! src/share/classes/sun/security/ssl/SSLServerSocketImpl.java ! src/share/classes/sun/security/ssl/SSLSocketImpl.java
hg: jdk7/tl/jdk: 6750401: SSL stress test with GF leads to 32 bit max process size in less than 5 minutes, with PCKS11 provider
Changeset: 85fe3cd9d6f9 Author:wetmore Date: 2008-12-19 10:35 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/85fe3cd9d6f9 6750401: SSL stress test with GF leads to 32 bit max process size in less than 5 minutes,with PCKS11 provider Summary: This is the JSSE portion of the fix. Main part is in PKCS11. Reviewed-by: valeriep, xuelei ! src/share/classes/sun/security/ssl/CipherBox.java ! src/share/classes/sun/security/ssl/SSLEngineImpl.java ! src/share/classes/sun/security/ssl/SSLSocketImpl.java
hg: jdk7/tl/jdk: 6782783: regtest HttpsURLConnection/B6216082.java throws ClosedByInterruptException
Changeset: a96a1f0edeeb Author:xuelei Date: 2009-02-04 19:10 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/a96a1f0edeeb 6782783: regtest HttpsURLConnection/B6216082.java throws ClosedByInterruptException Summary: make the test robust Reviewed-by: weijun ! test/sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/B6216082.java
hg: jdk7/tl/jdk: 4918870: Examine session cache implementation (sun.misc.Cache)
Changeset: a144afafb6fe Author:xuelei Date: 2009-02-20 12:50 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/a144afafb6fe 4918870: Examine session cache implementation (sun.misc.Cache) Summary: replace sun.misc.Cache with sun.security.util.Cache Reviewed-by: weijun ! src/share/classes/sun/security/ssl/SSLSessionContextImpl.java ! src/share/classes/sun/security/util/Cache.java
hg: jdk7/tl/jdk: 6697270: Inputstream dosent behave correct
Changeset: 6bdbb2f5c763 Author:xuelei Date: 2009-02-20 13:05 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/6bdbb2f5c763 6697270: Inputstream dosent behave correct Summary: do not try to read zero byte from a InputStream, and do always return immediately for zero byte reading in a InputStream implementation. Reviewed-by: weijun ! src/share/classes/sun/security/ssl/AppInputStream.java ! src/share/classes/sun/security/ssl/AppOutputStream.java ! src/share/classes/sun/security/ssl/ByteBufferInputStream.java + test/sun/security/ssl/com/sun/net/ssl/internal/ssl/AppInputStream/ReadZeroBytes.java
hg: jdk7/tl/jdk: 5067458: Loopback SSLSocketImpl createSocket is throwing an exception
Changeset: 2a7c1a997102 Author:xuelei Date: 2009-02-23 17:32 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/2a7c1a997102 5067458: Loopback SSLSocketImpl createSocket is throwing an exception Summary: A null hostname should be regarded as a loopback address. Reviewed-by: weijun ! src/share/classes/sun/security/ssl/SSLSocketImpl.java + test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLSocketImpl/LoopbackSSLSocket.java
hg: jdk7/tl/jdk: 6549506: Specification of Permission.toString() method contradicts with JDK implementation
Changeset: b656e842e1be Author:xuelei Date: 2009-03-02 23:17 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/b656e842e1be 6549506: Specification of Permission.toString() method contradicts with JDK implementation Summary: update the spec, and add double quotes around component. Reviewed-by: weijun ! src/share/classes/java/security/Permission.java + test/java/security/Permission/ToString.java
hg: jdk7/tl/jdk: 6798714: OCSPResponse class has to check the validity of signing certificate for OCSP response
Changeset: f381e737916d Author:xuelei Date: 2009-03-13 12:59 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/f381e737916d 6798714: OCSPResponse class has to check the validity of signing certificate for OCSP response Summary: checking validity and ocsp-nocheck extension. Reviewed-by: mullan, vinnie ! src/share/classes/sun/security/provider/certpath/OCSPResponse.java + src/share/classes/sun/security/x509/OCSPNoCheckExtension.java ! src/share/classes/sun/security/x509/OIDMap.java ! src/share/classes/sun/security/x509/PKIXExtensions.java
hg: jdk7/tl/jdk: 6383095: CRL revoked certificate failures masked by OCSP failures
Changeset: 181472dbbebb Author:xuelei Date: 2009-03-17 11:54 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/181472dbbebb 6383095: CRL revoked certificate failures masked by OCSP failures Summary: remove the mask if certificate revoked Reviewed-by: mullan ! src/share/classes/sun/security/provider/certpath/PKIXMasterCertPathValidator.java + test/java/security/cert/CertPathValidator/OCSP/FailoverToCRL.java
hg: jdk7/tl/jdk: 6822460: support self-issued certificate
Changeset: d93b7df1e260 Author:xuelei Date: 2009-05-26 16:19 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/d93b7df1e260 6822460: support self-issued certificate Summary: checking self-issued certificate during certification path building Reviewed-by: mullan, weijun ! src/share/classes/sun/security/validator/PKIXValidator.java ! src/share/classes/sun/security/validator/SimpleValidator.java + test/sun/security/ssl/com/sun/net/ssl/internal/ssl/X509TrustManagerImpl/SelfIssuedCert.java
hg: jdk7/tl/jdk: 6720721: CRL check with circular depency support needed
Changeset: c3c5cc0f2a3e Author:xuelei Date: 2009-05-26 16:43 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/c3c5cc0f2a3e 6720721: CRL check with circular depency support needed Summary: checking AKID of certificates and CRLs Reviewed-by: mullan, weijun ! src/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java + test/java/security/cert/CertPathValidator/indirectCRL/CircularCRLOneLevel.java + test/java/security/cert/CertPathValidator/indirectCRL/CircularCRLOneLevelRevoked.java + test/java/security/cert/CertPathValidator/indirectCRL/CircularCRLTwoLevel.java + test/java/security/cert/CertPathValidator/indirectCRL/CircularCRLTwoLevelRevoked.java + test/java/security/cert/CertPathValidator/indirectCRL/README + test/java/security/cert/CertPathValidator/indirectCRL/generate.sh + test/java/security/cert/CertPathValidator/indirectCRL/openssl.cnf
hg: jdk7/tl/jdk: 6845286: Add regression test for name constraints
Changeset: 25db260cb810 Author:xuelei Date: 2009-05-27 17:48 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/25db260cb810 6845286: Add regression test for name constraints Summary: create regression test cases on name constraints Reviewed-by: weijun + test/java/security/cert/CertPathValidator/nameConstraints/NameConstraintsWithRID.java + test/java/security/cert/CertPathValidator/nameConstraints/NameConstraintsWithUnexpectedRID.java + test/java/security/cert/CertPathValidator/nameConstraints/NameConstraintsWithoutRID.java + test/java/security/cert/CertPathValidator/nameConstraints/generate.sh + test/java/security/cert/CertPathValidator/nameConstraints/openssl.cnf
hg: jdk7/tl/jdk: 6847459: Allow trust anchor self-issued intermediate version 1 and version 2 certificate
Changeset: 045743e0eb2d Author:xuelei Date: 2009-06-04 11:28 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/045743e0eb2d 6847459: Allow trust anchor self-issued intermediate version 1 and version 2 certificate Reviewed-by: weijun ! src/share/classes/sun/security/provider/certpath/ConstraintsChecker.java
hg: jdk7/tl/jdk: 6570344: Invalid RSA OID in sun.security.x509.AlgorithmId
Changeset: ffbcf1d1103c Author:xuelei Date: 2009-06-12 09:00 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/ffbcf1d1103c 6570344: Invalid RSA OID in sun.security.x509.AlgorithmId Summary: change RSA OID to "2.5.8.1.1" Reviewed-by: mullan ! src/share/classes/sun/security/x509/AlgorithmId.java
hg: jdk7/tl/jdk: 6850783: InvalidityDateExtension returns reference to internal mutable state
Changeset: 1299804aa6e7 Author:xuelei Date: 2009-06-16 20:46 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/1299804aa6e7 6850783: InvalidityDateExtension returns reference to internal mutable state Summary: return cloned instead of referenced object Reviewed-by: weijun ! src/share/classes/sun/security/x509/CertificateVersion.java ! src/share/classes/sun/security/x509/InvalidityDateExtension.java
hg: jdk7/tl/jdk: 6853793: OutOfMemoryError in sun.security.provider.certpath.OCSPChecker.check
Changeset: c2baa2f0415e Author:xuelei Date: 2009-07-03 11:13 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/c2baa2f0415e 6853793: OutOfMemoryError in sun.security.provider.certpath.OCSPChecker.check Summary: allocate memory dynamically, keep reading until EOF. Reviewed-by: weijun ! src/share/classes/sun/security/provider/certpath/OCSPChecker.java ! src/share/classes/sun/security/timestamp/HttpTimestamper.java
hg: jdk7/tl/jdk: 6852744: PIT b61: PKI test suite fails because self signed certificates are beingrejected
Changeset: 6f26e2e5f4f3 Author:xuelei Date: 2009-07-10 17:27 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/6f26e2e5f4f3 6852744: PIT b61: PKI test suite fails because self signed certificates are beingrejected Summary: make the builder aware of SKID/AKID, break the internal circular dependences Reviewed-by: mullan ! src/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java ! src/share/classes/sun/security/provider/certpath/ForwardBuilder.java + test/java/security/cert/CertPathBuilder/selfIssued/DisableRevocation.java + test/java/security/cert/CertPathBuilder/selfIssued/KeyUsageMatters.java + test/java/security/cert/CertPathBuilder/selfIssued/README + test/java/security/cert/CertPathBuilder/selfIssued/StatusLoopDependency.java + test/java/security/cert/CertPathBuilder/selfIssued/generate.sh + test/java/security/cert/CertPathBuilder/selfIssued/openssl.cnf
hg: jdk7/tl/jdk: 6453837: PartialCompositeContext.allEmpty is buggy
Changeset: d0ce095004b2 Author:xuelei Date: 2009-07-13 23:01 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/d0ce095004b2 6453837: PartialCompositeContext.allEmpty is buggy Reviewed-by: weijun ! src/share/classes/com/sun/jndi/toolkit/ctx/PartialCompositeContext.java
hg: jdk7/tl/jdk: 6449574: Invalid ldap filter is accepted and processed
Changeset: d78bfd73ee42 Author:xuelei Date: 2009-07-27 22:04 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/d78bfd73ee42 6449574: Invalid ldap filter is accepted and processed Reviewed-by: vinnie ! src/share/classes/com/sun/jndi/ldap/Filter.java + test/com/sun/jndi/ldap/BalancedParentheses.java
hg: jdk7/tl/jdk: 6865482: test case BalancedParentheses.java is missing GPL header.
Changeset: 056c8e724015 Author:xuelei Date: 2009-07-28 11:15 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/056c8e724015 6865482: test case BalancedParentheses.java is missing GPL header. Reviewed-by: weijun ! test/com/sun/jndi/ldap/BalancedParentheses.java
hg: jdk7/tl/jdk: 6585239: Regression: 2 DNS tests fail with JDK 5.0u13 b01 and pass with 5.0u12fcs
Changeset: aface89bfcf6 Author:xuelei Date: 2009-08-11 18:27 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/aface89bfcf6 6585239: Regression: 2 DNS tests fail with JDK 5.0u13 b01 and pass with 5.0u12fcs Reviewed-by: vinnie ! src/share/classes/com/sun/jndi/dns/DnsContext.java
hg: jdk7/tl/jdk: 6862064: incorrect implementation of PKIXParameters.clone()
Changeset: dca3a251a001 Author:xuelei Date: 2010-01-20 21:38 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/dca3a251a001 6862064: incorrect implementation of PKIXParameters.clone() Reviewed-by: weijun, mullan ! src/share/classes/java/security/cert/PKIXParameters.java
hg: jdk7/tl/jdk: 6916202: More cases of invalid ldap filters accepted and processed
Changeset: 9929203a8b98 Author:xuelei Date: 2010-02-25 13:32 +0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/9929203a8b98 6916202: More cases of invalid ldap filters accepted and processed Reviewed-by: vinnie, weijun ! src/share/classes/com/sun/jndi/ldap/Filter.java + test/com/sun/jndi/ldap/InvalidLdapFilters.java
