Column Level Privileges are capitalized when applied to DB instance

[kmarse]

Please raise issues via the new interface
Currently I use the mysql-roles module to manage all app roles in our db instances. I have some column level privileges to a larger table that I don't want to grant access to the whole table to some roles. I have been receiving access denied errors now because when the mysql-roles module applied the grants it capitalized the column names.

In MySQL's documentation I have found that it states that column level privileges are not case sensitive but after speaking to an oracle support rep he has confirmed that they are.

I am confused on why the module when inputting the grants for column level privileges is uppercasing the column name and not leaving it the way I have it in my playbook example follows:

What I have in my playbook:
'database.table': 'SELECT(app_type, rec_id)'

What MySQL is returning after application:
GRANT SELECT (APP_TYPE, REC_ID) ON database.table TO role@%

[Andersson007]

@kmarse hello, thanks for reporting this!
I've taken a quick look at the code and i see that the issue is deeper and broader than just uppercasing the input as it relates to the way the collection handles priv-related data fetched from a DB- it just uppercases everything. The module is relatively new but it uses the old mysql_user's code.
I'll try to make time to find a solution but if someone wants to pick it up, please go ahead.

[kmarse]

@Andersson007 Thanks for getting back to me on this.

I did some investigating into the different modules and I don't see anywhere currently that it would be uppercasing the priv variable. Is there a place of reference you could point me to so that I can test a solution?

[kmarse]

@Andersson007 I ran some tests and found the issue which I have resolved locally.

The issue was found in user.py on line 644 which performs an upper() function on the entire privilege statement including column_names.

output[pieces[0]] = re.split(r',\s*(?=[^)]*(?:(|$))', pieces[1].upper())

This causes 'SELECT(app_type, rec_id)' to become 'SELECT(APP_TYPE, REC_ID)'. Privileges are reserved words in MySQL which means that SELECT will always be capitalized. I do understand that if some one in their privs variables are not passing uppercase privileges then ansible would report a change in grants every single time because the module would see GRANT select (app_type, rec_id) ON db.table TO user@host; but mysql databases would return when queried GRANT SELECT (app_type, rec_id) ON db.table TO user@host;

I believe that removing upper() to make sure that we are not breaking grants by forcing column_names to be uppercased. We should then handle uppercasing the SELECT(privileges) separate from the column names which I believe could be done by splitting the privilege(SELECT) from the column names then uppercase it and concatenate them and alter the privs.

Let me know if you want me to create a PR just removing the upper or if you want to look into making it more robust.

[Andersson007]

@kmarse thanks for the feedback! Your investigation is correct.

Let me know if you want me to create a PR just removing the upper

I think it would be a very dangerous path and definitely a breaking one

or if you want to look into making it more robust.

Considering the importance of the module and this particular feature, yep, I'm personally in favor of robust and non-breaking solutions:)

[kmarse]

@Andersson007 I wanted to give you an update on this as I have not had time to look for a more robust way but to provide you what mysql/oracle have updated me on through support tickets.

After talking to a support representative from MySQL/Oracle I have verified that column level grants although in their documentation, which states "For access-checking purposes, comparisons of User, Proxied_user, authentication_string, Db, and Table_name values are case-sensitive. Comparisons of Host, Proxied_host, Column_name, and Routine_name values are not case-sensitive." I was told by the rep that, " in order to evaluate the privilege as part of MySQL authorization system, MySQL compares the column columns_priv.column_name of the mysql system table."

This means that because I have a column named "first_name" and I want to grant select on just that column if I try to grant that in the module with the following privilege:

database.person: 'SELECT(first_name)'

When it goes through the module it would show as the following grant:

GRANT SELECT(FIRST_NAME) ON database.person TO user@host;

This then if I tried to query that column with that user would give me a SELECT COMMAND DENIED error because when it reads from columns_priv.column_name the column_name is lowercased which doesn't match and then in turn is not valid. From the updates I have received from MySQL/Oracle I feel more justified that my solution above is correct. The module should not be uppercasing any columns because that is the responsibility of the DBA/DBE to make sure that their yml files include the correct case sensitive typed column names so that the grant statement can be validated by MySQL correctly.

After reviewing your code some more I understand what you are uppercasing. How I understand it is that with your code you are trying to make sure the all reserved words are uppercased like SELECT, INSERT, UPDATE, DELETE. But because column level grants are defined inline with these words in the grant statement like 'SELECT(first_name)' you are wanting to make sure you are uppercasing the the SELECT and by default you are uppercasing the column names as well (which is causing errors in the mysql priv evaluation. In my mind there are two solutions here.

1. Remove the upper entirely from your code. I have removed the uppercase from my local code repository and have been running this module in 4 different environments for 5 months now with nothing breaking so I don't believe that this is a breaking change to your module. This works because the reserved words are not case sensitive meaning the following statements are equivalent:
   GRANT SELECT ON *.* TO user@host;
grant select on *.* to user@host;

2. The second solution would be using regex to extract all column names so that you are only uppercasing the privileges and not the column names which although robust seems to be unnecessary work since mysql handles it all correctly on their end.

They did open up a bug to update the documentation I believe which I am still investigating to see if that is the case so that all users of MySQL are not thinking that column level privileges are case insensitive.

Also I have removed the above code from my local code repository and have been running this module in 4 different environments for 5 months now with nothing breaking so I don't believe that this is a breaking change to your module. I can still submit a pull request but would like feedback from you.

[Andersson007]

@kmarse thanks for the detailed feedback!

i would highlight that this code had been introduced before i came:) The mysql_role module uses code shared with the old mysql_user module..
i know that there's no difference in things like GRANT and grant and I absolutely agree that this should be a DBA's concern.

Also I have removed the above code from my local code repository and have been running this module in 4 different environments for 5 months now with nothing breaking so I don't believe that this is a breaking change to your module. I can still submit a pull request but would like feedback from you.

I meant it can be breaking as users in their current / old playbooks can expect that whatever DB entity identifier they use (even wrong) does not depend on a case. So if we decide to go this way, i'd prefer the changes will be made in a planned major release.
Though as I'm not a user, it's hard to trust my judgment

@kmarse @bmalynovytch @laurent-indermuehle @rsicart thoughts?

[laurent-indermuehle]

I rarely use the mysql_user module. I'm not in a position to have an opinion yet.

[kmarse]

@Andersson007

I completely understand why this is a breaking change and should go out in a major release. Unfortunately after looking into the code some more I don't see a possible fix for a patch because if someone is dependent on the uppercasing of the columns to handle the matching of their uppercased columns in their db schema then unless done through a flag/boolean variable that we default to use the old code of uppercasing everything and then if the user sets their default value to false then it would skip that uppercasing.

Other than that fix I can't really think of another way without it going out in a major release.

Thanks for you feedback and let me know if there is anything I can do to help. I am more than willing to try and get a solution to you all.

[Andersson007]

@kmarse thanks for investigating the case!
Usually we try to avoid new parameters as it makes the module's interface harder to understand and use..

However, considering the importance of the related code, in this particular case,
I think it's acceptable and much safer to add the new boolean parameter you mentioned with a default behavior like it is now.
We could add a warning if it's not set explicitly for users to set it.
Then, in the next major release, we could change the default to the new behavior (w/o uppercasing).
In a long term perspective we could think about getting rid of the uppercasing entirely if it works well w/o it.

Would be great to hear opinions of you all on the plan: @kmarse @laurent-indermuehle @rsicart @bmalynovytch @betanummeric @Jorge-Rodriguez

[laurent-indermuehle]

I think we should review the tests to be sure that we cover all possible cases. And then try to remove the "upper". I say that because I almost never use capital letters and MySQL/MariaDB understand me without issues.

[Andersson007]

I think we should review the tests to be sure that we cover all possible cases. And then try to remove the "upper". I say that because I almost never use capital letters and MySQL/MariaDB understand me without issues.

@laurent-indermuehle thanks for the feedback! Agreed.
FYI I'll be on PTO since Monday until NY.

@kmarse if you're OK with #399 (comment) and If no opposition to the plan util Monday, i think we can proceed with the boolean with default as it is not.
Don't care about its name, we can discuss and decide on it later, after a PR is submitted.

What can help you.
Guides:

• Quick start guide
• Quick start integration test guide

Instruqt labs:

• https://play.instruqt.com/redhat/tracks/ansible-developer-environment
• https://play.instruqt.com/redhat/tracks/community-fixing-a-bug
• https://play.instruqt.com/redhat/tracks/adding-integration-tests-to-ansible-collections

Though I hope the rest of the team (@rsicart @bmalynovytch @betanummeric @Jorge-Rodriguez) will share their feedback until Monday. If not, feel free to proceed.
If any questions, just ask here, we'd be happy to help.

[kmarse]

@Andersson007 I apologize for the extremely late response. I will start to proceed with a request on this to add a boolean value with the default behavior being that of current functionality and then the ability to change the boolean to disable the capitalization of column level privileges.

Are you wanting this to be a known boolean that other users are able to change as well? Meaning updating documentation?

[Andersson007]

@kmarse no worries

Are you wanting this to be a known boolean that other users are able to change as well? Meaning updating documentation?

Sure, any new features should be documented + would be nice to have an example. The module's file contains both the sections

[kmarse]

@Andersson007 thanks. I have finished the code and am working on documentation and then trying to work through creating unit testing for this.

[Andersson007]

@Andersson007 thanks. I have finished the code and am working on documentation and then trying to work through creating unit testing for this.

@kmarse thanks! if there are any questions/unclear things, we'll be happy to clarify them

[kmarse]

@Andersson007 I wrote my test based on previous tests that you have written. But no matter what I input into the unit test I am not seeing output changes when I run the unit tests can you give me advice on this?

Unit Test:
@pytest.mark.parametrize(
'input_,output,mode,column_case_sensitive',
[
('SELECT (a)', 'SELECT (a)', 'NOTANSI', True),
('SELECT (a)', 'SELECT (a)', 'NOTANSI', True),
('UPDATE (b, a)', 'UPDATE (a, b)', 'NOTANSI', True),
('INSERT (a, b)', 'INSERT (a, b)', 'NOTANSI', True),
('REFERENCES (b, a)', 'REFERENCES (a, b)', 'NOTANSI', True),
('SELECT (b, a)', 'SELECT (a, b)', 'NOTANSI', True),
('SELECT (b, a, c)', 'SELECT (a, b, c)', 'NOTANSI', True),
('SELECT (A)', 'SELECT (A)', 'NOTANSI', True),
('SELECT (A)', 'SELECT (A)', 'NOTANSI', True),
('UPDATE (B, A)', 'UPDATE (A, B)', 'NOTANSI', True),
('INSERT (A, B)', 'INSERT (A, B)', 'NOTANSI', True),
('REFERENCES (B, A)', 'REFERENCES (A, B)', 'NOTANSI', True),
('SELECT (B, A)', 'SELECT (A, B)', 'NOTANSI', True),
('SELECT (B, A, C)', 'SELECT (A, B, C)', 'NOTANSI', True),
('SELECT (a)', 'SELECT (A)', 'NOTANSI', False),
('SELECT (a)', 'SELECT (A)', 'NOTANSI', False),
('UPDATE (b, a)', 'UPDATE (A, B)', 'NOTANSI', False),
('INSERT (a, b)', 'INSERT (A, B)', 'NOTANSI', False),
('REFERENCES (b, a)', 'REFERENCES (A, B)', 'NOTANSI', False),
('SELECT (b, a)', 'SELECT (A, B)', 'NOTANSI', False),
('SELECT (b, a, c)', 'SELECT (A, B, C)', 'NOTANSI', False),
]
)
def test_column_case_sensitive_grants(input_, mode, column_case_sensitive, output):
"""Tests privileges_unpack function."""
assert privileges_unpack(input_, mode, column_case_sensitive) == output

Unit Test Command:
ansible-test units tests/unit/plugins/modules/test_mysql_role.py --docker

Unit Test output:
`Unit test modules with Python 3.11
============================= test session starts ==============================
platform linux -- Python 3.11.0, pytest-7.1.3, pluggy-1.0.0
rootdir: /root/ansible_collections/community/mysql, configfile: ../../../ansible/test/lib/ansible_test/_data/pytest/config/default.ini
plugins: forked-1.4.0, xdist-2.5.0, mock-3.10.0
gw0 I / gw1 I / gw2 I / gw3 I / gw4 I / gw5 I
gw0 [32] / gw1 [32] / gw2 [32] / gw3 [32] / gw4 [32] / gw5 [32]

................................ [100%]

• generated xml file: /root/ansible_collections/community/mysql/tests/output/junit/python3.11-modules-units.xml -
  ============================== 32 passed in 0.37s ==============================`

I only posted one of the outputs as they all look the same.

[Andersson007]

@kmarse if you submit a PR with the changes and tests, it'd be easier for me to review (if you haven't done that before, Quickstart guide can help)

[laurent-indermuehle]

@kmarse you were close for the units tests. If ansible-tests returns nothing it means the tests pass.

I just added one based on your feedback: 11430f0

When I un-capitalize one column name, it immediately yell:

ansible-test units --docker -v --python 3.11 tests/unit/plugins/module_utils/test_mysql_user.py

>>>
priv = 'mydb.*:SELECT (b, a, c)', mode = 'NOTANSI'
column_case_sensitive = False, ensure_usage = False
expected = {'`mydb`.*': ['SELECT (A, b, C)']}

    @pytest.mark.parametrize(
        'priv,expected,mode,column_case_sensitive,ensure_usage',
        [
            ('mydb.*:SELECT (b, a, c)', {'`mydb`.*': ['SELECT (A, b, C)']}, 'NOTANSI', False, False),
        ]
    )
    def test_privileges_unpack(priv, mode, column_case_sensitive, ensure_usage, expected):
        """Tests privileges_unpack function."""
>       assert privileges_unpack(priv, mode, column_case_sensitive, ensure_usage) == expected
E       AssertionError: assert {'`mydb`.*': ...T (A, B, C)']} == {'`mydb`.*': ...T (A, b, C)']}
E         Differing items:
E         {'`mydb`.*': ['SELECT (A, B, C)']} != {'`mydb`.*': ['SELECT (A, b, C)']}
E         Full diff:
E         - {'`mydb`.*': ['SELECT (A, b, C)']}
E         ?                           ^
E         + {'`mydb`.*': ['SELECT (A, B, C)']}
E         ?                           ^

tests/unit/plugins/module_utils/test_mysql_user.py:113: AssertionError

This was my first experience with unit tests. Thank you for making me learning something new today :)

All integrations tests are now passing. I'm not sure they are the best thought. I discovered that only mysql 5.7 is case sensitive. Either MySQL 8 and MariaDB doesn't care and grant access no matter the case.

But, I think you're right @kmarse and setting column_case_senstive to True is the best thing to do. I already announced that this will be the default starting with 4.0.0 when we release it, here and here

Thanks you for your contribution! Feel free to comment on the PR but don't wait too much, I'll merge it soon to get back to #572.

[Andersson007]

@kmarse @laurent-indermuehle thanks for the contribution!

[laurent-indermuehle]

Hi @kmarse,
Every day I receive a message from Github action coming from your fork. This is because we have scheduled a job to run all tests and you inherited from it.
Could you please do something to stop these failing runs? Either update your fork or disable the schedule CI runs? Feel free to asks for help. Thanks!