From c3842b6bf98f2b51bf9bf7855c1944fb9dd00528 Mon Sep 17 00:00:00 2001 From: adamscmRH Date: Wed, 21 Mar 2018 12:07:18 -0400 Subject: [PATCH 01/18] upgrade python-saml for CVE fix --- requirements/requirements.in | 2 +- requirements/requirements.txt | 2 +- tools/docker-compose/Dockerfile | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/requirements/requirements.in b/requirements/requirements.in index e531173499..cf00960cbe 100644 --- a/requirements/requirements.in +++ b/requirements/requirements.in @@ -40,7 +40,7 @@ pyparsing==2.2.0 python-logstash==0.4.6 python-memcached==1.58 python-radius==1.0 -python-saml==2.2.1 +python-saml==2.2.4 python-social-auth==0.2.21 pyvmomi==6.5 redbaron==0.6.3 diff --git a/requirements/requirements.txt b/requirements/requirements.txt index fbd0b0b6b6..7bc1abb2e3 100644 --- a/requirements/requirements.txt +++ b/requirements/requirements.txt @@ -151,7 +151,7 @@ python-novaclient==9.0.1 # via python-openstackclient, shade python-openid==2.2.5 # via python-social-auth python-openstackclient==3.11.0 # via python-ironicclient python-radius==1.0 -python-saml==2.2.1 +python-saml==2.2.4 python-social-auth==0.2.21 pytz==2017.2 # via babel, celery, irc, oslo.serialization, oslo.utils, tempora, twilio pyvmomi==6.5 diff --git a/tools/docker-compose/Dockerfile b/tools/docker-compose/Dockerfile index 712c7cd607..79b133ac73 100644 --- a/tools/docker-compose/Dockerfile +++ b/tools/docker-compose/Dockerfile @@ -30,7 +30,7 @@ RUN ln -s /awx_devel/tools/docker-compose/bootstrap_development.sh /bootstrap_de RUN openssl req -nodes -newkey rsa:2048 -keyout /etc/nginx/nginx.key -out /etc/nginx/nginx.csr -subj "/C=US/ST=North Carolina/L=Durham/O=Ansible/OU=AWX Development/CN=awx.localhost" RUN openssl x509 -req -days 365 -in /etc/nginx/nginx.csr -signkey /etc/nginx/nginx.key -out /etc/nginx/nginx.crt WORKDIR /tmp -RUN SWIG_FEATURES="-cpperraswarn -includeall -D__`uname -m`__ -I/usr/include/openssl" VENV_BASE="/venv" make requirements_dev +RUN CFLAGS="-DXMLSEC_NO_SIZE_T" SWIG_FEATURES="-cpperraswarn -includeall -D__`uname -m`__ -I/usr/include/openssl" VENV_BASE="/venv" make requirements_dev RUN localedef -c -i en_US -f UTF-8 en_US.UTF-8 ENV LANG en_US.UTF-8 ENV LANGUAGE en_US:en From ad37f71af42bd0eb6947755509bb136d41af317f Mon Sep 17 00:00:00 2001 From: adamscmRH Date: Thu, 22 Mar 2018 11:26:17 -0400 Subject: [PATCH 02/18] fix_python_saml24_update --- requirements/requirements.in | 2 +- requirements/requirements.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements/requirements.in b/requirements/requirements.in index cf00960cbe..3a916cc9c7 100644 --- a/requirements/requirements.in +++ b/requirements/requirements.in @@ -40,7 +40,7 @@ pyparsing==2.2.0 python-logstash==0.4.6 python-memcached==1.58 python-radius==1.0 -python-saml==2.2.4 +python-saml==2.4.0 python-social-auth==0.2.21 pyvmomi==6.5 redbaron==0.6.3 diff --git a/requirements/requirements.txt b/requirements/requirements.txt index 7bc1abb2e3..9970dafab0 100644 --- a/requirements/requirements.txt +++ b/requirements/requirements.txt @@ -151,7 +151,7 @@ python-novaclient==9.0.1 # via python-openstackclient, shade python-openid==2.2.5 # via python-social-auth python-openstackclient==3.11.0 # via python-ironicclient python-radius==1.0 -python-saml==2.2.4 +python-saml==2.4.0 python-social-auth==0.2.21 pytz==2017.2 # via babel, celery, irc, oslo.serialization, oslo.utils, tempora, twilio pyvmomi==6.5 From 61aafe15d6f58133323392efce2fc0fb27de1df4 Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Mon, 5 Mar 2018 14:13:57 -0500 Subject: [PATCH 03/18] fix busted shippable builds --- Makefile | 1 - requirements/requirements_dev_uninstall.txt | 1 - tools/docker-compose/Dockerfile | 1 - 3 files changed, 3 deletions(-) delete mode 100644 requirements/requirements_dev_uninstall.txt diff --git a/Makefile b/Makefile index 0ba149c123..dec65947ab 100644 --- a/Makefile +++ b/Makefile @@ -182,7 +182,6 @@ requirements_awx: virtualenv_awx requirements_awx_dev: $(VENV_BASE)/awx/bin/pip install -r requirements/requirements_dev.txt - $(VENV_BASE)/awx/bin/pip uninstall --yes -r requirements/requirements_dev_uninstall.txt requirements: requirements_ansible requirements_awx diff --git a/requirements/requirements_dev_uninstall.txt b/requirements/requirements_dev_uninstall.txt deleted file mode 100644 index 963eac530b..0000000000 --- a/requirements/requirements_dev_uninstall.txt +++ /dev/null @@ -1 +0,0 @@ -certifi diff --git a/tools/docker-compose/Dockerfile b/tools/docker-compose/Dockerfile index 79b133ac73..d99d57bfde 100644 --- a/tools/docker-compose/Dockerfile +++ b/tools/docker-compose/Dockerfile @@ -9,7 +9,6 @@ requirements/requirements_ansible_git.txt \ requirements/requirements_dev.txt \ requirements/requirements_ansible_uninstall.txt \ requirements/requirements_tower_uninstall.txt \ -requirements/requirements_dev_uninstall.txt \ /tmp/requirements/ RUN yum -y update && yum -y install curl epel-release RUN curl --silent --location https://rpm.nodesource.com/setup_6.x | bash - From 35e38760aa507dfea5795ed6fdd182b2e484301d Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Thu, 22 Mar 2018 16:11:46 -0400 Subject: [PATCH 04/18] properly sanitize module arguments with no_log (like uri:password) this will _not_ sanitize playbooks that have secrets hard-coded *in* the playbook - for that, people will need to use Vault or a variable/lookup see: https://github.com/ansible/tower/issues/1101 see: https://github.com/ansible/awx/issues/1633 --- awx/lib/awx_display_callback/module.py | 21 ++++++++++++++++++--- awx/lib/tests/test_display_callback.py | 25 +++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/awx/lib/awx_display_callback/module.py b/awx/lib/awx_display_callback/module.py index 368063d0d1..8aef42301f 100644 --- a/awx/lib/awx_display_callback/module.py +++ b/awx/lib/awx_display_callback/module.py @@ -36,6 +36,8 @@ from .events import event_context from .minimal import CallbackModule as MinimalCallbackModule CENSORED = "the output has been hidden due to the fact that 'no_log: true' was specified for this result" # noqa +OMIT = "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER" + class BaseCallbackModule(CallbackBase): @@ -68,6 +70,7 @@ class BaseCallbackModule(CallbackBase): @contextlib.contextmanager def capture_event_data(self, event, **event_data): + censored_task_args = [] event_data.setdefault('uuid', str(uuid.uuid4())) if event not in self.EVENTS_WITHOUT_TASK: @@ -76,6 +79,14 @@ class BaseCallbackModule(CallbackBase): task = None if event_data.get('res'): + invocation = event_data['res'].get('invocation') + if invocation: + module_args = invocation.get('module_args') + sensitive_module_args = set([ + k for k, v in module_args.items() + if v == OMIT + ]) + censored_task_args = sensitive_module_args.intersection(task.args.keys()) if event_data['res'].get('_ansible_no_log', False): event_data['res'] = {'censored': CENSORED} if event_data['res'].get('results', []): @@ -88,7 +99,7 @@ class BaseCallbackModule(CallbackBase): try: event_context.add_local(event=event, **event_data) if task: - self.set_task(task, local=True) + self.set_task(task, local=True, censored_task_args=censored_task_args) event_context.dump_begin(sys.stdout) yield finally: @@ -120,7 +131,8 @@ class BaseCallbackModule(CallbackBase): event_context.remove_global(play=None, play_uuid=None, play_pattern=None) self.clear_task() - def set_task(self, task, local=False): + def set_task(self, task, local=False, censored_task_args=None): + censored_task_args = censored_task_args or [] # FIXME: Task is "global" unless using free strategy! task_ctx = dict( task=(task.name or task.action), @@ -134,7 +146,10 @@ class BaseCallbackModule(CallbackBase): if task.no_log: task_ctx['task_args'] = "the output has been hidden due to the fact that 'no_log: true' was specified for this result" else: - task_args = ', '.join(('%s=%s' % a for a in task.args.items())) + task_args = ', '.join(( + '%s=%s' % (k, CENSORED if k in censored_task_args else v) + for k, v in task.args.items() + )) task_ctx['task_args'] = task_args if getattr(task, '_role', None): task_role = task._role._role_name diff --git a/awx/lib/tests/test_display_callback.py b/awx/lib/tests/test_display_callback.py index e87f3ec306..46a7681694 100644 --- a/awx/lib/tests/test_display_callback.py +++ b/awx/lib/tests/test_display_callback.py @@ -279,3 +279,28 @@ def test_callback_plugin_saves_custom_stats(executor, cache, playbook): assert json.load(f) == {'foo': 'bar'} finally: shutil.rmtree(os.path.join(private_data_dir)) + + +@pytest.mark.parametrize('playbook', [ +{'no_log_module_with_var.yml': ''' +- name: ensure that module-level secrets are redacted + connection: local + hosts: all + vars: + - pw: SENSITIVE + tasks: + - uri: + url: https://example.org + user: john-jacob-jingleheimer-schmidt + password: "{{ pw }}" +'''}, # noqa +]) +def test_module_level_no_log(executor, cache, playbook): + # https://github.com/ansible/tower/issues/1101 + # It's possible for `no_log=True` to be defined at the _module_ level, + # e.g., for the URI module password parameter + # This test ensures that we properly redact those + executor.run() + assert len(cache) + assert 'john-jacob-jingleheimer-schmidt' in json.dumps(cache.items()) + assert 'SENSITIVE' not in json.dumps(cache.items()) From f6e507ad1289267d0951ae190ca457d19b36d14a Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Fri, 23 Mar 2018 13:36:01 -0400 Subject: [PATCH 05/18] add API setting for UI live updates include context data update help text --- awx/settings/defaults.py | 4 ++++ awx/ui/conf.py | 10 ++++++++++ awx/ui/templates/ui/index.html | 1 + awx/ui/views.py | 2 ++ 4 files changed, 17 insertions(+) diff --git a/awx/settings/defaults.py b/awx/settings/defaults.py index b381548668..89d1727188 100644 --- a/awx/settings/defaults.py +++ b/awx/settings/defaults.py @@ -168,6 +168,10 @@ STDOUT_MAX_BYTES_DISPLAY = 1048576 # on how many events to display before truncating/hiding MAX_UI_JOB_EVENTS = 4000 +# Returned in index.html, tells the UI if it should make requests +# to update job data in response to status changes websocket events +UI_LIVE_UPDATES_ENABLED = True + # The maximum size of the ansible callback event's res data structure # beyond this limit and the value will be removed MAX_EVENT_RES_DATA = 700000 diff --git a/awx/ui/conf.py b/awx/ui/conf.py index 0d626c28f0..df88890faf 100644 --- a/awx/ui/conf.py +++ b/awx/ui/conf.py @@ -63,3 +63,13 @@ register( category=_('UI'), category_slug='ui', ) + +register( + 'UI_LIVE_UPDATES_ENABLED', + field_class=fields.BooleanField, + label=_('Enable Live Updates in the UI'), + help_text=_('If disabled, the page will not refresh when events are received. ' + 'Reloading the page will be required to get the latest details.'), + category=_('UI'), + category_slug='ui', +) diff --git a/awx/ui/templates/ui/index.html b/awx/ui/templates/ui/index.html index 38bbad9de3..ce8af6b086 100644 --- a/awx/ui/templates/ui/index.html +++ b/awx/ui/templates/ui/index.html @@ -12,6 +12,7 @@ diff --git a/awx/ui/views.py b/awx/ui/views.py index 0a47154615..994da49732 100644 --- a/awx/ui/views.py +++ b/awx/ui/views.py @@ -2,6 +2,7 @@ # All Rights Reserved. from django.views.generic.base import TemplateView, RedirectView +from django.conf import settings class IndexView(TemplateView): @@ -9,6 +10,7 @@ class IndexView(TemplateView): def get_context_data(self, **kwargs): context = super(IndexView, self).get_context_data(**kwargs) + context['UI_LIVE_UPDATES_ENABLED'] = settings.UI_LIVE_UPDATES_ENABLED # Add any additional context info here. return context From 8643972064e0a6e525f6a26b39d5a8027d8f1254 Mon Sep 17 00:00:00 2001 From: Jared Tabor Date: Thu, 22 Mar 2018 16:10:07 -0700 Subject: [PATCH 06/18] Fixes issue with sockets and XHR requests for backgrounded tabs adjusts toggling based on API setting and doesn't toggle for job stdout page --- .../src/shared/socket/socket.service.js | 30 ++++++++++++++++++- awx/ui/templates/ui/index.html | 2 +- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/awx/ui/client/src/shared/socket/socket.service.js b/awx/ui/client/src/shared/socket/socket.service.js index f520fbcbe9..7c51298a72 100644 --- a/awx/ui/client/src/shared/socket/socket.service.js +++ b/awx/ui/client/src/shared/socket/socket.service.js @@ -8,7 +8,8 @@ export default ['$rootScope', '$location', '$log','$state', '$q', 'i18n', function ($rootScope, $location, $log, $state, $q, i18n) { var needsResubscribing = false, - socketPromise = $q.defer(); + socketPromise = $q.defer(), + needsRefreshAfterBlur; return { init: function() { var self = this, @@ -24,6 +25,26 @@ export default } url = `${protocol}://${host}/websocket/`; + // only toggle background tabbed sockets if the + // UI_LIVE_UPDATES_ENABLED flag is true in the settings file + if(window.liveUpdates){ + document.addEventListener('visibilitychange', function() { + $log.debug(document.visibilityState); + if(document.visibilityState === 'hidden'){ + window.liveUpdates = false; + } + else if(document.visibilityState === 'visible'){ + window.liveUpdates = true; + if(needsRefreshAfterBlur){ + $state.go('.', null, {reload: true}); + needsRefreshAfterBlur = false; + } + + } + }); + } + + if (!$rootScope.sessionTimer || ($rootScope.sessionTimer && !$rootScope.sessionTimer.isExpired())) { $log.debug('Socket connecting to: ' + url); @@ -75,6 +96,13 @@ export default $log.debug('Received From Server: ' + e.data); var data = JSON.parse(e.data), str = ""; + + if(!window.liveUpdates && data.group_name !== "control" && $state.current.name !== "jobResult"){ + $log.debug('Message from server dropped: ' + e.data); + needsRefreshAfterBlur = true; + return; + } + if(data.group_name==="jobs" && !('status' in data)){ // we know that this must have been a // summary complete message b/c status is missing. diff --git a/awx/ui/templates/ui/index.html b/awx/ui/templates/ui/index.html index ce8af6b086..3dae008dc7 100644 --- a/awx/ui/templates/ui/index.html +++ b/awx/ui/templates/ui/index.html @@ -12,7 +12,7 @@ From df60876bf3db8ecd60bb9c149e309728fe43eb88 Mon Sep 17 00:00:00 2001 From: Jared Tabor Date: Fri, 23 Mar 2018 16:17:24 -0700 Subject: [PATCH 07/18] Adds a debug function to turn on $log.debug --- awx/ui/client/src/app.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/awx/ui/client/src/app.js b/awx/ui/client/src/app.js index 529e5d7abe..fc79e6a015 100644 --- a/awx/ui/client/src/app.js +++ b/awx/ui/client/src/app.js @@ -146,7 +146,11 @@ var awApp = angular.module('awApp', [ .constant('AngularScheduler.showUTCField', true) .constant('$timezones.definitions.location', urlPrefix + 'lib/angular-tz-extensions/tz/data') .config(['$logProvider', function($logProvider) { - $logProvider.debugEnabled($ENV['ng-debug'] || false); + window.debug = function(){ + $logProvider.debugEnabled(!$logProvider.debugEnabled()); + return $logProvider.debugEnabled(); + }; + window.debug(false); }]) .config(['ngToastProvider', function(ngToastProvider) { ngToastProvider.configure({ From c1cc92afa06e14ee9c399eb3b48b0ad6d481e00d Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Wed, 28 Mar 2018 17:02:32 -0400 Subject: [PATCH 08/18] properly filter disabled hosts on smart inventory composition see: #1053 related: https://github.com/ansible/tower/pull/1155 --- awx/api/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/awx/api/views.py b/awx/api/views.py index f5fdcf6eb2..11a364f587 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2408,7 +2408,7 @@ class InventoryScriptView(RetrieveAPIView): return Response({}) else: all_group = data.setdefault('all', dict()) - smart_hosts_qs = obj.hosts.all() + smart_hosts_qs = obj.hosts.filter(**hosts_q).all() smart_hosts = list(smart_hosts_qs.values_list('name', flat=True)) all_group['hosts'] = smart_hosts else: From c4635fa683abe881a4ebbf259259463e6b81d2c9 Mon Sep 17 00:00:00 2001 From: Wayne Witzel III Date: Mon, 2 Apr 2018 16:03:00 -0400 Subject: [PATCH 09/18] Merge pull request #1199 from wwitzel3/fix-1189 Fixes RBAC issue, ensures can admin of sub_obj when needed --- awx/main/access.py | 8 ++++++++ awx/main/tests/functional/test_rbac_role.py | 12 ++++++++++++ 2 files changed, 20 insertions(+) diff --git a/awx/main/access.py b/awx/main/access.py index 1de1e8fb3f..ddecce39e2 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -2396,6 +2396,14 @@ class RoleAccess(BaseAccess): if not check_user_access(self.user, sub_obj_resource.__class__, 'read', sub_obj_resource): return False + # Being a user in the member_role or admin_role of an organization grants + # administrators of that Organization the ability to edit that user. To prevent + # unwanted escalations lets ensure that the Organization administartor has the abilty + # to admin the user being added to the role. + if isinstance(obj.content_object, Organization) and obj.role_field in ['member_role', 'admin_role']: + if not UserAccess(self.user).can_admin(sub_obj, None): + return False + if isinstance(obj.content_object, ResourceMixin) and \ self.user in obj.content_object.admin_role: return True diff --git a/awx/main/tests/functional/test_rbac_role.py b/awx/main/tests/functional/test_rbac_role.py index 16ad46f8db..62b8469abf 100644 --- a/awx/main/tests/functional/test_rbac_role.py +++ b/awx/main/tests/functional/test_rbac_role.py @@ -32,3 +32,15 @@ def test_role_access_attach(rando, inventory): inventory.read_role.members.add(rando) access = RoleAccess(rando) assert not access.can_attach(inventory.admin_role, rando, 'members', None) + + +@pytest.mark.django_db +def test_org_user_role_attach(user, organization): + admin = user('admin') + nonmember = user('nonmember') + + organization.admin_role.members.add(admin) + + access = RoleAccess(admin) + assert not access.can_attach(organization.member_role, nonmember, 'members', None) + assert not access.can_attach(organization.admin_role, nonmember, 'members', None) From 1195385492ccbf0a8ad596619852d2b83badff71 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Thu, 5 Apr 2018 14:46:52 -0400 Subject: [PATCH 10/18] User editing permission changes Only allow administrative action for a user who is a system admin or auditor if the the requesting-user is a system admin. Previously a user could be edited if the requesting-user was an admin of ANY of the orgs the user was member of. This is changed to require admin permission to ALL orgs the user is member of. As a special-case, allow org admins to add a user as a member to their organization if the following conditions are met: - the user is not member of any other orgs - the org admin has permissions to all of the roles the user has --- awx/main/access.py | 33 ++++++- awx/main/tests/functional/test_rbac_role.py | 96 ++++++++++++++++++++- 2 files changed, 121 insertions(+), 8 deletions(-) diff --git a/awx/main/access.py b/awx/main/access.py index ddecce39e2..73281630bf 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -491,10 +491,35 @@ class UserAccess(BaseAccess): # that a user should be able to edit for themselves. return bool(self.user == obj or self.can_admin(obj, data)) + def user_membership_roles(self, u): + return Role.objects.filter( + content_type=ContentType.objects.get_for_model(Organization), + role_field__in=['admin_role', 'member_role'], + members=u + ) + + def is_all_org_admin(self, u): + return not self.user_membership_roles(u).exclude( + ancestors__in=self.user.roles.filter(role_field='admin_role') + ).exists() + + def user_is_orphaned(self, u): + return not self.user_membership_roles(u).exists() + @check_superuser - def can_admin(self, obj, data): - return Organization.objects.filter(Q(member_role__members=obj) | Q(admin_role__members=obj), - Q(admin_role__members=self.user)).exists() + def can_admin(self, obj, data, allow_orphans=False): + if obj.is_superuser or obj.is_system_auditor: + # must be superuser to admin users with system roles + return False + if self.user_is_orphaned(obj): + if not allow_orphans: + # in these cases only superusers can modify orphan users + return False + return not obj.roles.all().exclude( + content_type=ContentType.objects.get_for_model(User) + ).filter(ancestors__in=self.user.roles.all()).exists() + else: + return self.is_all_org_admin(obj) def can_delete(self, obj): if obj == self.user: @@ -2401,7 +2426,7 @@ class RoleAccess(BaseAccess): # unwanted escalations lets ensure that the Organization administartor has the abilty # to admin the user being added to the role. if isinstance(obj.content_object, Organization) and obj.role_field in ['member_role', 'admin_role']: - if not UserAccess(self.user).can_admin(sub_obj, None): + if not UserAccess(self.user).can_admin(sub_obj, None, allow_orphans=True): return False if isinstance(obj.content_object, ResourceMixin) and \ diff --git a/awx/main/tests/functional/test_rbac_role.py b/awx/main/tests/functional/test_rbac_role.py index 62b8469abf..7ec80e8d94 100644 --- a/awx/main/tests/functional/test_rbac_role.py +++ b/awx/main/tests/functional/test_rbac_role.py @@ -4,6 +4,7 @@ from awx.main.access import ( RoleAccess, UserAccess, TeamAccess) +from awx.main.models import Role, Organization @pytest.mark.django_db @@ -35,12 +36,99 @@ def test_role_access_attach(rando, inventory): @pytest.mark.django_db -def test_org_user_role_attach(user, organization): +def test_visible_roles(admin_user, system_auditor, rando, organization, project): + ''' + system admin & system auditor fixtures needed to create system roles + ''' + organization.auditor_role.members.add(rando) + access = RoleAccess(rando) + + assert rando not in organization.admin_role + assert access.can_read(organization.admin_role) + assert organization.admin_role in Role.visible_roles(rando) + + assert rando not in project.admin_role + assert access.can_read(project.admin_role) + assert project.admin_role in Role.visible_roles(rando) + + +# Permissions when adding users to org member/admin +@pytest.mark.django_db +def test_org_user_role_attach(user, organization, inventory): + ''' + Org admins must not be able to add arbitrary users to their + organization, because that would give them admin permission to that user + ''' admin = user('admin') nonmember = user('nonmember') + inventory.admin_role.members.add(nonmember) organization.admin_role.members.add(admin) - access = RoleAccess(admin) - assert not access.can_attach(organization.member_role, nonmember, 'members', None) - assert not access.can_attach(organization.admin_role, nonmember, 'members', None) + role_access = RoleAccess(admin) + assert not role_access.can_attach(organization.member_role, nonmember, 'members', None) + assert not role_access.can_attach(organization.admin_role, nonmember, 'members', None) + + +# Singleton user editing restrictions +@pytest.mark.django_db +def test_org_superuser_role_attach(admin_user, org_admin, organization): + ''' + Ideally, you would not add superusers to roles (particularly member_role) + but it has historically been possible + this checks that the situation does not grant unexpected permissions + ''' + organization.member_role.members.add(admin_user) + + role_access = RoleAccess(org_admin) + assert not role_access.can_attach(organization.member_role, admin_user, 'members', None) + assert not role_access.can_attach(organization.admin_role, admin_user, 'members', None) + user_access = UserAccess(org_admin) + assert not user_access.can_change(admin_user, {'last_name': 'Witzel'}) + + +# Org admin user editing permission ANY to ALL change +@pytest.mark.django_db +def test_need_all_orgs_to_admin_user(user): + ''' + Old behavior - org admin to ANY organization that a user is member of + grants permission to admin that user + New behavior enforced here - org admin to ALL organizations that a + user is member of grants permission to admin that user + ''' + org1 = Organization.objects.create(name='org1') + org2 = Organization.objects.create(name='org2') + + org1_admin = user('org1-admin') + org1.admin_role.members.add(org1_admin) + + org12_member = user('org12-member') + org1.member_role.members.add(org12_member) + org2.member_role.members.add(org12_member) + + user_access = UserAccess(org1_admin) + assert not user_access.can_change(org12_member, {'last_name': 'Witzel'}) + + role_access = RoleAccess(org1_admin) + assert not role_access.can_attach(org1.admin_role, org12_member, 'members', None) + assert not role_access.can_attach(org1.member_role, org12_member, 'members', None) + + org2.admin_role.members.add(org1_admin) + assert role_access.can_attach(org1.admin_role, org12_member, 'members', None) + assert role_access.can_attach(org1.member_role, org12_member, 'members', None) + + +# Orphaned user can be added to member role, only in special cases +@pytest.mark.django_db +def test_orphaned_user_allowed(org_admin, rando, organization): + ''' + We still allow adoption of orphaned* users by assigning them to + organization member role, but only in the situation where the + org admin already posesses indirect access to all of the user's roles + *orphaned means user is not a member of any organization + ''' + role_access = RoleAccess(org_admin) + assert role_access.can_attach(organization.member_role, rando, 'members', None) + # Cannot edit the user directly without adding to org first + user_access = UserAccess(org_admin) + assert not user_access.can_change(rando, {'last_name': 'Witzel'}) From 77aab65f57a4623f189e3f72a8e270fc0712ae6e Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Thu, 12 Apr 2018 13:20:28 -0400 Subject: [PATCH 11/18] fix no_log leaking with_items values --- awx/lib/awx_display_callback/module.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/awx/lib/awx_display_callback/module.py b/awx/lib/awx_display_callback/module.py index 8aef42301f..422d00640b 100644 --- a/awx/lib/awx_display_callback/module.py +++ b/awx/lib/awx_display_callback/module.py @@ -330,6 +330,14 @@ class BaseCallbackModule(CallbackBase): with self.capture_event_data('playbook_on_stats', **event_data): super(BaseCallbackModule, self).v2_playbook_on_stats(stats) + @staticmethod + def _get_event_loop(task): + if hasattr(task, 'loop_with'): # Ansible >=2.5 + return task.loop_with + elif hasattr(task, 'loop'): # Ansible <2.4 + return task.loop + return None + def v2_runner_on_ok(self, result): # FIXME: Display detailed results or not based on verbosity. @@ -343,7 +351,7 @@ class BaseCallbackModule(CallbackBase): remote_addr=result._host.address, task=result._task, res=result._result, - event_loop=result._task.loop if hasattr(result._task, 'loop') else None, + event_loop=self._get_event_loop(result._task), ) with self.capture_event_data('runner_on_ok', **event_data): super(BaseCallbackModule, self).v2_runner_on_ok(result) @@ -356,7 +364,7 @@ class BaseCallbackModule(CallbackBase): res=result._result, task=result._task, ignore_errors=ignore_errors, - event_loop=result._task.loop if hasattr(result._task, 'loop') else None, + event_loop=self._get_event_loop(result._task), ) with self.capture_event_data('runner_on_failed', **event_data): super(BaseCallbackModule, self).v2_runner_on_failed(result, ignore_errors) @@ -366,7 +374,7 @@ class BaseCallbackModule(CallbackBase): host=result._host.get_name(), remote_addr=result._host.address, task=result._task, - event_loop=result._task.loop if hasattr(result._task, 'loop') else None, + event_loop=self._get_event_loop(result._task), ) with self.capture_event_data('runner_on_skipped', **event_data): super(BaseCallbackModule, self).v2_runner_on_skipped(result) From bba7f45972f92693ac0837f476011f0649c79b4d Mon Sep 17 00:00:00 2001 From: Bill Nottingham Date: Fri, 2 Feb 2018 23:30:51 -0500 Subject: [PATCH 12/18] Pass extra vars via file rather than via commandline, including custom creds. The extra vars file created lives in the playbook private runtime directory, and will be reaped along with the rest of the directory. Adjust assorted unit tests as necessary. --- awx/main/models/credential.py | 14 +- awx/main/tasks.py | 17 ++- .../tests/unit/models/test_survey_models.py | 8 +- awx/main/tests/unit/test_tasks.py | 141 +++++++++++------- 4 files changed, 118 insertions(+), 62 deletions(-) diff --git a/awx/main/models/credential.py b/awx/main/models/credential.py index f33ed07ee4..e4bbfff675 100644 --- a/awx/main/models/credential.py +++ b/awx/main/models/credential.py @@ -586,11 +586,21 @@ class CredentialType(CommonModelNameNotUnique): extra_vars[var_name] = Template(tmpl).render(**namespace) safe_extra_vars[var_name] = Template(tmpl).render(**safe_namespace) + def build_extra_vars_file(vars, private_dir): + handle, path = tempfile.mkstemp(dir = private_dir) + f = os.fdopen(handle, 'w') + f.write(json.dumps(vars)) + f.close() + os.chmod(path, stat.S_IRUSR) + return path + if extra_vars: - args.extend(['-e', json.dumps(extra_vars)]) + path = build_extra_vars_file(extra_vars, private_data_dir) + args.extend(['-e', '@%s' % path]) if safe_extra_vars: - safe_args.extend(['-e', json.dumps(safe_extra_vars)]) + path = build_extra_vars_file(safe_extra_vars, private_data_dir) + safe_args.extend(['-e', '@%s' % path]) @CredentialType.default diff --git a/awx/main/tasks.py b/awx/main/tasks.py index 52d70f51d3..1aeae770d2 100644 --- a/awx/main/tasks.py +++ b/awx/main/tasks.py @@ -622,6 +622,14 @@ class BaseTask(LogErrorsTask): '': '', } + def build_extra_vars_file(self, vars, **kwargs): + handle, path = tempfile.mkstemp(dir=kwargs.get('private_data_dir', None)) + f = os.fdopen(handle, 'w') + f.write(json.dumps(vars)) + f.close() + os.chmod(path, stat.S_IRUSR) + return path + def add_ansible_venv(self, env, add_awx_lib=True): env['VIRTUAL_ENV'] = settings.ANSIBLE_VENV_PATH env['PATH'] = os.path.join(settings.ANSIBLE_VENV_PATH, "bin") + ":" + env['PATH'] @@ -1205,7 +1213,8 @@ class RunJob(BaseTask): extra_vars.update(json.loads(job.display_extra_vars())) else: extra_vars.update(json.loads(job.decrypted_extra_vars())) - args.extend(['-e', json.dumps(extra_vars)]) + extra_vars_path = self.build_extra_vars_file(vars=extra_vars, **kwargs) + args.extend(['-e', '@%s' % (extra_vars_path)]) # Add path to playbook (relative to project.local_path). args.append(job.playbook) @@ -1460,7 +1469,8 @@ class RunProjectUpdate(BaseTask): 'scm_revision_output': self.revision_path, 'scm_revision': project_update.project.scm_revision, }) - args.extend(['-e', json.dumps(extra_vars)]) + extra_vars_path = self.build_extra_vars_file(vars=extra_vars, **kwargs) + args.extend(['-e', '@%s' % (extra_vars_path)]) args.append('project_update.yml') return args @@ -2220,7 +2230,8 @@ class RunAdHocCommand(BaseTask): "{} are prohibited from use in ad hoc commands." ).format(", ".join(removed_vars))) extra_vars.update(ad_hoc_command.extra_vars_dict) - args.extend(['-e', json.dumps(extra_vars)]) + extra_vars_path = self.build_extra_vars_file(vars=extra_vars, **kwargs) + args.extend(['-e', '@%s' % (extra_vars_path)]) args.extend(['-m', ad_hoc_command.module_name]) args.extend(['-a', ad_hoc_command.module_args]) diff --git a/awx/main/tests/unit/models/test_survey_models.py b/awx/main/tests/unit/models/test_survey_models.py index abd0b5fbba..284f69296c 100644 --- a/awx/main/tests/unit/models/test_survey_models.py +++ b/awx/main/tests/unit/models/test_survey_models.py @@ -71,7 +71,9 @@ def test_job_safe_args_redacted_passwords(job): run_job = RunJob() safe_args = run_job.build_safe_args(job, **kwargs) ev_index = safe_args.index('-e') + 1 - extra_vars = json.loads(safe_args[ev_index]) + extra_var_file = open(safe_args[ev_index][1:], 'r') + extra_vars = json.load(extra_var_file) + extra_var_file.close() assert extra_vars['secret_key'] == '$encrypted$' @@ -80,7 +82,9 @@ def test_job_args_unredacted_passwords(job, tmpdir_factory): run_job = RunJob() args = run_job.build_args(job, **kwargs) ev_index = args.index('-e') + 1 - extra_vars = json.loads(args[ev_index]) + extra_var_file = open(args[ev_index][1:], 'r') + extra_vars = json.load(extra_var_file) + extra_var_file.close() assert extra_vars['secret_key'] == 'my_password' diff --git a/awx/main/tests/unit/test_tasks.py b/awx/main/tests/unit/test_tasks.py index 20a051ec30..f807e02165 100644 --- a/awx/main/tests/unit/test_tasks.py +++ b/awx/main/tests/unit/test_tasks.py @@ -172,6 +172,15 @@ def pytest_generate_tests(metafunc): ) +def parse_extra_vars(args): + extra_vars = {} + for chunk in args: + if chunk.startswith('@/tmp/'): + with open(chunk.strip('@'), 'r') as f: + extra_vars.update(json.load(f)) + return extra_vars + + class TestJobExecution: """ For job runs, test that `ansible-playbook` is invoked with the proper @@ -296,15 +305,18 @@ class TestGenericRun(TestJobExecution): def test_created_by_extra_vars(self): self.instance.created_by = User(pk=123, username='angry-spud') - self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - assert '"tower_user_id": 123,' in ' '.join(args) - assert '"tower_user_name": "angry-spud"' in ' '.join(args) - assert '"awx_user_id": 123,' in ' '.join(args) - assert '"awx_user_name": "angry-spud"' in ' '.join(args) + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['tower_user_id'] == 123 + assert extra_vars['tower_user_name'] == "angry-spud" + assert extra_vars['awx_user_id'] == 123 + assert extra_vars['awx_user_name'] == "angry-spud" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) def test_survey_extra_vars(self): self.instance.extra_vars = json.dumps({ @@ -313,12 +325,15 @@ class TestGenericRun(TestJobExecution): self.instance.survey_passwords = { 'super_secret': '$encrypted$' } - self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - assert '"super_secret": "CLASSIFIED"' in ' '.join(args) + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['super_secret'] == "CLASSIFIED" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) def test_awx_task_env(self): patch = mock.patch('awx.main.tasks.settings.AWX_TASK_ENV', {'FOO': 'BAR'}) @@ -385,16 +400,19 @@ class TestAdhocRun(TestJobExecution): def test_created_by_extra_vars(self): self.instance.created_by = User(pk=123, username='angry-spud') - self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - assert '"tower_user_id": 123,' in ' '.join(args) - assert '"tower_user_name": "angry-spud"' in ' '.join(args) - assert '"awx_user_id": 123,' in ' '.join(args) - assert '"awx_user_name": "angry-spud"' in ' '.join(args) - assert '"awx_foo": "awx-bar' in ' '.join(args) + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['tower_user_id'] == 123 + assert extra_vars['tower_user_name'] == "angry-spud" + assert extra_vars['awx_user_id'] == 123 + assert extra_vars['awx_user_name'] == "angry-spud" + assert extra_vars['awx_foo'] == "awx-bar" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) class TestIsolatedExecution(TestJobExecution): @@ -986,14 +1004,16 @@ class TestJobCredentials(TestJobExecution): inputs = {'api_token': 'ABC123'} ) self.instance.extra_credentials.add(credential) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars["api_token"] == "ABC123" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - - assert '-e {"api_token": "ABC123"}' in ' '.join(args) - def test_custom_environment_injectors_with_boolean_extra_vars(self): some_cloud = CredentialType( kind='cloud', @@ -1018,12 +1038,15 @@ class TestJobCredentials(TestJobExecution): inputs={'turbo_button': True} ) self.instance.extra_credentials.add(credential) - self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - assert '-e {"turbo_button": "True"}' in ' '.join(args) + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars["turbo_button"] == "True" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) def test_custom_environment_injectors_with_complicated_boolean_template(self): some_cloud = CredentialType( @@ -1049,12 +1072,15 @@ class TestJobCredentials(TestJobExecution): inputs={'turbo_button': True} ) self.instance.extra_credentials.add(credential) - self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - assert '-e {"turbo_button": "FAST!"}' in ' '.join(args) + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars["turbo_button"] == "FAST!" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) def test_custom_environment_injectors_with_secret_extra_vars(self): """ @@ -1085,13 +1111,16 @@ class TestJobCredentials(TestJobExecution): ) credential.inputs['password'] = encrypt_field(credential, 'password') self.instance.extra_credentials.add(credential) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars["password"] == "SUPER-SECRET-123" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - - assert '-e {"password": "SUPER-SECRET-123"}' in ' '.join(args) assert 'SUPER-SECRET-123' not in json.dumps(self.task.update_model.call_args_list) def test_custom_environment_injectors_with_file(self): @@ -1217,20 +1246,22 @@ class TestProjectUpdateCredentials(TestJobExecution): pk=1, credential_type=ssh, ) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert ' '.join(args).startswith('bwrap') + assert ' '.join([ + '--bind', + os.path.realpath(settings.PROJECTS_ROOT), + os.path.realpath(settings.PROJECTS_ROOT) + ]) in ' '.join(args) + assert extra_vars["scm_revision_output"].startswith(settings.PROJECTS_ROOT) + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, call_kwargs = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - - assert ' '.join(args).startswith('bwrap') - ' '.join([ - '--bind', - settings.PROJECTS_ROOT, - settings.PROJECTS_ROOT, - ]) in ' '.join(args) - assert '"scm_revision_output": "/projects/tmp' in ' '.join(args) - def test_username_and_password_auth(self, scm_type): ssh = CredentialType.defaults['ssh']() self.instance.scm_type = scm_type From 88c243c92ae7ebf2fbad087411f5896c3d8749fd Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Wed, 11 Apr 2018 18:14:08 -0400 Subject: [PATCH 13/18] mark all unsafe launch-time extra vars as !unsafe see: https://github.com/ansible/tower/issues/1338 see: https://bugzilla.redhat.com/show_bug.cgi?id=1565865 --- awx/main/conf.py | 9 ++ awx/main/models/credential.py | 4 +- awx/main/tasks.py | 18 ++- .../tests/unit/models/test_survey_models.py | 6 +- awx/main/tests/unit/test_tasks.py | 134 +++++++++++++++++- awx/main/tests/unit/utils/test_safe_yaml.py | 59 ++++++++ awx/main/utils/safe_yaml.py | 66 +++++++++ awx/settings/defaults.py | 3 + 8 files changed, 290 insertions(+), 9 deletions(-) create mode 100644 awx/main/tests/unit/utils/test_safe_yaml.py create mode 100644 awx/main/utils/safe_yaml.py diff --git a/awx/main/conf.py b/awx/main/conf.py index 63714ab4bb..0ae74a6dfc 100644 --- a/awx/main/conf.py +++ b/awx/main/conf.py @@ -132,6 +132,15 @@ register( required=False, ) +register( + 'ALLOW_JINJA_IN_JOB_TEMPLATE_EXTRA_VARS', + field_class=fields.BooleanField, + label=_('Allow Jinja template execution in Job Template extra vars'), + help_text=_('Ansible allows variable substitution and templating via the Jinja2 templating language for a variety of arguments (such as --extra-vars); enabling this flag allows arbitrary Jinja templates to be used on extra vars defined in Job Templates.'), # noqa + category=_('Jobs'), + category_slug='jobs', +) + register( 'AWX_PROOT_ENABLED', field_class=fields.BooleanField, diff --git a/awx/main/models/credential.py b/awx/main/models/credential.py index e4bbfff675..fbdcbcafe8 100644 --- a/awx/main/models/credential.py +++ b/awx/main/models/credential.py @@ -2,7 +2,6 @@ # All Rights Reserved. from collections import OrderedDict import functools -import json import logging import operator import os @@ -25,6 +24,7 @@ from awx.main.fields import (ImplicitRoleField, CredentialInputField, CredentialTypeInputField, CredentialTypeInjectorField) from awx.main.utils import decrypt_field +from awx.main.utils.safe_yaml import safe_dump from awx.main.validators import validate_ssh_private_key from awx.main.models.base import * # noqa from awx.main.models.mixins import ResourceMixin @@ -589,7 +589,7 @@ class CredentialType(CommonModelNameNotUnique): def build_extra_vars_file(vars, private_dir): handle, path = tempfile.mkstemp(dir = private_dir) f = os.fdopen(handle, 'w') - f.write(json.dumps(vars)) + f.write(safe_dump(vars)) f.close() os.chmod(path, stat.S_IRUSR) return path diff --git a/awx/main/tasks.py b/awx/main/tasks.py index 1aeae770d2..b9af133218 100644 --- a/awx/main/tasks.py +++ b/awx/main/tasks.py @@ -58,6 +58,7 @@ from awx.main.utils import (get_ansible_version, get_ssh_version, decrypt_field, wrap_args_with_proot, get_system_task_capacity, OutputEventFilter, parse_yaml_or_json, ignore_inventory_computed_fields, ignore_inventory_group_removal, get_type_for_model, extract_ansible_vars) +from awx.main.utils.safe_yaml import safe_dump from awx.main.utils.reload import restart_local_services, stop_local_services from awx.main.utils.handlers import configure_external_logger from awx.main.consumers import emit_channel_notification @@ -625,7 +626,7 @@ class BaseTask(LogErrorsTask): def build_extra_vars_file(self, vars, **kwargs): handle, path = tempfile.mkstemp(dir=kwargs.get('private_data_dir', None)) f = os.fdopen(handle, 'w') - f.write(json.dumps(vars)) + f.write(safe_dump(vars, kwargs.get('safe_dict', {}) or None)) f.close() os.chmod(path, stat.S_IRUSR) return path @@ -1213,7 +1214,20 @@ class RunJob(BaseTask): extra_vars.update(json.loads(job.display_extra_vars())) else: extra_vars.update(json.loads(job.decrypted_extra_vars())) - extra_vars_path = self.build_extra_vars_file(vars=extra_vars, **kwargs) + + # By default, all extra vars disallow Jinja2 template usage for + # security reasons; top level key-values defined in JT.extra_vars, however, + # are whitelisted as "safe" (because they can only be set by users with + # higher levels of privilege - those that have the ability create and + # edit Job Templates) + safe_dict = {} + if job.job_template and settings.ALLOW_JINJA_IN_JOB_TEMPLATE_EXTRA_VARS is True: + safe_dict = job.job_template.extra_vars_dict + extra_vars_path = self.build_extra_vars_file( + vars=extra_vars, + safe_dict=safe_dict, + **kwargs + ) args.extend(['-e', '@%s' % (extra_vars_path)]) # Add path to playbook (relative to project.local_path). diff --git a/awx/main/tests/unit/models/test_survey_models.py b/awx/main/tests/unit/models/test_survey_models.py index 284f69296c..a235984ed8 100644 --- a/awx/main/tests/unit/models/test_survey_models.py +++ b/awx/main/tests/unit/models/test_survey_models.py @@ -1,6 +1,7 @@ import tempfile import pytest import json +import yaml from awx.main.utils.encryption import encrypt_value from awx.main.tasks import RunJob @@ -9,6 +10,7 @@ from awx.main.models import ( JobTemplate, WorkflowJobTemplate ) +from awx.main.utils.safe_yaml import SafeLoader ENCRYPTED_SECRET = encrypt_value('secret') @@ -72,7 +74,7 @@ def test_job_safe_args_redacted_passwords(job): 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 = json.load(extra_var_file) + extra_vars = yaml.load(extra_var_file, SafeLoader) extra_var_file.close() assert extra_vars['secret_key'] == '$encrypted$' @@ -83,7 +85,7 @@ def test_job_args_unredacted_passwords(job, tmpdir_factory): args = run_job.build_args(job, **kwargs) ev_index = args.index('-e') + 1 extra_var_file = open(args[ev_index][1:], 'r') - extra_vars = json.load(extra_var_file) + extra_vars = yaml.load(extra_var_file, SafeLoader) extra_var_file.close() assert extra_vars['secret_key'] == 'my_password' diff --git a/awx/main/tests/unit/test_tasks.py b/awx/main/tests/unit/test_tasks.py index f807e02165..ffbd90a625 100644 --- a/awx/main/tests/unit/test_tasks.py +++ b/awx/main/tests/unit/test_tasks.py @@ -23,6 +23,7 @@ from awx.main.models import ( InventorySource, InventoryUpdate, Job, + JobTemplate, Notification, Project, ProjectUpdate, @@ -32,7 +33,7 @@ from awx.main.models import ( from awx.main import tasks from awx.main.utils import encrypt_field, encrypt_value - +from awx.main.utils.safe_yaml import SafeLoader @contextmanager @@ -177,7 +178,7 @@ def parse_extra_vars(args): for chunk in args: if chunk.startswith('@/tmp/'): with open(chunk.strip('@'), 'r') as f: - extra_vars.update(json.load(f)) + extra_vars.update(yaml.load(f, SafeLoader)) return extra_vars @@ -243,7 +244,8 @@ class TestJobExecution: cancel_flag=False, project=Project(), playbook='helloworld.yml', - verbosity=3 + verbosity=3, + job_template=JobTemplate(extra_vars='') ) # mock the job.extra_credentials M2M relation so we can avoid DB access @@ -263,6 +265,131 @@ class TestJobExecution: return self.instance.pk +class TestExtraVarSanitation(TestJobExecution): + # By default, extra vars are marked as `!unsafe` in the generated yaml + # _unless_ they've been specified on the JobTemplate's extra_vars (which + # are deemed trustable, because they can only be added by users w/ enough + # privilege to add/modify a Job Template) + + UNSAFE = '{{ lookup(''pipe'',''ls -la'') }}' + + def test_vars_unsafe_by_default(self): + self.instance.created_by = User(pk=123, username='angry-spud') + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + + # ensure that strings are marked as unsafe + for unsafe in ['awx_job_template_name', 'tower_job_template_name', + 'awx_user_name', 'tower_job_launch_type', + 'awx_project_revision', + 'tower_project_revision', 'tower_user_name', + 'awx_job_launch_type']: + assert hasattr(extra_vars[unsafe], '__UNSAFE__') + + # ensure that non-strings are marked as safe + for safe in ['awx_job_template_id', 'awx_job_id', 'awx_user_id', + 'tower_user_id', 'tower_job_template_id', + 'tower_job_id']: + assert not hasattr(extra_vars[safe], '__UNSAFE__') + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + def test_launchtime_vars_unsafe(self): + self.instance.extra_vars = json.dumps({'msg': self.UNSAFE}) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['msg'] == self.UNSAFE + assert hasattr(extra_vars['msg'], '__UNSAFE__') + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + def test_nested_launchtime_vars_unsafe(self): + self.instance.extra_vars = json.dumps({'msg': {'a': [self.UNSAFE]}}) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['msg'] == {'a': [self.UNSAFE]} + assert hasattr(extra_vars['msg']['a'][0], '__UNSAFE__') + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + def test_whitelisted_jt_extra_vars(self): + self.instance.job_template.extra_vars = self.instance.extra_vars = json.dumps({'msg': self.UNSAFE}) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['msg'] == self.UNSAFE + assert not hasattr(extra_vars['msg'], '__UNSAFE__') + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + def test_nested_whitelisted_vars(self): + self.instance.extra_vars = json.dumps({'msg': {'a': {'b': [self.UNSAFE]}}}) + self.instance.job_template.extra_vars = self.instance.extra_vars + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['msg'] == {'a': {'b': [self.UNSAFE]}} + assert not hasattr(extra_vars['msg']['a']['b'][0], '__UNSAFE__') + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + def test_sensitive_values_dont_leak(self): + # JT defines `msg=SENSITIVE`, the job *should not* be able to do + # `other_var=SENSITIVE` + self.instance.job_template.extra_vars = json.dumps({'msg': self.UNSAFE}) + self.instance.extra_vars = json.dumps({ + 'msg': 'other-value', + 'other_var': self.UNSAFE + }) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + + assert extra_vars['msg'] == 'other-value' + assert hasattr(extra_vars['msg'], '__UNSAFE__') + + assert extra_vars['other_var'] == self.UNSAFE + assert hasattr(extra_vars['other_var'], '__UNSAFE__') + + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + def test_overwritten_jt_extra_vars(self): + self.instance.job_template.extra_vars = json.dumps({'msg': 'SAFE'}) + self.instance.extra_vars = json.dumps({'msg': self.UNSAFE}) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['msg'] == self.UNSAFE + assert hasattr(extra_vars['msg'], '__UNSAFE__') + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + class TestGenericRun(TestJobExecution): def test_cancel_flag(self): @@ -1009,6 +1136,7 @@ class TestJobCredentials(TestJobExecution): args, cwd, env, stdout = args extra_vars = parse_extra_vars(args) assert extra_vars["api_token"] == "ABC123" + assert hasattr(extra_vars["api_token"], '__UNSAFE__') return ['successful', 0] self.run_pexpect.side_effect = run_pexpect_side_effect diff --git a/awx/main/tests/unit/utils/test_safe_yaml.py b/awx/main/tests/unit/utils/test_safe_yaml.py new file mode 100644 index 0000000000..1c06ff5ca1 --- /dev/null +++ b/awx/main/tests/unit/utils/test_safe_yaml.py @@ -0,0 +1,59 @@ +from copy import deepcopy +from awx.main.utils.safe_yaml import safe_dump + + +def test_empty(): + assert safe_dump({}) == '' + + +def test_kv_int(): + assert safe_dump({'a': 1}) == "!unsafe 'a': 1\n" + + +def test_kv_float(): + assert safe_dump({'a': 1.5}) == "!unsafe 'a': 1.5\n" + + +def test_kv_unsafe(): + assert safe_dump({'a': 'b'}) == "!unsafe 'a': !unsafe 'b'\n" + + +def test_kv_unsafe_in_list(): + assert safe_dump({'a': ['b']}) == "!unsafe 'a':\n- !unsafe 'b'\n" + + +def test_kv_unsafe_in_mixed_list(): + assert safe_dump({'a': [1, 'b']}) == "!unsafe 'a':\n- 1\n- !unsafe 'b'\n" + + +def test_kv_unsafe_deep_nesting(): + yaml = safe_dump({'a': [1, [{'b': {'c': [{'d': 'e'}]}}]]}) + for x in ('a', 'b', 'c', 'd', 'e'): + assert "!unsafe '{}'".format(x) in yaml + + +def test_kv_unsafe_multiple(): + assert safe_dump({'a': 'b', 'c': 'd'}) == '\n'.join([ + "!unsafe 'a': !unsafe 'b'", + "!unsafe 'c': !unsafe 'd'", + "" + ]) + + +def test_safe_marking(): + assert safe_dump({'a': 'b'}, safe_dict={'a': 'b'}) == "a: b\n" + + +def test_safe_marking_mixed(): + assert safe_dump({'a': 'b', 'c': 'd'}, safe_dict={'a': 'b'}) == '\n'.join([ + "a: b", + "!unsafe 'c': !unsafe 'd'", + "" + ]) + + +def test_safe_marking_deep_nesting(): + deep = {'a': [1, [{'b': {'c': [{'d': 'e'}]}}]]} + yaml = safe_dump(deep, deepcopy(deep)) + for x in ('a', 'b', 'c', 'd', 'e'): + assert "!unsafe '{}'".format(x) not in yaml diff --git a/awx/main/utils/safe_yaml.py b/awx/main/utils/safe_yaml.py new file mode 100644 index 0000000000..6b8bdfd2b4 --- /dev/null +++ b/awx/main/utils/safe_yaml.py @@ -0,0 +1,66 @@ +import six +import yaml + + +__all__ = ['safe_dump', 'SafeLoader'] + + +class SafeStringDumper(yaml.SafeDumper): + + def represent_data(self, value): + if isinstance(value, six.string_types): + return self.represent_scalar('!unsafe', value) + return super(SafeStringDumper, self).represent_data(value) + + +class SafeLoader(yaml.Loader): + + def construct_yaml_unsafe(self, node): + class UnsafeText(six.text_type): + __UNSAFE__ = True + node = UnsafeText(self.construct_scalar(node)) + return node + + +SafeLoader.add_constructor( + u'!unsafe', + SafeLoader.construct_yaml_unsafe +) + + +def safe_dump(x, safe_dict=None): + """ + Used to serialize an extra_vars dict to YAML + + By default, extra vars are marked as `!unsafe` in the generated yaml + _unless_ they've been deemed "trusted" (meaning, they likely were set/added + by a user with a high level of privilege). + + This function allows you to pass in a trusted `safe_dict` to whitelist + certain extra vars so that they are _not_ marked as `!unsafe` in the + resulting YAML. Anything _not_ in this dict will automatically be + `!unsafe`. + + safe_dump({'a': 'b', 'c': 'd'}) -> + !unsafe 'a': !unsafe 'b' + !unsafe 'c': !unsafe 'd' + + safe_dump({'a': 'b', 'c': 'd'}, safe_dict={'a': 'b'}) + a: b + !unsafe 'c': !unsafe 'd' + """ + yamls = [] + safe_dict = safe_dict or {} + # Compare the top level keys so that we can find values that have + # equality matches (and consider those branches safe) + for k, v in x.items(): + dumper = yaml.SafeDumper + if safe_dict.get(k) != v: + dumper = SafeStringDumper + yamls.append(yaml.dump_all( + [{k: v}], + None, + Dumper=dumper, + default_flow_style=False, + )) + return ''.join(yamls) diff --git a/awx/settings/defaults.py b/awx/settings/defaults.py index 89d1727188..66e17a0174 100644 --- a/awx/settings/defaults.py +++ b/awx/settings/defaults.py @@ -585,6 +585,9 @@ CAPTURE_JOB_EVENT_HOSTS = False # Rebuild Host Smart Inventory memberships. AWX_REBUILD_SMART_MEMBERSHIP = False +# By default, allow arbitrary Jinja templating in extra_vars defined on a Job Template +ALLOW_JINJA_IN_JOB_TEMPLATE_EXTRA_VARS = True + # Enable bubblewrap support for running jobs (playbook runs only). # Note: This setting may be overridden by database settings. AWX_PROOT_ENABLED = True From 7074dcd677be72718dd5e0c490635c5cd9862f64 Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Fri, 13 Apr 2018 16:32:39 -0400 Subject: [PATCH 14/18] don't allow usage of jinja templates in certain ansible CLI flags see: https://github.com/ansible/tower/issues/1338 --- awx/main/tasks.py | 16 +++++++-------- awx/main/tests/unit/test_tasks.py | 34 +++++++++++++++++++++++++++++++ awx/main/utils/safe_yaml.py | 17 ++++++++++++++++ 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/awx/main/tasks.py b/awx/main/tasks.py index b9af133218..7f68ed80ec 100644 --- a/awx/main/tasks.py +++ b/awx/main/tasks.py @@ -58,7 +58,7 @@ from awx.main.utils import (get_ansible_version, get_ssh_version, decrypt_field, wrap_args_with_proot, get_system_task_capacity, OutputEventFilter, parse_yaml_or_json, ignore_inventory_computed_fields, ignore_inventory_group_removal, get_type_for_model, extract_ansible_vars) -from awx.main.utils.safe_yaml import safe_dump +from awx.main.utils.safe_yaml import safe_dump, sanitize_jinja from awx.main.utils.reload import restart_local_services, stop_local_services from awx.main.utils.handlers import configure_external_logger from awx.main.consumers import emit_channel_notification @@ -1151,7 +1151,7 @@ class RunJob(BaseTask): args = ['ansible-playbook', '-i', self.build_inventory(job, **kwargs)] if job.job_type == 'check': args.append('--check') - args.extend(['-u', ssh_username]) + args.extend(['-u', sanitize_jinja(ssh_username)]) if 'ssh_password' in kwargs.get('passwords', {}): args.append('--ask-pass') if job.become_enabled: @@ -1159,9 +1159,9 @@ class RunJob(BaseTask): if job.diff_mode: args.append('--diff') if become_method: - args.extend(['--become-method', become_method]) + args.extend(['--become-method', sanitize_jinja(become_method)]) if become_username: - args.extend(['--become-user', become_username]) + args.extend(['--become-user', sanitize_jinja(become_username)]) if 'become_password' in kwargs.get('passwords', {}): args.append('--ask-become-pass') # Support prompting for a vault password. @@ -2203,7 +2203,7 @@ class RunAdHocCommand(BaseTask): args = ['ansible', '-i', self.build_inventory(ad_hoc_command, **kwargs)] if ad_hoc_command.job_type == 'check': args.append('--check') - args.extend(['-u', ssh_username]) + args.extend(['-u', sanitize_jinja(ssh_username)]) if 'ssh_password' in kwargs.get('passwords', {}): args.append('--ask-pass') # We only specify sudo/su user and password if explicitly given by the @@ -2211,9 +2211,9 @@ class RunAdHocCommand(BaseTask): if ad_hoc_command.become_enabled: args.append('--become') if become_method: - args.extend(['--become-method', become_method]) + args.extend(['--become-method', sanitize_jinja(become_method)]) if become_username: - args.extend(['--become-user', become_username]) + args.extend(['--become-user', sanitize_jinja(become_username)]) if 'become_password' in kwargs.get('passwords', {}): args.append('--ask-become-pass') @@ -2248,7 +2248,7 @@ class RunAdHocCommand(BaseTask): args.extend(['-e', '@%s' % (extra_vars_path)]) args.extend(['-m', ad_hoc_command.module_name]) - args.extend(['-a', ad_hoc_command.module_args]) + args.extend(['-a', sanitize_jinja(ad_hoc_command.module_args)]) if ad_hoc_command.limit: args.append(ad_hoc_command.limit) diff --git a/awx/main/tests/unit/test_tasks.py b/awx/main/tests/unit/test_tasks.py index ffbd90a625..e81fe794a5 100644 --- a/awx/main/tests/unit/test_tasks.py +++ b/awx/main/tests/unit/test_tasks.py @@ -525,6 +525,13 @@ class TestAdhocRun(TestJobExecution): extra_vars={'awx_foo': 'awx-bar'} ) + def test_options_jinja_usage(self): + self.instance.module_args = '{{ ansible_ssh_pass }}' + with pytest.raises(Exception): + self.task.run(self.pk) + update_model_call = self.task.update_model.call_args[1] + assert 'Jinja variables are not allowed' in update_model_call['result_traceback'] + def test_created_by_extra_vars(self): self.instance.created_by = User(pk=123, username='angry-spud') @@ -647,6 +654,33 @@ class TestJobCredentials(TestJobExecution): ] } + def test_username_jinja_usage(self): + ssh = CredentialType.defaults['ssh']() + credential = Credential( + pk=1, + credential_type=ssh, + inputs = {'username': '{{ ansible_ssh_pass }}'} + ) + self.instance.credential = credential + with pytest.raises(Exception): + self.task.run(self.pk) + update_model_call = self.task.update_model.call_args[1] + assert 'Jinja variables are not allowed' in update_model_call['result_traceback'] + + @pytest.mark.parametrize("flag", ['become_username', 'become_method']) + def test_become_jinja_usage(self, flag): + ssh = CredentialType.defaults['ssh']() + credential = Credential( + pk=1, + credential_type=ssh, + inputs = {'username': 'joe', flag: '{{ ansible_ssh_pass }}'} + ) + self.instance.credential = credential + with pytest.raises(Exception): + self.task.run(self.pk) + update_model_call = self.task.update_model.call_args[1] + assert 'Jinja variables are not allowed' in update_model_call['result_traceback'] + def test_ssh_passwords(self, field, password_name, expected_flag): ssh = CredentialType.defaults['ssh']() credential = Credential( diff --git a/awx/main/utils/safe_yaml.py b/awx/main/utils/safe_yaml.py index 6b8bdfd2b4..af572e9fd5 100644 --- a/awx/main/utils/safe_yaml.py +++ b/awx/main/utils/safe_yaml.py @@ -1,3 +1,4 @@ +import re import six import yaml @@ -64,3 +65,19 @@ def safe_dump(x, safe_dict=None): default_flow_style=False, )) return ''.join(yamls) + + +def sanitize_jinja(arg): + """ + For some string, prevent usage of Jinja-like flags + """ + if isinstance(arg, six.string_types): + # If the argument looks like it contains Jinja expressions + # {{ x }} ... + if re.search('\{\{[^}]+}}', arg) is not None: + raise ValueError('Inline Jinja variables are not allowed.') + # If the argument looks like it contains Jinja statements/control flow... + # {% if x.foo() %} ... + if re.search('\{%[^%]+%}', arg) is not None: + raise ValueError('Inline Jinja variables are not allowed.') + return arg From 73043019480a9e407223c1df35060eb6c27b0e53 Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Tue, 17 Apr 2018 10:24:14 -0400 Subject: [PATCH 15/18] don't bother building a safe extra vars namespace; it's a file path now --- awx/main/models/credential.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/awx/main/models/credential.py b/awx/main/models/credential.py index fbdcbcafe8..5eddacd225 100644 --- a/awx/main/models/credential.py +++ b/awx/main/models/credential.py @@ -581,10 +581,8 @@ class CredentialType(CommonModelNameNotUnique): safe_env[env_var] = Template(tmpl).render(**safe_namespace) extra_vars = {} - safe_extra_vars = {} for var_name, tmpl in self.injectors.get('extra_vars', {}).items(): extra_vars[var_name] = Template(tmpl).render(**namespace) - safe_extra_vars[var_name] = Template(tmpl).render(**safe_namespace) def build_extra_vars_file(vars, private_dir): handle, path = tempfile.mkstemp(dir = private_dir) @@ -594,12 +592,9 @@ class CredentialType(CommonModelNameNotUnique): os.chmod(path, stat.S_IRUSR) return path + path = build_extra_vars_file(extra_vars, private_data_dir) if extra_vars: - path = build_extra_vars_file(extra_vars, private_data_dir) args.extend(['-e', '@%s' % path]) - - if safe_extra_vars: - path = build_extra_vars_file(safe_extra_vars, private_data_dir) safe_args.extend(['-e', '@%s' % path]) From fe47b75aad6787f41037e3c1d1f0628efa875a1c Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Tue, 17 Apr 2018 12:08:07 -0400 Subject: [PATCH 16/18] use a three-prong setting for Jinja extra vars policy --- awx/main/conf.py | 20 ++++++++++++++++---- awx/main/tasks.py | 10 ++++++---- awx/settings/defaults.py | 2 +- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/awx/main/conf.py b/awx/main/conf.py index 0ae74a6dfc..3dc3453fb1 100644 --- a/awx/main/conf.py +++ b/awx/main/conf.py @@ -133,10 +133,22 @@ register( ) register( - 'ALLOW_JINJA_IN_JOB_TEMPLATE_EXTRA_VARS', - field_class=fields.BooleanField, - label=_('Allow Jinja template execution in Job Template extra vars'), - help_text=_('Ansible allows variable substitution and templating via the Jinja2 templating language for a variety of arguments (such as --extra-vars); enabling this flag allows arbitrary Jinja templates to be used on extra vars defined in Job Templates.'), # noqa + 'ALLOW_JINJA_IN_EXTRA_VARS', + field_class=fields.ChoiceField, + choices=[ + ('always', _('Always')), + ('never', _('Never')), + ('template', _('Only On Job Template Definitions')), + ], + required=True, + label=_('When can extra variables contain Jinja templates?'), + help_text=_( + 'Ansible allows variable substitution via the Jinja2 templating ' + 'language for --extra-vars. This poses a potential security ' + 'risk where Tower users with the ability to specify extra vars at job ' + 'launch time can use Jinja2 templates to run arbitrary Python. It is ' + 'recommended that this value be set to "template" or "never".' + ), category=_('Jobs'), category_slug='jobs', ) diff --git a/awx/main/tasks.py b/awx/main/tasks.py index 7f68ed80ec..ce85ab4d83 100644 --- a/awx/main/tasks.py +++ b/awx/main/tasks.py @@ -626,7 +626,10 @@ class BaseTask(LogErrorsTask): def build_extra_vars_file(self, vars, **kwargs): handle, path = tempfile.mkstemp(dir=kwargs.get('private_data_dir', None)) f = os.fdopen(handle, 'w') - f.write(safe_dump(vars, kwargs.get('safe_dict', {}) or None)) + if settings.ALLOW_JINJA_IN_EXTRA_VARS == 'always': + f.write(yaml.safe_dump(vars)) + else: + f.write(safe_dump(vars, kwargs.get('safe_dict', {}) or None)) f.close() os.chmod(path, stat.S_IRUSR) return path @@ -909,8 +912,7 @@ class BaseTask(LogErrorsTask): except Exception: if status != 'canceled': tb = traceback.format_exc() - if settings.DEBUG: - logger.exception('%s Exception occurred while running task', instance.log_format) + logger.exception('%s Exception occurred while running task', instance.log_format) finally: try: stdout_handle.flush() @@ -1221,7 +1223,7 @@ class RunJob(BaseTask): # higher levels of privilege - those that have the ability create and # edit Job Templates) safe_dict = {} - if job.job_template and settings.ALLOW_JINJA_IN_JOB_TEMPLATE_EXTRA_VARS is True: + if job.job_template and settings.ALLOW_JINJA_IN_EXTRA_VARS == 'template': safe_dict = job.job_template.extra_vars_dict extra_vars_path = self.build_extra_vars_file( vars=extra_vars, diff --git a/awx/settings/defaults.py b/awx/settings/defaults.py index 66e17a0174..54b6110812 100644 --- a/awx/settings/defaults.py +++ b/awx/settings/defaults.py @@ -586,7 +586,7 @@ CAPTURE_JOB_EVENT_HOSTS = False AWX_REBUILD_SMART_MEMBERSHIP = False # By default, allow arbitrary Jinja templating in extra_vars defined on a Job Template -ALLOW_JINJA_IN_JOB_TEMPLATE_EXTRA_VARS = True +ALLOW_JINJA_IN_EXTRA_VARS = 'template' # Enable bubblewrap support for running jobs (playbook runs only). # Note: This setting may be overridden by database settings. From 835f2eebc38785aa0876524b44c4f70ec96720ad Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Tue, 17 Apr 2018 15:39:37 -0400 Subject: [PATCH 17/18] make extra var YAML serialization more robust to non-dict extra vars --- awx/main/tests/unit/utils/test_safe_yaml.py | 26 ++++++++++++++++ awx/main/utils/safe_yaml.py | 34 ++++++++++++--------- 2 files changed, 45 insertions(+), 15 deletions(-) diff --git a/awx/main/tests/unit/utils/test_safe_yaml.py b/awx/main/tests/unit/utils/test_safe_yaml.py index 1c06ff5ca1..7f528ea595 100644 --- a/awx/main/tests/unit/utils/test_safe_yaml.py +++ b/awx/main/tests/unit/utils/test_safe_yaml.py @@ -1,11 +1,25 @@ +# -*- coding: utf-8 -*- + from copy import deepcopy +import pytest +import yaml from awx.main.utils.safe_yaml import safe_dump +@pytest.mark.parametrize('value', [None, 1, 1.5, []]) +def test_native_types(value): + # Native non-string types should dump the same way that `yaml.safe_dump` does + assert safe_dump(value) == yaml.safe_dump(value) + + def test_empty(): assert safe_dump({}) == '' +def test_raw_string(): + assert safe_dump('foo') == "!unsafe 'foo'\n" + + def test_kv_int(): assert safe_dump({'a': 1}) == "!unsafe 'a': 1\n" @@ -18,6 +32,10 @@ def test_kv_unsafe(): assert safe_dump({'a': 'b'}) == "!unsafe 'a': !unsafe 'b'\n" +def test_kv_unsafe_unicode(): + assert safe_dump({'a': u'🐉'}) == '!unsafe \'a\': !unsafe "\\U0001F409"\n' + + def test_kv_unsafe_in_list(): assert safe_dump({'a': ['b']}) == "!unsafe 'a':\n- !unsafe 'b'\n" @@ -57,3 +75,11 @@ def test_safe_marking_deep_nesting(): yaml = safe_dump(deep, deepcopy(deep)) for x in ('a', 'b', 'c', 'd', 'e'): assert "!unsafe '{}'".format(x) not in yaml + + +def test_deep_diff_unsafe_marking(): + deep = {'a': [1, [{'b': {'c': [{'d': 'e'}]}}]]} + jt_vars = deepcopy(deep) + deep['a'][1][0]['b']['z'] = 'not safe' + yaml = safe_dump(deep, jt_vars) + assert "!unsafe 'z'" in yaml diff --git a/awx/main/utils/safe_yaml.py b/awx/main/utils/safe_yaml.py index af572e9fd5..1b958b69d0 100644 --- a/awx/main/utils/safe_yaml.py +++ b/awx/main/utils/safe_yaml.py @@ -50,21 +50,25 @@ def safe_dump(x, safe_dict=None): a: b !unsafe 'c': !unsafe 'd' """ - yamls = [] - safe_dict = safe_dict or {} - # Compare the top level keys so that we can find values that have - # equality matches (and consider those branches safe) - for k, v in x.items(): - dumper = yaml.SafeDumper - if safe_dict.get(k) != v: - dumper = SafeStringDumper - yamls.append(yaml.dump_all( - [{k: v}], - None, - Dumper=dumper, - default_flow_style=False, - )) - return ''.join(yamls) + if isinstance(x, dict): + yamls = [] + safe_dict = safe_dict or {} + + # Compare the top level keys so that we can find values that have + # equality matches (and consider those branches safe) + for k, v in x.items(): + dumper = yaml.SafeDumper + if safe_dict.get(k) != v: + dumper = SafeStringDumper + yamls.append(yaml.dump_all( + [{k: v}], + None, + Dumper=dumper, + default_flow_style=False, + )) + return ''.join(yamls) + else: + return yaml.dump_all([x], None, Dumper=SafeStringDumper, default_flow_style=False) def sanitize_jinja(arg): From f8211b05882ee487b74dd04a34875053b6cf0ebe Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Thu, 19 Apr 2018 09:13:56 -0400 Subject: [PATCH 18/18] add more edge case handling for yaml unsafe marking --- awx/main/tests/unit/utils/test_safe_yaml.py | 12 ++++++++++++ awx/main/utils/safe_yaml.py | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/awx/main/tests/unit/utils/test_safe_yaml.py b/awx/main/tests/unit/utils/test_safe_yaml.py index 7f528ea595..8e8dd933aa 100644 --- a/awx/main/tests/unit/utils/test_safe_yaml.py +++ b/awx/main/tests/unit/utils/test_safe_yaml.py @@ -20,6 +20,18 @@ def test_raw_string(): assert safe_dump('foo') == "!unsafe 'foo'\n" +def test_kv_null(): + assert safe_dump({'a': None}) == "!unsafe 'a': null\n" + + +def test_kv_null_safe(): + assert safe_dump({'a': None}, {'a': None}) == "a: null\n" + + +def test_kv_null_unsafe(): + assert safe_dump({'a': ''}, {'a': None}) == "!unsafe 'a': !unsafe ''\n" + + def test_kv_int(): assert safe_dump({'a': 1}) == "!unsafe 'a': 1\n" diff --git a/awx/main/utils/safe_yaml.py b/awx/main/utils/safe_yaml.py index 1b958b69d0..28c4dc4694 100644 --- a/awx/main/utils/safe_yaml.py +++ b/awx/main/utils/safe_yaml.py @@ -58,7 +58,7 @@ def safe_dump(x, safe_dict=None): # equality matches (and consider those branches safe) for k, v in x.items(): dumper = yaml.SafeDumper - if safe_dict.get(k) != v: + if k not in safe_dict or safe_dict.get(k) != v: dumper = SafeStringDumper yamls.append(yaml.dump_all( [{k: v}],