twitter / util

Wonderful reusable code from Twitter
https://twitter.github.io/util
Apache License 2.0
2.69k stars 583 forks source link

Security permissions are not being checked in ContentionHandler first when using a TwitterServer #281

Closed alilewin closed 4 years ago

alilewin commented 4 years ago

When running some Finatra feature tests in scala for a TwitterServer, permissions in the JVM SecurityManager policy are not first being checked before trying to invoke that code. Is this expected, and if so is this something we want to change?

Expected behavior

When running some Finatra feature tests, I ran into this JVM security error:

                     Waiting for warmup phases to complete...
                     Waiting for warmup phases to complete...

                     Embedded server MyServer failed to startup: access denied ("java.lang.management.ManagementPermission" "control")
                     java.security.AccessControlException: access denied ("java.lang.management.ManagementPermission" "control")
                        at java.security.AccessControlContext.checkPermission(AccessControlContext.java:472)
                        at java.security.AccessController.checkPermission(AccessController.java:884)
                        at java.lang.SecurityManager.checkPermission(SecurityManager.java:549)
                        at sun.management.Util.checkAccess(Util.java:77)
                        at sun.management.Util.checkControlAccess(Util.java:85)
                        at sun.management.ThreadImpl.setThreadContentionMonitoringEnabled(ThreadImpl.java:189)
                        at com.twitter.jvm.ContentionSnapshot.<init>(ContentionSnapshot.scala:14)
                        at com.twitter.server.handler.ContentionHandler.<init>(ContentionHandler.scala:10)
                        at com.twitter.server.Admin$.adminRoutes(Admin.scala:67)
                        at com.twitter.server.AdminHttpServer$$anonfun$1.apply$mcV$sp(AdminHttpServer.scala:296)
                        at com.twitter.app.App$$anonfun$nonExitingMain$3.apply(App.scala:361)
                        at com.twitter.app.App$$anonfun$nonExitingMain$3.apply(App.scala:361)
                        at scala.collection.mutable.ResizableArray$class.foreach(ResizableArray.scala:59)
                        at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:48)
                        at com.twitter.app.App$class.nonExitingMain(App.scala:361)
                        at /path/to/my/server.MyServer.nonExitingMain(FakeServer.scala:17)
                        at com.twitter.inject.server.EmbeddedTwitterServer$$anonfun$runNonExitingMain$1$$anonfun$apply$mcV$sp$1.apply$mcV$sp(EmbeddedTwitterServer.scala:626)
                        at com.twitter.inject.server.EmbeddedTwitterServer.withLocals(EmbeddedTwitterServer.scala:597)
                        at com.twitter.inject.server.EmbeddedTwitterServer$$anonfun$runNonExitingMain$1.apply$mcV$sp(EmbeddedTwitterServer.scala:624)
                        at com.twitter.inject.server.EmbeddedTwitterServer$$anonfun$runNonExitingMain$1.apply(EmbeddedTwitterServer.scala:624)
                        at com.twitter.inject.server.EmbeddedTwitterServer$$anonfun$runNonExitingMain$1.apply(EmbeddedTwitterServer.scala:624)
                        at com.twitter.util.Try$.apply(Try.scala:26)
                        at com.twitter.util.ExecutorServiceFuturePool$$anon$4.run(FuturePool.scala:140)
                        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
                        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
                        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
                        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
                        at java.lang.Thread.run(Thread.java:748)

While I understand a fix would be to just update my SecurityManager policy, I'm filing this issue because I was told we should likely be checking permissions in the ContentionSnapshot code before trying to invoke the ManagementFactory (see last two paragraphs here for more details.

Actual behavior

As described above, it looks like we are not actually doing a permission check before trying to execute the code that triggers this.

Steps to reproduce the behavior

To get this error I simply ran my finatra feature tests with pants.

cacoco commented 4 years ago

@alilewin addressed this here: https://github.com/twitter/twitter-server/commit/ce783a3cadb9b27473006b59547adf22c534f5d9

Let us know if that doesn't work for you, it should be in our release next month.