CSR Review Request, JDK-8213577, Update the default SSL session cache size to 20480
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 8213400: Support choosing curve name in keytool keypair generation
> 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.
>
>>
>> - 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?
In fact, I had wanted to add lines using "alternative names" like "-groupname
prime256v1" in the test but later I found out it must be `-groupname "X9.62
prime256v1"` which is not easy to write in the test. (the
SecurityTools.keytool() method does not like spaces inside an argument).
You might say since I have already called split(" ")[0] in src/ I can also call
it here in a test. But then I still want to have the chance to try out any
alternative name later, especially after we think "prime256v1" is better than
"X9.62 prime256v1".
Thanks
Max
>
>
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: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue
On 11/7/18 3:52 AM, Baesken, Matthias wrote:
Sorry, I haven't had time to look at this in more detail yet. But, let's
take a step back first. Can you or Matthias explain in more detail why
this fix is necessary? What are the use cases and motivation?
Hello,
adding paths (or maybe more details) to exception messages just makes
analyzing errors easier.
You do not get just "Bad path", but also the real bad path which gives you a
hint where to look and analyze further .
File paths are, in general, always something that demands extra scrutiny
as it can be the source of security issues (privacy leaks, traversal
attacks, etc). It's not just me that thinks that way, you can do a
search on the Internet and find lots of references.
That's why we introduced it in our JVM ages ago.
I have to agree that additionally printing cwd / user.dir is a bit special,
so I omit that from this revision of the patch.
This makes the patch more simple.
New revision :
http://cr.openjdk.java.net/~mbaesken/webrevs/8211752.1/
Unfortunately the usage of sun.security.util.SecurityProperties (which was
considered) in the static { ... }
class initializers (e.g. UnixFileSystem.java) just does not work.
It fails with already in the build (!) with :
Error occurred during initialization of boot layer
java.lang.ExceptionInInitializerError
Caused by: java.lang.NullPointerException
(seems it is too early in the game for SecurityProperties).
(btw. this is another NOT very helpful exception error message)
So I unfortunately had to go back to using system properties.
Btw. another question regarding path output in exceptions :
you seem to consider it a bad thing to (unconditionally) print paths in the
exception messages,
but then on the other hand we have it already in OpenJDK UnixFileSystem_md.c :
269 JNIEXPORT jboolean JNICALL
270 Java_java_io_UnixFileSystem_createFileExclusively(JNIEnv *env, jclass cls,
271 jstring pathname)
272 {
...
277 /* The root directory always exists */
278 if (strcmp (path, "/")) {
279 fd = handleOpen(path, O_RDWR | O_CREAT | O_EXCL, 0666);
280 if (fd < 0) {
281 if (errno != EEXIST)
282 JNU_ThrowIOExceptionWithLastError(env, path);
283 } else {
284 if (close(fd) == -1)
285 JNU_ThrowIOExceptionWithLastError(env, path);
Why is it fine here for a long time , but considered harmful at the other
places ?
If we want to be consistent, we should then write "Bad path" here (or allow
the path output at the other places too ).
It might be perfectly fine and your usage might be ok too. But I'll need
some time to evaluate it further. I am not familiar with the code in
this part of the JDK.
I would also see if you can think about the security issues as well.
Where do these paths come from? What are the application use cases that
invoke these lower methods? From application code or elsewhere? Are
relative paths used? Are paths containing ".." canonicalized? Are they
arbitrary paths or a fixed set of paths? Do they ever contain sensitive
information like home directory user names, etc?
Once we understand if there are any security issues, then we can decide
if allowing that via a system property is acceptable or not, and if so
the security risks that we would have to document for that property.
Thanks,
Sean
Thanks, Matthias
-Original Message-
From: Sean Mullan
Sent: Freitag, 12. Oktober 2018 17:19
To: Langer, Christoph ; Baesken, Matthias
; Alan Bateman ;
security-dev@openjdk.java.net; core-libs-...@openjdk.java.net
Subject: Re: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath -
enhance some IOExceptions with path causing the issue
On 10/12/18 10:33 AM, Langer, Christoph wrote:
Sean, what is your take on this?
Sorry, I haven't had time to look at this in more detail yet. But, let's
take a step back first. Can you or Matthias explain in more detail why
this fix is necessary? What are the use cases and motivation? The bug
report doesn't go into any detail about that and there isn't anything
in the initial RFR email that explains why this change is useful or
necessary. As a general guideline or advice, RFEs should include this
type of information so that Reviewers understand more of the context and
motivation behind the change.
Thanks,
Sean
Re: RFR 8213363: X25519 private key PKCS#8 encoding/decoding is incorrect
Oops. And the JBS ticket: https://bugs.openjdk.java.net/browse/JDK-8213363 On 11/8/2018 11:43 AM, Adam Petcher wrote: webrev: http://cr.openjdk.java.net/~apetcher/8213363/webrev.00/ CSR: https://bugs.openjdk.java.net/browse/JDK-8213493 This change fixes a bug related to the encoded format of X25519/X448 private keys. The new code will not preserve compatibility with the improperly-formatted keys produced by the existing code. I expect the compatibility impact of this change to be minimal, as described in the CSR ticket.
RFR 8213363: X25519 private key PKCS#8 encoding/decoding is incorrect
webrev: http://cr.openjdk.java.net/~apetcher/8213363/webrev.00/ CSR: https://bugs.openjdk.java.net/browse/JDK-8213493 This change fixes a bug related to the encoded format of X25519/X448 private keys. The new code will not preserve compatibility with the improperly-formatted keys produced by the existing code. I expect the compatibility impact of this change to be minimal, as described in the CSR ticket.
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
On 08/11/2018 15:03, Sean Mullan wrote: Ah, I see. I am sure there is a good reason, but why doesn't it have an implementation? IIRC there was an implementation in the deploy code. best regards, -- daniel
Re: RFR 8213400: Support choosing curve name in keytool keypair generation
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.
- 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?
Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation
I'm missing the motivation behind this question. Is the current set of
aliases causing some problem? Are they incomplete? Why is it bad that
"X9.62 prime256v1" works but "prime256v1" doesn't?
On 11/7/2018 10:05 PM, Weijun Wang wrote:
In CurveDB.java, we have
add("secp256r1 [NIST P-256, X9.62 prime256v1]", "1.2.840.10045.3.1.7", PD,
"0001",
"0001FFFC",
"5AC635D8AA3A93E7B3EBBD55769886BC651D06B0CC53B0F63BCE3C3E27D2604B",
"6B17D1F2E12C4247F8BCE6E563A440F277037D812DEB33A0F4A13945D898C296",
"4FE342E2FE1A7F9B8EE7EB4A7C0F9E162BCE33576B315ECECBB6406837BF51F5",
"BCE6FAADA7179E84F3B9CAC2FC632551",
1, nameSplitPattern);
So the aliases of secp256r1 are now "NIST P-256" and "X9.62 prime256v1". Do we
really want to keep the organization name prefix after JDK-8208156? The alias can be used in
ECGenParameterSpec and the proposed keytool -groupname option.
The following shows this behavior.
jshell> KeyPairGenerator.getInstance("EC")
$3 ==> java.security.KeyPairGenerator$Delegate@64bfbc86
jshell> $3.initialize(new ECGenParameterSpec("secp256r1"))
jshell> $3.initialize(new ECGenParameterSpec("prime256v1"))
| Exception java.security.InvalidAlgorithmParameterException: Unknown curve
name: prime256v1
|at ECKeyPairGenerator.initialize (ECKeyPairGenerator.java:103)
|at KeyPairGenerator$Delegate.initialize (KeyPairGenerator.java:699)
|at KeyPairGenerator.initialize (KeyPairGenerator.java:436)
|at (#6:1)
jshell> $3.initialize(new ECGenParameterSpec("X9.62 prime256v1"))
Thanks
Max
On Nov 7, 2018, at 11:48 PM, Weijun Wang wrote:
CSR updated. With such a generalized option, I won't recommend -groupname over
-keysize now, although I still intend to print some warning for EC.
Please take a review.
Thanks
Max
Re: RFR: 8148188: Enhance the security libraries to record events of interest
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. 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. 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. I could not find the file: test/lib/jdk/test/lib/security/TestJdkSecurityPropertyModification.java Thanks Erik With JDK-8203629 now pushed, I've re-based my patch on latest jdk/jdk code. Some modifications also made based on off-thread suggestions : src/java.base/share/classes/java/security/Security.java * Only record JDK security properties for now (i.e. those in java.security conf file) - we can consider a new event to record non-JDK security events if demand comes about - new event renamed to *Jdk*SecurityPropertyModificationEvent src/java.base/share/classes/sun/security/x509/X509CertImpl.java * Use hashCode() equality test for storing certs in List. Tests updated to capture these changes src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java * Verify that self signed certs exercise the modified code paths I've added new test functionality to test use of self signed cert. webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v7/webrev/ Regards, Sean. On 17/10/18 11:25, Seán Coffey wrote: I'd like to re-boot this review. I've been working with Erik off thread who's been helping with code and test design. Some of the main changes worthy of highlighting : * Separate out the tests for JFR and Logger events * Rename some events * Normalize the data view for X509ValidationEvent (old event name : CertChainEvent) * Introduce CertificateSerialNumber type to make use of the @Relational JFR annotation. * Keep calls for JFR event operations local to call site to optimize jvm compilation webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/ This code is dependent on the JDK-8203629 enhancement being integrated. Regards, Sean. On 10/07/18 13:50, Seán Coffey wrote: Erik, After some trial edits, I'm not so sure if moving the event & logger commit code into the class where it's used works too well after all. In the code you suggested, there's an assumption that calls such as EventHelper.certificateChain(..) are low cost. While that might be the case here, it's something we can't always assume. It's called once for the JFR operation and once for the Logger operation. That pattern multiplies itself further in other Events. i.e. set up and assign the variables for a JFR event without knowing whether they'll be needed again for the Logger call. We could work around it by setting up some local variables and re-using them in the Logger code but then, we're back to where we were. The current EventHelper design eliminates this problem and also helps to abstract the recording/logging functionality away from the functionality of the class itself. It also becomes a problem where we record events in multiple different classes (e.g. SecurityPropertyEvent). While we could leave the code in EventHelper for this case, we then have a mixed design pattern. Are you ok with leaving as is for now? A future wish might be one where JFR would handle Logger via its own framework/API in a future JDK release. I've removed the variable names using underscore. Also optimized some variable assignments in X509Impl.commitEvent(..) http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/ regards, Sean. On 09/07/2018 18:01, Seán Coffey wrote: Erik, Thanks for reviewing. Comments inline.. On 09/07/18 17:21, Erik Gahlin wrote: Thanks Sean. Some feedback on the code in the security libraries. - We should use camel case naming convention for variables (not underscore). Sure. I see two offending variable names which I'll fix up. - Looking at sun/security/ssl/Finished.java, I wonder if it wouldn't be less code and more easy to read, if we would commit the event in a local private function and then use the EventHelper class for common functionality. I'm fine with your suggested approach. I figured that tucking the recording/logging logic away in a helper class also had benefits but I'l
Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation
On 11/7/2018 8:53 PM, Weijun Wang wrote:
Oh, I didn't know that.
To make sure -keyalg matches KeyPairGenerator.getInstance(), I'd like to
support it. If I read the impl correctly, you don't need to initialize it
anymore and if you really want to initialize it the params must be the same.
Currently keytool always calls initialize(). In this case, there will be no
default -keysize, and initializa() will be not be called if user has not
specified one. If user provides -groupname or -keysize just use it and keytool
fails if API call fails.
This is correct. If you are using the "X25519" and "X448" algorithm
name, then there is no need to initialize with parameters, because they
are decided by the algorithm name. You can still initialize, and
different providers may be more permissive than others if you try to use
different parameters than what is specified by the algorithm name. For
example: KeyPairGenerator.getInstance("X448").initialize(255). SunEC is
very strict, and will not allow this sort of thing.
X25519/X448 keys can only be used for KeyAgreement, so they aren't
supported in keytool anyway, right? If this is the case, then you don't
need to worry about adding any code to keytool for them. Though I expect
EdDSA to work in a similar way (algorithm names "Ed25519" and "Ed448").
Still, I don't know if it is worthwhile to add special code for it. For
all algorithms, if no -keysize or -groupname is specified, then keytool
could skip the initialize on the KPG, and the implementation defaults
(if available) will be used. The hard part is deciding whether to emit a
warning in this case, and that will probably be algorithm-specific.
On Nov 8, 2018, at 8:01 AM, Xuelei Fan wrote:
On 11/7/2018 3:38 PM, Weijun Wang wrote:
This sounds a little misleading to me. Alg name and alg params are 2 different things.
This is like asking user to call KeyPairGenerator.getInstance("secp256r1").
Well, KeyPairGenerator.getInstance("x25519") is a case that JDK 11 has
supported now.
Otherwise, there is a need to check the conflict of alg name and group name.
This only works because "X25519" (and "X448") is both an algorithm name
and a parameter spec name. This makes sense for X25519/X448, but not for
all algorithms.
I don't think there is any need to check for algorithm/group conflicts
in keytool, because it is checked in the crypto provider already. All
keytool needs to do is pass down the algorithm name and group name (as a
NamedParameterSpec/ECGenParameterSpec) and see if it works. If we want
to support "secp256r1" in -keyalg, then we can accomplish that by adding
it as an algorithm name for KeyPairGenerator. Though I'm not sure this
is a good idea.
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
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
RFR 8213400: Support choosing curve name in keytool keypair generation
Please also review the code change at
https://cr.openjdk.java.net/~weijun/8213400/webrev.00/
Notes:
- CertAndKeyGen.java:
generate(String name):
+try {
+keyGen.initialize(new NamedParameterSpec(name), prng);
+} catch (InvalidAlgorithmParameterException e) {
+if (keyType.equalsIgnoreCase("EC")) {
+// EC has another NamedParameterSpec
+keyGen.initialize(new ECGenParameterSpec(name), prng);
+} else {
+throw e;
+}
+}
This is for future algorithms that accept -groupname. In fact, our own
ECKeyPairGenerator should have accepted NamedParameterSpec too.
generate (int keyBits) allows keyBits == -1. This is for future algorithms that
do not have a default -keysize.
- keytool/Main.java:
+private String ecGroupNameForSize(int size) throws Exception {
+AlgorithmParameters ap = AlgorithmParameters.getInstance("EC");
+ap.init(new ECKeySizeParameterSpec(size));
+// The following line assumes the toString value is "name (oid)"
+return ap.toString().split(" ")[0];
+}
Hopefully the ap.toString().split(" ")[0] return value is not too ugly, but the
toString() might contain alternative names.
- 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.
- NamedCurve.java:
A new field commonNames added, which is used by the new GroupName.java test.
Thanks
Max
