From d1590c6791a6a29f80f9068da8aedaa27c9f09e9 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 30 Aug 2023 08:56:30 -0400 Subject: [PATCH] [2.x] Enable security for bwc tests (#3257) ### 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](https://github.com/opensearch-project/OpenSearch/pull/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](https://github.com/opensearch-project/OpenSearch/issues/1108#issuecomment-886155806), 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 https://github.com/opensearch-project/security/issues/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 --- .github/actions/create-bwc-build/action.yaml | 4 +- .github/actions/run-bwc-suite/action.yaml | 11 +++ .github/workflows/ci.yml | 2 + bwc-test/build.gradle | 13 ++-- .../SecurityBackwardsCompatibilityIT.java | 68 +++++++++++++++++++ 5 files changed, 91 insertions(+), 7 deletions(-) diff --git a/.github/actions/create-bwc-build/action.yaml b/.github/actions/create-bwc-build/action.yaml index bfe64ff59b..25a348bcc0 100644 --- a/.github/actions/create-bwc-build/action.yaml +++ b/.github/actions/create-bwc-build/action.yaml @@ -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 @@ -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 diff --git a/.github/actions/run-bwc-suite/action.yaml b/.github/actions/run-bwc-suite/action.yaml index 6771faddab..6e6a17fb3f 100644 --- a/.github/actions/run-bwc-suite/action.yaml +++ b/.github/actions/run-bwc-suite/action.yaml @@ -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: @@ -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 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f750f75794..05ff3e2c17 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -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 diff --git a/bwc-test/build.gradle b/bwc-test/build.gradle index 9b8d9fcc0a..9999c631dc 100644 --- a/bwc-test/build.gradle +++ b/bwc-test/build.gradle @@ -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() @@ -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 @@ -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) { @@ -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") @@ -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\"]") diff --git a/bwc-test/src/test/java/SecurityBackwardsCompatibilityIT.java b/bwc-test/src/test/java/SecurityBackwardsCompatibilityIT.java index 6415a23bea..2447bb9fa9 100644 --- a/bwc-test/src/test/java/SecurityBackwardsCompatibilityIT.java +++ b/bwc-test/src/test/java/SecurityBackwardsCompatibilityIT.java @@ -7,14 +7,26 @@ */ 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; @@ -22,6 +34,11 @@ 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; @@ -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; @@ -50,6 +72,11 @@ protected boolean preserveTemplatesUponCompletion() { return true; } + @Override + protected String getProtocol() { + return "https"; + } + @Override protected final Settings restClientSettings() { return Settings.builder() @@ -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 headers = ThreadContext.buildDefaultHeaders(settings); + Header[] defaultHeaders = new Header[headers.size()]; + int i = 0; + for (Map.Entry 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"); @@ -73,6 +135,12 @@ public void testBasicBackwardsCompatibility() throws Exception { } } + @SuppressWarnings("unchecked") + public void testWhoAmI() throws Exception { + Map responseMap = (Map) getAsMap("_plugins/_security/whoami"); + Assert.assertTrue(responseMap.containsKey("dn")); + } + private enum ClusterType { OLD, MIXED,