From d81fbf38375dbac48fd37213c61de16db300430c Mon Sep 17 00:00:00 2001 From: Vamsi Manohar Date: Tue, 11 Jul 2023 12:26:32 -0700 Subject: [PATCH] Remvoe Default master key Signed-off-by: Vamsi Manohar --- .../datasources/encryptor/EncryptorImpl.java | 17 +++++- .../rest/RestDataSourceQueryAction.java | 3 +- .../encryptor/EncryptorImplTest.java | 54 +++++++++++++++++++ docs/user/ppl/admin/datasources.rst | 4 +- .../setting/OpenSearchSettings.java | 1 - plugin/build.gradle | 6 +++ .../org/opensearch/sql/plugin/SQLPlugin.java | 12 ++++- 7 files changed, 90 insertions(+), 7 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..4a30acb707 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 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..6f9ab2bf0a 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,58 @@ 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 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 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 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 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..26992e4f0a 100644 --- a/docs/user/ppl/admin/datasources.rst +++ b/docs/user/ppl/admin/datasources.rst @@ -121,10 +121,8 @@ 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 python script to generate a 24 character master key :: import random 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..80031cc484 100644 --- a/plugin/build.gradle +++ b/plugin/build.gradle @@ -253,9 +253,15 @@ testClusters.integTest { } // add customized keystore + keystore 'plugins.query.federation.datasources.config', new File("$projectDir/src/test/resources/", 'datasources.json') } +testClusters.integTest.nodes.each { node -> + node.setting("plugins.query.datasources.encryption.masterkey", + "1234567812345678") +} + 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..4187e155bd 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