From 19ad7d3983837eb5212bd53fbfc1766012e13096 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Tue, 19 Mar 2019 07:31:52 -0400 Subject: [PATCH] Inventory plugins data tweaks and finalization Disable use of azure_rm inventory plugin Disable use of ec2 inventory plugin due to compatibility issues that are unresolved Fix conflicts with ansible runner integration Add additional content enabled by Ansible core changes --- awx/main/models/inventory.py | 33 ++++---- awx/main/tasks.py | 13 ++- .../plugins/azure_rm/files/azure_rm.yml | 2 +- .../test_inventory_source_injectors.py | 20 +++-- awx/main/tests/unit/test_tasks.py | 81 +++++++++++-------- docs/inventory_plugins.md | 27 ++----- 6 files changed, 92 insertions(+), 84 deletions(-) diff --git a/awx/main/models/inventory.py b/awx/main/models/inventory.py index ae25d9f497..616dd9c5df 100644 --- a/awx/main/models/inventory.py +++ b/awx/main/models/inventory.py @@ -1880,7 +1880,7 @@ class PluginFileInjector(object): env.update(injector_env) return env - def _get_shared_env(self, inventory_update, private_data_dir, private_data_files, safe=False): + def _get_shared_env(self, inventory_update, private_data_dir, private_data_files): """By default, we will apply the standard managed_by_tower injectors for the script injection """ @@ -1894,31 +1894,28 @@ class PluginFileInjector(object): cred_kind = inventory_update.source.replace('ec2', 'aws') if cred_kind in dir(builtin_injectors): getattr(builtin_injectors, cred_kind)(credential, injected_env, private_data_dir) - if safe: - from awx.main.models.credential import build_safe_env - return build_safe_env(injected_env) elif self.base_injector == 'template': injected_env['INVENTORY_UPDATE_ID'] = str(inventory_update.pk) # so injector knows this is inventory safe_env = injected_env.copy() args = [] - safe_args = [] credential.credential_type.inject_credential( - credential, injected_env, safe_env, args, safe_args, private_data_dir + credential, injected_env, safe_env, args, private_data_dir ) - if safe: - return safe_env + # NOTE: safe_env is handled externally to injector class by build_safe_env static method + # that means that managed_by_tower injectors must only inject detectable env keys + # enforcement of this is accomplished by tests return injected_env - def get_plugin_env(self, inventory_update, private_data_dir, private_data_files, safe=False): - return self._get_shared_env(inventory_update, private_data_dir, private_data_files, safe) + def get_plugin_env(self, inventory_update, private_data_dir, private_data_files): + return self._get_shared_env(inventory_update, private_data_dir, private_data_files) - def get_script_env(self, inventory_update, private_data_dir, private_data_files, safe=False): - injected_env = self._get_shared_env(inventory_update, private_data_dir, private_data_files, safe) + def get_script_env(self, inventory_update, private_data_dir, private_data_files): + injected_env = self._get_shared_env(inventory_update, private_data_dir, private_data_files) # Put in env var reference to private ini data files, if relevant if self.ini_env_reference: credential = inventory_update.get_cloud_credential() - cred_data = private_data_files.get('credentials', '') + cred_data = private_data_files['credentials'] injected_env[self.ini_env_reference] = cred_data[credential] return injected_env @@ -1952,7 +1949,8 @@ class PluginFileInjector(object): class azure_rm(PluginFileInjector): plugin_name = 'azure_rm' - initial_version = '2.8' # Driven by unsafe group names issue, hostvars + # FIXME: https://github.com/ansible/ansible/issues/54065 need resolving to enable + # initial_version = '2.8' # Driven by unsafe group names issue, hostvars ini_env_reference = 'AZURE_INI_PATH' base_injector = 'managed' @@ -2074,7 +2072,8 @@ class azure_rm(PluginFileInjector): class ec2(PluginFileInjector): plugin_name = 'aws_ec2' - initial_version = '2.8' # Driven by unsafe group names issue, parent_group templating, hostvars + # blocked by https://github.com/ansible/ansible/issues/54059 + # initial_version = '2.8' # Driven by unsafe group names issue, parent_group templating, hostvars ini_env_reference = 'EC2_INI_PATH' base_injector = 'managed' @@ -2402,7 +2401,7 @@ class gce(PluginFileInjector): ret['zones'] = inventory_update.source_regions.split(',') return ret - def get_plugin_env(self, inventory_update, private_data_dir, private_data_files, safe=False): + def get_plugin_env(self, inventory_update, private_data_dir, private_data_files): # gce wants everything defined in inventory & cred files # this explicitly turns off injection of environment variables return {} @@ -2616,7 +2615,7 @@ class satellite6(PluginFileInjector): return self.dump_cp(cp, credential) - def get_plugin_env(self, inventory_update, private_data_dir, private_data_files, safe=False): + def get_plugin_env(self, inventory_update, private_data_dir, private_data_files): # this assumes that this is merged # https://github.com/ansible/ansible/pull/52693 credential = inventory_update.get_cloud_credential() diff --git a/awx/main/tasks.py b/awx/main/tasks.py index 6c625e81a1..7c63558cc4 100644 --- a/awx/main/tasks.py +++ b/awx/main/tasks.py @@ -2071,7 +2071,7 @@ class RunInventoryUpdate(BaseTask): getattr(settings, '%s_INSTANCE_ID_VAR' % src.upper()),]) # Add arguments for the source inventory script args.append('--source') - args.append(self.build_inventory(inventory_update, private_data_dir)) + args.append(self.psuedo_build_inventory(inventory_update, private_data_dir)) if src == 'custom': args.append("--custom") args.append('-v%d' % inventory_update.verbosity) @@ -2080,6 +2080,15 @@ class RunInventoryUpdate(BaseTask): return args def build_inventory(self, inventory_update, private_data_dir): + return None # what runner expects in order to not deal with inventory + + def psuedo_build_inventory(self, inventory_update, private_data_dir): + """Inventory imports are ran through a management command + we pass the inventory in args to that command, so this is not considered + to be "Ansible" inventory (by runner) even though it is + Eventually, we would like to cut out the management command, + and thus use this as the real inventory + """ src = inventory_update.source injector = None @@ -2133,7 +2142,7 @@ class RunInventoryUpdate(BaseTask): def build_credentials_list(self, inventory_update): # All credentials not used by inventory source injector - return [inventory_update.get_extra_credentials()] + return inventory_update.get_extra_credentials() def get_idle_timeout(self): return getattr(settings, 'INVENTORY_UPDATE_IDLE_TIMEOUT', None) diff --git a/awx/main/tests/data/inventory/plugins/azure_rm/files/azure_rm.yml b/awx/main/tests/data/inventory/plugins/azure_rm/files/azure_rm.yml index 1907b795e6..982fc5e6ba 100644 --- a/awx/main/tests/data/inventory/plugins/azure_rm/files/azure_rm.yml +++ b/awx/main/tests/data/inventory/plugins/azure_rm/files/azure_rm.yml @@ -16,7 +16,7 @@ keyed_groups: - key: location prefix: '' separator: '' -- key: tags.keys() if tags else [] +- key: tags.keys() | list if tags else [] prefix: '' separator: '' - key: security_group diff --git a/awx/main/tests/functional/test_inventory_source_injectors.py b/awx/main/tests/functional/test_inventory_source_injectors.py index 322204153a..825e475e39 100644 --- a/awx/main/tests/functional/test_inventory_source_injectors.py +++ b/awx/main/tests/functional/test_inventory_source_injectors.py @@ -3,6 +3,7 @@ from unittest import mock import os import json import re +from collections import namedtuple from awx.main.tasks import RunInventoryUpdate from awx.main.models import InventorySource, Credential, CredentialType, UnifiedJob @@ -151,6 +152,8 @@ def read_content(private_data_dir, raw_env, inventory_update): dir_contents = {} references = {} for filename in os.listdir(private_data_dir): + if filename in ('args', 'project'): + continue # Ansible runner abs_file_path = os.path.join(private_data_dir, filename) if abs_file_path in inverse_env: env_key = inverse_env[abs_file_path] @@ -248,17 +251,17 @@ def test_inventory_update_injected_content(this_kind, script_or_plugin, inventor if use_plugin and InventorySource.injectors[this_kind].plugin_name is None: pytest.skip('Use of inventory plugin is not enabled for this source') - def substitute_run(args, cwd, call_env, stdout_handle, **_kw): + def substitute_run(envvars=None, **_kw): """This method will replace run_pexpect instead of running, it will read the private data directory contents It will make assertions that the contents are correct If MAKE_INVENTORY_REFERENCE_FILES is set, it will produce reference files """ - private_data_dir = call_env.pop('AWX_PRIVATE_DATA_DIR') - assert call_env.pop('ANSIBLE_INVENTORY_ENABLED') == ('auto' if use_plugin else 'script') - assert call_env.pop('ANSIBLE_TRANSFORM_INVALID_GROUP_CHARS') == 'never' + private_data_dir = envvars.pop('AWX_PRIVATE_DATA_DIR') + assert envvars.pop('ANSIBLE_INVENTORY_ENABLED') == ('auto' if use_plugin else 'script') + assert envvars.pop('ANSIBLE_TRANSFORM_INVALID_GROUP_CHARS') == 'never' set_files = bool(os.getenv("MAKE_INVENTORY_REFERENCE_FILES", 'false').lower()[0] not in ['f', '0']) - env, content = read_content(private_data_dir, call_env, inventory_update) + env, content = read_content(private_data_dir, envvars, inventory_update) base_dir = os.path.join(DATA, script_or_plugin) if not os.path.exists(base_dir): os.mkdir(base_dir) @@ -290,7 +293,8 @@ def test_inventory_update_injected_content(this_kind, script_or_plugin, inventor except FileNotFoundError: ref_env = {} assert ref_env == env - return ('successful', 0) + Res = namedtuple('Result', ['status', 'rc']) + return Res('successful', 0) mock_licenser = mock.Mock(return_value=mock.Mock( validate=mock.Mock(return_value={'license_type': 'open'}) @@ -303,8 +307,8 @@ def test_inventory_update_injected_content(this_kind, script_or_plugin, inventor with mock.patch('awx.main.models.inventory.PluginFileInjector.should_use_plugin', return_value=use_plugin): # Also do not send websocket status updates with mock.patch.object(UnifiedJob, 'websocket_emit_status', mock.Mock()): - # The point of this test is that we replace run_pexpect with assertions - with mock.patch('awx.main.expect.run.run_pexpect', substitute_run): + # The point of this test is that we replace run with assertions + with mock.patch('awx.main.tasks.ansible_runner.interface.run', substitute_run): # mocking the licenser is necessary for the tower source with mock.patch('awx.main.models.inventory.get_licenser', mock_licenser): # so this sets up everything for a run and then yields control over to substitute_run diff --git a/awx/main/tests/unit/test_tasks.py b/awx/main/tests/unit/test_tasks.py index e92fa6a42e..ca4172cc67 100644 --- a/awx/main/tests/unit/test_tasks.py +++ b/awx/main/tests/unit/test_tasks.py @@ -32,6 +32,7 @@ from awx.main.models import ( CustomInventoryScript, build_safe_env ) +from awx.main.models.credential import ManagedCredentialType from awx.main import tasks from awx.main.utils import encrypt_field, encrypt_value @@ -163,12 +164,12 @@ def test_openstack_client_config_generation(mocker, source, expected, private_da inputs['verify_ssl'] = source credential = Credential(pk=1, credential_type=credential_type, inputs=inputs) - cred_method = mocker.Mock(return_value=credential) inventory_update = mocker.Mock(**{ 'source': 'openstack', 'source_vars_dict': {}, - 'get_cloud_credential': cred_method, - 'get_extra_credentials': lambda x: [] + 'get_cloud_credential': mocker.Mock(return_value=credential), + 'get_extra_credentials': lambda x: [], + 'ansible_virtualenv_path': '/venv/foo' }) cloud_config = update.build_private_data(inventory_update, private_data_dir) cloud_credential = yaml.load( @@ -205,12 +206,12 @@ def test_openstack_client_config_generation_with_private_source_vars(mocker, sou } credential = Credential(pk=1, credential_type=credential_type, inputs=inputs) - cred_method = mocker.Mock(return_value=credential) inventory_update = mocker.Mock(**{ 'source': 'openstack', 'source_vars_dict': {'private': source}, - 'get_cloud_credential': cred_method, - 'get_extra_credentials': lambda x: [] + 'get_cloud_credential': mocker.Mock(return_value=credential), + 'get_extra_credentials': lambda x: [], + 'ansible_virtualenv_path': '/venv/foo' }) cloud_config = update.build_private_data(inventory_update, private_data_dir) cloud_credential = yaml.load( @@ -1841,13 +1842,9 @@ class TestInventoryUpdateCredentials(TestJobExecution): private_data_files = task.build_private_data_files(inventory_update, private_data_dir) env = task.build_env(inventory_update, private_data_dir, False, private_data_files) - safe_env = {} - credentials = task.build_credentials_list(inventory_update) - for credential in credentials: - if credential: - credential.credential_type.inject_credential( - credential, env, safe_env, [], private_data_dir - ) + injector = InventorySource.injectors['ec2']('2.7') + env = injector.get_script_env(inventory_update, private_data_dir, private_data_files) + safe_env = build_safe_env(env) assert env['AWS_ACCESS_KEY_ID'] == 'bob' assert env['AWS_SECRET_ACCESS_KEY'] == 'secret' @@ -1921,13 +1918,10 @@ class TestInventoryUpdateCredentials(TestJobExecution): private_data_files = task.build_private_data_files(inventory_update, private_data_dir) env = task.build_env(inventory_update, private_data_dir, False, private_data_files) - safe_env = {} - credentials = task.build_credentials_list(inventory_update) - for credential in credentials: - if credential: - credential.credential_type.inject_credential( - credential, env, safe_env, [], private_data_dir - ) + + injector = InventorySource.injectors['azure_rm']('2.7') + env = injector.get_script_env(inventory_update, private_data_dir, private_data_files) + safe_env = build_safe_env(env) assert env['AZURE_CLIENT_ID'] == 'some-client' assert env['AZURE_SECRET'] == 'some-secret' @@ -1975,13 +1969,10 @@ class TestInventoryUpdateCredentials(TestJobExecution): private_data_files = task.build_private_data_files(inventory_update, private_data_dir) env = task.build_env(inventory_update, private_data_dir, False, private_data_files) - safe_env = {} - credentials = task.build_credentials_list(inventory_update) - for credential in credentials: - if credential: - credential.credential_type.inject_credential( - credential, env, safe_env, [], private_data_dir - ) + + injector = InventorySource.injectors['azure_rm']('2.7') + env = injector.get_script_env(inventory_update, private_data_dir, private_data_files) + safe_env = build_safe_env(env) assert env['AZURE_SUBSCRIPTION_ID'] == 'some-subscription' assert env['AZURE_AD_USER'] == 'bob' @@ -2048,7 +2039,7 @@ class TestInventoryUpdateCredentials(TestJobExecution): with mock.patch('awx.main.models.inventory.gce.initial_version', None): run('') - self.instance.source_regions = 'us-east-4' + inventory_update.source_regions = 'us-east-4' run('us-east-4') def test_openstack_source(self, inventory_update, private_data_dir, mocker): @@ -2187,13 +2178,9 @@ class TestInventoryUpdateCredentials(TestJobExecution): env = task.build_env(inventory_update, private_data_dir, False) - safe_env = {} - credentials = task.build_credentials_list(inventory_update) - for credential in credentials: - if credential: - credential.credential_type.inject_credential( - credential, env, safe_env, [], private_data_dir - ) + injector = InventorySource.injectors['tower']('2.7') + env = injector.get_script_env(inventory_update, private_data_dir, {}) + safe_env = build_safe_env(env) assert env['TOWER_HOST'] == 'https://tower.example.org' assert env['TOWER_USERNAME'] == 'bob' @@ -2315,3 +2302,27 @@ def test_aquire_lock_acquisition_fail_logged(fcntl_flock, logging_getLogger, os_ ProjectUpdate.acquire_lock(instance) os_close.assert_called_with(3) assert logger.err.called_with("I/O error({0}) while trying to aquire lock on file [{1}]: {2}".format(3, 'this_file_does_not_exist', 'dummy message')) + + +@pytest.mark.parametrize('injector_cls', [ + cls for cls in ManagedCredentialType.registry.values() if cls.injectors +]) +def test_managed_injector_redaction(injector_cls): + """See awx.main.models.inventory.PluginFileInjector._get_shared_env + The ordering within awx.main.tasks.BaseTask and contract with build_env + requires that all managed_by_tower injectors are safely redacted by the + static method build_safe_env without having to employ the safe namespace + as in inject_credential + + This test enforces that condition uniformly to prevent password leakages + """ + secrets = set() + for element in injector_cls.inputs.get('fields', []): + if element.get('secret', False): + secrets.add(element['id']) + env = {} + for env_name, template in injector_cls.injectors.get('env', {}).items(): + for secret_field_name in secrets: + if secret_field_name in template: + env[env_name] = 'very_secret_value' + assert 'very_secret_value' not in str(build_safe_env(env)) diff --git a/docs/inventory_plugins.md b/docs/inventory_plugins.md index 5980c214de..0b7bd6a626 100644 --- a/docs/inventory_plugins.md +++ b/docs/inventory_plugins.md @@ -1,7 +1,6 @@ # Transition to Ansible Inventory Plugins Inventory updates change from using scripts which are vendored as executable -python scripts in the AWX folder `awx/plugins/inventory` (taken originally from -Ansible folder `contrib/inventory`) to using dynamically-generated +python scripts to using dynamically-generated YAML files which conform to the specifications of the `auto` inventory plugin which are then parsed by their respective inventory plugin. @@ -86,13 +85,9 @@ construction used in compatibility mode to help get a sense of what new names replace old names. If you turn compatibility mode off or downgrade Ansible, you should -consider turning on `overwrite` and `overwrite_vars` to get rid of stale +turn on `overwrite` and `overwrite_vars` to get rid of stale variables (and potentially groups) no longer returned by the import. -In many cases, the host names will change. In all cases, accurate host -tracking will still be maintained via the host `instance_id`. -(after: https://github.com/ansible/awx/pull/3362) - Group names will be sanitized with compatibility mode turned off. That means that characters such as "-" will be replaced by underscores "\_". In some cases, this means that a large @@ -113,23 +108,13 @@ mode on. To maintain backward compatibility, the old names are added back where they have the same meaning as a variable returned by the plugin. New names are not removed. -Some hostvars will be lost, because of general deprecation needs. - - - ec2, see https://github.com/ansible/ansible/issues/52358 - - gce (see https://github.com/ansible/ansible/issues/51884) - - `gce_uuid` this came from libcloud and isn't a true GCP field - inventory plugins have moved away from libcloud - - The syntax of some hostvars, for some values, will change. - - - ec2 - - old: "ec2_block_devices": {"sda1": "vol-xxxxxx"} - - new: "ec2_block_devices": {"/dev/sda1": "vol-xxxxxx"} +A small number of hostvars will be lost because of general deprecation needs. #### Host names -Host names might change, but tracking host identity via `instance_id` -will still be reliable. +In many cases, the host names will change. In all cases, accurate host +tracking will still be maintained via the host `instance_id`. +(after: https://github.com/ansible/awx/pull/3362) ## How do I write my own Inventory File?