Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
54 changes: 43 additions & 11 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@


import com.diffplug.gradle.spotless.JavaExtension
import org.opensearch.gradle.info.FipsBuildParams
import org.opensearch.gradle.test.RestIntegTestTask
import groovy.json.JsonBuilder

Expand Down Expand Up @@ -235,6 +234,24 @@ tasks.register("listTasksAsJSON") {
}
}

def configureFipsJvmArgs(Test task) {
if ('true'.equalsIgnoreCase(System.getenv('OPENSEARCH_FIPS_MODE'))) {
def fipsSecurityFile = project.rootProject.file('src/test/resources/fips_java_test.security')
// BCFKS truststore is sourced from the core project: buildSrc/src/main/resources/opensearch-fips-truststore.bcfks
// It can also be regenerated by running distribution/src/bin/opensearch-fips-demo-installer
def fipsTrustStore = project.rootProject.file('src/test/resources/fips-jvm-truststore.bcfks')
task.jvmArgs += "-Djava.security.properties==${fipsSecurityFile}"
task.jvmArgs += "-Dorg.bouncycastle.fips.approved_only=true"
task.jvmArgs += "-Djavax.net.ssl.trustStore=${fipsTrustStore}"
task.jvmArgs += "-Djavax.net.ssl.trustStoreProvider=BCFIPS"
task.jvmArgs += "-Djavax.net.ssl.trustStoreType=BCFKS"
task.jvmArgs += "-Djavax.net.ssl.trustStorePassword=changeit"
} else {
def nonFipsSecurityFile = project.rootProject.file('src/test/resources/java_test.security')
task.jvmArgs += "-Djava.security.properties==${nonFipsSecurityFile}"
}
}

tasks.register('copyExtraTestResources', Copy) {
dependsOn testClasses

Expand Down Expand Up @@ -285,6 +302,7 @@ def setCommonTestConfig(Test task) {
}
task.dependsOn copyExtraTestResources
task.finalizedBy jacocoTestReport
configureFipsJvmArgs(task)
}

test {
Expand Down Expand Up @@ -547,6 +565,9 @@ subprojects {
configurations.integrationTestImplementation.extendsFrom configurations.implementation
configurations.integrationTestRuntimeOnly.extendsFrom configurations.runtimeOnly
}
tasks.withType(Test).configureEach { t ->
configureFipsJvmArgs(t)
}
}
}
}
Expand Down Expand Up @@ -580,6 +601,7 @@ allprojects {
integrationTestImplementation "org.apache.logging.log4j:log4j-jul:${versions.log4j}"
integrationTestImplementation 'org.hamcrest:hamcrest:2.2'
integrationTestImplementation "org.bouncycastle:bc-fips:${versions.bouncycastle_jce}"
integrationTestImplementation "org.bouncycastle:bctls-fips:${versions.bouncycastle_tls}"
integrationTestImplementation "org.bouncycastle:bcpkix-fips:${versions.bouncycastle_pkix}"
integrationTestImplementation "org.bouncycastle:bcutil-fips:${versions.bouncycastle_util}"
integrationTestImplementation('org.awaitility:awaitility:4.3.0') {
Expand Down Expand Up @@ -660,6 +682,7 @@ tasks.register("integrationTest", Test) {
"sun.util.resources.provider.*"
]
}
configureFipsJvmArgs(it)
}

tasks.named("integrationTest") {
Expand All @@ -684,16 +707,10 @@ dependencies {
implementation libs.google.guava
implementation 'org.greenrobot:eventbus-java:3.3.1'
implementation 'commons-cli:commons-cli:1.11.0'
// When building with crypto.standard=FIPS-140-3 (set in gradle.properties), bcFips jars are provided by OpenSearch
if (FipsBuildParams.isInFipsMode()) {
compileOnly "org.bouncycastle:bc-fips:${versions.bouncycastle_jce}"
compileOnly "org.bouncycastle:bcpkix-fips:${versions.bouncycastle_pkix}"
compileOnly "org.bouncycastle:bcutil-fips:${versions.bouncycastle_util}"
} else {
implementation "org.bouncycastle:bc-fips:${versions.bouncycastle_jce}"
implementation "org.bouncycastle:bcpkix-fips:${versions.bouncycastle_pkix}"
implementation "org.bouncycastle:bcutil-fips:${versions.bouncycastle_util}"
}
compileOnly "org.bouncycastle:bc-fips:${versions.bouncycastle_jce}"
compileOnly "org.bouncycastle:bctls-fips:${versions.bouncycastle_tls}"
compileOnly "org.bouncycastle:bcpkix-fips:${versions.bouncycastle_pkix}"
compileOnly "org.bouncycastle:bcutil-fips:${versions.bouncycastle_util}"
implementation 'org.ldaptive:ldaptive:1.2.3'
implementation 'com.nimbusds:nimbus-jose-jwt:10.9.1'
implementation 'com.rfksystems:blake2b:2.0.0'
Expand Down Expand Up @@ -799,6 +816,7 @@ dependencies {
exclude(group: 'org.hamcrest', module: 'hamcrest')
}
testImplementation "org.bouncycastle:bc-fips:${versions.bouncycastle_jce}"
testImplementation "org.bouncycastle:bctls-fips:${versions.bouncycastle_tls}"
testImplementation "org.bouncycastle:bcpkix-fips:${versions.bouncycastle_pkix}"
testImplementation "org.bouncycastle:bcutil-fips:${versions.bouncycastle_util}"
// JUnit build requirement
Expand Down Expand Up @@ -836,6 +854,18 @@ tasks.register('testsJar', Jar) {
from(sourceSets.test.output)
}

def configureSecurityAdminBcFips(AbstractArchiveTask task) {
def bcFipsJars = configurations.detachedConfiguration(
dependencies.create("org.bouncycastle:bc-fips:${versions.bouncycastle_jce}"),
dependencies.create("org.bouncycastle:bctls-fips:${versions.bouncycastle_tls}"),
dependencies.create("org.bouncycastle:bcpkix-fips:${versions.bouncycastle_pkix}"),
dependencies.create("org.bouncycastle:bcutil-fips:${versions.bouncycastle_util}")
)
task.from(bcFipsJars) {
into 'deps/'
}
}

tasks.register("bundleSecurityAdminStandalone", Zip) {
dependsOn(jar)
archiveClassifier = 'securityadmin-standalone'
Expand All @@ -852,6 +882,7 @@ tasks.register("bundleSecurityAdminStandalone", Zip) {
into 'deps/securityconfig'
}
}
configureSecurityAdminBcFips(bundleSecurityAdminStandalone)

tasks.register("bundleSecurityAdminStandaloneTarGz", Tar) {
dependsOn(jar)
Expand All @@ -871,6 +902,7 @@ tasks.register("bundleSecurityAdminStandaloneTarGz", Tar) {
into 'deps/securityconfig'
}
}
configureSecurityAdminBcFips(bundleSecurityAdminStandaloneTarGz)

buildRpm {
arch = 'NOARCH'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
*/
package org.opensearch.security;

import java.time.Duration;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -44,10 +45,15 @@ protected AbstractDefaultConfigurationTests(LocalCluster cluster) {
this.cluster = cluster;
}

private static final Duration AWAIT_TIMEOUT = Duration.ofSeconds(30);

@Test
public void shouldLoadDefaultConfiguration() {
try (TestRestClient client = cluster.getRestClient(NEW_USER)) {
Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200));
Awaitility.await()
.alias("Load default configuration")
.timeout(AWAIT_TIMEOUT)
.until(() -> client.getAuthInfo().getStatusCode(), equalTo(200));
}
try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) {
client.confirmCorrectCredentials(ADMIN_USER.getName());
Expand Down Expand Up @@ -103,7 +109,10 @@ private void prepareRolesTestCase() {
public void securityUpgrade() throws Exception {
try (var client = cluster.getRestClient(ADMIN_USER)) {
// Setup: Make sure the config is ready before starting modifications
Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200));
Awaitility.await()
.alias("Load default configuration")
.timeout(AWAIT_TIMEOUT)
.until(() -> client.getAuthInfo().getStatusCode(), equalTo(200));
// Setup: Collect default roles after cluster start
final var expectedRoles = client.get("_plugins/_security/api/roles/");
final var expectedRoleNames = extractFieldNames(expectedRoles.getBodyAs(JsonNode.class));
Expand Down Expand Up @@ -143,7 +152,10 @@ public void securityUpgrade() throws Exception {
public void securityRolesUpgrade() throws Exception {
try (var client = cluster.getRestClient(ADMIN_USER)) {
// Setup: Make sure the config is ready before starting modifications
Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200));
Awaitility.await()
.alias("Load default configuration")
.timeout(AWAIT_TIMEOUT)
.until(() -> client.getAuthInfo().getStatusCode(), equalTo(200));

// Setup: Collect default roles after cluster start
final var expectedRoles = client.get("_plugins/_security/api/roles/");
Expand Down Expand Up @@ -185,7 +197,10 @@ public void securityRolesUpgrade() throws Exception {
public void securityRolesUpgradeSpecifyingRoles() throws Exception {
try (var client = cluster.getRestClient(ADMIN_USER)) {
// Setup: Make sure the config is ready before starting modifications
Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200));
Awaitility.await()
.alias("Load default configuration")
.timeout(AWAIT_TIMEOUT)
.until(() -> client.getAuthInfo().getStatusCode(), equalTo(200));

// Setup: Collect default roles after cluster start
final var expectedRoles = client.get("_plugins/_security/api/roles/");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@

import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Objects;

import org.opensearch.security.securityconf.impl.CType;
import org.opensearch.test.framework.TestSecurityConfig;

public class ConfigurationFiles {

Expand All @@ -25,7 +27,6 @@ public static Path createConfigurationDirectory() {
String[] configurationFiles = {
CType.ACTIONGROUPS.configFileName(),
CType.CONFIG.configFileName(),
CType.INTERNALUSERS.configFileName(),
CType.NODESDN.configFileName(),
CType.ROLES.configFileName(),
CType.ROLESMAPPING.configFileName(),
Expand All @@ -34,12 +35,36 @@ public static Path createConfigurationDirectory() {
for (String fileName : configurationFiles) {
copyResourceToFile(fileName, tempDirectory.resolve(fileName));
}
writeInternalUsersFile(tempDirectory.resolve(CType.INTERNALUSERS.configFileName()));
return tempDirectory.toAbsolutePath();
} catch (IOException ex) {
throw new RuntimeException("Cannot create directory with security plugin configuration.", ex);
}
}

// Generates internal_users.yml with a hash derived from DEFAULT_TEST_PASSWORD so the
// admin user can authenticate in both FIPS and non-FIPS mode without a hardcoded BCrypt hash.
private static void writeInternalUsersFile(Path destination) throws IOException {
String hash = TestSecurityConfig.hashPassword(TestSecurityConfig.DEFAULT_TEST_PASSWORD);
String content = """
---
_meta:
type: "internalusers"
config_version: 2
new-user:
hash: "%s"
limited-user:
hash: "%s"
opendistro_security_roles:
- "user_limited-user__limited-role"
admin:
hash: "%s"
opendistro_security_roles:
- "user_admin__all_access"
""".formatted(hash, hash, hash);
Files.writeString(destination, content, StandardCharsets.UTF_8);
}

public static void copyResourceToFile(String resource, Path destination) {
try (InputStream input = ConfigurationFiles.class.getClassLoader().getResourceAsStream(resource)) {
Objects.requireNonNull(input, "Cannot find source resource " + resource);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.opensearch.common.collect.Tuple;
import org.opensearch.common.settings.Settings;
import org.opensearch.http.HttpTransportSettings;
import org.opensearch.security.support.FipsMode;
import org.opensearch.test.framework.AsyncActions;
import org.opensearch.test.framework.TestSecurityConfig;
import org.opensearch.test.framework.TestSecurityConfig.User;
Expand All @@ -48,7 +49,6 @@

import io.netty.handler.codec.http.FullHttpResponse;
import io.netty.handler.codec.http.HttpResponseStatus;
import reactor.netty.http.HttpProtocol;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -111,8 +111,8 @@ public void testUnauthenticatedManyMedium() {
// Tweaks:
final RequestBodySize size = RequestBodySize.Medium;
final String requestPath = "/*/_search";
final int parallelism = 20;
final int totalNumberOfRequests = 10_000;
final int parallelism = FipsMode.isEnabled() ? 8 : 20;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks odd, why do we have to change these settings? No relation to FIPS I believe

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The generic client fires all requests at once ParallelFlux.from(monos) - for the test with 15k unbounded requests in FIPS-mode (executed in local env) ~80% of exceptions are connect/handshake timeouts at the node: the server can't accept() + handshake fast enough, so SYNs/handshakes expire. FIPS trips it first because the BCTLS handshake is ~x4 costlier.

IMO it comes down to two solutions:

  • Explicitly limit parallel requests - this code (introduces in fd9ed30)
  • Proper saturation via backpressure (parallelism = demand). Let Reactor apply backpressure so parallelism is the concurrency limit:
Flux.fromArray(monos).flatMap(mono -> mono, parallelism).collectList().block(...);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What about third option to increase connect timeouts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

generic-client storm (15k requests, parallelism 100, FIPS)

Metric 30s / 30s 120s / 120s
ok 7,920 (52.8%) 13,525 (90.2%)
failed 7,080 1,475
TCP_CONNECT_TIMEOUT 5,144 0
TLS_HANDSHAKE_TIMEOUT 839 0
TCP_CONNECTION_RESET 738 1,281
PREMATURE_CLOSE 359 194
elapsed time 50.1s 81.9s

Takeaway: raising the connect/handshake timeout (30s -> 120s) eliminates the*_TIMEOUT failures but cannot make the run green - TCP_CONNECTION_RESET (accept-queue overflow) grows and wall-clock rises.
2nd option is a better solution, because it keeps the accept queue shallow so nothing overflows and nothing waits near a timeout.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see, thank you, let's do 2nd option

final int totalNumberOfRequests = FipsMode.isEnabled() ? 2_000 : 10_000;

runResourceTest(size, requestPath, parallelism, totalNumberOfRequests);
runResourceTestWithGenericClient(size, requestPath, parallelism, totalNumberOfRequests);
Expand All @@ -123,8 +123,8 @@ public void testUnauthenticatedTonsSmall() {
// Tweaks:
final RequestBodySize size = RequestBodySize.Small;
final String requestPath = "/*/_search";
final int parallelism = 100;
final int totalNumberOfRequests = 15_000;
final int parallelism = FipsMode.isEnabled() ? 8 : 100;
final int totalNumberOfRequests = FipsMode.isEnabled() ? 2_000 : 15_000;

runResourceTest(size, requestPath, parallelism, totalNumberOfRequests);
runResourceTestWithGenericClient(size, requestPath, parallelism, totalNumberOfRequests);
Expand Down Expand Up @@ -158,13 +158,7 @@ private void runResourceTestWithGenericClient(
final int totalNumberOfRequests
) {
final byte[] compressedRequestBody = createCompressedRequestBody(size);
try (
final ReactorHttpClient client = cluster.getGenericClient(
HttpProtocol.HTTP3,
true,
Settings.builder().loadFromMap(NODE_SETTINGS).build()
)
) {
try (final ReactorHttpClient client = cluster.getGenericClient(Settings.builder().loadFromMap(NODE_SETTINGS).build())) {
List<Tuple<String, byte[]>> requestUris = new ArrayList<>();
for (int i = 0; i < totalNumberOfRequests; i++) {
requestUris.add(Tuple.tuple(requestPath, compressedRequestBody));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,17 +139,21 @@ public void shouldStillLoadSecurityConfigDuringBootstrapAndActiveConfigUpdateReq
}
});

Awaitility.await().alias("Load default configuration").pollInterval(Duration.ofMillis(100)).until(() -> {
// After the configuration has been loaded, the rest clients should be able to connect successfully
cluster.triggerConfigurationReloadForCTypes(
internalNodeClient,
List.of(CType.ACTIONGROUPS, CType.CONFIG, CType.ROLES, CType.ROLESMAPPING, CType.TENANTS),
true
);
try (final TestRestClient freshClient = cluster.getRestClient(USER_ADMIN)) {
return client.getAuthInfo().getStatusCode();
}
}, equalTo(200));
Awaitility.await()
.alias("Load default configuration")
.pollInterval(Duration.ofMillis(100))
.timeout(Duration.ofSeconds(30))
.until(() -> {
// After the configuration has been loaded, the rest clients should be able to connect successfully
cluster.triggerConfigurationReloadForCTypes(
internalNodeClient,
List.of(CType.ACTIONGROUPS, CType.CONFIG, CType.ROLES, CType.ROLESMAPPING, CType.TENANTS),
true
);
try (final TestRestClient freshClient = cluster.getRestClient(USER_ADMIN)) {
return client.getAuthInfo().getStatusCode();
}
}, equalTo(200));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.junit.Rule;
import org.junit.Test;

import org.opensearch.security.support.FipsMode;
import org.opensearch.test.framework.certificate.TestCertificates;
import org.opensearch.test.framework.cluster.ClusterManager;
import org.opensearch.test.framework.cluster.LocalCluster;
Expand Down Expand Up @@ -71,7 +72,11 @@ public void clusterShouldNotStart_nodesSanIpsAreInvalid() {
);
clusterFuture.cancel(true);
} catch (Exception e) {
logsRule.assertThatContain("No subject alternative names matching IP address 127.0.0.1 found");
if (FipsMode.isEnabled()) {
logsRule.assertThatStackTraceContain("No subject alternative name found matching IP address 127.0.0.1");
} else {
logsRule.assertThatContain("No subject alternative names matching IP address 127.0.0.1 found");
}
}
}
}
Loading
Loading