Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal

[Vladimir Kozlov]

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

[Daniel Fuchs]

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

[Sean Mullan]

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

[Sean Mullan]

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

[Mandy Chung]

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

[Mandy Chung]

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

[Sean Mullan]

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

[Vladimir Kozlov]

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

[Vladimir Kozlov]

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

[Daniel Fuchs]

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

[Alan Bateman]

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

[Mandy Chung]

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

[Roger Riggs]

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

[Daniel Fuchs]

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

[Mandy Chung]

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

[Vladimir Kozlov]

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