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

[DPE-2167] Added admin extra user role #201

Merged
merged 9 commits into from Aug 4, 2023

Conversation

marceloneppel
Copy link
Member

@marceloneppel marceloneppel commented Jul 17, 2023

Issue

There is no admin extra user role that can be requested by client application charms.

Solution

Add only read permissions to the special admin role. The permission to create anything in the public schema of the postgres database was removed.

Also add to it the permission to create and drop databases (except the system database postgres).

While reviewing, the following files can be ignored (they are generated):

  • poetry.lock
  • requirements.txt
  • tests/integration/ha_tests/application-charm/requirements.txt
  • tests/integration/new_relations/application-charm/requirements.txt

We disabled the hashes temporarily on all the requirements.txt files due to [canonical/charmcraft#1179] (so we can pack the charms now). I will be reverted later.

Fixes #192.

]
if extra_user_role not in roles and extra_user_role != "admin"
}
if "SUPERUSER" in map(str.upper, privileges):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think PGB is using superuser. We should change that to admin after this change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I reverted this part of the code as I realised that Landscape needs a superuser (for plpython extension, a requirement that may or may not be removed at some time, but also due to some schema changes that may happen in a landscape server upgrade).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. we should document this properly. JFMI, which extra_user_role used by pgbouncer?
  2. re: plpython. We will have to hide it for now (c) Mohamed. Security concerns. This is why it was missing on mid-sync presentation. Mohamed will create a ticket to discuss and hide in documentation for now. No rush necessary today!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PgBouncer currently uses SUPERUSER (to be able to create other superusers).

Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
What about to improve https://charmhub.io/postgresql-k8s/docs/e-users to expand "Relation/integration users" with description of extra_user_roles (internal and allowed o be used "admin". BTW, if no extra-user-roles requested, how do we call this "role"? :-) )

lib/charms/postgresql_k8s/v0/postgresql.py Show resolved Hide resolved
lib/charms/postgresql_k8s/v0/postgresql.py Outdated Show resolved Hide resolved
@@ -347,15 +347,21 @@ def set_up_database(self) -> None:
"""Set up postgres database with the right permissions."""
connection = None
try:
self.create_user(
"admin",
extra_user_roles="pg_read_all_data,pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables,pg_monitor,pg_signal_backend",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would limit privs by read and write all databases.

Suggested change
extra_user_roles="pg_read_all_data,pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables,pg_monitor,pg_signal_backend",
extra_user_roles="pg_read_all_data,pg_write_all_data",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated on 3b95919.

@marceloneppel
Copy link
Member Author

Thank you! What about to improve https://charmhub.io/postgresql-k8s/docs/e-users to expand "Relation/integration users" with description of extra_user_roles (internal and allowed o be used "admin". BTW, if no extra-user-roles requested, how do we call this "role"? :-) )

Good suggestion. I added the following text to both https://discourse.charmhub.io/t/charmed-postgresql-k8s-explanations-users/10843 and https://discourse.charmhub.io/t/charmed-postgresql-explanations-users/10798:

When an application charm requests a new user through the relation/integration it can specify that the user should have the admin role in the extra_user_roles field. The admin role enables the new user to read and write to all databases (for the postgres system database it can only read data) and also to create and delete non-system databases.

@marceloneppel marceloneppel merged commit 343a236 into main Aug 4, 2023
28 checks passed
@marceloneppel marceloneppel deleted the dpe-2167-admin-extra-user-role branch August 4, 2023 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error on use of extra_user_roles="admin" with DatabaseRequires
4 participants