Skip to content

Commit 86ced4d

Browse files
marceloneppelmarceloneppel
authored
[DPE-7628] Fix permissions for predefined roles (#1003)
* Fix permissions for charmed_read and charmed_dml roles Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Add one more check in charmed_dml test and add test for both charmed_read and charmed_dml roles Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Check escalation possibility for predefined roles Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Add roles inheritance Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Add test configuration and setup Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix charmed_stats user permissions, increase timeout and delete database related users Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix connect privilege for charmed_read, charmed_dml, charmed_dba and charmed_databases_owner users Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Change admin user to charmed_admin and convert test to use jubilant Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Add initial connection test Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Major refactor of predefined roles and their connection tests Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Add sleep for HBA update Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix assertion Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix user existence check Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Check user auto escalation to database owner user Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix more permissions and add more checks Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Change CREATEDB auto escalation Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Block forbidden extra user roles combinations Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix blocked status check in invalid roles test Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix some linting with a new enum Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Test dropping owned and not owned databases Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Reduce complexity of helper function Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Test charmed_dba role not allowed Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Test pg_monitor not allowed and rename test file Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Check more CRUD operations Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Started to test charmed_dba role Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Test set_user function call Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix linting Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Test views creation Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Test escalation to database owner user Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Check charmed_stats role permissions Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Check read on views Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix charmed_backup role permisions Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix linting Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Block user requesting system databases Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Remove template0 from tested databases Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Avoid relation group in error message Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Allow connection to system databases Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Delete old tests Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Check if pg_hba temporary table exists before trying to delete it Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Don't log exception as error message is already logged Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Change event trigger function to be a security definer Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Check if relation_users temporary table exists before trying to delete it Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix pg_temp tables drop Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix test app channel in audit test Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Create schema to calll event trigger Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix check for invalid database name Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix mechanism that resolves blocked status after relation removal Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Cap database name at 49 characters to avoid issues when creating database level roles Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Handle None database name in relation broken event Signed-off-by: Marcelo Henrique Neppel <[email protected]> --------- Signed-off-by: Marcelo Henrique Neppel <[email protected]>
1 parent 94f31bd commit 86ced4d

File tree

17 files changed

+1648
-748
lines changed

17 files changed

+1648
-748
lines changed

‎lib/charms/postgresql_k8s/v1/postgresql.py

Lines changed: 157 additions & 57 deletions
Large diffs are not rendered by default.

‎src/relations/postgresql_provider.py

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
from charms.postgresql_k8s.v1.postgresql import (
1515
ACCESS_GROUP_RELATION,
1616
ACCESS_GROUPS,
17+
INVALID_DATABASE_NAME_BLOCKING_MESSAGE,
1718
INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE,
1819
PostgreSQLCreateDatabaseError,
1920
PostgreSQLCreateUserError,
@@ -113,7 +114,7 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None:
113114

114115
password = new_password()
115116
self.charm.postgresql.create_user(
116-
user, password, extra_user_roles=extra_user_roles, in_role=f"{database}_admin"
117+
user, password, extra_user_roles=extra_user_roles, database=database
117118
)
118119

119120
# Share the credentials with the application.
@@ -138,11 +139,14 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None:
138139
PostgreSQLCreateUserError,
139140
PostgreSQLGetPostgreSQLVersionError,
140141
) as e:
141-
logger.exception(e)
142142
self.charm.set_unit_status(
143143
BlockedStatus(
144144
e.message
145-
if issubclass(type(e), PostgreSQLCreateUserError) and e.message is not None
145+
if (
146+
issubclass(type(e), PostgreSQLCreateDatabaseError)
147+
or issubclass(type(e), PostgreSQLCreateUserError)
148+
)
149+
and e.message is not None
146150
else f"Failed to initialize relation {self.relation_name}"
147151
)
148152
)
@@ -299,9 +303,16 @@ def update_endpoints(self, event: DatabaseRequestedEvent = None) -> None: # noq
299303
def _update_unit_status(self, relation: Relation) -> None:
300304
"""# Clean up Blocked status if it's due to extensions request."""
301305
if (
302-
self.charm.is_blocked
303-
and self.charm.unit.status.message == INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE
304-
) and not self.check_for_invalid_extra_user_roles(relation.id):
306+
(
307+
self.charm.is_blocked
308+
and (
309+
self.charm.unit.status.message == INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE
310+
or self.charm.unit.status.message == INVALID_DATABASE_NAME_BLOCKING_MESSAGE
311+
)
312+
)
313+
and not self.check_for_invalid_extra_user_roles(relation.id)
314+
and not self.check_for_invalid_database_name(relation.id)
315+
):
305316
self.charm.set_unit_status(ActiveStatus())
306317
if (
307318
self.charm.is_blocked
@@ -326,6 +337,24 @@ def check_for_invalid_extra_user_roles(self, relation_id: int) -> bool:
326337
if (
327338
extra_user_role not in valid_privileges
328339
and extra_user_role not in valid_roles
340+
and extra_user_role != "createdb"
329341
):
330342
return True
331343
return False
344+
345+
def check_for_invalid_database_name(self, relation_id: int) -> bool:
346+
"""Checks if there are relations with invalid database names.
347+
348+
Args:
349+
relation_id: current relation to be skipped.
350+
"""
351+
for relation in self.charm.model.relations.get(self.relation_name, []):
352+
if relation.id == relation_id:
353+
continue
354+
for data in relation.data.values():
355+
database = data.get("database")
356+
if database is not None and (
357+
len(database) > 49 or database in ["postgres", "template0", "template1"]
358+
):
359+
return True
360+
return False

‎templates/patroni.yml.j2

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,13 @@ postgresql:
164164
- local all backup peer map=operator
165165
- local all operator scram-sha-256
166166
- local all monitoring password
167+
- {{ 'hostssl' if enable_tls else 'host' }} all +charmed_stats 0.0.0.0/0 scram-sha-256
168+
- {{ 'hostssl' if enable_tls else 'host' }} all +charmed_read 0.0.0.0/0 scram-sha-256
169+
- {{ 'hostssl' if enable_tls else 'host' }} all +charmed_dml 0.0.0.0/0 scram-sha-256
170+
- {{ 'hostssl' if enable_tls else 'host' }} all +charmed_backup 0.0.0.0/0 scram-sha-256
167171
- {{ 'hostssl' if enable_tls else 'host' }} all +charmed_dba 0.0.0.0/0 scram-sha-256
172+
- {{ 'hostssl' if enable_tls else 'host' }} all +charmed_admin 0.0.0.0/0 scram-sha-256
173+
- {{ 'hostssl' if enable_tls else 'host' }} all +charmed_databases_owner 0.0.0.0/0 scram-sha-256
168174
{%- if not connectivity %}
169175
- {{ 'hostssl' if enable_tls else 'host' }} all all {{ self_ip }} md5
170176
- {{ 'hostssl' if enable_tls else 'host' }} all all 0.0.0.0/0 reject

‎tests/integration/conftest.py

Lines changed: 174 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,13 @@
55
import uuid
66

77
import boto3
8+
import jubilant
89
import pytest
910
from pytest_operator.plugin import OpsTest
1011

1112
from . import architecture
1213
from .helpers import construct_endpoint
14+
from .jubilant_helpers import RoleAttributeValue
1315

1416
AWS = "AWS"
1517
GCP = "GCP"
@@ -25,7 +27,7 @@ def charm():
2527
return f"./[email protected]{architecture.architecture}.charm"
2628

2729

28-
def get_cloud_config(cloud: str) -> tuple[dict[str, str], dict[str, str]]:
30+
def get_cloud_config(cloud: str) -> tuple[dict[str, str], dict[str, str]] | None:
2931
# Define some configurations and credentials.
3032
if cloud == AWS:
3133
return {
@@ -47,6 +49,7 @@ def get_cloud_config(cloud: str) -> tuple[dict[str, str], dict[str, str]]:
4749
"access-key": os.environ["GCP_ACCESS_KEY"],
4850
"secret-key": os.environ["GCP_SECRET_KEY"],
4951
}
52+
return None
5053

5154

5255
def cleanup_cloud(config: dict[str, str], credentials: dict[str, str]) -> None:
@@ -94,3 +97,173 @@ async def gcp_cloud_configs(ops_test: OpsTest) -> None:
9497
yield config, credentials
9598

9699
cleanup_cloud(config, credentials)
100+
101+
102+
@pytest.fixture(scope="module")
103+
def juju(request: pytest.FixtureRequest):
104+
"""Pytest fixture that wraps :meth:`jubilant.with_model`.
105+
106+
This adds command line parameter ``--keep-models`` (see help for details).
107+
"""
108+
controller = request.config.getoption("--controller")
109+
model = request.config.getoption("--model")
110+
controller_and_model = None
111+
if controller and model:
112+
controller_and_model = f"{controller}:{model}"
113+
elif controller:
114+
controller_and_model = controller
115+
elif model:
116+
controller_and_model = model
117+
keep_models = bool(request.config.getoption("--keep-models"))
118+
119+
if controller_and_model:
120+
juju = jubilant.Juju(model=controller_and_model) # type: ignore
121+
yield juju
122+
log = juju.debug_log(limit=1000)
123+
else:
124+
with jubilant.temp_model(keep=keep_models) as juju:
125+
yield juju
126+
log = juju.debug_log(limit=1000)
127+
128+
if request.session.testsfailed:
129+
print(log, end="")
130+
131+
132+
@pytest.fixture(scope="module")
133+
def predefined_roles() -> dict:
134+
"""Return a list of predefined roles with their expected permissions."""
135+
return {
136+
"": {
137+
"auto-escalate-to-database-owner": RoleAttributeValue.REQUESTED_DATABASE,
138+
"permissions": {
139+
"connect": RoleAttributeValue.REQUESTED_DATABASE,
140+
"create-databases": RoleAttributeValue.NO,
141+
"create-objects": RoleAttributeValue.NO,
142+
"escalate-to-database-owner": RoleAttributeValue.REQUESTED_DATABASE,
143+
"read-data": RoleAttributeValue.NO,
144+
"read-stats": RoleAttributeValue.ALL_DATABASES,
145+
"run-backup-commands": RoleAttributeValue.NO,
146+
"set-up-predefined-catalog-roles": RoleAttributeValue.NO,
147+
"set-user": RoleAttributeValue.NO,
148+
"write-data": RoleAttributeValue.NO,
149+
},
150+
},
151+
"charmed_stats": {
152+
"auto-escalate-to-database-owner": RoleAttributeValue.NO,
153+
"permissions": {
154+
"connect": RoleAttributeValue.ALL_DATABASES,
155+
"create-databases": RoleAttributeValue.NO,
156+
"create-objects": RoleAttributeValue.NO,
157+
"escalate-to-database-owner": RoleAttributeValue.NO,
158+
"read-data": RoleAttributeValue.NO,
159+
"read-stats": RoleAttributeValue.ALL_DATABASES,
160+
"run-backup-commands": RoleAttributeValue.NO,
161+
"set-up-predefined-catalog-roles": RoleAttributeValue.NO,
162+
"set-user": RoleAttributeValue.NO,
163+
"write-data": RoleAttributeValue.NO,
164+
},
165+
},
166+
"charmed_read": {
167+
"auto-escalate-to-database-owner": RoleAttributeValue.NO,
168+
"permissions": {
169+
"connect": RoleAttributeValue.ALL_DATABASES,
170+
"create-databases": RoleAttributeValue.NO,
171+
"create-objects": RoleAttributeValue.NO,
172+
"escalate-to-database-owner": RoleAttributeValue.NO,
173+
"read-data": RoleAttributeValue.ALL_DATABASES,
174+
"read-stats": RoleAttributeValue.ALL_DATABASES,
175+
"run-backup-commands": RoleAttributeValue.NO,
176+
"set-up-predefined-catalog-roles": RoleAttributeValue.NO,
177+
"set-user": RoleAttributeValue.NO,
178+
"write-data": RoleAttributeValue.NO,
179+
},
180+
},
181+
"charmed_dml": {
182+
"auto-escalate-to-database-owner": RoleAttributeValue.NO,
183+
"permissions": {
184+
"connect": RoleAttributeValue.ALL_DATABASES,
185+
"create-databases": RoleAttributeValue.NO,
186+
"create-objects": RoleAttributeValue.NO,
187+
"escalate-to-database-owner": RoleAttributeValue.NO,
188+
"read-data": RoleAttributeValue.ALL_DATABASES,
189+
"read-stats": RoleAttributeValue.ALL_DATABASES,
190+
"run-backup-commands": RoleAttributeValue.NO,
191+
"set-up-predefined-catalog-roles": RoleAttributeValue.NO,
192+
"set-user": RoleAttributeValue.NO,
193+
"write-data": RoleAttributeValue.ALL_DATABASES,
194+
},
195+
},
196+
"charmed_backup": {
197+
"auto-escalate-to-database-owner": RoleAttributeValue.NO,
198+
"permissions": {
199+
"connect": RoleAttributeValue.ALL_DATABASES,
200+
"create-databases": RoleAttributeValue.NO,
201+
"create-objects": RoleAttributeValue.NO,
202+
"escalate-to-database-owner": RoleAttributeValue.NO,
203+
"read-data": RoleAttributeValue.NO,
204+
"read-stats": RoleAttributeValue.ALL_DATABASES,
205+
"run-backup-commands": RoleAttributeValue.YES,
206+
"set-up-predefined-catalog-roles": RoleAttributeValue.NO,
207+
"set-user": RoleAttributeValue.NO,
208+
"write-data": RoleAttributeValue.NO,
209+
},
210+
},
211+
"charmed_dba": {
212+
"auto-escalate-to-database-owner": RoleAttributeValue.NO,
213+
"permissions": {
214+
"connect": RoleAttributeValue.ALL_DATABASES,
215+
"create-databases": RoleAttributeValue.NO,
216+
"create-objects": RoleAttributeValue.NO,
217+
"escalate-to-database-owner": RoleAttributeValue.NO,
218+
"read-data": RoleAttributeValue.ALL_DATABASES,
219+
"read-stats": RoleAttributeValue.ALL_DATABASES,
220+
"run-backup-commands": RoleAttributeValue.NO,
221+
"set-up-predefined-catalog-roles": RoleAttributeValue.NO,
222+
"set-user": RoleAttributeValue.YES,
223+
"write-data": RoleAttributeValue.ALL_DATABASES,
224+
},
225+
},
226+
"charmed_admin": {
227+
"auto-escalate-to-database-owner": RoleAttributeValue.ALL_DATABASES,
228+
"permissions": {
229+
"connect": RoleAttributeValue.ALL_DATABASES,
230+
"create-databases": RoleAttributeValue.NO,
231+
"create-objects": RoleAttributeValue.NO,
232+
"escalate-to-database-owner": RoleAttributeValue.ALL_DATABASES,
233+
"read-data": RoleAttributeValue.ALL_DATABASES,
234+
"read-stats": RoleAttributeValue.ALL_DATABASES,
235+
"run-backup-commands": RoleAttributeValue.NO,
236+
"set-up-predefined-catalog-roles": RoleAttributeValue.NO,
237+
"set-user": RoleAttributeValue.NO,
238+
"write-data": RoleAttributeValue.ALL_DATABASES,
239+
},
240+
},
241+
"CREATEDB": {
242+
"auto-escalate-to-database-owner": RoleAttributeValue.NO,
243+
"permissions": {
244+
"connect": RoleAttributeValue.ALL_DATABASES,
245+
"create-databases": RoleAttributeValue.YES,
246+
"create-objects": RoleAttributeValue.NO,
247+
"escalate-to-database-owner": RoleAttributeValue.NO,
248+
"read-data": RoleAttributeValue.NO,
249+
"read-stats": RoleAttributeValue.NO,
250+
"run-backup-commands": RoleAttributeValue.NO,
251+
"set-up-predefined-catalog-roles": RoleAttributeValue.YES,
252+
"set-user": RoleAttributeValue.NO,
253+
"write-data": RoleAttributeValue.NO,
254+
},
255+
},
256+
}
257+
258+
259+
@pytest.fixture(scope="module")
260+
def predefined_roles_combinations() -> list:
261+
"""Return a list of valid combinations of predefined roles."""
262+
return [
263+
("",),
264+
("charmed_stats",),
265+
("charmed_read",),
266+
("charmed_dml",),
267+
("charmed_admin",),
268+
("charmed_admin", "CREATEDB"),
269+
]

‎tests/integration/helpers.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -747,10 +747,10 @@ def check_connected_user(
747747
if result is not None:
748748
instance = "primary" if primary else "replica"
749749
assert result[0] == session_user, (
750-
f"The session user should be the {session_user} user in the {instance}"
750+
f"The session user should be the {session_user} user in the {instance} (it's currently {result[0]})"
751751
)
752752
assert result[1] == current_user, (
753-
f"The current user should be the {current_user} user in the {instance}"
753+
f"The current user should be the {current_user} user in the {instance} (it's currently {result[1]})"
754754
)
755755
else:
756756
assert False, "No result returned from the query"
@@ -793,6 +793,12 @@ async def check_roles_and_their_permissions(
793793
check_connected_user(cursor, username, username)
794794
with pytest.raises(psycopg2.errors.InsufficientPrivilege):
795795
cursor.execute("CREATE TABLE test_table_2 (id INTEGER);")
796+
797+
logger.info(
798+
"Checking that the relation user can escalate back to the database owner user"
799+
)
800+
cursor.execute(f"SET ROLE {database_name}_owner;")
801+
check_connected_user(cursor, username, f"{database_name}_owner")
796802
finally:
797803
if connection is not None:
798804
connection.close()

0 commit comments

Comments
 (0)