postgresql_user: use standard module hashlib instead of passlib #23686

pilou- · 2017-04-18T08:42:48Z

SUMMARY

When passlib isn't available, an useless alter role query is executed.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

postgresql_user

ANSIBLE VERSION

ansible 2.4.0 (devel 8b4f5ba064)

ansibot · 2017-04-18T08:46:45Z

cc @nerzhul
click here for bot help

ansibot · 2017-04-18T08:54:46Z

The test ansible-test sanity --test yamllint failed with the following error:

test/integration/targets/postgresql/tasks/test_user.yml:43:5: syntax error: expected <block end>, but found '-'

click here for bot help

mscherer · 2017-04-28T09:45:25Z

lib/ansible/modules/database/postgresql/postgresql_user.py

@@ -52,8 +52,8 @@
      - >
        When passing an encrypted password, the encrypted parameter must also be true, and it must be generated with the format
        C('str[\\"md5\\"] + md5[ password + username ]'), resulting in a total of 35 characters.  An easy way to do this is:
-        C(echo \\"md5`echo -n \\"verysecretpasswordJOE\\" | md5`\\"). Note that if encrypted is set, the stored password will be hashed whether or not
-        it is pre-encrypted.
+        C(echo \\"md5`echo -n \\"verysecretpasswordJOE\\" | md5`\\"). Note that if the presented password string is already in


I think "presented password string" is not correct, shouldn't it be "provided password string" ?

Indeed, done.

mscherer · 2017-04-28T09:46:16Z

lib/ansible/modules/database/postgresql/postgresql_user.py

-        C(echo \\"md5`echo -n \\"verysecretpasswordJOE\\" | md5`\\"). Note that if encrypted is set, the stored password will be hashed whether or not
-        it is pre-encrypted.
+        C(echo \\"md5`echo -n \\"verysecretpasswordJOE\\" | md5`\\"). Note that if the presented password string is already in
+        MD5-encrypted format, then it is used as-is, regardless of encrypted parameter.


I think that's "MD5-hashed", crypto people tend to complain about it.

mscherer · 2017-04-28T09:48:08Z

test/integration/targets/postgresql/tasks/main.yml

+    db_password1: '{{ item.password }}'
+  with_items:
+    - encrypted: 'yes'
+      password: 'secretù'


Can you add a comment about ù not being a typo, and being used for Unicode testing ?

mscherer · 2017-04-28T09:49:45Z

test/integration/targets/postgresql/tasks/test_user.yml

+    <<: *task_parameters
+    postgresql_user: *parameters
+
+  - name: Check that ansible reports they were created


I think that should be "it was created"

Done (3 occurrences updated).

mscherer · 2017-04-28T09:53:55Z

test/integration/targets/postgresql/tasks/main.yml

+- include: test_user.yml
+  vars:
+    encrypted: '{{ item.encrypted }}'
+    db_password1: '{{ item.password }}'


Since the value do not change in the loop, wouldn't it be clearer to directly put the password here directly ?

mscherer · 2017-04-28T09:58:04Z

lib/ansible/modules/database/postgresql/postgresql_user.py

-                        # Cannot check if passlib is not installed, so assume password is different
-                        pwchanging = True
-            else:
+            if ((password.startswith('md5') and len(password) == 32+3) or encrypted == 'UNENCRYPTED'):


I would document why 32+3 (md5 hash + size of 'MD5').

mscherer · 2017-04-28T10:17:06Z

lib/ansible/modules/database/postgresql/postgresql_user.py

                if password != current_role_attrs['rolpassword']:
                    pwchanging = True

+            if not pwchanging and encrypted == 'ENCRYPTED':
+                if md5(to_bytes(password) + to_bytes(user)).hexdigest() != current_role_attrs['rolpassword']:


So, if the password start by 'md5', this trigger a special case (that's the whole point of the PR). but why do we compare the md5 of the password + user with current_role_attrs['rolpassword'], since current_role_attrs['rolpassword'] will be prefixed with 'md5' ? (ie, this would always change the password)

You're right. I reworked the whole pull-request (the next condition was wrong too) and improved the tests.

mattclay · 2017-05-01T06:30:57Z

CI failure due to traceback during integration tests on python 3:

2017-05-01 01:10:45 TASK [postgresql : Create a user (password encrypted: yes)] ********************
2017-05-01 01:10:46 An exception occurred during task execution. To see the full traceback, use -vvv. The error was: UnicodeEncodeError: 'ascii' codec can't encode character '\xf9' in position 6: ordinal not in range(128)
2017-05-01 01:10:46 fatal: [testhost]: FAILED! => {"changed": false, "failed": true, "module_stderr": "Traceback (most recent call last):\n  File \"/tmp/ansible__o2_anvd/ansible_module_postgresql_user.py\", line 770, in <module>\n    main()\n  File \"/tmp/ansible__o2_anvd/ansible_module_postgresql_user.py\", line 729, in main\n    changed = user_add(cursor, user, password, role_attr_flags, encrypted, expires)\n  File \"/tmp/ansible__o2_anvd/ansible_module_postgresql_user.py\", line 269, in user_add\n    cursor.execute(query, query_password_data)\n  File \"/usr/lib/python3/dist-packages/psycopg2/extras.py\", line 120, in execute\n    return super(DictCursor, self).execute(query, vars)\nUnicodeEncodeError: 'ascii' codec can't encode character '\\xf9' in position 6: ordinal not in range(128)\n", "module_stdout": "", "msg": "MODULE FAILURE", "rc": 1}

abadger · 2017-05-04T16:20:59Z

My guess is that the change in the test cases (maybe because the new password is now non-ascii?) is revealing a bug in a different part of hte code.

pilou- · 2017-05-04T16:34:30Z

It seems the second shippable pass with maximum verbosity don't fail.

Using locally a Xenial LXC container (python3-psycopg2: Installed: 2.6.1-1build2) and the command below, the test suite is successful:
ANSIBLE_REMOTE_USER=root ANSIBLE_ROLES_PATH=test/integration/targets ansible-playbook -i 'testhost,' -e ansible_host=$IP -e 'ansible_python_interpreter=/usr/bin/python3' --tags=test_postgresql,test_postgresql_db,test_postgresql_privs,test_postgresql_user,needs_privileged test/integration/destructive.yml.

Using docker, I encountered a problem using test/runner/ansible-test integration --verbose postgresql --docker ubuntu1604py3:

TASK [setup_postgresql_db : start postgresql service] **************************
fatal: [testhost]: FAILED! => {"changed": false, "cmd": "/bin/systemctl", "failed": true, "msg": "Failed to connect to bus: No such file or directory", "rc": 1, "stderr": "Failed to connect to bus: No such file or directory\n", "stderr_lines": ["Failed to connect to bus: No such file or directory"], "stdout": "", "stdout_lines": []}

so I added --docker-privileged parameter, then I got

TASK [postgresql : Create DB] **************************************************
fatal: [testhost]: FAILED! => {"changed": false, "failed": true, "module_stderr": "/bin/sh: 1: /tmp/ansible-test-coverage-wevrv_99/coverage/runner3.5: Permission denied\n", "module_stdout": "", "msg": "MODULE FAILURE", "rc": 126}

do you known how to disable coverage ?

mattclay · 2017-05-04T16:41:07Z

I'm guessing the maximum verbosity passing is not due to the verbosity, but the fact that the test is being run a second time.

mattclay · 2017-05-04T16:43:43Z

Coverage is off by default. The error you're getting indicates the interpreter interception script used by ansible-test is not executable, which is an error I haven't seen before. Is your /tmp directory not allowed to have executable files?

Regarding the docker error, what are you using for your docker host (platform and docker version)?

mattclay · 2017-05-04T16:45:59Z

If you don't mind running the tests without docker, you could try this on a system with python 3.5:

ansible-test integration -v postgresql --allow-destructive --python 3.5

sivel · 2017-05-04T16:52:34Z

I can duplicate the exception with the following command from this branch:

ansible-test integration -v --docker ubuntu1604py3 --docker-privileged --python 3.5 postgresql

When the except is encountered the following variables are in play (with postgresql_user.py modified to display the following):

raise Exception((query, query_password_data))\nException: ('CREATE USER \"ansible_db_user1\" WITH UNENCRYPTED PASSWORD %(password)s ', {'password': 'secretù', 'expires': None})

This relates to:

db_password1: 'secretù' # use UTF-8

ansibot · 2017-05-29T14:17:37Z

waiting_on: maintainer
module: postgresql_user
supported_by: community
maintainers: nerzhul matburt
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
mergeable_state: clean
shippable_status: success
maintainer_shipits: 1
community_shipits: 0
ansible_shipits: 1
shipit_actors: mscherer nerzhul

click here for bot help

See: https://www.postgresql.org/docs/current/static/sql-createrole.html

When an unchanged MD5-hashed password was used and passlib was unavailable, an useless 'ALTER USER' query was executed. Once this useless query avoided, the last 'SELECT' query becomes useless too.

This task is only executed when the playbook has already been executed once, for example using 'ansible-test integration' with '--retry-error' switch when the first run fails. This modification allows to recreate default databases (postgres, template0 and template1) using the same encoding that the one used by the Debian package. Default encoding is 'SQL_ASCII' when default locale is not set in /etc/default/locale.

By default, client encoding is determined either from the LANG_*/LC_* environment variables or using encoding of the database. Containers used in the CI don't define a default locale, then encoding of default databases was SQL_ASCII.

pilou- · 2017-05-29T14:36:50Z

Rebased

pilou- · 2017-05-29T21:25:04Z

shipit

mscherer · 2017-05-29T21:26:21Z

shipit

nerzhul · 2017-05-30T05:34:04Z

shipit

pilou- · 2017-05-30T13:15:48Z

bot_status

ansibot · 2017-05-30T14:11:34Z

waiting_on: maintainer
module: postgresql_user
supported_by: community
maintainers: nerzhul matburt
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
mergeable_state: clean
shippable_status: success
maintainer_shipits: 1
community_shipits: 0
ansible_shipits: 1
shipit_actors: mscherer nerzhul

click here for bot help

matburt · 2017-05-30T17:50:36Z

shipit

abadger · 2017-05-30T17:55:44Z

This has gotten enough shipits from various people since the last real change. Merging.

abadger · 2017-05-30T17:55:57Z

merged to devel.

ansibot added affects_2.4 This issue/PR affects Ansible v2.4 bugfix_pull_request community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. labels Apr 18, 2017

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 18, 2017

pilou- force-pushed the postgresql_user_use_hashlib branch 2 times, most recently from 7f5c54b to b35c107 Compare April 18, 2017 11:52

gundalow added ci_verified Changes made in this PR are causing tests to fail. and removed needs_triage Needs a first human triage before being processed. labels Apr 19, 2017

pilou- mentioned this pull request Apr 21, 2017

postgresql_user: fix bugs related to 'expires' option #23862

Merged

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 28, 2017

mscherer suggested changes Apr 28, 2017

View reviewed changes

pilou- force-pushed the postgresql_user_use_hashlib branch from b35c107 to 6fe3064 Compare April 30, 2017 23:07

ansibot removed ci_verified Changes made in this PR are causing tests to fail. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Apr 30, 2017

pilou- force-pushed the postgresql_user_use_hashlib branch 2 times, most recently from fa12292 to 065a296 Compare May 1, 2017 01:05

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label May 1, 2017

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 29, 2017

pilou- added 5 commits May 29, 2017 16:33

postgresql_user: fix doc

d8b114c

See: https://www.postgresql.org/docs/current/static/sql-createrole.html

Avoid useless queries: use standard module hashlib

498f199

When an unchanged MD5-hashed password was used and passlib was unavailable, an useless 'ALTER USER' query was executed. Once this useless query avoided, the last 'SELECT' query becomes useless too.

postgresql_user: add tests with hashed password

bb86d45

PostgreSQL tests: enforce UTF8

114888d

By default, client encoding is determined either from the LANG_*/LC_* environment variables or using encoding of the database. Containers used in the CI don't define a default locale, then encoding of default databases was SQL_ASCII.

pilou- force-pushed the postgresql_user_use_hashlib branch from d7d9579 to 114888d Compare May 29, 2017 14:34

ansibot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label May 29, 2017

ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels May 30, 2017

abadger merged commit a413119 into ansible:devel May 30, 2017

ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018

dagwieers added the postgresql PostgreSQL community label Jan 28, 2019

dagwieers added the database Database category label Feb 13, 2019

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

postgresql_user: use standard module hashlib instead of passlib #23686

postgresql_user: use standard module hashlib instead of passlib #23686

pilou- commented Apr 18, 2017

ansibot commented Apr 18, 2017

ansibot commented Apr 18, 2017

mscherer Apr 28, 2017

pilou- Apr 30, 2017 •

edited

mscherer Apr 28, 2017

pilou- Apr 30, 2017

mscherer Apr 28, 2017

pilou- Apr 30, 2017

mscherer Apr 28, 2017

pilou- Apr 30, 2017

mscherer Apr 28, 2017

pilou- Apr 30, 2017

mscherer Apr 28, 2017

pilou- Apr 30, 2017

mscherer Apr 28, 2017

pilou- Apr 30, 2017

mattclay commented May 1, 2017

abadger commented May 4, 2017

pilou- commented May 4, 2017

mattclay commented May 4, 2017

mattclay commented May 4, 2017

mattclay commented May 4, 2017

sivel commented May 4, 2017

ansibot commented May 29, 2017

pilou- commented May 29, 2017

pilou- commented May 29, 2017

mscherer commented May 29, 2017

nerzhul commented May 30, 2017

pilou- commented May 30, 2017

ansibot commented May 30, 2017

matburt commented May 30, 2017

abadger commented May 30, 2017

abadger commented May 30, 2017

postgresql_user: use standard module hashlib instead of passlib #23686

postgresql_user: use standard module hashlib instead of passlib #23686

Conversation

pilou- commented Apr 18, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ansibot commented Apr 18, 2017

ansibot commented Apr 18, 2017

Choose a reason for hiding this comment

pilou- Apr 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattclay commented May 1, 2017

abadger commented May 4, 2017

pilou- commented May 4, 2017

mattclay commented May 4, 2017

mattclay commented May 4, 2017

mattclay commented May 4, 2017

sivel commented May 4, 2017

ansibot commented May 29, 2017

pilou- commented May 29, 2017

pilou- commented May 29, 2017

mscherer commented May 29, 2017

nerzhul commented May 30, 2017

pilou- commented May 30, 2017

ansibot commented May 30, 2017

matburt commented May 30, 2017

abadger commented May 30, 2017

abadger commented May 30, 2017

pilou- Apr 30, 2017 •

edited