Skip to content

feat: add option to mask potential secrents in env vars #1418

Closed
kokosnuss wants to merge 15 commits intoansible:develfrom
kokosnuss:devel
Closed

feat: add option to mask potential secrents in env vars #1418
kokosnuss wants to merge 15 commits intoansible:develfrom
kokosnuss:devel

Conversation

@kokosnuss
Copy link

Ansible-Runner is used within AWX to run ansible-playbooks in transmit and worker mode.
Since AWX passes the contents (user + password) of some credential types (e.g. VMware vcenter) to the playbook as environment variables, these environment variables are written to stdout in streaming.py:
https://github.com/ansible/ansible-runner/blob/devel/src/ansible_runner/streaming.py#L228
self._output.write(json.dumps(printed_status_data).encode('utf-8'))

In environments such as Red Hat Openshift where global log forwarding to a log streaming server is enabled for all pods and their stdout, this immediately leads to the logging of sensitive data into log management.
This PR offers the possibility to suppress the logging of all environment variables by ansible-runner via CLI flag (--suppress-env-print) or environment variable (SUPPRESS_ENV_PRINT).

Due to the fact that awx uses a hard-coded CLI-call for the ansible-runner, it's important to allow log-suppressipon not only via CLI flag but also via environment variable. Otherwise, it would not be configurable:
https://github.com/ansible/awx/blob/devel/awx/main/tasks/receptor.py#L646
The suppress option can here be achieved via setting the environmet variable SUPPRESS_ENV_PRINT=True using the pod specification for the ansible-runner.

since worker is used within AWX, awx credentials are sometimes passed as ENV vars to the ansible-runner. therefore they should not be printed on stdout since they could contain sensitive informations
since worker is used within AWX, awx credentials are sometimes passed as ENV vars to the ansible-runner. therefore they should not be printed on stdout since they could contain sensitive informations
@kokosnuss kokosnuss requested a review from a team as a code owner February 24, 2025 09:59
@Shrews
Copy link
Contributor

Shrews commented Feb 24, 2025

Thanks for your PR.

On changes like this, we have to ask someone from the Controller/AWX team to test the change to validate that it won't adversely affect that system (ping @AlanCoding or @john-westcott-iv). It might take some time for this change to be tested, so until that feedback can be given, I'd suggest investing some time in adding some unit/integration tests to this PR as we will not merge a PR that is not testing its own changes.

Also, I'd like to point out that the AWX UI provides a way to use encrypted VMWare credentials (see https://ansible.readthedocs.io/projects/awx/en/24.6.1/userguide/credentials.html#vmware-vcenter), although I'm not personally able to guide you in that aspect. This is likely to be a more secure option for you than using unencrypted environment variables.

@kokosnuss
Copy link
Author

kokosnuss commented Feb 25, 2025

On changes like this, we have to ask someone from the Controller/AWX team to test the change to validate that it won't adversely affect that system (ping @AlanCoding or @john-westcott-iv). It might take some time for this change to be tested, so until that feedback can be given, I'd suggest investing some time in adding some unit/integration tests to this PR as we will not merge a PR that is not testing its own changes.

Yes sure, we will provide some unit/integration tests.

Also, I'd like to point out that the AWX UI provides a way to use encrypted VMWare credentials (see https://ansible.readthedocs.io/projects/awx/en/24.6.1/userguide/credentials.html#vmware-vcenter), although I'm not personally able to guide you in that aspect. This is likely to be a more secure option for you than using unencrypted environment variables.

That is exactly the way we are working. Unfortunately AWX passes the encrypted VMWare credentials ans env vars instead of ansible extra_vars to the play.

@Klaas-
Copy link

Klaas- commented Feb 26, 2025

This sounds like a security issue :)

@kokosnuss
Copy link
Author

@Shrews thanks for starting the action workflow.
We've now added a integration tests.

Co-authored-by: Abhijeet Kasurde <akasurde@redhat.com>
@kokosnuss kokosnuss marked this pull request as draft March 3, 2025 08:41
@Shrews
Copy link
Contributor

Shrews commented Mar 4, 2025

Random out loud thought... might be worth considering removing env at this point in the code so that suppressing will work even outside of the streaming protocol.

Also, unclear to me at this point that ever printing out env vars at the above point is going to be useful.

@kokosnuss kokosnuss changed the title feat: add option to suppress print of env vars in worker mode feat: add option to mask potential secrents in env vars Mar 4, 2025
@kokosnuss
Copy link
Author

Random out loud thought... might be worth considering removing env at this point in the code so that suppressing will work even outside of the streaming protocol.

Also, unclear to me at this point that ever printing out env vars at the above point is going to be useful.

Thanks for that not so random thought. Initially i had the same idea but there are components in AWX/tower which are requiring that ansible-runner prints at least one key-value pair in env since awx is iterating through the env vars at some point.

Anyways, we discovered that a project sync in awx did not work anymore after we included this fix in our execution environment:
2025-02-28 17:32:33,562 ERROR [54203f1c4a454517afe9a8c15ab62e10] awx.main.tasks.jobs inventory_update 55180 (running) Post run hook errored. Traceback (most recent call last): File "/var/lib/awx/venv/awx/lib64/python3.11/site-packages/awx/main/tasks/jobs.py", line 637, in run self.post_run_hook(self.instance, status) File "/var/lib/awx/venv/awx/lib64/python3.11/site-packages/awx/main/tasks/jobs.py", line 1645, in post_run_hook private_data_dir = inventory_update.job_env['AWX_PRIVATE_DATA_DIR'] ~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^ KeyError: 'AWX_PRIVATE_DATA_DIR'
Turns out that awx is reading the ansible_private_data_dir from the env key printed from ansible-runner:
https://github.com/ansible/awx/blob/7b8b37d9a8e46b3cda101f3c5691c870b9de46da/awx/main/tasks/jobs.py#L1664
That's why we have rewritten the PR into a “mask-secrets-in-env”. We copied the code for this from awx itself.

@kokosnuss kokosnuss marked this pull request as ready for review March 4, 2025 19:52
@Shrews
Copy link
Contributor

Shrews commented Mar 24, 2025

Thanks for the PR. It's a little problematic (through no fault of your own) in that it suffers from a problem we've been trying to address within runner when introducing a new interface.run() job argument, as your PR does. Within streaming mode, if the transmit runner is not the same version as the worker runner, the worker could crash when it receives a job argument that it does not understand. We currently have a proposed fix up that is awaiting testing from the Controller team. I think we should reconsider this fix after that big fix merges.

@gannaramu
Copy link

+1 on this issue. need a way to suppress env variables to stdout. leaking secrets to upstream logging.

@AlanCoding
Copy link
Member

At first glance I don't think this should break anything with AWX. You made the changes to Worker which reports the env vars in a callback. All that AWX does with that is used it to populate job_env. This is not fed into anything else in the server logic itself, merely presented in the API details of the job.

You're supporting it as a ansible-runner setting, which is how support for it would be enabled for AWX. There is some additional sanitization/hiding on the AWX layer. After all, it is what provides the env vars to begin with, so it already tracks what it knows should be hidden. The only value I could see for AWX would be that the system or podman configuration adds additional secrets that need to be hidden.

kokosnuss and others added 2 commits May 21, 2025 19:51
Co-authored-by: Ram Rohit Gannavarapu <ganna.ramu@gmail.com>
@sivel
Copy link
Member

sivel commented Sep 25, 2025

I meant to come back to this issue a while ago, and I apologize for that. However, the end result of the discussion we have had are that we have no intentions of implementing or accepting such a feature at this time.

Our recommendation is to implement security restrictions within the logging platform to prevent access to the sensitive data for anyone who does not otherwise have access to view the info.

Ultimately, the only way we could approach the needs of the platform, and prevent this from happening would be to implement a new non-stdout mechanism for communication between the execution node and ansible-runner. However using stdout has been determined to give us the highest level of reliability for communication, due to reliability implemented via k8s and other container platforms in ensuring stdout is not lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

Comments