passbolt / lab-passbolt-ansible-collection

Source repository for https://galaxy.ansible.com/anatomicjc/passbolt
MIT No Attribution
9 stars 10 forks source link

Use PASSBOLT_NEW_RESOURCE_PASSWORD_LENGTH when generating passwords #8

Closed PatrickHess closed 1 year ago

PatrickHess commented 1 year ago

While $PASSBOLT_NEW_RESOURCE_PASSWORD_LENGTH was correctly read from the environment, its value was effectively never used when generating the actual passwords for new resources.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

AnatomicJC commented 1 year ago

Good catch! Thank you for this PR. :+1:

AnatomicJC commented 1 year ago

I pushed a new release to https://galaxy.ansible.com/anatomicjc/passbolt FYI

PatrickHess commented 1 year ago

Thanks for releasing the fix this quickly, very much appreciated. :+1:

PatrickHess commented 1 year ago

After trying out the updated version, I'm afraid my fix doesn't work as intended.

An unhandled exception occurred while running the lookup plugin 'anatomicjc.passbolt.passbolt'. Error was a <class 'TypeError'>, original message: 'str' object cannot be interpreted as an integer. 'str' object cannot be interpreted as an integer"}

Upon further inspection, it appears that new_resource_password_length is stored as a string instead of an integer, at least when the value is being populated from the environment variable.

Unfortunately, I'm not set up to build the plugin locally right now, so I'll refrain from proposing any further fixes without being able to properly test my changes before submitting them as a PR.

I'm very sorry for the inconvenience caused by this PR.

AnatomicJC commented 1 year ago

Hum I see, maybe can we cast the value, like this:

return "".join(secrets.choice(characters) for i in range(int(self.dict_config.get("new_resource_password_length"))))

?

PatrickHess commented 1 year ago

That should definitely solve this particular issue.

A more thorough solution, in my opinion, would be for self._get_env_value() to return the expected type right away. I'm not entirely sure how to implement that, though. Would it be feasible to derive the desired return type from the default value?

AnatomicJC commented 1 year ago

Ok, I guess I got it.

So the environment variable is always a string but it should return an int, as the default value 20 is an int.

I tried this in the python console:

>>> s = '123'
>>> default = 20
>>> type(default)
<class 'int'>
>>> type(default)(s)
123

s is a string and default an integer. It seems we can use type(default) as a dynamic cast function.

Can you try to edit the _get_env_value() function like this and check if it works ?

    def _get_env_value(self, selected_variable, environment_variables, default=str()):
        if not environment_variables:
            return environ.get(selected_variable, default)
        else:
            return self._templar.template(
                next(
                    (
                        type(default)(item.get(selected_variable))
                        for item in environment_variables
                        if item.get(selected_variable)
                    ),
                    default,
                )
            )

I replaced item.get(selected_variable) with type(default)(item.get(selected_variable))

Thanks!

PatrickHess commented 1 year ago

I managed to perform a couple of tests by editing the contents of ~/.ansible/collections/ansible_collections/anatomicjc/passbolt/plugins/lookup/passbolt.py directly.

First of all, here's how our task is defined in Ansible:

- name: Fetch existing root password from Passbolt, or generate a new one
  environment:
    PASSBOLT_BASE_URL: "{{ passbolt.url }}"
    PASSBOLT_PRIVATE_KEY: "{{ lookup('env', 'PASSBOLT_PRIVATE_KEY') | replace('\\n', '\n') }}"
    PASSBOLT_PASSPHRASE: "{{ lookup('env', 'PASSBOLT_PASSPHRASE') }}"
    PASSBOLT_CREATE_NEW_RESOURCE: "true"
    PASSBOLT_NEW_RESOURCE_PASSWORD_LENGTH: "{{ passbolt.new_root_password_length }}"
  vars:
    response_from_passbolt: "{{ lookup('anatomicjc.passbolt.passbolt', inventory_hostname, username='root') }}"
  set_fact:
    root_password: "{{ response_from_passbolt.password }}"
  failed_when: "root_password == ''"

passbolt.new_root_password_length gets set to 32 in a group vars file.

After having applied the type conversion (i.e., type(default)(item.get(selected_variable))), the playbook fails with the following error:

An unhandled exception occurred while templating '{{ lookup('anatomicjc.passbolt.passbolt', inventory_hostname, username='root') }}'. Error was a <class 'ansible.errors.AnsibleError'>, original message: An unhandled exception occurred while running the lookup plugin 'anatomicjc.passbolt.passbolt'. Error was a <class 'ValueError'>, original message: invalid literal for int() with base 10: '{{ passbolt.new_root_password_length }}'. invalid literal for int() with base 10: '{{ passbolt.new_root_password_length }}'"}

It is not entirely clear to me why the plugin receives the original template string instead of the variable's actual value. I would have assumed that Ansible had already interpreted the template string even before calling the plugin.

However, your original suggestion of casting the argument that is passed to range() does in fact work and generate a (in our case) 32-character password:

return "".join(secrets.choice(characters) for i in range(int(self.dict_config.get("new_resource_password_length"))))
AnatomicJC commented 1 year ago

Thank you for testing and feeback, I just published a new version 0.0.11 with the cast method.

PatrickHess commented 1 year ago

After updating to 0.0.11, I can confirm that $PASSBOLT_NEW_RESOURCE_PASSWORD_LENGTH is now working as intended. Thanks for taking the time to work this out.