Fix entitlements in internalClusterTest #131539
Conversation
* Previously, entitlement checks got disabled when resetting the policy manager (which defaults to inactive). * The shared data dir is granted as additional data base directory. * Due to the lack of entitlement delegation and wipePendingDataDirectories using server's FileSystemUtils, node base directories won't be removed until after the test. * Disable entitlement checks for some command tests. * Disable entitlement checks for some tests requiring entitlement delegation.
mosche
added
>refactoring
test-windows
elasticsearchmachine
added
v9.2.0
Team:Core/Infra
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -367,9 +367,6 @@ private ModuleEntitlements getModuleScopeEntitlements( | |||
| * @return true if permission is granted regardless of the entitlement | |||
| */ | |||
| boolean isTriviallyAllowed(Class<?> requestingClass) { | |||
| if (generalLogger.isTraceEnabled()) { | |||
There was a problem hiding this comment.
logging in entitlements can be sketchy, especially on such a critical path. this breaks in a settings test that sets log level to trace
There was a problem hiding this comment.
We have encountered other such tests before, and managed to get them working without removing this logging. I'm not sure it's reasonable for a test to turn on trace logging globally and then make assertions about the logs that result.
An example is here. There was no particular reason that test needed to use the root logger, so I changed it to use a more specific logger.
There was a problem hiding this comment.
The trouble to me isn't the test, but (file) logging itself triggering further entitlement checks, especially on this path.
There was a problem hiding this comment.
This blew the logging framework in this case
There was a problem hiding this comment.
That sounds familiar too. 🤔
Could that happen in production? If so, we ought to fix it; if not, we ought to arrange things to make those entitlements trivially allowed. In either case, we shouldn't need to remove logging from PolicyManager.
There was a problem hiding this comment.
This can absolutely happen in production, though it should be very rare. Nevertheless I'd consider logging in isTriviallyAllowed rather dangerous and would rather remove, especially if it contains an exception.
What happens is:
- Java loads a new class via URLClassPath.
- We emit a trace log containing an exception in
isTriviallyAllowed - Serialization of that exception requires log4j to load another class (
ThrowableProxy) - We emit another trace log containing an exception in
isTriviallyAllowed - That blows log4j with a
ClassCircularityError
There was a problem hiding this comment.
If you insist, we can move the stacktrace down into
generalLogger.trace("Entitlement not trivially allowed");
once we've granted trusted system classes
There was a problem hiding this comment.
Yeah your scenario is a bit concerning. I'm good with moving it down if you are.
There was a problem hiding this comment.
(Oh and if you feel like you want to delete it, I can live with that too. I was just hesitating because being unable to log is a big problem for a system as complex as entitlements, so we just want to take some care before we resign ourselves to having "no logging zones" in the code.)
| @@ -115,20 +134,31 @@ private static Collection<Path> dataDirs(Settings settings, Path homeDir) { | |||
| : dataDirs.stream().map(TestEntitlementBootstrap::absolutePath).toList(); | |||
| } | |||
|
|
|||
| private static Path sharedDataDir(Settings settings) { | |||
There was a problem hiding this comment.
This is required for some tests, though it looks like we never grant PATH_SHARED_DATA_SETTING in production.
Is this a test-only thing? Or is that a bug?
| @@ -93,6 +94,7 @@ | |||
| import static org.hamcrest.Matchers.startsWith; | |||
|
|
|||
| @ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, numDataNodes = 0) | |||
| @ESTestCase.WithoutEntitlements // commands don't run with entitlements enforced | |||
There was a problem hiding this comment.
It's a bit unfortunate how often we need to do this. Makes me wonder if there's a more general rule we could apply so that WithoutEntitlements only needs to be used in exceptional cases. 🤔
I'll follow up with better managing the lifecycle of test entitlement state, as discussed on Slack.