From 800fe97e151cc1d0624d459f507d9b153a7fedd2 Mon Sep 17 00:00:00 2001
From: Nick Maludy <nmaludy@gmail.com>
Date: Tue, 12 Feb 2019 20:20:44 -0500
Subject: [PATCH 01/12] Added ability to pass in encrypted values into the CLI
 and API then decrypt them prior to being stored in the database

---
 CHANGELOG.rst                                 | 10 ++++
 st2api/tests/unit/controllers/v1/test_kvps.py | 47 ++++++++++++++++++
 st2client/st2client/commands/keyvalue.py      | 14 +++++-
 st2client/tests/unit/test_keyvalue.py         | 49 ++++++++++++++++++-
 st2common/st2common/models/api/keyvalue.py    | 17 +++++++
 st2common/st2common/openapi.yaml              |  3 ++
 st2common/st2common/openapi.yaml.j2           |  3 ++
 7 files changed, 141 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index c75117f82e..8849a14e74 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -17,6 +17,16 @@ Added
 
    For backward compatibility reasons, if pack metadata file doesn't contain that attribute, it's
    assumed it only works with Python 2. (new feature) #4474
+* Added a new flag ``-d/--decrypt`` to ``st2 key set`` that allows users to pass in values
+  in encrypted format using the system's crypto keys. This flag informs the API that the
+  value transmitted is encrypted and it must be decrypted prior to working with it. Similarly
+  a keys file given to ``st2 key load`` can contain the property ``decrypt: true`` in any of
+  its keys and it will have the same effect. This results in keys files being able to contain
+  encrypted data that no longer needs to be decrypted prior to working with the CLI.
+  The corresponding ``decrypt`` option has been added to the API endpoint
+  ``PUT /api/v1/keys/{name}``. (new feature) #4545
+
+  Contributed by Nick Maludy (Encore Technologies)  
 
 Changed
 ~~~~~~~
diff --git a/st2api/tests/unit/controllers/v1/test_kvps.py b/st2api/tests/unit/controllers/v1/test_kvps.py
index 2edcefca39..ad0d8bba5a 100644
--- a/st2api/tests/unit/controllers/v1/test_kvps.py
+++ b/st2api/tests/unit/controllers/v1/test_kvps.py
@@ -70,6 +70,23 @@
     'secret': True
 }
 
+# value = S3cret!Value
+# encrypted with st2tests/conf/st2_kvstore_tests.crypto.key.json
+ENCRYPTED_KVP = {
+    'name': 'secret_key1',
+    'value': ('3030303030298D848B45A24EDCD1A82FAB4E831E3FCE6E60956817A48A180E4C040801E'
+              'B30170DACF79498F30520236A629912C3584847098D'),
+    'decrypt': True,
+}
+
+ENCRYPTED_SECRET_KVP = {
+    'name': 'secret_key1',
+    'value': ('3030303030298D848B45A24EDCD1A82FAB4E831E3FCE6E60956817A48A180E4C040801E'
+              'B30170DACF79498F30520236A629912C3584847098D'),
+    'secret': True,
+    'decrypt': True,
+}
+
 
 class KeyValuePairControllerTestCase(FunctionalTest):
 
@@ -468,6 +485,36 @@ def test_get_all_decrypt(self):
         self.__do_delete(kvp_id_1)
         self.__do_delete(kvp_id_2)
 
+    def test_put_decrypt(self):
+        put_resp = self.__do_put('secret_key1', ENCRYPTED_KVP)
+        kvp_id = self.__get_kvp_id(put_resp)
+
+        # ensure that the data was decrypted prior to be stored in the datastore
+        get_resp = self.__do_get_one(kvp_id)
+        self.assertFalse(get_resp.json['encrypted'])
+        self.assertEqual(get_resp.json['value'], 'S3cret!Value')
+        self.__do_delete(self.__get_kvp_id(put_resp))
+
+    def test_put_decrypt_and_secret(self):
+        put_resp = self.__do_put('secret_key1', ENCRYPTED_SECRET_KVP)
+        kvp_id = self.__get_kvp_id(put_resp)
+
+        # ensure the value was decrypted and re-encrypted, meaning a different salt
+        # and resulting in a uniquely encrypted value in the datastore
+        get_resp = self.__do_get_one(kvp_id)
+        self.assertTrue(get_resp.json['encrypted'])
+        crypto_val = get_resp.json['value']
+        self.assertNotEqual(ENCRYPTED_KVP['value'], crypto_val)
+
+        # ensure that the data was decrypted prior to be stored in the datastore
+        get_resp = self.app.get('/v1/keys/secret_key1?decrypt=true')
+        self.assertEqual(get_resp.status_int, 200)
+        self.assertEqual(self.__get_kvp_id(get_resp), kvp_id)
+        self.assertTrue(get_resp.json['secret'])
+        self.assertFalse(get_resp.json['encrypted'])
+        self.assertEqual(get_resp.json['value'], 'S3cret!Value')
+        self.__do_delete(self.__get_kvp_id(put_resp))
+
     def test_put_delete(self):
         put_resp = self.__do_put('key1', KVP)
         self.assertEqual(put_resp.status_int, 200)
diff --git a/st2client/st2client/commands/keyvalue.py b/st2client/st2client/commands/keyvalue.py
index d6ab3803e8..763402f943 100644
--- a/st2client/st2client/commands/keyvalue.py
+++ b/st2client/st2client/commands/keyvalue.py
@@ -84,7 +84,7 @@ def __init__(self, resource, *args, **kwargs):
         # Filter options
         self.parser.add_argument('--prefix', help=('Only return values with names starting with '
                                                    'the provided prefix.'))
-        self.parser.add_argument('--decrypt', action='store_true',
+        self.parser.add_argument('-d', '--decrypt', action='store_true',
                                  help='Decrypt secrets and displays plain text.')
         self.parser.add_argument('-s', '--scope', default=DEFAULT_LIST_SCOPE, dest='scope',
                                  help='Scope item is under. Example: "user".')
@@ -168,6 +168,12 @@ def __init__(self, resource, *args, **kwargs):
         self.parser.add_argument('-e', '--encrypt', dest='secret',
                                  action='store_true',
                                  help='Encrypt value before saving.')
+        self.parser.add_argument('-d', '--decrypt', dest='decrypt',
+                                 action='store_true',
+                                 help=('Value provided is encrypted and must be decrypted'
+                                       ' before saving. This allows values to be entered in on'
+                                       ' the CLI and transmitted to the API in encrypted'
+                                       ' format.'))
         self.parser.add_argument('-s', '--scope', dest='scope', default=DEFAULT_CUD_SCOPE,
                                  help='Specify the scope under which you want ' +
                                       'to place the item.')
@@ -186,6 +192,9 @@ def run(self, args, **kwargs):
         if args.secret:
             instance.secret = args.secret
 
+        if args.decrypt:
+            instance.decrypt = args.decrypt
+
         if args.ttl:
             instance.ttl = args.ttl
 
@@ -313,6 +322,7 @@ def run(self, args, **kwargs):
             scope = item.get('scope', DEFAULT_CUD_SCOPE)
             user = item.get('user', None)
             secret = item.get('secret', False)
+            decrypt = item.get('decrypt', False)
             ttl = item.get('ttl', None)
 
             # if the value is not a string, convert it to JSON
@@ -336,6 +346,8 @@ def run(self, args, **kwargs):
                 instance.user = user
             if secret:
                 instance.secret = secret
+            if decrypt:
+                instance.decrypt = decrypt
             if ttl:
                 instance.ttl = ttl
 
diff --git a/st2client/tests/unit/test_keyvalue.py b/st2client/tests/unit/test_keyvalue.py
index 4ebcba1d8b..29ecc5c030 100644
--- a/st2client/tests/unit/test_keyvalue.py
+++ b/st2client/tests/unit/test_keyvalue.py
@@ -51,6 +51,15 @@
     'secret': True
 }
 
+KEYVALUE_DECRYPT = {
+    'id': 'kv_name',
+    'name': 'kv_name.',
+    'value': 'AAABBBCCC1234',
+    'scope': 'system',
+    'secret': True,
+    'decrypt': True
+}
+
 KEYVALUE_TTL = {
     'id': 'kv_name',
     'name': 'kv_name.',
@@ -69,10 +78,11 @@
 KEYVALUE_ALL = {
     'id': 'kv_name',
     'name': 'kv_name.',
-    'value': 'super cool value',
+    'value': 'AAAAABBBBBCCCCCCDDDDD11122345',
     'scope': 'system',
     'user': 'stanley',
     'secret': True,
+    'decrypt': True,
     'ttl': 100
 }
 
@@ -108,6 +118,25 @@ def tearDown(self):
         super(TestKeyValueBase, self).tearDown()
 
 
+class TestKeyValueSet(TestKeyValueBase):
+
+    @mock.patch.object(
+        requests, 'put',
+        mock.MagicMock(return_value=base.FakeResponse(json.dumps(KEYVALUE_DECRYPT), 200, 'OK')))
+    def test_set_keyvalue(self):
+        """Test setting key/value pair with optional derypt field
+        """
+        # short format
+        args = ['key', 'set', '-d', 'kv_name', 'AAABBBCCC1234']
+        retcode = self.shell.run(args)
+        self.assertEqual(retcode, 0)
+
+        # long format
+        args = ['key', 'set', '--decrypt', 'kv_name', 'AAABBBCCC1234']
+        retcode = self.shell.run(args)
+        self.assertEqual(retcode, 0)
+
+
 class TestKeyValueLoad(TestKeyValueBase):
 
     @mock.patch.object(
@@ -182,6 +211,24 @@ def test_load_keyvalue_secret(self):
             os.close(fd)
             os.unlink(path)
 
+    @mock.patch.object(
+        requests, 'put',
+        mock.MagicMock(return_value=base.FakeResponse(json.dumps(KEYVALUE_DECRYPT), 200, 'OK')))
+    def test_load_keyvalue_decrypt(self):
+        """Test loading of key/value pair with the optional decrypt field
+        """
+        fd, path = tempfile.mkstemp(suffix='.json')
+        try:
+            with open(path, 'a') as f:
+                f.write(json.dumps(KEYVALUE_DECRYPT, indent=4))
+
+            args = ['key', 'load', path]
+            retcode = self.shell.run(args)
+            self.assertEqual(retcode, 0)
+        finally:
+            os.close(fd)
+            os.unlink(path)
+
     @mock.patch.object(
         requests, 'put',
         mock.MagicMock(return_value=base.FakeResponse(json.dumps(KEYVALUE_TTL), 200, 'OK')))
diff --git a/st2common/st2common/models/api/keyvalue.py b/st2common/st2common/models/api/keyvalue.py
index 45ad1738ae..13c3574c18 100644
--- a/st2common/st2common/models/api/keyvalue.py
+++ b/st2common/st2common/models/api/keyvalue.py
@@ -67,6 +67,11 @@ class KeyValuePairAPI(BaseAPI):
                 'required': False,
                 'default': False
             },
+            'decrypt': {
+                'type': 'boolean',
+                'required': False,
+                'default': False
+            },
             'encrypted': {
                 'type': 'boolean',
                 'required': False,
@@ -158,6 +163,7 @@ def to_model(cls, kvp):
         description = getattr(kvp, 'description', None)
         value = kvp.value
         secret = False
+        decrypt = False
 
         if getattr(kvp, 'ttl', None):
             expire_timestamp = (date_utils.get_datetime_utc_now() +
@@ -165,8 +171,19 @@ def to_model(cls, kvp):
         else:
             expire_timestamp = None
 
+        decrypt = getattr(kvp, 'decrypt', False)
         secret = getattr(kvp, 'secret', False)
 
+        # if the user transmitted the value in encrypted format and is requesting
+        # that we decrypt the value prior to processing (may get re-encrypted when saved
+        # if secret=True).
+        if decrypt:
+            if not KeyValuePairAPI.crypto_key:
+                msg = ('Crypto key not found in %s. Unable to encrypt value for key %s.' %
+                       (KeyValuePairAPI.crypto_key_path, name))
+                raise CryptoKeyNotSetupException(msg)
+            value = symmetric_decrypt(KeyValuePairAPI.crypto_key, value)
+
         if secret:
             if not KeyValuePairAPI.crypto_key:
                 msg = ('Crypto key not found in %s. Unable to encrypt value for key %s.' %
diff --git a/st2common/st2common/openapi.yaml b/st2common/st2common/openapi.yaml
index 82e914f8bf..a4c63631bb 100644
--- a/st2common/st2common/openapi.yaml
+++ b/st2common/st2common/openapi.yaml
@@ -4889,6 +4889,9 @@ definitions:
       secret:
         description: Encrypt value before saving the value.
         type: boolean
+      decrypt:
+        description: Value provided is encrypted with this instances crypto key and must be decrypted when received. This allows values to be transmitted in encrypted format.
+        type: boolean
       scope:
         description: "Scope the item is under. Example: 'user'."
         type: string
diff --git a/st2common/st2common/openapi.yaml.j2 b/st2common/st2common/openapi.yaml.j2
index b7f1d56768..a5ee7a492b 100644
--- a/st2common/st2common/openapi.yaml.j2
+++ b/st2common/st2common/openapi.yaml.j2
@@ -4885,6 +4885,9 @@ definitions:
       secret:
         description: Encrypt value before saving the value.
         type: boolean
+      decrypt:
+        description: Value provided is encrypted with this instances crypto key and must be decrypted when received. This allows values to be transmitted in encrypted format.
+        type: boolean
       scope:
         description: "Scope the item is under. Example: 'user'."
         type: string

From 820b809c40b5e41905cfbb8a463d822e14f48f21 Mon Sep 17 00:00:00 2001
From: Tomaz Muraus <tomaz@tomaz.me>
Date: Mon, 18 Mar 2019 17:09:12 +0100
Subject: [PATCH 02/12] Throw an error if we fail to decrypt a value when
 working with a preencrypted value.

---
 st2common/st2common/models/api/keyvalue.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/st2common/st2common/models/api/keyvalue.py b/st2common/st2common/models/api/keyvalue.py
index 13c3574c18..fcacc9c7b7 100644
--- a/st2common/st2common/models/api/keyvalue.py
+++ b/st2common/st2common/models/api/keyvalue.py
@@ -182,7 +182,13 @@ def to_model(cls, kvp):
                 msg = ('Crypto key not found in %s. Unable to encrypt value for key %s.' %
                        (KeyValuePairAPI.crypto_key_path, name))
                 raise CryptoKeyNotSetupException(msg)
-            value = symmetric_decrypt(KeyValuePairAPI.crypto_key, value)
+
+            try:
+                value = symmetric_decrypt(KeyValuePairAPI.crypto_key, value)
+            except Exception:
+                msg = ('Failed to decrypt the provided value. Ensure that the value is encrypted'
+                       ' with the correct key and not corrupted.')
+                raise ValueError(msg)
 
         if secret:
             if not KeyValuePairAPI.crypto_key:

From 9a8d1fda2bde1dc997b2093a0708f3f0300042e6 Mon Sep 17 00:00:00 2001
From: Tomaz Muraus <tomaz@tomaz.me>
Date: Mon, 18 Mar 2019 17:35:55 +0100
Subject: [PATCH 03/12] Per discussion with @nmaludy on Slack, make the
 following changes:

1. Rename attribute and option name from "decrypt" to "pre_encrypted".
   This is a more accurate name and signals the API that the value is
   already encrypted and should be stored as-is.
2. Make sure pre_encrypted always implies secret=True. If we didn't do
   that, any user could read (decrypt) any encrypted value. Big no-no.
3. For additional safety, only allow RBAC admins to use "pre_encrypted"
   functionality.
---
 st2api/st2api/controllers/v1/keyvalue.py      | 11 ++++
 st2api/tests/unit/controllers/v1/test_kvps.py |  8 +--
 st2client/st2client/commands/keyvalue.py      | 18 +++----
 st2client/tests/unit/test_keyvalue.py         |  9 +---
 st2common/st2common/models/api/keyvalue.py    | 53 ++++++++++++-------
 st2common/st2common/openapi.yaml              |  4 +-
 6 files changed, 61 insertions(+), 42 deletions(-)

diff --git a/st2api/st2api/controllers/v1/keyvalue.py b/st2api/st2api/controllers/v1/keyvalue.py
index 8e0a2ec15e..31bde5aef3 100644
--- a/st2api/st2api/controllers/v1/keyvalue.py
+++ b/st2api/st2api/controllers/v1/keyvalue.py
@@ -274,6 +274,11 @@ def put(self, kvp, name, requester_user, scope=FULL_SYSTEM_SCOPE):
                                                              user=user,
                                                              require_rbac=True)
 
+        # Validate that pre_encrypted option can only be used by admins
+        pre_encrypted = getattr(kvp, 'pre_encrypted', False)
+        self._validate_pre_encrypted_query_parameter(pre_encrypted=pre_encrypted, scope=scope,
+                                                     requester_user=requester_user)
+
         key_ref = get_key_reference(scope=scope, name=name, user=user)
         lock_name = self._get_lock_name_for_key(name=key_ref, scope=scope)
         LOG.debug('PUT scope: %s, name: %s', scope, name)
@@ -409,6 +414,12 @@ def _validate_decrypt_query_parameter(self, decrypt, scope, requester_user):
             msg = 'Decrypt option requires administrator access'
             raise AccessDeniedError(message=msg, user_db=requester_user)
 
+    def _validate_pre_encrypted_query_parameter(self, pre_encrypted, scope, requester_user):
+        is_admin = rbac_utils.user_is_admin(user_db=requester_user)
+        if pre_encrypted and not is_admin:
+            msg = 'Pre-encrypted option requires administrator access'
+            raise AccessDeniedError(message=msg, user_db=requester_user)
+
     def _validate_scope(self, scope):
         if scope not in ALLOWED_SCOPES:
             msg = 'Scope %s is not in allowed scopes list: %s.' % (scope, ALLOWED_SCOPES)
diff --git a/st2api/tests/unit/controllers/v1/test_kvps.py b/st2api/tests/unit/controllers/v1/test_kvps.py
index ad0d8bba5a..14e0edc464 100644
--- a/st2api/tests/unit/controllers/v1/test_kvps.py
+++ b/st2api/tests/unit/controllers/v1/test_kvps.py
@@ -76,7 +76,7 @@
     'name': 'secret_key1',
     'value': ('3030303030298D848B45A24EDCD1A82FAB4E831E3FCE6E60956817A48A180E4C040801E'
               'B30170DACF79498F30520236A629912C3584847098D'),
-    'decrypt': True,
+    'pre_encrypted': True
 }
 
 ENCRYPTED_SECRET_KVP = {
@@ -84,7 +84,7 @@
     'value': ('3030303030298D848B45A24EDCD1A82FAB4E831E3FCE6E60956817A48A180E4C040801E'
               'B30170DACF79498F30520236A629912C3584847098D'),
     'secret': True,
-    'decrypt': True,
+    'pre_encrypted': True
 }
 
 
@@ -485,7 +485,7 @@ def test_get_all_decrypt(self):
         self.__do_delete(kvp_id_1)
         self.__do_delete(kvp_id_2)
 
-    def test_put_decrypt(self):
+    def test_put_pre_encrypted_balue(self):
         put_resp = self.__do_put('secret_key1', ENCRYPTED_KVP)
         kvp_id = self.__get_kvp_id(put_resp)
 
@@ -495,7 +495,7 @@ def test_put_decrypt(self):
         self.assertEqual(get_resp.json['value'], 'S3cret!Value')
         self.__do_delete(self.__get_kvp_id(put_resp))
 
-    def test_put_decrypt_and_secret(self):
+    def test_put_pre_encreypted_and_secret(self):
         put_resp = self.__do_put('secret_key1', ENCRYPTED_SECRET_KVP)
         kvp_id = self.__get_kvp_id(put_resp)
 
diff --git a/st2client/st2client/commands/keyvalue.py b/st2client/st2client/commands/keyvalue.py
index 763402f943..45f95ebe95 100644
--- a/st2client/st2client/commands/keyvalue.py
+++ b/st2client/st2client/commands/keyvalue.py
@@ -168,12 +168,10 @@ def __init__(self, resource, *args, **kwargs):
         self.parser.add_argument('-e', '--encrypt', dest='secret',
                                  action='store_true',
                                  help='Encrypt value before saving.')
-        self.parser.add_argument('-d', '--decrypt', dest='decrypt',
+        self.parser.add_argument('--pre-encrypted', dest='pre_encrypted',
                                  action='store_true',
-                                 help=('Value provided is encrypted and must be decrypted'
-                                       ' before saving. This allows values to be entered in on'
-                                       ' the CLI and transmitted to the API in encrypted'
-                                       ' format.'))
+                                 help=('Value provided is already encrypted with the instance '
+                                 'crypto key and should be stored as-is.'))
         self.parser.add_argument('-s', '--scope', dest='scope', default=DEFAULT_CUD_SCOPE,
                                  help='Specify the scope under which you want ' +
                                       'to place the item.')
@@ -192,8 +190,8 @@ def run(self, args, **kwargs):
         if args.secret:
             instance.secret = args.secret
 
-        if args.decrypt:
-            instance.decrypt = args.decrypt
+        if args.pre_encrypted:
+            instance.pre_encrypted = args.pre_encrypted
 
         if args.ttl:
             instance.ttl = args.ttl
@@ -322,7 +320,7 @@ def run(self, args, **kwargs):
             scope = item.get('scope', DEFAULT_CUD_SCOPE)
             user = item.get('user', None)
             secret = item.get('secret', False)
-            decrypt = item.get('decrypt', False)
+            pre_encrypted = item.get('pre_encrypted', False)
             ttl = item.get('ttl', None)
 
             # if the value is not a string, convert it to JSON
@@ -346,8 +344,8 @@ def run(self, args, **kwargs):
                 instance.user = user
             if secret:
                 instance.secret = secret
-            if decrypt:
-                instance.decrypt = decrypt
+            if pre_encrypted:
+                instance.pre_encrypted = pre_encrypted
             if ttl:
                 instance.ttl = ttl
 
diff --git a/st2client/tests/unit/test_keyvalue.py b/st2client/tests/unit/test_keyvalue.py
index 29ecc5c030..bca5d2c02e 100644
--- a/st2client/tests/unit/test_keyvalue.py
+++ b/st2client/tests/unit/test_keyvalue.py
@@ -57,7 +57,7 @@
     'value': 'AAABBBCCC1234',
     'scope': 'system',
     'secret': True,
-    'decrypt': True
+    'pre_encrypted': True
 }
 
 KEYVALUE_TTL = {
@@ -126,13 +126,8 @@ class TestKeyValueSet(TestKeyValueBase):
     def test_set_keyvalue(self):
         """Test setting key/value pair with optional derypt field
         """
-        # short format
-        args = ['key', 'set', '-d', 'kv_name', 'AAABBBCCC1234']
-        retcode = self.shell.run(args)
-        self.assertEqual(retcode, 0)
-
         # long format
-        args = ['key', 'set', '--decrypt', 'kv_name', 'AAABBBCCC1234']
+        args = ['key', 'set', '--pre-encrypted', 'kv_name', 'AAABBBCCC1234']
         retcode = self.shell.run(args)
         self.assertEqual(retcode, 0)
 
diff --git a/st2common/st2common/models/api/keyvalue.py b/st2common/st2common/models/api/keyvalue.py
index fcacc9c7b7..945fedbfac 100644
--- a/st2common/st2common/models/api/keyvalue.py
+++ b/st2common/st2common/models/api/keyvalue.py
@@ -67,7 +67,7 @@ class KeyValuePairAPI(BaseAPI):
                 'required': False,
                 'default': False
             },
-            'decrypt': {
+            'pre_encrypted': {
                 'type': 'boolean',
                 'required': False,
                 'default': False
@@ -162,8 +162,8 @@ def to_model(cls, kvp):
         name = getattr(kvp, 'name', None)
         description = getattr(kvp, 'description', None)
         value = kvp.value
+        original_value = value
         secret = False
-        decrypt = False
 
         if getattr(kvp, 'ttl', None):
             expire_timestamp = (date_utils.get_datetime_utc_now() +
@@ -171,30 +171,32 @@ def to_model(cls, kvp):
         else:
             expire_timestamp = None
 
-        decrypt = getattr(kvp, 'decrypt', False)
+        pre_encrypted = getattr(kvp, 'pre_encrypted', False)
         secret = getattr(kvp, 'secret', False)
 
-        # if the user transmitted the value in encrypted format and is requesting
-        # that we decrypt the value prior to processing (may get re-encrypted when saved
-        # if secret=True).
-        if decrypt:
-            if not KeyValuePairAPI.crypto_key:
-                msg = ('Crypto key not found in %s. Unable to encrypt value for key %s.' %
-                       (KeyValuePairAPI.crypto_key_path, name))
-                raise CryptoKeyNotSetupException(msg)
+        # If user transmitted the value in an pre-encrypted format, we perform the decryption here
+        # to ensure data integrity. Besides that, we store data as-is.
+        # Keep in mind that pre_encrypted=True also always implies secret=True. If we didn't do
+        # that and supported pre_encrypted=True, secret=False, this would allow users to decrypt
+        # any encrypted value.
+        if pre_encrypted:
+            secret = True
+
+            cls._verif_key_is_set_up(name=name)
 
             try:
-                value = symmetric_decrypt(KeyValuePairAPI.crypto_key, value)
+                symmetric_decrypt(KeyValuePairAPI.crypto_key, value)
             except Exception:
-                msg = ('Failed to decrypt the provided value. Ensure that the value is encrypted'
-                       ' with the correct key and not corrupted.')
+                msg = ('Failed to verify the integrity of the provided value for key "%s". Ensure '
+                       'that the value is encrypted with the correct key and not corrupted.' %
+                       (name))
                 raise ValueError(msg)
 
-        if secret:
-            if not KeyValuePairAPI.crypto_key:
-                msg = ('Crypto key not found in %s. Unable to encrypt value for key %s.' %
-                       (KeyValuePairAPI.crypto_key_path, name))
-                raise CryptoKeyNotSetupException(msg)
+            # Additional safety check to ensure that the value hasn't been decrypted
+            assert value == original_value
+        elif secret:
+            cls._verif_key_is_set_up(name=name)
+
             value = symmetric_encrypt(KeyValuePairAPI.crypto_key, value)
 
         scope = getattr(kvp, 'scope', FULL_SYSTEM_SCOPE)
@@ -204,12 +206,25 @@ def to_model(cls, kvp):
                 scope, ALLOWED_SCOPES)
             )
 
+        # NOTE: For security reasons, pre_encrypted always implies secret=True. See comment
+        # above for explanation.
+        if pre_encrypted and not secret:
+            raise ValueError('pre_encrypted option can only be used in combination with secret '
+                             'option')
+
         model = cls.model(id=kvp_id, name=name, description=description, value=value,
                           secret=secret, scope=scope,
                           expire_timestamp=expire_timestamp)
 
         return model
 
+    @classmethod
+    def _verif_key_is_set_up(cls, name):
+        if not KeyValuePairAPI.crypto_key:
+            msg = ('Crypto key not found in %s. Unable to encrypt / decrypt value for key %s.' %
+                   (KeyValuePairAPI.crypto_key_path, name))
+            raise CryptoKeyNotSetupException(msg)
+
 
 class KeyValuePairSetAPI(KeyValuePairAPI):
     """
diff --git a/st2common/st2common/openapi.yaml b/st2common/st2common/openapi.yaml
index a4c63631bb..93df5986b5 100644
--- a/st2common/st2common/openapi.yaml
+++ b/st2common/st2common/openapi.yaml
@@ -4889,8 +4889,8 @@ definitions:
       secret:
         description: Encrypt value before saving the value.
         type: boolean
-      decrypt:
-        description: Value provided is encrypted with this instances crypto key and must be decrypted when received. This allows values to be transmitted in encrypted format.
+      pre_encrypted:
+        description: Value provided is encrypted with this instances crypto key and should be stored as-is. This is useful when migrating / loading keys from one StackStorm instance to another.
         type: boolean
       scope:
         description: "Scope the item is under. Example: 'user'."

From 677c4b47167ddd4661f81d7469598b986c15c678 Mon Sep 17 00:00:00 2001
From: Tomaz Muraus <tomaz@tomaz.me>
Date: Mon, 18 Mar 2019 18:11:28 +0100
Subject: [PATCH 04/12] Add changelog entry.

---
 CHANGELOG.rst | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index ec96874032..c606980acc 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -20,16 +20,16 @@ Added
 
 * Adding ``Cache-Control`` header to all API responses so clients will favor
   refresh from API instead of using cached version.
-* Added a new flag ``-d/--decrypt`` to ``st2 key set`` that allows users to pass in values
-  in encrypted format using the system's crypto keys. This flag informs the API that the
-  value transmitted is encrypted and it must be decrypted prior to working with it. Similarly
-  a keys file given to ``st2 key load`` can contain the property ``decrypt: true`` in any of
-  its keys and it will have the same effect. This results in keys files being able to contain
-  encrypted data that no longer needs to be decrypted prior to working with the CLI.
-  The corresponding ``decrypt`` option has been added to the API endpoint
-  ``PUT /api/v1/keys/{name}``. (new feature) #4545
-
-  Contributed by Nick Maludy (Encore Technologies
+* Add new ``--pre-encrypted`` flag to ``st2 key set`` CLI command that allows users to pass in
+  values which are already encrypted. Similarly, a keys file given to ``st2 key load`` CLI command
+  can contain the property ``pre_encrypted: true`` in any of its keys and it will have the same
+  effect.
+
+  This attribute signals the API that the value is already encrypted and should be used as-is. The
+  most common use case for this feature is migrating / restoring datastore values from one
+  StackStorm instance to another which uses the same crypto key.
+
+  Contributed by Nick Maludy (Encore Technologies)
 
 Changed
 ~~~~~~~

From 5228fa7567d6b9661697eba4b564e9e25bb8fb22 Mon Sep 17 00:00:00 2001
From: Tomaz Muraus <tomaz@tomaz.me>
Date: Mon, 18 Mar 2019 18:18:47 +0100
Subject: [PATCH 05/12] Update "st2 key load" CLI command so it works out of
 the box with values which are already encrypted.

Now "encrypted: True" and "secret: True" parameter in a value implies
value is pre-encrypted and is treated as such (aka used as-is).
---
 st2client/st2client/commands/keyvalue.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/st2client/st2client/commands/keyvalue.py b/st2client/st2client/commands/keyvalue.py
index 45f95ebe95..2d575963c2 100644
--- a/st2client/st2client/commands/keyvalue.py
+++ b/st2client/st2client/commands/keyvalue.py
@@ -319,6 +319,7 @@ def run(self, args, **kwargs):
             # parse optional KeyValuePair properties
             scope = item.get('scope', DEFAULT_CUD_SCOPE)
             user = item.get('user', None)
+            encrypted = item.get('encrypted', False)
             secret = item.get('secret', False)
             pre_encrypted = item.get('pre_encrypted', False)
             ttl = item.get('ttl', None)
@@ -340,8 +341,11 @@ def run(self, args, **kwargs):
             instance.name = name
             instance.value = value
             instance.scope = scope
+
             if user:
                 instance.user = user
+            if encrypted:
+                instance.encrypted = encrypted
             if secret:
                 instance.secret = secret
             if pre_encrypted:
@@ -349,6 +353,11 @@ def run(self, args, **kwargs):
             if ttl:
                 instance.ttl = ttl
 
+            # encrypted=True and secret=True implies that the value is already encrypted and should
+            # be used as such
+            if instance.encrypted and instance.secret:
+                instance.pre_encrypted = True
+
             # call the API to create/update the KeyValuePair
             self.manager.update(instance, **kwargs)
             instances.append(instance)

From 4f6fa27d04d052f6e54f473816f36e848aae02bb Mon Sep 17 00:00:00 2001
From: Tomaz Muraus <tomaz@tomaz.me>
Date: Mon, 18 Mar 2019 18:25:49 +0100
Subject: [PATCH 06/12] Update changelog.

---
 CHANGELOG.rst | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index c606980acc..c1b1c67b50 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -21,12 +21,17 @@ Added
 * Adding ``Cache-Control`` header to all API responses so clients will favor
   refresh from API instead of using cached version.
 * Add new ``--pre-encrypted`` flag to ``st2 key set`` CLI command that allows users to pass in
-  values which are already encrypted. Similarly, a keys file given to ``st2 key load`` CLI command
-  can contain the property ``pre_encrypted: true`` in any of its keys and it will have the same
-  effect.
+  values which are already encrypted.
 
-  This attribute signals the API that the value is already encrypted and should be used as-is. The
-  most common use case for this feature is migrating / restoring datastore values from one
+  This attribute signals the API that the value is already encrypted and should be used as-is.
+
+  ``st2 key load`` CLI command has also been updated so it knows how to work with values which are
+  already encrypted. This means that ``st2 key list -n 100 -j < data.json ; st2 key load
+  data.json`` will now also work out of the box for encrypted datastore values (value which have
+  ``encrypted: True`` and ``secret: True`` attributes will be treated as pre-encrypted values as
+  treated as such).
+
+  The most common use case for this feature is migrating / restoring datastore values from one
   StackStorm instance to another which uses the same crypto key.
 
   Contributed by Nick Maludy (Encore Technologies)

From ddd1514d81876aaffe9001a55fe7872b367c5720 Mon Sep 17 00:00:00 2001
From: Tomaz Muraus <tomaz@tomaz.me>
Date: Mon, 18 Mar 2019 18:35:38 +0100
Subject: [PATCH 07/12] Update test cases to test the new behavior, verify
 there is no accidental data / secrets leakage.

---
 st2api/tests/unit/controllers/v1/test_kvps.py | 62 +++++++++++++------
 st2client/st2client/commands/keyvalue.py      |  2 +-
 st2client/tests/unit/test_keyvalue.py         | 23 +++----
 3 files changed, 57 insertions(+), 30 deletions(-)

diff --git a/st2api/tests/unit/controllers/v1/test_kvps.py b/st2api/tests/unit/controllers/v1/test_kvps.py
index 14e0edc464..a74d10570c 100644
--- a/st2api/tests/unit/controllers/v1/test_kvps.py
+++ b/st2api/tests/unit/controllers/v1/test_kvps.py
@@ -79,8 +79,8 @@
     'pre_encrypted': True
 }
 
-ENCRYPTED_SECRET_KVP = {
-    'name': 'secret_key1',
+ENCRYPTED_KVP_SECRET_FALSE = {
+    'name': 'secret_key2',
     'value': ('3030303030298D848B45A24EDCD1A82FAB4E831E3FCE6E60956817A48A180E4C040801E'
               'B30170DACF79498F30520236A629912C3584847098D'),
     'secret': True,
@@ -485,32 +485,58 @@ def test_get_all_decrypt(self):
         self.__do_delete(kvp_id_1)
         self.__do_delete(kvp_id_2)
 
-    def test_put_pre_encrypted_balue(self):
+    def test_put_pre_encrypted_value(self):
+        # 1. pre_encrypted=True, secret=True
         put_resp = self.__do_put('secret_key1', ENCRYPTED_KVP)
         kvp_id = self.__get_kvp_id(put_resp)
 
-        # ensure that the data was decrypted prior to be stored in the datastore
-        get_resp = self.__do_get_one(kvp_id)
+        # Verify there is no secrets leakage
+        self.assertEqual(put_resp.status_code, 200)
+        self.assertEqual(put_resp.json['name'], 'secret_key1')
+        self.assertEqual(put_resp.json['scope'], 'st2kv.system')
+        self.assertEqual(put_resp.json['encrypted'], True)
+        self.assertEqual(put_resp.json['secret'], True)
+        self.assertEqual(put_resp.json['value'], ENCRYPTED_KVP['value'])
+        self.assertTrue(put_resp.json['value'] != 'S3cret!Value')
+        self.assertTrue(len(put_resp.json['value']) > len('S3cret!Value') * 2)
+
+        get_resp = self.__do_get_one(kvp_id + '?decrypt=True')
+        self.assertEqual(put_resp.json['name'], 'secret_key1')
+        self.assertEqual(put_resp.json['scope'], 'st2kv.system')
+        self.assertEqual(put_resp.json['encrypted'], True)
+        self.assertEqual(put_resp.json['secret'], True)
+        self.assertEqual(put_resp.json['value'], ENCRYPTED_KVP['value'])
+
+        # Verify data integrity post decryption
+        get_resp = self.__do_get_one(kvp_id + '?decrypt=True')
         self.assertFalse(get_resp.json['encrypted'])
         self.assertEqual(get_resp.json['value'], 'S3cret!Value')
         self.__do_delete(self.__get_kvp_id(put_resp))
 
-    def test_put_pre_encreypted_and_secret(self):
-        put_resp = self.__do_put('secret_key1', ENCRYPTED_SECRET_KVP)
+        # 2. pre_encrypted=True, secret=False
+        # pre_encrypted should always imply secret=True
+        put_resp = self.__do_put('secret_key2', ENCRYPTED_KVP_SECRET_FALSE)
         kvp_id = self.__get_kvp_id(put_resp)
 
-        # ensure the value was decrypted and re-encrypted, meaning a different salt
-        # and resulting in a uniquely encrypted value in the datastore
-        get_resp = self.__do_get_one(kvp_id)
-        self.assertTrue(get_resp.json['encrypted'])
-        crypto_val = get_resp.json['value']
-        self.assertNotEqual(ENCRYPTED_KVP['value'], crypto_val)
+        # Verify there is no secrets leakage
+        self.assertEqual(put_resp.status_code, 200)
+        self.assertEqual(put_resp.json['name'], 'secret_key2')
+        self.assertEqual(put_resp.json['scope'], 'st2kv.system')
+        self.assertEqual(put_resp.json['encrypted'], True)
+        self.assertEqual(put_resp.json['secret'], True)
+        self.assertEqual(put_resp.json['value'], ENCRYPTED_KVP['value'])
+        self.assertTrue(put_resp.json['value'] != 'S3cret!Value')
+        self.assertTrue(len(put_resp.json['value']) > len('S3cret!Value') * 2)
+
+        get_resp = self.__do_get_one(kvp_id + '?decrypt=True')
+        self.assertEqual(put_resp.json['name'], 'secret_key2')
+        self.assertEqual(put_resp.json['scope'], 'st2kv.system')
+        self.assertEqual(put_resp.json['encrypted'], True)
+        self.assertEqual(put_resp.json['secret'], True)
+        self.assertEqual(put_resp.json['value'], ENCRYPTED_KVP['value'])
 
-        # ensure that the data was decrypted prior to be stored in the datastore
-        get_resp = self.app.get('/v1/keys/secret_key1?decrypt=true')
-        self.assertEqual(get_resp.status_int, 200)
-        self.assertEqual(self.__get_kvp_id(get_resp), kvp_id)
-        self.assertTrue(get_resp.json['secret'])
+        # Verify data integrity post decryption
+        get_resp = self.__do_get_one(kvp_id + '?decrypt=True')
         self.assertFalse(get_resp.json['encrypted'])
         self.assertEqual(get_resp.json['value'], 'S3cret!Value')
         self.__do_delete(self.__get_kvp_id(put_resp))
diff --git a/st2client/st2client/commands/keyvalue.py b/st2client/st2client/commands/keyvalue.py
index 2d575963c2..5a1f4d3043 100644
--- a/st2client/st2client/commands/keyvalue.py
+++ b/st2client/st2client/commands/keyvalue.py
@@ -355,7 +355,7 @@ def run(self, args, **kwargs):
 
             # encrypted=True and secret=True implies that the value is already encrypted and should
             # be used as such
-            if instance.encrypted and instance.secret:
+            if encrypted and secret:
                 instance.pre_encrypted = True
 
             # call the API to create/update the KeyValuePair
diff --git a/st2client/tests/unit/test_keyvalue.py b/st2client/tests/unit/test_keyvalue.py
index bca5d2c02e..2ee8d70a1f 100644
--- a/st2client/tests/unit/test_keyvalue.py
+++ b/st2client/tests/unit/test_keyvalue.py
@@ -51,13 +51,13 @@
     'secret': True
 }
 
-KEYVALUE_DECRYPT = {
+KEYVALUE_PRE_ENCRYPTED = {
     'id': 'kv_name',
     'name': 'kv_name.',
     'value': 'AAABBBCCC1234',
     'scope': 'system',
-    'secret': True,
-    'pre_encrypted': True
+    'encrypted': True,
+    'secret': True
 }
 
 KEYVALUE_TTL = {
@@ -82,7 +82,7 @@
     'scope': 'system',
     'user': 'stanley',
     'secret': True,
-    'decrypt': True,
+    'encrypted': True,
     'ttl': 100
 }
 
@@ -122,11 +122,11 @@ class TestKeyValueSet(TestKeyValueBase):
 
     @mock.patch.object(
         requests, 'put',
-        mock.MagicMock(return_value=base.FakeResponse(json.dumps(KEYVALUE_DECRYPT), 200, 'OK')))
+        mock.MagicMock(return_value=base.FakeResponse(json.dumps(KEYVALUE_PRE_ENCRYPTED), 200,
+                                                      'OK')))
     def test_set_keyvalue(self):
-        """Test setting key/value pair with optional derypt field
+        """Test setting key/value pair with optional pre_encrypted field
         """
-        # long format
         args = ['key', 'set', '--pre-encrypted', 'kv_name', 'AAABBBCCC1234']
         retcode = self.shell.run(args)
         self.assertEqual(retcode, 0)
@@ -208,14 +208,15 @@ def test_load_keyvalue_secret(self):
 
     @mock.patch.object(
         requests, 'put',
-        mock.MagicMock(return_value=base.FakeResponse(json.dumps(KEYVALUE_DECRYPT), 200, 'OK')))
-    def test_load_keyvalue_decrypt(self):
-        """Test loading of key/value pair with the optional decrypt field
+        mock.MagicMock(return_value=base.FakeResponse(json.dumps(KEYVALUE_PRE_ENCRYPTED), 200,
+                                                      'OK')))
+    def test_load_keyvalue_already_encrypted(self):
+        """Test loading of key/value pair with the pre-encrypted value
         """
         fd, path = tempfile.mkstemp(suffix='.json')
         try:
             with open(path, 'a') as f:
-                f.write(json.dumps(KEYVALUE_DECRYPT, indent=4))
+                f.write(json.dumps(KEYVALUE_PRE_ENCRYPTED, indent=4))
 
             args = ['key', 'load', path]
             retcode = self.shell.run(args)

From 8e34d21cc799de0ca23154f03f43780761b60e72 Mon Sep 17 00:00:00 2001
From: Tomaz Muraus <tomaz@tomaz.me>
Date: Mon, 18 Mar 2019 19:31:31 +0100
Subject: [PATCH 08/12] Re-generate openapi file.

---
 st2common/st2common/openapi.yaml.j2 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/st2common/st2common/openapi.yaml.j2 b/st2common/st2common/openapi.yaml.j2
index a5ee7a492b..2146980603 100644
--- a/st2common/st2common/openapi.yaml.j2
+++ b/st2common/st2common/openapi.yaml.j2
@@ -4885,8 +4885,8 @@ definitions:
       secret:
         description: Encrypt value before saving the value.
         type: boolean
-      decrypt:
-        description: Value provided is encrypted with this instances crypto key and must be decrypted when received. This allows values to be transmitted in encrypted format.
+      pre_encrypted:
+        description: Value provided is encrypted with this instances crypto key and should be stored as-is. This is useful when migrating / loading keys from one StackStorm instance to another.
         type: boolean
       scope:
         description: "Scope the item is under. Example: 'user'."

From 970b3eb56e90d7920acda8a3bd8980c6cdd8e2ff Mon Sep 17 00:00:00 2001
From: Tomaz Muraus <tomaz@tomaz.me>
Date: Mon, 18 Mar 2019 20:54:17 +0100
Subject: [PATCH 09/12] Make --encrypt and --pre-encrypt arguments mutually
 exclusive.

---
 st2client/st2client/commands/keyvalue.py |  3 +++
 st2client/tests/unit/test_keyvalue.py    | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/st2client/st2client/commands/keyvalue.py b/st2client/st2client/commands/keyvalue.py
index 5a1f4d3043..fa87849155 100644
--- a/st2client/st2client/commands/keyvalue.py
+++ b/st2client/st2client/commands/keyvalue.py
@@ -180,6 +180,9 @@ def __init__(self, resource, *args, **kwargs):
 
     @resource.add_auth_token_to_kwargs_from_cli
     def run(self, args, **kwargs):
+        if args.secret and args.pre_encrypted:
+            raise ValueError('--encrypt and --pre-encrypted arguments are mutually exclusive')
+
         instance = KeyValuePair()
         instance.id = args.name  # TODO: refactor and get rid of id
         instance.name = args.name
diff --git a/st2client/tests/unit/test_keyvalue.py b/st2client/tests/unit/test_keyvalue.py
index 2ee8d70a1f..a4127516b4 100644
--- a/st2client/tests/unit/test_keyvalue.py
+++ b/st2client/tests/unit/test_keyvalue.py
@@ -131,6 +131,17 @@ def test_set_keyvalue(self):
         retcode = self.shell.run(args)
         self.assertEqual(retcode, 0)
 
+    def test_encrypt_and_pre_encrypted_are_mutually_exclusive(self):
+        args = ['key', 'set', '--encrypt', '--pre-encrypted', 'kv_name', 'AAABBBCCC1234']
+        retcode = self.shell.run(args)
+        self.assertEqual(retcode, 1)
+
+        self.stdout.seek(0)
+        stdout = self.stdout.read()
+
+        expected_msg = ('ERROR: --encrypt and --pre-encrypted arguments are mutually exclusive')
+        self.assertTrue(expected_msg in stdout)
+
 
 class TestKeyValueLoad(TestKeyValueBase):
 

From 88607e3de9187da97a946935eb680ca711f30363 Mon Sep 17 00:00:00 2001
From: Tomaz Muraus <tomaz@tomaz.me>
Date: Mon, 18 Mar 2019 21:02:19 +0100
Subject: [PATCH 10/12] Change attribute and CLI flag name from
 "--pre-encrypted" to "--encrypted".

---
 CHANGELOG.rst                                 |  4 ++--
 st2api/st2api/controllers/v1/keyvalue.py      | 12 ++++++------
 st2api/tests/unit/controllers/v1/test_kvps.py | 12 ++++++------
 st2client/st2client/commands/keyvalue.py      | 15 ++++++---------
 st2client/tests/unit/test_keyvalue.py         |  8 ++++----
 st2common/st2common/models/api/keyvalue.py    | 19 +++++++------------
 st2common/st2common/openapi.yaml              |  4 ++--
 st2common/st2common/openapi.yaml.j2           |  4 ++--
 8 files changed, 35 insertions(+), 43 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index c1b1c67b50..1b12d32684 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -20,8 +20,8 @@ Added
 
 * Adding ``Cache-Control`` header to all API responses so clients will favor
   refresh from API instead of using cached version.
-* Add new ``--pre-encrypted`` flag to ``st2 key set`` CLI command that allows users to pass in
-  values which are already encrypted.
+* Add new ``--encrypted`` flag to ``st2 key set`` CLI command that allows users to pass in values
+  which are already encrypted.
 
   This attribute signals the API that the value is already encrypted and should be used as-is.
 
diff --git a/st2api/st2api/controllers/v1/keyvalue.py b/st2api/st2api/controllers/v1/keyvalue.py
index 31bde5aef3..db40b7da86 100644
--- a/st2api/st2api/controllers/v1/keyvalue.py
+++ b/st2api/st2api/controllers/v1/keyvalue.py
@@ -274,10 +274,10 @@ def put(self, kvp, name, requester_user, scope=FULL_SYSTEM_SCOPE):
                                                              user=user,
                                                              require_rbac=True)
 
-        # Validate that pre_encrypted option can only be used by admins
-        pre_encrypted = getattr(kvp, 'pre_encrypted', False)
-        self._validate_pre_encrypted_query_parameter(pre_encrypted=pre_encrypted, scope=scope,
-                                                     requester_user=requester_user)
+        # Validate that encrypted option can only be used by admins
+        encrypted = getattr(kvp, 'encrypted', False)
+        self._validate_encrypted_query_parameter(encrypted=encrypted, scope=scope,
+                                                 requester_user=requester_user)
 
         key_ref = get_key_reference(scope=scope, name=name, user=user)
         lock_name = self._get_lock_name_for_key(name=key_ref, scope=scope)
@@ -414,9 +414,9 @@ def _validate_decrypt_query_parameter(self, decrypt, scope, requester_user):
             msg = 'Decrypt option requires administrator access'
             raise AccessDeniedError(message=msg, user_db=requester_user)
 
-    def _validate_pre_encrypted_query_parameter(self, pre_encrypted, scope, requester_user):
+    def _validate_encrypted_query_parameter(self, encrypted, scope, requester_user):
         is_admin = rbac_utils.user_is_admin(user_db=requester_user)
-        if pre_encrypted and not is_admin:
+        if encrypted and not is_admin:
             msg = 'Pre-encrypted option requires administrator access'
             raise AccessDeniedError(message=msg, user_db=requester_user)
 
diff --git a/st2api/tests/unit/controllers/v1/test_kvps.py b/st2api/tests/unit/controllers/v1/test_kvps.py
index a74d10570c..1547bda0d2 100644
--- a/st2api/tests/unit/controllers/v1/test_kvps.py
+++ b/st2api/tests/unit/controllers/v1/test_kvps.py
@@ -76,7 +76,7 @@
     'name': 'secret_key1',
     'value': ('3030303030298D848B45A24EDCD1A82FAB4E831E3FCE6E60956817A48A180E4C040801E'
               'B30170DACF79498F30520236A629912C3584847098D'),
-    'pre_encrypted': True
+    'encrypted': True
 }
 
 ENCRYPTED_KVP_SECRET_FALSE = {
@@ -84,7 +84,7 @@
     'value': ('3030303030298D848B45A24EDCD1A82FAB4E831E3FCE6E60956817A48A180E4C040801E'
               'B30170DACF79498F30520236A629912C3584847098D'),
     'secret': True,
-    'pre_encrypted': True
+    'encrypted': True
 }
 
 
@@ -485,8 +485,8 @@ def test_get_all_decrypt(self):
         self.__do_delete(kvp_id_1)
         self.__do_delete(kvp_id_2)
 
-    def test_put_pre_encrypted_value(self):
-        # 1. pre_encrypted=True, secret=True
+    def test_put_encrypted_value(self):
+        # 1. encrypted=True, secret=True
         put_resp = self.__do_put('secret_key1', ENCRYPTED_KVP)
         kvp_id = self.__get_kvp_id(put_resp)
 
@@ -513,8 +513,8 @@ def test_put_pre_encrypted_value(self):
         self.assertEqual(get_resp.json['value'], 'S3cret!Value')
         self.__do_delete(self.__get_kvp_id(put_resp))
 
-        # 2. pre_encrypted=True, secret=False
-        # pre_encrypted should always imply secret=True
+        # 2. encrypted=True, secret=False
+        # encrypted should always imply secret=True
         put_resp = self.__do_put('secret_key2', ENCRYPTED_KVP_SECRET_FALSE)
         kvp_id = self.__get_kvp_id(put_resp)
 
diff --git a/st2client/st2client/commands/keyvalue.py b/st2client/st2client/commands/keyvalue.py
index fa87849155..7e725cb909 100644
--- a/st2client/st2client/commands/keyvalue.py
+++ b/st2client/st2client/commands/keyvalue.py
@@ -168,7 +168,7 @@ def __init__(self, resource, *args, **kwargs):
         self.parser.add_argument('-e', '--encrypt', dest='secret',
                                  action='store_true',
                                  help='Encrypt value before saving.')
-        self.parser.add_argument('--pre-encrypted', dest='pre_encrypted',
+        self.parser.add_argument('--encrypted', dest='encrypted',
                                  action='store_true',
                                  help=('Value provided is already encrypted with the instance '
                                  'crypto key and should be stored as-is.'))
@@ -180,8 +180,8 @@ def __init__(self, resource, *args, **kwargs):
 
     @resource.add_auth_token_to_kwargs_from_cli
     def run(self, args, **kwargs):
-        if args.secret and args.pre_encrypted:
-            raise ValueError('--encrypt and --pre-encrypted arguments are mutually exclusive')
+        if args.secret and args.encrypted:
+            raise ValueError('--encrypt and --encrypted arguments are mutually exclusive')
 
         instance = KeyValuePair()
         instance.id = args.name  # TODO: refactor and get rid of id
@@ -193,8 +193,8 @@ def run(self, args, **kwargs):
         if args.secret:
             instance.secret = args.secret
 
-        if args.pre_encrypted:
-            instance.pre_encrypted = args.pre_encrypted
+        if args.encrypted:
+            instance.encrypted = args.encrypted
 
         if args.ttl:
             instance.ttl = args.ttl
@@ -324,7 +324,6 @@ def run(self, args, **kwargs):
             user = item.get('user', None)
             encrypted = item.get('encrypted', False)
             secret = item.get('secret', False)
-            pre_encrypted = item.get('pre_encrypted', False)
             ttl = item.get('ttl', None)
 
             # if the value is not a string, convert it to JSON
@@ -351,15 +350,13 @@ def run(self, args, **kwargs):
                 instance.encrypted = encrypted
             if secret:
                 instance.secret = secret
-            if pre_encrypted:
-                instance.pre_encrypted = pre_encrypted
             if ttl:
                 instance.ttl = ttl
 
             # encrypted=True and secret=True implies that the value is already encrypted and should
             # be used as such
             if encrypted and secret:
-                instance.pre_encrypted = True
+                instance.encrypted = True
 
             # call the API to create/update the KeyValuePair
             self.manager.update(instance, **kwargs)
diff --git a/st2client/tests/unit/test_keyvalue.py b/st2client/tests/unit/test_keyvalue.py
index a4127516b4..af3ffcb532 100644
--- a/st2client/tests/unit/test_keyvalue.py
+++ b/st2client/tests/unit/test_keyvalue.py
@@ -127,19 +127,19 @@ class TestKeyValueSet(TestKeyValueBase):
     def test_set_keyvalue(self):
         """Test setting key/value pair with optional pre_encrypted field
         """
-        args = ['key', 'set', '--pre-encrypted', 'kv_name', 'AAABBBCCC1234']
+        args = ['key', 'set', '--encrypted', 'kv_name', 'AAABBBCCC1234']
         retcode = self.shell.run(args)
         self.assertEqual(retcode, 0)
 
-    def test_encrypt_and_pre_encrypted_are_mutually_exclusive(self):
-        args = ['key', 'set', '--encrypt', '--pre-encrypted', 'kv_name', 'AAABBBCCC1234']
+    def test_encrypt_and_encrypted_flags_are_mutually_exclusive(self):
+        args = ['key', 'set', '--encrypt', '--encrypted', 'kv_name', 'AAABBBCCC1234']
         retcode = self.shell.run(args)
         self.assertEqual(retcode, 1)
 
         self.stdout.seek(0)
         stdout = self.stdout.read()
 
-        expected_msg = ('ERROR: --encrypt and --pre-encrypted arguments are mutually exclusive')
+        expected_msg = ('ERROR: --encrypt and --encrypted arguments are mutually exclusive')
         self.assertTrue(expected_msg in stdout)
 
 
diff --git a/st2common/st2common/models/api/keyvalue.py b/st2common/st2common/models/api/keyvalue.py
index 945fedbfac..a9c5e162b8 100644
--- a/st2common/st2common/models/api/keyvalue.py
+++ b/st2common/st2common/models/api/keyvalue.py
@@ -67,11 +67,6 @@ class KeyValuePairAPI(BaseAPI):
                 'required': False,
                 'default': False
             },
-            'pre_encrypted': {
-                'type': 'boolean',
-                'required': False,
-                'default': False
-            },
             'encrypted': {
                 'type': 'boolean',
                 'required': False,
@@ -171,15 +166,15 @@ def to_model(cls, kvp):
         else:
             expire_timestamp = None
 
-        pre_encrypted = getattr(kvp, 'pre_encrypted', False)
+        encrypted = getattr(kvp, 'encrypted', False)
         secret = getattr(kvp, 'secret', False)
 
         # If user transmitted the value in an pre-encrypted format, we perform the decryption here
         # to ensure data integrity. Besides that, we store data as-is.
-        # Keep in mind that pre_encrypted=True also always implies secret=True. If we didn't do
-        # that and supported pre_encrypted=True, secret=False, this would allow users to decrypt
+        # Keep in mind that encrypted=True also always implies secret=True. If we didn't do
+        # that and supported encrypted=True, secret=False, this would allow users to decrypt
         # any encrypted value.
-        if pre_encrypted:
+        if encrypted:
             secret = True
 
             cls._verif_key_is_set_up(name=name)
@@ -206,10 +201,10 @@ def to_model(cls, kvp):
                 scope, ALLOWED_SCOPES)
             )
 
-        # NOTE: For security reasons, pre_encrypted always implies secret=True. See comment
+        # NOTE: For security reasons, encrypted always implies secret=True. See comment
         # above for explanation.
-        if pre_encrypted and not secret:
-            raise ValueError('pre_encrypted option can only be used in combination with secret '
+        if encrypted and not secret:
+            raise ValueError('encrypted option can only be used in combination with secret '
                              'option')
 
         model = cls.model(id=kvp_id, name=name, description=description, value=value,
diff --git a/st2common/st2common/openapi.yaml b/st2common/st2common/openapi.yaml
index 93df5986b5..cdf786d55b 100644
--- a/st2common/st2common/openapi.yaml
+++ b/st2common/st2common/openapi.yaml
@@ -4889,8 +4889,8 @@ definitions:
       secret:
         description: Encrypt value before saving the value.
         type: boolean
-      pre_encrypted:
-        description: Value provided is encrypted with this instances crypto key and should be stored as-is. This is useful when migrating / loading keys from one StackStorm instance to another.
+      encrypted:
+        description: Value provided is already encrypted with this instances crypto key and should be stored as-is. This is useful when migrating / loading keys from one StackStorm instance to another.
         type: boolean
       scope:
         description: "Scope the item is under. Example: 'user'."
diff --git a/st2common/st2common/openapi.yaml.j2 b/st2common/st2common/openapi.yaml.j2
index 2146980603..a405c265f2 100644
--- a/st2common/st2common/openapi.yaml.j2
+++ b/st2common/st2common/openapi.yaml.j2
@@ -4885,8 +4885,8 @@ definitions:
       secret:
         description: Encrypt value before saving the value.
         type: boolean
-      pre_encrypted:
-        description: Value provided is encrypted with this instances crypto key and should be stored as-is. This is useful when migrating / loading keys from one StackStorm instance to another.
+      encrypted:
+        description: Value provided is already encrypted with this instances crypto key and should be stored as-is. This is useful when migrating / loading keys from one StackStorm instance to another.
         type: boolean
       scope:
         description: "Scope the item is under. Example: 'user'."

From b77e98a4c3ebe6e5dbeba92fef72be2bf2df62fc Mon Sep 17 00:00:00 2001
From: Tomaz Muraus <tomaz@tomaz.me>
Date: Mon, 18 Mar 2019 21:29:52 +0100
Subject: [PATCH 11/12] Use "parser.add_mutually_exclusive_group".

---
 st2client/st2client/commands/keyvalue.py | 22 ++++++++++++----------
 st2client/tests/unit/test_keyvalue.py    | 12 ++++++------
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/st2client/st2client/commands/keyvalue.py b/st2client/st2client/commands/keyvalue.py
index 7e725cb909..848218f13e 100644
--- a/st2client/st2client/commands/keyvalue.py
+++ b/st2client/st2client/commands/keyvalue.py
@@ -159,19 +159,24 @@ def __init__(self, resource, *args, **kwargs):
             *args, **kwargs
         )
 
+        # --encrypt and --encrypted options are mutually exclusive.
+        # --encrypt implies provided value is plain text and should be encrypted whereas
+        # --encrypted implies value is already encrypted and should be treated as-is.
+        encryption_group = self.parser.add_mutually_exclusive_group()
+        encryption_group.add_argument('-e', '--encrypt', dest='secret',
+                                      action='store_true',
+                                      help='Encrypt value before saving.')
+        encryption_group.add_argument('--encrypted', dest='encrypted',
+                                      action='store_true',
+                                      help=('Value provided is already encrypted with the '
+                                            'instance crypto key and should be stored as-is.'))
+
         self.parser.add_argument('name',
                                  metavar='name',
                                  help='Name of the key value pair.')
         self.parser.add_argument('value', help='Value paired with the key.')
         self.parser.add_argument('-l', '--ttl', dest='ttl', type=int, default=None,
                                  help='TTL (in seconds) for this value.')
-        self.parser.add_argument('-e', '--encrypt', dest='secret',
-                                 action='store_true',
-                                 help='Encrypt value before saving.')
-        self.parser.add_argument('--encrypted', dest='encrypted',
-                                 action='store_true',
-                                 help=('Value provided is already encrypted with the instance '
-                                 'crypto key and should be stored as-is.'))
         self.parser.add_argument('-s', '--scope', dest='scope', default=DEFAULT_CUD_SCOPE,
                                  help='Specify the scope under which you want ' +
                                       'to place the item.')
@@ -180,9 +185,6 @@ def __init__(self, resource, *args, **kwargs):
 
     @resource.add_auth_token_to_kwargs_from_cli
     def run(self, args, **kwargs):
-        if args.secret and args.encrypted:
-            raise ValueError('--encrypt and --encrypted arguments are mutually exclusive')
-
         instance = KeyValuePair()
         instance.id = args.name  # TODO: refactor and get rid of id
         instance.name = args.name
diff --git a/st2client/tests/unit/test_keyvalue.py b/st2client/tests/unit/test_keyvalue.py
index af3ffcb532..dc7caf19b1 100644
--- a/st2client/tests/unit/test_keyvalue.py
+++ b/st2client/tests/unit/test_keyvalue.py
@@ -133,14 +133,14 @@ def test_set_keyvalue(self):
 
     def test_encrypt_and_encrypted_flags_are_mutually_exclusive(self):
         args = ['key', 'set', '--encrypt', '--encrypted', 'kv_name', 'AAABBBCCC1234']
-        retcode = self.shell.run(args)
-        self.assertEqual(retcode, 1)
 
-        self.stdout.seek(0)
-        stdout = self.stdout.read()
+        self.assertRaisesRegexp(SystemExit, '2', self.shell.run, args)
+
+        self.stderr.seek(0)
+        stderr = self.stderr.read()
 
-        expected_msg = ('ERROR: --encrypt and --encrypted arguments are mutually exclusive')
-        self.assertTrue(expected_msg in stdout)
+        expected_msg = ('error: argument --encrypted: not allowed with argument -e/--encrypt')
+        self.assertTrue(expected_msg in stderr)
 
 
 class TestKeyValueLoad(TestKeyValueBase):

From 17427bf6aacc111dcf2a3c75a5dd356cad4bbcfa Mon Sep 17 00:00:00 2001
From: Tomaz Muraus <tomaz@tomaz.me>
Date: Mon, 18 Mar 2019 21:51:23 +0100
Subject: [PATCH 12/12] Add a test case for a failed integrity check.

---
 st2api/tests/unit/controllers/v1/test_kvps.py | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/st2api/tests/unit/controllers/v1/test_kvps.py b/st2api/tests/unit/controllers/v1/test_kvps.py
index 1547bda0d2..85f0294944 100644
--- a/st2api/tests/unit/controllers/v1/test_kvps.py
+++ b/st2api/tests/unit/controllers/v1/test_kvps.py
@@ -13,6 +13,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import copy
+
 from tests import FunctionalTest
 
 from st2common.models.db.auth import UserDB
@@ -541,6 +543,25 @@ def test_put_encrypted_value(self):
         self.assertEqual(get_resp.json['value'], 'S3cret!Value')
         self.__do_delete(self.__get_kvp_id(put_resp))
 
+    def test_put_encrypted_value_integrity_check_failed(self):
+        data = copy.deepcopy(ENCRYPTED_KVP)
+        data['value'] = 'corrupted'
+        put_resp = self.__do_put('secret_key1', data, expect_errors=True)
+        self.assertEqual(put_resp.status_code, 400)
+
+        expected_error = ('Failed to verify the integrity of the provided value for key '
+                          '"secret_key1".')
+        self.assertTrue(expected_error in put_resp.json['faultstring'])
+
+        data = copy.deepcopy(ENCRYPTED_KVP)
+        data['value'] = str(data['value'][:-2])
+        put_resp = self.__do_put('secret_key1', data, expect_errors=True)
+        self.assertEqual(put_resp.status_code, 400)
+
+        expected_error = ('Failed to verify the integrity of the provided value for key '
+                          '"secret_key1".')
+        self.assertTrue(expected_error in put_resp.json['faultstring'])
+
     def test_put_delete(self):
         put_resp = self.__do_put('key1', KVP)
         self.assertEqual(put_resp.status_int, 200)