From 9a73353686b75d34c96851b042e94d5298106052 Mon Sep 17 00:00:00 2001 From: Vamsi Manohar Date: Tue, 11 Jul 2023 12:26:32 -0700 Subject: [PATCH] Remov:e Default master key Signed-off-by: Vamsi Manohar --- .../datasources/encryptor/EncryptorImpl.java | 17 +++++- .../rest/RestDataSourceQueryAction.java | 3 +- .../encryptor/EncryptorImplTest.java | 58 +++++++++++++++++++ docs/user/ppl/admin/datasources.rst | 10 +++- .../sql/datasource/DataSourceAPIsIT.java | 5 +- .../setting/OpenSearchSettings.java | 1 - plugin/build.gradle | 3 + .../org/opensearch/sql/plugin/SQLPlugin.java | 12 +++- 8 files changed, 101 insertions(+), 8 deletions(-) diff --git a/datasources/src/main/java/org/opensearch/sql/datasources/encryptor/EncryptorImpl.java b/datasources/src/main/java/org/opensearch/sql/datasources/encryptor/EncryptorImpl.java index 4838cd41a5..18e3e2f257 100644 --- a/datasources/src/main/java/org/opensearch/sql/datasources/encryptor/EncryptorImpl.java +++ b/datasources/src/main/java/org/opensearch/sql/datasources/encryptor/EncryptorImpl.java @@ -15,6 +15,7 @@ import java.util.Base64; import javax.crypto.spec.SecretKeySpec; import lombok.RequiredArgsConstructor; +import org.apache.commons.lang3.StringUtils; @RequiredArgsConstructor public class EncryptorImpl implements Encryptor { @@ -23,7 +24,7 @@ public class EncryptorImpl implements Encryptor { @Override public String encrypt(String plainText) { - + validate(masterKey); final AwsCrypto crypto = AwsCrypto.builder() .withCommitmentPolicy(CommitmentPolicy.RequireEncryptRequireDecrypt) .build(); @@ -39,6 +40,7 @@ public String encrypt(String plainText) { @Override public String decrypt(String encryptedText) { + validate(masterKey); final AwsCrypto crypto = AwsCrypto.builder() .withCommitmentPolicy(CommitmentPolicy.RequireEncryptRequireDecrypt) .build(); @@ -52,4 +54,17 @@ public String decrypt(String encryptedText) { return new String(decryptedResult.getResult()); } + private void validate(String masterKey) { + if (StringUtils.isEmpty(masterKey)) { + throw new IllegalStateException( + "Master key is a required config for using create and update datasource APIs." + + "Please set plugins.query.datasources.encryption.masterkey config " + + "in opensearch.yml in all the cluster nodes. " + + "More details can be found here: " + + "https://github.com/opensearch-project/sql/blob/main/docs/user/ppl/" + + "admin/datasources.rst#master-key-config-for-encrypting-credential-information"); + } + } + + } \ No newline at end of file diff --git a/datasources/src/main/java/org/opensearch/sql/datasources/rest/RestDataSourceQueryAction.java b/datasources/src/main/java/org/opensearch/sql/datasources/rest/RestDataSourceQueryAction.java index 95efd2e8f5..6cc5b3bd86 100644 --- a/datasources/src/main/java/org/opensearch/sql/datasources/rest/RestDataSourceQueryAction.java +++ b/datasources/src/main/java/org/opensearch/sql/datasources/rest/RestDataSourceQueryAction.java @@ -247,7 +247,8 @@ private void reportError(final RestChannel channel, final Exception e, final Res private static boolean isClientError(Exception e) { return e instanceof NullPointerException // NPE is hard to differentiate but more likely caused by bad query - || e instanceof IllegalArgumentException; + || e instanceof IllegalArgumentException + || e instanceof IllegalStateException; } } \ No newline at end of file diff --git a/datasources/src/test/java/org/opensearch/sql/datasources/encryptor/EncryptorImplTest.java b/datasources/src/test/java/org/opensearch/sql/datasources/encryptor/EncryptorImplTest.java index 22f5b09255..8c9b140f09 100644 --- a/datasources/src/test/java/org/opensearch/sql/datasources/encryptor/EncryptorImplTest.java +++ b/datasources/src/test/java/org/opensearch/sql/datasources/encryptor/EncryptorImplTest.java @@ -84,4 +84,62 @@ public void testDecryptWithDifferentKey() { encryptor2.decrypt(encrypted); }); } + + @Test + public void testEncryptionAndDecryptionWithNullMasterKey() { + String input = "This is a test input"; + Encryptor encryptor = new EncryptorImpl(null); + IllegalStateException illegalStateException + = Assertions.assertThrows(IllegalStateException.class, + () -> encryptor.encrypt(input)); + Assertions.assertEquals("Master key is a required config for using create and" + + " update datasource APIs." + + "Please set plugins.query.datasources.encryption.masterkey config " + + "in opensearch.yml in all the cluster nodes. " + + "More details can be found here: " + + "https://github.com/opensearch-project/sql/blob/main/docs/user/ppl/" + + "admin/datasources.rst#master-key-config-for-encrypting-credential-information", + illegalStateException.getMessage()); + illegalStateException + = Assertions.assertThrows(IllegalStateException.class, + () -> encryptor.decrypt(input)); + Assertions.assertEquals("Master key is a required config for using create and" + + " update datasource APIs." + + "Please set plugins.query.datasources.encryption.masterkey config " + + "in opensearch.yml in all the cluster nodes. " + + "More details can be found here: " + + "https://github.com/opensearch-project/sql/blob/main/docs/user/ppl/" + + "admin/datasources.rst#master-key-config-for-encrypting-credential-information", + illegalStateException.getMessage()); + } + + @Test + public void testEncryptionAndDecryptionWithEmptyMasterKey() { + String masterKey = ""; + String input = "This is a test input"; + Encryptor encryptor = new EncryptorImpl(masterKey); + IllegalStateException illegalStateException + = Assertions.assertThrows(IllegalStateException.class, + () -> encryptor.encrypt(input)); + Assertions.assertEquals("Master key is a required config for using create and" + + " update datasource APIs." + + "Please set plugins.query.datasources.encryption.masterkey config " + + "in opensearch.yml in all the cluster nodes. " + + "More details can be found here: " + + "https://github.com/opensearch-project/sql/blob/main/docs/user/ppl/" + + "admin/datasources.rst#master-key-config-for-encrypting-credential-information", + illegalStateException.getMessage()); + illegalStateException + = Assertions.assertThrows(IllegalStateException.class, + () -> encryptor.decrypt(input)); + Assertions.assertEquals("Master key is a required config for using create and" + + " update datasource APIs." + + "Please set plugins.query.datasources.encryption.masterkey config " + + "in opensearch.yml in all the cluster nodes. " + + "More details can be found here: " + + "https://github.com/opensearch-project/sql/blob/main/docs/user/ppl/" + + "admin/datasources.rst#master-key-config-for-encrypting-credential-information", + illegalStateException.getMessage()); + } + } \ No newline at end of file diff --git a/docs/user/ppl/admin/datasources.rst b/docs/user/ppl/admin/datasources.rst index 6a5871a9e9..0f026f72cf 100644 --- a/docs/user/ppl/admin/datasources.rst +++ b/docs/user/ppl/admin/datasources.rst @@ -121,10 +121,14 @@ Only users mapped with roles having above actions are authorized to execute data Master Key config for encrypting credential information ======================================================== * When users provide credentials for a data source, the system encrypts and securely stores them in the metadata index. System uses "AES/GCM/NoPadding" symmetric encryption algorithm. -* Users can set up a master key to use with this encryption method by configuring the plugins.query.datasources.encryption.masterkey setting in the opensearch.yml file. +* Master key is a required config and users can set this up by configuring the `plugins.query.datasources.encryption.masterkey` setting in the opensearch.yml file. * The master key must be 16, 24, or 32 characters long. -* It's highly recommended that users configure a master key for better security. -* If users don't provide a master key, the system will default to "0000000000000000". +* Sample Bash Script to generate a 24 character master key :: + + #!/bin/bash + # Generate a 24-character key + master_key=$(openssl rand -hex 12) + echo "Master Key: $master_key" * Sample python script to generate a 24 character master key :: import random diff --git a/integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceAPIsIT.java b/integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceAPIsIT.java index c942962fb8..86af85727d 100644 --- a/integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceAPIsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceAPIsIT.java @@ -50,7 +50,10 @@ public void createDataSourceAPITest() { //create datasource DataSourceMetadata createDSM = new DataSourceMetadata("create_prometheus", DataSourceType.PROMETHEUS, - ImmutableList.of(), ImmutableMap.of("prometheus.uri", "https://localhost:9090")); + ImmutableList.of(), ImmutableMap.of("prometheus.uri", "https://localhost:9090", + "prometheus.auth.type","basicauth", + "prometheus.auth.username", "username", + "prometheus.auth.password", "password")); Request createRequest = getCreateDataSourceRequest(createDSM); Response response = client().performRequest(createRequest); Assert.assertEquals(201, response.getStatusLine().getStatusCode()); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/setting/OpenSearchSettings.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/setting/OpenSearchSettings.java index 01c3aeb30d..0810312974 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/setting/OpenSearchSettings.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/setting/OpenSearchSettings.java @@ -111,7 +111,6 @@ public class OpenSearchSettings extends Settings { public static final Setting DATASOURCE_MASTER_SECRET_KEY = Setting.simpleString( ENCYRPTION_MASTER_KEY.getKeyValue(), - "0000000000000000", Setting.Property.NodeScope, Setting.Property.Final, Setting.Property.Filtered); diff --git a/plugin/build.gradle b/plugin/build.gradle index 11f97ea857..e13a498a8f 100644 --- a/plugin/build.gradle +++ b/plugin/build.gradle @@ -253,9 +253,12 @@ testClusters.integTest { } // add customized keystore + + setting "plugins.query.datasources.encryption.masterkey", "1234567812345678" keystore 'plugins.query.federation.datasources.config', new File("$projectDir/src/test/resources/", 'datasources.json') } + run { useCluster testClusters.integTest } diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java index 7e867be967..3adaf3eac5 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java @@ -15,6 +15,7 @@ import java.util.List; import java.util.Objects; import java.util.function.Supplier; +import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.action.ActionRequest; @@ -90,7 +91,8 @@ public class SQLPlugin extends Plugin implements ActionPlugin, ScriptPlugin { - private static final Logger LOG = LogManager.getLogger(); + private static final Logger LOGGER = LogManager.getLogger(SQLPlugin.class); + private ClusterService clusterService; /** * Settings should be inited when bootstrap the plugin. @@ -212,6 +214,14 @@ public ScriptEngine getScriptEngine(Settings settings, Collection