Merge pull request #4547 from nmaludy/feature/kvp-cli-api-decrypt

Added ability to pass in encrypted key/values into the CLI and API

CHANGELOG.rst

@@ -34,6 +34,21 @@ Added
 
 * Adding ``Cache-Control`` header to all API responses so clients will favor
   refresh from API instead of using cached version.
+* 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.
+
+  ``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)
 
 Changed
 ~~~~~~~

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 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)
         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_encrypted_query_parameter(self, encrypted, scope, requester_user):
+        is_admin = rbac_utils.user_is_admin(user_db=requester_user)
+        if 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)

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
@@ -70,6 +72,23 @@
     'secret': True
 }
 
+# value = S3cret!Value
+# encrypted with st2tests/conf/st2_kvstore_tests.crypto.key.json
+ENCRYPTED_KVP = {
+    'name': 'secret_key1',
+    'value': ('3030303030298D848B45A24EDCD1A82FAB4E831E3FCE6E60956817A48A180E4C040801E'
+              'B30170DACF79498F30520236A629912C3584847098D'),
+    'encrypted': True
+}
+
+ENCRYPTED_KVP_SECRET_FALSE = {
+    'name': 'secret_key2',
+    'value': ('3030303030298D848B45A24EDCD1A82FAB4E831E3FCE6E60956817A48A180E4C040801E'
+              'B30170DACF79498F30520236A629912C3584847098D'),
+    'secret': True,
+    'encrypted': True
+}
+
 
 class KeyValuePairControllerTestCase(FunctionalTest):
 
@@ -468,6 +487,81 @@ def test_get_all_decrypt(self):
         self.__do_delete(kvp_id_1)
         self.__do_delete(kvp_id_2)
 
+    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)
+
+        # 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))
+
+        # 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)
+
+        # 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'])
+
+        # 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_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)

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".')
@@ -159,15 +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('-s', '--scope', dest='scope', default=DEFAULT_CUD_SCOPE,
                                  help='Specify the scope under which you want ' +
                                       'to place the item.')
@@ -186,6 +195,9 @@ def run(self, args, **kwargs):
         if args.secret:
             instance.secret = args.secret
 
+        if args.encrypted:
+            instance.encrypted = args.encrypted
+
         if args.ttl:
             instance.ttl = args.ttl
 
@@ -312,6 +324,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)
             ttl = item.get('ttl', None)
 
@@ -332,13 +345,21 @@ 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 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.encrypted = True
+
             # call the API to create/update the KeyValuePair
             self.manager.update(instance, **kwargs)
             instances.append(instance)

st2client/tests/unit/test_keyvalue.py

@@ -51,6 +51,15 @@
     'secret': True
 }
 
+KEYVALUE_PRE_ENCRYPTED = {
+    'id': 'kv_name',
+    'name': 'kv_name.',
+    'value': 'AAABBBCCC1234',
+    'scope': 'system',
+    'encrypted': True,
+    'secret': 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,
+    'encrypted': True,
     'ttl': 100
 }
 
@@ -108,6 +118,31 @@ def tearDown(self):
         super(TestKeyValueBase, self).tearDown()
 
 
+class TestKeyValueSet(TestKeyValueBase):
+
+    @mock.patch.object(
+        requests, 'put',
+        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 pre_encrypted field
+        """
+        args = ['key', 'set', '--encrypted', 'kv_name', 'AAABBBCCC1234']
+        retcode = self.shell.run(args)
+        self.assertEqual(retcode, 0)
+
+    def test_encrypt_and_encrypted_flags_are_mutually_exclusive(self):
+        args = ['key', 'set', '--encrypt', '--encrypted', 'kv_name', 'AAABBBCCC1234']
+
+        self.assertRaisesRegexp(SystemExit, '2', self.shell.run, args)
+
+        self.stderr.seek(0)
+        stderr = self.stderr.read()
+
+        expected_msg = ('error: argument --encrypted: not allowed with argument -e/--encrypt')
+        self.assertTrue(expected_msg in stderr)
+
+
 class TestKeyValueLoad(TestKeyValueBase):
 
     @mock.patch.object(
@@ -182,6 +217,25 @@ 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_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_PRE_ENCRYPTED, 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')))

st2common/st2common/models/api/keyvalue.py

@@ -157,6 +157,7 @@ def to_model(cls, kvp):
         name = getattr(kvp, 'name', None)
         description = getattr(kvp, 'description', None)
         value = kvp.value
+        original_value = value
         secret = False
 
         if getattr(kvp, 'ttl', None):
@@ -165,13 +166,32 @@ def to_model(cls, kvp):
         else:
             expire_timestamp = None
 
+        encrypted = getattr(kvp, 'encrypted', False)
         secret = getattr(kvp, 'secret', False)
 
-        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)
+        # 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 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 encrypted:
+            secret = True
+
+            cls._verif_key_is_set_up(name=name)
+
+            try:
+                symmetric_decrypt(KeyValuePairAPI.crypto_key, value)
+            except Exception:
+                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)
+
+            # 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)
@@ -181,12 +201,25 @@ def to_model(cls, kvp):
                 scope, ALLOWED_SCOPES)
             )
 
+        # NOTE: For security reasons, encrypted always implies secret=True. See comment
+        # above for explanation.
+        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,
                           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):
     """