Repository navigation
fix(server): honor an S3 endpoint scheme, admit storage destinations under destinations-only, scope DuckLake secrets to their prefix - #1444
Open
Sha-Bang wants to merge 4 commits into
Conversation
…mit storage destinations Signed-off-by: Nathan Huff <nathan@credibledata.com>
…d-destinations-only
Comment on lines
+119
to
+121
| const hostAndPath = endpoint | ||
| .slice(scheme ? scheme[0].length : 0) | ||
| .replace(/\/+$/, ""); |
… and tighten endpoint validation Signed-off-by: Nathan Huff <nathan@credibledata.com>
…d-destinations-only Signed-off-by: Nathan Huff <nathan@credibledata.com> # Conflicts: # packages/server/src/service/connection.ts # packages/server/src/service/connection_config.ts # packages/server/src/service/gcs_s3_utils.ts
Collaborator
Author
|
@claude review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three fixes that stand between a bucket file and a Malloy source, plus the docs for that recipe.
1. S3 endpoint scheme
An
s3attachment'sendpointreached DuckDB verbatim, sohttp://localhost:9000was requested ashttps://http://localhost%3A9000/…. The AWS SDK listing path had the opposite bug: a barehost:portthrewInvalid URL. No endpoint form worked on both.parseS3Endpoint(gcs_s3_utils.ts) splits an endpoint intohost[:port][/path]anduseSsl. A leadinghttp(s)://(any case) and trailing slashes are accepted;http://is the only thing that turns TLS off (USE_SSL falseon the secret). A query, fragment, whitespace, userinfo, another scheme, or an empty host is refused at load: none of those ever produced a working query, and DuckDB and the SDK parse them differently. Bare hosts produce byte-identical SQL. The key-based secret arms now share one clause list with the chain arm, which also fixes endpoint +sessionTokensilently dropping the token.2.
S3_CREDENTIAL_CHAIN_POLICY=destinations-onlyrejected the destinations it exists forOnly
validateStorageDestinationstold the gate a config was a storage destination. The load re-validation, the destination Malloy config rebuild, serve-time attach and the build session all used the default"connection"origin, so under the policy a credential_chain destination was dropped (the rebuild catch installs an empty config) or failednot permitted. The existingoriginargument is now threaded through those paths.attachDuckLakeReadOnly(no callers) and theattachDuckLakewrapper are deleted."storage-destination"is only set on inputs from validatedstorageDestinations; no connection route reaches it.destinations-onlystill fences the connection routes. Environment create/update accepting destinations is the existing contract, not widened here.3. Scope each DuckLake storage secret to its own prefix
Found while reviewing (2). A modeler's raw-SQL dimension (
sql_number(...)) is carried verbatim into the storage-serve shape and runs on the destination's DuckDB, which held an unscopedcredential_chainsecret (default scope: all ofs3://). On AWS that is the worker's role, and the managed tier bucket is shared across orgs under per-org prefixes, so such a dimension could read another org's prefix. This predates the PR; it touches the same function, so it is fixed here.Every DuckLake storage secret (S3 chain, S3 key, GCS) now carries
SCOPE '<bucketUrl>/', derived inattachDuckLakeWithModeand carried onCloudStorageCredentialsso the expired-credential renewer re-emits it. Destination S3 sessions also runSET enable_global_s3_configuration=false: otherwise DuckDB signs unmatched reads with a legacy key seeded fromAWS_*env, which bypasses the scope. GenericattachedDatabasesstay unscoped.Not closed by this: an author's SQL still runs unsandboxed in the destination session (SSRF, local-file reads). That is a separate design question (refuse
sql_*in serve-shape refinements, or sandbox the session).Tested
assembleEnvironmentConnections, an environment-level destination lookup, the build-session list/cleanup paths, the endpoint forms on both secret arms and the SDK client, and scope emission (trailing-slash normalization, GCS, renewer). Dropping each threadedoriginin turn fails a spec for the policy reason (the empty-list fallback rebuild can't be proven: nothing to validate).compile_restriction.spec.ts"accepts every committed .malloy file" fails locally on a missingpackages/skills/skillsdirectory, also onmain; unrelated.endpoint: "http://localhost:9000",lake.table('s3://bucket/x.csv') extend { measure: total is amt.sum() }.Could not resolve hostname … 'https://http://localhost%3A9000/bucket/x.csv'total = 60,count = 3So
conn.table('s3://…')compiles and runs through a DuckDB attachment.attachDuckLakeReadWritewith a prefixbucketUrl. A 200k-rowCREATE TABLE ASwrote parquet under the prefix and read back correctly; nothing touched the bare prefix. Reads of a sibling prefix and another bucket returned 403. Withenable_global_s3_configurationleft on andAWS_*set, the same reads succeeded, which is why theSETis there. Not run: a real AWS bucket with an IRSA role. Worth a read-only serve attach, a build and a read-back on the AWS dev cell before release.Docs
connections.mdgains "Data files in a bucket or at a link";configuration.mddocumentsS3_CREDENTIAL_CHAIN_POLICY.🤖 Generated with Claude Code