From 47710cfb93fad4f98c5895d5a263fadd1d0cc8c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurent=20Inderm=C3=BChle?= Date: Tue, 16 Apr 2024 10:52:24 +0200 Subject: [PATCH] Enhance support of tls_requires in mysql_user and mysql_info (#628) * fix option name * Add tests for users using SSL * Rewrite get_tls_requires using mysql.user table * Add tls_requires to users_info filter * add more consistant test users * Add tls tests users in cleanup task * Fix tls_requires data structure inconsistencies between modules * Refactor user implementation to host get_tls_requires * fix MySQL tls_requires not removed from user passed as empty * Fix wrong variable used to return a hashed password * Fix sanity * fix unit tests * Add changelog fragment * Add PR URI to the changelog * Add more precise change log * fix documentation using wrong variable as an example * Document example returned value `tls_requires` from users_info filter * Revert changes that will be in a separate PR * Fix sanity --- .../fragments/mysql_user_tls_requires.yml | 6 ++ .../implementations/mariadb/user.py | 45 ++++++++++ .../implementations/mysql/user.py | 46 ++++++++++ plugins/module_utils/user.py | 43 +++------- plugins/modules/mysql_info.py | 22 +++-- plugins/modules/mysql_role.py | 4 +- plugins/modules/mysql_user.py | 3 - .../tasks/filter_users_info.yml | 85 +++++++++++++++++-- tests/unit/plugins/modules/test_mysql_info.py | 14 +-- 9 files changed, 213 insertions(+), 55 deletions(-) create mode 100644 changelogs/fragments/mysql_user_tls_requires.yml diff --git a/changelogs/fragments/mysql_user_tls_requires.yml b/changelogs/fragments/mysql_user_tls_requires.yml new file mode 100644 index 0000000..1fa0c94 --- /dev/null +++ b/changelogs/fragments/mysql_user_tls_requires.yml @@ -0,0 +1,6 @@ +--- +minor_changes: + - mysql_info - Add ``tls_requires`` returned value for the ``users_info`` filter (https://github.com/ansible-collections/community.mysql/pull/628). +bugfixes: + - mysql_user - Fix idempotence when using variables from the ``users_info`` filter of ``mysql_info`` as an input (https://github.com/ansible-collections/community.mysql/pull/628). + - mysql_user - Fix ``tls_requires`` not removing ``SSL`` and ``X509`` when sets as empty (https://github.com/ansible-collections/community.mysql/pull/628). diff --git a/plugins/module_utils/implementations/mariadb/user.py b/plugins/module_utils/implementations/mariadb/user.py index cdc14b2..fa9ecdf 100644 --- a/plugins/module_utils/implementations/mariadb/user.py +++ b/plugins/module_utils/implementations/mariadb/user.py @@ -29,3 +29,48 @@ def server_supports_password_expire(cursor): version = get_server_version(cursor) return LooseVersion(version) >= LooseVersion("10.4.3") + + +def get_tls_requires(cursor, user, host): + """Get user TLS requirements. + Reads directly from mysql.user table allowing for a more + readable code. + + Args: + cursor (cursor): DB driver cursor object. + user (str): User name. + host (str): User host name. + + Returns: Dictionary containing current TLS required + """ + tls_requires = dict() + + query = ('SELECT ssl_type, ssl_cipher, x509_issuer, x509_subject ' + 'FROM mysql.user WHERE User = %s AND Host = %s') + cursor.execute(query, (user, host)) + res = cursor.fetchone() + + # Mysql_info use a DictCursor so we must convert back to a list + # otherwise we get KeyError 0 + if isinstance(res, dict): + res = list(res.values()) + + # When user don't require SSL, res value is: ('', '', '', '') + if not any(res): + return None + + if res[0] == 'ANY': + tls_requires['SSL'] = None + + if res[0] == 'X509': + tls_requires['X509'] = None + + if res[1]: + tls_requires['CIPHER'] = res[1] + + if res[2]: + tls_requires['ISSUER'] = res[2] + + if res[3]: + tls_requires['SUBJECT'] = res[3] + return tls_requires diff --git a/plugins/module_utils/implementations/mysql/user.py b/plugins/module_utils/implementations/mysql/user.py index 4e41c05..700c355 100644 --- a/plugins/module_utils/implementations/mysql/user.py +++ b/plugins/module_utils/implementations/mysql/user.py @@ -8,6 +8,9 @@ __metaclass__ = type from ansible_collections.community.mysql.plugins.module_utils.version import LooseVersion from ansible_collections.community.mysql.plugins.module_utils.mysql import get_server_version +import re +import shlex + def use_old_user_mgmt(cursor): version = get_server_version(cursor) @@ -30,3 +33,46 @@ def server_supports_password_expire(cursor): version = get_server_version(cursor) return LooseVersion(version) >= LooseVersion("5.7") + + +def get_tls_requires(cursor, user, host): + """Get user TLS requirements. + We must use SHOW GRANTS because some tls fileds are encoded. + + Args: + cursor (cursor): DB driver cursor object. + user (str): User name. + host (str): User host name. + + Returns: Dictionary containing current TLS required + """ + if not use_old_user_mgmt(cursor): + query = "SHOW CREATE USER '%s'@'%s'" % (user, host) + else: + query = "SHOW GRANTS for '%s'@'%s'" % (user, host) + + cursor.execute(query) + grants = cursor.fetchone() + + # Mysql_info use a DictCursor so we must convert back to a list + # otherwise we get KeyError 0 + if isinstance(grants, dict): + grants = list(grants.values()) + grants_str = ''.join(grants) + + pattern = r"(?<=\bREQUIRE\b)(.*?)(?=(?:\bPASSWORD\b|$))" + requires_match = re.search(pattern, grants_str) + requires = requires_match.group().strip() if requires_match else "" + + if requires.startswith('NONE'): + return None + + if requires.startswith('SSL'): + return {'SSL': None} + + if requires.startswith('X509'): + return {'X509': None} + + items = iter(shlex.split(requires)) + requires = dict(zip(items, items)) + return requires or None diff --git a/plugins/module_utils/user.py b/plugins/module_utils/user.py index f042c85..d4ae9dd 100644 --- a/plugins/module_utils/user.py +++ b/plugins/module_utils/user.py @@ -17,6 +17,7 @@ from ansible.module_utils.six import iteritems from ansible_collections.community.mysql.plugins.module_utils.mysql import ( mysql_driver, + get_server_implementation, ) @@ -80,31 +81,6 @@ def do_not_mogrify_requires(query, params, tls_requires): return query, params -def get_tls_requires(cursor, user, host): - if user: - if not impl.use_old_user_mgmt(cursor): - query = "SHOW CREATE USER '%s'@'%s'" % (user, host) - else: - query = "SHOW GRANTS for '%s'@'%s'" % (user, host) - - cursor.execute(query) - require_list = [tuple[0] for tuple in filter(lambda x: "REQUIRE" in x[0], cursor.fetchall())] - require_line = require_list[0] if require_list else "" - pattern = r"(?<=\bREQUIRE\b)(.*?)(?=(?:\bPASSWORD\b|$))" - requires_match = re.search(pattern, require_line) - requires = requires_match.group().strip() if requires_match else "" - if any((requires.startswith(req) for req in ('SSL', 'X509', 'NONE'))): - requires = requires.split()[0] - if requires == 'NONE': - requires = None - else: - import shlex - - items = iter(shlex.split(requires)) - requires = dict(zip(items, items)) - return requires or None - - def get_grants(cursor, user, host): cursor.execute("SHOW GRANTS FOR %s@%s", (user, host)) grants_line = list(filter(lambda x: "ON *.*" in x[0], cursor.fetchall()))[0] @@ -166,6 +142,7 @@ def user_add(cursor, user, host, host_all, password, encrypted, return {'changed': True, 'password_changed': None, 'attributes': attributes} # Determine what user management method server uses + impl = get_user_implementation(cursor) old_user_mgmt = impl.use_old_user_mgmt(cursor) mogrify = do_not_mogrify_requires if old_user_mgmt else mogrify_requires @@ -244,6 +221,7 @@ def user_mod(cursor, user, host, host_all, password, encrypted, grant_option = False # Determine what user management method server uses + impl = get_user_implementation(cursor) old_user_mgmt = impl.use_old_user_mgmt(cursor) if host_all and not role: @@ -499,7 +477,7 @@ def user_mod(cursor, user, host, host_all, password, encrypted, continue # Handle TLS requirements - current_requires = get_tls_requires(cursor, user, host) + current_requires = sanitize_requires(impl.get_tls_requires(cursor, user, host)) if current_requires != tls_requires: msg = "TLS requires updated" if not module.check_mode: @@ -837,6 +815,7 @@ def privileges_grant(cursor, user, host, db_table, priv, tls_requires, maria_rol query.append("TO %s") params = (user) + impl = get_user_implementation(cursor) if tls_requires and impl.use_old_user_mgmt(cursor): query, params = mogrify_requires(" ".join(query), params, tls_requires) query = [query] @@ -973,6 +952,7 @@ def limit_resources(module, cursor, user, host, resource_limits, check_mode): Returns: True, if changed, False otherwise. """ + impl = get_user_implementation(cursor) if not impl.server_supports_alter_user(cursor): module.fail_json(msg="The server version does not match the requirements " "for resource_limits parameter. See module's documentation.") @@ -1108,12 +1088,11 @@ def attributes_get(cursor, user, host): return j if j else None -def get_impl(cursor): - global impl - cursor.execute("SELECT VERSION()") - if 'mariadb' in cursor.fetchone()[0].lower(): +def get_user_implementation(cursor): + db_engine = get_server_implementation(cursor) + if db_engine == 'mariadb': from ansible_collections.community.mysql.plugins.module_utils.implementations.mariadb import user as mariauser - impl = mariauser + return mariauser else: from ansible_collections.community.mysql.plugins.module_utils.implementations.mysql import user as mysqluser - impl = mysqluser + return mysqluser diff --git a/plugins/modules/mysql_info.py b/plugins/modules/mysql_info.py index 0be25fa..f30f1a1 100644 --- a/plugins/modules/mysql_info.py +++ b/plugins/modules/mysql_info.py @@ -146,7 +146,7 @@ EXAMPLES = r''' plugin: "{{ item.plugin | default(omit) }}" plugin_auth_string: "{{ item.plugin_auth_string | default(omit) }}" plugin_hash_string: "{{ item.plugin_hash_string | default(omit) }}" - tls_require: "{{ item.tls_require | default(omit) }}" + tls_requires: "{{ item.tls_requires | default(omit) }}" priv: "{{ item.priv | default(omit) }}" resource_limits: "{{ item.resource_limits | default(omit) }}" column_case_sensitive: true @@ -240,7 +240,8 @@ users_info: "host": "host.com", "plugin": "mysql_native_password", "priv": "db1.*:SELECT/db2.*:SELECT", - "resource_limits": { "MAX_USER_CONNECTIONS": 100 } } + "resource_limits": { "MAX_USER_CONNECTIONS": 100 }, + "tls_requires": { "SSL": null } } version_added: '3.8.0' engines: description: Information about the server's storage engines. @@ -300,6 +301,7 @@ from ansible_collections.community.mysql.plugins.module_utils.user import ( privileges_get, get_resource_limits, get_existing_authentication, + get_user_implementation, ) from ansible.module_utils.six import iteritems from ansible.module_utils._text import to_native @@ -327,10 +329,11 @@ class MySQL_Info(object): 5. add info about the new subset with an example to RETURN block """ - def __init__(self, module, cursor, server_implementation): + def __init__(self, module, cursor, server_implementation, user_implementation): self.module = module self.cursor = cursor self.server_implementation = server_implementation + self.user_implementation = user_implementation self.info = { 'version': {}, 'databases': {}, @@ -602,13 +605,17 @@ class MySQL_Info(object): priv_string.remove('*.*:USAGE') resource_limits = get_resource_limits(self.cursor, user, host) - copy_ressource_limits = dict.copy(resource_limits) + + tls_requires = self.user_implementation.get_tls_requires( + self.cursor, user, host) + output_dict = { 'name': user, 'host': host, 'priv': '/'.join(priv_string), 'resource_limits': copy_ressource_limits, + 'tls_requires': tls_requires, } # Prevent returning a resource limit if empty @@ -619,6 +626,10 @@ class MySQL_Info(object): if len(output_dict['resource_limits']) == 0: del output_dict['resource_limits'] + # Prevent returning tls_require if empty + if not tls_requires: + del output_dict['tls_requires'] + authentications = get_existing_authentication(self.cursor, user, host) if authentications: output_dict.update(authentications) @@ -745,11 +756,12 @@ def main(): module.fail_json(msg) server_implementation = get_server_implementation(cursor) + user_implementation = get_user_implementation(cursor) ############################### # Create object and do main job - mysql = MySQL_Info(module, cursor, server_implementation) + mysql = MySQL_Info(module, cursor, server_implementation, user_implementation) module.exit_json(changed=False, connector_name=connector_name, diff --git a/plugins/modules/mysql_role.py b/plugins/modules/mysql_role.py index 3e3462a..65ed894 100644 --- a/plugins/modules/mysql_role.py +++ b/plugins/modules/mysql_role.py @@ -309,7 +309,7 @@ from ansible_collections.community.mysql.plugins.module_utils.mysql import ( ) from ansible_collections.community.mysql.plugins.module_utils.user import ( convert_priv_dict_to_str, - get_impl, + get_user_implementation, get_mode, user_mod, privileges_grant, @@ -1054,7 +1054,7 @@ def main(): # Set defaults changed = False - get_impl(cursor) + impl = get_user_implementation(cursor) if priv is not None: try: diff --git a/plugins/modules/mysql_user.py b/plugins/modules/mysql_user.py index e02b153..fa54c7d 100644 --- a/plugins/modules/mysql_user.py +++ b/plugins/modules/mysql_user.py @@ -401,7 +401,6 @@ from ansible_collections.community.mysql.plugins.module_utils.mysql import ( ) from ansible_collections.community.mysql.plugins.module_utils.user import ( convert_priv_dict_to_str, - get_impl, get_mode, InvalidPrivsError, limit_resources, @@ -528,8 +527,6 @@ def main(): if session_vars: set_session_vars(module, cursor, session_vars) - get_impl(cursor) - if priv is not None: try: mode = get_mode(cursor) diff --git a/tests/integration/targets/test_mysql_info/tasks/filter_users_info.yml b/tests/integration/targets/test_mysql_info/tasks/filter_users_info.yml index 2c126c1..63ce190 100644 --- a/tests/integration/targets/test_mysql_info/tasks/filter_users_info.yml +++ b/tests/integration/targets/test_mysql_info/tasks/filter_users_info.yml @@ -47,7 +47,7 @@ state: import target: /root/create_procedure.sql - # Use a query instead of mysql_user, because we want to caches differences + # Use a query instead of mysql_user, because we want to catch differences # at the end and a bug in mysql_user would be invisible to this tests - name: Mysql_info users_info | Prepare common tests users community.mysql.mysql_query: @@ -147,6 +147,69 @@ '*CB3326D5279DE7915FE5D743232165EE887883CA' - GRANT SELECT ON users_info_db.* TO users_info_multi_hosts@'host2' + - >- + CREATE USER users_info_tls_none@'host' + IDENTIFIED WITH mysql_native_password AS + '*CB3326D5279DE7915FE5D743232165EE887883CA' REQUIRE NONE + - GRANT SELECT ON users_info_db.* TO users_info_tls_none@'host' + + - >- + CREATE USER users_info_tls_ssl@'host' + IDENTIFIED WITH mysql_native_password AS + '*CB3326D5279DE7915FE5D743232165EE887883CA' REQUIRE SSL + - GRANT SELECT ON users_info_db.* TO users_info_tls_ssl@'host' + + - >- + CREATE USER users_info_tls_cipher@'host' + IDENTIFIED WITH mysql_native_password AS + '*CB3326D5279DE7915FE5D743232165EE887883CA' + REQUIRE CIPHER 'ECDH-RSA-AES256-SHA384' + - GRANT SELECT ON users_info_db.* TO users_info_tls_cipher@'host' + + - >- + CREATE USER users_info_tls_x509@'host' + IDENTIFIED WITH mysql_native_password AS + '*CB3326D5279DE7915FE5D743232165EE887883CA' REQUIRE X509 + - GRANT SELECT ON users_info_db.* TO users_info_tls_x509@'host' + + - >- + CREATE USER users_info_tls_subject@'host' + IDENTIFIED WITH mysql_native_password AS + '*CB3326D5279DE7915FE5D743232165EE887883CA' + REQUIRE SUBJECT '/CN=Bob/O=MyDom/C=US/ST=Oregon/L=Portland' + - GRANT SELECT ON users_info_db.* TO users_info_tls_subject@'host' + + - >- + CREATE USER users_info_tls_issuer@'host' + IDENTIFIED WITH mysql_native_password AS + '*CB3326D5279DE7915FE5D743232165EE887883CA' + REQUIRE ISSUER '/C=FI/ST=Somewhere/L=City/ + O=CompanyX/CN=Bob/emailAddress=bob@companyx.com' + - GRANT SELECT ON users_info_db.* TO users_info_tls_issuer@'host' + + - >- + CREATE USER users_info_tls_subject_issuer@'host' + IDENTIFIED WITH mysql_native_password AS + '*CB3326D5279DE7915FE5D743232165EE887883CA' + REQUIRE SUBJECT '/CN=Bob/O=MyDom/C=US/ST=Oregon/L=Portland' + AND ISSUER '/C=FI/ST=Somewhere/L=City/ + O=CompanyX/CN=Bob/emailAddress=bob@companyx.com' + - >- + GRANT SELECT ON users_info_db.* + TO users_info_tls_subject_issuer@'host' + + - >- + CREATE USER users_info_tls_sub_issu_ciph@'host' + IDENTIFIED WITH mysql_native_password AS + '*CB3326D5279DE7915FE5D743232165EE887883CA' + REQUIRE SUBJECT '/CN=Bob/O=MyDom/C=US/ST=Oregon/L=Portland' + AND ISSUER '/C=FI/ST=Somewhere/L=City/ + O=CompanyX/CN=Bob/emailAddress=bob@companyx.com' + AND CIPHER 'ECDH-RSA-AES256-SHA384' + - >- + GRANT SELECT ON users_info_db.* + TO users_info_tls_sub_issu_ciph@'host' + - name: Mysql_info users_info | Prepare tests users for MariaDB community.mysql.mysql_user: name: "{{ item.name }}" @@ -154,7 +217,7 @@ plugin: "{{ item.plugin | default(omit) }}" plugin_auth_string: "{{ item.plugin_auth_string | default(omit) }}" plugin_hash_string: "{{ item.plugin_hash_string | default(omit) }}" - tls_require: "{{ item.tls_require | default(omit) }}" + tls_requires: "{{ item.tls_requires | default(omit) }}" priv: "{{ item.priv }}" resource_limits: "{{ item.resource_limits | default(omit) }}" column_case_sensitive: true @@ -174,7 +237,7 @@ plugin: "{{ item.plugin | default(omit) }}" plugin_auth_string: "{{ item.plugin_auth_string | default(omit) }}" plugin_hash_string: "{{ item.plugin_hash_string | default(omit) }}" - tls_require: "{{ item.tls_require | default(omit) }}" + tls_requires: "{{ item.tls_requires | default(omit) }}" priv: "{{ item.priv }}" resource_limits: "{{ item.resource_limits | default(omit) }}" column_case_sensitive: true @@ -196,7 +259,7 @@ plugin: "{{ item.plugin | default(omit) }}" plugin_auth_string: "{{ item.plugin_auth_string | default(omit) }}" plugin_hash_string: "{{ item.plugin_hash_string | default(omit) }}" - tls_require: "{{ item.tls_require | default(omit) }}" + tls_requires: "{{ item.tls_requires | default(omit) }}" priv: "{{ item.priv }}" resource_limits: "{{ item.resource_limits | default(omit) }}" column_case_sensitive: true @@ -227,7 +290,7 @@ plugin: "{{ item.plugin | default(omit) }}" plugin_auth_string: "{{ item.plugin_auth_string | default(omit) }}" plugin_hash_string: "{{ item.plugin_hash_string | default(omit) }}" - tls_require: "{{ item.tls_require | default(omit) }}" + tls_requires: "{{ item.tls_requires | default(omit) }}" priv: "{{ item.priv | default(omit) }}" resource_limits: "{{ item.resource_limits | default(omit) }}" column_case_sensitive: true @@ -237,7 +300,9 @@ label: "{{ item.name }}@{{ item.host }}" register: recreate_users_result failed_when: - - recreate_users_result is changed + - >- + recreate_users_result is changed or + recreate_users_result.msg != 'User unchanged' when: - item.name != 'root' - item.name != 'mysql' @@ -265,6 +330,14 @@ - users_info_usage_only - users_info_columns_uppercase - users_info_multi_hosts + - users_info_tls_none + - users_info_tls_ssl + - users_info_tls_cipher + - users_info_tls_x509 + - users_info_tls_subject + - users_info_tls_issuer + - users_info_tls_subject_issuer + - users_info_tls_sub_issu_ciph - name: Mysql_info users_info | Cleanup databases community.mysql.mysql_db: diff --git a/tests/unit/plugins/modules/test_mysql_info.py b/tests/unit/plugins/modules/test_mysql_info.py index 6aaf66e..0d086f4 100644 --- a/tests/unit/plugins/modules/test_mysql_info.py +++ b/tests/unit/plugins/modules/test_mysql_info.py @@ -14,15 +14,15 @@ from ansible_collections.community.mysql.plugins.modules.mysql_info import MySQL @pytest.mark.parametrize( - 'suffix,cursor_output,server_implementation', + 'suffix,cursor_output,server_implementation,user_implementation', [ - ('mysql', '5.5.1-mysql', 'mysql'), - ('log', '5.7.31-log', 'mysql'), - ('mariadb', '10.5.0-mariadb', 'mariadb'), - ('', '8.0.22', 'mysql'), + ('mysql', '5.5.1-mysql', 'mysql', 'mysql'), + ('log', '5.7.31-log', 'mysql', 'mysql'), + ('mariadb', '10.5.0-mariadb', 'mariadb', 'mariadb'), + ('', '8.0.22', 'mysql', 'mysql'), ] ) -def test_get_info_suffix(suffix, cursor_output, server_implementation): +def test_get_info_suffix(suffix, cursor_output, server_implementation, user_implementation): def __cursor_return_value(input_parameter): if input_parameter == "SHOW GLOBAL VARIABLES": cursor.fetchall.return_value = [{"Variable_name": "version", "Value": cursor_output}] @@ -32,6 +32,6 @@ def test_get_info_suffix(suffix, cursor_output, server_implementation): cursor = MagicMock() cursor.execute.side_effect = __cursor_return_value - info = MySQL_Info(MagicMock(), cursor, server_implementation) + info = MySQL_Info(MagicMock(), cursor, server_implementation, user_implementation) assert info.get_info([], [], False)['version']['suffix'] == suffix