Repository navigation
fix(storage): sign presigned URLs for CDN_PUBLIC_ENDPOINT, not the internal CDN_ENDPOINT - #1873
pyramation wants to merge 1 commit into
Conversation
…ternal CDN_ENDPOINT
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. No blocking issues — approved ✅; 1 nitpick below. 🧹 Nitpicks (1) — 🟢 1 low
This PR introduces a dual-endpoint S3 configuration: an internal endpoint for data operations and a public endpoint used solely for presigning, so the SigV4 signature is computed for the host the browser will hit rather than the internal one. The presigned-URL resolver builds a second
Reviewed commit: d91cbd5 |
|
Superseded by combined #1876 (CI green), which contains this branch unchanged. |
Summary
Fixes constructive-io/constructive-planning#2136.
What's broken: in Kubernetes,
CDN_ENDPOINTis the in-cluster storage host (e.g.http://rustfs.constructive-infra.svc.cluster.local:9000). Server-side S3 calls need it, but the presigned PUT/GET URLs fromuploadFiles/uploadPlatformFiles/downloadUrlwere signed with the same client. Browsers and Desktop run outside the cluster and can't resolve that host, so every upload failed.What this changes: a new optional setting,
CDN_PUBLIC_ENDPOINT→cdn.publicEndpoint. When it's set, the server still talks to storage overCDN_ENDPOINT, but presigned URLs are signed by a second S3 client built for the public endpoint. When it's unset, nothing changes: URLs are signed withendpoint, which is all single-endpoint local dev needs.All client-facing presigned URLs go through
generatePresignedPutUrl/generatePresignedGetUrl: therequestUploadUrl/ upload mutations inplugin.tsand thedownloadUrlfield.resolveS3ForDatabasespreads the global config, so per-database bucket configs keeppresignClient. Server-side calls (headObject,copyS3Object,deleteS3Object,readObjectPrefix, the streaming upload lane, the bucket provisioner) still useclient, which points at the internal endpoint.Decisions
Hostheader, so replacing the host after signing makes the signature invalid. The new test shows this.CDN_PUBLIC_URL_PREFIX. That prefix builds unsigned public-object URLs and can be a CDN domain that doesn't speak the S3 API. A signing endpoint has to be the S3 API host.presignClientis optional onS3Config, so plugin users with a single endpoint don't need to change anything.CDN_ENDPOINT=https://s3.us-east-1.amazonaws.com, which is already public. The legacyk8s/overlays/localMinIO is ClusterIP-only, so it has no public host to point at.compute/.env.k8s(the local kind stack from the issue) can now setCDN_PUBLIC_ENDPOINTto the host-side RustFS port-forward. That's a follow-up in that repo; this PR doesn't touch it.Tests
graphile-presigned-url-plugin/__tests__/s3-signer.test.ts(new) uses realS3Clients with fixed credentials and a fixed clock. It checks that PUT and GET URLs use the internal host when no public endpoint is configured and the public host when one is. An independent SigV4 verifier (recomputing the signature withcrypto) confirms the signature validates for the host in the URL and fails once the host is rewritten.graphile-settings/__tests__/presigned-url-resolver.test.ts:presignClientis built frompublicEndpointonly when that is set.pgpm/env/__tests__/merge.test.ts:CDN_PUBLIC_ENDPOINTmaps tocdn.publicEndpoint, which is unset by default.Without the fix, the new signer suite fails:
S3Confighas nopresignClient.Rollout
Any deployment whose
CDN_ENDPOINTis internal-only should setCDN_PUBLIC_ENDPOINTto its client-reachable storage host.Link to Devin session: https://app.devin.ai/sessions/abe5c10fecb54c08aa88f3e3760dd577
Open in Devin Desktop: https://app.devin.ai/desktop/session/abe5c10fecb54c08aa88f3e3760dd577?variant=devin
Requested by: @pyramation