Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have postgresql_privs update all privileges #387

Open
andreasscherbaum opened this issue Dec 12, 2022 · 12 comments
Open

Have postgresql_privs update all privileges #387

andreasscherbaum opened this issue Dec 12, 2022 · 12 comments

Comments

@andreasscherbaum
Copy link

This is a Request for Comments.

SUMMARY

Currently the postgresql_privs module can only revoke privileges when an extra task is created.

We have a large number of databases and tables, and managing privileges using the postgresql_privs module is complicated and cumbersome.
Especially removing privileges requires two steps:

  1. Update the existing postgresql_privs task and remove the revokes privileges
  2. Add a second task which revokes the privileges in the database

In addition to above problems, we would rather have the privileges listed in the inventory, not as tasks scattered across multiple places.

ISSUE TYPE

The idea is to extend the postgresql_privs module with a couple new options - or create a new module with the proposed functionality.

  • set_privs: receives a list of all privileges for the table, when specified this will update all privileges for objs to what is listed as argument, including all revokes
    • this option clashes with the following options: state, privs, roles, grant_option, target_roles
    • initially maybe limited to type = table
  • ignore_superuser: will not revoke privileges which are set for any database superuser (default = false)
  • ignore_owner: will not revoke privileges which are set for any object owner (default = false)
COMPONENT NAME

This is either an extension for the postgresql_privs module, or since this proposal clashes with required options like roles, it might be better to move this into a separate module like postgresql_set_privs.

Another option to discuss: allow to skip the objs parameter and specify object names as part of set_privs.

ADDITIONAL INFORMATION
@hunleyd
Copy link
Collaborator

hunleyd commented Dec 12, 2022

We recently accepted a PR for the hba module that added an overwrite option that feels very similar to this, so I personally don't see a need for a new module. What do you think @Andersson007 ?

@andreasscherbaum
Copy link
Author

Hmm, interesting approach, didn't think about this. However: how does this manage "all in one", as in: "different roles with different privileges on the same object"? I think this still would need a new parameter specifying all privileges in a condensed form. Or overload privs, but that will become problematic.

@betanummeric
Copy link
Member

betanummeric commented Dec 15, 2022

For comparison, the community.mysql.mysql_user module has one option priv to specify privileges on objects. The module has three ways of handling these:

  • replacing (default): grant the specified privileges, revoke other privileges on the same objects
  • grant-only (append_privs: true): grant the specified privileges, ignore others
  • revoke-only (subtract_privs: true): revoke the specified privileges, ignore others

The grant-only mode translates to the state: present mode of the postgresql_privs module, the revoke-only mode to state: absent. The replacing mode postgres module has no counterpart for the replacing mode, which I understand this RFC to be about.

We could introduce a state: exact (feel free to suggest a better name) to the postgresql_privs module to offer a replacing mode.

I think the ignore_superuser and ignore_owner arguments could be useful. They are rather independent of the replacing mode.

Further, a simpler, better scriptable way of specifying privileges (maybe a list of dicts) would help integration in high-level automation. But this would be a breaking change and a big effort.

@andreasscherbaum
Copy link
Author

If state: exact is introduced, what does state: present do if provided with a list of privileges, instead of a text like today? Fail/reject? Just pick the additions? Feel like backwards compatibility is important here.

@Andersson007
Copy link
Collaborator

folks, missed the end of the discussion as was on PTO at that time.
what do other contributors think on @betanummeric 's suggestion and @andreasscherbaum 's question?

@hunleyd
Copy link
Collaborator

hunleyd commented Feb 10, 2023

i feel like the mysql module makes the most sense in my mind, so i'd like to see us move towards that. as for backward compat, if we make this change part of the 3.0 milestone, i think that is less of a concern.

@Andersson007
Copy link
Collaborator

i feel like the mysql module makes the most sense in my mind, so i'd like to see us move towards that. as for backward compat, if we make this change part of the 3.0 milestone, i think that is less of a concern.

@hunleyd if we gonna introduce breaking changes, i believe 3.0 is imo coming too soon (i hope until summer but several months is too soon), i would suggest targeting at least version 4 but it depends on when we'll make the final decision and announce it.

@andreasscherbaum
Copy link
Author

May I ask which MySQL module you refer to?
Imho the Ansible MySQL collection has no module to manage tables, or table permissions.

Or, more specific: which exact feature do you have in mind which this module should replicate in PostgreSQL?

@hunleyd

@hunleyd
Copy link
Collaborator

hunleyd commented Feb 13, 2023

I'm referring to this comment @andreasscherbaum

@andreasscherbaum
Copy link
Author

If I read the past comments correct, state: present does not introduce any breaking changes.
The addition would be something like state: exact.

The only question is here: what if privs comes as a list or dict (today it's only a string), but state is default or present? The module can try to extract the new privileges, or simply reject everything. I'm opting for the latter, as this would be against today's documentation.

An alternative is to use set_privs instead of privs for state: exact. And throw an error if the arguments are not what is expected.

Summary:

  • state: present works as of today
  • state: exact uses privs but expects a structure with the exact privileges for the objects in objs
  • ignore_superuser flag (default=false) will not revoke any privileges which are granted for any superuser
  • ignore_owner flag (default=false) will not revoke any privileges which are granted for the owner of the object

Would also like to roll this out in steps, starting with tables and schema.

@betanummeric
Copy link
Member

We can change independently how to specify

  • what objects shall be acted on
  • what privileges shall be acted on
  • how those things shall be treated (scope of this issue)

Further, a simpler, better scriptable way of specifying privileges (maybe a list of dicts) would help integration in high-level automation. But this would be a breaking change and a big effort.

This idea was about how privileges can specified, not how actions are specified. So it's rather out of scope for this issue.
To clarify the proposal:

  • bigger change: we could use a dict to nest privileges below the database objects
  • more simple change: We could support specifying privileges as a list instead of a string quite easily. So not only allow "SELECT,INSERT", but also ["SELECT", "INSERT"]. Different syntax, same behavior.

Regardless of notation of objects and privileges, it works / would work like that:

  • state: present (default), all specified privileges are granted (added)
  • state: absent, all specified privileges are revoked (subtracted)
  • state: exact (proposed)
    • all specified privileges are granted (added)
    • and all existing, unspecified privileges are revoked (implicit subtraction)

@Andersson007
Copy link
Collaborator

I'm not a user and my nickels would be:

  • let's avoid breaking changes whenever possible
  • let's do things gradually, i.e. feature by feature in separate PRs. I like the state: exact proposal, btw we already have this in postgresql_membership, SGTM. so let's start with it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants