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

Properly escape quotes and other shell-chars #27

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

Conversation

pieterlexis
Copy link
Contributor

Hi,

I had some pwgen generated passwords that contained backticks and/or quote symbols, this made the cron jobs trip up. Ansible has the quote filter that translates strings to properly shelll-safe quoted strings. Using this filter in the patch, all if fine again!

@donat-b
Copy link
Owner

donat-b commented Feb 28, 2018

Is it going to be safe if someone had escaped passwords already?

@pieterlexis
Copy link
Contributor Author

Is it going to be safe if someone had escaped passwords already?

The honest answer is "I don't know" and I assume that folks that have already escaped (and have the password in the cron-file, not in a password file as this role did before) will indeed need to un-escape this in their configuration.

@paulfantom
Copy link
Contributor

I ran some tests to check how quote filter works and here are the results

Test data:
playbook: site.yml

---
- hosts: localhost
  become: no
  connection: local
  tasks:
  - template:
      src: test.j2
      dest: "{{ ansible_user_dir }}/testing_quotes"
  vars:
    val1: "tes\\ting"
    val2: "tes\ting"
    val3: tes\ting

template: test.j2

{{ val1 | quote }}
{{ val2 | quote }}
{{ val3 | quote }}

Output:

'tes\ting'
'tes    ing'
'tes\ting'

@pieterlexis
Copy link
Contributor Author

ok, in my testing this worked, copying the lines from the crontab file and running restic, but from cron it does not seem to work. So I have an alternative proposition:

Allow setting use-password-file in the restic_repos items, that writes out the password as-is to /var/lib/restic/{{ item.name }} and sets the RESTIC_PASSWORD_FILE environment variable in the crontab

@paulfantom
Copy link
Contributor

Why do you want to re-add functionality which was removed on purpose (in #18)?
Could you tell why setting password in cron file didn't work in you setup? Was it some kind of characters, if yes then which ones are causing problems?

@donat-b
Copy link
Owner

donat-b commented Mar 1, 2018

It's pretty obvious if you are going to generate passwords from whole ASCII range it's likely to break cronfiles. And I think it's a valid concern.
I'm not against merging this request, I'm just thinking how to make it clear that we added something that might break existing setups.

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.

3 participants