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

[Valerie Peng]

On Tue, 22 Jun 2021 20:08:03 GMT, Sean Coffey <coffeys at 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