Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v2]

2021-06-28 Thread Seán Coffey 

Hi Valerie,

many thanks for the thorough review. I've taken all your feedback on 
board with the latest push. Some of the test anomalies were a result of 
previous iterations of test edits I had been making.


Regarding the extra edits in 
"src/java.base/share/lib/security/default.policy", I had assumed it 
would be ok to tidy up the module under edit but I've reverted the 
unrelated changes now.


I was doubtful about removing the AccessController.doPrivileged blocks 
around the InnocuousThread.newSystemThread calls given that I wasn't 
sure which path the calling code would come from but on re-examination, 
I think it's ok to remove. The module provides the necessary permissions 
already and use of InnocuousThread solves the original permissions 
issue. Thanks for spotting!


In test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java 
:


> +if (args.length > 0) {
+Policy.setPolicy(new SimplePolicy());
+System.setSecurityManager(new SecurityManager());
+}


Just curious, why split the loop into 2 and set the SecurityManager in 
between the two loops? Can't we just set the policy/security manager 
before the loop?
The test infra requires various permissions including the problem 
setContextClassLoader permission. I figured it was better to set up the 
test infra first and then trigger the security manager.


New edits just pushed for review.

regards,
Sean.


On 25/06/2021 23:31, Valerie Peng wrote:

On Tue, 22 Jun 2021 20:08:03 GMT, Sean Coffey  wrote:


Sufficient permissions missing if this code was ever to run with 
SecurityManager.

Cleanest approach appears to be use of InnocuousThread to create the 
cleaner/poller threads.
Test case coverage extended to cover the SecurityManager scenario.

Reviewer request: @valeriepeng

Sean Coffey has updated the pull request incrementally with one additional 
commit since the last revision:

   Move TokenPoller to Runnable

src/java.base/share/lib/security/default.policy line 131:


129: permission java.lang.RuntimePermission 
"accessClassInPackage.com.sun.crypto.provider";
130: permission java.lang.RuntimePermission 
"accessClassInPackage.jdk.internal.misc";
131: permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.*";

Can we just do necessary changes? I noticed that this file seems to have mixed 
style, i.e. some lines are longer than 80 chars and some break into 2 lines 
with length less than 80 chars. Since the whole file is mixed, maybe just do 
what must be changed.

src/java.base/share/lib/security/default.policy line 142:


140: permission java.security.SecurityPermission 
"clearProviderProperties.*";
141: permission java.security.SecurityPermission "removeProviderProperty.*";
142: permission java.security.SecurityPermission 
"getProperty.auth.login.defaultCallbackHandler";

Same "avoid unnecessary changes" comment here.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
952:


950: AccessController.doPrivileged((PrivilegedAction) () -> {
951: Thread t = InnocuousThread.newSystemThread(
952: "Poller " + getName(),

nit: "Poller " -> "Poller-" (like before)?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
956:


954: assert t.getContextClassLoader() == null;
955: t.setDaemon(true);
956: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1033:


1031: }
1032: cleaner = new NativeResourceCleaner();
1033: AccessController.doPrivileged((PrivilegedAction) () -> {

It seems that the AccessController.doPrivileged((PrivilegedAction) () -> {} is 
un-necessary? I tried your test without it and test still passes.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1039:


1037: assert t.getContextClassLoader() == null;
1038: t.setDaemon(true);
1039: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1212:


1210:
1211: this.token = token;
1212: if (cleaner == null) {

This check seems duplicate to the one in createCleaner() call.

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 56:


54: System.out.println("No NSS config found. Skipping.");
55: return;
56: }

Move this if-check block of code up before the for-loop?

-

PR:

Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v2]

2021-06-25 Thread Valerie Peng 
On Tue, 22 Jun 2021 20:08:03 GMT, Sean Coffey  wrote:

>> Sufficient permissions missing if this code was ever to run with 
>> SecurityManager. 
>> 
>> Cleanest approach appears to be use of InnocuousThread to create the 
>> cleaner/poller threads.
>> Test case coverage extended to cover the SecurityManager scenario.
>> 
>> Reviewer request: @valeriepeng
>
> Sean Coffey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move TokenPoller to Runnable

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 63:

> 61: Policy.setPolicy(new SimplePolicy());
> 62: System.setSecurityManager(new SecurityManager());
> 63: }

Just curious, why split the loop into 2 and set the SecurityManager in between 
the two loops? Can't we just set the policy/security manager before the loop?

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 137:

> 135: perms.add(new SecurityPermission("insertProvider.*"));
> 136: perms.add(new SecurityPermission("removeProvider.*"));
> 137: }

The test still pass without the following permission:

 perms.add(new RuntimePermission("accessClassInPackage.sun.*"));
 perms.add(new 
RuntimePermission("accessClassInPackage.sun.security.pkcs11.*"));
 perms.add(new SecurityPermission("clearProviderProperties.*"));

Remove them?

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.sh line 142:

> 140: -Dtest.src=${TESTSRC} \
> 141: -Dtest.classes=${TESTCLASSES} \
> 142: -Djava.security.debug=${DEBUG} \

Save these java options and use it for both invocation? This way it's easier to 
tell that there is no difference among these two except for the extra argument.

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.sh line 143:

> 141: -Dtest.classes=${TESTCLASSES} \
> 142: -Djava.security.debug=${DEBUG} \
> 143: MultipleLogins ${TESTSRC}${FS}MultipleLogins.policy || exit 11

There is no MultipleLogins.policy file. The test just uses the internal 
SimplePolicy object. Maybe just use a string like "useSimplePolicy".

-

PR: https://git.openjdk.java.net/jdk17/pull/117

Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v2]

2021-06-25 Thread Valerie Peng 
On Fri, 25 Jun 2021 19:39:22 GMT, Valerie Peng  wrote:

>> Sean Coffey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Move TokenPoller to Runnable
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
> 952:
> 
>> 950: AccessController.doPrivileged((PrivilegedAction) () -> {
>> 951: Thread t = InnocuousThread.newSystemThread(
>> 952: "Poller " + getName(),
> 
> nit: "Poller " -> "Poller-" (like before)?

It seems that the AccessController.doPrivileged((PrivilegedAction) () -> 
{} is un-necessary? I tried your test without it and test still passes.

-

PR: https://git.openjdk.java.net/jdk17/pull/117

Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v2]

2021-06-25 Thread Valerie Peng 
On Tue, 22 Jun 2021 20:08:03 GMT, Sean Coffey  wrote:

>> Sufficient permissions missing if this code was ever to run with 
>> SecurityManager. 
>> 
>> Cleanest approach appears to be use of InnocuousThread to create the 
>> cleaner/poller threads.
>> Test case coverage extended to cover the SecurityManager scenario.
>> 
>> Reviewer request: @valeriepeng
>
> Sean Coffey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move TokenPoller to Runnable

src/java.base/share/lib/security/default.policy line 131:

> 129: permission java.lang.RuntimePermission 
> "accessClassInPackage.com.sun.crypto.provider";
> 130: permission java.lang.RuntimePermission 
> "accessClassInPackage.jdk.internal.misc";
> 131: permission java.lang.RuntimePermission 
> "accessClassInPackage.sun.security.*";

Can we just do necessary changes? I noticed that this file seems to have mixed 
style, i.e. some lines are longer than 80 chars and some break into 2 lines 
with length less than 80 chars. Since the whole file is mixed, maybe just do 
what must be changed.

src/java.base/share/lib/security/default.policy line 142:

> 140: permission java.security.SecurityPermission 
> "clearProviderProperties.*";
> 141: permission java.security.SecurityPermission 
> "removeProviderProperty.*";
> 142: permission java.security.SecurityPermission 
> "getProperty.auth.login.defaultCallbackHandler";

Same "avoid unnecessary changes" comment here.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
952:

> 950: AccessController.doPrivileged((PrivilegedAction) () -> {
> 951: Thread t = InnocuousThread.newSystemThread(
> 952: "Poller " + getName(),

nit: "Poller " -> "Poller-" (like before)?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
956:

> 954: assert t.getContextClassLoader() == null;
> 955: t.setDaemon(true);
> 956: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1033:

> 1031: }
> 1032: cleaner = new NativeResourceCleaner();
> 1033: AccessController.doPrivileged((PrivilegedAction) () -> {

It seems that the AccessController.doPrivileged((PrivilegedAction) () -> {} is 
un-necessary? I tried your test without it and test still passes.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1039:

> 1037: assert t.getContextClassLoader() == null;
> 1038: t.setDaemon(true);
> 1039: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1212:

> 1210: 
> 1211: this.token = token;
> 1212: if (cleaner == null) {

This check seems duplicate to the one in createCleaner() call.

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 56:

> 54: System.out.println("No NSS config found. Skipping.");
> 55: return;
> 56: }

Move this if-check block of code up before the for-loop?

-

PR: https://git.openjdk.java.net/jdk17/pull/117

Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v2]

2021-06-23 Thread Alan Bateman 
On Tue, 22 Jun 2021 20:08:03 GMT, Sean Coffey  wrote:

>> Sufficient permissions missing if this code was ever to run with 
>> SecurityManager. 
>> 
>> Cleanest approach appears to be use of InnocuousThread to create the 
>> cleaner/poller threads.
>> Test case coverage extended to cover the SecurityManager scenario.
>> 
>> Reviewer request: @valeriepeng
>
> Sean Coffey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move TokenPoller to Runnable

src/java.base/share/classes/module-info.java line 202:

> 200: jdk.charsets,
> 201: jdk.compiler,
> 202: jdk.crypto.cryptoki,

At some point I'd like to see this re-visited so we can avoid exporting this 
package to modules defined to the platform class loader.

-

PR: https://git.openjdk.java.net/jdk17/pull/117

Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v2]

2021-06-22 Thread Sean Coffey 
> Sufficient permissions missing if this code was ever to run with 
> SecurityManager. 
> 
> Cleanest approach appears to be use of InnocuousThread to create the 
> cleaner/poller threads.
> Test case coverage extended to cover the SecurityManager scenario.
> 
> Reviewer request: @valeriepeng

Sean Coffey has updated the pull request incrementally with one additional 
commit since the last revision:

  Move TokenPoller to Runnable

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/117/files
  - new: https://git.openjdk.java.net/jdk17/pull/117/files/2a168946..03af6494

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17=117=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=117=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/117.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/117/head:pull/117

PR: https://git.openjdk.java.net/jdk17/pull/117