Re: Request for review : backport of bug# 8031046 to 7u-dev
HI Max, Thanks for the review. Yes it means no extra code changes. :) rgds mala On 8/6/2014 6:53 AM, Weijun Wang wrote: Hi Mala Code change looks fine. When you say "Direct backport", is it equivalent to "applying the same patch with no conflict"? :-) Thanks Max On 8/5/2014 17:32, mala bankal wrote: HI, Request review for the direct backport of bug# 8031046 to 7u-dev. http://cr.openjdk.java.net/~mbankal/8031046/webrev.00/ JDK9 Changeset : http://hg.openjdk.java.net/jdk9/dev/jdk/rev/ab914c760352 JDK8 Changeset : http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/b32e9aba4888 Regression test is not added since it requires special setup, SQE bug filed for the same : https://bugs.openjdk.java.net/browse/INTJDK-7604925 Thanks. rgds mala
Re: Request for review : backport of bug# 8031046 to 7u-dev
Hi Mala Code change looks fine. When you say "Direct backport", is it equivalent to "applying the same patch with no conflict"? :-) Thanks Max On 8/5/2014 17:32, mala bankal wrote: HI, Request review for the direct backport of bug# 8031046 to 7u-dev. http://cr.openjdk.java.net/~mbankal/8031046/webrev.00/ JDK9 Changeset : http://hg.openjdk.java.net/jdk9/dev/jdk/rev/ab914c760352 JDK8 Changeset : http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/b32e9aba4888 Regression test is not added since it requires special setup, SQE bug filed for the same : https://bugs.openjdk.java.net/browse/INTJDK-7604925 Thanks. rgds mala
Re: [9] RFR: 8007706: X.509 cert extension SAN should support "_" in dNSName
Thanks, Florian. I will withdraw my review request and close this issue. I'll file a separate bug to allow the first character to be a digit, as RFC 1123 relaxed that restriction. Thanks, Jason On 08/04/2014 11:58 PM, Florian Weimer wrote: On 08/05/2014 07:52 AM, Jason Uh wrote: Hi Florian, I've reviewed the RFC again and think there might be some misinterpretation. The only part I see about underscores reads: Implementers should note that the at sign ('@') and underscore ('_') characters are not supported by the ASN.1 type PrintableString. These characters often appear in Internet addresses. Such addresses MUST be encoded using an ASN.1 type that supports them. They are usually encoded as IA5String in either the emailAddress attribute within a distinguished name or the rfc822Name field of GeneralName. Conforming implementations MUST NOT encode strings that include either the at sign or underscore character as PrintableString. RFC 5280 doesn't allow underscores for *PrintableString*, but DNSName is an *IA5String*, which does support them. By this argument, the patch is still not correct because it leaves in additional checking incompatible with IA5String. (It is also not clear to me what exactly is permissible in IA5Strings and how codepoints are supposedly mapped to their Unicode counterparts if a national variant of T.50 is used, but that's a different issue.) Relaxing all restrictions would match what other software does. My claim that '_' is not allowed in dNSName is based on these two sentences: When the subjectAltName extension contains a domain name system label, the domain name MUST be stored in the dNSName (an IA5String). The name MUST be in the "preferred name syntax", as specified by Section 3.5 of [RFC1034] and as modified by Section 2.1 of [RFC1123]. Section 3.5 of RFC 1034 and section 2.1 of RFC 1123 deal with host name syntax, and the grammar in RFC 1034 (and RFC 952, which is referenced in RFC 1123) does not permit underscores.
Re: RFR 8043836: Need new tests for AES cipher
Looks fine to me. Thanks, Valerie On 7/28/2014 1:57 AM, FELIX YANG wrote: May I request you to review these 6 new tests to be added for AES cipher. New tests are added to address following: - Test AES for different modes and padding schemes - Test AES encryption with no padding - same buffer can be used for encrypt and decrypt with AES JDK Issue: https://bugs.openjdk.java.net/browse/JDK-8043836 Webrev: http://cr.openjdk.java.net/~rhalade/8043836/webrev.00/ Thanks,
Re: RFR 6997010: Consolidate java.security files into one file with modifications
On 07/28/2014 09:53 AM, Wang Weijun wrote: Yes, you are right. Webrev updated at http://cr.openjdk.java.net/~weijun/6997010/webrev.02. GendataJavaSecurity.gmk and MakeJavaSecurity.java updated. There's an unnecessary indent at line 1 of GendataJavaSecurity.gmk In CheckSecurityProvider.java, ucrypto is in the closed sources, so Security.getProviders() will not return it if you are testing an OpenJDK build. You need to adjust the test to not include this provider if testing an OpenJDK build. --Sean Thanks Max On Jul 28, 2014, at 19:43, Erik Joelsson wrote: Hello Max, Shouldn't the rule for $(GENDATA_JAVA_SECURITY) depend on $(RESTRICTED_PKGS_SRC) so that updates to the pkgs file triggers a rebuild? For that to work, the variable $(RESTRICTED_PKGS_SRC) needs to be empty for the OPENJDK case rather than have a dummy name and MakeJavaSecurity.java needs to handle missing the last argument. /Erik On 2014-07-28 03:44, Wang Weijun wrote: Webrev updated at http://cr.openjdk.java.net/~weijun/6997010/webrev.01/ New test CheckSecurityProvider.java, and updates to MakeJavaSecurity.addPackages(). Thanks Max On Jul 25, 2014, at 22:44, Wang Weijun wrote: On Jul 25, 2014, at 22:30, Sean Mullan wrote: http://cr.openjdk.java.net/~weijun/6997010/webrev.00/ 4. *IMPORTANT*: In order to easily maintain platform-related entries, every line (including the last line) in package.access and package.definition MUST end with ',\' now. A blank line MUST exist after the last line. This avoid ugly lines like #ifndef windows entry1. #endif #ifdef windows entry1.,\ entry2 #endif What happens if someone (inevitably) adds a new package to the list and forgets to do either of these? Does it result in a build failure? No build failure, but test/java/security/SecurityManager/CheckPackageAccess.java will fail. I can add check in the build tool. Otherwise looks good, although I think it would be useful to write an additional test to make sure the correct providers are installed and ordered correctly on the different platforms, something similar to the java/lang/SecurityManager/CheckPackageAccess.java test but specific to providers. OK. Thanks Max
Re: apple.security.KeychainStore does not load private key (when called from javaws)
On 08/04/2014 11:10 AM, Florian Bruckner (3kraft) wrote: Hey guys, any feedback/comments on this? This seems like a reasonable change to me. In order to proceed with accepting your patch, you will first need to sign an OCA. See step 0 of http://openjdk.java.net/contribute/ Thanks, Sean Just to summarize again: KeychainStore does not load private keys when not called with a passphrase. This is the case in various deployment scenarios (like javaws), as a consequence identity certificates stored in Apple Keychain are not available (i.e. being offered for selection like on Windows). The reason for this is the implementation of KeychainStore, which uses this API to retrieve a PKCS#12 container from apple keychain: https://developer.apple.com/library/mac/documentation/security/Reference/keychainservices/Reference/reference.html#jumpTo_63 The API documentation says the PKCS#12 keystore being generated with this call requires a password. This is either supplied by the caller or the user is prompted (if a flag is set, which is not the case). KeychainStore then goes on to extract the private key from the returned PKCS#12 store and decrypts it with the password. Therefore, the password passed into engineGetKey is actually used to encrypt and decrypt only in the scope of this method. Therefore, the approach is to create a dummy password for this use case. Please consider these patches to fix this issue: For jdk7u-dev: diff -r 35aabd00a534 src/macosx/classes/apple/security/KeychainStore.java --- a/src/macosx/classes/apple/security/KeychainStore.javaTue Jul 15 02:26:55 2014 +0400 +++ b/src/macosx/classes/apple/security/KeychainStore.javaTue Jul 15 10:52:44 2014 +0200 @@ -134,7 +134,7 @@ * password to recover it. * * @param alias the alias name - * @param password the password for recovering the key + * @param password the password for recovering the key. This password is not used to access the private key in KeyChain, it is used internally only. * * @return the requested key, or null if the given alias does not exist * or does not identify a key entry. @@ -148,6 +148,16 @@ throws NoSuchAlgorithmException, UnrecoverableKeyException { permissionCheck(); +// An empty password is rejected by MacOS API, no private key data +// is exported. If no password is passed (as is the case when +// this implementation is used as browser keystore in various +// deployment scenarios like webstart, JFX and applets), create +// a dummy password to MacOS API is happy. +if (password == null || password.length ==0) { +// Must not be a char array with only a 0, as this is an empty +// string. Therefore use a single character. +password = new char[]{'A'} +} Object entry = entries.get(alias.toLowerCase()); For jdk8u-dev: diff -r baec3649f6c0 src/macosx/classes/apple/security/KeychainStore.java --- a/src/macosx/classes/apple/security/KeychainStore.javaTue Jul 15 02:00:52 2014 +0400 +++ b/src/macosx/classes/apple/security/KeychainStore.javaTue Jul 15 10:54:45 2014 +0200 @@ -140,7 +140,7 @@ * password to recover it. * * @param alias the alias name - * @param password the password for recovering the key + * @param password the password for recovering the key. This password is not used to access the private key in KeyChain, it is used internally only. * * @return the requested key, or null if the given alias does not exist * or does not identify a key entry. @@ -154,7 +154,16 @@ throws NoSuchAlgorithmException, UnrecoverableKeyException { permissionCheck(); - +// An empty password is rejected by MacOS API, no private key data +// is exported. If no password is passed (as is the case when +// this implementation is used as browser keystore in various +// deployment scenarios like webstart, JFX and applets), create +// a dummy password to MacOS API is happy. +if (password == null || password.length ==0) { +// Must not be a char array with only a 0, as this is an empty +// string. Therefore use a single character. +password = new char[]{'A'} +} Object entry = entries.get(alias.toLowerCase()); if (entry == null || !(entry instanceof KeyEntry)) { With best regards, Florian
Re: Review request for CR 8044193 Need to add known answer tests for AES cipher
The tests look fine. However, can you please update the test policy files with fine-grained permissions for SunJCE provider? Please refer to the current /lib/security/java.policy. Thanks, Valerie On 7/28/2014 9:36 PM, zaiyao liu wrote: Hello, Please help to review the tests for AES cipher. This tests test AES ciphers with different modes and padding schemes when provider change,are part of tests for bug 8044193(Open part) Bug - https://bugs.openjdk.java.net/browse/JDK-8044193 webrev- http://cr.openjdk.java.net/~rhalade/8044193/webrev.00/ Thanks Kevin Liu
Request for review : backport of bug# 8031046 to 7u-dev
HI, Request review for the direct backport of bug# 8031046 to 7u-dev. http://cr.openjdk.java.net/~mbankal/8031046/webrev.00/ JDK9 Changeset : http://hg.openjdk.java.net/jdk9/dev/jdk/rev/ab914c760352 JDK8 Changeset : http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/b32e9aba4888 Regression test is not added since it requires special setup, SQE bug filed for the same : https://bugs.openjdk.java.net/browse/INTJDK-7604925 Thanks. rgds mala