Fork or replace elasticsearch dependencies

[nknize]

The elasticsearch build relies on the following libraries independently built by elastic.co:

• JNA
• SecureMock
• MockSocket
• Geolite2DB - Being tracked in a dedicated issue: Geolite Database dependency #404

This issue is to track the tasks needed to fork into the following repos:

• search-jna-build
• search-securemock
• search-mocksocket

• Mirror fork JNA
• Mirror fork Secure Mock
• Mirror fork MockSocket

Update the OpenSearch build files for these dependencies

• server/build.gradle
• client/rest/build.gradle
• client/sniffer/build.gradle
• gradle/local-distribution.gradle
• test/framework/build.gradle

Remove the exclusions for thirdPartyAudit task

• [BUG] Fix thirdPartyAudit gradle task for test/frameworks for OpenSearch. #420

[dblock]

We've initially forked, but chose to, for now, archive securemock, mocksocket and jna-build and use the existing dependencies.

• mocksocket can likely be upstreamed to Mockito (https://github.com/opensearch-project/securemock/issues/2), discuss
• jna-build output can likely be replaced by jna proper (https://github.com/opensearch-project/jna-build/issues/3)

[VachaShah]

securemock: The grant permissions feature provided by securemock does not look to be readily available in the new mockito version. We could either upstream the changes to mockito or fork the repo. If we successfully upstream the changes, we would also have to upgrade mockito version in OpenSearch since the version currently being used is 1.9.5.

mocksocket: The grant permissions feature on top of Socket and ServerSocket apis could also either be upstreamed to mockito or something similar that can house it. It could also be implemented as a utility in OpenSearch or the repo could be forked similar to securemock.

@dblock @saratvemulapalli @CEHENKLE thoughts?

[dblock]

Thanks @VachaShah,

1. What are these "grant permissions" features used for? Is this something we want in OpenSearch and why?
2. Assuming we want to keep these features, are they incremental on top of mockito?

If (2) is true, I think we should start by extracting the features into a new OSS library that is dependent on mockito, rather than a fork, so we can have a clear cut dependency. If that's not possible, definitely upstream.

Either way we want to be upgrading mockito, the version used here is ancient.

[VachaShah]

1. The grant permissions features are the main features provided by both the repos, for securemock: Allows creating mocks in tests without having to grant dangerous permissions to all of your code and for mocksocket: Allows using sockets without having to grant dangerous permissions to all of your code. They look like useful features but I am open to your thoughts!
2. For being incremental, the securemock and mocksocket repos are pretty old (last updated about 5 years ago with mockito 1.9.5) so if/when we put it on top of mockito, there might be quite a few changes since we would be using the latest mockito version. For one of the instances, I saw securemock is using some CGLIB functionality but mockito has moved away from it in the newer versions and they now use ByteBuddy. There might be other things as well which might need to be changed.

Definitely agree on upgrading the mockito version!

[dblock]

1. The grant permissions features are the main features provided by both the repos, for securemock: Allows creating mocks in tests without having to grant dangerous permissions to all of your code and for mocksocket: Allows using sockets without having to grant dangerous permissions to all of your code. They look like useful features but I am open to your thoughts!

Can you help me understand how that's useful? This is used in tests on throwaway CI infrastructure, and not at runtime. So why does one need it?

[VachaShah]

I think from what I understood from the repos, it helps avoid giving permissions to all of the code for mocking and for using sockets during tests. I see that this practice has also been used for other codebases in the test framework: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/resources/org/opensearch/bootstrap/test-framework.policy. Let me know if you think this might not be required for us.

[dblock]

I think from what I understood from the repos, it helps avoid giving permissions to all of the code for mocking and for using sockets during tests. I see that this practice has also been used for other codebases in the test framework: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/resources/org/opensearch/bootstrap/test-framework.policy. Let me know if you think this might not be required for us.

Ok, but why? ;) This is used in tests. What bad things happen if this code doesn't exist and those permissions are given? I have not read a good reason why one would want this. Maybe @nknize can explain?

[VachaShah]

Honestly, I am also unclear on what the consequences would be of removing it. But whether we decide to keep it or remove it, we can update the mockito version. If we decide to keep it though, there will be an interdependency where the functionalities of securemock will have to be implemented on the newer version of mockito and we would update the version of mockito on OpenSearch as well.

[xuezhou25]

Elasticsearch rebuilds JNA to strip out native libraries for platforms that not supported by elastic.
Are we going change JNA dependency from elastic rebuild version to a GA version and ignore the strip out action?
@dblock

[nknize]

Without mocksocket core opensearch would need to grant the following so integration tests could be executed:

grant {
  // give permission to **all** core code just for tests
  permission java.net.SocketPermission "*", "accept,connect,resolve";
};

These permissions would be carried forward in the production clusters making them vulnerable. With mocksocket those permissions are granted to the mocksocket jar only which is used in the MockNioTransport inside the test framework.

[dblock]

Thanks for the clear explanation @nknize, I understand why we need mocksocket. Let's reproduce this verbatim in our mocksocket fork's README or somewhere else where it's not going to be lost @VachaShah.

[dblock]

Elasticsearch rebuilds JNA to strip out native libraries for platforms that not supported by elastic.
Are we going change JNA dependency from elastic rebuild version to a GA version and ignore the strip out action?
@dblock

My vote would be yes.

I don't see why we would keep restricting platforms. While we may not be releasing a package that has been tested on some platform, it doesn't feel right to artificially prevent others from trying to run on those platforms, reporting bugs, etc. Looking for @nknize to agree/disagree here as well to make sure I'm not missing more history or context.

[VachaShah]

Thanks for the clear explanation @nknize, I understand why we need mocksocket. Let's reproduce this verbatim in our mocksocket fork's README or somewhere else where it's not going to be lost @VachaShah.

Sure, will do!

Without mocksocket core opensearch would need to grant the following so integration tests could be executed:

grant {
  // give permission to **all** core code just for tests
  permission java.net.SocketPermission "*", "accept,connect,resolve";
};

These permissions would be carried forward in the production clusters making them vulnerable. With mocksocket those permissions are granted to the mocksocket jar only which is used in the MockNioTransport inside the test framework.

@nknize Thanks for the explanation, that really helps! For securemock, it gives permissions to mockito during the tests, does this apply to securemock as well?

[adnapibar]

Elasticsearch rebuilds JNA to strip out native libraries for platforms that not supported by elastic.
Are we going change JNA dependency from elastic rebuild version to a GA version and ignore the strip out action?
@dblock

My vote would be yes.

I don't see why we would keep restricting platforms. While we may not be releasing a package that has been tested on some platform, it doesn't feel right to artificially prevent others from trying to run on those platforms, reporting bugs, etc. Looking for @nknize to agree/disagree here as well to make sure I'm not missing more history or context.

Here is the list of native libraries that JNA supports - https://github.com/java-native-access/jna/tree/master/lib/native
Does It makes sense to include these libraries for the platforms which we probably won't support (e.g. Android)?

[dblock]

Elasticsearch rebuilds JNA to strip out native libraries for platforms that not supported by elastic.
Are we going change JNA dependency from elastic rebuild version to a GA version and ignore the strip out action?
@dblock

My vote would be yes.
I don't see why we would keep restricting platforms. While we may not be releasing a package that has been tested on some platform, it doesn't feel right to artificially prevent others from trying to run on those platforms, reporting bugs, etc. Looking for @nknize to agree/disagree here as well to make sure I'm not missing more history or context.

Here is the list of native libraries that JNA supports - https://github.com/java-native-access/jna/tree/master/lib/native
Does It makes sense to include these libraries for the platforms which we probably won't support (e.g. Android)?

Those seem minor, or aren't they? Are we worried about the bit of disk space? Rebuilding native bits has its own risks.

[adnapibar]

Elasticsearch rebuilds JNA to strip out native libraries for platforms that not supported by elastic.
Are we going change JNA dependency from elastic rebuild version to a GA version and ignore the strip out action?
@dblock

My vote would be yes.
I don't see why we would keep restricting platforms. While we may not be releasing a package that has been tested on some platform, it doesn't feel right to artificially prevent others from trying to run on those platforms, reporting bugs, etc. Looking for @nknize to agree/disagree here as well to make sure I'm not missing more history or context.

Here is the list of native libraries that JNA supports - https://github.com/java-native-access/jna/tree/master/lib/native
Does It makes sense to include these libraries for the platforms which we probably won't support (e.g. Android)?

Those seem minor, or aren't they? Are we worried about the bit of disk space? Rebuilding native bits has its own risks.

I'm not worried about the disk space, but there must be some reason why elastic chose to go this route. We should find that out and if that seems like an unreasonable reason we can start using the upstream JNA.

[adnapibar]

Those seem minor, or aren't they? Are we worried about the bit of disk space? Rebuilding native bits has its own risks.

I'm not worried about the disk space, but there must be some reason why elastic chose to go this route. We should find that out and if that seems like an unreasonable reason we can start using the upstream JNA.

The original intent is explained here https://discuss.elastic.co/t/why-elasticsearch-has-separate-jna-library/194986/4
Maybe this no longer holds true.

[dblock]

I don't see any real problems or bugs in that explanation, only FUD.

[reta]

@dblock my apologies, I cannot update the description of the issue but we could put checks on:

[X] client/rest/build.gradle
[X] client/sniffer/build.gradle

Those are out of scope now, thank you!

@VachaShah do you need help with this issue? There are a few options (besides forking and giving all permissions) which we could explore, what do you think?

[VachaShah]

@reta Thank you for the offer! @xuezhou25 will be working on this.

[dblock]

@reta since you PRed #1187, you've got a leg ahead on all of us already. We have mountains of other things we want to do - would you rather pick this one up?

[reta]

@dblock absolutely, @xuezhou25 how does it sound to you?

[CEHENKLE]

@reta Cool beans! Thank you :)

[xuezhou25]

@reta My plan is still forking and giving all permissions to mocksocket & securemock. Since you have the better solution and are ahead of us, it's all yours! :)

[reta]

@xuezhou25 thank you, since there is a dedicated security manager for testing, I will try to prototype the dedicated security checks in there, hopefully (:crossed_fingers: ) the forks will not be needed anymore.