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")