Skip to content

fix(aws): route the CD deploy payload through the authenticated s3:// fetch path - #2221

Merged
lionello merged 1 commit into
mainfrom
fix/aws-cd-payload-s3-scheme
Aug 19, 2026
Merged

fix(aws): route the CD deploy payload through the authenticated s3:// fetch path#2221
lionello merged 1 commit into
mainfrom
fix/aws-cd-payload-s3-scheme

Conversation

@defangdevs

Copy link
Copy Markdown
Contributor

Summary

cd's fetchPayload only authenticates s3:// URIs (via the deploy container's ambient AWS credentials, see fetchS3 in pulumi-defang's cd/fetch.go). The AWS BYOC driver strips the query string off the presigned upload URL and passes the bare virtual-hosted-style https://bucket.s3.region.amazonaws.com/key URL to cd up. That routes through fetchHTTP, which does a plain unsigned GET — a private bucket 403s it.

This mirrors the gs:// rewrite already done for GCP in gcp/byoc.go (with the same comment: "Only gs:// is supported in the payload as http get in gcpcd does not handle auth yet"). AWS never got the equivalent rewrite.

Context

Found while re-validating DefangLabs/pulumi-defang#423 (AWS CD image shell-base fix + defang#2217's entrypoint fix) against DefangLabs/defang-mvp#3181's new-provider-sanity smoketest. With those two fixes in place, the AWS leg now provisions and runs the correct entrypoint, but fails on this new blocker:

cd pulumi failed to fetch payload: GET https://defang-cd-bucket-cybpbzz8hzm7.s3.us-west-2.amazonaws.com/uploads/t7jl0wwq4cz9 returned 403 Forbidden

I confirmed the CodeBuild task role (defang-cd-TaskRole-*) has PowerUserAccess, which includes s3:GetObject — so a signed request through fetchS3 should succeed; the request is just never signed because it isn't recognized as an S3 URI.

Change

Adds s3PayloadURI, which rewrites bucket.s3.region.amazonaws.com/key (with or without a region segment) into s3://bucket/key; non-S3 URLs and base64 payloads pass through unchanged.

Test plan

  • go test -short ./pkg/cli/client/byoc/aws/... passes, including new TestS3PayloadURI cases (regional URL, legacy no-region URL, non-S3 URL, base64 payload)
  • golangci-lint run ./pkg/cli/client/byoc/aws/... — no new findings (4 pre-existing gosec findings, unrelated to this diff)
  • Re-dispatch new-provider-sanity.yml with aws-cli-version pointed at this branch to confirm the AWS leg gets past the payload fetch

… fetch path

cd's fetchPayload only signs s3:// URIs, using the deploy container's
ambient AWS credentials. The presigned upload URL, once stripped of its
query string, is a bare virtual-hosted-style https:// URL, so cd's
fetchHTTP did an unsigned GET and a private bucket 403'd it. Rewrite it
to s3://bucket/key, mirroring the existing gs:// rewrite in gcp/byoc.go.

Found while re-validating DefangLabs/pulumi-defang#423 (AWS CD image
shell base + entrypoint fixes) against DefangLabs/defang-mvp#3181's
new-provider-sanity smoketest.
@defangdevs
defangdevs requested a review from lionello as a code owner August 19, 2026 19:02
@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@defangdevs, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 9 minutes

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 687f80c3-1c52-497c-abc9-6c5f38a7b112

📥 Commits

Reviewing files that changed from the base of the PR and between 65fbdc6 and 9da058d.

📒 Files selected for processing (2)
  • src/pkg/cli/client/byoc/aws/byoc.go
  • src/pkg/cli/client/byoc/aws/byoc_test.go

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lionello lionello left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this work for both old defang-mvp TS CD and the new pulumi-defang Golang CD?

@defangdevs

Copy link
Copy Markdown
Contributor Author

Yes, works for both:

  • New pulumi-defang Go CD (cd/fetch.go's fetchPayload): this is the one that actually needs the fix. It only signs/authenticates s3:// URIs via fetchS3; anything else (including a bare virtual-hosted-style https://bucket.s3.region.amazonaws.com/key) falls through to fetchHTTP, an unsigned GET that 403s on a private bucket.

  • Old defang-mvp TS CD (pulumi/index.ts's readPayload + shared/s3.ts's normalizeS3Url): unaffected either way, since it does its own normalization on the CD side — it already recognizes both virtual-hosted-style (bucket.s3.region.amazonaws.com) and path-style (s3.region.amazonaws.com/bucket) HTTPS URLs and rewrites them to s3:// before doing an authenticated s3Client.getObject. Feeding it an already-s3:// URI (what this PR now produces) is a no-op for normalizeS3Urls3Url.startsWith("s3://") is true immediately — so it still routes through the authenticated S3 client path exactly as before.

So this CLI-side rewrite is required for the new Go CD and harmless (pass-through) for the old TS CD.

@lionello
lionello merged commit cfaa16e into main Aug 19, 2026
15 checks passed
@lionello
lionello deleted the fix/aws-cd-payload-s3-scheme branch August 19, 2026 20:11
defangdevs added a commit that referenced this pull request Aug 21, 2026
Four merges landed on main after the previous merge: #2225, #2226, #2224
(buildkit bump) and #2221. The conflicts were all dependency state, none
in the cleanup code:

- src/go.mod, src/go.sum: this branch carried AWS SDK pins from when it
  was opened in June, main has newer ones. Resolved to main's versions
  and re-ran `go mod tidy`, which re-added the ecr, elbv2 and rds modules
  this branch needs at their current versions.
- pkgs/defang/cli.nix: took main's vendorHash as the closer starting
  point. The Update vendorHash step in go.yml corrects and commits it if
  the tidy moved it.

src/pkg/cli/client/byoc/aws/byoc.go merged cleanly.

`go build ./...` and `go test -short ./...` green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T3WmpdY3zc555sNdkY9dzQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants