Add initial entitlement policy parsing #114448

jdconrad · 2024-10-09T19:19:11Z

This change adds entitlement policy parsing with the following design:

YAML file for readability and re-use of our x-content parsers
hierarchical structure to group entitlements under a single scope
no general entitlements without a scope or for the entire project

This is just the parsing logic. I would like early feedback before moving onto the immutable data structures we store the entitlements in for comparison.

Design decisions:

The parsing logic is contained inside a set of builders. I anticipate the builders will have another a method added to "build" and validate the parsed values into immutable objects after the parsing is completed. This will allow us to also convert module names into module objects once they are available.
I broke the builders into different policy, scope, and entitlement. This makes it easier to add scopes and entitlements rather than have a single giant builder.
I opted to do the x-content parsing by hand rather than use ConstructingObjectParser as this allows for better error messages and is often more intuitive in debugging with breakpoints if necessary.
I added a PolicyParserException to give consistent, detailed error messages throughout.

elasticsearchmachine · 2024-10-09T19:19:34Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

mark-vieira · 2024-10-09T19:24:03Z

libs/entitlement-runtime/build.gradle

-tasks.named('forbiddenApisMain').configure {
-  replaceSignatureFiles 'jdk-signatures'
+  testImplementation(project(":test:framework")) {
+    exclude group: 'org.elasticsearch', module: 'elasticsearch-entitlement-runtime'


Why would test-framework bring in the entitlement-runtime project? Does :server depend on this?

I didn't realize this project had been moved from libs to distribution. I'm working on fixing everything now when merging with main.

Yeah, I think you can just remove this exclude as it's unnecessary.

I think this is fixed with the main merge conflicts resolved.

prdoyle · 2024-10-09T20:38:13Z

...titlement-runtime/src/main/java/org/elasticsearch/entitlement/policy/EntitlementBuilder.java

+        this.policyParser = policyParser;
+    }
+
+    protected abstract void parseEntitlement() throws IOException;


What is the contract of this method? From the implementations, it seems to use the parser to validate the structure, but doesn't actually have any other side effects or return anything?

Is the idea that this will ultimately return whatever data structure we end up with?

I was thinking there would be two passes. One for parsing and one for building. The parsing one would validate things like is the structure correct and are these values strings, etc. The building would validate that the scopes exist and permissions have valid paths, etc then turn them into immutable structures that will be used for comparison. Open here to a single pass if we think that's better. Either way we will need to do a string lookup at some point to get the module object for comparison.

I see. I have the same feeling, it looks "strange" to have a method that has only side effects.
But in the context of your explanation it makes sense, if you think there will be a public build method, and this will be only used internally.
Maybe to make this clear these classes could be internal (package private?)

That sounds like an interesting separation, between syntax and semantics. Phase 1: make sure you can tell what they're asking for; phase 2: make sure they're asking for something reasonable.

I'm not 100% sure this is the right approach, yet, but have to start somewhere. Either way parsing code will be useful.

prdoyle · 2024-10-09T20:40:01Z

...titlement-runtime/src/test/java/org/elasticsearch/entitlement/policy/PolicyBuilderTests.java

+public class PolicyBuilderTests extends ESTestCase {
+
+    public void testPolicyBuilder() {
+        new PolicyBuilder("test-policy.yaml", PolicyBuilderTests.class.getResourceAsStream("test-policy.yaml")).buildPolicy();


This needs some sad-path tests too, with various kinds of invalid YAML structures.

Definitely!

In the past I have had some success with a tool called pitest to inspire unit test cases based on the sorts of bugs they would detect. It can be tricky to set up though.

prdoyle · 2024-10-09T20:40:37Z

...entitlement-runtime/src/test/resources/org/elasticsearch/entitlement/policy/test-policy.yaml

+policy:
+  - module:
+      name: entitlement-test
+      entitlements:


How do you envision this structure extending to support class-level grants?

I envisioned having sub objects of module for package and then for class with their own set of entitlements for those specific objects. @rjernst and I had chatted about having all scoping be at the top-level where scope was separate by a colon, but I thought the trade off of having them be sub objects made it easier for users to see what permissions belong where. I'm really open to changing this either way.

Oh I think I get it. This is the hierarchical structure you mentioned. I like it!

I think it makes sense. We can certainly make changes to simplify or move structure around if we want to.

ldematte

Looks like a very good start, thanks!
Looking forward to see the final shape, with the Builder -> immutable objects.
I have a slight concern with naming: they are not really what you would expect from a builder, are they? The only one vaguely resembling a "traditional" java builder is ModuleScopeBuilder.
Why don't we call them Parser or Reader?

ldematte · 2024-10-10T07:31:32Z

...lement-runtime/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyBuilder.java

+        }
+    }
+
+    public void buildPolicy() {


IMO this should be named consistently with the others (parsePolicy?) and be package private. So it's still accessible by tests, but it's clear that we will use build() instead (to be added).

prdoyle · 2024-10-10T12:59:45Z

distribution/tools/entitlement-runtime/build.gradle


 dependencies {
  compileOnly project(':libs:elasticsearch-core') // For @SuppressForbidden
+  compileOnly project(":libs:elasticsearch-x-content") // for parsing policy files


This is not an objection; just a thought...

Beyond the initialization phase, we don't need the ability to parse policy files anymore, so for 99% of its lifetime, the runtime library doesn't need this. This suggests that the parsing might be better in another module whose job is to generate (by whatever means) a policy object that is used by the runtime.

This sort of separation could enhance testing too, because the interface between them is an immutable data structure (records?). The parser module tests would feed in text and assert that the right structures are generated; the runtime module tests would feed in data structures and ensure that the right permissions are granted.

I'm not sure exactly what I'm suggesting here though. 🤔

I'm open to moving it for sure, but for now this still seems like the best starting point to me.

prdoyle · 2024-10-10T13:03:00Z

...ntime/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileEntitlementBuilder.java

+        super(policyName, scopeName, policyParser);
+    }
+
+    protected void parseEntitlement() throws IOException {


NGL, I didn't read the implementation here in detail. This seems like a fairly low-stakes situation; I assume you did smart things in here, and if we find out something isn't right, we can just fix it. An extensive test suite is probably better for demonstrating that this sort of method is right, as opposed to careful review.

I understand :) . Tests should cover this kind of thing typically.

prdoyle · 2024-10-10T13:04:21Z

...lement-runtime/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyBuilder.java

+                throw newPolicyParserException(policyParser.getTokenLocation(), policyName, "expected closing object");
+            }
+        } catch (IOException ioe) {
+            throw new UncheckedIOException(ioe);


Given that this is using an InputStream, should the method be declared throws IOException?

prdoyle · 2024-10-10T13:08:20Z

...untime/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyParserException.java

+
+import org.elasticsearch.xcontent.XContentLocation;
+
+public class PolicyParserException extends RuntimeException {


I'm not sure what Elastic's philosophy is on runtime exceptions. This strikes me as a situation where this feature (the policy parser) will be used in very few places--probably just one--and in that place, the caller is going to need to catch this exception and do something about it anyway. That means the drawbacks of checked exceptions are insignificant, and making it a checked exception might help remind future devs of the sharp edges here.

Most of our other parse exceptions are also runtime exceptions. I think I'd prefer to go with that precedent for now. If a policy fails, ES will not start anyway, so is should be pretty obvious to anyone adding code around this what happened.

prdoyle · 2024-10-10T13:10:09Z

...t-runtime/src/main/java/org/elasticsearch/entitlement/runtime/policy/EntitlementBuilder.java

+
+import java.io.IOException;
+
+public abstract class EntitlementBuilder {


I wonder, given the multi-phase parse-validate-build approach you mentioned in the comments... should this be called EntitlementParser?

++ (I suggested the same, Parser or Reader)

I renamed everything to parser. I had considered these builders for the validation/build step, but maybe what makes the most sense is to separate out the builders completely from the parsers. So it would end up being parser -> builder -> immutable policy object(s). If that's the case I'll have the parsers return builder objects. WDYT?

FWIW my hunch is that the extra step will pay off. There's a tendency to make parsers do a lot, but unless they're very simple, parsers are best left just determining what the text says rather than trying to validate that it makes sense, or trying to "execute" it by building some sort of efficient data structures.

It makes sense to parse into some convenient form, validate that, and then once we have all the validated grants, run a build step that generates a highly efficient data structure for entitlement checks.

It's not super clear though, because it's possible our situation will be simple enough that this distinction isn't pulling its weight, and the parser could just generate efficient data structures directly. 🤷

jdconrad · 2024-10-11T20:42:26Z

@ldematte @prdoyle Thanks for the reviews! I renamed everything to parser. I'm working on adding failure tests. Please let me know if there's anything else you would expect from this PR as I think this is a good start pointing to iterate from.

ldematte · 2024-10-14T10:17:52Z

@jdconrad I think this is a good start pointing
At this stage, I'm not sure if we need a separate set of builders too; I think you can start maybe with defining the immutable objects, and then see if you need extra steps to build them from the parsers. That will tell you if a set of separate builders is needed of if it is a non necessary complication.

jdconrad · 2024-10-14T21:27:03Z

@ldematte @prdoyle Good call on trying to start creating the immutable objects because after starting to code some of these coming from the parsers, I realized what I was considering the builders are actually the first stage of immutable objects. It's really like there's two stages of immutable where we don't have the classes loaded yet, and have to rely on strings vs where we do have all the classes loaded and we can use the module objects and class objects instead. I think this is worth discussing more.

prdoyle · 2024-10-15T13:25:23Z

Oh interesting. My recollection from our last meeting is that we'd need some sort of lazy resolution inside the permission checks themselves, since there's no straightforward way to identify a point in the initialization process where we know we'll have all the necessary modules/classes loaded.

The trouble is, making lazy resolution perform well is tricky. This is something that MutableCallSite was made for, but unfortunately that API is actually pretty hard to use from Java, being primarily targeted to non-Java languages on the JVM.

We could consider instrumenting the target methods with invokedynamic calls perhaps. 🤔

jdconrad · 2024-10-16T23:22:16Z

@ldematte @prdoyle I have updated this PR based on some feedback about removing future boilerplate. I have modified the design based on our discussions with the following significant changes:

The PolicyParser is now a single class that is much simpler.
All Entitlement classes are now expected to have a single constructor, and if available to plugins (or other external policy files) expected to annotate that constructor with ExternalEntitlement. This allows the parser to reflectively find available entitlements meaning we do not need any additional parsing as entitlements are potentially added. (Thus, way less boilerplate.)
Each entitlement is expected to do its own validation as part of its constructor. This allows the parser to stay simple, and validation to occur for internally built entitlements such as we expect to do with system exit.
This PR contains the very basic immutable classes Policy, Scope, and Entitlement. This uses FileEntitlement as the initial example with several tests build around it.
My expectation is that the immutable classes will be updated either lazily or after plugin classes are available in optimized versions, but that is certainly for a follow up.
Scope can be extended to have a child scope if we need it, but for now the assumption is that the only supported scope is modules.

rjernst

I have a few more suggestions, but this looks pretty good to me!

rjernst · 2024-10-16T23:33:36Z

...ment-runtime/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileEntitlement.java

+import java.util.List;
+import java.util.Objects;
+
+public class FileEntitlement implements Entitlement {


Perhaps this could be a record? That would remove more boilerplate. But that could be in a followup.

I will leave this as a follow up because I'm not sure we don't want to add some kind of entitlement comparison in each one where this one would take in a path and an action to see if it's permitted. (Not a new object.)

rjernst · 2024-10-16T23:33:56Z

...itlement-runtime/src/main/java/org/elasticsearch/entitlement/runtime/policy/Entitlement.java

+
+package org.elasticsearch.entitlement.runtime.policy;
+
+public interface Entitlement {


Some javadocs would be nice for other devs to know this is a marker interface

rjernst · 2024-10-16T23:34:22Z

...-runtime/src/main/java/org/elasticsearch/entitlement/runtime/policy/ExternalEntitlement.java

+@Retention(RetentionPolicy.RUNTIME)
+public @interface ExternalEntitlement {
+
+    String[] parameterNames() default {};


Similar here, javadocs to explain why this parameterNames exists at all

rjernst · 2024-10-16T23:34:39Z

...-runtime/src/main/java/org/elasticsearch/entitlement/runtime/policy/ExternalEntitlement.java

+import java.lang.annotation.RetentionPolicy;
+
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ExternalEntitlement {


Also @Target.

Added both documentation and an @target for constructor.

rjernst · 2024-10-16T23:35:42Z

...ment-runtime/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileEntitlement.java

+    public static final int READ_ACTION = 0x1;
+    public static final int WRITE_ACTION = 0x2;
+
+    private final String path;


We'll need to access these, so they should be public or have accessors?

This depends on the future design if we end up doing entitlement checks as part of each entitlement class or not. I would prefer to change this in a follow up to match the design we decide on moving forward.

rjernst · 2024-10-16T23:42:24Z

...tlement-runtime/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyParser.java

+            );
+        } catch (ClassNotFoundException cnfe) {
+            throw newPolicyParserException(
+                policyParser.getTokenLocation(),


nit: The token location and policy name seem to be the same in all calls to these helper exception methods, could those be removed as args?

I cleaned this up with some additional helper methods.

rjernst · 2024-10-16T23:48:01Z

...ls/entitlement-runtime/src/main/java/org/elasticsearch/entitlement/runtime/policy/Scope.java

+import java.util.List;
+import java.util.Objects;
+
+public class Scope {


Could this be a record?

It depends on how we check entitlements moving forward. If they all become part of the top-level Policy class then I'm happy to change it then.

rjernst · 2024-10-16T23:49:52Z

...ime/src/test/java/org/elasticsearch/entitlement/runtime/policy/PolicyParserFailureTests.java

+
+    public void testEntitlementExtraneousParameter() throws IOException {
+        try (XContentBuilder builder = YamlXContent.contentBuilder()) {
+            builder.startObject();


It would be much easier to read these tests if they used multi line strings, so the file content is more understandable. I know we have used builders in tests in the past for much of xcontent, indexing, etc, but in those cases I think we care about varying the underlying xcontent type. Here we only care about yaml.

rjernst · 2024-10-16T23:51:00Z

...ent-runtime/src/test/resources/org/elasticsearch/entitlement/runtime/policy/test-policy.yaml

@@ -0,0 +1,7 @@
+entitlement-module-1:


nit: maybe make this "example-module-name", with the 1 it looks like this is an arbitrary id

ldematte

Looks really good to me!
I agree with Ryan that more javadoc would be very nice (possibly copy/pasting some of the explanations from comments on this PR too).
Other than that, it looks good to me; I agree that more things (adding other entitlements, the "final" immutable classes, child scopes) can be added later, when needed.

jdconrad · 2024-10-18T21:53:49Z

@ldematte @prdoyle @rjernst Thank you all for the reviews! I've updated the PR based on your latest feedback, and will merge as soon as CI passes. If there's anything I missed, I'm happy to change in a follow up.

This change adds entitlement policy parsing with the following design: * YAML file for readability and re-use of our x-content parsers * hierarchical structure to group entitlements under a single scope * no general entitlements without a scope or for the entire project

* Add initial entitlement policy parsing (#114448) This change adds entitlement policy parsing with the following design: * YAML file for readability and re-use of our x-content parsers * hierarchical structure to group entitlements under a single scope * no general entitlements without a scope or for the entire project * Avoid double instrumentation via class annotation (#115398) * Move entitlement jars to libs (#115883) The distribution tools are meant to be CLIs. This commit moves the entitlements jar projects to the libs dir, under a single libs/entitlement root directory to keep the related jars together. * Entitlement tools: SecurityManager scanner (#116020) * Dynamic entitlement agent (#116125) * Refactor: treat "maybe" JVM options uniformly * WIP * Get entitlement running with bridge all the way through, with qualified exports * Cosmetic changes to SystemJvmOptions * Disable entitlements by default * Bridge module comments * Fixup forbidden APIs * spotless * Rename EntitlementChecker * Fixup InstrumenterTests * exclude recursive dep * Fix some compliance stuff * Rename asm-provider * Stop using bridge in InstrumenterTests * Generalize readme for asm-provider * InstrumenterTests doesn't need EntitlementCheckerHandle * Better javadoc * Call parseBoolean * Add entitlement to internal module list * Docs as requested by Lorenzo * Changes from Jack * Rename ElasticsearchEntitlementChecker * Remove logging javadoc * exportInitializationToAgent should reference EntitlementInitialization, not EntitlementBootstrap. They're currently in the same module, but if that ever changes, this code would have become wrong. * Some suggestions from Mark --------- Co-authored-by: Ryan Ernst <ryan@iernst.net> * Remove unused EntitlementInternals (#116473) * Revert "Entitlement tools: SecurityManager scanner (#116020)" This reverts commit 023fb66. --------- Co-authored-by: Jack Conradson <osjdconrad@gmail.com> Co-authored-by: Lorenzo Dematté <lorenzo.dematte@elastic.co> Co-authored-by: Ryan Ernst <ryan@iernst.net>

jdconrad added 3 commits October 4, 2024 16:18

start of policy parser

d7cc7ba

initial parser for module scope

8aa4a5c

split builders into pieces and clean up error messaging

8d40a4d

jdconrad added >non-issue :Core/Infra/Core Core issues without another label labels Oct 9, 2024

jdconrad requested review from ldematte, prdoyle and rjernst October 9, 2024 19:19

jdconrad requested a review from a team as a code owner October 9, 2024 19:19

elasticsearchmachine added v9.0.0 Team:Core/Infra Meta label for core/infra team labels Oct 9, 2024

mark-vieira reviewed Oct 9, 2024

View reviewed changes

prdoyle reviewed Oct 9, 2024

View reviewed changes

Merge branch 'main' into policy

1f510d5

jdconrad requested a review from a team as a code owner October 9, 2024 21:38

ldematte reviewed Oct 10, 2024

View reviewed changes

prdoyle reviewed Oct 10, 2024

View reviewed changes

jdconrad added 2 commits October 11, 2024 13:26

Merge branch 'main' into policy

4297326

update based on pr comments

70a1e47

spotless

3ea1020

jdconrad added 2 commits October 14, 2024 13:10

add failure tests class

de4ce60

Merge branch 'main' into policy

a522870

jdconrad added 3 commits October 16, 2024 12:04

move packages

821dc3f

Merge branch 'main' into policy

d2c3ce0

update parser to remove boilerplate

2c0d2c1

rjernst reviewed Oct 16, 2024

View reviewed changes

ldematte approved these changes Oct 17, 2024

View reviewed changes

jdconrad added 2 commits October 18, 2024 14:11

Merge branch 'main' into policy

3adb0d3

changes based on pr feedback

4ed58a1

jdconrad merged commit 68f0f00 into elastic:main Oct 18, 2024
16 checks passed


		import org.elasticsearch.xcontent.XContentLocation;

		public class PolicyParserException extends RuntimeException {


		import java.io.IOException;

		public abstract class EntitlementBuilder {


		package org.elasticsearch.entitlement.runtime.policy;

		public interface Entitlement {

Add initial entitlement policy parsing #114448

Add initial entitlement policy parsing #114448

Uh oh!

Conversation

jdconrad commented Oct 9, 2024

Uh oh!

elasticsearchmachine commented Oct 9, 2024

Uh oh!

mark-vieira Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldematte Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldematte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdconrad commented Oct 11, 2024

Uh oh!

ldematte commented Oct 14, 2024

Uh oh!

jdconrad commented Oct 14, 2024

mark-vieira Oct 9, 2024 •

edited

Loading

ldematte Oct 10, 2024 •

edited

Loading