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.

Sign up for GitHub

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

Jump to bottom

OpenSSLTest is not using the OpenSSL Provider #2301

Merged
merged 3 commits into from
Dec 15, 2022
Merged

OpenSSLTest is not using the OpenSSL Provider #2301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
49ff354
OpenSSLTest is not using the OpenSSL Provider
reta Nov 2, 2022
56115da
Enable OpenSSLTest on Windows
reta Dec 8, 2022
edf89c2
Extracted OpenSSL test into separate task to eliminate mess with syst…
reta Dec 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ plugins {
id "org.gradle.test-retry" version "1.4.1"
id 'eclipse'
id "com.github.spotbugs" version "5.0.13"
id "com.google.osdetector" version "1.7.1"
}

allprojects {
Expand Down Expand Up @@ -121,6 +122,7 @@ test {
include '**/*.class'
filter {
excludeTestsMatching "org.opensearch.security.sanity.tests.*"
excludeTestsMatching "org.opensearch.security.ssl.OpenSSL*"
}
maxParallelForks = 8
jvmArgs += "-Xmx3072m"
Expand Down Expand Up @@ -148,13 +150,37 @@ test {
}
}

//add new task that runs OpenSSL tests
task opensslTest(type: Test) {
Copy link
Collaborator Author

@reta reta Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

include '**/OpenSSL*.class'
retry {
failOnPassedAfterRetry = false
maxRetries = 5
}
jacoco {
excludes = [
"com.sun.jndi.dns.*",
"com.sun.security.sasl.gsskerb.*",
"java.sql.*",
"javax.script.*",
"org.jcp.xml.dsig.internal.dom.*",
"sun.nio.cs.ext.*",
"sun.security.ec.*",
"sun.security.jgss.*",
"sun.security.pkcs11.*",
"sun.security.smartcardio.*",
"sun.util.resources.provider.*"
]
}
}

task copyExtraTestResources(dependsOn: testClasses) {
copy {
from 'src/test/resources'
into 'build/testrun/test/src/test/resources'
}
}
tasks.test.dependsOn(copyExtraTestResources)
tasks.test.dependsOn(copyExtraTestResources, opensslTest)

jacoco {
reportsDirectory = file("$buildDir/reports/jacoco")
Expand Down Expand Up @@ -413,6 +439,11 @@ dependencies {
testImplementation 'org.springframework:spring-beans:5.3.20'
testImplementation 'org.junit.jupiter:junit-jupiter:5.8.2'
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.2'
// Only osx-x86_64, osx-aarch_64, linux-x86_64, linux-aarch_64, windows-x86_64 are available
if (osdetector.classifier in ["osx-x86_64", "osx-aarch_64", "linux-x86_64", "linux-aarch_64", "windows-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}"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@reta reta Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

}
// JUnit build requirement
testCompileOnly 'org.apiguardian:apiguardian-api:1.0.0'
// Kafka test execution
Expand Down
1 change: 0 additions & 1 deletion src/test/java/org/opensearch/security/ssl/OpenSSLTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ public static void restoreNettyDefaultAllocator() {

@Before
public void setup() {
Assume.assumeFalse(PlatformDependent.isWindows());
Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Thanks @reta !

allowOpenSSL = true;
}

Expand Down
4 changes: 2 additions & 2 deletions src/test/java/org/opensearch/security/ssl/SSLTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,9 @@ public void testHttps() throws Exception {
.put(SSLConfigConstants.SECURITY_SSL_TRANSPORT_ENABLE_OPENSSL_IF_AVAILABLE, allowOpenSSL)
.put(SSLConfigConstants.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")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

.putList(SSLConfigConstants.SECURITY_SSL_TRANSPORT_ENABLED_PROTOCOLS, "TLSv1.1", "TLSv1.2")
.putList(SSLConfigConstants.SECURITY_SSL_TRANSPORT_ENABLED_CIPHERS, "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256")
.putList(SSLConfigConstants.SECURITY_SSL_TRANSPORT_ENABLED_CIPHERS, "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256")
.put(SSLConfigConstants.SECURITY_SSL_HTTP_KEYSTORE_FILEPATH, FileHelper.getAbsoluteFilePathFromClassPath("ssl/node-0-keystore.jks"))
.put(SSLConfigConstants.SECURITY_SSL_HTTP_TRUSTSTORE_FILEPATH, FileHelper.getAbsoluteFilePathFromClassPath("ssl/truststore.jks"))
.build();
Expand Down