Add no_log to some module arguments #1725

dmsimard · 2021-02-04T19:26:17Z

This will prevent potentially sensitive information from being printed to the console.

See: CVE-2021-20191

This will prevent potentially sensitive information from being printed to the console. See: CVE-2021-20191

ansibullbot · 2021-02-04T19:27:09Z

cc @MatrixCrawler @eikef @getJack @ndclt @nerzhul @stearz
click here for bot help

changelogs/fragments/CVE-2021-20191_no_log.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

* Add no_log to some module arguments This will prevent potentially sensitive information from being printed to the console. See: CVE-2021-20191 * Update changelogs/fragments/CVE-2021-20191_no_log.yml Co-authored-by: Felix Fontein <felix@fontein.de> Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit ae8edc0)

felixfontein · 2021-02-04T20:02:36Z

@dmsimard thanks for fixing this!

* Add no_log to some module arguments This will prevent potentially sensitive information from being printed to the console. See: CVE-2021-20191 * Update changelogs/fragments/CVE-2021-20191_no_log.yml Co-authored-by: Felix Fontein <felix@fontein.de> Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit ae8edc0)

russoz · 2021-02-04T20:41:32Z

@dmsimard I was enticed by this PR to run a grep on the repo, and I spotted:

modules/cloud/misc/ovirt.py:409:            instance_key=dict(type='str', aliases=['key']),
modules/cloud/oneandone/oneandone_firewall_policy.py:502:            auth_token=dict(
modules/cloud/oneandone/oneandone_firewall_policy.py-503-                type='str',
modules/cloud/oneandone/oneandone_firewall_policy.py-504-                default=os.environ.get('ONEANDONE_AUTH_TOKEN')),
modules/cloud/oneandone/oneandone_load_balancer.py:596:            auth_token=dict(
modules/cloud/oneandone/oneandone_load_balancer.py-597-                type='str',
modules/cloud/oneandone/oneandone_load_balancer.py-598-                default=os.environ.get('ONEANDONE_AUTH_TOKEN')),
modules/cloud/oneandone/oneandone_monitoring_policy.py:949:            auth_token=dict(
modules/cloud/oneandone/oneandone_monitoring_policy.py-950-                type='str',
modules/cloud/oneandone/oneandone_monitoring_policy.py-951-                default=os.environ.get('ONEANDONE_AUTH_TOKEN')),
modules/cloud/oneandone/oneandone_private_network.py:386:            auth_token=dict(
modules/cloud/oneandone/oneandone_private_network.py-387-                type='str',
modules/cloud/oneandone/oneandone_private_network.py-388-                default=os.environ.get('ONEANDONE_AUTH_TOKEN')),
modules/cloud/oneandone/oneandone_public_ip.py:276:            auth_token=dict(
modules/cloud/oneandone/oneandone_public_ip.py-277-                type='str',
modules/cloud/oneandone/oneandone_public_ip.py-278-                default=os.environ.get('ONEANDONE_AUTH_TOKEN')),
modules/cloud/oneandone/oneandone_server.py:616:            auth_token=dict(
modules/cloud/oneandone/oneandone_server.py-617-                type='str',
modules/cloud/oneandone/oneandone_server.py-618-                default=os.environ.get('ONEANDONE_AUTH_TOKEN'),
modules/cloud/rackspace/rax_clb_ssl.py:241:        private_key=dict(),
modules/cloud/spotinst/spotinst_aws_elastigroup.py:1462:        multai_token=dict(type='str'),
modules/identity/keycloak/keycloak_client.py:710:        registration_access_token=dict(type='str', aliases=['registrationAccessToken']),
modules/monitoring/librato_annotation.py:151:            api_key=dict(required=True),
modules/monitoring/pagerduty_alert.py:200:            service_key=dict(required=False),
modules/monitoring/pagerduty_alert.py:201:            integration_key=dict(required=False),
modules/monitoring/pagerduty_alert.py:202:            api_key=dict(required=True),
modules/net_tools/nios/nios_nsgroup.py:401:        tsig_key=dict(),
modules/net_tools/nios/nios_nsgroup.py:402:        tsig_key_alg=dict(choices=['HMAC-MD5', 'HMAC-SHA256'], default='HMAC-MD5'),
modules/net_tools/nios/nios_nsgroup.py:403:        tsig_key_name=dict(required=True)
modules/packaging/os/pulp_repo.py:548:        feed_client_key=dict(aliases=['importer_ssl_client_key']),
modules/source_control/gitlab/gitlab_runner.py:312:        registration_token=dict(type='str', required=True),
modules/storage/ibm/ibm_sa_host.py:93:            iscsi_chap_secret=dict()

I have not investigated thoroughly, it was just a grep and I excluded entries with no_log=True, types path and bool and a couple of other obvious cases. Most likely some of these are false-positives. I just wanted to have that list registered somewhere - even if we do not address all of them in this PR, we will be able to refer back to this comment to investigate further.

Cheers

PS:
BTW, it was a:

git grep -n -A3 -E '(_secret|_password|_token|_key)\w*=dict'

from the {root}/plugins directory

* Add no_log to some module arguments This will prevent potentially sensitive information from being printed to the console. See: CVE-2021-20191 * Update changelogs/fragments/CVE-2021-20191_no_log.yml Co-authored-by: Felix Fontein <felix@fontein.de> Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit ae8edc0) Co-authored-by: David Moreau Simard <dmsimard@redhat.com>

felixfontein · 2021-02-04T21:01:42Z

@russoz please create a PR if you have a bit of time :)

Actually we were thinking of extending the sanity tests to test for more possible keys and report errors if no_log is not specified (one has to explicitly set it to False for false positives then). Nobody created a PR yet to actually implement that though. @gundalow didn't you had a commit / branch for that?

dmsimard · 2021-02-04T21:12:16Z

@russoz good find, thanks !

We can and should fix the instances where it's relevant and refer to the CVE like we did for this PR.
I can review the PR if you'd like to take care of it, otherwise let me know.

gundalow · 2021-02-04T21:19:28Z

@russoz hi, I had something roughly working, feel free to create a PR based on this branch https://github.com/gundalow/ansible/tree/ci-no-log

relrod · 2021-02-04T21:31:45Z

@russoz, @dmsimard

I think most of these are valid and need fixes.

Going through these:

modules/cloud/misc/ovirt.py:409:            instance_key=dict(type='str', aliases=['key']),

I'm not sure if this is one (I believe it's used for public keys?), but it did lead me to find instance_rootpw which is one.

modules/cloud/oneandone/oneandone_firewall_policy.py:502:            auth_token=dict(
modules/cloud/oneandone/oneandone_firewall_policy.py-503-                type='str',
modules/cloud/oneandone/oneandone_firewall_policy.py-504-                default=os.environ.get('ONEANDONE_AUTH_TOKEN')),
modules/cloud/oneandone/oneandone_load_balancer.py:596:            auth_token=dict(
modules/cloud/oneandone/oneandone_load_balancer.py-597-                type='str',
modules/cloud/oneandone/oneandone_load_balancer.py-598-                default=os.environ.get('ONEANDONE_AUTH_TOKEN')),
modules/cloud/oneandone/oneandone_monitoring_policy.py:949:            auth_token=dict(
modules/cloud/oneandone/oneandone_monitoring_policy.py-950-                type='str',
modules/cloud/oneandone/oneandone_monitoring_policy.py-951-                default=os.environ.get('ONEANDONE_AUTH_TOKEN')),
modules/cloud/oneandone/oneandone_private_network.py:386:            auth_token=dict(
modules/cloud/oneandone/oneandone_private_network.py-387-                type='str',
modules/cloud/oneandone/oneandone_private_network.py-388-                default=os.environ.get('ONEANDONE_AUTH_TOKEN')),
modules/cloud/oneandone/oneandone_public_ip.py:276:            auth_token=dict(
modules/cloud/oneandone/oneandone_public_ip.py-277-                type='str',
modules/cloud/oneandone/oneandone_public_ip.py-278-                default=os.environ.get('ONEANDONE_AUTH_TOKEN')),

Yep these seem valid.

modules/cloud/oneandone/oneandone_server.py:616:            auth_token=dict(
modules/cloud/oneandone/oneandone_server.py-617-                type='str',
modules/cloud/oneandone/oneandone_server.py-618-                default=os.environ.get('ONEANDONE_AUTH_TOKEN'),

This one already seems to set no_log.

modules/cloud/rackspace/rax_clb_ssl.py:241:        private_key=dict(),
modules/cloud/spotinst/spotinst_aws_elastigroup.py:1462:        multai_token=dict(type='str'),

Seem valid. Also in spotinst_aws_elastigroup, token.

modules/identity/keycloak/keycloak_client.py:710:        registration_access_token=dict(type='str', aliases=['registrationAccessToken']),

Possibly. Reading the keycloak docs, this token is invalidated after it's used once, so it leaking probably isn't a huge deal.

modules/monitoring/librato_annotation.py:151:            api_key=dict(required=True),
modules/monitoring/pagerduty_alert.py:200:            service_key=dict(required=False),
modules/monitoring/pagerduty_alert.py:201:            integration_key=dict(required=False),
modules/monitoring/pagerduty_alert.py:202:            api_key=dict(required=True),

Yep, I think so.

modules/net_tools/nios/nios_nsgroup.py:401:        tsig_key=dict(),
modules/net_tools/nios/nios_nsgroup.py:402:        tsig_key_alg=dict(choices=['HMAC-MD5', 'HMAC-SHA256'], default='HMAC-MD5'),
modules/net_tools/nios/nios_nsgroup.py:403:        tsig_key_name=dict(required=True)

tsig_key for sure. The other two, probably not.

modules/packaging/os/pulp_repo.py:548:        feed_client_key=dict(aliases=['importer_ssl_client_key']),

Yep, since this can be either a path to a key, or the key itself, it should be hidden.

modules/source_control/gitlab/gitlab_runner.py:312:        registration_token=dict(type='str', required=True),
modules/storage/ibm/ibm_sa_host.py:93:            iscsi_chap_secret=dict()

Yep, yep.

felixfontein · 2021-02-04T21:32:23Z

Whoever wants to create the PR for ansible-test should mention it here. If nothing happened until tomorrow, I might start working on that ;)

…al#1725 Signed-off-by: Rick Elrod <rick@elrod.me>

…73488) Change: - A number of modules were missing no_log=True where they should have had it. Test Plan: - Lots of grepping. Tickets: - Refs ansible-collections/community.general#1725 Signed-off-by: Rick Elrod <rick@elrod.me>

…73489) Change: - A number of modules were missing no_log=True where they should have had it. Test Plan: - Lots of grepping. Tickets: - Refs ansible-collections/community.general#1725 Signed-off-by: Rick Elrod <rick@elrod.me>

gundalow · 2021-02-05T06:55:51Z

If you are using grep, member that sometimes the argspec is defined in plugins/module_utils so need to check there as well

felixfontein · 2021-02-05T07:00:39Z

modules/cloud/misc/ovirt.py:409:            instance_key=dict(type='str', aliases=['key']),
I'm not sure if this is one (I believe it's used for public keys?), but it did lead me to find instance_rootpw which is one.

It's passed as authorized_ssh_keys to params.Initialization, so I guess it should be ok.

modules/identity/keycloak/keycloak_client.py:710:        registration_access_token=dict(type='str', aliases=['registrationAccessToken']),
Possibly. Reading the keycloak docs, this token is invalidated after it's used once, so it leaking probably isn't a huge deal.

I guess better safe than sorry :)

modules/monitoring/pagerduty_alert.py:200:            service_key=dict(required=False),
modules/monitoring/pagerduty_alert.py:201:            integration_key=dict(required=False),

I think these are not (the one is an alias for the other, the other is passed on to the API as incident_key). It seems to be an identifier for an incident.

modules/net_tools/nios/nios_nsgroup.py:401:        tsig_key=dict(),
modules/net_tools/nios/nios_nsgroup.py:402:        tsig_key_alg=dict(choices=['HMAC-MD5', 'HMAC-SHA256'], default='HMAC-MD5'),
modules/net_tools/nios/nios_nsgroup.py:403:        tsig_key_name=dict(required=True)

tsig_key for sure. The other two, probably not.

The other two definitely not.

I've created a PR for these: #1736

felixfontein · 2021-02-05T07:06:16Z

@gundalow: @russoz's command also checks module_utils; I looked through the results there, they seem fine - all have no_log explicitly set.

relrod · 2021-02-05T07:08:19Z

I think these are not (the one is an alias for the other, the other is passed on to the API as incident_key). It seems to be an identifier for an incident.

@felixfontein I didn't look in detail, but I googled earlier (as I was writing that comment) and found this page: https://support.pagerduty.com/docs/services-and-integrations which talks about generating service and integration API keys, so I assumed that was for that (in which case they are private api keys). But again I didn't look in much detail, maybe they're just misnomers in the module.

Edit: I did mark them as no_log in the backports, btw, just to be safe.

felixfontein · 2021-02-05T07:11:03Z

@relrod good question, I found the API docs hard to navigate, so I'm not really sure. I've updated my PR to also mark them no_log, just in case :)

relrod · 2021-02-05T07:13:59Z

@felixfontein Also I agree with "better safe than sorry" about the keycloak one. I didn't include it in the backport PRs (which I already merged), but I'm regretting that, and I think I'll fix it.

Even if it can only be used once, if it gets logged/emitted in an error condition, there's a chance that it won't have been used (due to the error). I didn't think that through earlier. 😳

felixfontein · 2021-02-05T17:45:12Z

I've started creating a PR out of @gundalow's branch: ansible/ansible#73508

Add no_log to some module arguments

2234dc1

This will prevent potentially sensitive information from being printed to the console. See: CVE-2021-20191

ansibullbot added affects_2.10 community_review identity module module module_utils module_utils needs_triage plugins plugin (any type) web_infrastructure labels Feb 4, 2021

felixfontein reviewed Feb 4, 2021

View reviewed changes

changelogs/fragments/CVE-2021-20191_no_log.yml Outdated Show resolved Hide resolved

felixfontein added backport-1 labels Feb 4, 2021

ansibullbot removed the needs_triage label Feb 4, 2021

Update changelogs/fragments/CVE-2021-20191_no_log.yml

3d01f26

Co-authored-by: Felix Fontein <felix@fontein.de>

felixfontein approved these changes Feb 4, 2021

View reviewed changes

felixfontein merged commit ae8edc0 into ansible-collections:main Feb 4, 2021

patchback bot mentioned this pull request Feb 4, 2021

[PR #1725/ae8edc02 backport][stable-1] Add no_log to some module arguments #1729

Merged

patchback bot mentioned this pull request Feb 4, 2021

[PR #1725/ae8edc02 backport][stable-2] Add no_log to some module arguments #1730

Merged

relrod added a commit to relrod/ansible that referenced this pull request Feb 4, 2021

Add a bunch of missing ones. Ref: ansible-collections/community.gener…

69ed45b

…al#1725 Signed-off-by: Rick Elrod <rick@elrod.me>

relrod added a commit to relrod/ansible that referenced this pull request Feb 4, 2021

Add a bunch of missing ones. Ref: ansible-collections/community.gener…

75ea5a3

…al#1725 Signed-off-by: Rick Elrod <rick@elrod.me>

felixfontein mentioned this pull request Feb 5, 2021

Fix a bunch of potential security issues (secret leaking) #1736

Merged

felixfontein mentioned this pull request Feb 5, 2021

validate-modules: make sure that options that potentially contain secret data have no_log set ansible/ansible#73508

Merged

This was referenced Feb 8, 2021

[PR #1736/29bd5a94 backport][stable-1] Fix a bunch of potential security issues (secret leaking) #1750

Merged

[PR #1736/29bd5a94 backport][stable-2] Fix a bunch of potential security issues (secret leaking) #1751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add no_log to some module arguments #1725

Add no_log to some module arguments #1725

dmsimard commented Feb 4, 2021

ansibullbot commented Feb 4, 2021

felixfontein commented Feb 4, 2021

russoz commented Feb 4, 2021

felixfontein commented Feb 4, 2021

dmsimard commented Feb 4, 2021

gundalow commented Feb 4, 2021

relrod commented Feb 4, 2021 •

edited

Loading

felixfontein commented Feb 4, 2021

gundalow commented Feb 5, 2021

felixfontein commented Feb 5, 2021

felixfontein commented Feb 5, 2021

relrod commented Feb 5, 2021 •

edited

Loading

felixfontein commented Feb 5, 2021

relrod commented Feb 5, 2021

felixfontein commented Feb 5, 2021

Add no_log to some module arguments #1725

Add no_log to some module arguments #1725

Conversation

dmsimard commented Feb 4, 2021

ansibullbot commented Feb 4, 2021

felixfontein commented Feb 4, 2021

russoz commented Feb 4, 2021

felixfontein commented Feb 4, 2021

dmsimard commented Feb 4, 2021

gundalow commented Feb 4, 2021

relrod commented Feb 4, 2021 • edited Loading

felixfontein commented Feb 4, 2021

gundalow commented Feb 5, 2021

felixfontein commented Feb 5, 2021

felixfontein commented Feb 5, 2021

relrod commented Feb 5, 2021 • edited Loading

felixfontein commented Feb 5, 2021

relrod commented Feb 5, 2021

felixfontein commented Feb 5, 2021

relrod commented Feb 4, 2021 •

edited

Loading

relrod commented Feb 5, 2021 •

edited

Loading