Re: RFR 8193255: Root Certificates should be stored in text format and assembled at build time
One suggestion is to put a printable form of the contents of the certificate at the top of each of the PEM files. It would be nice as a quick-look to see what is in the certificate. Of course, you can also use keytool -printcert to do that, but if I am just perusing the source code via a browser or something like that, it would be nice to not have to do that. --Sean On 5/30/19 9:01 AM, Weijun Wang wrote: Please take a review at http://cr.openjdk.java.net/~weijun/8193255/webrev.00/ Please pay attention to the 1st 3 and the last 2 files. Others are PEM files for all certs inside the original cacerts. There is one thing I cannot get correct. If I update the GenerateCacerts.java file and rerun make, the cacerts file is unchanged. I thought the following line $(GENDATA_CACERTS): $(BUILD_TOOLS) $(GENDATA_CACERTS_SRC) means when when the tool is changed, GENDATA_CACERTS will be called. Thanks, Max
Re: RFR 8193255: Root Certificates should be stored in text format and assembled at build time
On 5/30/19 8:49 PM, Weijun Wang wrote: Sure. How many info do you want to see? I can prepend `keytool -printcert` but that's too much. At least I think the extensions part is not needed. Also, I don't wish people reading the fingerprint inside as genuine and does not calculate it from the cert itself. So, I'm thinking of Owner: CN=XRamp Global Certification Authority, O=XRamp Security Services Inc, OU=www.xrampsecurity.com, C=US Issuer: CN=XRamp Global Certification Authority, O=XRamp Security Services Inc, OU=www.xrampsecurity.com, C=US Serial number: 50946cec18ead59c4dd597ef758fa0ad Valid from: 1 Nov 2004 17:14:04 GMT until: 1 Jan 2035 05:37:19 GMT Signature algorithm name: SHA1withRSA Subject Public Key Algorithm: 2048-bit RSA key Version: 3 Is that OK? This is good. Did you use keytool to emit those fields? It might make sense to add a brief README in this directory with instructions or a code snippet so that the next time we add a cert we know what to include at the top for consistency. Thanks, Sean Thanks, Max p.s. `keytool -printcert` shows validity in local timezone. Does not look good to me. On May 31, 2019, at 6:51 AM, Sean Mullan wrote: One suggestion is to put a printable form of the contents of the certificate at the top of each of the PEM files. It would be nice as a quick-look to see what is in the certificate. Of course, you can also use keytool -printcert to do that, but if I am just perusing the source code via a browser or something like that, it would be nice to not have to do that. --Sean On 5/30/19 9:01 AM, Weijun Wang wrote: Please take a review at http://cr.openjdk.java.net/~weijun/8193255/webrev.00/ Please pay attention to the 1st 3 and the last 2 files. Others are PEM files for all certs inside the original cacerts. There is one thing I cannot get correct. If I update the GenerateCacerts.java file and rerun make, the cacerts file is unchanged. I thought the following line $(GENDATA_CACERTS): $(BUILD_TOOLS) $(GENDATA_CACERTS_SRC) means when when the tool is changed, GENDATA_CACERTS will be called. Thanks, Max
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
Using the certificate's notBefore date as the KeyStore entry creation date is misleading since many of these root certs were not integrated into the JDK until after they were created by the CA. Can we somehow extract the last revision time of each PEM file instead? That is more aligned with the previous creation date that we used. --Sean On 6/12/19 12:38 PM, Erik Joelsson wrote: Hello Max, Much appreciated! I will need to have this fixed one way or other in JDK 13, so depending on if you get your fix there in time, I will retract my proposal. If your fix only hits 14, I will push mine to 13. /Erik On 2019-06-12 08:41, Weijun Wang wrote: This is my version of the fix: http://cr.openjdk.java.net/~weijun/8225392/webrev.00/ Now you can still compare cacerts bit by bit. Thanks, Max On Jun 12, 2019, at 10:50 PM, Weijun Wang wrote: Hi Erik, Are you going to fix this bug soon? I am inspired by Martin's words and would like to update GenerateCacerts.java so that as long as the certs and their aliases are unchanged, the output cacerts will always be the same. I can send out a code review today. Thanks, Max On Jun 12, 2019, at 10:59 AM, Weijun Wang wrote: Good idea about the creation time. --Max On Jun 12, 2019, at 10:53 AM, Martin Buchholz wrote: Google culture really likes build output determinism, and we recently built our own cacerts generator. To get determinism, we are using cert digest as alias (must have a unique alias, but value doesn't seem to matter much), and using cert notBefore instead of current (build) timestamp. On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson wrote: Since JDK-8193255, when we started generating the cacerts file in the build, the build compare baseline builds have started failing. It seems the cacerts binary file has some non determinism built in so it doesn't get generated exactly the same given the same input. This patch adds special handling when comparing that file by comparing the output of "keytool -list" on the files instead. Bug: https://bugs.openjdk.java.net/browse/JDK-8225392 Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/ /Erik
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
On 6/12/19 4:01 PM, Erik Joelsson wrote: Hello, We cannot rely on querying mercurial at build time. The source must be buildable from a source distribution. I had a feeling it wouldn't work but thought I would ask anyway. Well, offhand I can't think of any better solution than notBefore then, unless we included it as a comment in the PEM file, and then extracted it. With notBefore, someone might be curious about why some keystore entries were created so long ago (ex: the earliest notBefore date is 1996). But the creationDate doesn't really have any usefulness for cacerts, so it's probably ok. --Sean /Erik On 2019-06-12 11:39, Sean Mullan wrote: Using the certificate's notBefore date as the KeyStore entry creation date is misleading since many of these root certs were not integrated into the JDK until after they were created by the CA. Can we somehow extract the last revision time of each PEM file instead? That is more aligned with the previous creation date that we used. --Sean On 6/12/19 12:38 PM, Erik Joelsson wrote: Hello Max, Much appreciated! I will need to have this fixed one way or other in JDK 13, so depending on if you get your fix there in time, I will retract my proposal. If your fix only hits 14, I will push mine to 13. /Erik On 2019-06-12 08:41, Weijun Wang wrote: This is my version of the fix: http://cr.openjdk.java.net/~weijun/8225392/webrev.00/ Now you can still compare cacerts bit by bit. Thanks, Max On Jun 12, 2019, at 10:50 PM, Weijun Wang wrote: Hi Erik, Are you going to fix this bug soon? I am inspired by Martin's words and would like to update GenerateCacerts.java so that as long as the certs and their aliases are unchanged, the output cacerts will always be the same. I can send out a code review today. Thanks, Max On Jun 12, 2019, at 10:59 AM, Weijun Wang wrote: Good idea about the creation time. --Max On Jun 12, 2019, at 10:53 AM, Martin Buchholz wrote: Google culture really likes build output determinism, and we recently built our own cacerts generator. To get determinism, we are using cert digest as alias (must have a unique alias, but value doesn't seem to matter much), and using cert notBefore instead of current (build) timestamp. On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson wrote: Since JDK-8193255, when we started generating the cacerts file in the build, the build compare baseline builds have started failing. It seems the cacerts binary file has some non determinism built in so it doesn't get generated exactly the same given the same input. This patch adds special handling when comparing that file by comparing the output of "keytool -list" on the files instead. Bug: https://bugs.openjdk.java.net/browse/JDK-8225392 Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/ /Erik
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
On 6/12/19 9:52 PM, Weijun Wang wrote: Hi Sean, The previous fix for JDK-8193255 already made the creation date useless. Before that, each time cacerts was regenerated the date changed. I compared cacerts of different releases and the same cert have difference creation dates. The only other solution I can think of is to look at make/autoconf/version-numbers and use the DEFAULT_VERSION_DATE=2019-09-17 there. Yes, a possibility, use the GA release date, which maybe "makes more sense" as a creation date, although a bit odd to have creation dates in the future before GA. I'll leave it up to you. I think nobody really looks at the creationDate. Have you reviewed the code changes? You can see I stored the hash of the whole file into the test. This means anyone updating the CA certs will have to create cacerts and calculate the correct hash and update this test. I suppose this won't be a hassle. Not really, since you had to update VerifyCACerts anyway whenever a change to cacerts was made, but what's the benefit of the hash? Are you worried the cacerts binary will be corrupted somehow, or you just want extra assurance something didn't go wrong? It might be useful to run older versions of keytool against the cacerts binary you created - this would give you more assurance that your hand-coded format is correct. I would also try various commands, etc. --Sean Thanks, Max On Jun 13, 2019, at 4:15 AM, Sean Mullan wrote: On 6/12/19 4:01 PM, Erik Joelsson wrote: Hello, We cannot rely on querying mercurial at build time. The source must be buildable from a source distribution. I had a feeling it wouldn't work but thought I would ask anyway. Well, offhand I can't think of any better solution than notBefore then, unless we included it as a comment in the PEM file, and then extracted it. With notBefore, someone might be curious about why some keystore entries were created so long ago (ex: the earliest notBefore date is 1996). But the creationDate doesn't really have any usefulness for cacerts, so it's probably ok. --Sean /Erik On 2019-06-12 11:39, Sean Mullan wrote: Using the certificate's notBefore date as the KeyStore entry creation date is misleading since many of these root certs were not integrated into the JDK until after they were created by the CA. Can we somehow extract the last revision time of each PEM file instead? That is more aligned with the previous creation date that we used. --Sean On 6/12/19 12:38 PM, Erik Joelsson wrote: Hello Max, Much appreciated! I will need to have this fixed one way or other in JDK 13, so depending on if you get your fix there in time, I will retract my proposal. If your fix only hits 14, I will push mine to 13. /Erik On 2019-06-12 08:41, Weijun Wang wrote: This is my version of the fix: http://cr.openjdk.java.net/~weijun/8225392/webrev.00/ Now you can still compare cacerts bit by bit. Thanks, Max On Jun 12, 2019, at 10:50 PM, Weijun Wang wrote: Hi Erik, Are you going to fix this bug soon? I am inspired by Martin's words and would like to update GenerateCacerts.java so that as long as the certs and their aliases are unchanged, the output cacerts will always be the same. I can send out a code review today. Thanks, Max On Jun 12, 2019, at 10:59 AM, Weijun Wang wrote: Good idea about the creation time. --Max On Jun 12, 2019, at 10:53 AM, Martin Buchholz wrote: Google culture really likes build output determinism, and we recently built our own cacerts generator. To get determinism, we are using cert digest as alias (must have a unique alias, but value doesn't seem to matter much), and using cert notBefore instead of current (build) timestamp. On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson wrote: Since JDK-8193255, when we started generating the cacerts file in the build, the build compare baseline builds have started failing. It seems the cacerts binary file has some non determinism built in so it doesn't get generated exactly the same given the same input. This patch adds special handling when comparing that file by comparing the output of "keytool -list" on the files instead. Bug: https://bugs.openjdk.java.net/browse/JDK-8225392 Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/ /Erik
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
On 6/14/19 1:49 AM, Weijun Wang wrote: BTW, something not related but similar: Do you like me to also sort aliases alphabetically in the output of "keytool -list"? Yes, I think that is useful. --Sean
Re: [13] RFR: JDK-8225392: Comparison builds are failing due to cacerts file
On 6/14/19 11:33 AM, Weijun Wang wrote: Here is the updated webrev http://cr.openjdk.java.net/~weijun/8225392/webrev.01/ The only change is ordering in 'keytool -list' and its test. Looks fine. --Sean Thanks, Max On Jun 14, 2019, at 7:55 PM, Sean Mullan wrote: On 6/14/19 1:49 AM, Weijun Wang wrote: BTW, something not related but similar: Do you like me to also sort aliases alphabetically in the output of "keytool -list"? Yes, I think that is useful. --Sean
Re: [9] RFR 8086002: Move apple.security.AppleProvider to a proper module
Couple of minor comments on ProviderConfig.java 183: you can use the diamond operator for anonymous classes now (PrivilegedAction<>). You could also use a lambda expression here but I'll leave that up to you. 193: the braces around "if (debug != null)" are not indented properly Looks fine otherwise. --Sean On 08/13/2015 10:31 PM, Valerie Peng wrote: Can someone please help reviewing this change? This is to move Apple provider from jdk.deploy.osx module to java.base module. The native library for Apple provider is separated out from the "osx" one generated in jdk.deploy.osx module and named "osxapple" (sort of following the convention of SunMSCAPI provider whose native library is named "sunmscapi"). webrev: http://cr.openjdk.java.net/~valeriep/8086002/webrev.00/ Thanks, Valerie
Re: RFR 8141690: JDK-8133151 change to MakeJavaSecurity.java is not complete
The fix looks fine to me. For testing, can you create a test that uses a custom java.security file with "#ifndef solaris-sparc" in it, and check whether the property is used or not depending on what system is being tested? --Sean On 11/24/2015 06:10 AM, Magnus Ihse Bursie wrote: On 2015-11-20 02:12, Wang Weijun wrote: Ding dong. Even though this code resides in the make directory, it is not really build code, so I cannot comment on it. /Magnus On Nov 9, 2015, at 3:35 PM, Wang Weijun wrote: Hi All Please review the code change at http://cr.openjdk.java.net/~weijun/8141690/webrev.00/ JDK-8133151 added support for "#ifdef solaris-sparc" but not "#ifndef solaris-spare". This is fixed and I also added "#else". How do I test the change? Thanks Max
Re: RFR 8141690: JDK-8133151 change to MakeJavaSecurity.java is not complete
Looks good, although the location of the new test (test/sun/security) doesn't seem right. A new directory named test/conf/security would probably make the most sense, but I think putting it in the new test/jdk/security directory is also a better option. --Sean On 11/25/2015 09:36 PM, Wang Weijun wrote: On Nov 25, 2015, at 11:04 PM, Sean Mullan wrote: The fix looks fine to me. For testing, can you create a test that uses a custom java.security file with "#ifndef solaris-sparc" in it, and check whether the property is used or not depending on what system is being tested? The tool is inside another repo. I can still write one though. Please take a look at the updated webrev: http://cr.openjdk.java.net/~weijun/8141690/webrev.01/ I also fixed a bug inside MakeJavaSecurity.java. If package.access or package.definition happens to be the last property, an NPE will be thrown. Thanks Max --Sean http://cr.openjdk.java.net/~weijun/8141690/webrev.00/
Re: RFR: 8159752: Grant de-privileged module permissions by default with java.security.policy override option
Adding build-dev for review since there is one change to a Makefile in
the webrev below.
Thanks,
Sean
On 07/14/2016 04:05 PM, Sean Mullan wrote:
Please review this change to the default Policy provider implementation
to grant de-privileged module permissions by default even when the
java.security.policy override option is specified or when the
Policy.getInstance API is used:
http://cr.openjdk.java.net/~mullan/webrevs/8159752/webrev.00/
A new system-wide policy file located in
${java.home}/lib/security/default.policy has been created. It contains
grant statements containing the permissions that need to be granted to
de-privileged modules. These grant statements were previously located in
the ${java.home}/conf/security/java.policy file and have been relocated
to the default.policy file.
The default.policy file is now always loaded by the default Policy
provider implementation (sun/security/provider/PolicyFile). It is loaded
if the java.security.policy '=' or '==' option is specified, and also if
the application uses the Policy.getInstance methods and specifies the
"JavaPolicy" type. If the default.policy file cannot be loaded, an
InternalError is thrown, on the basis that the runtime cannot operate
correctly unless these permissions are granted.
The rationale for making this change is that the runtime should be
responsible for granting the permissions it needs to operate correctly.
We should not expect users to have to determine or copy and paste these
permissions into their own policy files.
Thanks,
Sean
Re: RFR 8162739: Create new keytool option to access cacerts file
- src/java.base/share/classes/sun/security/tools/keytool/Resources.java 131 "operates on the cacerts keystore"}, // -cacerts "operates" sounds a little odd. How about "access the cacerts keystore" (so it is consistent with the warning below). 133 "Warning: use -cacerts option to access cacerts keystore"}, Warning: use the -cacerts option to access the cacerts keystore --Sean On 08/08/2016 09:46 AM, Weijun Wang wrote: Please review the code changes at http://cr.openjdk.java.net/~weijun/8162739/webrev.00/ A new -cacerts option is added to keytool so there is no need to provide the full cacerts path on the command line. lib/security/cacerts is considered an implementation detail and subject to change. The conf/security/cacerts file in src/ is moved to lib/security/cacerts, which matches its destination path. A small change in make/ is needed. Thanks Max
RFR: 8164071: Default.policy file missing content for solaris
Please review this simple fix to append the solaris default.policy file to the overall default.policy file. Bug: https://bugs.openjdk.java.net/browse/JDK-8164071 diff -r 551f7617b2c0 make/copy/Copy-java.base.gmk --- a/make/copy/Copy-java.base.gmk Wed Aug 17 10:08:18 2016 +0800 +++ b/make/copy/Copy-java.base.gmk Wed Aug 17 12:17:19 2016 -0400 @@ -185,6 +185,8 @@ ifeq ($(OPENJDK_TARGET_OS), windows) DEF_POLICY_SRC_LIST += $(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy +else ifeq ($(OPENJDK_TARGET_OS), solaris) + DEF_POLICY_SRC_LIST += $(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy endif # Allow imported modules to modify the java.policy Thanks, Sean
Re: RFR: 8164071: Default.policy file missing content for solaris
On 8/17/2016 12:33 PM, Erik Joelsson wrote: Hello Sean, The change looks ok, but it could also be expressed like this to avoid duplication: ifneq ($(filter $(OPENJDK_TARGET_OS), windows solaris), ) DEF_POLICY_SRC_LIST += $(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy endif Thanks. However, shouldn't that be "ifeq" and not "ifneq"? --Sean /Erik On 2016-08-17 18:18, Sean Mullan wrote: Please review this simple fix to append the solaris default.policy file to the overall default.policy file. Bug: https://bugs.openjdk.java.net/browse/JDK-8164071 diff -r 551f7617b2c0 make/copy/Copy-java.base.gmk --- a/make/copy/Copy-java.base.gmkWed Aug 17 10:08:18 2016 +0800 +++ b/make/copy/Copy-java.base.gmkWed Aug 17 12:17:19 2016 -0400 @@ -185,6 +185,8 @@ ifeq ($(OPENJDK_TARGET_OS), windows) DEF_POLICY_SRC_LIST += $(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy +else ifeq ($(OPENJDK_TARGET_OS), solaris) + DEF_POLICY_SRC_LIST += $(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy endif # Allow imported modules to modify the java.policy Thanks, Sean
Re: RFR: JDK-8178278 Move Standard Algorithm Names document to specs directory
General: change the text of all of the links from "Java Cryptography Architecture Standard Algorithm Name Documentation" (the old name) to "Java Security Standard Algorithm Names Specification". Looks good otherwise. Thanks, Sean On 5/5/17 9:17 AM, Magnus Ihse Bursie wrote: The Security Standard Names document will be moved to a new location, so all links needs to be updated. Also, a minor fix to the build system was needed. Bug: https://bugs.openjdk.java.net/browse/JDK-8178278 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8178278-standard-names-spec-as-markdown/webrev.01 /Magnus
Re: some config files out of conf directory
On 8/4/17 11:12 AM, Alan Bateman wrote: On 04/08/2017 07:59, Jiri Vanek wrote: Hello! I'm packaging openjdk9 for Fedora, and following files: jdk/lib/security/blacklisted.certs jdk/lib/security/default.policy Seems to be config files. Still, they are in lib/security, whether all other config files were (finally! Thank you!) moved to jdk/conf/ and its subdirectories. Is it intentional? Why so? Are there plans to change it? cc'ing security-dev in case there is more needed on this but in summary, these are not intended to be edited so this is why they are in the `lib` rather than `conf` directory. Yes, Alan is correct. The default.policy file contains the permissions granted by default to the modules included in the JDK (that are loaded by the platform loader) and blacklisted.certs contains a system-wide list of certificates that are distrusted by the CertPath implementation in the JDK. They are not configuration files, they are generally meant to be files that typically never need to be modified. Thanks, Sean
Re: CSR Review for 8159544: Remove deprecated classes in com.sun.security.auth.**
Thanks, can you also review the corresponding webrev: http://cr.openjdk.java.net/~mullan/webrevs/8159544/ I'm also copying build-dev as there is one Makefile change in jdk.webrev.00. --Sean On 8/10/17 9:38 PM, Weijun Wang wrote: Done. Thanks Max On Aug 10, 2017, at 11:36 PM, Sean Mullan wrote: On 8/9/17 8:02 PM, Weijun Wang wrote: Looks fine. Should the specification part be formatted with and fixed fonts? Fixed. Can you add your name as Reviewer (you need to edit the CSR and add your name to the "Reviewed By" box). Thanks, Sean Thanks Max On Aug 10, 2017, at 3:15 AM, Sean Mullan wrote: Max, Could you please review the following CSR: https://bugs.openjdk.java.net/browse/JDK-8186047 Thanks, Sean
RFR: JDK-8159544: Remove deprecated classes in com.sun.security.auth.**
Please review this JDK 10 change to remove the deprecated classes in com.sun.security.auth.** that have been previously marked with forRemoval=true in JDK 9. webrev: http://cr.openjdk.java.net/~mullan/webrevs/8159544/ I have also copied Jan for reviewing a change in langtools, and also build-dev for a change to one of the JDK Makefiles. Thanks, Sean
Re: RFR: JDK-8159544: Remove deprecated classes in com.sun.security.auth.**
On 8/17/17 8:16 PM, Weijun Wang wrote: Hi Sean Change looks fine. And I found another 4 references in comments in jdk/src/java.base/share/classes/javax/security/auth/Policy.java. Good catch. I changed SolarisPrincipal and SolarisNumericUserPrincipal to UnixPrincipal and UnixNumericUserPrincipal, respectively. Updated webrev: http://cr.openjdk.java.net/~mullan/webrevs/8159544/jdk.webrev.01/ BTW, do we have a test to show what lines in various Resources.java files are used where and if one is useless? I don't know. I have copied Naoto to see if he knows. --Sean Thanks Max On Aug 18, 2017, at 3:08 AM, Sean Mullan wrote: Please review this JDK 10 change to remove the deprecated classes in com.sun.security.auth.** that have been previously marked with forRemoval=true in JDK 9. webrev: http://cr.openjdk.java.net/~mullan/webrevs/8159544/ I have also copied Jan for reviewing a change in langtools, and also build-dev for a change to one of the JDK Makefiles. Thanks, Sean
Re: RFR: JDK-8159544: Remove deprecated classes in com.sun.security.auth.**
On 8/18/17 8:31 AM, Jan Lahoda wrote: For the langtools change, I think we shouldn't remove the entries from the include.list. This list defines which packages/classes should and should not be available when compiling for previous versions of the platform using --release. The removed entries are exclude entries, so if removed, it would mean the classes should be available when compiling for previous platform versions, which is not the intent, I think. That makes sense. Maybe adding a comment (or a JBS entry) that these should be cleaned up when --release is no longer supporting 9? Sure. I restored the entries and added the following comment: #Some of the APIs below have been removed (see JDK-8159544). However, these #need to be retained until --release no longer supports 9. See also the updated webrev: http://cr.openjdk.java.net/~mullan/webrevs/8159544/langtools.webrev.01/ --Sean Thanks, Jan On 17.8.2017 21:08, Sean Mullan wrote: Please review this JDK 10 change to remove the deprecated classes in com.sun.security.auth.** that have been previously marked with forRemoval=true in JDK 9. webrev: http://cr.openjdk.java.net/~mullan/webrevs/8159544/ I have also copied Jan for reviewing a change in langtools, and also build-dev for a change to one of the JDK Makefiles. Thanks, Sean
Re: RFR 8189131: Open-source the Oracle JDK Root Certificates
On 12/5/17 12:01 PM, Volker Simonis wrote: Hi Rajan, 'cacerts' is a binary file and I thought we have at least the convention in the OpenJDK project that we don't want to check in binary artefact's if possible. One problem with 'cacerts' being a binary file is that we can not add a license and copyright to it. Another one is that it is hard to look inside the file to see what it provides. The biggest problem from my point of view is however that updates to the file will be opaque. Wouldn't it make more sense to add the root certificates in plain text format (e.g. like the Mozilla cacert data [1]) and create the binary cacert file at build time? This would also make it easy to merge the OpenJDK built-in root certificates with user/distributor provided ones. But that's really just a nice side effect. The main reason for my request is that I'm somehow feeling uncomfortable to maintain a security-relevant part of the OpenJDK in an opaque, binary blob. What do others think? When all is said and done, the certs themselves are binary; we cannot change that. But I agree having some sort of build mechanism that imports each cert from a textual representation (which can be annotated with comments/copyright) to create the binary cacerts keystore would be nice -- however, I think implementing something like what Mozilla/NSS is doing is not a trivial project and would put this JEP in jeopardy for making JDK 10. I suggest filing an RFE for now. --Sean Regards, Volker [1] https://hg.mozilla.org/mozilla-central/raw-file/tip/security/nss/lib/ckfw/builtins/certdata.txt On Fri, Dec 1, 2017 at 5:54 PM, Rajan Halade wrote: May I request for your review of this fix to open source the root certificates in Oracle's Java SE Root CA program. The fix is to populate cacerts keystore with root certificates and add corresponding tests for it as per the test plan outlined at JDK-8191711. interoperability tests are added against CAs with available test certificates. Webrev: http://cr.openjdk.java.net/~rhalade/8189131/webrev.00/ JEP: https://bugs.openjdk.java.net/browse/JDK-8191486 Thanks, Rajan
RFR: 8284105: Update security libraries to use sealed classes
Please review these changes to update the security libraries to use sealed classes. See JEP 409 for more details on sealed classes. No CSR is required as all the changes are to internal classes. A few classes that did not have subclasses were simply marked final instead of sealed. - Commit messages: - Update security libraries to use sealed classes. Changes: https://git.openjdk.java.net/jdk/pull/8165/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8165&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284105 Stats: 50 lines in 20 files changed: 8 ins; 0 del; 42 mod Patch: https://git.openjdk.java.net/jdk/pull/8165.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8165/head:pull/8165 PR: https://git.openjdk.java.net/jdk/pull/8165
Re: Code review request: 8011402: Move blacklisting certificate logic from hard code to data
On 09/06/2013 09:30 AM, Weijun Wang wrote: Hi Sean Please review the code changes at 8011402: Move blacklisting certificate logic from hard code to data Hard coded blacklisted certificates are moved out of the class file and now inside a data file. Furthermore, only their fingerprints are released in the JRE. The makefile covers blacklist files in both open and closed repo. X509CertImpl: Might it not be better to store the fingerprints in UntrustedCertificates in a WeakHashMap (using the Certificate as a key)? This way we don't need to add public mutator methods to this immutable (for the most part) class. If you agree, we should also change Certificate.hashCode to cache the hashcode instead of calculating it every time. UntrustedCertificates: [65] We should log the exception (could be a parsing error, so we would want to know that) BlacklistedCertsConverter: I'm a little concerned that this tool re-writes the blacklisted.certs file each time, as a mistake could wipe out previous entries. I would prefer if it just appended to the existing file. I would suggest that the input be a file containing a single PEM encoded cert, and that the tool append the hash to the blacklist.certs file, and the PEM cert to the blacklist.pem file. --Sean No regression test, cleanup. *build-dev*, I am not an export of Makefile, and I have some questions: 1. I create a new macro (or function?) called cat-files. Its only difference from install-file is that it needs to deal with two inputs. Do we already have a similar macro somewhere? 2. cat-files is defined inside CopyFiles.gmk right beside its usage. Do you think it's better to define it in a common file? 3. Most important: it only works if both $(BLACKLISTED_CERTS_SRC_OPEN) and $(BLACKLISTED_CERTS_SRC_CLOSED) already exists. Currently there is no closed blacklist, but I still have to create an empty file there. Otherwise, there will be make[2]: *** No rule to make target `/space/repos/jdk8/tl/jdk/src/closed/share/lib/security/blacklisted.certs', needed by `/space/repos/jdk8/tl/build/macosx-x86_64-normal-server-release/jdk/lib/security/blacklisted.certs'. Stop. Is there a way to make it work without adding that empty file? Thanks Max
Re: webrev.01 of 8011402: Move blacklisting certificate logic from hard code to data
Ok, I suggested you use a WeakHashMap but now I'm a little concerned this could become a bottleneck if every certificate check has to lock the map. Hmm. Maybe we should go back to the previous code, that also had some concurrency issues but it was only per certificate, and wasn't too bad since the hash would always be the same (maybe we could just mark the fingerprint variable volatile). I think this is worse because the lock needs to be obtained when any certificate is checked. Also, have you thought about computing the fingerprint as you read in the bytes of the certificate? This means every certificate object (at least our own implementation) would have the fingerprint calculated already, but since you are calculating the hash as you are reading in the bytes, the performance impact might not be much at all. However one issue is that you don't know what algorithm to use, unless you read in the blacklist file. We could just assume SHA-256 for now. Certificate: [70] I would mark this volatile. [134] the old code returned 0, seems we should preserve that even though this is an odd error case CheckAll: I would move this test to test/lib/java/security and rename it to CheckBlacklistedCerts. You should also check that the files have the same number of entries. UntrustedCertificates: [84] nit: insert spaces around the "=" and "<" --Sean On 09/11/2013 09:32 AM, Weijun Wang wrote: Slightly updated at the same location. Added different algorithm check in the 2 makefiles. Thanks Max On 9/11/13 3:57 PM, Weijun Wang wrote: Hi Sean and Erik An updated webrev is at http://cr.openjdk.java.net/~weijun/8011402/webrev.01/ Changes since the last webrev: - Some makefile changes * wildcard on closed file * make sure the file's first line is always "Algorithm=" - Move fingerprint cache for cert from X509CertImpl to UntrustedCertificates - Cache hash for Certificate - log blacklist parsing error in UntrustedCertificates - A new test Thanks Max On 9/6/13 9:30 PM, Weijun Wang wrote: Hi Sean Please review the code changes at 8011402: Move blacklisting certificate logic from hard code to data http://cr.openjdk.java.net/~weijun/8011402/webrev.00/ Hard coded blacklisted certificates are moved out of the class file and now inside a data file. Furthermore, only their fingerprints are released in the JRE. The makefile covers blacklist files in both open and closed repo. No regression test, cleanup. *build-dev*, I am not an export of Makefile, and I have some questions: 1. I create a new macro (or function?) called cat-files. Its only difference from install-file is that it needs to deal with two inputs. Do we already have a similar macro somewhere? 2. cat-files is defined inside CopyFiles.gmk right beside its usage. Do you think it's better to define it in a common file? 3. Most important: it only works if both $(BLACKLISTED_CERTS_SRC_OPEN) and $(BLACKLISTED_CERTS_SRC_CLOSED) already exists. Currently there is no closed blacklist, but I still have to create an empty file there. Otherwise, there will be make[2]: *** No rule to make target `/space/repos/jdk8/tl/jdk/src/closed/share/lib/security/blacklisted.certs', needed by `/space/repos/jdk8/tl/build/macosx-x86_64-normal-server-release/jdk/lib/security/blacklisted.certs'. Stop. Is there a way to make it work without adding that empty file? Thanks Max
Re: webrev.01 of 8011402: Move blacklisting certificate logic from hard code to data
On 09/17/2013 07:07 AM, Weijun Wang wrote: Webrev updated to version 02 at http://cr.openjdk.java.net/~weijun/8011402/webrev.02/ Changes since webrev.01: 1. Makefiles: - new build logic outside "ifndef OPENJDK" - Add a sed check to make sure open and close list use same algorithm 2. Fingerprint calculation moved into X509CertImpl using a ConcurrrentHashMap, although we only use one algorithm now. Can you set the default size to 1 or 2? I think it may be worth adding (maybe not for JDK 8 but JDK 9) a new method to the Certificate class called getFingerPrint(String alg) ... This way the fingerprint would not have to be calculated every time when using 3rd party providers for CertificateFactory. Also, you still have the fingerprints HashMap in UntrustedCertificates.java though it is no longer used. 3. Certificate::hashCode is now 0 if it's not a X509Cert Ok. 4. Cleanup comments in blacklisted.certs.pem, only subject/issuer/serial remain Ok. 5. Test moved to lib/security and check more. I didn't change Certificate's private hash field to volatile. Ok. --Sean
[8] Review Request for 8007292 : Add JavaFX internal packages to package.access
Please review the fix for the following bug: https://bugs.openjdk.java.net/browse/JDK-8007292 This bug requires build changes and a new build tool to add additional restricted packages to the java.security file which are not part of OpenJDK. These packages are only added when doing a build including the open and closed sources. The restricted packages and new test are in the closed sources and will be reviewed separately. webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/ Thanks, Sean
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
On 10/09/2013 05:14 AM, Erik Joelsson wrote:
Hello Sean,
On 2013-10-09 06:33, David Holmes wrote:
Hi Sean,
Not a full review.
On 9/10/2013 5:52 AM, Sean Mullan wrote:
Please review the fix for the following bug:
https://bugs.openjdk.java.net/browse/JDK-8007292
This bug requires build changes and a new build tool to add additional
restricted packages to the java.security file which are not part of
OpenJDK. These packages are only added when doing a build including the
open and closed sources.
The restricted packages and new test are in the closed sources and will
be reviewed separately.
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/
Based on your description and the ifndef OPENJDK it sounds to me that
this doesn't belong in the OpenJDK makefiles.
I agree in principle, but the pattern on how to handle this isn't well
established yet and something we need to improve on. In this case we
would need to make changes on the open side anyway to provide hooks for
overrides of the behavior. I'm willing to accept this for now.
Thanks. Also, keep in mind that this hook allows each vendor,etc to add
additional proprietary or internal packages to the restricted packages
properties for their own build. So I think it is useful in general in
that respect.
That aside I would think the CP+RM could be changed to a MV.
Agreed.
Right. Will do.
I would prefer the TOOL_ADDTO... line to be moved to
jdk/makefiles/Tools.gmk with the other similar definitions, even if it
is only used here atm.
Ok.
In the tool this code doesn't show correct use of try-with-resources:
51 try (BufferedReader br = new BufferedReader(new
FileReader(args[0]));
52 BufferedWriter bw = new BufferedWriter(new
FileWriter(args[1]))) {
The FileReader and FileWriter should also be covered by TWR:
try (FileReader fr = new FileReader(args[0]);
BufferedReader br = new BufferedReader(fr);
FileWriter fw = new FileWriter(args[1]);
BufferedWriter bw = new BufferedWriter(fw)) {
I'm not familiar with the try-with-resources, but calling close on a
BufferedReader/writer will close the underlying reader/writer so nothing
will be left open, will it not?
That's what I thought as well. David?
Finally do we still use make/tools/Makefile in the new build?
No, we don't. If you want to add old build support for this, you would
also need to add usage of the tool to the correct old makefile.
I don't think it's necessary to add this to the old build at this point.
I'll post another webrev later in the day with these updates.
Thanks,
Sean
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
Updated webrev:
http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.01/
Let me know if there are any more comments, otherwise I will plan to
push tomorrow.
Thanks,
Sean
On 10/09/2013 09:20 AM, Sean Mullan wrote:
On 10/09/2013 05:14 AM, Erik Joelsson wrote:
Hello Sean,
On 2013-10-09 06:33, David Holmes wrote:
Hi Sean,
Not a full review.
On 9/10/2013 5:52 AM, Sean Mullan wrote:
Please review the fix for the following bug:
https://bugs.openjdk.java.net/browse/JDK-8007292
This bug requires build changes and a new build tool to add additional
restricted packages to the java.security file which are not part of
OpenJDK. These packages are only added when doing a build including the
open and closed sources.
The restricted packages and new test are in the closed sources and will
be reviewed separately.
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/
Based on your description and the ifndef OPENJDK it sounds to me that
this doesn't belong in the OpenJDK makefiles.
I agree in principle, but the pattern on how to handle this isn't well
established yet and something we need to improve on. In this case we
would need to make changes on the open side anyway to provide hooks for
overrides of the behavior. I'm willing to accept this for now.
Thanks. Also, keep in mind that this hook allows each vendor,etc to add
additional proprietary or internal packages to the restricted packages
properties for their own build. So I think it is useful in general in
that respect.
That aside I would think the CP+RM could be changed to a MV.
Agreed.
Right. Will do.
I would prefer the TOOL_ADDTO... line to be moved to
jdk/makefiles/Tools.gmk with the other similar definitions, even if it
is only used here atm.
Ok.
In the tool this code doesn't show correct use of try-with-resources:
51 try (BufferedReader br = new BufferedReader(new
FileReader(args[0]));
52 BufferedWriter bw = new BufferedWriter(new
FileWriter(args[1]))) {
The FileReader and FileWriter should also be covered by TWR:
try (FileReader fr = new FileReader(args[0]);
BufferedReader br = new BufferedReader(fr);
FileWriter fw = new FileWriter(args[1]);
BufferedWriter bw = new BufferedWriter(fw)) {
I'm not familiar with the try-with-resources, but calling close on a
BufferedReader/writer will close the underlying reader/writer so nothing
will be left open, will it not?
That's what I thought as well. David?
Finally do we still use make/tools/Makefile in the new build?
No, we don't. If you want to add old build support for this, you would
also need to add usage of the tool to the correct old makefile.
I don't think it's necessary to add this to the old build at this point.
I'll post another webrev later in the day with these updates.
Thanks,
Sean
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
On 10/10/2013 05:57 AM, Erik Joelsson wrote: Adding makefiles to make/tools is not needed for the new build. Either remove those changes or make a complete port to the old build. I'm not pushing for porting this to the old build at this point since missing this will not cause the old build to stop working, it will just diverge the resulting bits a bit more. I have ported the changes to the old build, please review. New webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.02/ The only modified file is make/java/security/Makefile Thanks, Sean
Re: J2SE est mort, vive Java SE!
On 11/27/2013 03:41 AM, Alan Bateman wrote: On 27/11/2013 00:29, David Holmes wrote: On 27/11/2013 8:49 AM, mark.reinh...@oracle.com wrote: Now that we've removed the old build system, can we please now remove the last vestiges of Sun's pre-Java 5 naming scheme? There are also a bunch of security libs with j2 in their names - should these be renamed too, or will that cause too many compatibility issues? What security libs have j2 in their names? The security providers have vestiges of Sun in the provider names (SUN, SunPKCS11, SunJSSE ..) but these are documented and I don't think can be changed (although it not recommended to select services from specific providers). To my knowledge, the names of the shared libraries are not something that anyone should depend on, they are purely an implementation detail. I'm sure someone in the security area with jump to review any proposed changes here but I wouldn't expect it to cause problems. Changing the provider names is a very high risk change to make this late in JDK 8. If we decided to change them, I would recommend deprecating (but not removing) the existing Sun* providers for at least one major release to allow apps to transition. Removing "sun" from the names of the JAR files (ex: sunjce_provider.jar, sunpkcs11.jar, sunec.jar) is probably less risky from a compatibility perspective, but I would still want to think about it some more and find out if this would break anyone. Offhand, it feels like this type of change would be risky for JDK 8. --Sean
Re: J2SE est mort, vive Java SE!
On 11/27/2013 10:46 AM, Wang Weijun wrote: What security libs have j2 in their names? libj2gss.so etc. Any idea what the compatibility risk of removing the "2" would be? This seems less prominent because it doesn't say "j2se", "j2re" or "j2sdk" but I think it would be fine to rename these if the risk is low. --Sean
Re: J2SE est mort, vive Java SE!
On 11/29/2013 06:47 PM, Omair Majid wrote: * Sean Mullan [2013-11-27 12:16]: On 11/27/2013 10:46 AM, Wang Weijun wrote: What security libs have j2 in their names? libj2gss.so etc. Any idea what the compatibility risk of removing the "2" would be? This seems less prominent because it doesn't say "j2se", "j2re" or "j2sdk" but I think it would be fine to rename these if the risk is low. Out of curiosity, what would be the replacement prefix for this (and other native libraries)? Just 'j' instead of 'j2'? A name like libgss.so is probably not a good idea, since then using the gss implementation library (or another native library) which could have the prefix-less name becomes problematic. 'j' sounds reasonable to me. However, I'm not really sure if the 'j2' here is from 'j2se', 'j2re', or 'j2sdk', I would have to ask some developers who did the original implementation. It could have been simply to help avoid naming clashes with other software using these names. A search for libjpkcs11.so brings up a few results. If that's the case, I would prefer to leave the names as is. --Sean As a concrete case, an OpenJDK patch I am maintaining [1] tries to use the system libjpeg instead of the one bundled with OpenJDK. The bundled copy gets built with the code from upstream libjpeg as well as JNI bits that are specific to OpenJDK. So I have to split it out into a OpenJDK-specific library that links to the real libjpeg. I went with a 'java' prefix for the OpenJDK-specific JNI library, but I would like to know if there is a consistent standard I should try and use. I am going to try and push this patch upstream, if possible. Thanks, Omair [1] http://pkgs.fedoraproject.org/cgit/java-1.8.0-openjdk.git/tree/system-libjpeg.patch#n191
Re: J2SE est mort, vive Java SE!
On 12/02/2013 04:50 PM, Phil Race wrote: On 12/2/2013 1:28 PM, Sean Mullan wrote: 'j' sounds reasonable to me. However, I'm not really sure if the 'j2' here is from 'j2se', 'j2re', or 'j2sdk', I would have to ask some developers who did the original implementation. It could have been simply to help avoid naming clashes with other software using these names. A search for libjpkcs11.so brings up a few results. If that's the case, I would prefer to leave the names as is. As Mark indicated its from a marketing name. When JDK 1.2 was shipped it branded as "Java 2". Yes, I understand that now. The question directed to me was whether we should remove the j2 from the names of the security libraries. What I am not 100% sure is whether the 'j2' in those library names is from "Java 2". The 2 simply could have been a way to avoid naming clashes. IBM's JDK includes a pkcs11 library named libjpkcs11.so. Also, pkcs11 support and the libj2pkcs11.so library were not added to the JDK until 1.5, long after Java 2 was announced. Anyway, I'll go ask around and try to find out. --Sean It didn't mean version 1.2, it was just a name. Other names were considered too, I still remember the ripple of horror around the large, packed, room when the initial proposal was revealed but that was dropped in favour of Java 2. -phil.
Re: J2SE est mort, vive Java SE!
On 12/02/2013 05:05 PM, Sean Mullan wrote: What I am not 100% sure is whether the 'j2' in those library names is from "Java 2". The 2 simply could have been a way to avoid naming clashes. IBM's JDK includes a pkcs11 library named libjpkcs11.so. Also, pkcs11 support and the libj2pkcs11.so library were not added to the JDK until 1.5, long after Java 2 was announced. Anyway, I'll go ask around and try to find out. Just following up on this. So I asked around, and though nobody knows for sure (the original developer has moved on), a few people were pretty sure the j2 in those library names does not stand for "Java 2" but "Java to" as in "Java to PKCS11" since they are essentially libraries that allow you to bridge between the standard Java crypto API and a standard native crypto API, such as PKCS11. --Sean
Re: RFR 8047765: Generate blacklist.certs in build
On 07/02/2014 01:02 AM, Wang Weijun wrote: On Jul 2, 2014, at 12:48, David Holmes wrote: 73 // Output sorted for eye pleasure. ?? "eye pleasure" Well, it's easy for a human to locate one from a sorted output. Or maybe it's because the old one is sorted and I don't want the new one looks ugly. I think David's comment was in reference to the term "eye pleasure" and not to the benefits of sorting the list. Why not just change this to something like: // Output sorted to make it easier to find entries --Sean
Re: RFR 6997010: Consolidate java.security files into one file with modifications
On 07/22/2014 10:28 PM, Wang Weijun wrote: Please review the code change at http://cr.openjdk.java.net/~weijun/6997010/webrev.00/ The fix consolidates java.security- files into one with #ifdef directives. There are several major changes: 1. Creation of file is moved from CopyFiles to GenerateData, since we are really generating something now. Said that, the source data is kept in src/share/lib/security instead of make/data. I am OK with moving it if anyone desires. 2. The new tool MakeJavaSecurity includes the function of old AddToRestrictedPkgs. MakeJavaSecurity includes a new argument to deal with the platform dependent entries. The restricted.pkgs argument is also changed from a list of entries to a file name, so that we can also support the same #ifdef mechanism inside restricted.pkgs. 3. The new consolidated java.security supports #ifdef and #ifndef. It is not necessary to support #else or (and|or) of multiple #ifdef's now. 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? 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. Thanks for doing this, it will really help maintaining this file going forward! --Sean The MakeJavaSecurity tool will strip the trailing ",\" from the last line to make the file exactly the same as before, although personally I don't think it's really necessary since the following empty line will terminate the entry automatically. Thanks Max
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: RFR [JEP 220] Modular Run-Time Images
The JDK security related changes look fine to me. The test/java/net/URLPermission/policy.* files only have copyright changes for some reason. --Sean On 11/20/2014 04:41 PM, Chris Hegarty wrote: From: Chris Hegarty Subject: RFR [JEP 220] Modular Run-Time Images Date: 20 November 2014 21:39:14 GMT To: jigsaw-dev , jdk9-dev , build-dev , Alan Bateman , Alex Buckley , Chris Hegarty , Erik Joelsson , Jonathan Gibbons , Karen Kinnear , "Jim Laskey (Oracle)" , Magnus Ihse Bursie , Mandy Chung , Mark Reinhold , Paul Sandoz , "A. Sundararajan" This is a review request for the changes for JEP 220: Modular Run-Time Images [1]. There are a number of individuals responsible for these changes. Some, possibly not all, are explicitly listed in the 'To' section of this mail, and they will help address any comments arising from this review request. The new run-time image structure is defined in JEP 220 [1], and can be seen as a result of building the staging forest [2], or in the early access builds [3]. Webrevs: http://cr.openjdk.java.net/~chegar/8061971/00/ Due to the significant impact of these changes, a JDK 9 promotion has been tentatively reserved for their integration. All comments are welcome, although given the nature of the changes then we might have to create separate issues in JIRA to address some of them later in jdk9/dev. -Chris. [1] http://openjdk.java.net/jeps/220 [2] http://hg.openjdk.java.net/jigsaw/m2/ [3] http://openjdk.java.net/projects/jigsaw/ea
Re: RFR: 8235710: Remove the legacy elliptic curves [v2]
On Tue, 22 Sep 2020 00:18:07 GMT, Anthony Scarpino wrote: >> This change removes the native elliptic curves library code; as well as, and >> calls to that code, tests, and files >> associated with those libraries. The makefiles have been changed to remove >> from all source builds of the ec code. The >> SunEC system property is removed and java.security configurations changed to >> reflect the removed curves. This will >> remove the following elliptic curves from SunEC: secp112r1, secp112r2, >> secp128r1, secp128r2, secp160k1, secp160r1, >> secp160r2, secp192k1, secp192r1, secp224k1, secp224r1, secp256k1, sect113r1, >> sect113r2, sect131r1, sect131r2, >> sect163k1, sect163r1, sect163r2, sect193r1, sect193r2, sect233k1, sect233r1, >> sect239k1, sect283k1, sect283r1, >> sect409k1, sect409r1, sect571k1, sect571r1, X9.62 c2tnb191v1, X9.62 >> c2tnb191v2, X9.62 c2tnb191v3, X9.62 c2tnb239v1, >> X9.62 c2tnb239v2, X9.62 c2tnb239v3, X9.62 c2tnb359v1, X9.62 c2tnb431r1, >> X9.62 prime192v2, X9.62 prime192v3, X9.62 >> prime239v1, X9.62 prime239v2, X9.62 prime239v3, brainpoolP256r1 >> brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 > > Anthony Scarpino has updated the pull request incrementally with one > additional commit since the last revision: > > remove JDKOPT_DETECT_INTREE_EC from configure.ac throw new IllegalStateException( new InvalidAlgorithmParameterException( "Curve not supported: Private: " + ((privNC != null) ? privNC.toString() : " unknown") + ", PublicKey:" + ((pubNC != null) ? pubNC.toString() : " unknown"))); src/jdk.crypto.ec/share/classes/sun/security/ec/ECDHKeyAgreement.java line 180: > 178: ((privNC != null) ? privNC.toString() : " > unknown") + > 179: ", PublicKey:" + > 180: ((pubNC != null) ? pubNC.toString() : " > unknown"))); Spacing issues: "PublicKey:" should be "PublicKey: " and " unknown" should be "unknown". - PR: https://git.openjdk.java.net/jdk/pull/289
RFR: 8243559: Remove root certificates with 1024-bit keys
This change removes five root certificates with 1024-bit RSA public keys from the system-wide `cacerts` keystore. These are older VeriSign and Thawte root CA certificates which are no longer necessary to retain and should have minimal compatibility risk if removed. See the CSR for more details: https://bugs.openjdk.java.net/browse/JDK-8256502 - Commit messages: - 8256502: Remove root certificates with 1024-bit keys Changes: https://git.openjdk.java.net/jdk/pull/1387/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1387&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8243559 Stats: 140 lines in 6 files changed: 0 ins; 138 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1387.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1387/head:pull/1387 PR: https://git.openjdk.java.net/jdk/pull/1387
Re: RFR: 8253866: Security Libs Terminology Refresh
On Thu, 14 Jan 2021 06:32:37 GMT, Jamil Nimeh wrote: > This is the security libs portion of the effort to replace > archaic/non-inclusive words with more neutral terms (see JDK-8253315 for > details). > > Here are the changes covering core libraries code and tests. Terms were > changed as follows: > > - blacklisted.certs -> blocked.certs (along with supporting make > infrastructure and tests) > - master/slave in KRB5 -> primary/replica > - blacklist in other files -> deny list > - whitelist -> allow list > > Addressing similar issues in upstream 3rd party code is out of scope of this > PR. Such changes will be picked up from their upstream sources. Marked as reviewed by mullan (Reviewer). src/java.base/share/conf/security/java.security line 454: > 452: #configuration, but with smaller max_retries and timeout values. > 453: #max_retries and timeout are optional numerical parameters (default > 1 and > 454: #5000, which means once and 5 seconds). Please notes that if any of > the Typo: s/notes/note/ src/java.base/share/conf/security/java.security line 455: > 453: #max_retries and timeout are optional numerical parameters (default > 1 and > 454: #5000, which means once and 5 seconds). Please notes that if any of > the > 455: #values defined here is more than what is defined in krb5.conf, it > will be Typo: s/is more/are more/ - PR: https://git.openjdk.java.net/jdk/pull/2074
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 06:31:06 GMT, Alan Bateman wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. > > src/java.base/share/classes/java/lang/SecurityManager.java line 315: > >> 313: * >> 314: * @since 1.0 >> 315: * @deprecated The Security Manager is deprecated and subject to >> removal in a > > Javadoc will prefix, in bold, "Deprecated, for removal: This API element is > subject to removal in a future version". I'm just wondering if the sentence > will be repeated here, can you check? It includes both:  - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 15:19:21 GMT, Alan Bateman wrote: >> It includes both: >>  > > Thanks for checking, I assumed that was the case so wondering if it should be > dropped from the deprecation text to avoid the repeated sentence. My feeling is that it is better to be more specific than the generic removal text as this is the most significant class that is being deprecated for removal. Also, this says "the Security Manager" (note the space between) which really encompasses more than the `java.lang.SecurityManager` API and which is explained more completely in the JEP. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Fri, 21 May 2021 15:27:39 GMT, Daniel Fuchs wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
>
> src/java.base/share/classes/java/lang/SecurityManager.java line 104:
>
>> 102: * method will throw an {@code UnsupportedOperationException}). If the
>> 103: * {@systemProperty java.security.manager} system property is set to the
>> 104: * special token "{@code allow}", then a security manager will not be
>> set at
>
> Can/should the `{@systemProperty ...}` tag be used more than once for a given
> system property? I thought it should be used only once, at the place where
> the system property is defined. Maybe @jonathan-gibbons can offer some more
> guidance on this.
Good point. I would remove the extra @systemProperty tags on lines 103, 106,
and 113. Also, in `System.setSecurityManager` there are 3 @systemProperty
java.security.manager tags, I would just keep the first one. (I think it's ok
to have more than one, if they are defined in different APIs).
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8225082: Remove IdenTrust certificate that is expiring in September 2021
On Wed, 21 Jul 2021 02:02:06 GMT, Rajan Halade wrote: > This CA had no code signing certificates issued so it is safe to remove it > from cacerts truststore. Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4851
Re: RFR: 8225083: Remove Google certificate that is expiring in December 2021
On Tue, 17 Aug 2021 03:59:15 GMT, Rajan Halade wrote: > Removed the expiring root certificate. Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5137
Re: RFR: 8275252: Migrate cacerts from JKS to password-less PKCS12
On Thu, 14 Oct 2021 13:36:19 GMT, Weijun Wang wrote: > The cacerts file is now a password-less PKCS12 file. This make sure old code > that uses a JKS KeyStore object can continuously load it using a null > password (in fact, any password) and see all certificates inside. make/jdk/src/classes/build/tools/generatecacerts/GenerateCacerts.java line 74: > 72: cert = (X509Certificate) cf.generateCertificate(fis); > 73: } > 74: ks.setCertificateEntry(alias, cert); In the previous code, we always used a fixed date (cert's notBefore) for the creation date. Now, it seems it will be always different and based on when it was created. I'm not really sure if this is an issue in practice, but I think it is worth thinking about a bit more - do you have any thoughts on this? - PR: https://git.openjdk.java.net/jdk/pull/5948
Re: RFR: 8275252: Migrate cacerts from JKS to password-less PKCS12 [v2]
On Tue, 19 Oct 2021 19:48:23 GMT, Weijun Wang wrote:
>> The cacerts file is now a password-less PKCS12 file. This make sure old code
>> that uses a JKS KeyStore object can continuously load it using a null
>> password (in fact, any password) and see all certificates inside.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> use a standard name
Marked as reviewed by mullan (Reviewer).
make/jdk/src/classes/build/tools/generatecacerts/GenerateCacerts.java line 54:
> 52: public static void store(String dir, OutputStream stream) throws
> Exception {
> 53:
> 54: CertificateFactory cf = CertificateFactory.getInstance("X509");
Nit: better to use the standard name here: "X.509".
-
PR: https://git.openjdk.java.net/jdk/pull/5948
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 12:13:03 GMT, Andrew Leonard wrote: >> Addition of a configure option --with-cacerts-src='user cacerts folder' to >> allow developers to specify their own cacerts PEM folder for generation of >> the cacerts store using the deterministic openjdk GenerateCacerts tool. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > deterministic cacerts generation > >Signed-off-by: Andrew Leonard > - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard I don’t have any major concerns with this change, as long as the default cacerts are still the ones that are in the JDK. As an aside, using Mozilla's root certificates might be fine for TLS certificates, but if you need to support code signing certificates you may run into issues with missing CAs as Mozilla's Root program does not support that use case. Also, by overriding the roots included in the JDK, you are taking on the responsibility (which is significant, in my opinion) of ensuring that those roots are trusted and have not been compromised, revoked, have weak keys, etc. - PR: https://git.openjdk.java.net/jdk/pull/6647
