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

Enable security for bwc tests #3269

Merged
merged 3 commits into from
Aug 31, 2023
Merged

Enable security for bwc tests #3269

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
a53e385
[2.x] Enable security for bwc tests (#3257)
cwperks Aug 30, 2023
39d4923
3.0.0
cwperks Aug 30, 2023
8a713ff
Use Apache HTTP Client5 on main
cwperks Aug 30, 2023
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
11 changes: 11 additions & 0 deletions .github/actions/run-bwc-suite/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ inputs:
description: 'The name of the artifacts for this run, e.g. "BWC-2.1-to-2.4-results"'
required: true

username:
description: 'Username to use for cluster health check in testClusters'
required: true

password:
description: 'Password to use for cluster health check in testClusters'
required: true

runs:
using: "composite"
steps:
Expand All @@ -35,6 +43,9 @@ runs:
arguments: |
bwcTestSuite
-Dtests.security.manager=false
-Dtests.opensearch.secure=true
-Dtests.opensearch.username=${{ inputs.username }}
-Dtests.opensearch.password=${{ inputs.password }}
-Dbwc.version.previous=${{ steps.build-previous.outputs.built-version }}
-Dbwc.version.next=${{ steps.build-next.outputs.built-version }} -i
build-root-directory: bwc-test
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ jobs:
plugin-previous-branch: "2.x"
plugin-next-branch: "current_branch"
report-artifact-name: bwc-${{ matrix.platform }}-jdk${{ matrix.jdk }}
username: admin
password: admin

code-ql:
runs-on: ubuntu-latest
Expand Down
10 changes: 5 additions & 5 deletions bwc-test/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ buildscript {
ext {
opensearch_version = System.getProperty("opensearch.version", "3.0.0-SNAPSHOT")
opensearch_group = "org.opensearch"
common_utils_version = System.getProperty("common_utils.version", '2.9.0.0-SNAPSHOT')
}
repositories {
mavenLocal()
Expand All @@ -70,6 +71,7 @@ dependencies {
testImplementation "com.google.guava:guava:${versions.guava}"
testImplementation "org.opensearch.test:framework:${opensearch_version}"
testImplementation "org.apache.logging.log4j:log4j-core:${versions.log4j}"
testImplementation "org.opensearch:common-utils:${common_utils_version}"
}

loggerUsageCheck.enabled = false
Expand All @@ -87,9 +89,6 @@ String projectVersion = nextVersion
String previousOpenSearch = extractVersion(previousVersion) + "-SNAPSHOT";
String nextOpenSearch = extractVersion(nextVersion) + "-SNAPSHOT";

println previousOpenSearch + nextOpenSearch;


// Extracts the OpenSearch version from a plugin version string, 2.4.0.0 -> 2.4.0.
def String extractVersion(versionStr) {
def versionMatcher = versionStr =~ /(.+?)(\.\d+)$/
Expand Down Expand Up @@ -125,7 +124,8 @@ def String extractVersion(versionStr) {
node.extraConfigFile("esnode.pem", file("src/test/resources/security/esnode.pem"))
node.extraConfigFile("esnode-key.pem", file("src/test/resources/security/esnode-key.pem"))
node.extraConfigFile("root-ca.pem", file("src/test/resources/security/root-ca.pem"))
node.setting("plugins.security.disabled", "true")
node.setting("network.bind_host", "127.0.0.1")
node.setting("network.publish_host", "127.0.0.1")
node.setting("plugins.security.ssl.transport.pemcert_filepath", "esnode.pem")
node.setting("plugins.security.ssl.transport.pemkey_filepath", "esnode-key.pem")
node.setting("plugins.security.ssl.transport.pemtrustedcas_filepath", "root-ca.pem")
Expand All @@ -137,7 +137,7 @@ def String extractVersion(versionStr) {
node.setting("plugins.security.allow_unsafe_democertificates", "true")
node.setting("plugins.security.allow_default_init_securityindex", "true")
node.setting("plugins.security.authcz.admin_dn", "CN=kirk,OU=client,O=client,L=test,C=de")
node.setting("plugins.security.audit.type", "internal_elasticsearch")
node.setting("plugins.security.audit.type", "internal_opensearch")
node.setting("plugins.security.enable_snapshot_restore_privilege", "true")
node.setting("plugins.security.check_snapshot_restore_write_privileges", "true")
node.setting("plugins.security.restapi.roles_enabled", "[\"all_access\", \"security_rest_api_access\"]")
Expand Down
93 changes: 93 additions & 0 deletions bwc-test/src/test/java/SecurityBackwardsCompatibilityIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,32 @@
*/
package org.opensearch.security.bwc;

import java.io.IOException;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;

import org.apache.hc.client5.http.auth.AuthScope;
import org.apache.hc.client5.http.auth.UsernamePasswordCredentials;
import org.apache.hc.client5.http.impl.auth.BasicCredentialsProvider;
import org.apache.hc.client5.http.impl.nio.PoolingAsyncClientConnectionManagerBuilder;
import org.apache.hc.client5.http.nio.AsyncClientConnectionManager;
import org.apache.hc.client5.http.ssl.ClientTlsStrategyBuilder;
import org.apache.hc.client5.http.ssl.NoopHostnameVerifier;
import org.apache.hc.core5.function.Factory;
import org.apache.hc.core5.http.Header;
import org.apache.hc.core5.http.HttpHost;
import org.apache.hc.core5.http.message.BasicHeader;
import org.apache.hc.core5.http.nio.ssl.TlsStrategy;
import org.apache.hc.core5.reactor.ssl.TlsDetails;
import org.apache.hc.core5.ssl.SSLContextBuilder;
import org.junit.Assume;
import org.junit.Before;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.test.rest.OpenSearchRestTestCase;

import org.opensearch.Version;
import org.opensearch.common.settings.Settings;
Expand All @@ -22,6 +41,14 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasItem;

import org.opensearch.client.RestClient;
import org.opensearch.client.RestClientBuilder;

import org.junit.Assert;

import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLEngine;

public class SecurityBackwardsCompatibilityIT extends OpenSearchRestTestCase {

private ClusterType CLUSTER_TYPE;
Expand All @@ -35,6 +62,11 @@ private void testSetup() {
CLUSTER_NAME = System.getProperty("tests.clustername");
}

@Override
protected final boolean preserveClusterUponCompletion() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for my knowledge what is this boolean for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its to bypass this method which throws a WarningFailureException when using snapshots. It fails because its trying to json parse a response along with a warning that's printed out around the dot syntax for system index deprecation.

return true;
}

@Override
protected final boolean preserveIndicesUponCompletion() {
return true;
Expand All @@ -50,6 +82,11 @@ protected boolean preserveTemplatesUponCompletion() {
return true;
}

@Override
protected String getProtocol() {
return "https";
}

@Override
protected final Settings restClientSettings() {
return Settings.builder()
Expand All @@ -61,6 +98,56 @@ protected final Settings restClientSettings() {
.build();
}

@Override
protected RestClient buildClient(Settings settings, HttpHost[] hosts) throws IOException {
RestClientBuilder builder = RestClient.builder(hosts);
configureHttpsClient(builder, settings);
boolean strictDeprecationMode = settings.getAsBoolean("strictDeprecationMode", true);
builder.setStrictDeprecationMode(strictDeprecationMode);
return builder.build();
}

protected static void configureHttpsClient(RestClientBuilder builder, Settings settings) throws IOException {
Map<String, String> headers = ThreadContext.buildDefaultHeaders(settings);
Header[] defaultHeaders = new Header[headers.size()];
int i = 0;
for (Map.Entry<String, String> entry : headers.entrySet()) {
defaultHeaders[i++] = new BasicHeader(entry.getKey(), entry.getValue());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see that within the loop, a new BasicHeader object is created for each entry, using the key as the header name and the value as the header value. This BasicHeader object is then stored in the defaultHeaders array at the index specified by i, which is then incremented. And my question is more like "why we need these basic header for each entry?"

Copy link
Member Author

Choose a reason for hiding this comment

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

Apache HttpClient has 2 concrete types of headers: BasicHeader and BufferedHeader.

BasicHeader is a Basic implementation of Header which allows a header key and a header value.

https://www.javadoc.io/static/org.apache.httpcomponents/httpcore/4.4.1/index.html?org/apache/http/message/BasicHeader.html

}
builder.setDefaultHeaders(defaultHeaders);
builder.setHttpClientConfigCallback(httpClientBuilder -> {
String userName = Optional.ofNullable(System.getProperty("tests.opensearch.username"))
.orElseThrow(() -> new RuntimeException("user name is missing"));
String password = Optional.ofNullable(System.getProperty("tests.opensearch.password"))
.orElseThrow(() -> new RuntimeException("password is missing"));
BasicCredentialsProvider credentialsProvider = new BasicCredentialsProvider();
credentialsProvider.setCredentials(new AuthScope(null, -1), new UsernamePasswordCredentials(userName, password.toCharArray()));
try {
SSLContext sslContext = SSLContextBuilder.create().loadTrustMaterial(null, (chains, authType) -> true).build();

TlsStrategy tlsStrategy = ClientTlsStrategyBuilder.create()
.setSslContext(sslContext)
.setTlsVersions(new String[] { "TLSv1", "TLSv1.1", "TLSv1.2", "SSLv3" })
.setHostnameVerifier(NoopHostnameVerifier.INSTANCE)
// See please https://issues.apache.org/jira/browse/HTTPCLIENT-2219
.setTlsDetailsFactory(new Factory<SSLEngine, TlsDetails>() {
@Override
public TlsDetails create(final SSLEngine sslEngine) {
return new TlsDetails(sslEngine.getSession(), sslEngine.getApplicationProtocol());
}
})
.build();

final AsyncClientConnectionManager cm = PoolingAsyncClientConnectionManagerBuilder.create()
.setTlsStrategy(tlsStrategy)
.build();
return httpClientBuilder.setDefaultCredentialsProvider(credentialsProvider).setConnectionManager(cm);
} catch (Exception e) {
throw new RuntimeException(e);
}
});
}

public void testBasicBackwardsCompatibility() throws Exception {
String round = System.getProperty("tests.rest.bwcsuite_round");

Expand All @@ -73,6 +160,12 @@ public void testBasicBackwardsCompatibility() throws Exception {
}
}

@SuppressWarnings("unchecked")
public void testWhoAmI() throws Exception {
Map<String, Object> responseMap = (Map<String, Object>) getAsMap("_plugins/_security/whoami");
Assert.assertTrue(responseMap.containsKey("dn"));
}

private enum ClusterType {
OLD,
MIXED,
Expand Down