[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 <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