Re: [8u] request for review: 8062552 Support keystore type detection for JKS and PKCS12 keystores
Unfortunately we cannot modify a Java SE API in an update release so there is no opportunity to backport the keystore probe mechanism to JDK 8u. On 23 May 2015, at 22:57, Thomas Lußnig open...@suche.org wrote: On 23.05.2015 10:59, Vincent Ryan wrote: The aim of this enhancement is to address a specific compatibility risk for JKS and not to offer a general purpose mechanism for loading any keystore type. In general, the keystore type should match the keystore file format. In JDK 9 there is a new probe mechanism for keystores that is more similar to what you are proposing. The advantage of that mechanism is that the keystore type will exactly match the keystore file format. When there is already an new probe mechanism for keystore detetion, why do not backport/use it ? Why build this limited version for one single usecase instead of using the more gerneral solution ? On 23 May 2015, at 09:42, Thomas Lußnig open...@suche.org wrote: Hi, 1) Would it not be an good idea to check the first bytes of the message so that the dual class already know what type the stream is and there is no unnecessary instanciation of exceptions and engine class? 2) If we add an smart keystore why we limit it to two types? I do not see any reason why it should not be possible to add other store types to: - JCEKS - PKCS11 It could be extended via securit property java.security.smartKeystore.N.type = PKCS11 java.security.smartKeystore.N.magic = HexSequence (Optional for Performance) java.security.smartKeystore.N.engineClass = CanonicalEngine Class Name This would be only an small code change but an usefull improvement. Gruß Thomas On 22.05.2015 22:01, Sean Mullan wrote: Looks fine to me. --Sean On 05/22/2015 03:10 PM, Vincent Ryan wrote: Thanks Thomas and Sean for your review comments. KeyStoreDelegator matches the JDK 9 version. I’ve moved it to the sun.security.package and modified it as suggested. I also made JavaKeyStore package-private but DualFormatJKS needs to remain public. The cert in trusted.pem is an arbitrary X.509 cert and I’ve added a comment in the TestKeystoreCompat test. A new webrev is available at: http://cr.openjdk.java.net/~vinnie/8062552/webrev.02/
Re: [8u] request for review: 8062552 Support keystore type detection for JKS and PKCS12 keystores
On 23.05.2015 10:59, Vincent Ryan wrote: The aim of this enhancement is to address a specific compatibility risk for JKS and not to offer a general purpose mechanism for loading any keystore type. In general, the keystore type should match the keystore file format. In JDK 9 there is a new probe mechanism for keystores that is more similar to what you are proposing. The advantage of that mechanism is that the keystore type will exactly match the keystore file format. When there is already an new probe mechanism for keystore detetion, why do not backport/use it ? Why build this limited version for one single usecase instead of using the more gerneral solution ? On 23 May 2015, at 09:42, Thomas Lußnig open...@suche.org wrote: Hi, 1) Would it not be an good idea to check the first bytes of the message so that the dual class already know what type the stream is and there is no unnecessary instanciation of exceptions and engine class? 2) If we add an smart keystore why we limit it to two types? I do not see any reason why it should not be possible to add other store types to: - JCEKS - PKCS11 It could be extended via securit property java.security.smartKeystore.N.type = PKCS11 java.security.smartKeystore.N.magic = HexSequence (Optional for Performance) java.security.smartKeystore.N.engineClass = CanonicalEngine Class Name This would be only an small code change but an usefull improvement. Gruß Thomas On 22.05.2015 22:01, Sean Mullan wrote: Looks fine to me. --Sean On 05/22/2015 03:10 PM, Vincent Ryan wrote: Thanks Thomas and Sean for your review comments. KeyStoreDelegator matches the JDK 9 version. I’ve moved it to the sun.security.package and modified it as suggested. I also made JavaKeyStore package-private but DualFormatJKS needs to remain public. The cert in trusted.pem is an arbitrary X.509 cert and I’ve added a comment in the TestKeystoreCompat test. A new webrev is available at: http://cr.openjdk.java.net/~vinnie/8062552/webrev.02/
Re: [8u] request for review: 8062552 Support keystore type detection for JKS and PKCS12 keystores
Hi, 1) Would it not be an good idea to check the first bytes of the message so that the dual class already know what type the stream is and there is no unnecessary instanciation of exceptions and engine class? 2) If we add an smart keystore why we limit it to two types? I do not see any reason why it should not be possible to add other store types to: - JCEKS - PKCS11 It could be extended via securit property java.security.smartKeystore.N.type = PKCS11 java.security.smartKeystore.N.magic = HexSequence (Optional for Performance) java.security.smartKeystore.N.engineClass = CanonicalEngine Class Name This would be only an small code change but an usefull improvement. Gruß Thomas On 22.05.2015 22:01, Sean Mullan wrote: Looks fine to me. --Sean On 05/22/2015 03:10 PM, Vincent Ryan wrote: Thanks Thomas and Sean for your review comments. KeyStoreDelegator matches the JDK 9 version. I’ve moved it to the sun.security.package and modified it as suggested. I also made JavaKeyStore package-private but DualFormatJKS needs to remain public. The cert in trusted.pem is an arbitrary X.509 cert and I’ve added a comment in the TestKeystoreCompat test. A new webrev is available at: http://cr.openjdk.java.net/~vinnie/8062552/webrev.02/
Re: [8u] request for review: 8062552 Support keystore type detection for JKS and PKCS12 keystores
The aim of this enhancement is to address a specific compatibility risk for JKS and not to offer a general purpose mechanism for loading any keystore type. In general, the keystore type should match the keystore file format. In JDK 9 there is a new probe mechanism for keystores that is more similar to what you are proposing. The advantage of that mechanism is that the keystore type will exactly match the keystore file format. On 23 May 2015, at 09:42, Thomas Lußnig open...@suche.org wrote: Hi, 1) Would it not be an good idea to check the first bytes of the message so that the dual class already know what type the stream is and there is no unnecessary instanciation of exceptions and engine class? 2) If we add an smart keystore why we limit it to two types? I do not see any reason why it should not be possible to add other store types to: - JCEKS - PKCS11 It could be extended via securit property java.security.smartKeystore.N.type = PKCS11 java.security.smartKeystore.N.magic = HexSequence (Optional for Performance) java.security.smartKeystore.N.engineClass = CanonicalEngine Class Name This would be only an small code change but an usefull improvement. Gruß Thomas On 22.05.2015 22:01, Sean Mullan wrote: Looks fine to me. --Sean On 05/22/2015 03:10 PM, Vincent Ryan wrote: Thanks Thomas and Sean for your review comments. KeyStoreDelegator matches the JDK 9 version. I’ve moved it to the sun.security.package and modified it as suggested. I also made JavaKeyStore package-private but DualFormatJKS needs to remain public. The cert in trusted.pem is an arbitrary X.509 cert and I’ve added a comment in the TestKeystoreCompat test. A new webrev is available at: http://cr.openjdk.java.net/~vinnie/8062552/webrev.02/
Re: [8u] request for review: 8062552 Support keystore type detection for JKS and PKCS12 keystores
Looks fine to me. --Sean On 05/22/2015 03:10 PM, Vincent Ryan wrote: Thanks Thomas and Sean for your review comments. KeyStoreDelegator matches the JDK 9 version. I’ve moved it to the sun.security.package and modified it as suggested. I also made JavaKeyStore package-private but DualFormatJKS needs to remain public. The cert in trusted.pem is an arbitrary X.509 cert and I’ve added a comment in the TestKeystoreCompat test. A new webrev is available at: http://cr.openjdk.java.net/~vinnie/8062552/webrev.02/ On 22 May 2015, at 16:27, Sean Mullan sean.mul...@oracle.com mailto:sean.mul...@oracle.com wrote: Just a couple of other comments: - Both JavaKeyStore and JavaKeyStore$DualFormatJKS can be package-private since they are only referenced from SunEntries which is in the same package. I would also move KeyStoreDelegator into sun.security.provider and make it package-private. - Can you add a comment describing the contents of the trusted.pem cert in case we need to re-create it at some point in the future? - In KeyStoreDelegator, I think most/all of the fields can be made final. --Sean On 05/21/2015 11:44 AM, Vincent Ryan wrote: Please review this enhancement to JDK 8u that addresses a compatibility risk for certain applications that access keystores across JDK 8 and JDK 9 releases. The issue arises because the default keystore type is now PKCS12 in JDK 9 but is JKS in earlier releases. The problem can occur when a keystore is created on JDK 9 using the default keystore type but accessed on JDK 8 also using the default keystore type. This keystore type mismatch results in an error. The change introduces a keystore compatibility mode for JKS keystores where both JKS and PKCS12 file formats are understood. Similar behaviour is already present in JDK 9 (JEP-229). The keystore.type.compat security property controls whether the mode is enabled or not. By default it is enabled. This enhancement enables at risk applications to continue to function across JDK 8 and JDK 9 without requiring any application code changes. Thanks. Bug: https://bugs.openjdk.java.net/browse/JDK-8062552 Webrev: http://cr.openjdk.java.net/~vinnie/8062552/webrev.01/
Re: [8u] request for review: 8062552 Support keystore type detection for JKS and PKCS12 keystores
On 05/21/2015 06:02 PM, Thomas Lußnig wrote:
Hi,
most of it look ok for me, but in
http://cr.openjdk.java.net/~vinnie/8062552/webrev.01/src/share/classes/sun/security/util/KeyStoreDelegator.java.patch;
i found in the
method engineLoad the exception handling an bit ugly.
+} catch (Exception e) {
+
+// incorrect password
+if (e instanceof IOException
+e.getCause() instanceof UnrecoverableKeyException) {
+throw (IOException)e;
+}
+
+try {
+// Ignore secondary keystore when no compatibility mode
+if (!compatModeEnabled) {
+throw e;
+}
i would have expected:
+} catch (Exception e) {
+if (!compatModeEnabled) {
+throw e;
+}
+
+// incorrect password
+if (e instanceof IOException
+e.getCause() instanceof UnrecoverableKeyException) {
+throw (IOException)e;
+}
+
+try {
+// Ignore secondary keystore when no compatibility mode
because if no compatModeEnabled i would expect that the exception if
transparently thrown.
Alternatively, if there is no compatModeEnabled, there is no need to
try/catch so line 203 should probably be:
203 if (stream == null || !compatModeEnabled) {
and then you can remove the check for compatModeEnabled in the try/catch
block.
--Sean
And since JDK8 this is allowed tho throw an Exception if it was catched and can
only by an declared one.
Also this only compare an boolean instead of two instanceof + additional
exception handling.
Gruß Thomas
Re: [8u] request for review: 8062552 Support keystore type detection for JKS and PKCS12 keystores
Just a couple of other comments: - Both JavaKeyStore and JavaKeyStore$DualFormatJKS can be package-private since they are only referenced from SunEntries which is in the same package. I would also move KeyStoreDelegator into sun.security.provider and make it package-private. - Can you add a comment describing the contents of the trusted.pem cert in case we need to re-create it at some point in the future? - In KeyStoreDelegator, I think most/all of the fields can be made final. --Sean On 05/21/2015 11:44 AM, Vincent Ryan wrote: Please review this enhancement to JDK 8u that addresses a compatibility risk for certain applications that access keystores across JDK 8 and JDK 9 releases. The issue arises because the default keystore type is now PKCS12 in JDK 9 but is JKS in earlier releases. The problem can occur when a keystore is created on JDK 9 using the default keystore type but accessed on JDK 8 also using the default keystore type. This keystore type mismatch results in an error. The change introduces a keystore compatibility mode for JKS keystores where both JKS and PKCS12 file formats are understood. Similar behaviour is already present in JDK 9 (JEP-229). The keystore.type.compat security property controls whether the mode is enabled or not. By default it is enabled. This enhancement enables at risk applications to continue to function across JDK 8 and JDK 9 without requiring any application code changes. Thanks. Bug: https://bugs.openjdk.java.net/browse/JDK-8062552 Webrev: http://cr.openjdk.java.net/~vinnie/8062552/webrev.01/
Re: [8u] request for review: 8062552 Support keystore type detection for JKS and PKCS12 keystores
Thanks Thomas and Sean for your review comments. KeyStoreDelegator matches the JDK 9 version. I’ve moved it to the sun.security.package and modified it as suggested. I also made JavaKeyStore package-private but DualFormatJKS needs to remain public. The cert in trusted.pem is an arbitrary X.509 cert and I’ve added a comment in the TestKeystoreCompat test. A new webrev is available at: http://cr.openjdk.java.net/~vinnie/8062552/webrev.02/ http://cr.openjdk.java.net/~vinnie/8062552/webrev.02/ On 22 May 2015, at 16:27, Sean Mullan sean.mul...@oracle.com wrote: Just a couple of other comments: - Both JavaKeyStore and JavaKeyStore$DualFormatJKS can be package-private since they are only referenced from SunEntries which is in the same package. I would also move KeyStoreDelegator into sun.security.provider and make it package-private. - Can you add a comment describing the contents of the trusted.pem cert in case we need to re-create it at some point in the future? - In KeyStoreDelegator, I think most/all of the fields can be made final. --Sean On 05/21/2015 11:44 AM, Vincent Ryan wrote: Please review this enhancement to JDK 8u that addresses a compatibility risk for certain applications that access keystores across JDK 8 and JDK 9 releases. The issue arises because the default keystore type is now PKCS12 in JDK 9 but is JKS in earlier releases. The problem can occur when a keystore is created on JDK 9 using the default keystore type but accessed on JDK 8 also using the default keystore type. This keystore type mismatch results in an error. The change introduces a keystore compatibility mode for JKS keystores where both JKS and PKCS12 file formats are understood. Similar behaviour is already present in JDK 9 (JEP-229). The keystore.type.compat security property controls whether the mode is enabled or not. By default it is enabled. This enhancement enables at risk applications to continue to function across JDK 8 and JDK 9 without requiring any application code changes. Thanks. Bug: https://bugs.openjdk.java.net/browse/JDK-8062552 Webrev: http://cr.openjdk.java.net/~vinnie/8062552/webrev.01/
Re: [8u] request for review: 8062552 Support keystore type detection for JKS and PKCS12 keystores
Hi,
most of it look ok for me, but in
http://cr.openjdk.java.net/~vinnie/8062552/webrev.01/src/share/classes/sun/security/util/KeyStoreDelegator.java.patch;
i found in the
method engineLoad the exception handling an bit ugly.
+} catch (Exception e) {
+
+// incorrect password
+if (e instanceof IOException
+e.getCause() instanceof UnrecoverableKeyException) {
+throw (IOException)e;
+}
+
+try {
+// Ignore secondary keystore when no compatibility mode
+if (!compatModeEnabled) {
+throw e;
+}
i would have expected:
+} catch (Exception e) {
+if (!compatModeEnabled) {
+throw e;
+}
+
+// incorrect password
+if (e instanceof IOException
+e.getCause() instanceof UnrecoverableKeyException) {
+throw (IOException)e;
+}
+
+try {
+// Ignore secondary keystore when no compatibility mode
because if no compatModeEnabled i would expect that the exception if
transparently thrown.
And since JDK8 this is allowed tho throw an Exception if it was catched and can
only by an declared one.
Also this only compare an boolean instead of two instanceof + additional
exception handling.
Gruß Thomas
[8u] request for review: 8062552 Support keystore type detection for JKS and PKCS12 keystores
Please review this enhancement to JDK 8u that addresses a compatibility risk for certain applications that access keystores across JDK 8 and JDK 9 releases. The issue arises because the default keystore type is now PKCS12 in JDK 9 but is JKS in earlier releases. The problem can occur when a keystore is created on JDK 9 using the default keystore type but accessed on JDK 8 also using the default keystore type. This keystore type mismatch results in an error. The change introduces a keystore compatibility mode for JKS keystores where both JKS and PKCS12 file formats are understood. Similar behaviour is already present in JDK 9 (JEP-229). The keystore.type.compat security property controls whether the mode is enabled or not. By default it is enabled. This enhancement enables at risk applications to continue to function across JDK 8 and JDK 9 without requiring any application code changes. Thanks. Bug: https://bugs.openjdk.java.net/browse/JDK-8062552 Webrev: http://cr.openjdk.java.net/~vinnie/8062552/webrev.01/
