tobias-richter / ansible-tasmota

Ansible Role for managing tasmota devices with tasmota commands
Apache License 2.0
31 stars 12 forks source link

Password incorrectly parsed when coming from Ansible Vault #23

Closed nununo closed 3 years ago

nununo commented 3 years ago

Hi,

Before anything else, thank you for this role. It's great.

When the password is defined in an Ansible Vault, which is my case, the password in the status_params dictionary is of type ansible.parsing.yaml.objects.AnsibleVaultEncryptedUnicode. As a consequence, it is incorrectly processed by the requests.get() method: if password is 123, the resulting uri is: cm/?user=theuser&password=1&password=2&password=3.

The simplest solution is to use str() when building the status_params dictionary. For extra safety I would apply this to all the values (because the username and the command can also be defined in the Vault).

I will submit a PR with a proposal to solve this issue.

Regards, Nuno

tobias-richter commented 3 years ago

@nununo thank you for the honors and thank you even more for your contribution!

I have just tested your issue and I am not able to reproduce.

I added password authentication to one of my tasmota deviced and used

tasmota_user: admin
tasmota_password: "{{ vault_tasmota_password }}"

with an value of "admin" for "{{ vault_tasmota_password }}".

I am not getting the error you are getting. I have used Ansible 2.9.17 in my case. I am also wondering why you have an AnsibleVaultEncryptedUnicode there because we are using templar before: https://github.com/tobias-richter/ansible-tasmota/blob/master/action_plugins/tasmota.py#L244

This should already simply return string/values because it "templates" all values.

One more thing I am wondering about is that you are mentioning cm/?user=theuser&password=1&password=2&password=3 as URL for authentication. We are using basic auth headers in the authentication so the username and password are not passed via the URL.

I assume that you are not using the tasmota_user and tasmota_password option correctly.

Can you please recheck and if the issue still exists, please provide a sample playbook for reproducing the issue?

nununo commented 3 years ago

Hi @tobias-richter,

Thank you so much for merging my PR so quickly. Glad I could help.

Regarding your questions:

  1. I am currently defining the variable directly from an encrypted string (as opposed to assigning from a variable in an encrypted file:

    - command: MqttPassword
    value: !vault |
    $ANSIBLE_VAULT;1.1;AES256
    39333135343538366239333236326232316162363939303165326161341363323337396565646436
    39333135343538366239333236326232316162363939303165326161341363323337396565646436
    39333135343538366239333236326232316162363939303165326161341363323337396565646436

    That's probably why I am getting that type and you're not.

  2. I'm confused. Because I also assumed you were using basic auth headers. But from your code I found that the username and password are indeed being sent directly in the URL. This can be found in the code here:

    auth_params = { 'user' : user, 'password' : password }
    status_params = copy.deepcopy(auth_params)
    status_response = requests.get(url = endpoint_uri, params = status_params)

    And using Wireshark I confirmed that username and password are indeed being sent directly in the uri.

According to Python request.get docs, in order to implement basic auth you need to send the username and password through parameter auth. Can you clarify how are you using basic auth?

Cheers, Nuno

tobias-richter commented 3 years ago

@nununo you are totally right. I was not thinking about that variant. I also did not check the implementation in detail since the auth feature originates not from me.

Thank you again for contributing, will merge and release with the fix :)