Re: RFR 7191662: JCE providers should be located via ServiceLoader
On 06/16/2015 10:40 PM, Valerie Peng wrote: It's to make the runtime configuration of 3rd party PKCS11 provider easier on non-Solaris systems. With this entry, callers can simply do Security.getProvider(SunPKCS11) and then call Provider.configure with their own configuration file. Otherwise, they will have to use reflection, e.g. Class.forName(sun.security.pkcs11.SunPKCS11) and then call its newInstance() method and then call Provider.configure method. Or, iteration through the ServiceLoader until the SunPKCS11 provider is found. Both doesn't seem very user-friendly in my view. Agreed, I also think this will provide a better out-of-the-box experience as it avoids having to modify/override the java.security file to add a PKCS11 provider on non-Solaris platforms. --Sean
Re: RFR 7191662: JCE providers should be located via ServiceLoader
On 16/06/2015 00:58, Valerie Peng wrote: It seems that the jimage refresh change may still take some time, so we will end up removing the makefile related changes and then deferring the ServiceLoader related changes to Jake workspace. Here is the latest webrev (Security source/test changes only, no more makefile changes) http://cr.openjdk.java.net/~valeriep/7191662/webrev.04/ Summary of changes from webrev.03: 1) switch back to provider class names for java.security file 2) separated out the java.policy change into its own as SQE has filed a bug for it 3) updated sun.security.jca.Providers class due to 1) 4) fixed sun.security.tools.jarsigner.Main to use the new Provider.configure() api 5) fixed a bug in sun.security.pkcs11.PKCS11Test regarding searching and configuring PKCS11 provider I'm also happy with dropping the make file changes and the META-INF/services files from this patch. Not for this patch but at some point I think we should re-examine the javadoc in Provider to make it clearer how it can be deployed as a service provider. Specifically the wording A provider normally identifies itself with a file named java.security.Provider in the resource directory META-INF/services could be re-examined. -Alan.
Re: RFR 7191662: JCE providers should be located via ServiceLoader
I skimmed it and looks okay to me. Thanks for separating this build work and avoid interfering the jimage refresh work. Mandy On Jun 15, 2015, at 4:58 PM, Valerie Peng valerie.p...@oracle.com wrote: It seems that the jimage refresh change may still take some time, so we will end up removing the makefile related changes and then deferring the ServiceLoader related changes to Jake workspace. Here is the latest webrev (Security source/test changes only, no more makefile changes) http://cr.openjdk.java.net/~valeriep/7191662/webrev.04/ Summary of changes from webrev.03: 1) switch back to provider class names for java.security file 2) separated out the java.policy change into its own as SQE has filed a bug for it 3) updated sun.security.jca.Providers class due to 1) 4) fixed sun.security.tools.jarsigner.Main to use the new Provider.configure() api 5) fixed a bug in sun.security.pkcs11.PKCS11Test regarding searching and configuring PKCS11 provider Thanks, Valerie On 6/8/2015 4:44 PM, Valerie Peng wrote: What is the bug id for the image refresh change? Just so I can keep a watch and hold my changes for the time being. My webrev has a new regression test which compares the list of providers found by ServiceLoader and Security.getProviders() call. So, if the META-INF/services/java.security.Provider file content is not correct, the new test would fail and it's a clear indication that ServiceLoader can't find one or more of the providers. Thanks, Valerie On 6/5/2015 10:43 PM, Mandy Chung wrote: On Jun 5, 2015, at 4:32 PM, Valerie Pengvalerie.p...@oracle.com wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The
Re: RFR 7191662: JCE providers should be located via ServiceLoader
On 06/15/2015 07:58 PM, Valerie Peng wrote: It seems that the jimage refresh change may still take some time, so we will end up removing the makefile related changes and then deferring the ServiceLoader related changes to Jake workspace. Here is the latest webrev (Security source/test changes only, no more makefile changes) http://cr.openjdk.java.net/~valeriep/7191662/webrev.04/ Summary of changes from webrev.03: 1) switch back to provider class names for java.security file Why is there an entry for security.provider.tbd=sun.security.pkcs11.SunPKCS11 on line 90? 2) separated out the java.policy change into its own as SQE has filed a bug for it 3) updated sun.security.jca.Providers class due to 1) 4) fixed sun.security.tools.jarsigner.Main to use the new Provider.configure() api 5) fixed a bug in sun.security.pkcs11.PKCS11Test regarding searching and configuring PKCS11 provider Looks fine otherwise. --Sean Thanks, Valerie On 6/8/2015 4:44 PM, Valerie Peng wrote: What is the bug id for the image refresh change? Just so I can keep a watch and hold my changes for the time being. My webrev has a new regression test which compares the list of providers found by ServiceLoader and Security.getProviders() call. So, if the META-INF/services/java.security.Provider file content is not correct, the new test would fail and it's a clear indication that ServiceLoader can't find one or more of the providers. Thanks, Valerie On 6/5/2015 10:43 PM, Mandy Chung wrote: On Jun 5, 2015, at 4:32 PM, Valerie Pengvalerie.p...@oracle.com wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of
Re: RFR 7191662: JCE providers should be located via ServiceLoader
It's to make the runtime configuration of 3rd party PKCS11 provider easier on non-Solaris systems. With this entry, callers can simply do Security.getProvider(SunPKCS11) and then call Provider.configure with their own configuration file. Otherwise, they will have to use reflection, e.g. Class.forName(sun.security.pkcs11.SunPKCS11) and then call its newInstance() method and then call Provider.configure method. Or, iteration through the ServiceLoader until the SunPKCS11 provider is found. Both doesn't seem very user-friendly in my view. Valerie On 6/16/2015 1:34 PM, Sean Mullan wrote: On 06/15/2015 07:58 PM, Valerie Peng wrote: It seems that the jimage refresh change may still take some time, so we will end up removing the makefile related changes and then deferring the ServiceLoader related changes to Jake workspace. Here is the latest webrev (Security source/test changes only, no more makefile changes) http://cr.openjdk.java.net/~valeriep/7191662/webrev.04/ Summary of changes from webrev.03: 1) switch back to provider class names for java.security file Why is there an entry for security.provider.tbd=sun.security.pkcs11.SunPKCS11 on line 90? 2) separated out the java.policy change into its own as SQE has filed a bug for it 3) updated sun.security.jca.Providers class due to 1) 4) fixed sun.security.tools.jarsigner.Main to use the new Provider.configure() api 5) fixed a bug in sun.security.pkcs11.PKCS11Test regarding searching and configuring PKCS11 provider Looks fine otherwise. --Sean Thanks, Valerie On 6/8/2015 4:44 PM, Valerie Peng wrote: What is the bug id for the image refresh change? Just so I can keep a watch and hold my changes for the time being. My webrev has a new regression test which compares the list of providers found by ServiceLoader and Security.getProviders() call. So, if the META-INF/services/java.security.Provider file content is not correct, the new test would fail and it's a clear indication that ServiceLoader can't find one or more of the providers. Thanks, Valerie On 6/5/2015 10:43 PM, Mandy Chung wrote: On Jun 5, 2015, at 4:32 PM, Valerie Pengvalerie.p...@oracle.com wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote:
Re: RFR 7191662: JCE providers should be located via ServiceLoader
It seems that the jimage refresh change may still take some time, so we will end up removing the makefile related changes and then deferring the ServiceLoader related changes to Jake workspace. Here is the latest webrev (Security source/test changes only, no more makefile changes) http://cr.openjdk.java.net/~valeriep/7191662/webrev.04/ Summary of changes from webrev.03: 1) switch back to provider class names for java.security file 2) separated out the java.policy change into its own as SQE has filed a bug for it 3) updated sun.security.jca.Providers class due to 1) 4) fixed sun.security.tools.jarsigner.Main to use the new Provider.configure() api 5) fixed a bug in sun.security.pkcs11.PKCS11Test regarding searching and configuring PKCS11 provider Thanks, Valerie On 6/8/2015 4:44 PM, Valerie Peng wrote: What is the bug id for the image refresh change? Just so I can keep a watch and hold my changes for the time being. My webrev has a new regression test which compares the list of providers found by ServiceLoader and Security.getProviders() call. So, if the META-INF/services/java.security.Provider file content is not correct, the new test would fail and it's a clear indication that ServiceLoader can't find one or more of the providers. Thanks, Valerie On 6/5/2015 10:43 PM, Mandy Chung wrote: On Jun 5, 2015, at 4:32 PM, Valerie Pengvalerie.p...@oracle.com wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the
Re: RFR 7191662: JCE providers should be located via ServiceLoader
What is the bug id for the image refresh change? Just so I can keep a watch and hold my changes for the time being. My webrev has a new regression test which compares the list of providers found by ServiceLoader and Security.getProviders() call. So, if the META-INF/services/java.security.Provider file content is not correct, the new test would fail and it's a clear indication that ServiceLoader can't find one or more of the providers. Thanks, Valerie On 6/5/2015 10:43 PM, Mandy Chung wrote: On Jun 5, 2015, at 4:32 PM, Valerie Pengvalerie.p...@oracle.com wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On
Re: RFR 7191662: JCE providers should be located via ServiceLoader
JDK-8066492 Mandy On 06/08/2015 04:44 PM, Valerie Peng wrote: What is the bug id for the image refresh change? Just so I can keep a watch and hold my changes for the time being. My webrev has a new regression test which compares the list of providers found by ServiceLoader and Security.getProviders() call. So, if the META-INF/services/java.security.Provider file content is not correct, the new test would fail and it's a clear indication that ServiceLoader can't find one or more of the providers. Thanks, Valerie On 6/5/2015 10:43 PM, Mandy Chung wrote: On Jun 5, 2015, at 4:32 PM, Valerie Pengvalerie.p...@oracle.com wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on
Re: RFR 7191662: JCE providers should be located via ServiceLoader
Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Pengvalerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader
I don't know image builder well enough to answer your question. Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Pengvalerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader
On Jun 5, 2015, at 4:32 PM, Valerie Peng valerie.p...@oracle.com wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Pengvalerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image
Re: RFR 7191662: JCE providers should be located via ServiceLoader
Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Pengvalerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader
Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Pengvalerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader
On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Pengvalerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader
Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie . Mandy On May 27, 2015, at 3:29 PM, Valerie Pengvalerie.p...@oracle.com wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
Please find comments in line.
On 5/27/2015 4:40 PM, Mandy Chung wrote:
On May 27, 2015, at 4:17 PM, Valerie Pengvalerie.p...@oracle.com wrote:
Hi, Mandy,
I have second thought on using the provider name for the java.security file...
There are a few things that I want to discuss/brainstorm internally to make sure that
there is no surprise later.
Thus, I had not made the switch in this webrev. It doesn't affect the overall
functionality.
What is the issue you concern about with ServiceLoader to load providers when
it matches its provider name rather than the classname? SPI allows you to move
away from using the classname in the configuration and separate implementation
from interfaces.
There are several things...or questions that I'd like to clarify before
making the switch.
Not all of them are related to this switch though, but still, I don't
want to complicate things further by adding new changes into this mix.
First, provider name seems looser than provider class name. With just
a name, what happens when there is a name collision?
Is there an ordering on how ServiceLoader loads/looks up services? If
yes/no, does it affect provider validation?
Second, it's a bit awkward in the SunPKCS11 provider case. For SunPKCS11
provider, its name is set in the configuration file which is passed to
the provider object upon the configure call. When ServiceLoader loads
the SunPKCS11 provider object, you can't use the provider name to look
for a match as the provider itself doesn't have that info yet. Sure, we
can always do special handling for SunPKCS11 to make this work. But this
special handling isn't needed for the class name setting.
Lastly, this overlaps with Tony's JEP and his current proposal fits
better with class name setting (this may just be my personal opinion).
So, I feel it's better to clarify all these questions before making the
switch.
I did mention about this (not using provider name in java.security) when
sending out my webrev to security alias but didn't not notify Alan and
you separately. Sorry~
Valerie
The legacyLoad() is for finding providers the old-fashioned way, e.g. through
reflection, instead of ServiceLoader.
The legacyLoad can only work using class name.
For the first push, it’s okay to leave them loaded by the legacy mechanism via
reflection until you sort out the provider name issue before you make them true
service providers. But I still think ProviderLoader.load should not tie with
the classname unless there is a strong reason to.
Running test/java/lang/SecurityManager/CheckSecurityProvider.java with a
security manager should cover more than without.
Ok with me.
I don't see much value in adding provider name verification? Verifying class
name seems enough in making sure that the providers are present as expected.
If you are using the provider name to load service providers, this adds
additional verification. I’m surprised that your webrev was changed as I
didn’t see any mail about your concern that leads to the change.
As for test/tools/launcher/MiscTests.java, it's for verifying a bug in
launcher, so I have stick with the internal API as I am not sure if switching
to public API may be too great of a change as the call path changed
significantly…
This test verifies 6856415 that needs to use sun.* API and ensures that the
right Exception is thrown. Ok - it’s okay to leave it then.
Mandy
Thanks,
Valerie
On 5/26/2015 3:27 PM, Mandy Chung wrote:
sun/security/jca/ProviderConfig.java
287 /**
288 * Loads the provider with the specified class name.
289 *
290 * @param name the name of the provider class
291 * @return the Provider, or null if it cannot be found or loaded
292 * @throws ProviderException all other exceptions are ignored
293 */
294 public Provider load(String classname) {
:
305 if (p.getClass().getName().equals(classname)) {
306 return p;
307 }
Is this load method supposed to take the provider name instead of the classname?
line 305 should check against p.getName() instead? The legacyLoad method is
for the fallback to the class name.
java.security now uses classname. Did you decide to use security provider name
instead?
Even so, the legacyLoader method should be able to find them and the load method
looks to me should check the provider name instead.
test/java/lang/SecurityManager/CheckSecurityProvider.java
you add this test to run with security manager. I wonder if you want
to run with and without security manager through @run othervm.
Now each security provider has a name. Should this test verify the provider
name as well?
test/tools/launcher/MiscTests.java
- does this test need to call sun.security.pkcs11 internal API? Can this be
changed to call public API?
I didn't review other files.
Mandy
On 05/26/2015 01:57 PM,
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On May 27, 2015, at 4:17 PM, Valerie Peng valerie.p...@oracle.com wrote:
Hi, Mandy,
I have second thought on using the provider name for the java.security file...
There are a few things that I want to discuss/brainstorm internally to make
sure that there is no surprise later.
Thus, I had not made the switch in this webrev. It doesn't affect the overall
functionality.
What is the issue you concern about with ServiceLoader to load providers when
it matches its provider name rather than the classname? SPI allows you to move
away from using the classname in the configuration and separate implementation
from interfaces.
The legacyLoad() is for finding providers the old-fashioned way, e.g. through
reflection, instead of ServiceLoader.
The legacyLoad can only work using class name.
For the first push, it’s okay to leave them loaded by the legacy mechanism via
reflection until you sort out the provider name issue before you make them true
service providers. But I still think ProviderLoader.load should not tie with
the classname unless there is a strong reason to.
Running test/java/lang/SecurityManager/CheckSecurityProvider.java with a
security manager should cover more than without.
Ok with me.
I don't see much value in adding provider name verification? Verifying class
name seems enough in making sure that the providers are present as expected.
If you are using the provider name to load service providers, this adds
additional verification. I’m surprised that your webrev was changed as I
didn’t see any mail about your concern that leads to the change.
As for test/tools/launcher/MiscTests.java, it's for verifying a bug in
launcher, so I have stick with the internal API as I am not sure if switching
to public API may be too great of a change as the call path changed
significantly…
This test verifies 6856415 that needs to use sun.* API and ensures that the
right Exception is thrown. Ok - it’s okay to leave it then.
Mandy
Thanks,
Valerie
On 5/26/2015 3:27 PM, Mandy Chung wrote:
sun/security/jca/ProviderConfig.java
287 /**
288 * Loads the provider with the specified class name.
289 *
290 * @param name the name of the provider class
291 * @return the Provider, or null if it cannot be found or loaded
292 * @throws ProviderException all other exceptions are ignored
293 */
294 public Provider load(String classname) {
:
305 if (p.getClass().getName().equals(classname)) {
306 return p;
307 }
Is this load method supposed to take the provider name instead of the
classname?
line 305 should check against p.getName() instead? The legacyLoad method is
for the fallback to the class name.
java.security now uses classname. Did you decide to use security provider
name instead?
Even so, the legacyLoader method should be able to find them and the load
method
looks to me should check the provider name instead.
test/java/lang/SecurityManager/CheckSecurityProvider.java
you add this test to run with security manager. I wonder if you want
to run with and without security manager through @run othervm.
Now each security provider has a name. Should this test verify the
provider
name as well?
test/tools/launcher/MiscTests.java
- does this test need to call sun.security.pkcs11 internal API? Can this be
changed to call public API?
I didn't review other files.
Mandy
On 05/26/2015 01:57 PM, Sean Mullan wrote:
This all looks fine to me (except for the Makefile stuff which I'll leave
to others).
--Sean
On 05/21/2015 12:21 AM, Valerie Peng wrote:
Sean,
Could you please review this change? The changes are mostly the same as
the prototype in Jake, but I have to make some modification due to the
difference in ServiceLoader lookup in OpenJDK (corresponding
META-INF/services/java.security.Provider files in each module) and the
related makefile change (merge their content into one for the final
image build). Then, I adjusted the Provider.configure() method to take a
single String argument to be consistent with the providerarg option
that keytool defined.
In addition, I also made some misc changes, such as configuring the
providers inside ProviderConfig instead of ProviderLoader, add back the
doPrivileged block to all the provider constructors. I also have second
thought on making the switch to privider name (instead of provider class
name) in java.security file, so I reverted the changes on that - that
SunPKCS11 provider has its name specified in its configuration file, so
when ServiceLoader loads the PKCS11 provider, the configuration file has
not been passed to it, so the name is not known at that time. Thus,
using the class name for the provider list entry seems to fit the flow
better. I also updated the default policy
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
Hi, Mandy,
I have second thought on using the provider name for the java.security
file...
There are a few things that I want to discuss/brainstorm internally to
make sure that there is no surprise later.
Thus, I had not made the switch in this webrev. It doesn't affect the
overall functionality.
The legacyLoad() is for finding providers the old-fashioned way, e.g.
through reflection, instead of ServiceLoader.
The legacyLoad can only work using class name.
Running test/java/lang/SecurityManager/CheckSecurityProvider.java with a
security manager should cover more than without.
I don't see much value in adding provider name verification? Verifying
class name seems enough in making sure that the providers are present as
expected.
As for test/tools/launcher/MiscTests.java, it's for verifying a bug in
launcher, so I have stick with the internal API as I am not sure if
switching to public API may be too great of a change as the call path
changed significantly...
Thanks,
Valerie
On 5/26/2015 3:27 PM, Mandy Chung wrote:
sun/security/jca/ProviderConfig.java
287 /**
288 * Loads the provider with the specified class name.
289 *
290 * @param name the name of the provider class
291 * @return the Provider, or null if it cannot be found or
loaded
292 * @throws ProviderException all other exceptions are
ignored
293 */
294 public Provider load(String classname) {
:
305 if (p.getClass().getName().equals(classname)) {
306 return p;
307 }
Is this load method supposed to take the provider name instead of the
classname?
line 305 should check against p.getName() instead? The legacyLoad
method is
for the fallback to the class name.
java.security now uses classname. Did you decide to use security
provider name instead?
Even so, the legacyLoader method should be able to find them and the
load method
looks to me should check the provider name instead.
test/java/lang/SecurityManager/CheckSecurityProvider.java
you add this test to run with security manager. I wonder if you want
to run with and without security manager through @run othervm.
Now each security provider has a name. Should this test verify the
provider
name as well?
test/tools/launcher/MiscTests.java
- does this test need to call sun.security.pkcs11 internal API? Can
this be
changed to call public API?
I didn't review other files.
Mandy
On 05/26/2015 01:57 PM, Sean Mullan wrote:
This all looks fine to me (except for the Makefile stuff which I'll
leave to others).
--Sean
On 05/21/2015 12:21 AM, Valerie Peng wrote:
Sean,
Could you please review this change? The changes are mostly the same as
the prototype in Jake, but I have to make some modification due to the
difference in ServiceLoader lookup in OpenJDK (corresponding
META-INF/services/java.security.Provider files in each module) and the
related makefile change (merge their content into one for the final
image build). Then, I adjusted the Provider.configure() method to
take a
single String argument to be consistent with the providerarg option
that keytool defined.
In addition, I also made some misc changes, such as configuring the
providers inside ProviderConfig instead of ProviderLoader, add back the
doPrivileged block to all the provider constructors. I also have second
thought on making the switch to privider name (instead of provider
class
name) in java.security file, so I reverted the changes on that - that
SunPKCS11 provider has its name specified in its configuration file, so
when ServiceLoader loads the PKCS11 provider, the configuration file
has
not been passed to it, so the name is not known at that time. Thus,
using the class name for the provider list entry seems to fit the flow
better. I also updated the default policy for SunPKCS11 provider given
its recent change of using sun.misc.
Webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/
CCC: http://ccc.us.oracle.com/7191662
Thanks,
Valerie
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
This all looks fine to me (except for the Makefile stuff which I'll leave to others). --Sean On 05/21/2015 12:21 AM, Valerie Peng wrote: Sean, Could you please review this change? The changes are mostly the same as the prototype in Jake, but I have to make some modification due to the difference in ServiceLoader lookup in OpenJDK (corresponding META-INF/services/java.security.Provider files in each module) and the related makefile change (merge their content into one for the final image build). Then, I adjusted the Provider.configure() method to take a single String argument to be consistent with the providerarg option that keytool defined. In addition, I also made some misc changes, such as configuring the providers inside ProviderConfig instead of ProviderLoader, add back the doPrivileged block to all the provider constructors. I also have second thought on making the switch to privider name (instead of provider class name) in java.security file, so I reverted the changes on that - that SunPKCS11 provider has its name specified in its configuration file, so when ServiceLoader loads the PKCS11 provider, the configuration file has not been passed to it, so the name is not known at that time. Thus, using the class name for the provider list entry seems to fit the flow better. I also updated the default policy for SunPKCS11 provider given its recent change of using sun.misc. Webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/ CCC: http://ccc.us.oracle.com/7191662 Thanks, Valerie
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
Thanks, I will sort out the Makefile stuff with Mandy and other build experts... Valerie On 5/26/2015 1:57 PM, Sean Mullan wrote: This all looks fine to me (except for the Makefile stuff which I'll leave to others). --Sean On 05/21/2015 12:21 AM, Valerie Peng wrote: Sean, Could you please review this change? The changes are mostly the same as the prototype in Jake, but I have to make some modification due to the difference in ServiceLoader lookup in OpenJDK (corresponding META-INF/services/java.security.Provider files in each module) and the related makefile change (merge their content into one for the final image build). Then, I adjusted the Provider.configure() method to take a single String argument to be consistent with the providerarg option that keytool defined. In addition, I also made some misc changes, such as configuring the providers inside ProviderConfig instead of ProviderLoader, add back the doPrivileged block to all the provider constructors. I also have second thought on making the switch to privider name (instead of provider class name) in java.security file, so I reverted the changes on that - that SunPKCS11 provider has its name specified in its configuration file, so when ServiceLoader loads the PKCS11 provider, the configuration file has not been passed to it, so the name is not known at that time. Thus, using the class name for the provider list entry seems to fit the flow better. I also updated the default policy for SunPKCS11 provider given its recent change of using sun.misc. Webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/ CCC: http://ccc.us.oracle.com/7191662 Thanks, Valerie
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
sun/security/jca/ProviderConfig.java
287 /**
288 * Loads the provider with the specified class name.
289 *
290 * @param name the name of the provider class
291 * @return the Provider, or null if it cannot be found or
loaded
292 * @throws ProviderException all other exceptions are ignored
293 */
294 public Provider load(String classname) {
:
305 if (p.getClass().getName().equals(classname)) {
306 return p;
307 }
Is this load method supposed to take the provider name instead of the
classname?
line 305 should check against p.getName() instead? The legacyLoad
method is
for the fallback to the class name.
java.security now uses classname. Did you decide to use security
provider name instead?
Even so, the legacyLoader method should be able to find them and the
load method
looks to me should check the provider name instead.
test/java/lang/SecurityManager/CheckSecurityProvider.java
you add this test to run with security manager. I wonder if you want
to run with and without security manager through @run othervm.
Now each security provider has a name. Should this test verify the
provider
name as well?
test/tools/launcher/MiscTests.java
- does this test need to call sun.security.pkcs11 internal API? Can
this be
changed to call public API?
I didn't review other files.
Mandy
On 05/26/2015 01:57 PM, Sean Mullan wrote:
This all looks fine to me (except for the Makefile stuff which I'll
leave to others).
--Sean
On 05/21/2015 12:21 AM, Valerie Peng wrote:
Sean,
Could you please review this change? The changes are mostly the same as
the prototype in Jake, but I have to make some modification due to the
difference in ServiceLoader lookup in OpenJDK (corresponding
META-INF/services/java.security.Provider files in each module) and the
related makefile change (merge their content into one for the final
image build). Then, I adjusted the Provider.configure() method to take a
single String argument to be consistent with the providerarg option
that keytool defined.
In addition, I also made some misc changes, such as configuring the
providers inside ProviderConfig instead of ProviderLoader, add back the
doPrivileged block to all the provider constructors. I also have second
thought on making the switch to privider name (instead of provider class
name) in java.security file, so I reverted the changes on that - that
SunPKCS11 provider has its name specified in its configuration file, so
when ServiceLoader loads the PKCS11 provider, the configuration file has
not been passed to it, so the name is not known at that time. Thus,
using the class name for the provider list entry seems to fit the flow
better. I also updated the default policy for SunPKCS11 provider given
its recent change of using sun.misc.
Webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/
CCC: http://ccc.us.oracle.com/7191662
Thanks,
Valerie
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 25/05/15 09:42, Erik Joelsson wrote: On 2015-05-22 18:53, Mandy Chung wrote: On 05/22/2015 08:09 AM, Alan Bateman wrote: On 22/05/2015 13:55, Chris Hegarty wrote: : I think it could be done either way. Valerie - have you considered not pushing the services configuration files with this change? With the change then the java.security configuration is still class names, not provider names, so the fallback should just work. This is what we've done in a few other areas (like JNDI for example). I wasn't aware of the other areas that move to service provider but remain being loaded with the fallback Class.forName. URL protocol handlers, and JDNI initial context, fall back to Class.forName for built-in handlers/implementations, and do not expose the implementation as services. I would prefer java.security should convert to use the provider names as an example and also exercise the code path using service I think URL protocol handlers works well as an example too. It is a minimal new service type, easy to understand. You can write your own Foo handler in a few lines of code. providers. If this causes much work to workaround it temporarily, I won't object the security providers are not truly service providers (no META-INF/services and java.security lists class name instead) Another option to workaround this: we only need to merge the service config files for generating the image. Can we have do the concatenation of jdk/modules/*/META-INF/services file and output to supports/image_gensrc before the images target and have the image builder to exclude all jdk/modules/*/META-INF/services files and take the supports/image_gensrc instead? This will remove the process-provider logic from Gensrc-*.gmk files. Would this be a better alternative? Maybe, I'm not sure. I still think solving this in java code in the ImageBuilder is the right thing to do. If it is agreed that these files are needed, then I can look at expanding the ImageBuilder to do concatenate them. -Chris. /Erik
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 2015-05-22 18:53, Mandy Chung wrote: On 05/22/2015 08:09 AM, Alan Bateman wrote: On 22/05/2015 13:55, Chris Hegarty wrote: : I think it could be done either way. Valerie - have you considered not pushing the services configuration files with this change? With the change then the java.security configuration is still class names, not provider names, so the fallback should just work. This is what we've done in a few other areas (like JNDI for example). I wasn't aware of the other areas that move to service provider but remain being loaded with the fallback Class.forName. I would prefer java.security should convert to use the provider names as an example and also exercise the code path using service providers. If this causes much work to workaround it temporarily, I won't object the security providers are not truly service providers (no META-INF/services and java.security lists class name instead) Another option to workaround this: we only need to merge the service config files for generating the image. Can we have do the concatenation of jdk/modules/*/META-INF/services file and output to supports/image_gensrc before the images target and have the image builder to exclude all jdk/modules/*/META-INF/services files and take the supports/image_gensrc instead? This will remove the process-provider logic from Gensrc-*.gmk files. Would this be a better alternative? Maybe, I'm not sure. I still think solving this in java code in the ImageBuilder is the right thing to do. /Erik
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 25/05/2015 09:53, Chris Hegarty wrote: If it is agreed that these files are needed, then I can look at expanding the ImageBuilder to do concatenate them. I agree with Mandy's point that java.security should be change to list the provider name rather than the class name. If that happens then it means that the service configuration files will be required. I don't have a strong view on whether the concatenation is done via make files or the image builder as it's all just temporary and will go away once resources are keyed by a module. One thing about rev'ing the image builder is that we should probably let the jimage refresh get into jdk9/dev first. I don't think we want to delay that due to merge conflicts. -Alan
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On May 25, 2015, at 3:00 AM, Alan Bateman alan.bate...@oracle.com wrote: On 25/05/2015 09:53, Chris Hegarty wrote: If it is agreed that these files are needed, then I can look at expanding the ImageBuilder to do concatenate them. I agree with Mandy's point that java.security should be change to list the provider name rather than the class name. If that happens then it means that the service configuration files will be required. I don't have a strong view on whether the concatenation is done via make files or the image builder as it's all just temporary and will go away once resources are keyed by a module. One thing about rev'ing the image builder is that we should probably let the jimage refresh get into jdk9/dev first. I don't think we want to delay that due to merge conflicts. Right this is all temporary. One benefit of having the concatenation done by image builder will get the makefile cleaned up and get ready for the resources keyed for a module as long as the work required is small. Good point about image refresh and any image builder change should come after jimage refresh to avoid causing any merge conflict. Depending on when the image refresh and this security provider change are ready to push to jdk9/dev, I can work with Valerie to determine whether to wait or phase it. thanks Mandy
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 2015-05-22 02:46, Mandy Chung wrote: I’m including build-dev and we need to ask for Erik and Magnus advice what’s the best way to work around this. Erik, Magnus, Security providers now become service providers. They are provided from 11 different modules, 3 of them are os-specific. The current image builder doesn’t merge duplicate resources and we currently work around it in the build doing the merge as it’s temporary until the module system is moving further. Gensrc-jdk.jdi.gmk is the example. Valerie has a patch attempting to concatenate them and it turns out that it might not be straight forward I have thought. http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/make/gensrc/Gensrc-java.naming.gmk.html As we can’t say which one image builder will pick, it needs to copy gensrc (merged version) to all modules/$MODULE/META-INF/services/java.security.Provider config files. I wonder if it’s quicker to hack ImageBuilder to special case the service config file and merge them. Any thought? I could certainly make this work in the makefile, but it is getting a bit tedious to keep these hacks going there. How hard would it be to change the ImageBuilder, which is just a temporary solution anyway, to identify and merge all service provider files with the same name that it finds? With that solution, the makefiles do not need to change as much when the module system is introduced and taking care of this properly. /Erik On May 21, 2015, at 3:03 PM, Valerie Peng valerie.p...@oracle.com wrote: Mandy, Please find comments in line. On 5/20/2015 10:39 PM, Mandy Chung wrote: A quick comment on the META-INF/services config files and the makefile. Merging the service config files is temporary until the module system is moving further along. src/jdk.crypto.ucrypto/solaris/classes/META-INF/services/java.security.Provider.html and other os-specific service configuration: 1 #[solaris]com.oracle.security.ucrypto.UcryptoProvider - why is this commented out? Does the makefile uncomment it? It should be simple concatenation with In an example that I found through another makefile, it would uncomment the entry start with #[OS] (and process/remove this prefix) when the OS matches. We need OS-specific file processing when concatenate these files. Ah… that’s from Gensrc-jdk.jdi.gmk. The service config file contains a mixture of cross-neutral and os-specific providers. That’s not the example to copy. define process-provider $(MKDIR) -p $(@D) $(CAT) $^ $@ endef What decides if it's appropriate or not? These are not just crypto providers that we are defining here, but all classes which extend from java.security.Provider. I recall using jdk.crypto.ec as the gensrc module as you suggested initially. But when testing it, it doesn't seem to work as expected. I ended up using java.naming as that's the one ended up in the final image instead of the concatenated one under jdk.crypto.ec. Could there be some alphabetic ordering when processing/building these modules? Well, since this is really a hack and only temporary, does it really matter whether it's under java.naming or jdk.crypto.ec? Both contains providers for the java.security provider list. The key thing is that the resulting image works Mandy
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 22/05/15 07:58, Erik Joelsson wrote: On 2015-05-22 02:46, Mandy Chung wrote: I’m including build-dev and we need to ask for Erik and Magnus advice what’s the best way to work around this. Erik, Magnus, Security providers now become service providers. They are provided from 11 different modules, 3 of them are os-specific. The current image builder doesn’t merge duplicate resources and we currently work around it in the build doing the merge as it’s temporary until the module system is moving further. Gensrc-jdk.jdi.gmk is the example. Valerie has a patch attempting to concatenate them and it turns out that it might not be straight forward I have thought. http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/make/gensrc/Gensrc-java.naming.gmk.html As we can’t say which one image builder will pick, it needs to copy gensrc (merged version) to all modules/$MODULE/META-INF/services/java.security.Provider config files. I wonder if it’s quicker to hack ImageBuilder to special case the service config file and merge them. Any thought? I could certainly make this work in the makefile, but it is getting a bit tedious to keep these hacks going there. How hard would it be to change the ImageBuilder, which is just a temporary solution anyway, to identify and merge all service provider files with the same name that it finds? With that solution, the makefiles do not need to change as much when the module system is introduced and taking care of this properly. I think it could be done either way. -Chris. /Erik On May 21, 2015, at 3:03 PM, Valerie Peng valerie.p...@oracle.com wrote: Mandy, Please find comments in line. On 5/20/2015 10:39 PM, Mandy Chung wrote: A quick comment on the META-INF/services config files and the makefile. Merging the service config files is temporary until the module system is moving further along. src/jdk.crypto.ucrypto/solaris/classes/META-INF/services/java.security.Provider.html and other os-specific service configuration: 1 #[solaris]com.oracle.security.ucrypto.UcryptoProvider - why is this commented out? Does the makefile uncomment it? It should be simple concatenation with In an example that I found through another makefile, it would uncomment the entry start with #[OS] (and process/remove this prefix) when the OS matches. We need OS-specific file processing when concatenate these files. Ah… that’s from Gensrc-jdk.jdi.gmk. The service config file contains a mixture of cross-neutral and os-specific providers. That’s not the example to copy. define process-provider $(MKDIR) -p $(@D) $(CAT) $^ $@ endef What decides if it's appropriate or not? These are not just crypto providers that we are defining here, but all classes which extend from java.security.Provider. I recall using jdk.crypto.ec as the gensrc module as you suggested initially. But when testing it, it doesn't seem to work as expected. I ended up using java.naming as that's the one ended up in the final image instead of the concatenated one under jdk.crypto.ec. Could there be some alphabetic ordering when processing/building these modules? Well, since this is really a hack and only temporary, does it really matter whether it's under java.naming or jdk.crypto.ec? Both contains providers for the java.security provider list. The key thing is that the resulting image works Mandy
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 22/05/2015 13:55, Chris Hegarty wrote: : I think it could be done either way. Valerie - have you considered not pushing the services configuration files with this change? With the change then the java.security configuration is still class names, not provider names, so the fallback should just work. This is what we've done in a few other areas (like JNDI for example). -Alan
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On 05/22/2015 08:09 AM, Alan Bateman wrote: On 22/05/2015 13:55, Chris Hegarty wrote: : I think it could be done either way. Valerie - have you considered not pushing the services configuration files with this change? With the change then the java.security configuration is still class names, not provider names, so the fallback should just work. This is what we've done in a few other areas (like JNDI for example). I wasn't aware of the other areas that move to service provider but remain being loaded with the fallback Class.forName. I would prefer java.security should convert to use the provider names as an example and also exercise the code path using service providers. If this causes much work to workaround it temporarily, I won't object the security providers are not truly service providers (no META-INF/services and java.security lists class name instead) Another option to workaround this: we only need to merge the service config files for generating the image. Can we have do the concatenation of jdk/modules/*/META-INF/services file and output to supports/image_gensrc before the images target and have the image builder to exclude all jdk/modules/*/META-INF/services files and take the supports/image_gensrc instead? This will remove the process-provider logic from Gensrc-*.gmk files. Would this be a better alternative? Mandy
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
But you are correct that the content which uses jdk.crypto.ec seems inconsistent to the file name. I will fix those and re-test. Thanks, Valerie On 5/21/2015 3:03 PM, Valerie Peng wrote: Mandy, Please find comments in line. On 5/20/2015 10:39 PM, Mandy Chung wrote: A quick comment on the META-INF/services config files and the makefile. Merging the service config files is temporary until the module system is moving further along. src/jdk.crypto.ucrypto/solaris/classes/META-INF/services/java.security.Provider.html and other os-specific service configuration: 1 #[solaris]com.oracle.security.ucrypto.UcryptoProvider - why is this commented out? Does the makefile uncomment it? It should be simple concatenation with In an example that I found through another makefile, it would uncomment the entry start with #[OS] (and process/remove this prefix) when the OS matches. We need OS-specific file processing when concatenate these files. The makefile doesn’t seem right though. make/gensrc/Gensrc-java.naming.gmk.html 96 jdk.crypto.ec: $(GENSRC_PROVIDER) 98 all: jdk.crypto.ec java.naming doesn’t seem an appropriate module to be the main module for containing all service provider config files. I initially propose to use jdk.crypto.ec as the gensrc module as indicated in line 96,98. What decides if it's appropriate or not? These are not just crypto providers that we are defining here, but all classes which extend from java.security.Provider. I recall using jdk.crypto.ec as the gensrc module as you suggested initially. But when testing it, it doesn't seem to work as expected. I ended up using java.naming as that's the one ended up in the final image instead of the concatenated one under jdk.crypto.ec. Could there be some alphabetic ordering when processing/building these modules? Well, since this is really a hack and only temporary, does it really matter whether it's under java.naming or jdk.crypto.ec? Both contains providers for the java.security provider list. The key thing is that the resulting image works. Valerie You can rename the file to Gensrc-jdk.crypto.ec and update the content.. GENSRC_PROVIDER := $(SUPPORT_OUTPUTDIR)/gensrc/java.naming/META-INF/services/java.security.Provider GENSRC_PROVIDER is the output file. line 79-89 is building the target list. I think you need another variable to build up the target list but not GENSRC_PROVIDER. You can reference how Gensrc-jdk.jdi.gmk concatenates the service config for jdk.jdi and dk.hotspot.agent module. # Filter com.sun.jdi.connect.Connector $(SUPPORT_OUTPUTDIR)/gensrc/jdk.jdi/META-INF/services/com.sun.jdi.connect.Connector: \ $(JDK_TOPDIR)/src/jdk.jdi/share/classes/META-INF/services/com.sun.jdi.connect.Connector \ $(SUPPORT_OUTPUTDIR)/gensrc/jdk.hotspot.agent/_the.sa.services $(process-provider) # Copy the same service file into jdk.hotspot.agent so that they are kept the same. $(JDK_OUTPUTDIR)/modules/jdk.hotspot.agent/META-INF/services/com.sun.jdi.connect.Connector: \ $(SUPPORT_OUTPUTDIR)/gensrc/jdk.jdi/META-INF/services/com.sun.jdi.connect.Connector $(install-file) Mandy
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
Mandy, Please find comments in line. On 5/20/2015 10:39 PM, Mandy Chung wrote: A quick comment on the META-INF/services config files and the makefile. Merging the service config files is temporary until the module system is moving further along. src/jdk.crypto.ucrypto/solaris/classes/META-INF/services/java.security.Provider.html and other os-specific service configuration: 1 #[solaris]com.oracle.security.ucrypto.UcryptoProvider - why is this commented out? Does the makefile uncomment it? It should be simple concatenation with In an example that I found through another makefile, it would uncomment the entry start with #[OS] (and process/remove this prefix) when the OS matches. We need OS-specific file processing when concatenate these files. The makefile doesn’t seem right though. make/gensrc/Gensrc-java.naming.gmk.html 96 jdk.crypto.ec: $(GENSRC_PROVIDER) 98 all: jdk.crypto.ec java.naming doesn’t seem an appropriate module to be the main module for containing all service provider config files. I initially propose to use jdk.crypto.ec as the gensrc module as indicated in line 96,98. What decides if it's appropriate or not? These are not just crypto providers that we are defining here, but all classes which extend from java.security.Provider. I recall using jdk.crypto.ec as the gensrc module as you suggested initially. But when testing it, it doesn't seem to work as expected. I ended up using java.naming as that's the one ended up in the final image instead of the concatenated one under jdk.crypto.ec. Could there be some alphabetic ordering when processing/building these modules? Well, since this is really a hack and only temporary, does it really matter whether it's under java.naming or jdk.crypto.ec? Both contains providers for the java.security provider list. The key thing is that the resulting image works. Valerie You can rename the file to Gensrc-jdk.crypto.ec and update the content.. GENSRC_PROVIDER := $(SUPPORT_OUTPUTDIR)/gensrc/java.naming/META-INF/services/java.security.Provider GENSRC_PROVIDER is the output file. line 79-89 is building the target list. I think you need another variable to build up the target list but not GENSRC_PROVIDER. You can reference how Gensrc-jdk.jdi.gmk concatenates the service config for jdk.jdi and dk.hotspot.agent module. # Filter com.sun.jdi.connect.Connector $(SUPPORT_OUTPUTDIR)/gensrc/jdk.jdi/META-INF/services/com.sun.jdi.connect.Connector: \ $(JDK_TOPDIR)/src/jdk.jdi/share/classes/META-INF/services/com.sun.jdi.connect.Connector \ $(SUPPORT_OUTPUTDIR)/gensrc/jdk.hotspot.agent/_the.sa.services $(process-provider) # Copy the same service file into jdk.hotspot.agent so that they are kept the same. $(JDK_OUTPUTDIR)/modules/jdk.hotspot.agent/META-INF/services/com.sun.jdi.connect.Connector: \ $(SUPPORT_OUTPUTDIR)/gensrc/jdk.jdi/META-INF/services/com.sun.jdi.connect.Connector $(install-file) Mandy
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
I’m including build-dev and we need to ask for Erik and Magnus advice what’s the best way to work around this. Erik, Magnus, Security providers now become service providers. They are provided from 11 different modules, 3 of them are os-specific. The current image builder doesn’t merge duplicate resources and we currently work around it in the build doing the merge as it’s temporary until the module system is moving further. Gensrc-jdk.jdi.gmk is the example. Valerie has a patch attempting to concatenate them and it turns out that it might not be straight forward I have thought. http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/make/gensrc/Gensrc-java.naming.gmk.html As we can’t say which one image builder will pick, it needs to copy gensrc (merged version) to all modules/$MODULE/META-INF/services/java.security.Provider config files. I wonder if it’s quicker to hack ImageBuilder to special case the service config file and merge them. Any thought? On May 21, 2015, at 3:03 PM, Valerie Peng valerie.p...@oracle.com wrote: Mandy, Please find comments in line. On 5/20/2015 10:39 PM, Mandy Chung wrote: A quick comment on the META-INF/services config files and the makefile. Merging the service config files is temporary until the module system is moving further along. src/jdk.crypto.ucrypto/solaris/classes/META-INF/services/java.security.Provider.html and other os-specific service configuration: 1 #[solaris]com.oracle.security.ucrypto.UcryptoProvider - why is this commented out? Does the makefile uncomment it? It should be simple concatenation with In an example that I found through another makefile, it would uncomment the entry start with #[OS] (and process/remove this prefix) when the OS matches. We need OS-specific file processing when concatenate these files. Ah… that’s from Gensrc-jdk.jdi.gmk. The service config file contains a mixture of cross-neutral and os-specific providers. That’s not the example to copy. define process-provider $(MKDIR) -p $(@D) $(CAT) $^ $@ endef What decides if it's appropriate or not? These are not just crypto providers that we are defining here, but all classes which extend from java.security.Provider. I recall using jdk.crypto.ec as the gensrc module as you suggested initially. But when testing it, it doesn't seem to work as expected. I ended up using java.naming as that's the one ended up in the final image instead of the concatenated one under jdk.crypto.ec. Could there be some alphabetic ordering when processing/building these modules? Well, since this is really a hack and only temporary, does it really matter whether it's under java.naming or jdk.crypto.ec? Both contains providers for the java.security provider list. The key thing is that the resulting image works Mandy
Re: RFR 7191662: JCE providers should be located via ServiceLoader,
On May 20, 2015, at 9:21 PM, Valerie Peng valerie.p...@oracle.com wrote: Sean, Could you please review this change? The changes are mostly the same as the prototype in Jake, but I have to make some modification due to the difference in ServiceLoader lookup in OpenJDK (corresponding META-INF/services/java.security.Provider files in each module) and the related makefile change (merge their content into one for the final image build). Then, I adjusted the Provider.configure() method to take a single String argument to be consistent with the providerarg option that keytool defined. In addition, I also made some misc changes, such as configuring the providers inside ProviderConfig instead of ProviderLoader, add back the doPrivileged block to all the provider constructors. I also have second thought on making the switch to privider name (instead of provider class name) in java.security file, so I reverted the changes on that - that SunPKCS11 provider has its name specified in its configuration file, so when ServiceLoader loads the PKCS11 provider, the configuration file has not been passed to it, so the name is not known at that time. Thus, using the class name for the provider list entry seems to fit the flow better. I also updated the default policy for SunPKCS11 provider given its recent change of using sun.misc. Webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/ CCC: http://ccc.us.oracle.com/7191662 http://ccc.us.oracle.com/7191662 A quick comment on the META-INF/services config files and the makefile. Merging the service config files is temporary until the module system is moving further along. src/jdk.crypto.ucrypto/solaris/classes/META-INF/services/java.security.Provider.html and other os-specific service configuration: 1 #[solaris]com.oracle.security.ucrypto.UcryptoProvider - why is this commented out? Does the makefile uncomment it? It should be simple concatenation with The makefile doesn’t seem right though. make/gensrc/Gensrc-java.naming.gmk.html 96 jdk.crypto.ec: $(GENSRC_PROVIDER) 98 all: jdk.crypto.ec java.naming doesn’t seem an appropriate module to be the main module for containing all service provider config files. I initially propose to use jdk.crypto.ec as the gensrc module as indicated in line 96,98. You can rename the file to Gensrc-jdk.crypto.ec and update the content.. GENSRC_PROVIDER := $(SUPPORT_OUTPUTDIR)/gensrc/java.naming/META-INF/services/java.security.Provider GENSRC_PROVIDER is the output file. line 79-89 is building the target list. I think you need another variable to build up the target list but not GENSRC_PROVIDER. You can reference how Gensrc-jdk.jdi.gmk concatenates the service config for jdk.jdi and dk.hotspot.agent module. # Filter com.sun.jdi.connect.Connector $(SUPPORT_OUTPUTDIR)/gensrc/jdk.jdi/META-INF/services/com.sun.jdi.connect.Connector: \ $(JDK_TOPDIR)/src/jdk.jdi/share/classes/META-INF/services/com.sun.jdi.connect.Connector \ $(SUPPORT_OUTPUTDIR)/gensrc/jdk.hotspot.agent/_the.sa.services $(process-provider) # Copy the same service file into jdk.hotspot.agent so that they are kept the same. $(JDK_OUTPUTDIR)/modules/jdk.hotspot.agent/META-INF/services/com.sun.jdi.connect.Connector: \ $(SUPPORT_OUTPUTDIR)/gensrc/jdk.jdi/META-INF/services/com.sun.jdi.connect.Connector $(install-file) Mandy
