OpenSSLTest is not using the OpenSSL Provider by reta · Pull Request #2301 · opensearch-project/security

reta · 2022-12-06T15:48:18Z

Signed-off-by: Andriy Redko andriy.redko@aiven.io

Description

OpenSSLTest should be using the OpenSSL Provider. There are few issues with netty-tcnative and OpenSSL:

it uses OpenSSL 1.0 and needs compat-openssl10
netty-tcnative uses old OpenSSL version even though newer versions are available during runtime netty/netty-tcnative#746
[Upgrade] Use OpenSSL v3 netty/netty-tcnative#742
Link against to the libcrypt.so.2 netty/netty-tcnative#703

Issues Resolved

Closes #2208

Is this a backport? If so, please add backport PR # and/or commits #

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

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.

reta · 2022-12-06T16:01:13Z

@peternied reopened this guy, was able to make it run with BoringSSL but found a number of issues on OpenSearch core side (opensearch-project/OpenSearch#5460)

codecov-commenter · 2022-12-06T16:45:38Z

Codecov Report

Merging #2301 (edf89c2) into main (ff798dd) will decrease coverage by 0.07%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #2301      +/-   ##
============================================
- Coverage     61.10%   61.03%   -0.08%     
+ Complexity     3272     3266       -6     
============================================
  Files           260      260              
  Lines         18369    18369              
  Branches       3251     3251              
============================================
- Hits          11225    11212      -13     
- Misses         5558     5566       +8     
- Partials       1586     1591       +5

Impacted Files	Coverage Δ
...ecurity/configuration/StaticResourceException.java	`0.00% <0.00%> (-25.00%)`	⬇️
...urity/ssl/transport/SecuritySSLNettyTransport.java	`62.36% <0.00%> (-4.31%)`	⬇️
...nsearch/security/dlic/rest/api/AuditApiAction.java	`63.82% <0.00%> (-4.26%)`	⬇️
...earch/security/ssl/util/SSLConnectionTestUtil.java	`93.18% <0.00%> (-2.28%)`	⬇️
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java	`57.46% <0.00%> (-1.50%)`	⬇️
...search/security/transport/SecurityInterceptor.java	`75.38% <0.00%> (-0.77%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

reta · 2022-12-08T15:14:07Z

src/test/java/org/opensearch/security/ssl/SSLTest.java

                .put("plugins.security.ssl.http.clientauth_mode", "REQUIRE")
                .putList(SSLConfigConstants.SECURITY_SSL_HTTP_ENABLED_PROTOCOLS, "TLSv1.1","TLSv1.2")
-                .putList(SSLConfigConstants.SECURITY_SSL_HTTP_ENABLED_CIPHERS, "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256")
+                .putList(SSLConfigConstants.SECURITY_SSL_HTTP_ENABLED_CIPHERS, "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256")


Updating to GCM since OpenSSL does not support CBC for this particular cipher (the list of supported OpenSSL/BoringSSL ciphers is below):

ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-RSA-AES128-GCM-SHA256 ECDHE-ECDSA-AES256-GCM-SHA384 ECDHE-RSA-AES256-GCM-SHA384 ECDHE-ECDSA-CHACHA20-POLY1305 ECDHE-RSA-CHACHA20-POLY1305 ECDHE-PSK-CHACHA20-POLY1305 ECDHE-ECDSA-AES128-SHA ECDHE-RSA-AES128-SHA ECDHE-PSK-AES128-CBC-SHA ECDHE-ECDSA-AES256-SHA ECDHE-RSA-AES256-SHA ECDHE-PSK-AES256-CBC-SHA AES128-GCM-SHA256 AES256-GCM-SHA384 AES128-SHA PSK-AES128-CBC-SHA AES256-SHA PSK-AES256-CBC-SHA DES-CBC3-SHA TLS_AES_128_GCM_SHA256 TLS_AES_256_GCM_SHA384 TLS_CHACHA20_POLY1305_SHA256 AEAD-AES128-GCM-SHA256 AEAD-AES256-GCM-SHA384 AEAD-CHACHA20-POLY1305-SHA256

DarshitChanpura

Thank you for fixing this one @reta !

DarshitChanpura · 2022-12-08T16:09:47Z

build.gradle

+    // Only osx-x86_64 and linux-x86_64 are available
+    if (osdetector.classifier in ["osx-x86_64", "linux-x86_64"]) {
+        testImplementation "io.netty:netty-tcnative-classes:2.0.54.Final"
+        testImplementation "io.netty:netty-tcnative-boringssl-static:2.0.54.Final:${osdetector.classifier}"


OpenSSL will still be broken for windows for now, am I understanding this right?

Good one, thanks for catching that, the assumption was for OpenSSL but BoringSSL has more platforms supported, updated the condition

DarshitChanpura · 2022-12-08T16:23:19Z

WhiteSource check is failing due to bcel version error which was fixed #2275. However, @reta when I click on view file for build.gradle on this PR, it doesn't show this change. You probably need to sync with main?

reta · 2022-12-08T16:37:33Z

WhiteSource check is failing due to bcel version error which was fixed #2275. However, @reta when I click on view file for build.gradle on this PR, it doesn't show this change. You probably need to sync with main?

Rebased, thank you!

DarshitChanpura

LGTM! thanks @reta !

DarshitChanpura · 2022-12-08T17:51:49Z

src/test/java/org/opensearch/security/ssl/OpenSSLTest.java


    @Before
    public void setup() {
-        Assume.assumeFalse(PlatformDependent.isWindows());


Good catch. Thanks @reta !

reta · 2022-12-08T19:18:59Z

[2022-12-08T17:46:35,922][WARN ][org.opensearch.node.Node] version [3.0.0-SNAPSHOT] is a pre-release version of OpenSearch and is not suitable for production
[2022-12-08T17:46:35,923][WARN ][org.opensearch.security.ssl.DefaultSecurityKeyStore] Support for OpenSSL with Java 11 or prior versions require using Netty allocator. Set 'opensearch.unsafe.use_netty_default_allocator' system property to true
...
{
  "principal" : "CN=node-0.example.com,OU=SSL,O=Test,L=Test,C=DE",
  "peer_certificates" : "3",
  "ssl_protocol" : "TLSv1.2",
  "ssl_cipher" : "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384",
  "ssl_openssl_available" : true,
  "ssl_openssl_version" : 269488255,
  "ssl_openssl_version_string" : "BoringSSL",
  "ssl_openssl_non_available_cause" : "",
  "ssl_openssl_supports_key_manager_factory" : true,
  "ssl_openssl_supports_hostname_validation" : true,
  "ssl_provider_http" : "JDK",
  "ssl_provider_transport_server" : "JDK",
  "ssl_provider_transport_client" : "JDK"
}

reta · 2022-12-08T21:03:35Z

build.gradle

 }

+//add new task that runs OpenSSL tests
+task opensslTest(type: Test) {


@cwperks it turned out that manipulation with system properties complicates OpenSSL tests (since OpenSearchSecuritySSLPlugin.OPENSSL_SUPPORTED is initialized only once within same JVM and order matters a lot, otherwise tests are just ignored). So I have extracted OpenSSL tests into separate task opensslTest which is run before test.

reta · 2022-12-08T21:31:36Z

Cool, OpenSSL tests are now run and failing, should be fixed by opensearch-project/OpenSearch#5460

cwperks

The corresponding PR in core was just merged. I will re-run the CI as soon as a new snapshot build is available. Great work @reta!

reta · 2022-12-09T14:34:23Z

The corresponding PR in core was just merged. I will re-run the CI as soon as a new snapshot build is available. Great work @reta!

Thanks @cwperks ! The distribution build should be fixed by opensearch-project/sql#1148 but it is not merged yet :(

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

…em properties Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

reta · 2022-12-13T20:54:41Z

@cwperks @DarshitChanpura I think we are good to go!

{
  "principal" : "CN=node-0.example.com,OU=SSL,O=Test,L=Test,C=DE",
  "peer_certificates" : "3",
  "ssl_protocol" : "TLSv1.2",
  "ssl_cipher" : "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
  "ssl_openssl_available" : true,
  "ssl_openssl_version" : 269488255,
  "ssl_openssl_version_string" : "BoringSSL",
  "ssl_openssl_non_available_cause" : "",
  "ssl_openssl_supports_key_manager_factory" : true,
  "ssl_openssl_supports_hostname_validation" : true,
  "ssl_provider_http" : "OPENSSL",
  "ssl_provider_transport_server" : "OPENSSL",
  "ssl_provider_transport_client" : "OPENSSL"
}

🥳

cwperks

@reta Awesome! Thank you for all of the hard work on this.

I will approve in a moment, but have one quick question. Will the OpenSSLTest suite fail if it is not using the OpenSSL provider?

reta · 2022-12-13T22:05:56Z

I will approve in a moment, but have one quick question. Will the OpenSSLTest suite fail if it is not using the OpenSSL provider?

No, but it won't run - all tests will be ignored

DarshitChanpura

Thank you @reta !

opensearch-trigger-bot · 2023-03-09T15:51:06Z

The backport to 2.x failed:

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

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
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2301-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d14143d315174a23a6ad215b71bd938a16c182ab
# Push it to GitHub
git push --set-upstream origin backport/backport-2301-to-2.x
# Go back to the original working tree
cd ../..
# 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-2301-to-2.x.

* OpenSSLTest is not using the OpenSSL Provider Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Enable OpenSSLTest on Windows Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Extracted OpenSSL test into separate task to eliminate mess with system properties Signed-off-by: Andriy Redko <andriy.redko@aiven.io> Signed-off-by: Andriy Redko <andriy.redko@aiven.io> (cherry picked from commit d14143d) Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

* OpenSSLTest is not using the OpenSSL Provider * Enable OpenSSLTest on Windows * Extracted OpenSSL test into separate task to eliminate mess with system properties (cherry picked from commit d14143d) Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

…ject#2301) (opensearch-project#2533)" This reverts commit 17596f7.

reta requested a review from a team December 6, 2022 15:48

cwperks mentioned this pull request Dec 6, 2022

Remove OpenSSL provider as an option. JDK SSL Provider will be used by default. #2298

Closed

3 tasks

reta force-pushed the issue-2208 branch from 801c3b2 to e2a528f Compare December 8, 2022 15:11

reta commented Dec 8, 2022

View reviewed changes

DarshitChanpura previously approved these changes Dec 8, 2022

View reviewed changes

reta dismissed DarshitChanpura’s stale review via 4cfb44c December 8, 2022 16:36

reta force-pushed the issue-2208 branch from e2a528f to 4cfb44c Compare December 8, 2022 16:36

DarshitChanpura previously approved these changes Dec 8, 2022

View reviewed changes

reta dismissed DarshitChanpura’s stale review via 61a8ded December 8, 2022 17:23

DarshitChanpura previously approved these changes Dec 8, 2022

View reviewed changes

reta dismissed DarshitChanpura’s stale review via ab16582 December 8, 2022 20:29

reta force-pushed the issue-2208 branch from 61a8ded to ab16582 Compare December 8, 2022 20:29

reta commented Dec 8, 2022

View reviewed changes

cwperks previously approved these changes Dec 9, 2022

View reviewed changes

DarshitChanpura previously approved these changes Dec 9, 2022

View reviewed changes

reta dismissed stale reviews from DarshitChanpura and cwperks via 1f1ff33 December 13, 2022 13:59

reta force-pushed the issue-2208 branch 2 times, most recently from 1f1ff33 to 541db5b Compare December 13, 2022 17:58

reta added 2 commits December 13, 2022 13:27

OpenSSLTest is not using the OpenSSL Provider

49ff354

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

Enable OpenSSLTest on Windows

56115da

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

reta force-pushed the issue-2208 branch from 541db5b to a231543 Compare December 13, 2022 18:27

Extracted OpenSSL test into separate task to eliminate mess with syst…

edf89c2

…em properties Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

reta force-pushed the issue-2208 branch from a231543 to edf89c2 Compare December 13, 2022 19:32

cwperks reviewed Dec 13, 2022

View reviewed changes

cwperks approved these changes Dec 13, 2022

View reviewed changes

DarshitChanpura approved these changes Dec 15, 2022

View reviewed changes

cwperks merged commit d14143d into opensearch-project:main Dec 15, 2022

cwperks added the backport 2.x label Mar 9, 2023

peternied mentioned this pull request Mar 9, 2023

[Backport] [2.x] OpenSSLTest is not using the OpenSSL Provider (#2301) #2533

Merged

Rishikesh1159 added a commit to Rishikesh1159/security that referenced this pull request Mar 15, 2023

Revert "OpenSSLTest is not using the OpenSSL Provider (opensearch-pro…

c965069

…ject#2301) (opensearch-project#2533)" This reverts commit 17596f7.

peternied mentioned this pull request Jun 16, 2023

[Enhancement] Parallel test jobs for CI #2861

Merged

3 tasks

Conversation

reta commented Dec 6, 2022

Description

Issues Resolved

Testing

Check List

Uh oh!

reta commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

reta Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

reta Dec 8, 2022 • 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!

DarshitChanpura commented Dec 8, 2022

Uh oh!

reta commented Dec 8, 2022

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

reta commented Dec 8, 2022

Uh oh!

reta Dec 8, 2022 • 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!

reta commented Dec 8, 2022

Uh oh!

cwperks left a comment

Choose a reason for hiding this comment

Uh oh!

reta commented Dec 9, 2022

Uh oh!

reta commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwperks left a comment • 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!

reta commented Dec 13, 2022

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

opensearch-trigger-bot bot commented Mar 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

reta commented Dec 6, 2022 •

edited

Loading

codecov-commenter commented Dec 6, 2022 •

edited

Loading

reta Dec 8, 2022 •

edited

Loading

reta Dec 8, 2022 •

edited

Loading

reta commented Dec 13, 2022 •

edited

Loading

cwperks left a comment •

edited

Loading