Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 113 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,119 @@ hardening standalone use; the highlights:
- `postgresql_dbname` was the one option whose default ignored the `defaults`
dictionary passed to `Configuration`. (#119)

### Security

- **Column types are validated against the whole string, not a prefix.** The
type of a column has to be interpolated into `CREATE TABLE` and `ALTER TABLE`
as SQL text, since PostgreSQL has no placeholder for a type. The check that
guarded that interpolation matched a prefix, so a type beginning with a valid
type — `text; ...`, `varchar(16); ...` — was accepted with the rest attached
and executed. `create_table`, `create_table_like`, `add_column` and the
column types read from the header of a reloaded data file
(`reload(adjust_schema=True)`, `reload_all(adjust_schema=True)`) were all
affected. Types are now validated by `psycodict.base.validate_column_type`,
which matches the complete string and returns the spelling that callers must
emit; the character-type grammar it replaces was also malformed, so
`varchar(N)`, `char(N)` and `character varying(N)` are now described
deliberately rather than accepted as prefixes of `varchar`. An invalid type
raises `InvalidColumnTypeError`, a subclass of both `ValueError` and the
`RuntimeError` raised before.

- **Index and constraint definitions are validated when imported and again
when used.** `meta_indexes` and `meta_constraints` hold what an index or
constraint is rebuilt from, sometimes years and several psycodict versions
after it was created, and in between the rows can be edited with plain SQL,
exported to a file, carried to another database and imported, or restored
from history. `create_index` and `create_constraint` checked their arguments,
but nothing rechecked a row on the way back out, and the access method,
column modifiers, storage-parameter names, check function and partial-index
predicate were formatted into the statement as text. A poisoned
`meta_indexes` row could therefore run its own statements the next time the
index was restored. Definitions are now checked by
`validate_index_definition` / `validate_constraint_definition` at import
(`reload_indexes`, `reload_constraints`, the `revert_*` methods) and again
immediately before the DDL is built, and everything they are built from is
either a quoted identifier or fixed SQL selected by a validated key. An
invalid definition raises `InvalidDefinitionError` (a `ValueError`). Names
are held to what they must be rather than to a convention: a column is quoted
wherever it is used and any string can be one, and a relation name is not
length-checked at DDL time because psycodict makes the names it uses by
appending `_tmp` or `_oldN` to one that already exists. Because
the import runs inside the reload transaction, the metadata that was there
before survives intact. A partial-index predicate remains raw administrative
SQL, but may no longer contain a semicolon, a comment, a dollar-quoted string
or control characters. The unused, unvalidated `_copy_from_meta` helper is
gone.

- **A reconnect reproduces the connection it replaces.** `reset_connection`
rebuilt the connection from the configuration alone, dropping the overrides
passed to `PostgresDatabase(...)` — TLS mode and certificates, an alternate
host, port, database or user, libpq `options`, connect and keepalive
timeouts. A connection that dropped could come back weaker than the one it
replaced, or somewhere else entirely. Options now come from one place
(`_connection_options`), used for the first connection and every replacement;
session setup moved into `_configure_session`, so the statement timeout and
friends are reapplied rather than lost with the session they were set on; and
a replacement is checked to have reached the same database as the same role,
with the read-only, superuser, knowls and userdb capability flags recomputed
from it instead of carried over. Retry behavior is unchanged: a standalone
statement is retried once, and a statement inside `DelayCommit` still raises
rather than replaying a transaction whose earlier statements are gone.
- **`PostgresDatabase(session_settings=...)`** applies `statement_timeout`,
`lock_timeout`, `idle_in_transaction_session_timeout` or `application_name`
to every connection the database opens. The 25 second statement timeout for
the `webserver` role is now the default value of this setting rather than a
one-off `SET` in the constructor.

- **Search table names have one grammar, and table objects have their own
registry.** A table name is used as a PostgreSQL identifier, as the key the
table object is reached by, and as the stem of the files `copy_to` writes,
but nothing checked it against any of those uses. A name with a path
separator or a `..` component sent an export out of the folder it was asked
for, and because table objects were stored in the database object's instance
dictionary, a table named `conn`, `config` or `tablenames` — from
`create_table` or from a `meta_tables` row edited with plain SQL — replaced
that attribute. Names are now checked by `validate_search_table_name`
(lowercase letters, digits and underscores, starting with a letter, at most
63 bytes, not ending in a suffix psycodict appends to a table's own name, not
the name of something else on the database object) in `create_table`,
`create_table_like` and `rename_table`; names already in `meta_tables` are
held to the identifier rule alone, since an existing database is entitled to
a name that predates the convention; table objects live in `db.tables`; and
the files an export generates go through
`safe_child_path`, which resolves symlinks and refuses anything that is not
under the requested folder. *Migration:* `db.tablenames` is now a sorted
property derived from `db.tables` rather than a list to mutate, and
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`.

### Infrastructure

- A test suite of 500+ tests and a continuous-integration workflow covering the
Expand Down
34 changes: 33 additions & 1 deletion DataManagement.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,19 @@ 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. 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`.

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`
Expand All @@ -21,7 +34,7 @@ db.create_table(
)
```

* **`name`** must contain an underscore. The call also creates the companion tables `name_counts` and `name_stats`, and writes one row to `meta_tables`.
* **`name`** must be lowercase letters, digits and underscores, start with a letter, be at most 63 bytes, and not end in one of the suffixes psycodict appends to a search table's own name (`_counts`, `_stats`, `_pkey`, `_tmp`, `_oldN`, `_depN`) or collide with an attribute of the database object. By convention it contains an underscore. The name is used as a PostgreSQL identifier, as the key the table object is reached by (`db.name`), and as the stem of the files `copy_to` writes. A name already in `meta_tables` is held to less than this — an existing database is entitled to a name that predates the convention, and the LMFDB has a search table called `hgcwa_per_group_stats` — but it must still be an identifier: a row edited to something with a path separator, a `..` component or whitespace in it makes `refresh_tables` fail rather than be acted on. The call also creates the companion tables `name_counts` and `name_stats`, and writes one row to `meta_tables`.
* **`search_columns`** takes two forms: a dictionary whose keys are Postgres types and whose values are lists of column names (a bare string is allowed for a single column), or a list of `(column, type)` pairs. The two forms are interchangeable.
* **the `id` column** is added automatically as a `bigint` primary key unless you already list one; use `id_type=` to choose a different integer type. Columns are physically laid out ordered by type (widest alignment first) for storage efficiency, so the on-disk column order is not your declaration order — but every file operation is header-driven, so this never matters to you.
* **`label_col`** names the column used by `lookup`; it must be one of the search columns, or `None`.
Expand All @@ -48,6 +61,25 @@ psycodict keeps its own bookkeeping in a handful of tables that live alongside y
* **`meta_tables_hist`**, **`meta_indexes_hist`**, **`meta_constraints_hist`** — versioned history of the three tables above, so that `reload_meta`/`revert_meta` can roll a table's metadata forward and back.
* **`meta_format`** — a single-row `(version, min_compat)` stamp of the metadata *format*: the layout of the `meta_*` tables, versioned by an integer aligned with psycodict's major version (`META_FORMAT`). Every connection checks it. The same format connects silently; an older but compatible format connects with a warning and operates at that older format (newer features unavailable) so a not-yet-migrated or read-only database — the LMFDB devmirror, say — keeps working; a newer format connects when its stamped `min_compat` admits this psycodict, and is otherwise refused. A database that has meta tables but no `meta_format` is the unstamped 0.x baseline (format 0); one with no meta tables at all is fresh and must be connected to with `PostgresDatabase(create=True)`, which bootstraps all of the above. Migrating to a newer format is deliberate — `db.upgrade_metadata()` or `PostgresDatabase(upgrade=True)`, never a side effect of connecting. See [MetadataFormats.md](MetadataFormats.md) for the full policy (including why a pre-1.0 psycodict must not be pointed at a migrated database).

Rows of `meta_indexes` and `meta_constraints` become DDL when an index or
constraint is rebuilt, so psycodict validates them both when they are imported
(`reload_indexes`, `reload_constraints`, `revert_indexes`, `revert_constraints`)
and again immediately before the `CREATE INDEX` or `ALTER TABLE` statement is
built. The access method, the column modifiers, the storage-parameter names and
the check function of a `CHECK` constraint must be ones psycodict recognizes,
and are emitted as quoted identifiers or fixed SQL rather than as the stored
text. A metadata file is therefore no more trusted than the database it is
loaded into: an invalid definition raises, and because the import runs in a
transaction, the metadata that was there before is left untouched. A `CHECK`
constraint's function must be listed in `PostgresTable._valid_check_functions`
both to create it and to rebuild it.

The predicate of a partial index (`create_index(..., where=...)`) stays raw SQL
— psycodict does not parse it — but it is checked for the things that would let
it stop being a predicate: a semicolon, a comment, a dollar-quoted string, a
control character or an unreasonable length. It is administrative input, not
website input.

## Row-level writes

These mutate the live table directly. They are convenient for small edits; for anything large prefer the [file operations](#bulk-file-operations). Each records an entry in the change log and, on success, updates the maintained row `total`.
Expand Down
10 changes: 10 additions & 0 deletions SECURITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,16 @@ with untrusted input. Injection caused solely by deliberately passing untrusted
input through `$raw` is not considered a psycodict vulnerability. A way to
inject SQL through an interface that is intended to be safe is in scope.

The `where=` predicate of a partial index (`create_index`) is likewise an
administrative SQL expression: psycodict does not parse it, and it must not be
built from untrusted input. It is validated to the extent that it must remain a
predicate — it may not contain a statement terminator, a comment or a
dollar-quoted string — so that a predicate stored in `meta_indexes` cannot turn
a later `CREATE INDEX` into two statements. Index and constraint definitions
read back out of the `meta_*` tables, out of an exported metadata file or out
of the history tables are validated before they are turned into DDL, so a
poisoned metadata row is in scope.

Problems in PostgreSQL, psycopg, or another dependency should normally be
reported to that project unless psycodict uses the dependency unsafely.
Ordinary bugs without a security impact may be reported through the public
Expand Down
5 changes: 5 additions & 0 deletions docs/api/grants.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# psycodict.grants

```{eval-rst}
.. automodule:: psycodict.grants
```
3 changes: 3 additions & 0 deletions docs/api/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -36,6 +38,7 @@ table
statstable
encoding
config
grants
utils
notifications
dbdiff
Expand Down
Loading