Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable limited OpenSSL support #422

Merged
merged 1 commit into from
Apr 30, 2020
Merged

Enable limited OpenSSL support #422

merged 1 commit into from
Apr 30, 2020

Conversation

vrozov
Copy link
Contributor

@vrozov vrozov commented Apr 24, 2020

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@vrozov vrozov requested a review from a team as a code owner April 24, 2020 19:05
@codecov
Copy link

codecov bot commented Apr 24, 2020

Codecov Report

Merging #422 into master will decrease coverage by 0.23%.
The diff coverage is 40.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #422      +/-   ##
============================================
- Coverage     62.34%   62.11%   -0.24%     
+ Complexity     2874     2872       -2     
============================================
  Files           223      223              
  Lines         15992    16032      +40     
  Branches       2994     3008      +14     
============================================
- Hits           9971     9959      -12     
- Misses         4461     4503      +42     
- Partials       1560     1570      +10     
Impacted Files Coverage Δ Complexity Δ
...ecurity/ssl/DefaultOpenDistroSecurityKeyStore.java 65.89% <33.33%> (-7.59%) 68.00 <0.00> (ø)
...arch/security/ssl/OpenDistroSecuritySSLPlugin.java 81.81% <50.00%> (-0.42%) 24.00 <1.00> (ø)
...transport/OpenDistroSecuritySSLRequestHandler.java 50.00% <0.00%> (-6.42%) 7.00% <0.00%> (-1.00%)
...search/security/tools/OpenDistroSecurityAdmin.java 47.14% <0.00%> (-0.40%) 73.00% <0.00%> (-1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a989b28...8519585. Read the comment docs.

if(Constants.JRE_IS_MINIMUM_JAVA11) {
text += "Since you are running Java "+Constants.JAVA_VERSION+" you should not experience any performance impact but maybe not all your ciphers are supported. If you experience problems upgrade to Java 11+";
if (PlatformDependent.javaVersion() < 12) {
log.warn("Support for OpenSSL with Java 11 or prior versions require using Netty allocator. Set 'es.unsafe.use_netty_default_allocator' system property to true");
Copy link
Contributor

@debjanibnrj debjanibnrj Apr 24, 2020

Choose a reason for hiding this comment

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

What does the es.unsafe.use_netty_default_allocator property do and why is it necessary for supporting node to node communication with OpenSSL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The es.unsafe.use_netty_default_allocator System property is used by NettyAllocator to determine which ByteBufAllocator to use. With this property set to true, ES will use native Netty allocator, otherwise, it will use its own NettyAllocator.NoDirectBuffers allocator that does not support direct buffers. As openssl is supported through netty and uses tcnative that needs direct buffers, it is necessary to configure ES to use Netty allocator if openssl is required/preferred.

@debjanibnrj
Copy link
Contributor

Please elaborate on how you tested these changes.

@vrozov
Copy link
Contributor Author

vrozov commented Apr 24, 2020

Tested using OpenSSLTest. Please let me know if this is not sufficient.

@debjanibnrj
Copy link
Contributor

debjanibnrj commented Apr 24, 2020

Tested using OpenSSLTest. Please let me know if this is not sufficient.

Would be useful to manually test out the changes as well. This can be done by spinning up cluster of nodes with elasticsearch oss (in this case for es 7.6) with the security plugin built with these changes. As far as I remember before we removed support for OpenSSL, the tests were still passing but inter-cluster communication between nodes was failing for Java version <12.

@vrozov
Copy link
Contributor Author

vrozov commented Apr 25, 2020

There is a difference in the test environment introduced in #339 that explains why tests were passing while inter-cluster communication was failing for Java 11 or prior versions. In the past, tests were running on the docker image without openssl installed and tests that exercise openssl functionality were disabled on OpenSsl.isAvailable() condition. Those tests are now enabled and they won't pass without changes in this PR.

@debjanibnrj
Copy link
Contributor

There is a difference in the test environment introduced in #339 that explains why tests were passing while inter-cluster communication was failing for Java 11 or prior versions. In the past, tests were running on the docker image without openssl installed and tests that exercise openssl functionality were disabled on OpenSsl.isAvailable() condition. Those tests are now enabled and they won't pass without changes in this PR.

Thanks for the explanation.

@vrozov vrozov requested a review from hardik-k-shah April 27, 2020 20:12
@vrozov vrozov merged commit 663053a into opensearch-project:master Apr 30, 2020
vrozov added a commit that referenced this pull request May 1, 2020
(cherry picked from commit 663053a)
vrozov added a commit that referenced this pull request May 1, 2020
(cherry picked from commit 663053a)
@vrozov vrozov deleted the enable_openssl branch May 2, 2020 23:25
lbreinig pushed a commit to lbreinig/security that referenced this pull request Dec 23, 2021
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
gaobinlong pushed a commit to gaobinlong/security that referenced this pull request Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants