-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Coroutines doesn't play well with JNLP security model #216
Comments
Are you sure it is a problem with coroutines? I'm not seeing any |
Hello @elizarov |
hi: Have yo try to execute http call (httpurlconnection) with AccessController.doProvileged??? Even if the JAR is signed, you must use AccessController.doPrivileged, JNLP or applet or in a sandbox with security manager enabled. |
@userquin I am aware of AccessController but I don't think that is a viable solution. The JNLP in question is signed and has the following tag which means it shouldn't need to request permissions:
The SwingWorker version and coroutine version executes exactly the same bit of code, the difference being that coroutine code behaves differently. |
try use this: fun String.fetch(): String = AccessController.doPrivileged(PrivilegedAction<String> {
URL(this).let {
var httpURLConnection: HttpURLConnection? = null
try {
httpURLConnection = it.openConnection() as HttpURLConnection
httpURLConnection.requestMethod = "GET"
BufferedReader(InputStreamReader(httpURLConnection.inputStream)).use {
it.readLines().joinToString("")
}
} catch (e: Exception) {
throw RuntimeException(e)
} finally {
httpURLConnection?.disconnect()
}
}
}) |
@userquin I know that will work, however I shouldn't have to do that. In our more complicated production app I had to add these calls all over the place where coroutines were used and in the end the application still didn't work because it ended up with a NullPointerException. I am looking for an explanation why in the example app that I have attached as part of this bug report the SwingWorker code works as expected and the coroutines code doesn't. |
ok, the problem may be the pool you are using to execute the coroutine, maybe providing your own context using on it a security manager... I'll trying to find the code I use... |
I have downloaded your zip and I have executed the jnlp from build\jnlp directory (enabling file:// in java control panel: Java 1.8) and HttpURLConnection works, Apache not working. The only difference from swing worker and coroutine is java web start prompts a confirm dialog to connect to google using coroutine (confirm in spanish): |
@userquin thanks for testing. You have just confirmed everything I have said in the original bug report. If you enable the JNLP console you will see the exception that happens with the apache version there. |
Also I should point out that you can open the project in Idea and view/edit the code to try and see if you can figure out why coroutines is breaking the JNLP security model. |
As I mention before, the problem is the pool that Swing (launch(Swing)) context uses: it must delegate on security manage or at least check if one exists and then enqueue the pool in the same group the security manager runs. The problem with its pool is that in its context there is no way to elevate required permissions, so you must create one that checks if a security manager. One simple way is provide to your pool a ThreadFactory; then check if a security manager exists, if so, then create threads using the same group as the security manager and remeber to create the thread with a runnable that switchs the context class loader to the loader that creates de access controller context required to elevate the privileges. In 5 minutes I provide an example... |
Executors.privilegedThreadFactory method can be used to create a new FixedThreadPool (also from Executors class) providing number of threads and privilegedThreadFactory as threadfactory. By default launch uses CommonPool: public val DefaultDispatcher: CoroutineDispatcher = CommonPool
...
public fun launch(
context: CoroutineContext = DefaultDispatcher, For example, kotlinx.coroutines.experimental.CommonPool. uses this: private fun createPool(): ExecutorService {
val fjpClass = Try { Class.forName("java.util.concurrent.ForkJoinPool") }
?: return createPlainPool()
if (!usePrivatePool) {
Try { fjpClass.getMethod("commonPool")?.invoke(null) as? ExecutorService }
?.let { return it }
}
Try { fjpClass.getConstructor(Int::class.java).newInstance(defaultParallelism()) as? ExecutorService }
?. let { return it }
return createPlainPool()
}
private fun createPool(): ExecutorService {
val threadId = AtomicInteger()
return Executors.newFixedThreadPool(defaultParallelism()) {
Thread(it, "CommonPool-worker-${threadId.incrementAndGet()}").apply { isDaemon = true }
}
} Create a new CoroutineDispatcher like CommonPool and just change the way it creates the pool: private fun createPool(): ExecutorService {
val threadId = AtomicInteger()
return Executors.newFixedThreadPool(defaultParallelism(), Executors.privilegedThreadFactory()) {
Thread(it, "PrivilegedCommonPool-worker-${threadId.incrementAndGet()}").apply { isDaemon = true }
}
} and finally just call launch with the new one CoroutineDispatcher created (copy/paste CommonPool and change only createPool and remove createPlainPool: you will use this on desktop environment, so using fork join does not matter): object PrivilegedCommonPool: CoroutineDispatcher() {
private fun createPool(): ExecutorService {
val threadId = AtomicInteger()
return Executors.newFixedThreadPool(defaultParallelism(), Executors.privilegedThreadFactory()) {
Thread(it, "PrivilegedCommonPool-worker-${threadId.incrementAndGet()}").apply { isDaemon = true }
}
}
} define your dispatcher and your privileged dispatcher: public val PrivilegedDispatcher: CoroutineDispatcher = PrivilegedCommonPool
...
public fun launchPrivileged(
context: CoroutineContext = PrivilegedDispatcher,
start: CoroutineStart = CoroutineStart.DEFAULT,
block: suspend CoroutineScope.() -> Unit
): Job {
val newContext = newCoroutineContext(context)
val coroutine = if (start.isLazy)
LazyStandaloneCoroutine(newContext, block) else
StandaloneCoroutine(newContext, active = true)
coroutine.initParentJob(context[Job])
start(block, coroutine, coroutine)
return coroutine
} and use it: launchPrivileged({
showResult(jFrame, CompletableFuture.supplyAsync {
getHtml()
}.await())
}) or you just name it launch and import your launch instead kotlin, the code will be the same, just change 1 or 2 imports in the file... |
ok, cannot be done, we must use DefaultExecutor it is internal... We must request this new feature to kotlin: @elizarov can this be included in kotlin coroutines (or in swing or jnlp extension)? Include in CoroutineContext.kt /**
* This is the privileged [CoroutineDispatcher] that is used by all standard builders like
* [launch], [async], etc if no dispatcher nor any other [ContinuationInterceptor] is specified in their context.
*
* It is currently equal to [PrivilegedCommonPool], but the value is subject to change in the future.
*/
public val PrivilegedDispatcher: CoroutineDispatcher = PrivilegedCommonPool New PrivilegedCommonPool.kt package kotlinx.coroutines.experimental
import java.util.concurrent.*
import kotlin.coroutines.experimental.CoroutineContext
object PrivilegedCommonPool: CoroutineDispatcher() {
@Volatile
private var _pool: Executor? = null
private fun createPool(): ExecutorService {
return Executors.newFixedThreadPool(defaultParallelism(), Executors.privilegedThreadFactory())
}
private fun defaultParallelism() = (Runtime.getRuntime().availableProcessors() - 1).coerceAtLeast(1)
@Synchronized
private fun getOrCreatePoolSync(): Executor =
_pool
?: createPool().also { _pool = it }
override fun dispatch(context: CoroutineContext, block: Runnable) =
try { (PrivilegedCommonPool._pool ?: PrivilegedCommonPool.getOrCreatePoolSync()).execute(timeSource.trackTask(block)) }
catch (e: RejectedExecutionException) {
timeSource.unTrackTask()
DefaultExecutor.execute(block)
}
// used for tests
@Synchronized
internal fun usePrivatePool() {
shutdown(0)
PrivilegedCommonPool._pool = null
}
// used for tests
@Synchronized
internal fun shutdown(timeout: Long) {
(PrivilegedCommonPool._pool as? ExecutorService)?.apply {
shutdown()
if (timeout > 0)
awaitTermination(timeout, TimeUnit.MILLISECONDS)
shutdownNow().forEach { DefaultExecutor.execute(it) }
}
PrivilegedCommonPool._pool = Executor { throw RejectedExecutionException("PrivilegedCommonPoolwas shutdown") }
}
// used for tests
@Synchronized
internal fun restore() {
shutdown(0)
PrivilegedCommonPool._pool = null
}
override fun toString(): String = "PrivilegedCommonPool"
} with these changes we can use launch(PrivilegedCommonPool) {...} when running on a sandbox (SecurityManager) solving the problem. by the way, how can I test this on intellij? |
I have a few questions here.
So, when |
Hello, It seems I am beginning to understand what is going on here. Kotlin Coroutines are using the ForkJoinPool and it seems that this pool creates threads that have no privileges. I found a very nice blog post which talks about this: I tried modifying the example code to use the ForkJoinPool directly and the bug reproduced. Unfortunately according to the article there doesn't appear to be a simple workaround. My suggestion right now is to allow some way of specifying a different threadPool other than the ForkJoinPool because as far as I can tell that thread pool cannot be configured to create threads with the correct security within JNLP. |
@NikolayMetchev but why HttpURLConnection is working? it uses the same thread pool Apache HttpClient sample uses. You must call HttpClient code with AccessController.doPrivileged and should work (see 2 below).
And yes, it would help if CommonPool uses this aproach if security manager is present |
@userquin SwingWorker works with both HttpURLConnection and apache HttpClient and there is no need for AccessController. As I said in my earlier comment the problem is the ForkJoinPool that cannot be used in JNLP if you wish to perform any privileged actions. |
HttpURLConnection just works if you grant the permission from the dialog, if you revoke it you just get the original exception (the same stack but with different context). The problem is not the common pool, is the way HttpClient is done (see how sun.net.www.protocol.http.HttpURLConnection uses AccessController.doPrivileged all the time). By the way, I suggest you to use always this method when doing privileged operations in a sandbox ;): getHTML = ForkJoinPool.commonPool-worker-1
Exception in thread "AWT-EventQueue-2" java.util.concurrent.CompletionException: java.security.AccessControlException: access denied ("java.net.SocketPermission" "www.google.com:80" "connect,resolve")
at java.util.concurrent.CompletableFuture.encodeThrowable(Unknown Source)
at java.util.concurrent.CompletableFuture.completeThrowable(Unknown Source)
at java.util.concurrent.CompletableFuture$AsyncSupply.run(Unknown Source)
at java.util.concurrent.CompletableFuture$AsyncSupply.exec(Unknown Source)
at java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(Unknown Source)
at java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
at java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
Caused by: java.security.AccessControlException: access denied ("java.net.SocketPermission" "www.google.com:80" "connect,resolve")
at java.security.AccessControlContext.checkPermission(Unknown Source)
at java.security.AccessController.checkPermission(Unknown Source)
at java.lang.SecurityManager.checkPermission(Unknown Source)
at com.sun.javaws.security.JavaWebStartSecurity.checkPermission(Unknown Source)
at java.lang.SecurityManager.checkConnect(Unknown Source)
at com.sun.javaws.security.JavaWebStartSecurity.checkConnectHelper(Unknown Source)
at com.sun.javaws.security.JavaWebStartSecurity.checkConnect(Unknown Source)
at sun.net.www.http.HttpClient.openServer(Unknown Source)
at sun.net.www.http.HttpClient.<init>(Unknown Source)
at sun.net.www.http.HttpClient.New(Unknown Source)
at sun.net.www.http.HttpClient.New(Unknown Source)
at sun.net.www.protocol.http.HttpURLConnection.getNewHttpClient(Unknown Source)
at sun.net.www.protocol.http.HttpURLConnection.plainConnect0(Unknown Source)
at sun.net.www.protocol.http.HttpURLConnection.plainConnect(Unknown Source)
at sun.net.www.protocol.http.HttpURLConnection.connect(Unknown Source)
at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(Unknown Source)
at sun.net.www.protocol.http.HttpURLConnection.getInputStream(Unknown Source)
at jnlp.MainKt.getHTML(main.kt:78)
at jnlp.MainKt$main$3.invoke(main.kt:28)
at jnlp.MainKt$main$3.invoke(main.kt)
at jnlp.MainKt$coroutine$1$1$1.get(main.kt:38)
at jnlp.MainKt$coroutine$1$1$1.get(main.kt)
... 6 more By the way, can you test if this works?: fun getHTMLWithApache(): String = AccessController.doPrivileged(PrivilegedAction<String> {
val connection = HttpClients.createDefault()
val execute = connection.execute(HttpGet("http://www.google.com"))
EntityUtils.toString(execute.entity)
}) |
I'm talking about corourines not swingworker. My previous respnse to @elizarov was wrong, it is not working with the forkjoin problem you mention.
|
By the way, in the previous stack trace you can see how the java web start stack request the permissions, but this only occurs if you use AccessController.doPrivileged call. Using HttpURLConnection, java do it for you but Apache HttpClient no. |
@userquin I wrote the sample app to demonstrate the problem. In the general case an app can choose to do all sorts of privileged actions in a coroutine and it may invoke 3rd party library code for which it may not necessarily be possible to correct the security context, by wrapping in an AccessController.doPriviledged(). In order to solve this in the general case I think we need to be able to override the threadPool used in co-routines (or at the very least the pool used in the Swing coroutines context). |
OK, that is when privilegedThreadFactory come into place!!!
|
@userquin are you sure your approach would work in the example app? The reason for using the Swing CoroutineDispatcher is that we want the updates to the GUI to be done on the AWT event thread. |
As a summary, it is not a problem with how CommonPool works, it only needs to fix the use of the threadgroup's security manager if one is present. If anyone tries to use a 3rd party library that does not check privileged operations via AccessController.doPrivileged calls, security manager cannot be bypassed, so depending on the context, things will work or not. In the case of coroutines we must take care with it, just because it will execute the code, so the context is different that using SwingWorker. In this scenario, you are trying to execute unprivileged code from 3rd party library in a way (coroutine) java does not allow you. With HttpURLConnection works just because jws request the permissions and because HttpURLConnection uses AccessController.doPrivileged. In the other hand, why is working both examples using SwingWorker? Just because SwingWorker takes care and create its own pool in the same secutity manager thread group. |
yes, it should work, wait response from @elizarov In your code you are using CommonPool not Swing: private fun coroutine(jFrame: JFrame, getHtml: () -> String): (Any) -> Unit {
return {
launch({
showResult(jFrame, CompletableFuture.supplyAsync {
getHtml()
}.await())
})
}
} |
upps, sorry, your launch is using Swing.... I'm going to review your code... With intellij 2017.1.5 cannot configure your projec., some gradle problem with proxy and url... |
Ahh, sorry I'm using the EAP |
it should work, Swing just declares the continuation: from Builders.launch documentation: If the context does not have any dispatcher nor any other [ContinuationInterceptor], then [DefaultDispatcher] is used. |
Not sure I follow. |
No, you just use launch(Swing) {}, kotlin must change CommonPool to configure the thread group from the security manager and you must always wrap your code with AccessController.doPrivileged. |
when launch create the coruotine using Swing as parameter, it would use internally CommonPool with ForkJoin executor... then the continuation will be invokeg throw Swing, that is EDT. |
It doesn't look like it's possible to change the thread group if you use the ForkJoinPool unless you do some hack. Not sure how you plan on changing CommonPool. |
We can change implementation of the |
I think it would be a better solution to allow enable it, via some hack in the dispatcher (for example a flag). In fact, it would be a better solution to include 2 flags, one to enable it and other to enable privileged thread factory (similar solution to increase the size of the pool). |
@userquin It don't think it is going to help in JNLP case, since with JNLP it is also notoriously difficult to pass any flags/system properties. I don't see how it is going to harm either. Who in the world uses |
OK, I agree with you, also it is the simple way to workaround the problem... For your info: |
Hello, However if I use I also then built my sample app with a modified CommonPool.kt that checks if a SecurityManager exists and create a privileged pool. This did indeed fix the problem. Would you like me to create a pull request with my suggested changes? |
I've created a pull request: |
The metod In my -humble- opinion a globally shared Privileged Executor is a big issue on JVM: you can call a privileged method only if you is albe to execute a privileged action. |
@fvasco Thanks for your comment. I wasn't too sure about a privileged thread factory either. For my example code to work the required change was just not to use |
I also agree with @fvasco to NOT use a privileged thread factory, you MUST call always AccessController.doPrivileged to elevate privileges and only change CommonPool to create the executor in the same security manager's thread group if one exists. In your PR you must use Executors.defaultThreadFactory() instead Executors.privilegedThreadFactory() |
I've amended the pull request to use |
Is it the following function enough to fix this issue?
I suppose there is a better way than execute always all code as privileged. Edit: this should work with Apache HTTP |
@fvasco when using 3rd party libs you must ALWAYS call the code as privileged (Java Web Start or Applet security manager). If you invoke the unprivileged code from a context where it is not allowed then you will have problems. In the context of @NikolayMetchev, I think (I must check it) there is no need to change CommonPool, and a call to apache http client code with doPrivileged must work, even with ForkJoin executor. If in the previous context, ForkJoin still fails, changing CommonPool to switch from ForkJoin to: Executors.newFixedThreadPool(defaultParallelism(), Executors.defaultThreadFactory()) when a SecurityManager is configured, calling apache http client code with doPrivileged should work. |
Hi @userquin, Unfortunately I don't have a toy project for your use case, however my proposal is to save the Security Context inside your application (withot sharing it) and to use it if and only if it is required (possibly without avoidable thread switch). I dump to you some code to sketch my idea.
|
Hi @fvasco Not exactly 100% but almost: the problem is use Swing as the continuation, with CommonPool as Disptacher (replacing javax.swing.SwingWorker). Your code is not exactly what originally requested: launch(Swing) {
[optionally try]
async task in common pool executor
continuation con EDT via Swing
[optionally catch]
continuation on EDT via Swing
} Using Android example: async(UI) {
try {
val response: Deferred<String> = bg { doWork() } // Common Pool
uiView.text = response.await() // continuation on UI thread
} catch(e: Exception) {
uiView.text = e.message // continuation on UI thread
}
} The rest of the history remains... |
jnlptest.zip
I have created and attached a simple swing app that uses JNLP and co-routines.
I have in this app code that uses both swingworker and co-routines to download the html from google.com.
It also does this using java.net.HttpURLConnection and apache httpclient.
The swingworker code when run through JNLP works just fine without any problems (as long as the jnlp is signed).
The co-routines code exhibits 2 different behaviours:
Exception in thread "AWT-EventQueue-2" java.util.concurrent.CompletionException: java.security.AccessControlException: access denied ("java.util.PropertyPermission" "java.version" "read")
at java.util.concurrent.CompletableFuture.encodeThrowable(Unknown Source)
at java.util.concurrent.CompletableFuture.completeThrowable(Unknown Source)
at java.util.concurrent.CompletableFuture$AsyncSupply.run(Unknown Source)
at java.util.concurrent.CompletableFuture$AsyncSupply.exec(Unknown Source)
at java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(Unknown Source)
at java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
at java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
Caused by: java.security.AccessControlException: access denied ("java.util.PropertyPermission" "java.version" "read")
at java.security.AccessControlContext.checkPermission(Unknown Source)
at java.security.AccessController.checkPermission(Unknown Source)
at java.lang.SecurityManager.checkPermission(Unknown Source)
at com.sun.javaws.security.JavaWebStartSecurity.checkPermission(Unknown Source)
at java.lang.SecurityManager.checkPropertyAccess(Unknown Source)
at java.lang.System.getProperty(Unknown Source)
at org.apache.http.util.VersionInfo.getUserAgent(VersionInfo.java:321)
at org.apache.http.impl.client.HttpClientBuilder.build(HttpClientBuilder.java:1046)
at org.apache.http.impl.client.HttpClients.createDefault(HttpClients.java:56)
at jnlp.MainKt.getHTMLWithApache(main.kt:88)
at jnlp.MainKt$main$4.invoke(main.kt:29)
at jnlp.MainKt$main$4.invoke(main.kt)
at jnlp.MainKt$coroutine$1$1$1.get(main.kt:38)
at jnlp.MainKt$coroutine$1$1$1.get(main.kt)
... 6 more
To reproduce you should be able to just unzip the directory and double click on build/jnlp/launch.jnlp
The text was updated successfully, but these errors were encountered: