community.postgresql.postgresql_set incorrectly quotes strings containing commas

[nergdron]

just tested the latest release (1.2.0) since it was only released 15h ago, devel doesn't have any notable changes i can see which would affect this bug.

SUMMARY

when using postgresql_set with a string value that contains a comma, for some reason it puts extra doublequotes (") around the string, breaking it when it's included in postgresql.auto.conf:

password_encryption = 'scram-sha-256'
shared_preload_libraries = '"pg_stat_statements,repmgr"'

ISSUE TYPE

• Bug Report

COMPONENT NAME

community.postgresql.postgresql_set

ANSIBLE VERSION

ansible 2.10.6
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/tessa/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/tessa/.local/lib/python3.9/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.9.2 (default, Feb 20 2021, 18:40:11) [GCC 10.2.0]

CONFIGURATION

OS / ENVIRONMENT

EndeavourOS (Arch variant) running Ansible from system ansible package, version 3.1.0.

STEPS TO REPRODUCE

in role defaults/main.yml:

postgres:
  config:
    password_encryption: scram-sha-256
    shared_preload_libraries: pg_stat_statements,repmgr

and in role tasks/main.yml

- name: configure postgresql
  community.postgresql.postgresql_set:
    name: "{{ item.key }}"
    value: "{{ item.value }}"
  become_user: postgres
  loop: "{{ postgres.config | dict2items }}"
  notify: restart postgres

EXPECTED RESULTS

keys with values that contain commas get correctly rendered out to postgresql and entered into the postgresql.auto.conf without extraneous quotes.

ACTUAL RESULTS

for some reason module renders values with commas with extra double quotes around them, breaking postgres config. I've verified that if I change the above config line to the following, it works correctly and doesn't inject extra quotes:

    shared_preload_libraries: repmgr

interestingly, ansible doesn't show the extra quotes in the command output, they only show up in the destination file itself:

TASK [postgres : configure postgresql] ********************************************
changed: [mgmt1] => (item={'key': 'password_encryption', 'value': 'scram-sha-256'})
changed: [mgmt1] => (item={'key': 'shared_preload_libraries', 'value': 'pg_stat_statements,repmgr'})
[...]
$ cat /var/lib/postgresql/12/main# cat postgresql.auto.conf
[...]
password_encryption = 'scram-sha-256'
shared_preload_libraries = '"pg_stat_statements,repmgr"'

[Andersson007]

@nergdron hi, thanks for reporting this! CC @tcraxs @andytom @kostiantyn-nemchenko @sebasmannem @ilicmilan @popov83 If anyone wants to fix this, please put something here first ASAP

[Andersson007]

and folks, now we have this step-by-step guide how to setup everything and submit a PR very quickly. Would be happy to review

[Andersson007]

@nergdron it's not the module. I ran in psql the following command: postgres=# alter system set shared_preload_libraries = 'pg_stat_statements,repmgr';

And then checked postgresql.auto.conf: shared_preload_libraries = '"pg_stat_statements,repmgr"' So PostgreSQL added the extra quotes.

It's a bug of PostgreSQL https://www.postgresql.org/message-id/10860.1438980591%40sss.pgh.pa.us

[Andersson007]

We can't fix it on the module's side. So i think the issue should be closed. Anyway, @nergdron thanks for reporting! if anyone comes across this in the future my investigation will be helpful I hope.

[Andersson007]

I'll close the issue then because of my investigations above, we can open it later if needed. Thanks you!

[hubiongithub]

Hello I'm not convinced this is a postgresql bug (the link above shows only that postgresql behaves that way) I tried the following:

postgres=# alter system set shared_preload_libraries = pg_stat_statements,pgaudit,pgauditlogtofile;
ALTER SYSTEM
postgres=# alter system set shared_preload_libraries = 'pg_stat_statements,pgaudit,pgauditlogtofile';
ALTER SYSTEM
postgres=# alter system set shared_preload_libraries = 'pg_stat_statements','pgaudit','pgauditlogtofile';
ALTER SYSTEM

ang get accordingly:

grep shared_preload postgresql.auto.conf
shared_preload_libraries = 'pg_stat_statements, pgaudit, pgauditlogtofile'
grep shared_preload postgresql.auto.conf
shared_preload_libraries = '"pg_stat_statements,pgaudit,pgauditlogtofile"'
grep shared_preload postgresql.auto.conf
shared_preload_libraries = 'pg_stat_statements, pgaudit, pgauditlogtofile'

So only if we send the list in quotes to the alter statement, it will get extra quotes, if we send the list without quotes OR a list of comma seperated quoted strings it will be ok in postgresql.auto.conf

plugins/modules/postgresql_set.py does:

def param_set(cursor, module, name, value, context):
    try:
        if str(value).lower() == 'default':
            query = "ALTER SYSTEM SET %s = DEFAULT" % name
        else:
            query = "ALTER SYSTEM SET %s = '%s'" % (name, value)
        cursor.execute(query)

So it adds single quotes around the value parameter (which I assume is wanted and needed for most settings(?)) We could try to send query = "ALTER SYSTEM SET %s = %s" % (name, value)" for only shared_preload_libraries to prevent the additional " from postgresql. As the link above shows a mail from 2015 with no followup so I assume that side won't move.

[Andersson007]

@hubiongithub hello, thanks for investigating! would you like to submit a PR?

[hubiongithub]

@Andersson007 Hello I'm not sure if this would break other stuff, not quoting/checking input could lead to other problems (can we break stuff by send sql injection like stuff to this special shared_preload_libraries handling if we do not use quotes here?) Perhaps we need a list parsing piece of code here for pg settings which can accept comma separated lists instead of single strings. (Are there other pg settings accepting list other than shared_preload_libraries?) Something that on the input side accept variable: "a, b, c" in host_vars .yaml files and parse it to 'a','b','c' which the code then generates to something "ALTER SYSTEM SET %s = '%s','%s", ... % (name, value1,...). depending on how many list items are given.But I don't know how to code this in python esp. in a secure way or if it is a good idea to do so.

[Andersson007]

@hubiongithub good questions, thanks for raising them!

We could try to send query = "ALTER SYSTEM SET %s = %s" % (name, value)" for only shared_preload_libraries to prevent the additional " from postgresql.

can we break stuff by send sql injection like stuff to this special shared_preload_libraries handling if we do not use quotes here?

@hunleyd @jchancojr what do you think? I think in the following implementation the code anyway isn't safe in terms of SQL injections or i'm wrong?

• The simplest and safest way is to hardcode shared_preload_libraries (only if it's the only param where we can pass a list) OR
• We could try to split value by commas if there are commas, add ' and join the result.

Thoughts?

[hubiongithub]

Hello a simple but not full list of parameters with comma separated lists as values derived from the reset value in pg_settings:

postgres=# select name,vartype,reset_val from pg_settings where reset_val like '%,%';
           name           | vartype |                   reset_val                   
--------------------------+---------+-----------------------------------------------
 DateStyle                | string  | ISO, DMY
 search_path              | string  | "$user", public
 shared_preload_libraries | string  | pg_stat_statements, pgaudit, pgauditlogtofile

search_path is dypically set at role/user level so this might fly under the radar of postgresql_set most of the time but DateStyle might be used instance wide (never touched it myself)

There are 74 (PG 14) parameters of type 'string', one good candidate would be local_preload_libraries listen_addresses: The value takes the form of a comma-separated list of host names and/or numeric IP addresses

Some extensions also have such parameters, e.g. pg_audit.log (Multiple classes can be provided using a comma-separated list)

So definitely "more than one", but I'm not sure if these all get mangled up by quoting them.

[hunleyd]

We could try to split value by commas if there are commas, add ' and join the result.

this seems reasonable as there is definitely >1 parameter that takes a comma-separated list

[Andersson007]

@hunleyd @hubiongithub thanks!

@hubiongithub would you like to submit a PR? If you haven't done it before, we have the Quick-start guide. Please let us know

[hubiongithub]

@Andersson007 for a PR I probably would need to write code for " try to split value by commas"? (which I doubt will end well)

[Andersson007]

@hubiongithub i think you could use something like:

if ',' in value:
  value = ','.join(["'" + elem.strip() + "'" for elem in value.split(',')])

as

>>> value = 'one, two, tree'
>>> 
>>> value = ','.join(["'" + elem.strip() + "'" for elem in value.split(',')])
>>> value
"'one','two','tree'"

maybe it can be optimized or there's another better solution

[Andersson007]

@hubiongithub what do you think? ^ If you have no time for that, please let us know

[hubiongithub]

@Andersson007 As I'm not that experienced in writing python it's probably faster is someone with that skills do it. Additionally the PR part isn't working great, I have one open in community.mysql which ends up in errors for tests I don't understand how to get along with.

[Andersson007]

I created a PR https://github.com/ansible-collections/community.postgresql/pull/357, ready for review

[Andersson007]

@nergdron thanks for reporting the issue! @hubiongithub @hunleyd thanks for reporting and reviewing it! @hunleyd thanks for reviewing and merging the PR!

[RealGreenDragon]

The PR #357 fix the multi-value parameters management (issue #78), but the new check assumes as multi-value each parameter with a comma in the value, that is incorrect as there are single-value parameters with comma in value.

If a single-value parameter is treated as a multi-value, the 'param_set' function builds an ALTER SYSTEM SET command with multiple comma-separated values, that fails with the message:

ERROR:  SET log_line_prefix takes only one argument

Single-value parameters with possible comma in value are "_command" and "_prefix" that for exmple in v12 are:

• ssl_passphrase_command
• archive_command
• restore_command
• archive_cleanup_command
• recovery_end_command
• log_line_prefix

The most critical (that I add in tests) is 'log_line_prefix' because often it contains comma and a space at the end.

I simply fix the check to evaluate as single-value all parameters that ends with '_command' and '_prefix'.

The change is contained in PR #400 .

[Legushka]

ALTER SYSTEM SET "shared_preload_libraries" TO '' gives the result in the file postgresql.auto.conf shared_preload_libraries = '""' WHICH WILL LEAD TO A CRASH