Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add loop control for options list task in otelwinreg #5132

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alanbty
Copy link

@alanbty alanbty commented Jul 23, 2024

Description:
Fixing an issue - Add a loop control for loop task "Get Splunk OpenTelemetry Collector options list" in otel_win_reg.yml.
This fix prevents a potentially sensitive value from being displayed.

Testing:

TASK [signalfx.splunk_otel_collector.collector : Get Splunk OpenTelemetry Collector options list]
**********************************************************************************************************************************
skipping: [hostA] => (item=KEYOFMYSECRETVALUE) 
skipping: [hostA]

@alanbty alanbty requested review from a team as code owners July 23, 2024 15:25
Copy link
Contributor

github-actions bot commented Jul 23, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@alanbty
Copy link
Author

alanbty commented Jul 23, 2024

I have read the CLA Document and I hereby sign the CLA

srv-gh-o11y-gdi-cla added a commit to splunk/cla-agreement that referenced this pull request Jul 23, 2024
@alanbty alanbty force-pushed the ansible_fix_loop_otelwinreg branch 2 times, most recently from 6f424de to c285092 Compare July 29, 2024 15:12
hughesjj
hughesjj previously approved these changes Jul 29, 2024
@@ -6,6 +6,8 @@
{{ (splunk_otel_collector_options_list | default([])) + [item.key + '=' + (value | string)] }}
loop: >
{{ splunk_otel_collector_options | default({}) | combine(splunk_otel_collector_additional_env_vars) | dict2items }}
loop_control:
label: "{{ item.key }}"
Copy link
Contributor

@hughesjj hughesjj Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note in your example you have item=KEYOFMYSECRETVALUE, so wanted to double check that we indeed want item.key.

In general I'm for adding the loop control + label

Copy link
Contributor

@hughesjj hughesjj Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relevant parts of code for other reviewers:

  • ansible docs
  • splunk_otel_collector_options
    •   SPLUNK_ACCESS_TOKEN: "{{ splunk_access_token }}"
        SPLUNK_API_URL: "{{ splunk_api_url }}"
        GOMEMLIMIT: "{{ gomemlimit if gomemlimit != '' else omit }}"
        SPLUNK_BUNDLE_DIR: >-
          {{ splunk_bundle_dir if splunk_bundle_dir != '' else
          '{{ansible_env.ProgramFiles}}\Splunk\OpenTelemetry Collector\agent-bundle' }}
        SPLUNK_COLLECTD_DIR: "{{ splunk_collectd_dir if splunk_collectd_dir != '' else omit }}"
        SPLUNK_CONFIG: >-
          {{ splunk_otel_collector_config if splunk_otel_collector_config != '' else
          '{{ ansible_env.ProgramData }}\Splunk\OpenTelemetry Collector\agent_config.yaml' }}
        SPLUNK_INGEST_URL: "{{ splunk_ingest_url }}"
        SPLUNK_HEC_TOKEN: "{{ splunk_hec_token }}"
        SPLUNK_HEC_URL: "{{ splunk_hec_url }}"
        SPLUNK_LISTEN_INTERFACE: "{{ splunk_listen_interface if splunk_listen_interface != '' else omit }}"
        SPLUNK_MEMORY_TOTAL_MIB: "{{ splunk_memory_total_mib }}"
        SPLUNK_REALM: "{{ splunk_realm }}"
        SPLUNK_TRACE_URL: "{{ splunk_trace_url }}"
      
  • splunk_otel_collector_additional_env_vars (looks empty from defaults)

@alanbty alanbty force-pushed the ansible_fix_loop_otelwinreg branch from 4a41f77 to 5c3a8e9 Compare July 30, 2024 06:57
@jeffreyc-splunk
Copy link
Contributor

I'm not too familiar, but the output from our CI tests still shows the values:

TASK [signalfx.splunk_otel_collector.collector : Get Splunk OpenTelemetry Collector options list] ***
ok: [2016] => (item=SPLUNK_ACCESS_TOKEN) => {"ansible_facts": {"splunk_otel_collector_options_list": ["SPLUNK_ACCESS_TOKEN=fake-token"]}, "ansible_loop_var": "item", "changed": false, "item": {"key": "SPLUNK_ACCESS_TOKEN", "value": "fake-token"}}
...

According to the note for label:

This is for making console output more readable, not protecting sensitive data. If there is sensitive data in `loop`, set `no_log: true` on the task to prevent disclosure.

So if the goal is hide the sensitive data, then maybe we should try no_log: true instead of loop_control?

@alanbty
Copy link
Author

alanbty commented Aug 12, 2024

I'm not too familiar, but the output from our CI tests still shows the values:

TASK [signalfx.splunk_otel_collector.collector : Get Splunk OpenTelemetry Collector options list] ***
ok: [2016] => (item=SPLUNK_ACCESS_TOKEN) => {"ansible_facts": {"splunk_otel_collector_options_list": ["SPLUNK_ACCESS_TOKEN=fake-token"]}, "ansible_loop_var": "item", "changed": false, "item": {"key": "SPLUNK_ACCESS_TOKEN", "value": "fake-token"}}
...

According to the note for label:

This is for making console output more readable, not protecting sensitive data. If there is sensitive data in `loop`, set `no_log: true` on the task to prevent disclosure.

So if the goal is hide the sensitive data, then maybe we should try no_log: true instead of loop_control?

Hello @jeffreyc-splunk,

Sorry for the late response.

I think your tests have the Ansible verbosity parameter enabled, that's why the pipeline shows values.

You're right, no log: true is the best solution to hide sensitive values. But it also remove error messages if there is an issue on this task.
So, as you wish, either you want to avoid displaying sensitive values and use no log: true, or you use loop_control and allow sensitive values to be displayed ONLY with Ansible verbose parameter.

@hughesjj
Copy link
Contributor

Let's go with no log: true for now, and if needed we can modify it or come back in the future to properly redact the token with loop_control.

Alternatively, if you want to modify loop_control to not log the credential (regardless of verbosity, maybe truncated on verbose or just log <REDACTED> if it's non-empty?)

@hughesjj hughesjj dismissed their stale review October 10, 2024 21:46

credential log issue

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.

4 participants