From 827ad0fa7585c8a04a77280f764b6bec7657daf2 Mon Sep 17 00:00:00 2001 From: chris meyers Date: Fri, 8 Mar 2019 13:48:50 -0500 Subject: [PATCH] remove safe_args and add status_handler * safe_args no longer makes sense. We have moved extra_vars to a file and thus do not pass sensitive content on the cmdline --- awx/main/models/credential/__init__.py | 8 +- awx/main/tasks.py | 191 ++++++++---------- .../tests/unit/models/test_survey_models.py | 23 --- 3 files changed, 89 insertions(+), 133 deletions(-) diff --git a/awx/main/models/credential/__init__.py b/awx/main/models/credential/__init__.py index 12bfe6efe8..e67d1492d7 100644 --- a/awx/main/models/credential/__init__.py +++ b/awx/main/models/credential/__init__.py @@ -606,7 +606,7 @@ class CredentialType(CommonModelNameNotUnique): match = cls.objects.filter(**requirements)[:1].get() return match - def inject_credential(self, credential, env, safe_env, args, safe_args, private_data_dir): + def inject_credential(self, credential, env, safe_env, args, private_data_dir): """ Inject credential data into the environment variables and arguments passed to `ansible-playbook` @@ -627,9 +627,6 @@ class CredentialType(CommonModelNameNotUnique): additional arguments based on custom `extra_vars` injectors defined on this CredentialType. - :param safe_args: a list of arguments stored in the database for - the job run (`UnifiedJob.job_args`); secret - values should be stripped :param private_data_dir: a temporary directory to store files generated by `file` injectors (like config files or key files) @@ -650,7 +647,7 @@ class CredentialType(CommonModelNameNotUnique): # maintain a normal namespace for building the ansible-playbook arguments (env and args) namespace = {'tower': tower_namespace} - # maintain a sanitized namespace for building the DB-stored arguments (safe_env and safe_args) + # maintain a sanitized namespace for building the DB-stored arguments (safe_env) safe_namespace = {'tower': tower_namespace} # build a normal namespace with secret values decrypted (for @@ -724,7 +721,6 @@ class CredentialType(CommonModelNameNotUnique): path = build_extra_vars_file(extra_vars, private_data_dir) if extra_vars: args.extend(['-e', '@%s' % path]) - safe_args.extend(['-e', '@%s' % path]) class ManagedCredentialType(SimpleNamespace): diff --git a/awx/main/tasks.py b/awx/main/tasks.py index ee24feef44..95566ea6bf 100644 --- a/awx/main/tasks.py +++ b/awx/main/tasks.py @@ -801,7 +801,7 @@ class BaseTask(object): '': '', } - def build_extra_vars_file(self, instance, private_data_dir, passwords, display=False): + def build_extra_vars_file(self, instance, private_data_dir, passwords): ''' Build ansible yaml file filled with extra vars to be passed via -e@file.yml ''' @@ -906,9 +906,6 @@ class BaseTask(object): os.chmod(path, stat.S_IRUSR) return path - def build_safe_args(self, instance, private_data_dir, passwords): - return self.build_args(instance, private_data_dir, passwords) - def build_cwd(self, instance, private_data_dir): raise NotImplementedError @@ -957,10 +954,10 @@ class BaseTask(object): ''' Run the job/task and capture its output. ''' - instance = self.update_model(pk, status='running', - start_args='') # blank field to remove encrypted passwords + self.instance = self.update_model(pk, status='running', + start_args='') # blank field to remove encrypted passwords - instance.websocket_emit_status("running") + self.instance.websocket_emit_status("running") status, rc, tb = 'error', None, '' output_replacements = [] extra_update_fields = {} @@ -970,75 +967,69 @@ class BaseTask(object): private_data_dir = None try: - isolated = instance.is_isolated() - self.pre_run_hook(instance) - if instance.cancel_flag: - instance = self.update_model(instance.pk, status='canceled') - if instance.status != 'running': + isolated = self.instance.is_isolated() + self.pre_run_hook(self.instance) + if self.instance.cancel_flag: + self.instance = self.update_model(self.instance.pk, status='canceled') + if self.instance.status != 'running': # Stop the task chain and prevent starting the job if it has # already been canceled. - instance = self.update_model(pk) - status = instance.status - raise RuntimeError('not starting %s task' % instance.status) + self.instance = self.update_model(pk) + status = self.instance.status + raise RuntimeError('not starting %s task' % self.instance.status) if not os.path.exists(settings.AWX_PROOT_BASE_PATH): raise RuntimeError('AWX_PROOT_BASE_PATH=%s does not exist' % settings.AWX_PROOT_BASE_PATH) # store a record of the venv used at runtime - if hasattr(instance, 'custom_virtualenv'): - self.update_model(pk, custom_virtualenv=getattr(instance, 'ansible_virtualenv_path', settings.ANSIBLE_VENV_PATH)) - private_data_dir = self.build_private_data_dir(instance) + if hasattr(self.instance, 'custom_virtualenv'): + self.update_model(pk, custom_virtualenv=getattr(self.instance, 'ansible_virtualenv_path', settings.ANSIBLE_VENV_PATH)) + private_data_dir = self.build_private_data_dir(self.instance) # Fetch "cached" fact data from prior runs and put on the disk # where ansible expects to find it - if getattr(instance, 'use_fact_cache', False): - instance.start_job_fact_cache( - os.path.join(private_data_dir, 'artifacts', str(instance.id), 'fact_cache'), + if getattr(self.instance, 'use_fact_cache', False): + self.instance.start_job_fact_cache( + os.path.join(private_data_dir, 'artifacts', str(self.instance.id), 'fact_cache'), fact_modification_times, ) # May have to serialize the value - private_data_files = self.build_private_data_files(instance, private_data_dir) - passwords = self.build_passwords(instance, kwargs) + private_data_files = self.build_private_data_files(self.instance, private_data_dir) + passwords = self.build_passwords(self.instance, kwargs) proot_custom_virtualenv = None - if getattr(instance, 'ansible_virtualenv_path', settings.ANSIBLE_VENV_PATH) != settings.ANSIBLE_VENV_PATH: - proot_custom_virtualenv = instance.ansible_virtualenv_path - self.build_extra_vars_file(instance, private_data_dir, passwords) - args = self.build_args(instance, private_data_dir, passwords) - safe_args = self.build_safe_args(instance, private_data_dir, passwords) + if getattr(self.instance, 'ansible_virtualenv_path', settings.ANSIBLE_VENV_PATH) != settings.ANSIBLE_VENV_PATH: + proot_custom_virtualenv = self.instance.ansible_virtualenv_path + self.build_extra_vars_file(self.instance, private_data_dir, passwords) + args = self.build_args(self.instance, private_data_dir, passwords) # TODO: output_replacements hurts my head right now - #output_replacements = self.build_output_replacements(instance, **kwargs) + #output_replacements = self.build_output_replacements(self.instance, **kwargs) output_replacements = [] - cwd = self.build_cwd(instance, private_data_dir) - env = self.build_env(instance, private_data_dir, isolated, + cwd = self.build_cwd(self.instance, private_data_dir) + env = self.build_env(self.instance, private_data_dir, isolated, private_data_files=private_data_files) safe_env = build_safe_env(env) # handle custom injectors specified on the CredentialType credentials = [] - if isinstance(instance, Job): - credentials = instance.credentials.all() - elif isinstance(instance, InventoryUpdate): + if isinstance(self.instance, Job): + credentials = self.instance.credentials.all() + elif isinstance(self.instance, InventoryUpdate): # TODO: allow multiple custom creds for inv updates - credentials = [instance.get_cloud_credential()] - elif isinstance(instance, Project): + credentials = [self.instance.get_cloud_credential()] + elif isinstance(self.instance, Project): # once (or if) project updates # move from a .credential -> .credentials model, we can # lose this block - credentials = [instance.credential] + credentials = [self.instance.credential] for credential in credentials: if credential: credential.credential_type.inject_credential( - credential, env, safe_env, args, safe_args, private_data_dir + credential, env, safe_env, args, private_data_dir ) self.write_args_file(private_data_dir, args) - # If we're executing on an isolated host, don't bother adding the - # key to the agent in this environment - instance = self.update_model(pk, job_args=json.dumps(safe_args), - job_cwd=cwd, job_env=safe_env) - expect_passwords = {} password_prompts = self.get_password_prompts(passwords) for k, v in password_prompts.items(): @@ -1047,12 +1038,12 @@ class BaseTask(object): extra_update_fields=extra_update_fields, proot_cmd=getattr(settings, 'AWX_PROOT_CMD', 'bwrap'), ) - instance = self.update_model(instance.pk, output_replacements=output_replacements) + self.instance = self.update_model(self.instance.pk, output_replacements=output_replacements) - def event_handler(self, instance, event_data): + def event_handler(self, event_data): should_write_event = False dispatcher = CallbackQueueDispatcher() - event_data.setdefault(self.event_data_key, instance.id) + event_data.setdefault(self.event_data_key, self.instance.id) dispatcher.dispatch(event_data) self.event_ct += 1 @@ -1060,48 +1051,55 @@ class BaseTask(object): Handle artifacts ''' if event_data.get('event_data', {}).get('artifact_data', {}): - instance.artifacts = event_data['event_data']['artifact_data'] - instance.save(update_fields=['artifacts']) + self.instance.artifacts = event_data['event_data']['artifact_data'] + self.instance.save(update_fields=['artifacts']) return should_write_event - def cancel_callback(instance): - instance = self.update_model(pk) - if instance.cancel_flag or instance.status == 'canceled': - cancel_wait = (now() - instance.modified).seconds if instance.modified else 0 + def cancel_callback(self): + self.instance = self.update_model(self.instance.pk) + if self.instance.cancel_flag or self.instance.status == 'canceled': + cancel_wait = (now() - self.instance.modified).seconds if self.instance.modified else 0 if cancel_wait > 5: - logger.warn('Request to cancel {} took {} seconds to complete.'.format(instance.log_format, cancel_wait)) + logger.warn('Request to cancel {} took {} seconds to complete.'.format(self.instance.log_format, cancel_wait)) return True return False - def finished_callback(self, instance, runner_obj): + def finished_callback(self, runner_obj): dispatcher = CallbackQueueDispatcher() event_data = { 'event': 'EOF', 'final_counter': self.event_ct, } - event_data.setdefault(self.event_data_key, instance.id) + event_data.setdefault(self.event_data_key, self.instance.id) dispatcher.dispatch(event_data) + def status_handler(self, status_data, runner_config): + if status_data['status'] == 'starting': + self.instance = self.update_model(pk, job_args=json.dumps(runner_config.command), + job_cwd=runner_config.cwd, job_env=runner_config.env) + + params = { - 'ident': instance.id, + 'ident': self.instance.id, 'private_data_dir': private_data_dir, 'project_dir': cwd, - 'playbook': self.build_playbook_path_relative_to_cwd(instance, private_data_dir), - 'inventory': self.build_inventory(instance, private_data_dir), + 'playbook': self.build_playbook_path_relative_to_cwd(self.instance, private_data_dir), + 'inventory': self.build_inventory(self.instance, private_data_dir), 'passwords': expect_passwords, 'envvars': env, - 'event_handler': functools.partial(event_handler, self, instance), - 'cancel_callback': functools.partial(cancel_callback, instance), - 'finished_callback': functools.partial(finished_callback, self, instance), + 'event_handler': functools.partial(event_handler, self), + 'cancel_callback': functools.partial(cancel_callback, self), + 'finished_callback': functools.partial(finished_callback, self), + 'status_handler': functools.partial(status_handler, self), 'settings': { 'idle_timeout': self.get_idle_timeout() or "", - 'job_timeout': self.get_instance_timeout(instance), + 'job_timeout': self.get_instance_timeout(self.instance), 'pexpect_timeout': getattr(settings, 'PEXPECT_TIMEOUT', 5), } } - if self.should_use_proot(instance): + if self.should_use_proot(self.instance): process_isolation_params = { 'process_isolation': True, 'process_isolation_path': settings.AWX_PROOT_BASE_PATH, @@ -1126,11 +1124,11 @@ class BaseTask(object): process_isolation_params['process_isolation_ro_paths'].append(proot_custom_virtualenv) params = {**params, **process_isolation_params} - if isinstance(instance, AdHocCommand): - params['module'] = self.build_module_name(instance) - params['module_args'] = self.build_module_args(instance) + if isinstance(self.instance, AdHocCommand): + params['module'] = self.build_module_name(self.instance) + params['module_args'] = self.build_module_args(self.instance) - if getattr(instance, 'use_fact_cache', False): + if getattr(self.instance, 'use_fact_cache', False): # Enable Ansible fact cache. params['fact_cache_type'] = 'jsonfile' else: @@ -1144,7 +1142,7 @@ class BaseTask(object): if not params[v]: del params[v] - if instance.is_isolated() is True: + if self.instance.is_isolated() is True: playbook = params['playbook'] shutil.move( params.pop('inventory'), @@ -1153,7 +1151,7 @@ class BaseTask(object): copy_tree(cwd, os.path.join(private_data_dir, 'project')) ansible_runner.utils.dump_artifacts(params) manager_instance = isolated_manager.IsolatedManager(env, **_kw) - status, rc = manager_instance.run(instance, + status, rc = manager_instance.run(self.instance, private_data_dir, playbook, event_data_key=self.event_data_key) @@ -1163,40 +1161,40 @@ class BaseTask(object): rc = res.rc if status == 'timeout': - instance.job_explanation = "Job terminated due to timeout" + self.instance.job_explanation = "Job terminated due to timeout" status = 'failed' - extra_update_fields['job_explanation'] = instance.job_explanation + extra_update_fields['job_explanation'] = self.instance.job_explanation except Exception: # run_pexpect does not throw exceptions for cancel or timeout # this could catch programming or file system errors tb = traceback.format_exc() - logger.exception('%s Exception occurred while running task', instance.log_format) + logger.exception('%s Exception occurred while running task', self.instance.log_format) finally: - logger.info('%s finished running, producing %s events.', instance.log_format, self.event_ct) + logger.info('%s finished running, producing %s events.', self.instance.log_format, self.event_ct) try: - self.post_run_hook(instance, status) + self.post_run_hook(self.instance, status) except Exception: - logger.exception('{} Post run hook errored.'.format(instance.log_format)) + logger.exception('{} Post run hook errored.'.format(self.instance.log_format)) - instance = self.update_model(pk) - instance = self.update_model(pk, status=status, result_traceback=tb, - output_replacements=output_replacements, - emitted_events=self.event_ct, - **extra_update_fields) + self.instance = self.update_model(pk) + self.instance = self.update_model(pk, status=status, result_traceback=tb, + output_replacements=output_replacements, + emitted_events=self.event_ct, + **extra_update_fields) try: - self.final_run_hook(instance, status, private_data_dir, fact_modification_times) + self.final_run_hook(self.instance, status, private_data_dir, fact_modification_times) except Exception: - logger.exception('{} Final run hook errored.'.format(instance.log_format)) + logger.exception('{} Final run hook errored.'.format(self.instance.log_format)) - instance.websocket_emit_status(status) + self.instance.websocket_emit_status(status) if status != 'successful': if status == 'canceled': - raise AwxTaskError.TaskCancel(instance, rc) + raise AwxTaskError.TaskCancel(self.instance, rc) else: - raise AwxTaskError.TaskError(instance, rc) + raise AwxTaskError.TaskError(self.instance, rc) @task() @@ -1357,7 +1355,7 @@ class RunJob(BaseTask): return env - def build_args(self, job, private_data_dir, passwords, display=False): + def build_args(self, job, private_data_dir, passwords): ''' Build command line argument list for running ansible-playbook, optionally using ssh-agent for public/private key authentication. @@ -1421,9 +1419,6 @@ class RunJob(BaseTask): return args - def build_safe_args(self, job, private_data_dir, passwords): - return self.build_args(job, private_data_dir, passwords, display=True) - def build_cwd(self, job, private_data_dir): cwd = job.project.get_project_path() if not cwd: @@ -1435,16 +1430,12 @@ class RunJob(BaseTask): def build_playbook_path_relative_to_cwd(self, job, private_data_dir): return os.path.join(job.playbook) - def build_extra_vars_file(self, job, private_data_dir, passwords, display=False): + def build_extra_vars_file(self, job, private_data_dir, passwords): # Define special extra_vars for AWX, combine with job.extra_vars. extra_vars = job.awx_meta_vars() if job.extra_vars_dict: - # TODO: Is display needed here? We are building a file that isn't visible - if display and job.job_template: - extra_vars.update(json.loads(job.display_extra_vars())) - else: - extra_vars.update(json.loads(job.decrypted_extra_vars())) + extra_vars.update(json.loads(job.decrypted_extra_vars())) # By default, all extra vars disallow Jinja2 template usage for # security reasons; top level key-values defined in JT.extra_vars, however, @@ -1688,14 +1679,6 @@ class RunProjectUpdate(BaseTask): }) self._write_extra_vars_file(private_data_dir, extra_vars) - def build_safe_args(self, project_update, private_data_dir, passwords): - pwdict = dict(passwords.items()) - for pw_name, pw_val in list(pwdict.items()): - if pw_name in ('', 'yes', 'no', 'scm_username'): - continue - pwdict[pw_name] = HIDDEN_PASSWORD - return self.build_args(project_update, private_data_dir, passwords) - def build_cwd(self, project_update, private_data_dir): return self.get_path_to('..', 'playbooks') @@ -2432,7 +2415,7 @@ class RunAdHocCommand(BaseTask): return args - def build_extra_vars_file(self, ad_hoc_command, private_data_dir, passwords={}, display=False): + def build_extra_vars_file(self, ad_hoc_command, private_data_dir, passwords={}): extra_vars = ad_hoc_command.awx_meta_vars() if ad_hoc_command.extra_vars_dict: diff --git a/awx/main/tests/unit/models/test_survey_models.py b/awx/main/tests/unit/models/test_survey_models.py index c6751e9b27..09ec14d0b0 100644 --- a/awx/main/tests/unit/models/test_survey_models.py +++ b/awx/main/tests/unit/models/test_survey_models.py @@ -132,29 +132,6 @@ def test_survey_passwords_not_in_extra_vars(): } -def test_job_safe_args_redacted_passwords(job): - """Verify that safe_args hides passwords in the job extra_vars""" - kwargs = {'ansible_version': '2.1', 'private_data_dir': tempfile.mkdtemp()} - run_job = RunJob() - safe_args = run_job.build_safe_args(job, **kwargs) - ev_index = safe_args.index('-e') + 1 - extra_var_file = open(safe_args[ev_index][1:], 'r') - extra_vars = yaml.load(extra_var_file, SafeLoader) - extra_var_file.close() - assert extra_vars['secret_key'] == '$encrypted$' - - -def test_job_args_unredacted_passwords(job, tmpdir_factory): - kwargs = {'ansible_version': '2.1', 'private_data_dir': tempfile.mkdtemp()} - run_job = RunJob() - args = run_job.build_args(job, **kwargs) - ev_index = args.index('-e') + 1 - extra_var_file = open(args[ev_index][1:], 'r') - extra_vars = yaml.load(extra_var_file, SafeLoader) - extra_var_file.close() - assert extra_vars['secret_key'] == 'my_password' - - def test_launch_config_has_unprompted_vars(survey_spec_factory): jt = JobTemplate( survey_enabled = True,