Revert 'Added jackson dependency to server" and change extension reading

[ryanbogan]

Signed-off-by: Ryan Bogan [email protected]

Description

Reverts #5366. Changes extension reading to use SnakeYAML instead of jackson-databind

Issues Resolved

#5504

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/9108/
• CommitID: 61c617d
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/9115/
• CommitID: 631255e
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[peternied]

I like it @ryanbogan, much cleaner

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/9120/
• CommitID: a9c63f9
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/9121/
• CommitID: 9eb43fa
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/9122/
• CommitID: 582c97d
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[uschindler]

Otherwise looks good! Thanks!

[uschindler]

You can fix this test in the following way: Add a setter marked as package-private (no access flags) in the s
Settings class to modify the default. By that only a class from the same package can modify the default value, not code anywjere outside.

WARNING: If the fields to modify are final, don't test this at all, kill this test!

[ryanbogan]

Failing test is flaky: #5766

[peternied]

Reverts #5366.

I disagree with remove jackson-databind - pragmatically this is a considerable burden on all the plugin times migrated to get Jackson from OpenSearch. Regardless of our feelings of the quality of jackson-databind, it is still popular in the OpenSearch project, complete removal will require effort on the verge of a release.

I suggest that all jackson dependencies should be transitively included by OpenSearch for security patching purposes.

[peternied]

I thought there was a way to keep the dependency in without elevated the permissions in server, is this not the case?

[uschindler]

Reverts #5366.

I disagree with remove jackson-databind - pragmatically this is a considerable burden on all the plugin times migrated to get Jackson from OpenSearch. Regardless of our feelings of the quality of jackson-databind, it is still popular in the OpenSearch project, complete removal will require effort on the verge of a release.

I suggest that all jackson dependencies should be transitively included by OpenSearch for security patching purposes.

For that we have BOM maven dependencies. You can have an Opensearch BOM that have fixed versions for jackson, but jackson-databind should never ever be in server's classpath.

The problem here is: whe each plugin has its own JAR file in its private classpath (shiedled by classloader), there are no malicious interactions between the instances as each only sees its classloader. Without sandboxing, jackson-databind can easily be used to create malicious code if ask it to deserialize classes, also those in server core. So, if jackson-databind is in server's classpath there is a chance that a malicious plugin may deserialize and trigger updates in fields of classes values in server's classpath.

If the plugin has its own copy of jackson databind, due to shielding it is not possible to update core's classes, especially when OpenSearch has moved to module system (Elasticsearch aleady did this).

[uschindler]

So please don't do this. Use a POM of opensearch to force versions for downstream code.

[uschindler]

I thought there was a way to keep the dependency in without elevated the permissions in server, is this not the case?

Yes it works, but a plugin can still trick jackson-databind to access server's classes if it is not only in its private module/classloader.

[peternied]

I thought there was a way to keep the dependency in without elevated the permissions in server, is this not the case?

Yes it works, but a plugin can still trick jackson-databind to access server's classes if it is not only in its private module/classloader.

Well damn. Thanks for the details. Lets get this in

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/9248/
• CommitID: 0675de8
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testDeleteOperations
      1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testCancelPrimaryAllocation
      1 org.opensearch.indices.replication.SegmentReplicationIT.testCancellation

• URL: https://build.ci.opensearch.org/job/gradle-check/9251/
• CommitID: 6b96909
  Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[opensearch-trigger-bot]

The backport to 2.5 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.5 2.5
# Navigate to the new working tree
pushd ../.worktrees/backport-2.5
# Create a new branch
git switch --create backport/backport-5768-to-2.5
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d22610b91729f596f273cdd44e836929f897f2e4
# Push it to GitHub
git push --set-upstream origin backport/backport-5768-to-2.5
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.5

Then, create a pull request where the base branch is 2.5 and the compare/head branch is backport/backport-5768-to-2.5.

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-5768-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d22610b91729f596f273cdd44e836929f897f2e4
# Push it to GitHub
git push --set-upstream origin backport/backport-5768-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-5768-to-2.x.

[uschindler]

Hi,

I thought there was a way to keep the dependency in without elevated the permissions in server, is this not the case?

Yes it works, but a plugin can still trick jackson-databind to access server's classes if it is not only in its private module/classloader.

Well damn. Thanks for the details. Lets get this in

I have to admit that without using the Java Module System, it is still possible that a plugin may change server's data using deep reflection, as only the module system prevents setAccessible() when the core does not "open" the module to specific submodules. So a stong deep reflection protection is only there if the Java moudle system is used (this was already suggested by @rmuir).

But the revert is also useful to actually migrate to the module system, because if you don't use a dependency it should never ever be part of the "requires" clauses of a module. Sure, you could exclude it in the module-info.java and still keep it in dependencies, but then downstream module users won't see the classes anyways, so adding it as a maven/gradle dependency would not bring anything.

So my proposal to proceed would be:

• Adapt Java Module System (JMS) - Elasticsearch already did this, gladly they had help by one of the OpenJDK committers hired by them
• Clean up dependency hell and also remove any JAR files from dependencies with are neither useable as real module or auto-module. In fact you have to decide here: Write a bit more code or add dependency hell entries. Sorry, jackson-databind is one of the dependencies to remove on longer term, although it is used often in open source. But somebody has to stop such outdated dependecies and move to "module system compatible ones" using no deep reflection and instead use MethodHandle.Lookup instances that were instantiated by the consumer code and passed to serialization lib to give it access to private APIs.
• As very very very last resort: In case of dependencies that cannot be included as real modules, shade them into another JAR file that actually uses it and make a module together with the classes using that nonmodule jar. This may be a good way to still use jackson-databind.

[dblock]

@uschindler As an additional option to JMS, a handful of folks (w/@saratvemulapalli) are working on extracting an SDK out of OpenSearch (https://github.com/opensearch-project/opensearch-sdk-java) and stop plugins from taking direct dependencies on OpenSearch or executing in the same JVM. Ultimately that should allow full sandboxing of plugins (extensions), including by containerizing them or running them on something like firecracker vm. Lots of words in #2447.