Add SECURITY DEFINER to internal functions
#191
Conversation
This function must be able to write to the `pgroll.migrations` table regardless of the permissions of the user who causes it to be executed.
This function needs to be able to read the `pgroll.migrations` table regardless of the permissions of the invoking user.
| @@ -51,6 +51,7 @@ CREATE OR REPLACE FUNCTION %[1]s.is_active_migration_period(schemaname NAME) RET | |||
|
|
|||
| -- Get the latest version name (this is the one with child migrations) | |||
| CREATE OR REPLACE FUNCTION %[1]s.latest_version(schemaname NAME) RETURNS text | |||
| SECURITY DEFINER | |||
There was a problem hiding this comment.
This probably needs a search path and probably should also get some explicit schemas for all called functions, otherwise I can imagine that this can be used to gain the rights of the user used for migrations.
There was a problem hiding this comment.
Thanks for raising this! I'm adding SET search_path and reviewing calls
|
I took over this PR and updated the description, let me know what you think! |
pkg/state/state.go
Outdated
| @@ -192,7 +196,7 @@ BEGIN | |||
| VALUES ( | |||
| schemaname, | |||
| format('sql_%%s', substr(md5(random()::text), 0, 15)), | |||
There was a problem hiding this comment.
While I currently think adding a search path is sufficient, I find it good practise to use schema qualified function calls all over, e.g. also pg_catalog.format and pg_catalog.count(DISTINCT schema_name).
pkg/state/state.go
Outdated
| LANGUAGE plpgsql AS $$ | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| SET search_path = %[1]s, pg_catalog AS $$ |
There was a problem hiding this comment.
This suggests that this should also include pg_temp as a last entry: https://github.com/timescale/pgspot/blob/main/src/pgspot/state.py#L123-L127
# we require explicit pg_temp as last entry for a schema to
# be safe because not having explicit pg_temp in search_path
# will result in a search_path with pg_temp as first entry
if schemas == ["pg_temp"]:
return True
|
Thank you @jankatins for your input! I believe I've addressed all comments |
|
There are probably a few more which could be schema qualified (e.g. stuff like If you want to have some fun with testing/linting: https://github.com/Aiven-Open/pghostile and https://github.com/timescale/pgspot |
We want to work on the security model for the
pgrollschema, allowing admins to remove access while maintaining functionality intact. Because of this, some functions must still be marked as executable for any user doing migrations directly with postgres.This PR adds
SECURITY DEFINERto these functions