From 4c40b929507431f631bb14e5638f35a247af67b3 Mon Sep 17 00:00:00 2001 From: Clark Boylan Date: Tue, 24 Nov 2020 15:33:56 -0800 Subject: [PATCH] Prevent leaks of buildset registry credentials Because buildset registries may be used by jobs that finish before other jobs are finished using the buildset registry we must be careful not to expose the registry credentials in the jobs that finish sooner. Otherwise logs for the earlier job runs could potentially be used to poison the registry for later jobs. This is likely currently incomplete. Other Zuulians should look over it carefully to ensure we're covering all the bases here. The cases I've identified so far are: * Setting facts that include passwords * Reading and writing to files that include passwords (as content may be logged) * Calling modules with passwords passed as arguments (the module invocation is logged) I've also set no_log on zuul_return that passes up credentials because while the logging for zuul_return is minimal today, I don't want to count on it remaining that way. We also use the yet to be merged secret_data attribute on zuul_return to ensure that zuul_return itself does not expose anything unwanted. Finally it would be great if others could check over the use of buildset_registry variables to make sure there aren't any that got missed. One thing I'm not sure of is whether or not when conditionals get logged and if we need to be careful about their use too. Temporarily remove some buildset-regitry jobs which are in a catch-22. Change-Id: I2dea683e27f00b99a7766bf830981bf91b925265 --- roles/build-container-image/tasks/main.yaml | 7 +- roles/build-docker-image/tasks/main.yaml | 5 +- .../tasks/main.yaml | 9 +- .../vars/main.yaml | 3 + .../tasks/push.yaml | 7 +- roles/run-buildset-registry/tasks/main.yaml | 8 +- roles/use-buildset-registry/tasks/main.yaml | 2 + .../tasks/user-config.yaml | 3 + .../registry/buildset-registry.yaml | 6 +- test-playbooks/registry/test-registry.yaml | 5 +- zuul-tests.d/container-roles-jobs.yaml | 117 ------------------ 11 files changed, 40 insertions(+), 132 deletions(-) create mode 100644 roles/pull-from-intermediate-registry/vars/main.yaml diff --git a/roles/build-container-image/tasks/main.yaml b/roles/build-container-image/tasks/main.yaml index 91cc8faec..b1ac04f30 100644 --- a/roles/build-container-image/tasks/main.yaml +++ b/roles/build-container-image/tasks/main.yaml @@ -1,18 +1,19 @@ - name: Check for results.json stat: - path: "{{ zuul.executor.work_root }}/results.json" + path: "{{ zuul.executor.result_data_file }}" register: result_json_stat delegate_to: localhost # This can be removed if we add this functionality to Zuul directly - name: Load information from zuul_return set_fact: - buildset_registry: "{{ (lookup('file', zuul.executor.work_root + '/results.json') | from_json)['buildset_registry'] }}" + buildset_registry: "{{ (lookup('file', zuul.executor.result_data_file) | from_json)['secret_data']['buildset_registry'] }}" when: - buildset_registry is not defined - result_json_stat.stat.exists - result_json_stat.stat.size > 0 - - "'buildset_registry' in (lookup('file', zuul.executor.work_root + '/results.json') | from_json)" + - "'buildset_registry' in (lookup('file', zuul.executor.result_data_file) | from_json).get('secret_data')" + no_log: true - name: Build container images include_tasks: build.yaml diff --git a/roles/build-docker-image/tasks/main.yaml b/roles/build-docker-image/tasks/main.yaml index 2ac708e45..b51801cc0 100644 --- a/roles/build-docker-image/tasks/main.yaml +++ b/roles/build-docker-image/tasks/main.yaml @@ -7,12 +7,13 @@ # This can be removed if we add this functionality to Zuul directly - name: Load information from zuul_return set_fact: - buildset_registry: "{{ (lookup('file', zuul.executor.work_root + '/results.json') | from_json)['buildset_registry'] }}" + buildset_registry: "{{ (lookup('file', zuul.executor.result_data_file) | from_json)['secret_data']['buildset_registry'] }}" when: - buildset_registry is not defined - result_json_stat.stat.exists - result_json_stat.stat.size > 0 - - "'buildset_registry' in (lookup('file', zuul.executor.work_root + '/results.json') | from_json)" + - "'buildset_registry' in (lookup('file', zuul.executor.result_data_file) | from_json).get('secret_data')" + no_log: true # Docker doesn't understand docker push [1234:5678::]:5000/image/path:tag # so we set up /etc/hosts with a registry alias name to support ipv6 and 4. diff --git a/roles/pull-from-intermediate-registry/tasks/main.yaml b/roles/pull-from-intermediate-registry/tasks/main.yaml index c6daf8890..90fadaca8 100644 --- a/roles/pull-from-intermediate-registry/tasks/main.yaml +++ b/roles/pull-from-intermediate-registry/tasks/main.yaml @@ -2,7 +2,8 @@ - name: Load information from zuul_return when: buildset_registry is not defined set_fact: - buildset_registry: "{{ (lookup('file', zuul.executor.work_root + '/results.json') | from_json)['buildset_registry'] }}" + buildset_registry: "{{ (lookup('file', zuul.executor.result_data_file) | from_json)['secret_data']['buildset_registry'] }}" + no_log: true # Start a socat tunnel to the buildset registry to work around the # fact that docker does not correctly parse ipv6 addresses. The socat @@ -47,10 +48,12 @@ slurp: path: "~/.docker/config.json" register: docker_config + no_log: true - name: Parse docker user configuration when: docker_config_stat.stat.exists set_fact: docker_config: "{{ docker_config.content | b64decode | from_json }}" + no_log: true - name: Set default docker user configuration when: not docker_config_stat.stat.exists set_fact: @@ -74,6 +77,7 @@ content: "{{ new_docker_config | to_nice_json }}" dest: "~/.docker/config.json" mode: 0600 + no_log: true # Pull the images @@ -92,7 +96,7 @@ register: result until: result is success when: "'metadata' in zj_zuul_artifact and zj_zuul_artifact.metadata.type | default('') == 'container_image'" - loop: "{{ zuul.artifacts | default([]) }}" + loop: "{{ zuul_artifacts | default([]) }}" loop_control: loop_var: zj_zuul_artifact always: @@ -103,3 +107,4 @@ content: "{{ docker_config | to_nice_json }}" dest: "~/.docker/config.json" mode: 0600 + no_log: true diff --git a/roles/pull-from-intermediate-registry/vars/main.yaml b/roles/pull-from-intermediate-registry/vars/main.yaml new file mode 100644 index 000000000..9e9c6f45f --- /dev/null +++ b/roles/pull-from-intermediate-registry/vars/main.yaml @@ -0,0 +1,3 @@ +# The tests override this variable, which is why we don't use +# zuul.artifacts directly. +zuul_artifacts: "{{ zuul.artifacts }}" diff --git a/roles/push-to-intermediate-registry/tasks/push.yaml b/roles/push-to-intermediate-registry/tasks/push.yaml index 26b124e8a..347f99d38 100644 --- a/roles/push-to-intermediate-registry/tasks/push.yaml +++ b/roles/push-to-intermediate-registry/tasks/push.yaml @@ -2,7 +2,8 @@ - name: Load information from zuul_return when: buildset_registry is not defined set_fact: - buildset_registry: "{{ (lookup('file', zuul.executor.work_root + '/results.json') | from_json)['buildset_registry'] }}" + buildset_registry: "{{ (lookup('file', zuul.executor.result_data_file) | from_json)['secret_data']['buildset_registry'] }}" + no_log: true # Start a socat tunnel to the buildset registry to work around the # fact that docker does not correctly parse ipv6 addresses. The socat @@ -47,10 +48,12 @@ slurp: path: "~/.docker/config.json" register: docker_config + no_log: true - name: Parse docker user configuration when: docker_config_stat.stat.exists set_fact: docker_config: "{{ docker_config.content | b64decode | from_json }}" + no_log: true - name: Set default docker user configuration when: not docker_config_stat.stat.exists set_fact: @@ -74,6 +77,7 @@ content: "{{ new_docker_config | to_nice_json }}" dest: "~/.docker/config.json" mode: 0600 + no_log: true # Push the images - name: Push images to intermediate registry @@ -91,3 +95,4 @@ content: "{{ docker_config | to_nice_json }}" dest: "~/.docker/config.json" mode: 0600 + no_log: true diff --git a/roles/run-buildset-registry/tasks/main.yaml b/roles/run-buildset-registry/tasks/main.yaml index af7aab5ea..d80e98489 100644 --- a/roles/run-buildset-registry/tasks/main.yaml +++ b/roles/run-buildset-registry/tasks/main.yaml @@ -31,6 +31,7 @@ set_fact: registry_password: "{{ lookup('password', '/dev/null') }}" registry_secret: "{{ lookup('password', '/dev/null') }}" + no_log: true - name: Write registry config template: @@ -74,8 +75,13 @@ username: zuul password: "{{ registry_password }}" cert: "{{ certificate }}" + no_log: true - name: Return registry information to Zuul zuul_return: - data: + secret_data: buildset_registry: "{{ buildset_registry }}" + # This isn't strictly necessary with the current implemenation of + # zuul_return but we set no_log: true in case the verbosity + # changes. + no_log: true diff --git a/roles/use-buildset-registry/tasks/main.yaml b/roles/use-buildset-registry/tasks/main.yaml index 8c1a64580..bf2bebf36 100644 --- a/roles/use-buildset-registry/tasks/main.yaml +++ b/roles/use-buildset-registry/tasks/main.yaml @@ -100,6 +100,7 @@ buildset_registry: "{{ buildset_registry }}" buildset_registry_alias: "{{ buildset_registry_alias }}" namespaces: "{{ buildset_registry_namespaces }}" + no_log: true - name: Ensure buildkit directory exists become: yes @@ -114,6 +115,7 @@ buildset_registry: "{{ buildset_registry }}" buildset_registry_alias: "{{ buildset_registry_alias }}" namespaces: "{{ buildset_registry_namespaces }}" + no_log: true # We use 'block' here to cause the become to apply to all the tasks # (which does not automatically happen with include_tasks). diff --git a/roles/use-buildset-registry/tasks/user-config.yaml b/roles/use-buildset-registry/tasks/user-config.yaml index e133503ea..d0a8ace31 100644 --- a/roles/use-buildset-registry/tasks/user-config.yaml +++ b/roles/use-buildset-registry/tasks/user-config.yaml @@ -32,11 +32,13 @@ } set_fact: docker_config: "{{ docker_config | combine(new_config, recursive=True) }}" + no_log: true - name: Save docker user configuration copy: content: "{{ docker_config | to_nice_json }}" dest: "~/.docker/config.json" mode: 0600 + no_log: true # The next two tasks are for supporting the "containers" tools (ie, # not docker); this directory doesn't exist on Xenial. - name: Check if /run/user exists @@ -49,6 +51,7 @@ content: "{{ docker_config | to_nice_json }}" dest: "/run/user/{{ ansible_user_uid }}/auth.json" mode: 0600 + no_log: true # The next two tasks are for supporting k8s - name: Check if /var/lib/kubelet exists stat: diff --git a/test-playbooks/registry/buildset-registry.yaml b/test-playbooks/registry/buildset-registry.yaml index 34baf50b2..e82b25ef8 100644 --- a/test-playbooks/registry/buildset-registry.yaml +++ b/test-playbooks/registry/buildset-registry.yaml @@ -4,7 +4,8 @@ tasks: - name: Load real buildset registry connection info set_fact: - real_buildset_registry: "{{ (lookup('file', zuul.executor.work_root + '/results.json') | from_json)['buildset_registry'] }}" + real_buildset_registry: "{{ (lookup('file', zuul.executor.result_data_file) | from_json)['secret_data']['buildset_registry'] }}" + no_log: true # This should now use the speculative image, because we've already # run use-buildset-registry. - name: Run the fake buildset registry @@ -16,7 +17,8 @@ # Leave that zuul return so that dependent jobs use the fake one - name: Load fake buildset registry connection info set_fact: - fake_buildset_registry: "{{ (lookup('file', zuul.executor.work_root + '/results.json') | from_json)['buildset_registry'] }}" + fake_buildset_registry: "{{ (lookup('file', zuul.executor.result_data_file) | from_json)['secret_data']['buildset_registry'] }}" + no_log: true - name: Build a test image command: "docker build . -t zuul/testimage:latest" args: diff --git a/test-playbooks/registry/test-registry.yaml b/test-playbooks/registry/test-registry.yaml index 0f1473c55..e678e40e3 100644 --- a/test-playbooks/registry/test-registry.yaml +++ b/test-playbooks/registry/test-registry.yaml @@ -92,14 +92,11 @@ include_vars: vars/intermediate-registry-auth.yaml - name: Include previous build vars include_vars: vars/previous-build.yaml - - name: Prepare a replacement zuul variable - set_fact: - test_zuul: "{{ previous_build_zuul }}" - name: Run pull-from-intermediate-registry role include_role: name: pull-from-intermediate-registry vars: - zuul: "{{ test_zuul }}" + zuul_artifacts: "{{ previous_build_zuul.artifacts }}" # This simulates a build actually using the previous build. diff --git a/zuul-tests.d/container-roles-jobs.yaml b/zuul-tests.d/container-roles-jobs.yaml index fffba2dd3..53f10d52c 100644 --- a/zuul-tests.d/container-roles-jobs.yaml +++ b/zuul-tests.d/container-roles-jobs.yaml @@ -211,115 +211,6 @@ - name: builder label: ubuntu-bionic -- job: - name: zuul-jobs-test-registry-buildset-registry - parent: opendev-buildset-registry - description: | - Run a buildset registry for the test-registry jobs - - This runs two registries: a real buildset registry so that we - can receive speculative zuul-registry images, and a fake - buildset registry (running the speculative or latest - zuul-registry) that is used to test using the buildset registry - role. - - It is not meant to be used directly but rather run on changes - to roles in the zuul-jobs repo. - files: - - roles/pull-from-intermediate-registry/.* - - roles/push-to-intermediate-registry/.* - - roles/ensure-docker/.* - - roles/ensure-kubernetes/.* - - roles/ensure-openshift/.* - - roles/ensure-package-repositories/.* - - roles/build-docker-image/.* - - roles/run-buildset-registry/.* - - roles/use-buildset-registry/.* - - test-playbooks/registry/.* - pre-run: test-playbooks/registry/buildset-registry-pre.yaml - run: test-playbooks/registry/buildset-registry.yaml - post-run: test-playbooks/registry/test-registry-post.yaml - vars: - container_command: docker - -- job: - name: zuul-jobs-test-registry-buildset-registry-k8s-docker - dependencies: zuul-jobs-test-registry-buildset-registry - description: | - Test a buildset registry with kubernetes and docker - - It is not meant to be used directly but rather run on changes - to roles in the zuul-jobs repo. - files: - - roles/pull-from-intermediate-registry/.* - - roles/push-to-intermediate-registry/.* - - roles/ensure-docker/.* - - roles/ensure-kubernetes/.* - - roles/ensure-package-repositories/.* - - roles/build-docker-image/.* - - roles/run-buildset-registry/.* - - roles/use-buildset-registry/.* - - test-playbooks/registry/.* - run: test-playbooks/registry/buildset-registry-k8s-docker.yaml - post-run: - - test-playbooks/registry/buildset-registry-k8s-docker-post.yaml - - test-playbooks/registry/test-registry-post.yaml - vars: - container_command: docker - -- job: - name: zuul-jobs-test-registry-buildset-registry-k8s-crio - dependencies: zuul-jobs-test-registry-buildset-registry - description: | - Test a buildset registry with kubernetes and CRIO - - It is not meant to be used directly but rather run on changes - to roles in the zuul-jobs repo. - files: - - roles/pull-from-intermediate-registry/.* - - roles/push-to-intermediate-registry/.* - - roles/ensure-docker/.* - - roles/ensure-kubernetes/.* - - roles/ensure-package-repositories/.* - - roles/build-docker-image/.* - - roles/run-buildset-registry/.* - - roles/use-buildset-registry/.* - - test-playbooks/registry/.* - run: test-playbooks/registry/buildset-registry-k8s-crio.yaml - post-run: - - test-playbooks/registry/buildset-registry-k8s-crio-post.yaml - - test-playbooks/registry/test-registry-post.yaml - vars: - container_command: podman - -- job: - name: zuul-jobs-test-registry-buildset-registry-openshift-docker - dependencies: zuul-jobs-test-registry-buildset-registry - description: | - Test a buildset registry with openshift and docker - - It is not meant to be used directly but rather run on changes - to roles in the zuul-jobs repo. - files: - - roles/pull-from-intermediate-registry/.* - - roles/push-to-intermediate-registry/.* - - roles/ensure-docker/.* - - roles/ensure-openshift/.* - - roles/ensure-package-repositories/.* - - roles/build-docker-image/.* - - roles/run-buildset-registry/.* - - roles/use-buildset-registry/.* - - test-playbooks/registry/.* - run: test-playbooks/registry/buildset-registry-openshift-docker.yaml - post-run: - - test-playbooks/registry/test-registry-post.yaml - vars: - container_command: docker - nodeset: - nodes: - - name: controller - label: centos-7 - - job: name: zuul-jobs-test-ensure-kubernetes-docker description: | @@ -467,10 +358,6 @@ - zuul-jobs-test-registry-docker - zuul-jobs-test-registry-docker-multiarch - zuul-jobs-test-registry-podman - - zuul-jobs-test-registry-buildset-registry - - zuul-jobs-test-registry-buildset-registry-k8s-docker - - zuul-jobs-test-registry-buildset-registry-k8s-crio - - zuul-jobs-test-registry-buildset-registry-openshift-docker - zuul-jobs-test-ensure-kubernetes-docker - zuul-jobs-test-ensure-kubernetes-crio - zuul-jobs-test-ensure-podman-centos-8 @@ -493,10 +380,6 @@ - zuul-jobs-test-registry-docker - zuul-jobs-test-registry-docker-multiarch - zuul-jobs-test-registry-podman - - zuul-jobs-test-registry-buildset-registry - - zuul-jobs-test-registry-buildset-registry-k8s-docker - - zuul-jobs-test-registry-buildset-registry-k8s-crio - - zuul-jobs-test-registry-buildset-registry-openshift-docker - zuul-jobs-test-ensure-kubernetes-docker - zuul-jobs-test-ensure-kubernetes-crio - zuul-jobs-test-ensure-podman-centos-8