From 88b6a124bd35d5506452ad0140e298a81e1f0373 Mon Sep 17 00:00:00 2001 From: Alejandro Roiz Walss Date: Wed, 31 Jul 2024 11:50:47 -0600 Subject: [PATCH] Revert "Revert "Further Sanitize User Input (#425)"" This reverts commit 8e2493476ae311f5438f7bfd7d91b6b8fbab1d6b. --- VERSION | 2 +- confidant/routes/credentials.py | 71 +++++++++++++++---- .../unit/confidant/routes/credentials_test.py | 28 ++++++++ 3 files changed, 87 insertions(+), 14 deletions(-) diff --git a/VERSION b/VERSION index cc81d718..54c95af1 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.5.5 +6.5.7 diff --git a/confidant/routes/credentials.py b/confidant/routes/credentials.py index 13434add..4bbf89e3 100644 --- a/confidant/routes/credentials.py +++ b/confidant/routes/credentials.py @@ -594,9 +594,8 @@ def create_credential(): ''' with stats.timer('create_credential'): if not acl_module_check(resource_type='credential', action='create'): - msg = "{} does not have access to create credentials".format( - authnz.get_logged_in_user() - ) + msg = f"{authnz.get_logged_in_user()}" + msg += "does not have access to create credentials" error_msg = {'error': msg} return jsonify(error_msg), 403 @@ -621,7 +620,7 @@ def create_credential(): for cred in Credential.data_type_date_index.query( 'credential', filter_condition=Credential.name == data['name']): # Conflict, the name already exists - msg = 'Name already exists. See id: {0}'.format(cred.id) + msg = f'Name already exists. See id: {cred.id}' return jsonify({'error': msg, 'reference': cred.id}), 409 # Generate an initial stable ID to allow name changes id = str(uuid.uuid4()).replace('-', '') @@ -636,13 +635,20 @@ def create_credential(): credential_pairs = cipher.encrypt(credential_pairs) last_rotation_date = misc.utcnow() + metadata = data.get('metadata', {}) + for key, value in metadata.items(): + value = escape(value) + metadata[key] = value + + data['documentation'] = escape(data.get('documentation')) + sanitized_name = escape(data['name']) cred = Credential( - id='{0}-{1}'.format(id, revision), + id=f'{id}-{revision}', data_type='archive-credential', name=sanitized_name, credential_pairs=credential_pairs, - metadata=data.get('metadata'), + metadata=metadata, revision=revision, enabled=data.get('enabled'), data_key=data_key['ciphertext'], @@ -797,10 +803,8 @@ def update_credential(id): if not acl_module_check(resource_type='credential', action='update', resource_id=id): - msg = "{} does not have access to update credential {}".format( - authnz.get_logged_in_user(), - id - ) + msg = f"{authnz.get_logged_in_user()}" + msg += f"does not have access to update credential {id}" error_msg = {'error': msg, 'reference': id} return jsonify(error_msg), 403 @@ -816,6 +820,34 @@ def update_credential(id): if not isinstance(data.get('metadata', {}), dict): return jsonify({'error': 'metadata must be a dict'}), 400 + # We check for a name change and ensure it doesn't conflict with an + # existing credential and to ensure we don't escape the name if it + # hasn't changed + if data.get('name') != _cred.name: + data['name'] = escape(data.get('name')) + for cred in Credential.data_type_date_index.query( + 'credential', + filter_condition=Credential.name == data['name']): + # Conflict, the name already exists + msg = f'Name already exists. See id: {cred.id}' + return jsonify({'error': msg, 'reference': cred.id}), 409 + + # Escape metadata values by checking for new metadata keys and values + # to ensure we don't escape values that haven't changed + if data.get('metadata') != _cred.metadata: + new_metadata = { + key: value + for key, value in data.get('metadata', {}).items() + if key not in _cred.metadata or + value != _cred.metadata.get(key) + } + for key, value in new_metadata.items(): + value = escape(value) + data['metadata'][key] = value + + if data.get('documentation') != _cred.documentation: + data['documentation'] = escape(data.get('documentation')) + update = { 'name': data.get('name', _cred.name), 'last_rotation_date': _cred.last_rotation_date, @@ -874,6 +906,19 @@ def update_credential(id): # this is a new credential pair and update last_rotation_date if credential_pairs != _cred.decrypted_credential_pairs: update['last_rotation_date'] = misc.utcnow() + + # We escape credential pairs by checking for new credential + # pairs and values to ensure we don't escape values that haven't + # changed + new_credential_pairs = { + key: value + for key, value in credential_pairs.items() + if key not in _cred.decrypted_credential_pairs or + value != _cred.decrypted_credential_pairs.get(key) + } + for key, value in new_credential_pairs.items(): + value = escape(value) + credential_pairs[key] = value data_key = keymanager.create_datakey(encryption_context={'id': id}) cipher = CipherManager(data_key['plaintext'], version=2) update['credential_pairs'] = cipher.encrypt( @@ -883,7 +928,7 @@ def update_credential(id): # Try to save to the archive try: Credential( - id='{0}-{1}'.format(id, revision), + id=f'{id}-{revision}', name=update['name'], data_type='archive-credential', credential_pairs=update['credential_pairs'], @@ -925,8 +970,8 @@ def update_credential(id): if services: service_names = [x.id for x in services] - msg = 'Updated credential "{0}" ({1}); Revision {2}' - msg = msg.format(cred.name, cred.id, cred.revision) + msg = f'Updated credential "{cred.name}"' + msg += f'({cred.id}); Revision {cred.revision}' graphite.send_event(service_names, msg) webhook.send_event('credential_update', service_names, [cred.id]) permissions = { diff --git a/tests/unit/confidant/routes/credentials_test.py b/tests/unit/confidant/routes/credentials_test.py index 80f97e80..ed881726 100644 --- a/tests/unit/confidant/routes/credentials_test.py +++ b/tests/unit/confidant/routes/credentials_test.py @@ -501,6 +501,11 @@ def test_update_credential(mocker: MockerFixture, credential: Credential): 'confidant.routes.credentials.Credential.get', return_value=credential, ) + mocker.patch( + ('confidant.routes.credentials.Credential' + '.data_type_date_index.query'), + return_value=[], + ) mocker.patch( ('confidant.routes.credentials.credentialmanager' '.get_latest_credential_revision'), @@ -567,7 +572,30 @@ def test_update_credential(mocker: MockerFixture, credential: Credential): assert ret.status_code == 400 assert 'Credential Pairs cannot be empty.' == json_data['error'] + # Credential name already exists (ie: query returns a value) + mocker.patch( + 'confidant.routes.credentials.Credential.data_type_date_index.query', + return_value=[credential], + ) + ret = app.test_client().put( + '/v1/credentials/123', + headers={"Content-Type": 'application/json'}, + data=json.dumps({ + 'name': 'me', + 'documentation': 'doc', + 'credential_pairs': {'key': 'value'}, + }), + ) + json_data = json.loads(ret.data) + assert ret.status_code == 409 + assert 'Name already exists' in json_data['error'] + # All good + mocker.patch( + ('confidant.routes.credentials.Credential' + '.data_type_date_index.query'), + return_value=[], + ) mocker.patch( ('confidant.routes.credentials.servicemanager' '.pair_key_conflicts_for_services'),