CreateClassloaderEntitlement + extensions to parse logic #117754
Conversation
ldematte
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -143,6 +150,14 @@ protected Entitlement parseEntitlement(String scopeName, String entitlementType) | |||
| } | |||
| } | |||
|
|
|||
| static String getEntitlementClassName(String entitlementType) { | |||
There was a problem hiding this comment.
Huh, this is pretty clever, but something feels odd about allowing the policy files to specify any class they want (though the fact that you put the mandatory Entitlement suffix on there helps).
There's also a performance problem here, where we look up the entitlement class by name, scan it for annotations, and reflect on its constructor arguments every time we encounter it in a policy file. Perhaps we could memoize those the first time we find them, so we do the reflection at most once per entitlement class. I suppose this is an optimization we could do later on.
Anyway, no need to change this if you like it the way it is. Just thinking out loud.
There was a problem hiding this comment.
This also allows multiple spellings of the same entitlement, like createClassloader and create_classloader.
There was a problem hiding this comment.
You are probably right -- but we want to discuss it in a follow up I think (definitely with Jack, who decided for this approach). It could be as simple as an allowlist of names though.
This also allows multiple spellings of the same entitlement, like createClassloader and create_classloader.
Yes, I'm really on the fence for this - if we have an allowlist of names it would limit this case.
For now, I'm thinking of changing it to just look for _ (if that's what we want to use as a separator).
Wdyt?
There was a problem hiding this comment.
TBH I'm fine even to merge this as-is and iterate on it. It's not like other teams will swoop in and start using these features before they're finalized anyway.
libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyParser.java
Show resolved
Hide resolved
| package org.elasticsearch.entitlement.runtime.policy; | ||
|
|
||
| public class CreateClassloaderEntitlement implements Entitlement { | ||
| @ExternalEntitlement |
There was a problem hiding this comment.
What is this annotation for? Does it differentiate entitlements that can appear in policy files versus those we just grant to the server?
There was a problem hiding this comment.
Thinking some more... if we instead had a list somewhere of "entitlements allowed in policy files" that would also allow us to scan those classes once at initialization, which would also solve the reflection perf problem I was mentioning.
There was a problem hiding this comment.
Does it differentiate entitlements that can appear in policy files versus those we just grant to the server?
Yes. These are the Entitlements that can be parsed.
There was a problem hiding this comment.
which would also solve the reflection perf problem I was mentioning.
That, I think, would be a very nice thing to add in a follow-up!
Create a simple CreateClassloader entitlement, that can be used in this format:
The PR expands the current parser to accept simple "just a string value" entitlements (previously they needed to be objects, with parameters).
It also refines the name mapping logic (entitlement name in YAML to class name) to account for "space" characters (like _ here). ATM any non-alpha character is considered a "space" character -- let me know if you think this is good or not (see tests).
This PR is small and simple on purpose: usages of CreateClassloader entitlement (in policies, code, IT tests) will be in a separate PR (I split this out as "preliminary work" to avoid having a gigantic PR).