Skip to content

Conversation

@ldematte
Copy link
Contributor

@ldematte ldematte commented Nov 13, 2024

This PR implements automatic entry-point description for Instrumenter, implementing Option 1 from Entitlement checks and entry-point definition.

It introduces one annotation (@InstrumentationTarget) to use on EntitlementChecker methods to specify which method in the JVM the check is targeting. The annotation will be used to populate a MethodKey, like before.

This PR also changes the way to describe the checker method to inject in the target prologue: before, it was a java.reflect.Method, now it's simple record named CheckerMethod.
This is because we decided to avoid using reflection, and use ASM to scan and find functions, so to avoid any chance of loading additional types before instrumentation.

EDIT:
After discussing options we the team, we came up with the idea to avoid using annotations and try and express everything in one place (in this case, the function signature, name + arguments).
The name and arguments of the checker will directly point to the class/method to instrument (always by populating a MethodKey, like before.

Examples:

void check$org_example_SomeClass$staticMethod(Class<?> callerClass, int arg)
--> static staticMethod(int) in org.example.SomeClass 
void check$$method(Class<?> callerClass, org.example.SomeClass that, int arg)
--> method(int) in org.example.SomeClass 

@ldematte ldematte added >non-issue :Core/Infra/Core Core issues without another label auto-backport Automatically create backport pull requests when merged v9.0.0 v8.17.0 labels Nov 13, 2024
@ldematte ldematte requested review from a team and jdconrad November 13, 2024 16:40
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Nov 13, 2024
@ldematte ldematte requested a review from prdoyle November 14, 2024 08:37
Copy link
Contributor

@prdoyle prdoyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First initial pass. I'm intending to take a second look...

@prdoyle
Copy link
Contributor

prdoyle commented Nov 18, 2024

The annotation name @InstrumentationTarget will be inconsistent with the terminology the code currently uses. We have:

  • The "instrumentation" or "check" method is the method in EntitlementChecks. (Going forward, I think the term "check" is probably preferred here.)
  • The "target" method is the method into which the instrumentation is inserted

In an earlier branch of mine, I used the name @CheckBefore. I think some variation of the word "check" would be preferable to "target". Perhaps just @Check?

@ldematte ldematte changed the title [Entitlements] Implement entry point definitions via annotations [Entitlements] Implement entry point definitions via checker function signature Nov 20, 2024
@ldematte
Copy link
Contributor Author

ldematte commented Nov 20, 2024

As discussed in the sync, removed annotations in favour of "name mangling", i.e. using only the function signature (name + arguments) to identify the method to instrument.
We can always re-introduce annotations at a later phase if e.g. we need to override this behaviour, like expressing names that do no play well with our mangling.

@ldematte
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Contributor

@prdoyle prdoyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some debatable style comments. If you disagree with all of them, I think this is mergeable as-is. 😂

String.format(
Locale.ROOT,
"Checker method %s has incorrect name format. "
+ "It should be either check$$methodName (instance) or check$package_ClassName$methodName (static)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, that's kind of clever, but I'm torn on it. On the one hand, this reduces redundancy because the class could already be derived from the argument list in the case of an instance method. But on the other hand, a uniform name-mangling convention seems desirable too. 🤔

@ldematte ldematte requested a review from rjernst November 20, 2024 15:49
@ldematte ldematte merged commit adcc5be into elastic:main Nov 21, 2024
15 of 16 checks passed
@ldematte ldematte deleted the entitlements/instrumentation-targets branch November 21, 2024 06:40
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.18 The branch "8.18" is invalid or doesn't exist

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 116754

rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Nov 22, 2024
rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Nov 22, 2024
elasticsearchmachine pushed a commit that referenced this pull request Nov 22, 2024
* [Entitlements] Consider only system modules in the boot layer (#117017)

* [Entitlements] Implement entry point definitions via checker function signature (#116754)

* Policy manager for entitlements (#116695)

* Add java version variants of entitlements checker (#116878)

As each version of Java is released, there may be additional methods we
want to instrument for entitlements. Since new methods won't exist in
the base version of Java that Elasticsearch is compiled with, we need to
hava different classes and compilation for each version.

This commit adds a scaffolding for adding the classes for new versions
of Java. Unfortunately it requires several classes in different
locations. But hopefully these are infrequent enough that the
boilerplate is ok. We could consider adding a helper Gradle task to
templatize the new classes in the future if it is too cumbersome. Note
that the example for Java23 does not have anything meaningful in it yet,
it's only meant as an example until we find go through classes and
methods that were added after Java 21.

* Spotless

---------

Co-authored-by: Lorenzo Dematté <lorenzo.dematte@elastic.co>
Co-authored-by: Jack Conradson <osjdconrad@gmail.com>
Co-authored-by: Patrick Doyle <patrick.doyle@elastic.co>
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.18.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants