Restrict apm agent entitlements to the system classloader unnamed module#120546
Restrict apm agent entitlements to the system classloader unnamed module#120546jdconrad merged 17 commits intoelastic:mainfrom
Conversation
system classloader
jdconrad
added
>bug
:Core/Infra/Plugins
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @jdconrad, I've created a changelog YAML for you. |
jdconrad
changed the title
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
Looks good, a couple suggestions
|
|
||
| private static PolicyManager policyManagerWithEntitlementsModule(Module entitlementsModule) { | ||
| return new PolicyManager(createEmptyTestServerPolicy(), List.of(), Map.of(), c -> "test", entitlementsModule); | ||
| private static PolicyManager policyManagerWithEntitlementsModule(Module agentsModule, Module entitlementsModule) { |
There was a problem hiding this comment.
nit, the method name is no longer accurate. Could this just be policyManager(...) now?
|
|
||
| var policyManager = policyManagerWithEntitlementsModule(entitlementsClass.getModule()); | ||
| var policyManager = policyManagerWithEntitlementsModule( | ||
| ClassLoader.getSystemClassLoader().getUnnamedModule(), |
There was a problem hiding this comment.
Instead of relying on the real classloader, can we create a test class in its own loader, similar to the above makeClassInItsOwnModule above? Then we can test that class gets the right policy when another class in an unnamed module does not.
There was a problem hiding this comment.
Added a test for a test agents module.
| List.of(), | ||
| Map.of("plugin1", createPluginPolicy("plugin.module")), | ||
| c -> "plugin1", | ||
| NO_ENTITLEMENTS_MODULE, |
There was a problem hiding this comment.
Rather than reuse NO_ENTITLEMENTS_MODULE, can we create a test specific agents module?
|
Looks good to me too, but ++ on all of Ryan's comments above |
|
@elasticmachine run elasticsearch-ci/part-2 |
ldematte
left a comment
ldematte
left a comment
There was a problem hiding this comment.
LGTM; I'd add one additional test, but that's up to you
| ModuleEntitlements agentsEntitlements = policyManager.getEntitlements(agentsClass); | ||
| assertThat(agentsEntitlements.hasEntitlement(CreateClassLoaderEntitlement.class), is(true)); | ||
| ModuleEntitlements notAgentsEntitlements = policyManager.getEntitlements(notAgentsClass); | ||
| assertThat(notAgentsEntitlements.hasEntitlement(CreateClassLoaderEntitlement.class), is(false)); |
There was a problem hiding this comment.
Nit: I'd like a check that uses two unnamed modules, to show that using named/unnamed is no longer a factor.
It should be simple, create 2 classloaders, load 2 classes (even the same) with them, and use classloader.getUnnamedModule() to pass to PolicyManager fot the agent module for one of the 2.
Even if it's the "same" class, one should pass, the other should not.
There was a problem hiding this comment.
That's a good idea! I will add this.
| <method v="2" /> | ||
| </configuration> | ||
| </component> | ||
| </component> No newline at end of file |
There was a problem hiding this comment.
Did you mean to commit this?
…le (elastic#120546) This change closes a hole where we assumed any check against an unnamed-module from any classloader was for one of our apm agent. This was not the case and made it so scripts could in theory have the same entitlements as apm agent. Instead we now check to see if a class is part of the apm package in an unnamed module to ensure it's actually for the apm agent. Relates to ES-10192
💚 Backport successful
|
…le (#120546) (#121054) This change closes a hole where we assumed any check against an unnamed-module from any classloader was for one of our apm agent. This was not the case and made it so scripts could in theory have the same entitlements as apm agent. Instead we now check to see if a class is part of the apm package in an unnamed module to ensure it's actually for the apm agent. Relates to ES-10192
5 participants
This change closes a hole where we assumed any check against an unnamed-module from any classloader was for one of our apm agent. This was not the case and made it so scripts could in theory have the same entitlements as apm agent. Instead we now check to see if a class in an unnamed module is part of the system classloader to ensure it's actually for the apm agent.
Relates to ES-10192