Re: CSR Review Request, JDK-8213577, Update the default SSL session cache size to 20480
Hi Xuelei, I've added myself as a reviewer. --Jamil On 11/9/2018 10:13 AM, Xuelei Fan wrote: On 11/9/2018 9:34 AM, Jamil Nimeh wrote: Hi Xuelei, Content looks good. I'd remove specific references to Amazon from the CSR (it's fine to leave it in the source bug though). Removed. Where'd you get the 20480 session cache limit from? I saw a similar limit using the builtin SSL session cache from NGINX, is that where that number comes from? Or is that common to other TLS library or webserver cache sizes? The number is coming from what NGINX is using. In the bug description, it is said 10K could be a good one. However, the number is really depends on the platform resources. I'd like to use a bigger one so that the performance impact of the existing applications is as minimal as possible. Thanks, Xuelei --Jamil On 11/8/2018 8:00 PM, Xuelei Fan wrote: Hi, Please review the proposal to update the default SSL session cache size from infinite to 20480. https://bugs.openjdk.java.net/browse/JDK-8213577 I know that the default 20480 does not fit all. I'd appreciate your feedback if the value is acceptable. Thanks, Xuelei
Re: RFR 8212003: Obsoleting the default keytool -keyalg option
The CSR looks fine to me. Thanks. -coderaptor On Wed, Nov 7, 2018 at 7:53 AM Weijun Wang wrote: > > Oops, I take this back. The CSR needs more update. > > Sorry if you have already start reading it. > > Thanks > Max > > > > On Nov 7, 2018, at 9:27 AM, Weijun Wang wrote: > > > > After some discussion, we decided to cover -keysize and -sigalg in this > > deprecation process too. > > > > Please review the updated CSR at > > > >https://bugs.openjdk.java.net/browse/JDK-8212111 > > > > No webrev available yet. > > > > Thanks > > Max > > > > > >> On Oct 18, 2018, at 10:34 AM, Weijun Wang wrote: > >> > >> Please review the code change and CSR for > >> > >> JBS: https://bugs.openjdk.java.net/browse/JDK-8212003 > >> > >> at > >> > >> webrev: http://cr.openjdk.java.net/~weijun/8212003/webrev.00/ > >> CSR: https://bugs.openjdk.java.net/browse/JDK-8212111 > >> > >> When -keyalg is not provided for -genkeypair or -genseckey, keytool will > >> print out a warning. We plan to make this an error in a future release. > >> > >> A new regression test ObsoleteKeyalg.java added. "-keyalg DSA" or "-keyalg > >> DES" added to other tests. > >> > >> A Mach5 job on tier1 and tier2 running now. > >> > >> Thanks > >> Max > >> > > >
RFR 8213202: Possible race condition in TLS 1.3 session resumption
JBS: https://bugs.openjdk.java.net/browse/JDK-8213202 webrev: http://cr.openjdk.java.net/~apetcher/8213202/webrev.00/ This change fixes a bug that allows multiple clients thread to offer the same PSK to the server, even though only one thread may actually use the PSK to resume the session. The other threads will fail to connect and throw an exception. This is noreg-hard because the bug doesn't happen with the JDK TLS server, and we don't have a regression test harness that allows us to simulate some particular server behavior. I tested the fix by connecting multiple JDK clients to an openssl server.
Re: CSR Review Request, JDK-8213577, Update the default SSL session cache size to 20480
I think that's fine as a default. It can be tuned for those sites that need something larger or smaller. --Jamil On 11/9/2018 10:13 AM, Xuelei Fan wrote: On 11/9/2018 9:34 AM, Jamil Nimeh wrote: Hi Xuelei, Content looks good. I'd remove specific references to Amazon from the CSR (it's fine to leave it in the source bug though). Removed. Where'd you get the 20480 session cache limit from? I saw a similar limit using the builtin SSL session cache from NGINX, is that where that number comes from? Or is that common to other TLS library or webserver cache sizes? The number is coming from what NGINX is using. In the bug description, it is said 10K could be a good one. However, the number is really depends on the platform resources. I'd like to use a bigger one so that the performance impact of the existing applications is as minimal as possible. Thanks, Xuelei --Jamil On 11/8/2018 8:00 PM, Xuelei Fan wrote: Hi, Please review the proposal to update the default SSL session cache size from infinite to 20480. https://bugs.openjdk.java.net/browse/JDK-8213577 I know that the default 20480 does not fit all. I'd appreciate your feedback if the value is acceptable. Thanks, Xuelei
Re: CSR Review Request, JDK-8213577, Update the default SSL session cache size to 20480
On 11/9/2018 9:34 AM, Jamil Nimeh wrote: Hi Xuelei, Content looks good. I'd remove specific references to Amazon from the CSR (it's fine to leave it in the source bug though). Removed. Where'd you get the 20480 session cache limit from? I saw a similar limit using the builtin SSL session cache from NGINX, is that where that number comes from? Or is that common to other TLS library or webserver cache sizes? The number is coming from what NGINX is using. In the bug description, it is said 10K could be a good one. However, the number is really depends on the platform resources. I'd like to use a bigger one so that the performance impact of the existing applications is as minimal as possible. Thanks, Xuelei --Jamil On 11/8/2018 8:00 PM, Xuelei Fan wrote: Hi, Please review the proposal to update the default SSL session cache size from infinite to 20480. https://bugs.openjdk.java.net/browse/JDK-8213577 I know that the default 20480 does not fit all. I'd appreciate your feedback if the value is acceptable. Thanks, Xuelei
Re: CSR Review Request, JDK-8213577, Update the default SSL session cache size to 20480
Hi Xuelei, Content looks good. I'd remove specific references to Amazon from the CSR (it's fine to leave it in the source bug though). Where'd you get the 20480 session cache limit from? I saw a similar limit using the builtin SSL session cache from NGINX, is that where that number comes from? Or is that common to other TLS library or webserver cache sizes? --Jamil On 11/8/2018 8:00 PM, Xuelei Fan wrote: Hi, Please review the proposal to update the default SSL session cache size from infinite to 20480. https://bugs.openjdk.java.net/browse/JDK-8213577 I know that the default 20480 does not fit all. I'd appreciate your feedback if the value is acceptable. Thanks, Xuelei
Re: Code Review Request : JDK-8213694 : Test Timeout.java should run in othervm mode
Looks fine to me. On 11/9/2018 8:43 AM, Xuelei Fan wrote: Hi, Could I get the following small change reviewed please? http://cr.openjdk.java.net/~xuelei/8213694/webrev.00/ The test SSLSessionContextImpl/Timeout.java is running in the default mode. As the test initializes the SSLContext with the current System Properties, while the SunJSSE provider does not support dynamic System Properties, this test may impact other test cases running in samevm/agentvm mode. The impact is very hard to debugging. I updated the test to run in othervm mode, and cleanup the code by removing commented out codes and close the server socket with a try-with-resource. Thanks, Xuelei
Code Review Request : JDK-8213694 : Test Timeout.java should run in othervm mode
Hi, Could I get the following small change reviewed please? http://cr.openjdk.java.net/~xuelei/8213694/webrev.00/ The test SSLSessionContextImpl/Timeout.java is running in the default mode. As the test initializes the SSLContext with the current System Properties, while the SunJSSE provider does not support dynamic System Properties, this test may impact other test cases running in samevm/agentvm mode. The impact is very hard to debugging. I updated the test to run in othervm mode, and cleanup the code by removing commented out codes and close the server socket with a try-with-resource. Thanks, Xuelei
Re: RFR 8213400: Support choosing curve name in keytool keypair generation
On 11/8/2018 10:30 PM, Weijun Wang wrote:
On Nov 9, 2018, at 12:28 AM, Adam Petcher wrote:
On 11/8/2018 8:10 AM, Weijun Wang wrote:
- CurveDB.java:
-add("sect163r2 [NIST B-163]", "1.3.132.0.15", BD,
+add("sect163r2 [NIST B-163]", "1.3.132.0.15", B,
All other NIST B-*** curves do not have BD. This should have been a typo.
I think this will change the default 163-bit curve from sect163r2 to sect163k1.
We shouldn't change these defaults without a compelling reason.
I think this is a bug, as there should not be 2 curves with the same field size
both having BD.
I can file another bug on this. Maybe it needs it own CSR.
Yes, it's probably a bug/typo. You can also fix it by changing sect163k1
from BD to B, since this won't change the behavior (though this would
need to be tested). But I wouldn't worry about it. You can create a
separate ticket if you like, but I would expect that fixing this is very
low priority. The current behavior may even be correct (or at least "as
intended"), and this is just a code cleanup issue.
- NamedCurve.java:
A new field commonNames added, which is used by the new GroupName.java test.
I don't see why this is necessary. The test is using this list of common names
to make sure the correct named curve is used. Why not just check the value
returned by NamedCurve.getName() against the expected (canonical) name of the
curve? Or check the OID?
NamedCurve.getName() returns "secp256r1 [NIST P-256, X9.62 prime256v1]".
I don't want to canonicalize the name (how? returning NamedCurve.getName()?) or
use an OID. The test is about making sure the curve used matches the one user
specified. What if I am making the same error inside keytool and this
canonicalization or string-to-oid method?
I think what I am suggesting is map from common name to canonical name
or OID in the test. Then you can test that the correct common name is
used, and that the mapping from common name to curve is correct. If you
don't want to write out this map, then you can implement it by looking
up the common name in the EC AlgorithmParameters, and then converting to
an ECParameterSpec. This just moves the complexity from CurveDB to the
test, but I think that is better.
Though I don't have very strong feelings about this. Perhaps the list of
common names in the NamedCurve will be useful for other things. If you
prefer it the way that it is, then I am okay with it.
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
On 11/8/18 4:06 PM, Xuelei Fan wrote: 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/ I think you mean http://cr.openjdk.java.net/~xuelei/8212261/webrev.05/ Looks good. --Sean 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: 8148188: Enhance the security libraries to record events of interest
Erik, comments inline.. On 08/11/18 15:12, Erik Gahlin wrote: Hi Sean, I think we could still call the event "jdk.SecurityPropertyModification", but add a @Description that explains that events are only emitted for the JDK due to security concerns. If we at a later stage want to include user events, we could add those and remove the @Description, possibly with a setting where you can specify scope, or perhaps a new event all together. sounds fine. I've made those changes. Perhaps we could find a better name for the field validationEventNumber? It sounds like we have an event number, which is not really the case. We have a counter for the validation id. How about 'validationCounter' ? I noticed that you use hashCode as an id. I assume this is to simplify the implementation? That is probably OK, the risk of a collision is perhaps low, but could you make the field a long, so we in the future could add something more unique (Flight Recorder uses compressed integers, so a long uses the same amount of disk space as an int). Could you also rename the field and the annotation, perhaps certificationId could work, so we don't leak how the id was implemented. Yes - originally, I was using the certificate serial number but that may not always be unique (esp. for test generated certs). I've changed the variable name to 'certificateId' and made it a long. Renamed the annotation also. I could not find the file: test/lib/jdk/test/lib/security/TestJdkSecurityPropertyModification.java whoops - forgot to add it to mercurial tracking. there now : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v8/webrev/ regards, Sean.
