Skip to content

Commit

Permalink
[2.x] Enable security for bwc tests (#3257)
Browse files Browse the repository at this point in the history
### Description

Opening up a PR to describe the issues faced with BWC tests with the
security plugin installed and solicit feedback.

I plan to forward port this change to main, but first wanted to show
this working for 2.9 -> 2.10 tests (as of the time of this writing).

Thanks to the work that @scrawfor99 did in
[core](opensearch-project/OpenSearch#8900) to
supply security settings to testClusters to be able to run the initial
wait for cluster yellow checks with a URL that includes the right
protocol (`https` when security is enabled) along with a username and
password to authenticate the request.

I ran into 4 hurdles to get this to run:

1. Initially the cluster didn't form. After a lot of frustration, I
ended up finding that by supplying `network.bind_host` and
`network.publish_host` to both 127.0.0.1 it resolved the issue. These
could probably be combined into a single `network.host`, but I chose to
keep them separated.
2. I had issue testing changes to the gradle build-tools after making
changes locally. This was the most frustrating hurdle, but ultimately
the solution was to change the [`opensearch.version` setting in
`bwc-test/build.gradle`](https://github.com/opensearch-project/security/blob/2.x/bwc-test/build.gradle#L47)
to `2.10.0-SNAPSHOT`. This value is specifically used as the version of
the gradle build-tools that the [BWC tests
use](https://github.com/opensearch-project/security/blob/main/bwc-test/build.gradle#L58).
The changes I made locally didn't reflect because I was publishing to
maven local from the 2.x branch (currently 2.10) and it was looking for
2.9.0-SNAPSHOT artifacts. After updating the value it found my maven
local snapshots. For this artifact you can produce maven local snapshots
using `./gradlew :build-tools:publishToMavenLocal` from the respective
branch in the core repo.
3. After the waitForYellow checks were able to run successfully, the
REST Client in the SecurityBackwardsCompatibilityIT was also having
problems connecting to the cluster because it didn't recognize the
certificates of the server. I ended up using the overly trustworthy
route where there is no SSL verification for the REST Client used in
this test. I borrowed this implementation from [k-NN's
ODFERestTestCase](https://github.com/opensearch-project/k-NN/blob/2.x/src/testFixtures/java/org/opensearch/knn/ODFERestTestCase.java#L118-L141)
which is widely used in the plugin ecosystem. There is an open issue to
abstract this class into common-utils. More work can be done here to
ensure the rest-high-level-client runs with a truststore with the root
certificate.
4. The last hurdle I faced was a WarningFailureException where the REST
Client could not deserialize the cluster health response because of a
warning that was returned with the response about the request including
system indices. According to this
[comment](opensearch-project/OpenSearch#1108 (comment)),
this may only be enabled in snapshots. To fix this, I set preserve
cluster to true which [bypasses the
method](https://github.com/opensearch-project/OpenSearch/blob/main/test/framework/src/main/java/org/opensearch/test/rest/OpenSearchRestTestCase.java#L364)
where the error was thrown.

* Category (Enhancement, New feature, Bug fix, Test fix, Refactoring,
Maintenance, Documentation)

Enhancement

### Issues Resolved

#3056

### 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
  • Loading branch information
cwperks committed Aug 30, 2023
1 parent 8197431 commit d1590c6
Show file tree
Hide file tree
Showing 5 changed files with 91 additions and 7 deletions.
4 changes: 2 additions & 2 deletions .github/actions/create-bwc-build/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ runs:
uses: gradle/gradle-build-action@v2
with:
cache-disabled: true
arguments: assemble -Dbuild.snapshot=false
arguments: assemble
build-root-directory: ${{ inputs.plugin-branch }}

- id: get-opensearch-version
Expand All @@ -47,5 +47,5 @@ runs:
- name: Copy current distro into the expected folder
run: |
mkdir -p ./bwc-test/src/test/resources/${{ steps.get-opensearch-version.outputs.version }}
cp ${{ inputs.plugin-branch }}/build/distributions/opensearch-security-${{ steps.get-opensearch-version.outputs.version }}.zip ./bwc-test/src/test/resources/${{ steps.get-opensearch-version.outputs.version }}
cp ${{ inputs.plugin-branch }}/build/distributions/opensearch-security-${{ steps.get-opensearch-version.outputs.version }}-SNAPSHOT.zip ./bwc-test/src/test/resources/${{ steps.get-opensearch-version.outputs.version }}
shell: bash
11 changes: 11 additions & 0 deletions .github/actions/run-bwc-suite/action.yaml
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
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ jobs:
plugin-previous-branch: "2.9"
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
13 changes: 8 additions & 5 deletions bwc-test/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ ext {

buildscript {
ext {
opensearch_version = System.getProperty("opensearch.version", "2.9.0-SNAPSHOT")
opensearch_version = System.getProperty("opensearch.version", "2.10.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 @@ -84,8 +86,8 @@ String baseName = "securityBwcCluster"
String bwcFilePath = "src/test/resources/"
String projectVersion = nextVersion

String previousOpenSearch = extractVersion(previousVersion);
String nextOpenSearch = extractVersion(nextVersion);
String previousOpenSearch = extractVersion(previousVersion) + "-SNAPSHOT";
String nextOpenSearch = extractVersion(nextVersion) + "-SNAPSHOT";

// Extracts the OpenSearch version from a plugin version string, 2.4.0.0 -> 2.4.0.
def String extractVersion(versionStr) {
Expand Down Expand Up @@ -122,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 @@ -134,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
68 changes: 68 additions & 0 deletions bwc-test/src/test/java/SecurityBackwardsCompatibilityIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,38 @@
*/
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.http.Header;
import org.apache.http.HttpHost;
import org.apache.http.auth.AuthScope;
import org.apache.http.auth.UsernamePasswordCredentials;
import org.apache.http.client.CredentialsProvider;
import org.apache.http.conn.ssl.NoopHostnameVerifier;
import org.apache.http.impl.client.BasicCredentialsProvider;
import org.apache.http.message.BasicHeader;
import org.apache.http.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 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;

public class SecurityBackwardsCompatibilityIT extends OpenSearchRestTestCase {

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

@Override
protected final boolean preserveClusterUponCompletion() {
return true;
}

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

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

@Override
protected final Settings restClientSettings() {
return Settings.builder()
Expand All @@ -61,6 +88,41 @@ 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());
}
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"));
CredentialsProvider credentialsProvider = new BasicCredentialsProvider();
credentialsProvider.setCredentials(AuthScope.ANY, new UsernamePasswordCredentials(userName, password));
try {
return httpClientBuilder.setDefaultCredentialsProvider(credentialsProvider)
// disable the certificate since our testing cluster just uses the default security configuration
.setSSLHostnameVerifier(NoopHostnameVerifier.INSTANCE)
.setSSLContext(SSLContextBuilder.create().loadTrustMaterial(null, (chains, authType) -> true).build());
} catch (Exception e) {
throw new RuntimeException(e);
}
});
}

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

Expand All @@ -73,6 +135,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

0 comments on commit d1590c6

Please sign in to comment.