Split PolicyChecker from PolicyManager #128004
Conversation
prdoyle
added
>refactoring
auto-backport
prdoyle
force-pushed
the
externalized-cache
branch
from
5d95452 to
60de9dc
Compare
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
prdoyle
force-pushed
the
externalized-cache
branch
2 times, most recently
from
6b023dc to
cba405d
Compare
prdoyle
force-pushed
the
externalized-cache
branch
from
caef5bc to
6d17a3a
Compare
prdoyle
force-pushed
the
externalized-cache
branch
from
6d17a3a to
8530cc1
Compare
There was a problem hiding this comment.
I'm neutral wrt to this design; it is different from what I thought we discussed (keep one PolicyManager, inject different behaviours for entitlements resolution in test vs prod) but it might be OK.
But if we go in this direction, there are a couple of things that did not need to change (see comments) and I think they need to be reverted.
...entitlement-test-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/PathActions.java
Show resolved
Hide resolved
...ment/src/main/java/org/elasticsearch/entitlement/initialization/EntitlementCheckerUtils.java
Outdated
Show resolved
Hide resolved
...nt/src/main/java/org/elasticsearch/entitlement/initialization/EntitlementInitialization.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public void check$java_net_URL$openConnection(Class<?> callerClass, java.net.URL that) { | ||
| checkEntitlementForUrl(callerClass, that); | ||
| policyChecker.checkEntitlementForUrl(callerClass, that); |
There was a problem hiding this comment.
I don't understand why these were moved - they were private to EntitlementChecker, why widen their visibility?
There was a problem hiding this comment.
(I prefer the way they are, and leave them out of the interface so it just have the "basic blocks", but I'm OK with moving them and exposing them if you feel strongly about it)
There was a problem hiding this comment.
Before, some of the check utility methods were in ElasticsearchEntitlementChecker and some were in PolicyManager, with no clear reason to pick one or the other. Now, all the check utilities are in PolicyChecker, leaving EntitlementChecker with nothing but the one-liner methods describing which entitlement is required for each sensitive JDK method.
| private static boolean isPathOnDefaultFilesystem(Path path) { | ||
| var pathFileSystemClass = path.getFileSystem().getClass(); | ||
| if (path.getFileSystem().getClass() != DEFAULT_FILESYSTEM_CLASS) { | ||
| PolicyManager.generalLogger.trace( |
There was a problem hiding this comment.
Nit: I find it strange this uses PolicyManager logger; I guess it is for having more consistent logs, but it can be confusing I think.
There was a problem hiding this comment.
Agreed, this is an unusual thing to do. I can change it to use PolicyCheckerImpl, especially since it's a trace log.
There was a problem hiding this comment.
I think I was concerned that we already have some alerting on PolicyManager logs and didn't want to worry about those. But we definitely don't have alerts on this one, so that reasoning isn't really sound.
There was a problem hiding this comment.
Your call :) I'm OK with keeping it unchanged if you are worried about something like alerts
💔 Backport failed
You can use sqren/backport to manually backport by running |
* Split PolicyChecker from PolicyManager * Restore EntitlementCheckerUtils * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>
* Split PolicyChecker from PolicyManager * Restore EntitlementCheckerUtils * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>
* Split PolicyChecker from PolicyManager * Restore EntitlementCheckerUtils * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>
* Split PolicyChecker from PolicyManager * Restore EntitlementCheckerUtils * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>
* Split PolicyChecker from PolicyManager * Restore EntitlementCheckerUtils * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>
* Split PolicyChecker from PolicyManager * Restore EntitlementCheckerUtils * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>
* Split PolicyChecker from PolicyManager (#128004) * Split PolicyChecker from PolicyManager * Restore EntitlementCheckerUtils * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]> * PolicyChecker change in JavaXX entitlement checkers --------- Co-authored-by: elasticsearchmachine <[email protected]>
PolicyManagerhas a number of responsibilities, including at least:checkmethods that determine whether some required entitlement is presentThis PR splits those into two objects, with the immediate consequence being that we can now swap in a
PolicyManagerForTestingthat modifies the behaviour.With this change, the design looks as follows:
EntitlementCheckerinterface has one method per sensitive JDK method. Its implementation (calledElasticsearchEntitlementChecker) determines which kind of check we should do for each method. Its methods are all one-liners that callPolicyChecker.PolicyCheckerinterface has one method per distinct kind of check (about 27 methods in total). Its implementation (PolicyCheckerImpl) has the job of determining the caller class, querying thePolicyManagerto determine what entitlements are granted to that class, and then checking whether the required entitlements are present.PolicyManagerclass determines, for a given caller class, what entitlements that class has been granted, and whether it is trivially allowed.It's
PolicyManagerthat implementsgetEntitlementsandisTriviallyAllowed. The idea is that we'd create aPolicyManagerForTestingthat would override these to provide the required semantics when running in junit.