From 32d6a354d74c2ac18bb73e50b843d5bbf55c6b13 Mon Sep 17 00:00:00 2001 From: Strahinja Kustudic Date: Mon, 21 May 2018 21:49:44 +0200 Subject: [PATCH] postgresql_user: set encrypted as default and fix empty password reporting changed (#36931) * Set encrypted as default and fix empty password reporting changed * Starting with Postgres 10 `UNENCRYPTED` passwords are removed and because of that this module fails with the default `encrypted=no`. Also encrypted passwords are suported since version 7.2 (https://www.postgresql.org/docs/7.2/static/sql-createuser.html) which went EOL in 2007 and since 7.3 it is the default. Because of this it makes a lot more sense to make `encrypted=yes` the default. This won't break backward compatibility, the module would just update the user's password in the DB in the hashed format and everything else will work like before. It's also a security bad practice to store passwords in plain text. fixes #25823 * There was also a bug with `encrypted=yes` and an empty password always reported as changed. * Improved documentation for `encrypted`/`password` parameters, and removed some obsolete notes about passlib. * Fix clearing user's password to work with all versions of Postgres * Add tests for clearing the user password * Fix documentation atfer rebase * Add changelog fragment --- CHANGELOG.md | 5 + .../postgresql_user-encrypted-fixes.yml | 7 ++ .../database/postgresql/postgresql_user.py | 108 +++++++++++------- .../postgresql/tasks/test_password.yml | 92 +++++++++++++++ test/sanity/validate-modules/ignore.txt | 1 - 5 files changed, 169 insertions(+), 44 deletions(-) create mode 100644 changelogs/fragments/postgresql_user-encrypted-fixes.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index 4de710bd2f..9b33e109fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,11 @@ See [Porting Guide](https://docs.ansible.com/ansible/devel/porting_guides/portin * Removed restriction from protocol in cloudflare_dns module to allow other protocols than tcp and udp to be specified. * Ansible 2.6 and onwards, `target_id` parameter in `vmware_target_canonical_facts` module is an optional parameter. +* `postgresql_user` module changed `encrypted=yes` to be the default. This + shouldn't break any current playbooks, the module will just store passwords + hashed by default. This change was done because Postgres 10 dropped support for + `UNENCRYPTED` passwords and because all versions since Postgres 7.2 support + storing encrypted passwords. #### Removed modules (previously deprecated) diff --git a/changelogs/fragments/postgresql_user-encrypted-fixes.yml b/changelogs/fragments/postgresql_user-encrypted-fixes.yml new file mode 100644 index 0000000000..d6967801b2 --- /dev/null +++ b/changelogs/fragments/postgresql_user-encrypted-fixes.yml @@ -0,0 +1,7 @@ +--- +minor_changes: +- "`postgresql_user` module changed `encrypted=yes` to be the default. This + shouldn't break any current playbooks, the module will just store passwords + hashed by default. This change was done because Postgres 10 dropped support for + `UNENCRYPTED` passwords and because all versions since Postgres 7.2 support + storing encrypted passwords." diff --git a/lib/ansible/modules/database/postgresql/postgresql_user.py b/lib/ansible/modules/database/postgresql/postgresql_user.py index 0e5cd0556e..4074f26173 100644 --- a/lib/ansible/modules/database/postgresql/postgresql_user.py +++ b/lib/ansible/modules/database/postgresql/postgresql_user.py @@ -7,9 +7,11 @@ from __future__ import absolute_import, division, print_function __metaclass__ = type -ANSIBLE_METADATA = {'metadata_version': '1.1', - 'status': ['stableinterface'], - 'supported_by': 'community'} +ANSIBLE_METADATA = { + 'metadata_version': '1.1', + 'status': ['stableinterface'], + 'supported_by': 'community' +} DOCUMENTATION = ''' @@ -34,60 +36,70 @@ version_added: "0.6" options: name: description: - - name of the user (role) to add or remove + - Name of the user (role) to add or remove. required: true password: description: - - set the user's password, before 1.4 this was required. - - > - 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 the provided password string is already in - MD5-hashed format, then it is used as-is, regardless of encrypted parameter. + - Set the user's password, before 1.4 this was required. + - Password can be passed unhashed or hashed (MD5-hashed). + - Unhashed password will automatically be hashed when saved into the + database if C(encrypted) parameter is set, otherwise it will be save in + plain text format. + - When passing a hashed password 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' | md5sum)"). + - Note that if the provided password string is already in MD5-hashed + format, then it is used as-is, regardless of C(encrypted) parameter. db: description: - - name of database where permissions will be granted + - Name of database where permissions will be granted. fail_on_user: description: - - if C(yes), fail when user can't be removed. Otherwise just log and continue - type: bool + - If C(yes), fail when user can't be removed. Otherwise just log and + continue. default: 'yes' + type: bool port: description: - Database port to connect to. default: 5432 login_user: description: - - User (role) used to authenticate with PostgreSQL + - User (role) used to authenticate with PostgreSQL. default: postgres login_password: description: - - Password used to authenticate with PostgreSQL + - Password used to authenticate with PostgreSQL. login_host: description: - Host running PostgreSQL. default: localhost login_unix_socket: description: - - Path to a Unix domain socket for local connections + - Path to a Unix domain socket for local connections. priv: description: - - "PostgreSQL privileges string in the format: C(table:priv1,priv2)" + - "PostgreSQL privileges string in the format: C(table:priv1,priv2)." role_attr_flags: description: - - "PostgreSQL role attributes string in the format: CREATEDB,CREATEROLE,SUPERUSER" + - "PostgreSQL role attributes string in the format: CREATEDB,CREATEROLE,SUPERUSER." - Note that '[NO]CREATEUSER' is deprecated. - default: "" - choices: [ "[NO]SUPERUSER", "[NO]CREATEROLE", "[NO]CREATEDB", "[NO]INHERIT", "[NO]LOGIN", "[NO]REPLICATION", "[NO]BYPASSRLS" ] + choices: ["[NO]SUPERUSER", "[NO]CREATEROLE", "[NO]CREATEDB", "[NO]INHERIT", "[NO]LOGIN", "[NO]REPLICATION", "[NO]BYPASSRLS"] state: description: - - The user (role) state + - The user (role) state. default: present - choices: [ present, absent ] + choices: ["present", "absent"] encrypted: description: - - whether the password is stored hashed in the database. boolean. Passwords can be passed already hashed or unhashed, and postgresql ensures the - stored password is hashed when encrypted is set. + - Whether the password is stored hashed in the database. Passwords can be + passed already hashed or unhashed, and postgresql ensures the stored + password is hashed when C(encrypted) is set. + - "Note: Postgresql 10 and newer doesn't support unhashed passwords." + - Previous to Ansible 2.6, this was C(no) by default. + default: 'yes' + type: bool version_added: '1.4' expires: description: @@ -97,22 +109,26 @@ options: version_added: '1.4' no_password_changes: description: - - if C(yes), don't inspect database for password changes. Effective when C(pg_authid) is not accessible (such as AWS RDS). Otherwise, make + - If C(yes), don't inspect database for password changes. Effective when + C(pg_authid) is not accessible (such as AWS RDS). Otherwise, make password changes as necessary. - type: bool default: 'no' + type: bool version_added: '2.0' ssl_mode: description: - - Determines whether or with what priority a secure SSL TCP/IP connection will be negotiated with the server. - - See https://www.postgresql.org/docs/current/static/libpq-ssl.html for more information on the modes. + - Determines whether or with what priority a secure SSL TCP/IP connection + will be negotiated with the server. + - See U(https://www.postgresql.org/docs/current/static/libpq-ssl.html) for + more information on the modes. - Default of C(prefer) matches libpq default. default: prefer - choices: [disable, allow, prefer, require, verify-ca, verify-full] + choices: ["disable", "allow", "prefer", "require", "verify-ca", "verify-full"] version_added: '2.3' ssl_rootcert: description: - - Specifies the name of a file containing SSL certificate authority (CA) certificate(s). If the file exists, the server's certificate will be + - Specifies the name of a file containing SSL certificate authority (CA) + certificate(s). If the file exists, the server's certificate will be verified to be signed by one of these authorities. version_added: '2.3' conn_limit: @@ -128,14 +144,11 @@ notes: PostgreSQL must also be installed on the remote host. For Ubuntu-based systems, install the postgresql, libpq-dev, and python-psycopg2 packages on the remote host before using this module. - - If the passlib library is installed, then passwords that are encrypted - in the DB but not encrypted when passed as arguments can be checked for - changes. If the passlib library is not installed, unencrypted passwords - stored in the DB encrypted will be assumed to have changed. - If you specify PUBLIC as the user, then the privilege changes will apply to all users. You may not specify password or role_attr_flags when the PUBLIC user is specified. - - The ssl_rootcert parameter requires at least Postgres version 8.4 and I(psycopg2) version 2.4.3. + - The ssl_rootcert parameter requires at least Postgres version 8.4 and + I(psycopg2) version 2.4.3. requirements: [ psycopg2 ] author: "Ansible Core Team" ''' @@ -148,10 +161,11 @@ EXAMPLES = ''' password: ceec4eif7ya priv: "CONNECT/products:ALL" -# Create rails user, grant privilege to create other databases and demote rails from super user status +# Create rails user, set its password (MD5-hashed) and grant privilege to create other +# databases and demote rails from super user status - postgresql_user: name: rails - password: secret + password: md59543f1d82624df2b31672ec0f7050460 role_attr_flags: CREATEDB,NOSUPERUSER # Remove test user privileges from acme @@ -184,7 +198,7 @@ EXAMPLES = ''' - postgresql_user: db: test user: test - password: NULL + password: "" ''' import itertools @@ -248,7 +262,7 @@ def user_add(cursor, user, password, role_attr_flags, encrypted, expires, conn_l query_password_data = dict(password=password, expires=expires) query = ['CREATE USER %(user)s' % {"user": pg_quote_identifier(user, 'role')}] - if password is not None: + if password is not None and password != '': query.append("WITH %(crypt)s" % {"crypt": encrypted}) query.append("PASSWORD %(password)s") if expires is not None: @@ -277,11 +291,16 @@ def user_should_we_change_password(current_role_attrs, user, password, encrypted # Do we actually need to do anything? pwchanging = False if password is not None: + # Empty password means that the role shouldn't have a password, which + # means we need to check if the current password is None. + if password == '': + if current_role_attrs['rolpassword'] is not None: + pwchanging = True # 32: MD5 hashes are represented as a sequence of 32 hexadecimal digits # 3: The size of the 'md5' prefix # When the provided password looks like a MD5-hash, value of # 'encrypted' is ignored. - if ((password.startswith('md5') and len(password) == 32 + 3) or encrypted == 'UNENCRYPTED'): + elif (password.startswith('md5') and len(password) == 32 + 3) or encrypted == 'UNENCRYPTED': if password != current_role_attrs['rolpassword']: pwchanging = True elif encrypted == 'ENCRYPTED': @@ -360,8 +379,11 @@ def user_alter(db_connection, module, user, password, role_attr_flags, encrypted alter = ['ALTER USER %(user)s' % {"user": pg_quote_identifier(user, 'role')}] if pwchanging: - alter.append("WITH %(crypt)s" % {"crypt": encrypted}) - alter.append("PASSWORD %(password)s") + if password != '': + alter.append("WITH %(crypt)s" % {"crypt": encrypted}) + alter.append("PASSWORD %(password)s") + else: + alter.append("WITH PASSWORD NULL") alter.append(role_attr_flags) elif role_attr_flags: alter.append('WITH %s' % role_attr_flags) @@ -715,7 +737,7 @@ def main(): port=dict(default='5432'), fail_on_user=dict(type='bool', default='yes'), role_attr_flags=dict(default=''), - encrypted=dict(type='bool', default='no'), + encrypted=dict(type='bool', default='yes'), no_password_changes=dict(type='bool', default='no'), expires=dict(default=None), ssl_mode=dict(default='prefer', choices=[ diff --git a/test/integration/targets/postgresql/tasks/test_password.yml b/test/integration/targets/postgresql/tasks/test_password.yml index 3112ee02db..335d830124 100644 --- a/test/integration/targets/postgresql/tasks/test_password.yml +++ b/test/integration/targets/postgresql/tasks/test_password.yml @@ -161,6 +161,52 @@ - <<: *changed + - name: 'Using MD5-hashed password: check that password changed when clearing the password' + <<: *task_parameters + postgresql_user: + <<: *parameters + password: '' + encrypted: 'yes' + environment: + PGCLIENTENCODING: 'UTF8' + + - <<: *changed + + - name: 'Using MD5-hashed password: check that password not changed when clearing the password again' + <<: *task_parameters + postgresql_user: + <<: *parameters + password: '' + encrypted: 'yes' + environment: + PGCLIENTENCODING: 'UTF8' + PGOPTIONS: '-c default_transaction_read_only=on' # ensure 'alter user' query isn't executed + + - <<: *not_changed + + - name: 'Using cleartext password: check that password not changed when clearing the password again' + <<: *task_parameters + postgresql_user: + <<: *parameters + password: '' + encrypted: 'no' + environment: + PGCLIENTENCODING: 'UTF8' + PGOPTIONS: '-c default_transaction_read_only=on' # ensure 'alter user' query isn't executed + + - <<: *not_changed + + - name: 'Using MD5-hashed password: check that password changed when using a cleartext password' + <<: *task_parameters + postgresql_user: + <<: *parameters + password: '{{ db_password1 }}' + encrypted: 'yes' + environment: + PGCLIENTENCODING: 'UTF8' + + - <<: *changed + when: encrypted == 'yes' - block: @@ -201,6 +247,52 @@ - <<: *changed + - name: 'Using cleartext password: check that password changed when clearing the password' + <<: *task_parameters + postgresql_user: + <<: *parameters + password: '' + encrypted: 'no' + environment: + PGCLIENTENCODING: 'UTF8' + + - <<: *changed + + - name: 'Using cleartext password: check that password not changed when clearing the password again' + <<: *task_parameters + postgresql_user: + <<: *parameters + password: '' + encrypted: 'no' + environment: + PGCLIENTENCODING: 'UTF8' + PGOPTIONS: '-c default_transaction_read_only=on' # ensure 'alter user' query isn't executed + + - <<: *not_changed + + - name: 'Using MD5-hashed password: check that password not changed when clearing the password again' + <<: *task_parameters + postgresql_user: + <<: *parameters + password: '' + encrypted: 'yes' + environment: + PGCLIENTENCODING: 'UTF8' + PGOPTIONS: '-c default_transaction_read_only=on' # ensure 'alter user' query isn't executed + + - <<: *not_changed + + - name: 'Using cleartext password: check that password changed when using cleartext password' + <<: *task_parameters + postgresql_user: + <<: *parameters + password: "{{ db_password1 }}" + encrypted: 'no' + environment: + PGCLIENTENCODING: 'UTF8' + + - <<: *changed + when: encrypted == 'no' - name: Remove user diff --git a/test/sanity/validate-modules/ignore.txt b/test/sanity/validate-modules/ignore.txt index f371f33b12..d1cab64ab5 100644 --- a/test/sanity/validate-modules/ignore.txt +++ b/test/sanity/validate-modules/ignore.txt @@ -643,7 +643,6 @@ lib/ansible/modules/database/postgresql/postgresql_schema.py E322 lib/ansible/modules/database/postgresql/postgresql_schema.py E324 lib/ansible/modules/database/postgresql/postgresql_user.py E322 lib/ansible/modules/database/postgresql/postgresql_user.py E324 -lib/ansible/modules/database/postgresql/postgresql_user.py E325 lib/ansible/modules/database/postgresql/postgresql_user.py E326 lib/ansible/modules/database/proxysql/proxysql_backend_servers.py E322 lib/ansible/modules/database/proxysql/proxysql_backend_servers.py E325