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

[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 
<https://urldefense.com/v3/__https://github.com/openjdk/jdk17/pull/117*discussion_r659082598__;Iw!!ACWV5N9M2RV99hQ!d12Warg0hfv-NYucqgRxDuXw4PmjSksLRMhHR5EVxfKvS4tDEmBgHSX1cxoYzXyS1A$>:

> +        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 <coff...@openjdk.org> 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<Void>) () -> {
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<Void>) () -> {
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