fix: refuse userinfo in an object-store endpoint (#995)

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

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

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

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

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

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