diff --git a/docs/doctoring/bootstrap-dsn-precedence.md b/docs/doctoring/bootstrap-dsn-precedence.md index d7bd2fa15..6ab0198c3 100644 --- a/docs/doctoring/bootstrap-dsn-precedence.md +++ b/docs/doctoring/bootstrap-dsn-precedence.md @@ -6,6 +6,8 @@ For the required Postgres DSN, an explicitly empty or whitespace-only value must not be replaced by `PG_LLM_BATCH_DSN` or passed down to libpq defaults. Non-string explicit values must also fail at the package boundary rather than reaching unrelated lower-layer behavior. For the optional Fernet bootstrap key, an explicit empty string is a deliberate statement that no key was supplied for this invocation and must not inherit an ambient key. +A separate CLI confidentiality boundary applies before bootstrap resolution. PostgreSQL connection information can carry passwords, password-file locations, TLS private-key material, TLS key passwords, and OAuth client secrets. Accepting those values through `--dsn` copies credential material or credential-bearing locations into process invocation state, where operating-system process inspection and shell history can expose them. The CLI therefore needs to retain explicit database targeting without making credential-bearing conninfo a normal argv transport. + ## Contract `resolve_dsn()` distinguishes source absence, source type, and source value: @@ -23,22 +25,44 @@ For the required Postgres DSN, an explicitly empty or whitespace-only value must - an explicit empty string remains the empty string and does not inherit `PG_LLM_BATCH_SECRET_KEY`; and - when neither source provides a key, the result remains `None`. -This boundary does not make secret persistence, serialization, or transport safer by itself. It only prevents ambient bootstrap state from silently replacing explicitly supplied caller intent. +The standalone CLI adds a narrower transport rule for explicit `--dsn` values: + +- parse the supplied value with Psycopg/libpq-compatible `conninfo_to_dict()` rather than ad-hoc URI or keyword matching; +- permit credential-free selectors such as password-free PostgreSQL URIs, keyword conninfo, and `service=` selectors; +- reject conninfo that explicitly contains `password`, `passfile`, `sslkey`, `sslpassword`, or `oauth_client_secret` before bootstrap resolution or database connection work; +- reject malformed conninfo with a fixed parser diagnostic that does not reproduce the rejected argv value; and +- preserve the exact accepted selector string so downstream source-precedence and libpq semantics remain unchanged. + +The CLI restriction does not prohibit standard libpq authentication. Operators may keep password/private-key material outside argv using reviewed libpq mechanisms such as the default password file, `PGPASSFILE`, a connection service file, default or environment-selected TLS key material, or deployment-owned secret injection. `PG_LLM_BATCH_DSN` remains a bootstrap transport and is not claimed to be a universal secrets manager; deployments should select an appropriate secret mechanism for their threat model. + +This boundary does not make secret persistence, serialization, transport, TLS, or server identity safe by itself. It prevents two specific authority/confidentiality failures: ambient bootstrap state silently replacing explicit caller intent, and credential-bearing explicit CLI conninfo becoming process-argument data. ## Verification `tests/test_bootstrap_source_precedence.py` proves the replacement behavior against the public bootstrap helpers. The regressions populate ambient environment values while passing explicit invalid values so a rejected caller value cannot be confused with ordinary omitted-input fallback. They also prove that an omitted whitespace-only DSN is rejected, a valid explicit DSN retains exact text, and an explicit empty secret key remains explicit. -The fail-first replacement head demonstrated that protected-main truthiness selected ambient values or admitted the wrong type before the production repair. The production repair then moved fallback behind an explicit `None` check and added exact-string and required-nonblank validation. Final acceptance still requires the repository's complete exact-head CI, security, coverage, package, provenance, and review policy on the unchanged final source. +`tests/test_cli_dsn_argv_security.py` defines the CLI transport contract. It requires password-bearing PostgreSQL URIs, keyword `password=`, and explicit `passfile=` values to fail without reflecting a unique secret sentinel; it separately requires malformed conninfo to fail without reflection and confirms that credential-free URI, keyword, and service selectors retain exact text. + +The fail-first bootstrap replacement head demonstrated that protected-main truthiness selected ambient values or admitted the wrong type before the production repair. The CLI fail-first branch independently demonstrated that protected main accepted credential-bearing `--dsn` values unchanged. The CLI production repair uses libpq-compatible parsing only to classify whether argv contains prohibited credential parameters; it does not rewrite accepted connection information or change bootstrap precedence. Final acceptance still requires the repository's complete exact-head Python 3.10/3.12/3.14, 100% owned production statement/branch coverage, public docstrings, package, security, SAST, required-workflow, review-thread, and live ruleset evidence on one unchanged final source. ## Compatibility and rollback -Constructor and CLI call shapes remain unchanged. Callers that intentionally depended on explicit empty/non-string values falling through to environment state must now omit the argument to request environment fallback. Valid explicit DSNs and keys retain their original string values. +Bootstrap helper call shapes remain unchanged. Callers that intentionally depended on explicit empty/non-string values falling through to environment state must now omit the argument to request environment fallback. Valid explicit DSNs and keys retain their original string values. -Rollback is an ordinary Git revert of this bounded bootstrap change. Rolling back reintroduces ambiguous authority selection and should occur only with a documented compatibility requirement and a safer replacement contract. +The CLI keeps `--dsn` for explicit database selection but no longer accepts credentials or credential-file/private-key parameters in that process argument. Existing automation that embeds such material in `--dsn` must move authentication data to a standard libpq mechanism outside argv while preserving its database selector. This is an intentional confidentiality hardening, not silent credential removal. + +Rollback is an ordinary Git revert of the bounded change. Rolling back the bootstrap rule reintroduces ambiguous authority selection; rolling back the CLI rule reintroduces credential-bearing process arguments. Either rollback should occur only with a documented compatibility requirement and a safer replacement contract. ## References +MITRE. (2026). *CWE-214: Invocation of process using visible sensitive information* (CWE Version 4.20). https://cwe.mitre.org/data/definitions/214.html + +PostgreSQL Global Development Group. (2026). *PostgreSQL 18 documentation: Database connection control functions*. https://www.postgresql.org/docs/18/libpq-connect.html + +PostgreSQL Global Development Group. (2026). *PostgreSQL 18 documentation: The password file*. https://www.postgresql.org/docs/18/libpq-pgpass.html + +The Psycopg Team. (2026). *Psycopg 3 documentation: `conninfo` — manipulate connection strings*. https://www.psycopg.org/psycopg3/docs/api/conninfo.html + Python Software Foundation. (2026). *argparse — Parser for command-line options, arguments and subcommands*. Python 3.14 documentation. https://docs.python.org/3.14/library/argparse.html Python Software Foundation. (2026). *os — Miscellaneous operating system interfaces*. Python 3.14 documentation. https://docs.python.org/3.14/library/os.html diff --git a/pg_llm_batch/cli.py b/pg_llm_batch/cli.py index b43fefb0a..8d09cded3 100644 --- a/pg_llm_batch/cli.py +++ b/pg_llm_batch/cli.py @@ -17,7 +17,9 @@ serve-healthz serve GET /healthz The DSN is resolved from --dsn or the PG_LLM_BATCH_DSN bootstrap env var only. -All other config/secrets come from the database KV stores. Secret plaintext and +Command-line DSNs may select a database but may not carry password/private-key +credentials; use standard libpq secret mechanisms outside process argv. All +other config/secrets come from the database KV stores. Secret plaintext and count-tokens prompt content are never accepted as command-line arguments. """ @@ -33,6 +35,9 @@ from contextlib import ExitStack from typing import List, Optional +from psycopg import ProgrammingError +from psycopg.conninfo import conninfo_to_dict + from . import db from .batch_api_client import BatchAPIClient, config_credentials_provider from .bootstrap import resolve_dsn, resolve_secret_key @@ -44,6 +49,15 @@ MAX_SECRET_INPUT_CHARACTERS = 65_536 MAX_TOKEN_INPUT_BYTES = 1_048_576 SECRET_LINE_SEPARATORS = frozenset("\n\r\v\f\x1c\x1d\x1e\x85\u2028\u2029") +CLI_DSN_SENSITIVE_PARAMETERS = frozenset( + { + "password", + "passfile", + "sslkey", + "sslpassword", + "oauth_client_secret", + } +) class _RedactingArgumentParser(argparse.ArgumentParser): @@ -59,12 +73,32 @@ def error(self, message: str) -> None: super().error(redacted_message) +def _validate_cli_dsn(value: str) -> str: + """Accept valid libpq selectors while refusing credential-bearing argv data.""" + try: + parameters = conninfo_to_dict(value) + except ProgrammingError: + raise argparse.ArgumentTypeError( + "Postgres DSN must be valid libpq connection information" + ) from None + if CLI_DSN_SENSITIVE_PARAMETERS.intersection(parameters): + raise argparse.ArgumentTypeError( + "Credential-bearing Postgres DSNs are not accepted in --dsn; " + "use libpq secret mechanisms outside process argv" + ) + return value + + def _add_common(parser: argparse.ArgumentParser) -> None: - """Add the shared ``--dsn`` option to a subcommand parser.""" + """Add the shared credential-free ``--dsn`` selector to a subcommand parser.""" parser.add_argument( "--dsn", default=None, - help="Postgres DSN (else PG_LLM_BATCH_DSN bootstrap env var)", + type=_validate_cli_dsn, + help=( + "Credential-free Postgres selector " + "(else PG_LLM_BATCH_DSN bootstrap env var)" + ), ) @@ -227,7 +261,7 @@ def build_parser() -> argparse.ArgumentParser: p_serve = sub.add_parser("serve-healthz", help="Serve GET /healthz") _add_common(p_serve) - p_serve.add_argument("--host", default="0.0.0.0") + p_serve.add_argument("--host", default="127.0.0.1") p_serve.add_argument("--port", type=int, default=8080) return parser diff --git a/tests/test_cli_dsn_argv_security.py b/tests/test_cli_dsn_argv_security.py new file mode 100644 index 000000000..c10ef31ed --- /dev/null +++ b/tests/test_cli_dsn_argv_security.py @@ -0,0 +1,90 @@ +# SPDX-License-Identifier: Apache-2.0 +"""Regression tests for PostgreSQL credential disclosure through CLI argv.""" + +from __future__ import annotations + +import pytest + +from pg_llm_batch import cli + + +@pytest.mark.parametrize( + "credential_dsn", + [ + "postgresql://app:secret-sentinel@db.example/batch", + "postgres://app:secret-sentinel@db.example/batch", + "host=db.example dbname=batch user=app password=secret-sentinel", + "host=db.example dbname=batch user=app passfile=/tmp/secret-sentinel.pgpass", + "host=db.example dbname=batch user=app sslkey=/tmp/secret-sentinel.key", + "host=db.example dbname=batch user=app sslpassword=secret-sentinel", + "host=db.example dbname=batch user=app oauth_client_secret=secret-sentinel", + ], +) +def test_cli_rejects_credential_bearing_dsn_arguments_without_reflection( + credential_dsn: str, + capsys: pytest.CaptureFixture[str], +) -> None: + """Credential-bearing DSNs fail in parsing without echoing sensitive argv.""" + parser = cli.build_parser() + + with pytest.raises(SystemExit): + parser.parse_args(["health", "--dsn", credential_dsn]) + + captured = capsys.readouterr() + assert "secret-sentinel" not in captured.err + assert "secret-sentinel" not in captured.out + + +def test_cli_rejects_malformed_dsn_without_reflection( + capsys: pytest.CaptureFixture[str], +) -> None: + """Malformed conninfo fails with a fixed parser diagnostic.""" + parser = cli.build_parser() + + with pytest.raises(SystemExit): + parser.parse_args( + ["health", "--dsn", "host=db.example password=secret-sentinel broken"] + ) + + captured = capsys.readouterr() + assert "secret-sentinel" not in captured.err + assert "secret-sentinel" not in captured.out + + +@pytest.mark.parametrize( + "selector", + [ + "postgresql://db.example/batch?sslmode=verify-full", + "host=db.example dbname=batch sslmode=verify-full", + "service=pg-llm-batch", + ], +) +def test_cli_retains_credential_free_explicit_database_selectors(selector: str) -> None: + """Explicit database targeting remains usable when argv contains no secret.""" + args = cli.build_parser().parse_args(["health", "--dsn", selector]) + + assert args.dsn == selector + + +def test_serve_healthz_cli_defaults_to_loopback() -> None: + """Direct CLI readiness serving must not bind every host interface by default.""" + args = cli.build_parser().parse_args( + ["serve-healthz", "--dsn", "postgresql://db.example/batch"] + ) + + assert args.host == "127.0.0.1" + + +def test_serve_healthz_cli_allows_explicit_container_binding() -> None: + """Container callers may deliberately request an all-interface listener.""" + args = cli.build_parser().parse_args( + [ + "serve-healthz", + "--dsn", + "postgresql://db.example/batch", + "--host", + "0.0.0.0", + ] + ) + + assert args.host == "0.0.0.0"