diff --git a/CHANGELOG.md b/CHANGELOG.md index d57dfad3..e74f47e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2054,6 +2054,52 @@ true until the next version shipped. The blobs stay reachable in the repository's history -- removing a file from the tree does not unwrite it, and rewriting `main` is not something a stray artifact justifies. What this stops is the tree carrying them, and the next `git add -A` re-adding them. +- `s3://` and `gs://` now refuse URL userinfo, as `http(s)://` has since #706. They were + absorbing it into the bucket name and reporting a missing object (#995). + + http://u:p@127.0.0.1:1/x.parquet 22023 userinfo in "..." is not supported + s3://u:p@mybucket/x.parquet 58P01 "..." does not exist (HTTP 404) <- before + s3://u:p@mybucket/x.parquet 22023 userinfo in "..." is not supported <- after + + The old message was true and useless: the object did not exist *because* `u:p@mybucket` + had become a bucket name. `s3://user:key@bucket/obj` is a form other tools accept, so it + is a mistake a user makes, and the answer sent them looking for a missing object. + + **Measured on main with an endpoint configured**, which is the only way to reach the + authority parse at all: + + path style HEAD /u%3Ap%40pgc-bucket/vh.parquet, Host untouched -> HTTP 404 + virtual host the bucket becomes the leftmost Host label -> could not resolve + "u:p@pgc-bucket.s3.local" + write export_parquet raised NO ERROR and PUT to + /u%3Ap%40pgc-bucket/ui.parquet -- a bucket nobody named + + **Not an SSRF, and that was checked rather than assumed.** Nothing splits the authority + at `@`, so the userinfo never becomes basic auth and never moves the host. The write row + is why this is refused rather than documented: acceptance there sends bytes to a bucket + the caller did not ask for. + + The guard sits **before the endpoint is resolved**. Placed after it, the refusal would be + unreachable whenever no endpoint is configured, because the s3 branch demands one first + -- and the arms could then say nothing without an object-store fixture. + + **Twelve arms** added to `test/objstore_userinfo.sh` -- five refusals and **seven + controls**, taking the file from 7 checks to 19 -- because + the first probe of this gap proved nothing: with no credentials every s3 URL returned + `28000`, the clean one included, so a userinfo refusal was indistinguishable from an + unreachable object store. + + The controls hold that a clean URL still reaches the endpoint demand **and that the + message names `AWS_ENDPOINT_URL`**, that a malformed URL still gets the bucket/key + refusal, and that an `@` in the KEY is left alone. + + That positive matcher matters more than it looks: `28000` is raised by **three** different + demands in `os_resolve_s3` -- a missing endpoint, a missing credential, and the + authorization refusal -- so a control that sees `28000` and no `userinfo` has pinned + nothing about which one fired. The `gs://` control makes the point concretely: its `28000` + is the **credential** demand, because `gs` defaults its endpoint to the interop host and + never reaches the endpoint demand at all. Same code, different cause, and only a matcher + that names the variable can tell them apart. ## [1.0-alpha3] - 2026-09-02 diff --git a/objstore/columnar_objstore_module.c b/objstore/columnar_objstore_module.c index d257431b..3db1403f 100644 --- a/objstore/columnar_objstore_module.c +++ b/objstore/columnar_objstore_module.c @@ -1342,6 +1342,40 @@ os_resolve_s3(PgColumnarObjHandle *h, const char *url, errmsg("columnar: \"%s\" is not %s://bucket/key", url, isGs ? "gs" : "s3"))); + /* + * Userinfo is refused here, the same as os_open and http_request do for + * http(s) (#706). This path never did, and the failure was quiet rather than + * open: the '@' is not special to a bucket name, so `s3://u:p@bucket/key` + * parsed, the userinfo became part of the BUCKET, and the caller was told the + * object does not exist. + * + * Measured on main with an endpoint configured (#995): + * + * path style HEAD /u%3Ap%40pgc-bucket/vh.parquet, Host untouched + * -> "does not exist (HTTP 404)" + * virtual host the bucket is the leftmost Host label, so the whole string + * goes to the resolver -> "could not resolve + * \"u:p@pgc-bucket.s3.local\"" + * write export_parquet raised NO ERROR and PUT to + * /u%3Ap%40pgc-bucket/ui.parquet -- a bucket the caller + * never named + * + * So this is a wrong-reason defect and not an SSRF: nothing here splits the + * authority at '@', and the userinfo never becomes basic auth or moves the + * host. The write row is why it is worth refusing rather than documenting -- + * acceptance there sends bytes somewhere nobody asked for. + * + * BEFORE THE ENDPOINT IS RESOLVED, deliberately. Placed after it, the refusal + * would be unreachable whenever no endpoint is configured, because the s3 + * branch demands one first (28000) -- and the arms for this in + * test/objstore_userinfo.sh would then need an object-store fixture to say + * anything at all. + */ + if (memchr(bucket, '@', slash - bucket) != NULL) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("columnar: userinfo in \"%s\" is not supported", url))); + if (cfg != NULL && cfg->endpoint != NULL) ep = cfg->endpoint; else diff --git a/test/objstore_userinfo.sh b/test/objstore_userinfo.sh index e30cc48b..b0effc9c 100755 --- a/test/objstore_userinfo.sh +++ b/test/objstore_userinfo.sh @@ -45,6 +45,20 @@ msg_of() { -d "$PGC_DB" -qtA -c "$1" 2>&1 | grep -c 'userinfo' } +# msg_of answers only "does the message say userinfo", so a control built on it asserts an +# ABSENCE. That is not enough here: 28000 is raised by THREE different demands in +# os_resolve_s3 -- a missing endpoint, a missing credential, and the allow-list's own +# authorization refusal -- so a control that sees 28000 and no `userinfo` has not pinned +# WHICH refusal fired. Reported by @jdatcmd in review on #997. +msg_has() { # msg_has SQL PATTERN -> count of matches in the ERROR MESSAGE alone + # The message alone, not the whole output: the HINT beside it also names the + # variable ("Set AWS_ENDPOINT_URL and restart"), so counting every line gives 2 + # and the arm would be pinned to how many places the hint mentions it rather + # than to which refusal fired. + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -qtA -c "$1" 2>&1 | sed -n 's/^ERROR: //p' | grep -c -- "$2" +} + URL="http://u:p@127.0.0.1:1/x.parquet" # --- premise: the read path refuses userinfo with the parse guard ------------ @@ -63,6 +77,61 @@ check "and the export_parquet message names userinfo" \ check "export_arrow refuses userinfo through the same handle (22023)" \ "$(sqlstate_of "SELECT pgcolumnar.export_arrow('ex', 'http://u@127.0.0.1:1/x.arrow')")" "22023" +# --- the same gap on the s3/gs path, which was never closed (#995) ----------- +# +# Measured on main with an endpoint configured: the userinfo is percent-encoded +# INTO THE BUCKET NAME -- `HEAD /u%3Ap%40pgc-bucket/vh.parquet`, Host untouched -- +# and the caller is told the object does not exist, which is true and useless. The +# object does not exist because we made `u:p@pgc-bucket` a bucket name. Under +# virtual-host addressing the same string becomes the leftmost label of the +# hostname and fails at DNS. +# +# Nothing splits the authority at '@', so this is a wrong-reason defect rather +# than an SSRF. The write path is the half with a consequence: `export_parquet` +# raised NO error and PUT to a bucket the caller never named. +# +# NO OBJECT-STORE SERVER IS NEEDED. The guard is in the bucket parse, which runs +# BEFORE the endpoint is resolved, so it fires whether or not one is configured -- +# which is also why these arms can live in this suite beside the http ones. +S3UI="s3://u:p@mybucket/x.parquet" +check "read_parquet refuses userinfo in an s3:// bucket authority (22023)" \ + "$(sqlstate_of "SELECT * FROM pgcolumnar.read_parquet('$S3UI') AS t(v int)")" "22023" +check "and the s3 read message names userinfo" \ + "$(msg_of "SELECT * FROM pgcolumnar.read_parquet('$S3UI') AS t(v int)")" "1" +check "a gs:// bucket authority is refused by the same guard" \ + "$(sqlstate_of "SELECT * FROM pgcolumnar.read_parquet('gs://u@mybucket/x.parquet') AS t(v int)")" "22023" +check "export_parquet refuses it on the WRITE path too, where acceptance writes to a bucket nobody named" \ + "$(sqlstate_of "SELECT pgcolumnar.export_parquet('ex', '$S3UI')")" "22023" +check "and the s3 write message names userinfo" \ + "$(msg_of "SELECT pgcolumnar.export_parquet('ex', '$S3UI')")" "1" + +# THE CONTROL, without which the five arms above are worth nothing. My first probe +# of this gap proved exactly nothing: with no credentials every s3 URL returned +# 28000, the CLEAN ONE INCLUDED, so a userinfo refusal was indistinguishable from +# an unreachable object store. A clean URL must still reach the endpoint demand. +check "control: a clean s3:// URL is not refused as userinfo, it demands an endpoint" \ + "$(sqlstate_of "SELECT * FROM pgcolumnar.read_parquet('s3://mybucket/x.parquet') AS t(v int)")" "28000" +check "control: and that refusal NAMES AWS_ENDPOINT_URL, pinning which 28000 fired" \ + "$(msg_has "SELECT * FROM pgcolumnar.read_parquet('s3://mybucket/x.parquet') AS t(v int)" \ + 'AWS_ENDPOINT_URL')" "1" +check "control: and it does not name userinfo, so the new guard did not fire on it" \ + "$(msg_of "SELECT * FROM pgcolumnar.read_parquet('s3://mybucket/x.parquet') AS t(v int)")" "0" +# The gs arm's 28000 is a DIFFERENT demand from the s3 arm's, which is the whole reason the +# two controls above are needed: gs defaults its endpoint to the interop host and then +# demands a credential, so it never reaches the endpoint demand at all. +check "control: the gs:// refusal names a CREDENTIAL, not the endpoint -- same 28000, other cause" \ + "$(msg_has "SELECT * FROM pgcolumnar.read_parquet('gs://mybucket/x.parquet') AS t(v int)" \ + 'AWS_ACCESS_KEY_ID')" "1" +check "control: a malformed s3:// URL is still the bucket/key refusal, naming no userinfo" \ + "$(msg_of "SELECT * FROM pgcolumnar.read_parquet('s3://nokey') AS t(v int)")" "0" +# The false positive this guard could plausibly have: '@' is legal in an S3 KEY, +# and refusing one would break a working read. The guard scans only up to the first +# slash, so the key is outside it -- asserted rather than trusted to the comment. +check "control: an '@' in the KEY is not userinfo, and is not refused as it" \ + "$(msg_of "SELECT * FROM pgcolumnar.read_parquet('s3://mybucket/my@file.parquet') AS t(v int)")" "0" +check "control: and such a URL still reaches the endpoint demand, so it was not refused earlier" \ + "$(sqlstate_of "SELECT * FROM pgcolumnar.read_parquet('s3://mybucket/my@file.parquet') AS t(v int)")" "28000" + # --- the allow-list still does its own job on a clean URL -------------------- # The guard must not have swallowed the 42501 class: a userinfo-free URL to a # non-allowed endpoint is still the allow-list's refusal.