diff --git a/CHANGELOG.md b/CHANGELOG.md index 49040f4..f38bd24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -293,6 +293,40 @@ hardening standalone use; the highlights: attribute access (`db.mytable`) is unchanged. `reload_all` and `rename_table` raise `ValueError` where they used to `assert`. +- **Table creation no longer grants privileges to hard-coded roles.** Creating + a search table granted `SELECT` on it to `lmfdb` and `webserver`, and + `SELECT`/`INSERT` on its counts and stats tables, wherever those roles + happened to exist; the metadata tables did the same, and a reload reapplied + it. That is the LMFDB's deployment rather than a fact about a PostgreSQL + database, so a new table holding anything at all was readable by two roles + the administrator may never have heard of. Privileges are now stated by a + `psycodict.grants.GrantPolicy`, passed as + `PostgresDatabase(grant_policy=...)` and applied through one helper wherever + psycodict creates or swaps a relation. *Migration:* the default policy grants + nothing; LMFDB and any other deployment that wants the old behavior passes + `LMFDBGrantPolicy()`. A policy that names a role the cluster does not have + raises by default (`LMFDBGrantPolicy` warns and skips, as before), and + `grant_select`, `grant_insert`, `grant_update` and `grant_delete` now require + their `users` argument instead of defaulting to those roles. +- **A reload warns when it takes access away.** A reload swaps in a clone, and + PostgreSQL gives a clone no privileges of its own, so under the default + policy the replacement is reachable only by its owner even when the table it + replaced was readable by an application role. That is what the policy asks + for, but it should not be discovered by a website going dark, so psycodict + logs a warning naming the roles that lost access. +- **A reload's backup table is no longer left as readable as the table it + replaced.** PostgreSQL carries privileges along with a rename, so the `_oldN` + table — a copy of the live data — inherited the live table's grants. The + policy is now applied to it as its own relation kind, which grants nothing + under `LMFDBGrantPolicy`. `reload_revert` applies the same rule, so reverting + a reload no longer hands the restored table its backup's privileges and the + backup the live table's. The legacy `_oldstats` relation has its own + `oldstats` kind and keeps the read-only access it always had, rather than + being treated as an ordinary stats table (which under `LMFDBGrantPolicy` + would have given the website `INSERT` on it). Note that the *default* policy + names no roles, so it revokes nothing: a freshly created relation has no + non-owner grants to begin with, but a renamed one keeps the ACL it had. + ### Infrastructure - A test suite of 500+ tests and a continuous-integration workflow covering the diff --git a/DataManagement.md b/DataManagement.md index 0e05663..0554b22 100644 --- a/DataManagement.md +++ b/DataManagement.md @@ -8,6 +8,23 @@ Everything here is a method on either the database object (`db`) or a search tab A note on permissions: most write methods first check `db._read_only`, and several branches of behavior depend on whether your table's statistics class has `saving` turned on (see [Statistics and counts](#statistics-and-counts)). A bare psycodict install has `saving = False`. +Permissions on the relations psycodict *creates* are a separate matter, decided by the database's grant policy (`psycodict.grants`). The default policy grants nothing, so a new search table, its `_counts` and `_stats` companions and the meta tables are reachable only by their owner. A deployment that wants the LMFDB's roles asks for them: + +```python +from psycodict.grants import LMFDBGrantPolicy +db = PostgresDatabase(grant_policy=LMFDBGrantPolicy()) +``` + +which grants `SELECT` to `lmfdb` and `webserver` and `INSERT` on the counts and stats tables to `webserver`, as psycodict used to do unconditionally. A policy names a role that must exist: by default a policy naming a role the cluster does not have raises — at connection time, rather than partway through the first thing that creates a relation — and `missing_role="skip"` (what `LMFDBGrantPolicy` uses, since development databases rarely have those roles) warns and carries on. + +The policy is authoritative **for the roles it names**. Applying it revokes `SELECT`, `INSERT`, `UPDATE` and `DELETE` from those roles before granting what the policy says, so a relation psycodict creates or swaps ends up with the policy rather than with the policy plus whatever it inherited; roles the policy does not name are never touched. That distinction matters for the default policy, which names no roles at all: it therefore revokes nothing. A *freshly created* relation has no non-owner grants to begin with, so under the default it stays owner-only — but a relation psycodict *renames* (a backup, say) keeps the ACL it already had, and the default policy will not take it away. `LMFDBGrantPolicy` does take it away, because it names `lmfdb` and `webserver` and gives the `backup` kind nothing. + +Policies are applied where psycodict creates or swaps a relation, not on connection: pointing a policy at an existing database does not retroactively change the privileges on relations that are already there. Cleaning up historical ACLs is an administrative job. + +This matters most for the `_oldN` table a [reload](#reload) leaves behind: PostgreSQL carries privileges along with a rename, so the backup would otherwise stay as readable as the live table it used to be, while holding a copy of the same production data. The `backup` relation kind decides what it gets instead — nothing, under `LMFDBGrantPolicy`. `reload_revert` applies the same rule in the other direction, so reverting does not hand the restored table its backup's privileges. The legacy `_oldstats` relation has its own kind, and gets read access only. + +The reverse is worth knowing too: a reload swaps in a clone, and PostgreSQL creates a clone with no privileges of its own, so under the default policy the replacement is reachable only by its owner even if the table it replaced was readable by an application role. psycodict warns when a swap takes access away like this, naming the roles that lost it. `db.grant_select` and its siblings remain, for granting something to roles you name explicitly. + ## Creating tables ### `create_table` diff --git a/docs/api/grants.md b/docs/api/grants.md new file mode 100644 index 0000000..4e0e4e7 --- /dev/null +++ b/docs/api/grants.md @@ -0,0 +1,5 @@ +# psycodict.grants + +```{eval-rst} +.. automodule:: psycodict.grants +``` diff --git a/docs/api/index.md b/docs/api/index.md index 9756cf2..19b4c32 100644 --- a/docs/api/index.md +++ b/docs/api/index.md @@ -18,6 +18,8 @@ Generated from the docstrings. The map of the library: PostgreSQL, including the file formats used by the bulk operations. - {mod}`psycodict.config` — configuration discovery and parsing (`config.ini`, command-line arguments, `$PSYCODICT_CONFIG`). +- {mod}`psycodict.grants` — {class}`~psycodict.grants.GrantPolicy`, the + privileges the relations psycodict creates come into existence with. - {mod}`psycodict.utils` — {class}`~psycodict.utils.DelayCommit` and other helpers shared across the library. - {mod}`psycodict.notifications` — LISTEN/NOTIFY support: schema-change @@ -40,6 +42,7 @@ table statstable encoding config +grants utils notifications dbdiff diff --git a/psycodict/database.py b/psycodict/database.py index 964c2f0..7cd95f9 100644 --- a/psycodict/database.py +++ b/psycodict/database.py @@ -50,6 +50,7 @@ validate_search_table_name, validate_search_table_registry, ) +from .grants import GRANT_ACTIONS, GrantPolicy from .searchtable import PostgresSearchTable from .utils import DelayCommit, safe_child_path @@ -521,7 +522,7 @@ def _register_object(self, obj): self._objects.append(obj) def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, - session_settings=None, **kwargs): + session_settings=None, grant_policy=None, **kwargs): if config is None: from .config import Configuration config = Configuration() @@ -536,12 +537,25 @@ def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, # or database than the sender writes to). self._connect_kwargs = dict(kwargs) self._session_settings = self._resolve_session_settings(session_settings) + # What the relations psycodict creates are readable and writable by. + # The default grants nothing: a deployment that wants the LMFDB's roles + # asks for them (see psycodict.grants). + self.grant_policy = GrantPolicy() if grant_policy is None else grant_policy self.conn = self._new_connection() self._user = self.conn.info.user # What a replacement connection has to reach to be adopted. self._identity = (self.conn.info.dbname, self.conn.info.user) PostgresBase.__init__(self, "db_all", self) + # Check the policy's roles here, rather than when the first relation is + # created: a policy naming a role this cluster does not have is a + # mistake worth hearing about before it interrupts a bootstrap or a + # reload halfway through. + if self.grant_policy.roles: + self._existing_roles( + self.grant_policy.roles, missing=self.grant_policy.missing_role + ) + if create: # Create any missing metadata tables before the read-only detection # below, which concludes read-only when no tables are visible @@ -694,34 +708,122 @@ def _log_db_change(self, operation, tablename=None, logid=None, aborted=False, * """ pass - def _existing_roles(self, users): + def _existing_roles(self, users, missing="error"): """ - Filters a list of role names down to those that exist in the cluster. + The roles among ``users`` that the cluster actually has. + + INPUT: - Missing roles produce a warning rather than an error, so that table - creation works on clusters without the LMFDB roles (lmfdb, webserver). + - ``users`` -- role names + - ``missing`` -- ``"error"`` to refuse when one of them does not exist, + ``"skip"`` to warn and leave it out + + A policy that names a role the cluster does not have has not been + applied, whatever it says; the default is to say so rather than to + leave the caller believing otherwise. """ existing = {rec[0] for rec in self._execute(SQL("SELECT rolname FROM pg_roles"))} - missing = [user for user in users if user not in existing] - if missing: + absent = [user for user in users if user not in existing] + if absent: + if missing == "error": + raise ValueError( + "Postgres role(s) %s do not exist, so the grant policy " + "cannot be applied; create them, correct the policy, or " + "use a policy with missing_role='skip'" + % (", ".join(absent),) + ) logging.warning( "Postgres role(s) %s do not exist; skipping grants", - ", ".join(missing), + ", ".join(absent), ) return [user for user in users if user in existing] - def _grant(self, action, table_name, users): + def _grant(self, action, table_name, users, missing="error"): """ Utility function for granting permissions on tables. """ action = action.upper() - if action not in ["SELECT", "INSERT", "UPDATE", "DELETE"]: + if action not in GRANT_ACTIONS: raise ValueError("%s is not a valid action" % action) - grantor = SQL("GRANT %s ON TABLE {0} TO {1}" % action) - for user in self._existing_roles(users): + grantor = SQL("GRANT " + action + " ON TABLE {0} TO {1}") + for user in self._existing_roles(users, missing=missing): self._execute(grantor.format(Identifier(table_name), Identifier(user)), silent=True) - def grant_select(self, table_name, users=["lmfdb", "webserver"]): + def _apply_grant_policy(self, table_name, kind): + """ + Give a relation psycodict has just created the privileges the policy + says a relation of its kind has. + + INPUT: + + - ``table_name`` -- the relation + - ``kind`` -- one of ``psycodict.grants.RELATION_KINDS`` + + The policy is authoritative for the roles it names: the actions it + manages are revoked from all of them first, so a relation ends up with + what the policy says rather than with that plus whatever it inherited. + A rename carries privileges with it, which is how a backup table would + otherwise keep the live table's, so this matters most there. Roles the + policy does not name are not touched at all. + """ + policy = self.grant_policy + if not policy.roles: + # the default policy: nothing to grant, and nothing to look up + return + roles = self._existing_roles(policy.roles, missing=policy.missing_role) + if not roles: + return + self._execute( + SQL("REVOKE {0} ON TABLE {1} FROM {2}").format( + SQL(", ").join(SQL(action) for action in GRANT_ACTIONS), + Identifier(table_name), + SQL(", ").join(Identifier(role) for role in roles), + ), + silent=True, + ) + for action, granted in policy.for_kind(kind).items(): + wanted = [role for role in granted if role in roles] + if wanted: + self._grant(action, table_name, wanted, missing=policy.missing_role) + + def _grantees(self, table_name): + """ + The roles that have been granted something on a relation, other than + the grantor's own privileges on it. + """ + cur = self._execute( + SQL( + "SELECT DISTINCT grantee FROM information_schema.role_table_grants " + "WHERE table_name = %s AND grantee <> grantor" + ), + [table_name], + silent=True, + ) + return {rec[0] for rec in cur} + + def _warn_if_access_was_lost(self, table_name, previous_name): + """ + Say so when a swap has replaced a relation others could read with one + they cannot. + + A reload swaps in a clone, which PostgreSQL creates with no privileges + of its own, so under a policy that says nothing about the relation the + replacement is reachable only by its owner -- an application role that + could read the table a moment ago now cannot. That is what the policy + asks for, but it should not be discovered by a website going dark. + """ + lost = self._grantees(previous_name) - self._grantees(table_name) + if lost: + logging.warning( + "%s was replaced by a copy that %s cannot access; the grant " + "policy in force grants them nothing on it. Pass a grant_policy " + "to PostgresDatabase (LMFDBGrantPolicy() reproduces psycodict's " + "pre-1.0 grants) if they should keep it.", + table_name, + ", ".join(sorted(lost)), + ) + + def grant_select(self, table_name, users): """ Grant users the ability to run SELECT statements on a given table @@ -729,10 +831,14 @@ def grant_select(self, table_name, users=["lmfdb", "webserver"]): - ``table_name`` -- a string, the name of the table - ``users`` -- a list of users to grant this permission + + Note that this grants exactly what it is asked to, to exactly the roles + it is given: what a relation psycodict creates starts out with is the + grant policy's business (see :class:`psycodict.grants.GrantPolicy`). """ self._grant("SELECT", table_name, users) - def grant_insert(self, table_name, users=["webserver"]): + def grant_insert(self, table_name, users): """ Grant users the ability to run INSERT statements on a given table @@ -743,7 +849,7 @@ def grant_insert(self, table_name, users=["webserver"]): """ self._grant("INSERT", table_name, users) - def grant_update(self, table_name, users=["webserver"]): + def grant_update(self, table_name, users): """ Grant users the ability to run UPDATE statements on a given table @@ -754,7 +860,7 @@ def grant_update(self, table_name, users=["webserver"]): """ self._grant("UPDATE", table_name, users) - def grant_delete(self, table_name, users=["webserver"]): + def grant_delete(self, table_name, users): """ Grant users the ability to run DELETE statements on a given table @@ -986,7 +1092,7 @@ def _meta_creator(self, meta_name, hist=False, fmt=None): parts.append(part) tbl = meta_name + ("_hist" if hist else "") self._execute(SQL("CREATE TABLE {0} ({1})").format(Identifier(tbl), SQL(", ").join(parts))) - self.grant_select(tbl) + self._apply_grant_policy(tbl, "meta_hist" if hist else "meta") @property def meta_format(self): @@ -1172,7 +1278,7 @@ def _stamp_meta_format(self, version): )) self._execute(SQL("INSERT INTO meta_format (version, min_compat) VALUES (%s, %s)"), [version, min_compat]) - self.grant_select("meta_format") + self._apply_grant_policy("meta_format", "meta_format") self._execute(SQL("DROP TABLE IF EXISTS meta_version")) print("Stamped metadata format %s (min_compat %s)" % (version, min_compat)) @@ -1526,7 +1632,7 @@ def create_table( with DelayCommit(self, silence=True): self._create_table(name, search_columns, addid=id_type, tablespace=tablespace) - self.grant_select(name) + self._apply_grant_policy(name, "search") tablespace = self._tablespace_clause(tablespace) creator = SQL( "CREATE TABLE {0} " @@ -1537,8 +1643,7 @@ def create_table( Identifier(physical_table_name(name, "counts")), tablespace ) self._execute(creator) - self.grant_select(name + "_counts") - self.grant_insert(name + "_counts") + self._apply_grant_policy(name + "_counts", "counts") creator = SQL( "CREATE TABLE {0} " '(cols jsonb, stat text COLLATE "C", value numeric, ' @@ -1548,8 +1653,7 @@ def create_table( Identifier(physical_table_name(name, "stats")), tablespace ) self._execute(creator) - self.grant_select(name + "_stats") - self.grant_insert(name + "_stats") + self._apply_grant_policy(name + "_stats", "stats") # FIXME use global constants ? # include_nones is written explicitly rather than left to the # column default: existing databases keep the DDL default their diff --git a/psycodict/grants.py b/psycodict/grants.py new file mode 100644 index 0000000..4769c22 --- /dev/null +++ b/psycodict/grants.py @@ -0,0 +1,203 @@ +# -*- coding: utf-8 -*- +""" +Who gets to read and write the relations psycodict creates. + +Creating a search table creates several relations -- the table itself, its +counts and stats tables -- and a reload creates and swaps more. Something has +to decide what privileges those come into existence with, and psycodict used to +decide it in the code: SELECT to ``lmfdb`` and ``webserver``, INSERT to +``webserver``, wherever those roles happened to exist. That is the LMFDB's +deployment, not a fact about a PostgreSQL database, and it meant a new table +holding whatever you put in it was readable by two roles you may never have +heard of. + +A :class:`GrantPolicy` states those privileges explicitly, by relation kind. +The default policy grants nothing, so a relation is reachable only by its owner +and by whatever the cluster's own defaults give away; :func:`LMFDBGrantPolicy` +reproduces what psycodict used to do, for the deployments that want it:: + + db = PostgresDatabase(grant_policy=LMFDBGrantPolicy()) + +A policy is authoritative for the relations psycodict applies it to: it revokes +the actions it manages from the roles it names before granting, so the result +is the policy and not the policy plus whatever was there before. +""" +from dataclasses import dataclass, field + +from .validation import InvalidDefinitionError, MAX_IDENTIFIER_LENGTH + +# The actions a policy can grant. A policy manages exactly these: privileges +# outside this set (TRUNCATE, REFERENCES, TRIGGER) are left alone. +GRANT_ACTIONS = ("SELECT", "INSERT", "UPDATE", "DELETE") + +# The kinds of relation psycodict creates, which is what a policy is written in +# terms of -- a policy should not have to know that the counts table of foo is +# called foo_counts. +RELATION_KINDS = ( + "search", # a search table + "counts", # its counts table + "stats", # its stats table + "meta", # meta_tables, meta_indexes, meta_constraints + "meta_hist", # their _hist counterparts + "meta_format", # the metadata format stamp + "backup", # the _oldN table a reload leaves behind + "oldstats", # the legacy Mongo statistics import made by create_oldstats +) + +# What to do about a role a policy names that the cluster does not have. +MISSING_ROLE_ACTIONS = ("error", "skip") + + +def _validate_role_name(name): + """ + Check a role name a policy names. + + Roles are quoted with ``Identifier`` wherever they are used, and PostgreSQL + allows more in a role name than psycodict allows in a table name, so this + checks only what would make the name unusable. + """ + if not isinstance(name, str) or not name: + raise InvalidDefinitionError("A role name must be a non-empty string") + if len(name.encode("utf-8")) > MAX_IDENTIFIER_LENGTH: + raise InvalidDefinitionError( + "Role name %r is longer than PostgreSQL's %s byte limit" + % (name, MAX_IDENTIFIER_LENGTH) + ) + for char in name: + if ord(char) < 0x20 or 0x7F <= ord(char) <= 0x9F: + raise InvalidDefinitionError( + "Role name %r contains the control character %r" % (name, char) + ) + return name + + +@dataclass(frozen=True) +class GrantPolicy: + """ + The privileges psycodict grants on the relations it creates. + + INPUT: + + - ``grants`` -- a mapping from relation kind (see ``RELATION_KINDS``) to a + mapping from action (see ``GRANT_ACTIONS``) to the roles that get it. + Kinds left out get nothing. + - ``missing_role`` -- what to do when the policy names a role the cluster + does not have: ``"error"`` (the default) refuses, so that a policy is + never half-applied without saying so; ``"skip"`` warns and carries on, + which is what a development machine without the deployment's roles + wants. + + EXAMPLES:: + + >>> GrantPolicy({"search": {"SELECT": ("readonly",)}}) + GrantPolicy(grants={'search': {'SELECT': ('readonly',)}}, missing_role='error') + """ + + grants: dict = field(default_factory=dict) + missing_role: str = "error" + + def __post_init__(self): + if self.missing_role not in MISSING_ROLE_ACTIONS: + raise ValueError( + "missing_role must be one of %s, not %r" + % (", ".join(MISSING_ROLE_ACTIONS), self.missing_role) + ) + normalized = {} + for kind, actions in self.grants.items(): + if kind not in RELATION_KINDS: + raise ValueError( + "Unknown relation kind %r; psycodict creates %s" + % (kind, ", ".join(RELATION_KINDS)) + ) + normalized[kind] = {} + for action, roles in actions.items(): + if action.upper() not in GRANT_ACTIONS: + raise ValueError( + "Unknown action %r; a policy grants %s" + % (action, ", ".join(GRANT_ACTIONS)) + ) + if isinstance(roles, str): + raise ValueError( + "The roles granted %s on a %s relation must be a " + "sequence of role names, not the string %r" + % (action, kind, roles) + ) + try: + # materialized before it is validated: validating a + # generator would consume it, and the policy would then + # silently name nobody + roles = tuple(roles) + except TypeError as err: + raise ValueError( + "The roles granted %s on a %s relation must be an " + "iterable of role names, not %r" % (action, kind, roles) + ) from err + for role in roles: + _validate_role_name(role) + normalized[kind][action.upper()] = roles + # frozen dataclass: this is the one place the fields are set + object.__setattr__(self, "grants", normalized) + + @property + def roles(self): + """ + Every role this policy mentions. + + These are the roles whose privileges psycodict manages: applying the + policy revokes the managed actions from them first, so that what a + relation ends up with is the policy rather than the policy plus + whatever it inherited. Roles the policy does not mention are not + touched. + """ + return sorted({ + role + for actions in self.grants.values() + for roles in actions.values() + for role in roles + }) + + def for_kind(self, kind): + """ + The ``{action: roles}`` this policy gives a relation of ``kind``. + """ + if kind not in RELATION_KINDS: + raise ValueError("Unknown relation kind %r" % (kind,)) + return self.grants.get(kind, {}) + + +def LMFDBGrantPolicy(missing_role="skip"): + """ + The permissions psycodict granted before they were a policy. + + SELECT to ``lmfdb`` and ``webserver`` on search, counts, stats and metadata + relations, and INSERT to ``webserver`` on counts and stats, which the + website needs in order to record the counts it computes. The legacy + ``_oldstats`` relation gets SELECT only, as it always did. Backup (``_oldN``) + tables get nothing: they hold the data the live table held before a reload, + and a rename carries the live table's privileges over to them, so the + policy revokes what the live table had rather than leaving a copy of + production readable by the application roles. + + INPUT: + + - ``missing_role`` -- defaults to ``"skip"``, since a development database + typically has neither role; pass ``"error"`` on a deployment that should + have both. + """ + read = ("lmfdb", "webserver") + write = ("webserver",) + return GrantPolicy( + { + "search": {"SELECT": read}, + "counts": {"SELECT": read, "INSERT": write}, + "stats": {"SELECT": read, "INSERT": write}, + "meta": {"SELECT": read}, + "meta_hist": {"SELECT": read}, + "meta_format": {"SELECT": read}, + # create_oldstats only ever granted SELECT: the relation holds an + # import of statistics computed elsewhere, and nothing writes to it + # through the website. + "oldstats": {"SELECT": read}, + }, + missing_role=missing_role, + ) diff --git a/psycodict/statstable.py b/psycodict/statstable.py index 3133df9..92d575e 100644 --- a/psycodict/statstable.py +++ b/psycodict/statstable.py @@ -2155,7 +2155,7 @@ def create_oldstats(self, filename): with DelayCommit(self, silence=True): creator = SQL('CREATE TABLE {0} (_id text COLLATE "C", data jsonb)').format(Identifier(name)) self._execute(creator) - self._db.grant_select(name) + self._db._apply_grant_policy(name, "oldstats") with open(filename) as F: try: self._copy_from_stdin(F, self.search_table + "_oldstats") diff --git a/psycodict/table.py b/psycodict/table.py index a82876b..a7a24a5 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -1872,6 +1872,44 @@ def _staged_label_index_name(self, suffix="_tmp"): to the server to truncate. """ return derived_identifier(self.search_table + suffix, "_staged_label") + def _grant_kind_for_relation(self, table): + """ + Which kind of relation one of this table's relations is. + + By identity, not by suffix: a search table may itself be called + something ending in ``_stats`` (the LMFDB has one), and reading the + kind off the name would give it a stats table's privileges. + """ + return { + self.search_table: "search", + self.stats.counts: "counts", + self.stats.stats: "stats", + }.get(table, "search") + + def _apply_post_swap_grant_policies(self, tables, backup_suffix): + """ + Give each relation the privileges of what it now is. + + INPUT: + + - ``tables`` -- the live names that were just swapped + - ``backup_suffix`` -- the suffix the relations they replaced now carry + + PostgreSQL ACLs follow a relation through a rename, so a swap hands the + live table's privileges to the backup and the backup's to the table + that just went live. Every swap therefore has to say again what each + relation is: a reload and a revert are the same exchange in opposite + directions, and an invariant that held only one way would leave a + reverted table with its backup's privileges. + """ + for table in tables: + backup = table + backup_suffix + self._db._apply_grant_policy(table, self._grant_kind_for_relation(table)) + self._db._apply_grant_policy(backup, "backup") + # The relation now live may have been built by _clone, which copies + # no privileges, so under a policy that grants nothing this swap can + # quietly take away access the live table had. + self._db._warn_if_access_was_lost(table, backup) def _next_backup_number(self): """ @@ -1913,10 +1951,11 @@ def _swap_in_tmp(self, tables): with DelayCommit(self, silence=True): self._swap(tables, "", "_old" + str(backup_number)) self._swap(tables, "_tmp", "") - for table in tables: - self._db.grant_select(table) - if table.endswith("_counts") or table.endswith("_stats"): - self._db.grant_insert(table) + # Which relation is which is known here, so the policy is looked up + # by it rather than guessed from the name: a search table may end + # in _stats (the LMFDB has one), and granting it a counts table's + # privileges would be wrong. + self._apply_post_swap_grant_policies(tables, "_old" + str(backup_number)) print( "Swapped temporary tables for %s into place in %s secs\nNew backup at %s" % ( @@ -2325,6 +2364,10 @@ def reload_revert(self, backup_number=None): - ``backup_number`` -- the backup version to restore, or ``None`` for the most recent. + + The grant policy is reapplied to both relations afterwards: PostgreSQL + privileges follow a relation through a rename, so without that the + restored table would come back with its backup's privileges. """ if self._table_exists(self.search_table + "_tmp"): print( @@ -2348,6 +2391,10 @@ def reload_revert(self, backup_number=None): self._swap(tables, "", "_tmp") self._swap(tables, old, "") self._swap(tables, "_tmp", old) + # The exchange moved each relation's privileges to the other one, + # so both are told again what they are -- inside this DelayCommit, + # with the renames. + self._apply_post_swap_grant_policies(tables, old) self._log_db_change("reload_revert") print( "Swapped backup %s with %s" diff --git a/tests/test_locks.py b/tests/test_locks.py index 233a39b..5977103 100644 --- a/tests/test_locks.py +++ b/tests/test_locks.py @@ -53,8 +53,12 @@ def sleeper(): runner = threading.Thread(target=sleeper) runner.start() try: + # pg_stat_activity.query is NULL for a session whose query text + # this role may not see, so another connection running while this + # test does must not turn the search into a TypeError assert wait_until(lambda: any( - q[0] == pid and "pg_sleep" in q[3] for q in db._get_queries() + q[0] == pid and q[3] and "pg_sleep" in q[3] + for q in db._get_queries() )), "the other session's query never showed up in _get_queries" db.show_queries() out = capsys.readouterr().out diff --git a/tests/test_security.py b/tests/test_security.py index 7b07fe7..03c0f68 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -43,6 +43,7 @@ ) from psycodict.encoding import Json from psycodict.table import PostgresTable +from psycodict.grants import GrantPolicy, LMFDBGrantPolicy from psycodict.utils import safe_child_path import conftest @@ -2560,3 +2561,296 @@ def test_two_keepers_of_different_kinds_may_share_a_name(db, table_factory, chec SQL("DELETE FROM meta_constraints WHERE constraint_name = %s"), [shared] ) db.drop_table(new, force=True) +################################################################## +# what a new relation is readable by # +################################################################## + + +def role_grants(db, table): + """ + The (grantee, privilege) pairs on a table, excluding the owner's own. + """ + cur = db._execute( + SQL( + "SELECT grantee, privilege_type FROM information_schema.role_table_grants " + "WHERE table_name = %s AND grantee <> grantor" + ), + [table], + ) + return sorted((grantee, privilege) for grantee, privilege in cur) + + +@pytest.fixture +def roles(db): + """ + Two disposable roles, or a skip where the test user may not make them. + """ + names = ["psycodict_test_reader", "psycodict_test_writer"] + try: + for name in names: + db._execute(SQL("CREATE ROLE {0}").format(Identifier(name))) + db.conn.commit() + except Exception: + db.conn.rollback() + pytest.skip("this test user may not create roles") + yield names + for name in names: + db._execute(SQL("DROP OWNED BY {0}").format(Identifier(name))) + db._execute(SQL("DROP ROLE {0}").format(Identifier(name))) + db.conn.commit() + + +def test_the_default_policy_grants_nothing(db, table_factory): + """ + A new table is not readable by anyone merely because a role exists. + """ + table = table_factory() + name = table.search_table + for relation in [name, name + "_counts", name + "_stats"]: + assert role_grants(db, relation) == [] + for relation in ["meta_tables", "meta_indexes", "meta_constraints", "meta_format"]: + assert role_grants(db, relation) == [] + + +def test_an_explicit_policy_grants_exactly_what_it_says(db, table_factory, roles, monkeypatch): + reader, writer = roles + monkeypatch.setattr( + db, + "grant_policy", + GrantPolicy({ + "search": {"SELECT": (reader,)}, + "counts": {"SELECT": (reader,), "INSERT": (writer,)}, + }), + ) + table = table_factory() + name = table.search_table + assert role_grants(db, name) == [(reader, "SELECT")] + assert role_grants(db, name + "_counts") == [(reader, "SELECT"), (writer, "INSERT")] + # a kind the policy says nothing about gets nothing + assert role_grants(db, name + "_stats") == [] + + +def test_the_lmfdb_policy_reproduces_the_old_behavior(db, roles, monkeypatch): + policy = LMFDBGrantPolicy() + assert policy.for_kind("search") == {"SELECT": ("lmfdb", "webserver")} + assert policy.for_kind("counts") == { + "SELECT": ("lmfdb", "webserver"), + "INSERT": ("webserver",), + } + # and nothing for the tables a reload leaves behind + assert policy.for_kind("backup") == {} + # the legacy statistics import was only ever readable + assert policy.for_kind("oldstats") == {"SELECT": ("lmfdb", "webserver")} + + +def test_a_missing_role_is_refused_and_nothing_is_created(db, monkeypatch): + name = "test_%s" % uuid.uuid4().hex[:12] + monkeypatch.setattr( + db, + "grant_policy", + GrantPolicy({"search": {"SELECT": ("no_such_role",)}}, missing_role="error"), + ) + with pytest.raises(ValueError, match="do not exist"): + db.create_table(name, [("n", "integer")], "n") + # the whole creation rolled back + assert not table_exists(db, name) + assert name not in db.tablenames + assert not db._execute( + SQL("SELECT 1 FROM meta_tables WHERE name = %s"), [name] + ).fetchone() + + +def test_a_missing_role_can_be_skipped_instead(db, table_factory, monkeypatch): + monkeypatch.setattr( + db, + "grant_policy", + GrantPolicy({"search": {"SELECT": ("no_such_role",)}}, missing_role="skip"), + ) + table = table_factory() + assert table.search_table in db.tablenames + assert role_grants(db, table.search_table) == [] + + +def test_a_reload_leaves_the_live_table_with_the_policy_and_the_backup_without( + db, filled_table, roles, monkeypatch, tmp_path +): + """ + A rename carries privileges with it, so the table a reload pushes aside + would keep the live table's readers unless the policy is applied to it too. + """ + reader, _ = roles + monkeypatch.setattr( + db, + "grant_policy", + GrantPolicy({"search": {"SELECT": (reader,)}}), + ) + name = filled_table.search_table + db._apply_grant_policy(name, "search") + assert role_grants(db, name) == [(reader, "SELECT")] + + searchfile = str(tmp_path / "search.txt") + filled_table.copy_to(searchfile) + filled_table.reload(searchfile) + + table = db[name] + assert table.count() == 200 + # the replacement is readable by exactly what the policy says + assert role_grants(db, name) == [(reader, "SELECT")] + # and the backup, which holds what was live a moment ago, is not + assert table._table_exists(name + "_old1") + assert role_grants(db, name + "_old1") == [] + for relation in [name + "_old1", name + "_counts_old1", name + "_stats_old1"]: + if table._table_exists(relation): + db._execute(SQL("DROP TABLE {0}").format(Identifier(relation))) + + +def test_a_policy_rejects_what_it_cannot_apply(): + with pytest.raises(ValueError, match="relation kind"): + GrantPolicy({"nosuchkind": {"SELECT": ("r",)}}) + with pytest.raises(ValueError, match="action"): + GrantPolicy({"search": {"TRUNCATE": ("r",)}}) + with pytest.raises(ValueError, match="missing_role"): + GrantPolicy({}, missing_role="ignore") + with pytest.raises(ValueError, match="sequence of role names"): + GrantPolicy({"search": {"SELECT": "lmfdb"}}) + with pytest.raises(ValueError, match="iterable of role names"): + GrantPolicy({"search": {"SELECT": 17}}) + with pytest.raises(ValueError, match="role name"): + GrantPolicy({"search": {"SELECT": ("",)}}) + with pytest.raises(ValueError, match="control character"): + GrantPolicy({"search": {"SELECT": ("role\x00",)}}) + + +def test_a_role_name_carrying_sql_cannot_build_a_statement(db, monkeypatch): + """ + Role names are quoted, and which role names are legal is PostgreSQL's + business, so a name carrying SQL is not refused for its shape -- it is + refused because no role is called that. + """ + marker = marker_name() + role = 'r"; CREATE TABLE %s (x integer); --' % marker + monkeypatch.setattr( + db, "grant_policy", GrantPolicy({"search": {"SELECT": (role,)}}) + ) + name = "test_%s" % uuid.uuid4().hex[:12] + with pytest.raises(ValueError, match="do not exist"): + db.create_table(name, [("n", "integer")], "n") + assert not table_exists(db, marker) + assert not table_exists(db, name) + + +def test_the_manual_grant_methods_need_explicit_roles(db, empty_table): + with pytest.raises(TypeError): + db.grant_select(empty_table.search_table) + with pytest.raises(TypeError): + db.grant_insert(empty_table.search_table) + + +def test_a_reload_that_takes_access_away_says_so(db, filled_table, roles, caplog, tmp_path): + """ + A reload swaps in a clone, which PostgreSQL creates with no privileges, so + a policy that grants nothing silently takes away what the live table had. + It should not be discovered by a website going dark. + """ + reader, _ = roles + name = filled_table.search_table + db.grant_select(name, [reader]) + assert role_grants(db, name) == [(reader, "SELECT")] + + searchfile = str(tmp_path / "search.txt") + filled_table.copy_to(searchfile) + with caplog.at_level("WARNING"): + filled_table.reload(searchfile) + assert reader in caplog.text + assert "grant_policy" in caplog.text + for relation in [name + "_old1", name + "_counts_old1", name + "_stats_old1"]: + if db._table_exists(relation): + db._execute(SQL("DROP TABLE {0}").format(Identifier(relation))) + + +def test_a_policy_materializes_one_shot_role_iterables(): + """ + Validating a generator would consume it, and the policy would then name + nobody -- failing closed, but silently and against what it was given. + """ + roles = (role for role in ["reader", "writer"]) + policy = GrantPolicy({"search": {"SELECT": roles}}) + assert policy.for_kind("search")["SELECT"] == ("reader", "writer") + assert policy.roles == ["reader", "writer"] + + +def test_the_relation_kind_comes_from_identity_not_the_name(db, table_factory): + """ + A search table may be called something ending in _stats, so the kind has to + be looked up rather than read off the name. + """ + table = table_factory() + assert table._grant_kind_for_relation(table.search_table) == "search" + assert table._grant_kind_for_relation(table.stats.counts) == "counts" + assert table._grant_kind_for_relation(table.stats.stats) == "stats" + + +def test_a_revert_gives_each_relation_back_its_own_privileges( + db, filled_table, roles, monkeypatch, tmp_path +): + """ + PostgreSQL privileges follow a relation through a rename, so a revert + exchanges the live table's ACL with its backup's just as a reload does. + Both directions have to say again what each relation is. + """ + reader, _ = roles + monkeypatch.setattr( + db, "grant_policy", GrantPolicy({"search": {"SELECT": (reader,)}}) + ) + name = filled_table.search_table + db._apply_grant_policy(name, "search") + assert role_grants(db, name) == [(reader, "SELECT")] + + searchfile = str(tmp_path / "search.txt") + filled_table.copy_to(searchfile) + filled_table.reload(searchfile) + table = db[name] + assert role_grants(db, name) == [(reader, "SELECT")] + assert role_grants(db, name + "_old1") == [] + + try: + # revert: the backup goes live and the live table becomes the backup + table.reload_revert(1) + assert role_grants(db, name) == [(reader, "SELECT")] + assert role_grants(db, name + "_old1") == [] + + # and back again -- reload_revert toggles + db[name].reload_revert(1) + assert role_grants(db, name) == [(reader, "SELECT")] + assert role_grants(db, name + "_old1") == [] + finally: + for relation in [name + "_old1", name + "_counts_old1", name + "_stats_old1"]: + if db._table_exists(relation): + db._execute(SQL("DROP TABLE {0}").format(Identifier(relation))) + + +def test_the_legacy_statistics_import_is_not_an_ordinary_stats_table( + db, empty_table, roles, monkeypatch, tmp_path +): + """ + create_oldstats granted SELECT and nothing else; classifying its relation + as a stats table would hand the website INSERT on it under the LMFDB + policy. + """ + reader, writer = roles + monkeypatch.setattr( + db, + "grant_policy", + GrantPolicy({ + "stats": {"SELECT": (reader,), "INSERT": (writer,)}, + "oldstats": {"SELECT": (reader,)}, + }), + ) + empty = tmp_path / "oldstats.txt" + empty.write_text("") + name = empty_table.search_table + "_oldstats" + empty_table.stats.create_oldstats(str(empty)) + try: + assert role_grants(db, name) == [(reader, "SELECT")] + finally: + db._execute(SQL("DROP TABLE IF EXISTS {0}").format(Identifier(name)))