Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
Thank you, Mandy On 6/20/19 6:20 PM, Mandy Chung wrote: Hi Vladimir, As long as the owner of the test review the patch and confirm the test policy intends to extend the default policy, those test change will be fine. test/jdk/java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java 417 DEFAULT_POLICY.implies(domain, permission)) return true; Nit: separating this to a new if-case after 426 to make it distinct. I did as you suggested: @@ -420,6 +422,7 @@ return true; } } +if (DEFAULT_POLICY.implies(domain, permission)) return true; return false; } I reviewed test/jdk/java/lang/Class/getDeclaredField/* and test/jdk/java/lang/invoke/* patch and they look fine. Thanks. My suggestion is to minimize the risk of your patch. Since Daniel has reviewed the logging test change (which is the bulk of this patch), I have no issue if the reviewers approve this patch. I will file a separate RFE for the test library idea. I will be pushing my changes since I don't see objections in reviews. Main discussion is how make these tests more robust which could be done separately. Thanks, Vladimir Mandy On 6/20/19 11:05 AM, Vladimir Kozlov wrote: Hi Mandy I am not sure about this suggestion. Graal module may not be present in the JDK (on SPARC for example). And I don't want pollute general tests with Graal specific code: ModulePolicy.get("jdk.internal.vm.compiler"). If you or other have concerns about some tests looking on default policy I can keep them on problem list to run them later only with libgraal. I found only 2 closed tests in which I had doubt about using default policy. I kept them on problem list. Other tests, as I understand, manipulate permissions for test classes. They don't extend system classes so default policy should not affect test class permissions. Thanks, Vladimir On 6/19/19 11:23 PM, Mandy Chung wrote: Hi Vladimir, Indeed these are test issues that the tests will need to grant permissions to jdk.internal.vm.compiler as the default policy grants. Thanks for going extra miles to fix the tests. My suggestion may be a bit general. What I intend to say the custom security policy should extend the default policy unless it intentionally excludes configuring permissions for specific modules. I review the the patch but the test doesn't clearly tell what the test intends to do w.r.t. security configuration. We should avoid inadvertently granting permissions that the test expects to disallow. A better solution is to limit granting permissions just for `jdk.internal.vm.compiler` module rather than all. Attached is ModulePolicy class that allows you to get the Policy for a specific module. It can be put in the test library that these tests can use them. So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and implies method will call the returned ModulePolicy if present. test/lib/jdk/test/lib/security is one existing testlibrary for security related stuff. You can consider putting ModulePolicy.java there. This is one idea. Mandy On 6/19/19 6:03 PM, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/8185139/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8185139 For Graal to work we have to give Graal module all permissions which is specified in default policy [1]. Unfortunately this cause problem for Graal running tests which overwrite default policy. I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her suggestion. Note, this is not only Graal problem. There were already similar fixes before [2]. I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable such tests again when libgraal is supported. I ran testing which execute these tests with Graal. It shows only known problems which are not related to these changes. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
Hi Sean, On 21/06/2019 15:36, Sean Mullan wrote: Ok, I see that is challenging, and I understand why you had to do that. You probably could have done something similar by breaking up the test into multiple classes in separate directories (or jars) with different ProtectionDomains, but that would require a bit of work, and maybe it is overkill. But I think then you could probably use the JDK Policy provider with a single policy file. (I'm not suggesting doing this right now, just was wondering if you considered this). Yes - I considered it - but then it makes the test much more difficult to understand (as its logic is split over several files) - and requires either invoking the compiler from within the test or moving classes around after jtreg has compiled them in a @driver command so that you can actually have different codesource locations. Using a custom Policy implementation looked much simpler at the time. And when you've done it once - well - copy & paste is your friend... If there comes a time where having a custom policy implementation becomes a maintenance issue and a technical debt then we/I will need to take what time is required to revisit. But I'd rather not do it if it's not a necessity. best regards, -- daniel
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
On 6/20/19 9:02 PM, Mandy Chung wrote: On 6/20/19 1:40 PM, Sean Mullan wrote: Sorry, I'm just catching up and looking at this now. The one thing I don't like about these tests that use their own Policy implementation is that the permissions that are granted inside the test are granted to all code, and not just the test, which may lead to cases where permissions that should be granted to JDK modules in default.policy may be missed. When the test wants a fine-control on turning on security manager programmatically and test various security permission combinations, I prefer to use a custom Policy implementation. Otherwise, I have to maintain multiple different test.policy files to test different permission combination which will be prone to editing error. I think we should provide a test library to help building a custom Policy implementation that can take the default policy, preferrably the policy for the resolved modules such that the test can focus its logic of security configuration and does not need to code with the system modules in mind. It seems a little related to https://bugs.openjdk.java.net/browse/JDK-8080294 --Sean That's what I started with ModulePolicy and could potentially build up test library for configuring security permissions programmatically. Mandy In tests that use policy files, we should grant permissions to only the test, for example: grant codebase "file:${test.classes}/.../-" { permission ...; }; However, in looking through our policy files, there are many that are not doing that. Something to fix, so I'll file a bug. This could also be fixed in these tests by inspecting the CodeSource of the ProtectionDomain. Or better yet, they should just use the jtreg java.security.policy option and a policy file and avoid the need to create a Policy implementation. Thanks, Sean On 6/20/19 11:04 AM, Mandy Chung wrote: The Policy API does not assume the presence of the default policy for the runtime to use. jdk.internal.vm.compiler already uses doPrivileged. The module ends up with no permission as the test policy does not consult the default policy. Mandy On 6/20/19 6:32 AM, Roger Riggs wrote: Hi, If this were java.base, we would use doPrivilege to ignore the policy and place specific limits. Encumbering the default policy with conditions needed by a trusted subsystem seems like distributing what should be a local implementation issue. $.02, Roger On 6/20/19 2:23 AM, Mandy Chung wrote: Hi Vladimir, Indeed these are test issues that the tests will need to grant permissions to jdk.internal.vm.compiler as the default policy grants. Thanks for going extra miles to fix the tests. My suggestion may be a bit general. What I intend to say the custom security policy should extend the default policy unless it intentionally excludes configuring permissions for specific modules. I review the the patch but the test doesn't clearly tell what the test intends to do w.r.t. security configuration. We should avoid inadvertently granting permissions that the test expects to disallow. A better solution is to limit granting permissions just for `jdk.internal.vm.compiler` module rather than all. Attached is ModulePolicy class that allows you to get the Policy for a specific module. It can be put in the test library that these tests can use them. So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and implies method will call the returned ModulePolicy if present. test/lib/jdk/test/lib/security is one existing testlibrary for security related stuff. You can consider putting ModulePolicy.java there. This is one idea. Mandy On 6/19/19 6:03 PM, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/8185139/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8185139 For Graal to work we have to give Graal module all permissions which is specified in default policy [1]. Unfortunately this cause problem for Graal running tests which overwrite default policy. I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her suggestion. Note, this is not only Graal problem. There were already similar fixes before [2]. I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable such tests again when libgraal is supported. I ran testing which execute these tests with Graal. It shows only known problems which are not related to these changes. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
On 6/21/19 7:15 AM, Daniel Fuchs wrote: Hi Sean, On 20/06/2019 21:40, Sean Mullan wrote: This could also be fixed in these tests by inspecting the CodeSource of the ProtectionDomain. Or better yet, they should just use the jtreg java.security.policy option and a policy file and avoid the need to create a Policy implementation. In what logging tests are concerned that would be quite an undertaking. For those tests, part of the test need to have permission to change the configuration, and part of the test need to not have the permission to double check that either the permission is not required for those operations, or that the permission is checked - if it is required. Hence the use of a custom Policy implementation that uses ThreadLocale to figure out whether to grant or deny permissions. Ok, I see that is challenging, and I understand why you had to do that. You probably could have done something similar by breaking up the test into multiple classes in separate directories (or jars) with different ProtectionDomains, but that would require a bit of work, and maybe it is overkill. But I think then you could probably use the JDK Policy provider with a single policy file. (I'm not suggesting doing this right now, just was wondering if you considered this). --Sean best regards, -- daniel Thanks, Sean On 6/20/19 11:04 AM, Mandy Chung wrote: The Policy API does not assume the presence of the default policy for the runtime to use. jdk.internal.vm.compiler already uses doPrivileged. The module ends up with no permission as the test policy does not consult the default policy. Mandy On 6/20/19 6:32 AM, Roger Riggs wrote: Hi, If this were java.base, we would use doPrivilege to ignore the policy and place specific limits. Encumbering the default policy with conditions needed by a trusted subsystem seems like distributing what should be a local implementation issue. $.02, Roger On 6/20/19 2:23 AM, Mandy Chung wrote: Hi Vladimir, Indeed these are test issues that the tests will need to grant permissions to jdk.internal.vm.compiler as the default policy grants. Thanks for going extra miles to fix the tests. My suggestion may be a bit general. What I intend to say the custom security policy should extend the default policy unless it intentionally excludes configuring permissions for specific modules. I review the the patch but the test doesn't clearly tell what the test intends to do w.r.t. security configuration. We should avoid inadvertently granting permissions that the test expects to disallow. A better solution is to limit granting permissions just for `jdk.internal.vm.compiler` module rather than all. Attached is ModulePolicy class that allows you to get the Policy for a specific module. It can be put in the test library that these tests can use them. So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and implies method will call the returned ModulePolicy if present. test/lib/jdk/test/lib/security is one existing testlibrary for security related stuff. You can consider putting ModulePolicy.java there. This is one idea. Mandy On 6/19/19 6:03 PM, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/8185139/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8185139 For Graal to work we have to give Graal module all permissions which is specified in default policy [1]. Unfortunately this cause problem for Graal running tests which overwrite default policy. I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her suggestion. Note, this is not only Graal problem. There were already similar fixes before [2]. I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable such tests again when libgraal is supported. I ran testing which execute these tests with Graal. It shows only known problems which are not related to these changes. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
Hi Vladimir, As long as the owner of the test review the patch and confirm the test policy intends to extend the default policy, those test change will be fine. test/jdk/java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java 417 DEFAULT_POLICY.implies(domain, permission)) return true; Nit: separating this to a new if-case after 426 to make it distinct. I reviewedtest/jdk/java/lang/Class/getDeclaredField/* and test/jdk/java/lang/invoke/* patch and they look fine. My suggestion is to minimize the risk of your patch. Since Daniel has reviewed the logging test change (which is the bulk of this patch), I have no issue if the reviewers approve this patch. I will file a separate RFE for the test library idea. Mandy On 6/20/19 11:05 AM, Vladimir Kozlov wrote: Hi Mandy I am not sure about this suggestion. Graal module may not be present in the JDK (on SPARC for example). And I don't want pollute general tests with Graal specific code: ModulePolicy.get("jdk.internal.vm.compiler"). If you or other have concerns about some tests looking on default policy I can keep them on problem list to run them later only with libgraal. I found only 2 closed tests in which I had doubt about using default policy. I kept them on problem list. Other tests, as I understand, manipulate permissions for test classes. They don't extend system classes so default policy should not affect test class permissions. Thanks, Vladimir On 6/19/19 11:23 PM, Mandy Chung wrote: Hi Vladimir, Indeed these are test issues that the tests will need to grant permissions to jdk.internal.vm.compiler as the default policy grants. Thanks for going extra miles to fix the tests. My suggestion may be a bit general. What I intend to say the custom security policy should extend the default policy unless it intentionally excludes configuring permissions for specific modules. I review the the patch but the test doesn't clearly tell what the test intends to do w.r.t. security configuration. We should avoid inadvertently granting permissions that the test expects to disallow. A better solution is to limit granting permissions just for `jdk.internal.vm.compiler` module rather than all. Attached is ModulePolicy class that allows you to get the Policy for a specific module. It can be put in the test library that these tests can use them. So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and implies method will call the returned ModulePolicy if present. test/lib/jdk/test/lib/security is one existing testlibrary for security related stuff. You can consider putting ModulePolicy.java there. This is one idea. Mandy On 6/19/19 6:03 PM, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/8185139/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8185139 For Graal to work we have to give Graal module all permissions which is specified in default policy [1]. Unfortunately this cause problem for Graal running tests which overwrite default policy. I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her suggestion. Note, this is not only Graal problem. There were already similar fixes before [2]. I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable such tests again when libgraal is supported. I ran testing which execute these tests with Graal. It shows only known problems which are not related to these changes. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
On 6/20/19 1:40 PM, Sean Mullan wrote: Sorry, I'm just catching up and looking at this now. The one thing I don't like about these tests that use their own Policy implementation is that the permissions that are granted inside the test are granted to all code, and not just the test, which may lead to cases where permissions that should be granted to JDK modules in default.policy may be missed. When the test wants a fine-control on turning on security manager programmatically and test various security permission combinations, I prefer to use a custom Policy implementation. Otherwise, I have to maintain multiple different test.policy files to test different permission combination which will be prone to editing error. I think we should provide a test library to help building a custom Policy implementation that can take the default policy, preferrably the policy for the resolved modules such that the test can focus its logic of security configuration and does not need to code with the system modules in mind. That's what I started with ModulePolicy and could potentially build up test library for configuring security permissions programmatically. Mandy In tests that use policy files, we should grant permissions to only the test, for example: grant codebase "file:${test.classes}/.../-" { permission ...; }; However, in looking through our policy files, there are many that are not doing that. Something to fix, so I'll file a bug. This could also be fixed in these tests by inspecting the CodeSource of the ProtectionDomain. Or better yet, they should just use the jtreg java.security.policy option and a policy file and avoid the need to create a Policy implementation. Thanks, Sean On 6/20/19 11:04 AM, Mandy Chung wrote: The Policy API does not assume the presence of the default policy for the runtime to use. jdk.internal.vm.compiler already uses doPrivileged. The module ends up with no permission as the test policy does not consult the default policy. Mandy On 6/20/19 6:32 AM, Roger Riggs wrote: Hi, If this were java.base, we would use doPrivilege to ignore the policy and place specific limits. Encumbering the default policy with conditions needed by a trusted subsystem seems like distributing what should be a local implementation issue. $.02, Roger On 6/20/19 2:23 AM, Mandy Chung wrote: Hi Vladimir, Indeed these are test issues that the tests will need to grant permissions to jdk.internal.vm.compiler as the default policy grants. Thanks for going extra miles to fix the tests. My suggestion may be a bit general. What I intend to say the custom security policy should extend the default policy unless it intentionally excludes configuring permissions for specific modules. I review the the patch but the test doesn't clearly tell what the test intends to do w.r.t. security configuration. We should avoid inadvertently granting permissions that the test expects to disallow. A better solution is to limit granting permissions just for `jdk.internal.vm.compiler` module rather than all. Attached is ModulePolicy class that allows you to get the Policy for a specific module. It can be put in the test library that these tests can use them. So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and implies method will call the returned ModulePolicy if present. test/lib/jdk/test/lib/security is one existing testlibrary for security related stuff. You can consider putting ModulePolicy.java there. This is one idea. Mandy On 6/19/19 6:03 PM, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/8185139/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8185139 For Graal to work we have to give Graal module all permissions which is specified in default policy [1]. Unfortunately this cause problem for Graal running tests which overwrite default policy. I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her suggestion. Note, this is not only Graal problem. There were already similar fixes before [2]. I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable such tests again when libgraal is supported. I ran testing which execute these tests with Graal. It shows only known problems which are not related to these changes. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
Sorry, I'm just catching up and looking at this now. The one thing I don't like about these tests that use their own Policy implementation is that the permissions that are granted inside the test are granted to all code, and not just the test, which may lead to cases where permissions that should be granted to JDK modules in default.policy may be missed. In tests that use policy files, we should grant permissions to only the test, for example: grant codebase "file:${test.classes}/.../-" { permission ...; }; However, in looking through our policy files, there are many that are not doing that. Something to fix, so I'll file a bug. This could also be fixed in these tests by inspecting the CodeSource of the ProtectionDomain. Or better yet, they should just use the jtreg java.security.policy option and a policy file and avoid the need to create a Policy implementation. Thanks, Sean On 6/20/19 11:04 AM, Mandy Chung wrote: The Policy API does not assume the presence of the default policy for the runtime to use. jdk.internal.vm.compiler already uses doPrivileged. The module ends up with no permission as the test policy does not consult the default policy. Mandy On 6/20/19 6:32 AM, Roger Riggs wrote: Hi, If this were java.base, we would use doPrivilege to ignore the policy and place specific limits. Encumbering the default policy with conditions needed by a trusted subsystem seems like distributing what should be a local implementation issue. $.02, Roger On 6/20/19 2:23 AM, Mandy Chung wrote: Hi Vladimir, Indeed these are test issues that the tests will need to grant permissions to jdk.internal.vm.compiler as the default policy grants. Thanks for going extra miles to fix the tests. My suggestion may be a bit general. What I intend to say the custom security policy should extend the default policy unless it intentionally excludes configuring permissions for specific modules. I review the the patch but the test doesn't clearly tell what the test intends to do w.r.t. security configuration. We should avoid inadvertently granting permissions that the test expects to disallow. A better solution is to limit granting permissions just for `jdk.internal.vm.compiler` module rather than all. Attached is ModulePolicy class that allows you to get the Policy for a specific module. It can be put in the test library that these tests can use them. So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and implies method will call the returned ModulePolicy if present. test/lib/jdk/test/lib/security is one existing testlibrary for security related stuff. You can consider putting ModulePolicy.java there. This is one idea. Mandy On 6/19/19 6:03 PM, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/8185139/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8185139 For Graal to work we have to give Graal module all permissions which is specified in default policy [1]. Unfortunately this cause problem for Graal running tests which overwrite default policy. I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her suggestion. Note, this is not only Graal problem. There were already similar fixes before [2]. I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable such tests again when libgraal is supported. I ran testing which execute these tests with Graal. It shows only known problems which are not related to these changes. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
Hi Mandy I am not sure about this suggestion. Graal module may not be present in the JDK (on SPARC for example). And I don't want pollute general tests with Graal specific code: ModulePolicy.get("jdk.internal.vm.compiler"). If you or other have concerns about some tests looking on default policy I can keep them on problem list to run them later only with libgraal. I found only 2 closed tests in which I had doubt about using default policy. I kept them on problem list. Other tests, as I understand, manipulate permissions for test classes. They don't extend system classes so default policy should not affect test class permissions. Thanks, Vladimir On 6/19/19 11:23 PM, Mandy Chung wrote: Hi Vladimir, Indeed these are test issues that the tests will need to grant permissions to jdk.internal.vm.compiler as the default policy grants. Thanks for going extra miles to fix the tests. My suggestion may be a bit general. What I intend to say the custom security policy should extend the default policy unless it intentionally excludes configuring permissions for specific modules. I review the the patch but the test doesn't clearly tell what the test intends to do w.r.t. security configuration. We should avoid inadvertently granting permissions that the test expects to disallow. A better solution is to limit granting permissions just for `jdk.internal.vm.compiler` module rather than all. Attached is ModulePolicy class that allows you to get the Policy for a specific module. It can be put in the test library that these tests can use them. So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and implies method will call the returned ModulePolicy if present. test/lib/jdk/test/lib/security is one existing testlibrary for security related stuff. You can consider putting ModulePolicy.java there. This is one idea. Mandy On 6/19/19 6:03 PM, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/8185139/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8185139 For Graal to work we have to give Graal module all permissions which is specified in default policy [1]. Unfortunately this cause problem for Graal running tests which overwrite default policy. I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her suggestion. Note, this is not only Graal problem. There were already similar fixes before [2]. I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable such tests again when libgraal is supported. I ran testing which execute these tests with Graal. It shows only known problems which are not related to these changes. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
I agree with Daniel. I did look into factoring SimplePolicy into a separate file but found, as Daniel pointed, there were some differences which would complicate this common class and would require more time to make such changes. Thank you Daniel, Roger and Alan for reviews. And special thanks to Mandy for helping me with this issue from start to finish. Thanks, Vladimir On 6/20/19 9:40 AM, Daniel Fuchs wrote: Hi Alan, On 20/06/2019 16:36, Alan Bateman wrote: I think that's right. In the case of the j.u.logging tests then maybe SimplePolicy could be factored out into a separate source file so there aren't 20 or so tests with a SimplePolicy and implies method that checks the default policy. I don't like this idea much as there can be some subtle differences between these SimplePolicy classes. I strongly hope that having to modify these test policy implementations is not something that we will need to do very often, and trying to factor out the SimplePolicy implementation is likely to be more trouble than it's worth. best regards, -- daniel -Alan
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
Hi Alan, On 20/06/2019 16:36, Alan Bateman wrote: I think that's right. In the case of the j.u.logging tests then maybe SimplePolicy could be factored out into a separate source file so there aren't 20 or so tests with a SimplePolicy and implies method that checks the default policy. I don't like this idea much as there can be some subtle differences between these SimplePolicy classes. I strongly hope that having to modify these test policy implementations is not something that we will need to do very often, and trying to factor out the SimplePolicy implementation is likely to be more trouble than it's worth. best regards, -- daniel -Alan
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
On 20/06/2019 07:23, Mandy Chung wrote: : My suggestion may be a bit general. What I intend to say the custom security policy should extend the default policy unless it intentionally excludes configuring permissions for specific modules. I think that's right. In the case of the j.u.logging tests then maybe SimplePolicy could be factored out into a separate source file so there aren't 20 or so tests with a SimplePolicy and implies method that checks the default policy. -Alan
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
The Policy API does not assume the presence of the default policy for the runtime to use. jdk.internal.vm.compiler already uses doPrivileged. The module ends up with no permission as the test policy does not consult the default policy. Mandy On 6/20/19 6:32 AM, Roger Riggs wrote: Hi, If this were java.base, we would use doPrivilege to ignore the policy and place specific limits. Encumbering the default policy with conditions needed by a trusted subsystem seems like distributing what should be a local implementation issue. $.02, Roger On 6/20/19 2:23 AM, Mandy Chung wrote: Hi Vladimir, Indeed these are test issues that the tests will need to grant permissions to jdk.internal.vm.compiler as the default policy grants. Thanks for going extra miles to fix the tests. My suggestion may be a bit general. What I intend to say the custom security policy should extend the default policy unless it intentionally excludes configuring permissions for specific modules. I review the the patch but the test doesn't clearly tell what the test intends to do w.r.t. security configuration. We should avoid inadvertently granting permissions that the test expects to disallow. A better solution is to limit granting permissions just for `jdk.internal.vm.compiler` module rather than all. Attached is ModulePolicy class that allows you to get the Policy for a specific module. It can be put in the test library that these tests can use them. So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and implies method will call the returned ModulePolicy if present. test/lib/jdk/test/lib/security is one existing testlibrary for security related stuff. You can consider putting ModulePolicy.java there. This is one idea. Mandy On 6/19/19 6:03 PM, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/8185139/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8185139 For Graal to work we have to give Graal module all permissions which is specified in default policy [1]. Unfortunately this cause problem for Graal running tests which overwrite default policy. I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her suggestion. Note, this is not only Graal problem. There were already similar fixes before [2]. I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable such tests again when libgraal is supported. I ran testing which execute these tests with Graal. It shows only known problems which are not related to these changes. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
Hi, If this were java.base, we would use doPrivilege to ignore the policy and place specific limits. Encumbering the default policy with conditions needed by a trusted subsystem seems like distributing what should be a local implementation issue. $.02, Roger On 6/20/19 2:23 AM, Mandy Chung wrote: Hi Vladimir, Indeed these are test issues that the tests will need to grant permissions to jdk.internal.vm.compiler as the default policy grants. Thanks for going extra miles to fix the tests. My suggestion may be a bit general. What I intend to say the custom security policy should extend the default policy unless it intentionally excludes configuring permissions for specific modules. I review the the patch but the test doesn't clearly tell what the test intends to do w.r.t. security configuration. We should avoid inadvertently granting permissions that the test expects to disallow. A better solution is to limit granting permissions just for `jdk.internal.vm.compiler` module rather than all. Attached is ModulePolicy class that allows you to get the Policy for a specific module. It can be put in the test library that these tests can use them. So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and implies method will call the returned ModulePolicy if present. test/lib/jdk/test/lib/security is one existing testlibrary for security related stuff. You can consider putting ModulePolicy.java there. This is one idea. Mandy On 6/19/19 6:03 PM, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/8185139/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8185139 For Graal to work we have to give Graal module all permissions which is specified in default policy [1]. Unfortunately this cause problem for Graal running tests which overwrite default policy. I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her suggestion. Note, this is not only Graal problem. There were already similar fixes before [2]. I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable such tests again when libgraal is supported. I ran testing which execute these tests with Graal. It shows only known problems which are not related to these changes. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
Hi Vladimir, On 20/06/2019 02:03, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/8185139/webrev.00/ Ah - I see that the logging test are the principal offenders here. Sorry about that ;-( I looked at the various logging tests and the proposed changes look good to me. Thanks for updating them! I skimmed through the rest and didn't see any particular issue. best regards, -- daniel https://bugs.openjdk.java.net/browse/JDK-8185139 For Graal to work we have to give Graal module all permissions which is specified in default policy [1]. Unfortunately this cause problem for Graal running tests which overwrite default policy. I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her suggestion. Note, this is not only Graal problem. There were already similar fixes before [2]. I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable such tests again when libgraal is supported. I ran testing which execute these tests with Graal. It shows only known problems which are not related to these changes. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
Hi Vladimir, Indeed these are test issues that the tests will need to grant permissions to jdk.internal.vm.compiler as the default policy grants. Thanks for going extra miles to fix the tests. My suggestion may be a bit general. What I intend to say the custom security policy should extend the default policy unless it intentionally excludes configuring permissions for specific modules. I review the the patch but the test doesn't clearly tell what the test intends to do w.r.t. security configuration. We should avoid inadvertently granting permissions that the test expects to disallow. A better solution is to limit granting permissions just for `jdk.internal.vm.compiler` module rather than all. Attached is ModulePolicy class that allows you to get the Policy for a specific module. It can be put in the test library that these tests can use them. So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and implies method will call the returned ModulePolicy if present. test/lib/jdk/test/lib/security is one existing testlibrary for security related stuff. You can consider putting ModulePolicy.java there. This is one idea. Mandy On 6/19/19 6:03 PM, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/8185139/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8185139 For Graal to work we have to give Graal module all permissions which is specified in default policy [1]. Unfortunately this cause problem for Graal running tests which overwrite default policy. I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her suggestion. Note, this is not only Graal problem. There were already similar fixes before [2]. I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable such tests again when libgraal is supported. I ran testing which execute these tests with Graal. It shows only known problems which are not related to these changes. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156 [2] https://bugs.openjdk.java.net/browse/JDK-8189291 import java.lang.module.*; import java.net.*; import java.util.HashMap; import java.util.Map; import java.util.Optional; import java.util.function.Function; import java.util.stream.*; import java.security.*; public class ModulePolicy extends Policy { private static final Policy DEFAULT_POLICY = Policy.getPolicy(); private static final Map modulePolicies = ModuleLayer.boot() .configuration().modules().stream() .map(rm -> new ModulePolicy(rm)) .collect(Collectors.toMap(ModulePolicy::name, Function.identity())); public static Optional get(String name) { return Optional.ofNullable(modulePolicies.get(name)); } final String name; final URL codeSourceUrl; final PermissionCollection permissions; private ModulePolicy(ResolvedModule rm) { URL url = createURL(rm.reference().location().orElseThrow()); CodeSource cs = new CodeSource(url, (CodeSigner[]) null); this.name = rm.name(); this.codeSourceUrl = url; this.permissions = DEFAULT_POLICY.getPermissions(cs); } public String name() { return name; } private URL createURL(URI uri) { URL url = null; try { url = uri.toURL(); } catch (MalformedURLException | IllegalArgumentException e) { } return url; } @Override public PermissionCollection getPermissions(CodeSource cs) { return codeSourceUrl.equals(cs.getLocation()) ? permissions : new Permissions(); } @Override public boolean implies(ProtectionDomain domain, Permission perm) { return permissions.implies(perm); } }
[14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
http://cr.openjdk.java.net/~kvn/8185139/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8185139 For Graal to work we have to give Graal module all permissions which is specified in default policy [1]. Unfortunately this cause problem for Graal running tests which overwrite default policy. I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her suggestion. Note, this is not only Graal problem. There were already similar fixes before [2]. I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable such tests again when libgraal is supported. I ran testing which execute these tests with Graal. It shows only known problems which are not related to these changes. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156 [2] https://bugs.openjdk.java.net/browse/JDK-8189291