From 1850be812669d05d7295f1ece27779b53d31ecdd Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Mon, 14 Sep 2026 15:24:43 +0000 Subject: [PATCH] fix: refuse userinfo in an object-store endpoint (#995) #997 closed `s3://u:p@bucket/key` -- userinfo in the URL the CALLER writes. The endpoint the OPERATOR configures was still unguarded, and the bucket guard cannot see it, because it is not in the URL at all. TWO SHAPES, AND ONLY ONE WAS EVER CAUGHT. Measured on the authority parse before writing anything: http://u:p@host:30829 -> host "u", port 0 refused, wrong reason http://user@host:30829 -> host "user@host", port 30829 port VALID http://host:30829 -> host "host", port 30829 (clean control) The first has a colon INSIDE the userinfo, so the authority split lands there and the port becomes atoi("p@host:30829") = 0. The second has no such colon: the real port survives, the '@' rides along in the host, and the invalid-port refusal never fires at all. #995 measured the first and concluded "refused as invalid host or port" -- true of that shape rather than of the code. THE SECOND SHAPE IS WHY THIS IS A GUARD AND NOT A MESSAGE CHANGE. Its refusal came from the allow-list, naming the host it could not match, and the hint then told the operator: ALTER SYSTEM SET pgcolumnar.objstore_allowed_endpoints = 'user@host' A diagnostic that invites widening a security boundary to accommodate a parse bug is worse than a wrong error code. Following it moves the failure from the allow-list to a DNS miss. PLACED BEFORE THE SCHEME AND REGION DEMANDS, not at the authority parse eighty lines later. Placed there the guard is unreachable whenever no region is configured, and an operator with a userinfo endpoint and no AWS_REGION is told about the region. MEASURED: every endpoint arm returned "requires a region option" until it moved -- the unreachable guard trap #995 itself names. The message names the ENDPOINT rather than the URL, because that is the string carrying the userinfo. Removal proof, on both harnesses, each run asserting the .so was REBUILT AND INSTALLED (mtime moved) before its verdict is read: bash CONTROL 26 passed guard removed 5 failed restored 26 passed pytest CONTROL 5 passed guard removed 3 failed restored 5 passed Source restored to md5 0de468b1 after each. TWO TRAPS WALKED INTO WHILE MEASURING, both already in the record: `PGC_SKIP_BUILD=0` SKIPS THE BUILD. lib.sh tests `[ -z "${PGC_SKIP_BUILD:-}" ]`, so any non-empty value means skip -- including `0`, which reads as "do not skip". The first run measured a .so from the previous day and reported five failures against code that did not contain the guard. The suite said so on line 5 of its own output and I did not read it; the timestamps did. `git checkout -- FILE` REVERTED THE UNCOMMITTED FIX. The removal proof's restore took the file back to HEAD, which did not have the guard, so the final control failed identically to the mutant. Caught because the restore's md5 assertion did not fire. Fixed by committing before mutating. Arms in test/objstore_userinfo.sh beside the existing ones, with a clean-endpoint control in the same run, and independently in test/pytest/test_objstore_endpoint_userinfo.py -- which is NOT a port and not a pair, since `objstore_endpoint_userinfo.sh` does not exist. objstore_userinfo.sh 26 checks, 0 failed (pg16a) guard leg 340 passed, 867 checks, 0 fail cluster leg 325 passed, 898 checks, 0 fail `cluster_tests` 320 -> 325 by collection. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs --- CHANGELOG.md | 36 ++++ objstore/columnar_objstore_module.c | 50 ++++++ test/objstore_userinfo.sh | 63 +++++++ test/pytest/TESTS.md | 14 ++ test/pytest/expected_tests.txt | 8 +- .../pytest/test_objstore_endpoint_userinfo.py | 155 ++++++++++++++++++ 6 files changed, 325 insertions(+), 1 deletion(-) create mode 100644 test/pytest/test_objstore_endpoint_userinfo.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 33e28fb0..6ad0b461 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,42 @@ true until the next version shipped. ### Added +- Userinfo in an object-store ENDPOINT was accepted, and the diagnostic told the + operator to allow-list it (#995). + + #997 closed `s3://u:p@bucket/key` -- userinfo in the URL the caller writes. The + endpoint the OPERATOR configures was still unguarded, and the bucket guard cannot see + it because it is not in the URL at all. + + TWO SHAPES, AND ONLY ONE WAS EVER CAUGHT. Measured on the authority parse: + + http://u:p@host:30829 -> host "u", port 0 refused, wrong reason + http://user@host:30829 -> host "user@host", port 30829 port VALID + + The first has a colon inside the userinfo, so the split lands there and the port + becomes `atoi("p@host:30829")` = 0. The second has no such colon: the real port + survives, the `@` rides along in the host, and the invalid-port refusal never fires. + #995 measured the first and concluded "refused as invalid host or port", which is + true of that shape rather than of the code. + + The second shape is why this is a guard and not a message change. Its refusal came + from the allow-list, naming the host it could not match, and the hint then said: + + ALTER SYSTEM SET pgcolumnar.objstore_allowed_endpoints = 'user@host' + + A diagnostic that invites widening a security boundary to accommodate a parse bug is + worse than a wrong error code. Following it moves the failure from the allow-list to + a DNS miss. + + The guard sits BEFORE the scheme and region demands rather than at the authority + parse eighty lines later. Placed there it is unreachable whenever no region is + configured: measured, every endpoint arm reported `requires a region option` until it + moved. That is the same reasoning the bucket guard carries, and the trap #995 named. + + Arms in `test/objstore_userinfo.sh` beside the existing ones, with a clean-endpoint + control in the same run, and in `test/pytest/test_objstore_endpoint_userinfo.py` + independently. + - A bash suite that unrolls a family as literals graded MISSING against the port that parametrises it (#1045 class 3). diff --git a/objstore/columnar_objstore_module.c b/objstore/columnar_objstore_module.c index 3db1403f..69b90bdf 100644 --- a/objstore/columnar_objstore_module.c +++ b/objstore/columnar_objstore_module.c @@ -1391,6 +1391,56 @@ os_resolve_s3(PgColumnarObjHandle *h, const char *url, ep = os_require_env("AWS_ENDPOINT_URL", url); } } + /* + * Userinfo in the ENDPOINT, which #997 left (#995). The bucket guard above + * cannot see it: the userinfo is in the endpoint the operator configured, + * not in the s3:// URL the caller wrote. + * + * HERE, AND NOT AT THE AUTHORITY PARSE BELOW. Placed there it sits behind the + * region demand, so an endpoint carrying userinfo with no region configured + * reports the region instead -- and an arm for it would need a region set for + * no reason connected to what it tests. MEASURED: all five endpoint arms in + * test/objstore_userinfo.sh returned "requires a region option" until this + * moved. That is the same reasoning the bucket guard carries for sitting + * before the endpoint is resolved at all, and the trap #995 named. + * + * The scheme is not yet checked here, so the scan is the whole endpoint + * rather than its authority. An '@' cannot occur in "http://" or "https://", + * and one in a PATH is still a malformed endpoint, so the wider scan refuses + * the same set and costs nothing. + * + * TWO SHAPES, AND ONLY ONE WAS EVER CAUGHT -- measured on the parse itself: + * + * http://u:p@host:30829 -> host "u", port 0 <- refused + * http://user@host:30829 -> host "user@host", port 30829 <- port VALID + * + * The first has a colon INSIDE the userinfo, so the authority split lands + * there and the port becomes atoi("p@host:30829") = 0. The second has no such + * colon, the real port survives, and the invalid-port refusal never fires at + * all. #995 measured the first and concluded "refused as invalid host or + * port" -- true of that shape, not of the code. + * + * The second shape is why this is a guard and not a message change. Its + * refusal came from the allow-list, naming the host it could not match, and + * the hint then told the operator: + * + * ALTER SYSTEM SET pgcolumnar.objstore_allowed_endpoints = 'user@host' + * + * A diagnostic that invites widening a security boundary to accommodate a + * parse bug is worse than a wrong error code. + * + * The message names the ENDPOINT rather than `url`, because that is the + * string carrying the userinfo. + */ + if (strchr(ep, '@') != NULL) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("columnar: userinfo in object-store endpoint \"%s\" is " + "not supported", ep), + errhint("Remove the user:password@ from the endpoint and supply " + "credentials through AWS_ACCESS_KEY_ID and " + "AWS_SECRET_ACCESS_KEY."))); + if (pg_strncasecmp(ep, "https://", 8) == 0) { #ifdef HAVE_OBJSTORE_OPENSSL diff --git a/test/objstore_userinfo.sh b/test/objstore_userinfo.sh index b0effc9c..0f4b896d 100755 --- a/test/objstore_userinfo.sh +++ b/test/objstore_userinfo.sh @@ -140,4 +140,67 @@ check "a clean URL to a non-allowed endpoint is still the allow-list's 42501" \ check "backend alive" "$(q 'SELECT 1;')" "1" +# --- and userinfo in the ENDPOINT, which #997 left (#995) -------------------- +# +# The bucket guard above cannot see this one: the userinfo is in the endpoint the +# operator configured, not in the s3:// URL the caller wrote. Reached here through a +# foreign server's `endpoint` option, which is `cfg->endpoint` in os_resolve_s3 -- the +# same variable `AWS_ENDPOINT_URL` feeds, so one arm per SHAPE proves the guard and no +# postmaster restart is needed. That the env source also reaches it is what the +# AWS_ENDPOINT_URL control above already shows. +# +# TWO SHAPES, AND ONLY ONE OF THEM USED TO BE CAUGHT -- measured on the parse itself +# before writing the guard: +# +# http://u:p@127.0.0.1:1 -> host "u", port 0 <- port 0, refused +# http://user@127.0.0.1:1 -> host "user@127.0.0.1", port 1 <- port VALID +# +# The first has a colon INSIDE the userinfo, so the authority split lands there and the +# port becomes atoi("p@127.0.0.1:1") = 0. The second has no such colon, so the real port +# survives and the '@' rides along in the host. #995 measured only the first and +# concluded "refused as invalid host or port" -- true of that shape, not of the code. +# +# The second shape is the one that argues for the guard. Its refusal came from the +# allow-list, naming the host it could not match, and the HINT then told the operator: +# +# ALTER SYSTEM SET pgcolumnar.objstore_allowed_endpoints = 'user@127.0.0.1' +# +# A diagnostic that invites widening a security boundary to accommodate a parse bug is +# worse than a wrong error code, which is why the arms below pin the message and not +# only the SQLSTATE. +q "CREATE SERVER s3ui FOREIGN DATA WRAPPER pgcolumnar_parquet + OPTIONS (endpoint 'http://u:p@127.0.0.1:1');" >/dev/null +q "CREATE FOREIGN TABLE ft_ui (id int) SERVER s3ui + OPTIONS (path 's3://mybucket/x.parquet');" >/dev/null +q "CREATE SERVER s3ui2 FOREIGN DATA WRAPPER pgcolumnar_parquet + OPTIONS (endpoint 'http://user@127.0.0.1:1');" >/dev/null +q "CREATE FOREIGN TABLE ft_ui2 (id int) SERVER s3ui2 + OPTIONS (path 's3://mybucket/x.parquet');" >/dev/null +q "CREATE SERVER s3ok FOREIGN DATA WRAPPER pgcolumnar_parquet + OPTIONS (endpoint 'http://127.0.0.1:1');" >/dev/null +q "CREATE FOREIGN TABLE ft_ok (id int) SERVER s3ok + OPTIONS (path 's3://mybucket/x.parquet');" >/dev/null + +check "a userinfo endpoint is refused by the parse guard (22023)" \ + "$(sqlstate_of "SELECT * FROM ft_ui")" "22023" +check "and the message names userinfo" \ + "$(msg_of "SELECT * FROM ft_ui")" "1" +check "and it names the ENDPOINT, not the s3:// URL, which carries none" \ + "$(msg_has "SELECT * FROM ft_ui" 'endpoint')" "1" +# The shape the port check never caught: no colon before the '@', so the real port +# survived and only the allow-list refused it -- at 42501, with a hint to allow-list it. +check "the user@host shape is refused by the same guard (22023, not an allow-list 42501)" \ + "$(sqlstate_of "SELECT * FROM ft_ui2")" "22023" +check "and its message names userinfo too" \ + "$(msg_of "SELECT * FROM ft_ui2")" "1" + +# THE CONTROL. Without it these five cannot tell a userinfo refusal from "this foreign +# server cannot reach anything" -- the same trap the s3 controls above exist for, and +# the one that wasted a run on #995's first probe. A clean endpoint must get PAST the +# guard and fail for a connection reason instead. +check "control: a clean endpoint is not refused as userinfo" \ + "$(msg_of "SELECT * FROM ft_ok")" "0" +check "control: and it gets past the parse to a connection failure, not 22023" \ + "$([ "$(sqlstate_of "SELECT * FROM ft_ok")" = "22023" ] && echo no || echo yes)" "yes" + pgc_summary diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index d482ef3c..fdb7c151 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -3875,6 +3875,20 @@ the tool grades THIS tree. | `test_every_pair_in_the_tree_is_declared` | the declaration is asserted BOTH ways, so a new pair cannot be silently ungraded | | `test_the_ported_suites_in_this_tree_are_graded_one_for_one` | the standing arm: every pair in the tree, graded | + +### `test_objstore_endpoint_userinfo.py` -- userinfo in an object-store endpoint (#995) + +Not a port and not a pair: `objstore_endpoint_userinfo.sh` does not exist. These assert +the same properties as `test/objstore_userinfo.sh`'s endpoint arms, independently, +through the python harness. + +| test | asserts | +| --- | --- | +| `test_a_userinfo_endpoint_is_refused` | both shapes refuse at `22023`, naming userinfo and naming the ENDPOINT rather than the s3:// URL | +| `test_the_guard_fires_without_a_region_configured` | the placement: with no region set the refusal is userinfo, not the region demand | +| `test_a_clean_endpoint_is_not_refused_as_userinfo` | the control -- a clean endpoint gets past the guard and fails for another reason | +| `test_an_at_sign_in_the_object_key_is_not_userinfo` | the other direction: `@` is legal in a key and is untouched | + ### `test_iceberg_fdw.py` -- the Iceberg FDW's pruning surface (#388, #432) Ports `test/iceberg_fdw.sh`. 74 of its 76 check names, one for one; the two it cannot diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index f7a52680..8cafbae9 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -180,4 +180,10 @@ guard_tests 340 # right here; two independent 322s merged just as silently earlier today and the # merged tree collected 323. Re-derive BOTH after every merge, not the one git # complained about. -cluster_tests 320 +# 320 -> 325 when test_objstore_endpoint_userinfo.py landed (#995): the two userinfo +# endpoint shapes, the placement premise that the guard fires with no region +# configured, the clean-endpoint control, and the '@'-in-the-key control. NOT a port +# and not a pair -- `objstore_endpoint_userinfo.sh` does not exist; these assert the +# same properties through the python harness independently. +# Re-derived by collection: `325 tests collected`. +cluster_tests 325 diff --git a/test/pytest/test_objstore_endpoint_userinfo.py b/test/pytest/test_objstore_endpoint_userinfo.py new file mode 100644 index 00000000..b00837ad --- /dev/null +++ b/test/pytest/test_objstore_endpoint_userinfo.py @@ -0,0 +1,155 @@ +"""Userinfo in an object-store ENDPOINT is refused, and refused before anything else +the endpoint is missing (#995). + +`s3://u:p@bucket/key` — userinfo in the URL the CALLER writes — was closed by #997. +This is the other half: userinfo in the endpoint the OPERATOR configures, which the +bucket guard cannot see because it is not in the URL at all. + +WHY IT IS A GUARD AND NOT A MESSAGE CHANGE. Both shapes failed closed already, and by +two different accidents: + + http://u:p@host:30829 -> host "u", port 0 <- refused, wrong reason + http://user@host:30829 -> host "user@host", port 30829 <- port VALID + +The first has a colon inside the userinfo, so the authority split lands there and the +port becomes `atoi("p@host:30829")` = 0. The second has no such colon: the real port +survives, the `@` rides along in the host, and the invalid-port refusal never fires. +What caught it instead was the allow-list, whose hint then told the operator + + ALTER SYSTEM SET pgcolumnar.objstore_allowed_endpoints = 'user@host' + +— a diagnostic inviting them to widen a security boundary to accommodate a parse bug. + +THIS FILE IS NOT A PORT of `objstore_userinfo.sh` and does not pair with it. It +asserts the same properties through the python harness, independently: nothing here +sources, invokes or reads anything under `test/*.sh`, and it behaves identically if +that suite is deleted. +""" +import pytest + +ALLOWED = "127.0.0.1" + +# endpoint, alias, and what the shape is called. The two userinfo shapes are separate +# rows because they used to fail by DIFFERENT mechanisms, and a single row would not +# have told them apart. +ENDPOINTS = [ + ("http://u:p@127.0.0.1:1", "ui_pass", + "user:password@ — the colon inside the userinfo used to eat the port"), + ("http://user@127.0.0.1:1", "ui_bare", + "user@ — no colon, so the port stayed valid and only the allow-list refused it"), +] + + +@pytest.fixture(scope="module") +def endpoints(pgc_cluster): + """-> (connection, {alias: foreign table}) over servers with the endpoints above. + + A FOREIGN SERVER rather than `AWS_ENDPOINT_URL`, because the environment variable + is read from the POSTMASTER's environment and would need a restart. Both reach the + same `ep` in `os_resolve_s3`, so the server option exercises the guard without one. + """ + import psycopg + + conn = psycopg.connect(pgc_cluster.dsn(), autocommit=True) + with conn.cursor() as cur: + cur.execute("CREATE EXTENSION IF NOT EXISTS pgcolumnar") + # SUSET, and this connection is a superuser, so no restart is needed. Set at + # all because an EMPTY allow-list refuses every endpoint, which would make a + # clean control indistinguishable from a refused one. + cur.execute(f"SET pgcolumnar.objstore_allowed_endpoints = '{ALLOWED}'") + tables = {} + for endpoint, alias, _why in ENDPOINTS + [("http://127.0.0.1:1", "clean", "")]: + cur.execute(f"CREATE SERVER srv_{alias} FOREIGN DATA WRAPPER " + f"pgcolumnar_parquet OPTIONS (endpoint '{endpoint}')") + cur.execute(f"CREATE FOREIGN TABLE ft_{alias} (id int) SERVER srv_{alias} " + f"OPTIONS (path 's3://mybucket/x.parquet')") + tables[alias] = f"ft_{alias}" + yield conn, tables + conn.close() + + +def _error(conn, sql): + """-> (the exception, its message) for a statement expected to fail. + + THE EXCEPTION ITSELF, not its sqlstate string: `expect.sqlstate` refuses a bare + string, because a string carries no evidence that the comparison is about an error + at all. It caught that here on the first run. + """ + import psycopg + try: + with conn.cursor() as cur: + cur.execute(sql) + return None, "" + except psycopg.Error as exc: + return exc, str(exc) + + +@pytest.mark.parametrize("endpoint,alias,name", [ + (e, a, f"a {w.split(' — ')[0]} endpoint is refused by the parse guard") + for e, a, w in ENDPOINTS +]) +def test_a_userinfo_endpoint_is_refused(endpoints, expect, endpoint, alias, name): + conn, tables = endpoints + exc, message = _error(conn, f"SELECT * FROM {tables[alias]}") + expect.num(1 if exc is not None else 0, 1, + f"premise: {endpoint} raises at all, so the state below is an error's") + expect.sqlstate(exc, "22023", name) + expect.num(1 if "userinfo" in message else 0, 1, + f"and the message for {endpoint} names userinfo") + # The offending string is the ENDPOINT. Saying `userinfo in "s3://bucket/key"` + # would name a URL that carries none, and send the reader to the wrong place. + expect.num(1 if "endpoint" in message else 0, 1, + f"and it names the endpoint rather than the s3:// URL, for {endpoint}") + + +def test_the_guard_fires_without_a_region_configured(endpoints, expect): + """THE PLACEMENT, which is the part a message change would not have given. + + The guard sits before the scheme and region demands rather than at the authority + parse eighty lines later. Placed there it would be unreachable whenever no region + is configured -- an operator with a userinfo endpoint and no `AWS_REGION` would be + told about the region, and an arm for it would need a region set for no reason + connected to what it tests. + + MEASURED: every endpoint arm returned `requires a region option` until the guard + moved. This cluster configures no region, so a `userinfo` message here IS the + reachability claim. + """ + conn, tables = endpoints + _exc, message = _error(conn, f"SELECT * FROM {tables['ui_pass']}") + expect.num(1 if "region" in message else 0, 0, + "no region is configured, and the refusal is NOT about the region") + expect.num(1 if "userinfo" in message else 0, 1, + "so the userinfo guard is reached before the region demand") + + +def test_a_clean_endpoint_is_not_refused_as_userinfo(endpoints, expect): + """THE CONTROL, without which the arms above are worth nothing. + + #995's first probe proved exactly this much and no more: with no credentials every + s3 URL failed, the clean one included, so a userinfo refusal was indistinguishable + from an object store that could not be reached at all. A clean endpoint must get + PAST the guard and fail for some other reason. + """ + conn, tables = endpoints + exc, message = _error(conn, f"SELECT * FROM {tables['clean']}") + expect.num(1 if "userinfo" in message else 0, 0, + "a clean endpoint is not refused as userinfo") + state = getattr(exc, "sqlstate", "noerror") + expect.text("refused" if state == "22023" else "reached further", "reached further", + "and it is not the parse guard's 22023 at all, so the guard is " + "discriminating rather than refusing everything") + + +def test_an_at_sign_in_the_object_key_is_not_userinfo(endpoints, expect): + """AND THE OTHER DIRECTION. `@` is legal in an S3 key, and the guard scans the + ENDPOINT, so a key carrying one must be untouched. Without this the arms above + would also pass on a guard that refused every `@` anywhere. + """ + conn, tables = endpoints + with conn.cursor() as cur: + cur.execute("CREATE FOREIGN TABLE ft_atkey (id int) SERVER srv_clean " + "OPTIONS (path 's3://mybucket/my@file.parquet')") + _exc, message = _error(conn, "SELECT * FROM ft_atkey") + expect.num(1 if "userinfo" in message else 0, 0, + "an '@' in the object KEY is not userinfo and is not refused as it")