Add warning for misconfigured security manager #2510
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
| Nice! |
| The thrown exception is still there even when trying with |
| /test |
| return; | ||
| } | ||
| try { | ||
| sm.checkPermission(new AllPermission()); |
There was a problem hiding this comment.
What if someone has configured more fine-grained permissions, such as
permission java.lang.RuntimePermission "setFactory"; permission java.lang.RuntimePermission "getClassLoader"; permission java.lang.RuntimePermission "setContextClassLoader"; permission java.net.SocketPermission "*", "connect,resolve"; There was a problem hiding this comment.
I think here we can rely on the past experience of @eyalkoren with that: there are simply lots of them.
Even simple things like reading a system property need to be explicitly enabled when using fine-grained permissions, and some like SocketPermission might have their values depend on server_url in agent configuration that can't be read and thus be added to the policy snippet here.
If the need to provide fine-grained details arises, then we can do the extra effort of gathering (and maintaining) an exhaustive list.
There was a problem hiding this comment.
Trying to maintain a minimal set of permissions sounds like a terrible idea for me. Even if we compile such, any agent upgrade may break due to additional required permission.
I am personally fine with issuing this warning if AllPermission is not granted. If the user does maintain a proper policy with minimal permissions, the agent will work as expected and this warning can be ignored. On the other hand, if we just check specific permission here (e.g. try to getClassLoader or read a system property) and the permission set in the policy is not exhaustive, there won't be a warning and the agent will not work as expected, which is worse.
| return; | ||
| } | ||
| try { | ||
| sm.checkPermission(new AllPermission()); |
There was a problem hiding this comment.
Trying to maintain a minimal set of permissions sounds like a terrible idea for me. Even if we compile such, any agent upgrade may break due to additional required permission.
I am personally fine with issuing this warning if AllPermission is not granted. If the user does maintain a proper policy with minimal permissions, the agent will work as expected and this warning can be ignored. On the other hand, if we just check specific permission here (e.g. try to getClassLoader or read a system property) and the permission set in the policy is not exhaustive, there won't be a warning and the agent will not work as expected, which is worse.
| Could this have been implemented as a |
Yes, I initially thought of that, but by default we can't even read a system property, as a result we can't have a good hint for configuration:
|
3 participants
What does this PR do?
Issue a written warning in the process standard error output as there is nothing that the agent can do:
We have to do this very early in the agent startup as we can't even check for defined properties or use agent configuration (which would have been convenient to use the actual path to the java agent).
Checklist
./bin/catalina.sh run -security+ policy in./conf/catalina.policy1.29.0, thus it's not a regression.Thrown exception with Tomcat + security manager