diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 5782c58096..3b2af6cc07 100644 --- a/CHANGELOG.rst +++ b/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 ~~~~~~~ diff --git a/st2api/st2api/controllers/v1/keyvalue.py b/st2api/st2api/controllers/v1/keyvalue.py index 8e0a2ec15e..db40b7da86 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 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) diff --git a/st2api/tests/unit/controllers/v1/test_kvps.py b/st2api/tests/unit/controllers/v1/test_kvps.py index 2edcefca39..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 @@ -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) diff --git a/st2client/st2client/commands/keyvalue.py b/st2client/st2client/commands/keyvalue.py index d6ab3803e8..848218f13e 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".') @@ -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) diff --git a/st2client/tests/unit/test_keyvalue.py b/st2client/tests/unit/test_keyvalue.py index 4ebcba1d8b..dc7caf19b1 100644 --- a/st2client/tests/unit/test_keyvalue.py +++ b/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'))) diff --git a/st2common/st2common/models/api/keyvalue.py b/st2common/st2common/models/api/keyvalue.py index 45ad1738ae..a9c5e162b8 100644 --- a/st2common/st2common/models/api/keyvalue.py +++ b/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): """ diff --git a/st2common/st2common/openapi.yaml b/st2common/st2common/openapi.yaml index aa92aace65..cffff2e05f 100644 --- a/st2common/st2common/openapi.yaml +++ b/st2common/st2common/openapi.yaml @@ -4932,6 +4932,9 @@ definitions: secret: description: Encrypt value before saving the value. type: boolean + 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'." type: string diff --git a/st2common/st2common/openapi.yaml.j2 b/st2common/st2common/openapi.yaml.j2 index b3b4592fc1..05f20aaa46 100644 --- a/st2common/st2common/openapi.yaml.j2 +++ b/st2common/st2common/openapi.yaml.j2 @@ -4928,6 +4928,9 @@ definitions: secret: description: Encrypt value before saving the value. type: boolean + 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'." type: string