Arvados

22945-ansible-secrets-nolog at 29199818

• All agreed upon points are implemented / addressed. Describe changes from pre-implementation design.
  • Yes. Added no_log: yes in every task that touches contents with secrets.
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • No
• Code is tested and passing, both automated and manual, what manual testing was done is described.
  • Even though I've manually ran the single node playbook against a local VM with -vvvvv (the max verbosity level), I couldn't see passwords or tokens being leaked, so I have no clear way of validating that my changes are enough. OTOH, I've confirmed that no_log: yes produces the desired effect:
    

    TASK [arvados_service : Write Arvados config.yml] **********************************************************************************
    ...[SSH related output]...
    ok: [192.168.100.158] => {
        "censored": "the output has been hidden due to the fact that 'no_log: true' was specified for this result",
        "changed": false
    }
    

• New or changed UI/UX has gotten feedback from stakeholders.
  • N/A
• Documentation has been updated.
  • N/A
• Behaves appropriately at the intended scale (describe intended scale).
  • No changes in scale
• Considered backwards and forwards compatibility issues between client and server.
  • No issues here
• Follows our coding standards and GUI style guidelines.
  • N/A

Lucas Di Pentima wrote in #note-6:

• Even though I've manually ran the single node playbook against a local VM with -vvvvv (the max verbosity level), I couldn't see passwords or tokens being leaked, so I have no clear way of validating that my changes are enough.

So I was going by this FAQ (which I should've linked in the description, sorry) which is admittedly vague on exactly when this is a concern. The things that are for sure a concern are commands and their stdout and stderr. I believe I have also seen some file manipulation tasks like lineinfile record old and new versions. But even exactly what those are, or beyond that, 🤷

Personally I'd rather err on the side of caution, especially considering these are public playbooks where we're not totally sure how people are going to run them. I'm open to revisiting that if it gets to the point where this makes playbooks difficult to debug. But considering most of these are very straightforward facts loading/setting tasks, I'm not concerned. For example, when you wrote:

1. Not sure if there's a risk of leaking a secret in a diff output here

I don't think that's a concern when setting the fact here. I am concerned about for the task that actually uploads the file (which you also marked), but in here the only thing we look at is arvados_config_copy.changed, which is just a boolean and definitely does not contain the diff. But, again, because the task is so simple, I'm also not worried about not logging it.

All this to say, everything LGTM, thanks.

For the record, I did notice this ticket happened in parallel with #23027, but I double-checked there's no sensitive tasks in there, so there's nothing to worry about falling through the cracks.