Fix authorization_endpoint_url for blob uploads#11436
Fix authorization_endpoint_url for blob uploads#11436agup006 wants to merge 3 commits intofluent:masterfrom
Conversation
When using pre-signed URLs for blob uploads via authorization_endpoint_url), the plugin didn't extract or use the host from the pre-signed URL. It treated the URL as a URI path, so requests went to the wrong host or failed. This patch added s3_parse_presigned_url() to parse pre-signed URLs and extract host, URI, and port Updated put_blob_object(), complete_multipart_upload(), and abort_multipart_upload() to: - Extract the host from the pre-signed URL - Temporarily set ctx->s3_client->host to the extracted host - Validate the port matches the configuration - Restore the original host after the request Now blob uploads using pre-signed URLs now correctly use the host specified in the URL instead of the default S3 client host. Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
- Presigned multipart uploads now temporarily override both the S3 client host and upstream target when using authorization-generated URLs, ensuring each multipart operation connects to the correct presigned endpoint before restoring the original settings. - Presigned single-part blob uploads likewise update the upstream host/port while the presigned URL is in use so requests are sent to the right destination. Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
- Change port validation from error to debug log when using auth endpoint - Disable keepalive when switching hosts to force new connections - Disable TLS for HTTP (port 80) presigned URLs - Fix blob_fetch_multipart_abort_pre_signed_url using wrong endpoint URL
📝 WalkthroughWalkthroughThe changes add presigned URL parsing capability to S3 PutObject and multipart operations. A new function parses presigned URLs to extract host, port, and URI components. The S3 client configuration is temporarily swapped when using presigned URLs, with automatic restoration and proper resource cleanup on all code paths. Changes
Sequence DiagramsequenceDiagram
participant Operation as Multipart/PutObject
participant Parser as s3_parse_presigned_url
participant Config as S3 Client Config
participant Request as HTTP Request
participant Cleanup as Cleanup Handler
Operation->>Parser: Parse presigned URL
Parser->>Parser: Extract host, port, URI
Parser-->>Operation: Return parsed components
Operation->>Config: Save original host/port
Operation->>Config: Swap to presigned host/port
Operation->>Config: Adjust TLS/keepalive for presigned
Operation->>Request: Execute with presigned config
Request-->>Operation: Response received
Operation->>Cleanup: Trigger cleanup
Cleanup->>Config: Restore original host/port
Cleanup->>Cleanup: Free presigned resources
Cleanup-->>Operation: Complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5369de3a9c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /* Disable TLS for HTTP (port 80), enable for HTTPS (port 443) */ | ||
| if (presigned_port == 80) { | ||
| ctx->s3_client->upstream->base.flags &= ~FLB_IO_TLS; | ||
| } |
There was a problem hiding this comment.
The new presigned-host switch only clears TLS when the parsed port is 80, but it never re-enables TLS for HTTPS URLs. If authorization_endpoint_url is HTTP (so the original upstream flags have TLS disabled) and the presigned URL is HTTPS (e.g., https://... on 443 or another port), the client will keep TLS off and attempt a plain HTTP request to an HTTPS endpoint, which fails. The same mismatch happens for HTTP presigned URLs on non-80 ports (TLS stays enabled). Consider explicitly setting FLB_IO_TLS based on the URL scheme (or port) rather than only clearing it for port 80.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_s3/s3.c (1)
2652-2661:⚠️ Potential issue | 🟡 MinorLog message will include presigned URL query parameters.
When using a presigned URL,
uricontains the full path + query string (e.g.,/bucket/key?X-Amz-Algorithm=...). Thefinal_keycalculation at line 2657 (uri + strlen(ctx->bucket) + 1) skips the bucket prefix but retains the query parameters, producing a noisy log like:Successfully uploaded object /my-key?X-Amz-Algorithm=AWS4-HMAC-SHA256&...For virtual-hosted-style presigned URLs (where the bucket is in the hostname, not the path), this offset would point to the wrong position in the string entirely. Consider logging
uridirectly or truncating at?when in the presigned URL path.
🤖 Fix all issues with AI agents
In `@plugins/out_s3/s3_multipart.c`:
- Around line 449-455: The code relies on presigned_port to decide TLS which
fails for non-standard HTTP/HTTPS ports; change s3_parse_presigned_url to return
a boolean (e.g., use_tls) derived from the URL scheme (http vs https) and update
the call sites that currently inspect presigned_port to instead set/clear
FLB_IO_TLS based on that use_tls flag (explicitly set FLB_IO_TLS when use_tls is
true, clear it when false) for the ctx->s3_client->upstream->base.flags
manipulation; apply this same change to all five occurrences of the port-based
TLS toggle and keep the existing keepalive-disable line as-is.
🧹 Nitpick comments (2)
plugins/out_s3/s3_multipart.c (2)
416-455: Extract duplicated presigned URL setup/teardown into helpers.The presigned URL setup block (~20 lines: parse URL → save original state → override host/port/flags → disable TLS/keepalive) and the cleanup block (~8 lines: restore originals → free presigned_host) are copy-pasted into five functions (
complete_multipart_upload,abort_multipart_upload,create_multipart_upload,upload_part, andput_blob_objectin s3.c).Extracting these into two small helpers (e.g.,
s3_presigned_override_apply/s3_presigned_override_restore) would reduce ~115 lines of duplication and ensure future fixes (like the TLS port logic) propagate automatically.Also applies to: 515-526
727-773: Cleanup runs before response processing—correct but inconsistent with peer functions.Unlike
complete_multipart_upload,abort_multipart_upload, andupload_partwhich process the HTTP response before the cleanup label,create_multipart_uploadrestores host/freesuriat thecleanup:label and then falls through to processc. This works because the response incis self-contained, but the structural inconsistency may confuse future readers.Consider restructuring to match the pattern used by the other functions (process response first, then cleanup) for uniformity.
| /* Disable TLS for HTTP (port 80), enable for HTTPS (port 443) */ | ||
| if (presigned_port == 80) { | ||
| ctx->s3_client->upstream->base.flags &= ~FLB_IO_TLS; | ||
| } | ||
|
|
||
| /* Disable keepalive to force new connection to the new host */ | ||
| ctx->s3_client->upstream->base.flags &= ~FLB_IO_TCP_KA; |
There was a problem hiding this comment.
TLS toggle based on port 80 alone may miss non-standard HTTP ports.
The comment says "Disable TLS for HTTP (port 80), enable for HTTPS (port 443)" but only the disable-for-80 branch is implemented. If the authorization endpoint returns a presigned URL on a non-standard HTTP port (e.g., http://s3-proxy:8080/...), presigned_port will be 8080 and TLS won't be disabled, causing the request to fail against the original HTTPS-configured upstream.
Consider returning a use_tls boolean from s3_parse_presigned_url based on the URL scheme rather than relying on port number. This same issue appears in all five instances of this pattern.
🤖 Prompt for AI Agents
In `@plugins/out_s3/s3_multipart.c` around lines 449 - 455, The code relies on
presigned_port to decide TLS which fails for non-standard HTTP/HTTPS ports;
change s3_parse_presigned_url to return a boolean (e.g., use_tls) derived from
the URL scheme (http vs https) and update the call sites that currently inspect
presigned_port to instead set/clear FLB_IO_TLS based on that use_tls flag
(explicitly set FLB_IO_TLS when use_tls is true, clear it when false) for the
ctx->s3_client->upstream->base.flags manipulation; apply this same change to all
five occurrences of the port-based TLS toggle and keep the existing
keepalive-disable line as-is.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5369de3a9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /* Disable TLS for HTTP (port 80), enable for HTTPS (port 443) */ | ||
| if (presigned_port == 80) { | ||
| ctx->s3_client->upstream->base.flags &= ~FLB_IO_TLS; |
There was a problem hiding this comment.
The presigned URL path switches the upstream host/port, but TLS is only disabled when presigned_port == 80. If the authorization endpoint returns an HTTP URL on a non‑80 port (e.g., http://...:8080/...), s3_parse_presigned_url will set presigned_port to 8080 and the original FLB_IO_TLS flag remains enabled. That causes a TLS handshake against a plain‑HTTP endpoint, so uploads fail in that configuration. This logic is new with the presigned URL host/port handling and should instead key TLS on the URL scheme (or explicitly disable TLS for any non‑HTTPS scheme), not just port 80.
Useful? React with 👍 / 👎.
2 participants
Summary
This PR fixes several issues with the authorization_endpoint_url feature for blob uploads:
Port validation: Changed from error to debug log when presigned URL uses a different port than configured. When using an authorization endpoint, the presigned URL is to a different server, so port validation doesn't make sense.
Keepalive connection reuse: Disabled keepalive when switching hosts to force new connections. Previously, keepalive connections bound to the original endpoint would be reused, causing requests to go to the wrong server.
TLS handling: Disabled TLS for HTTP (port 80) presigned URLs. The auth endpoint may return HTTP URLs which require non-TLS connections.
Wrong endpoint URL: Fixed blob_fetch_multipart_abort_pre_signed_url which was incorrectly using /multipart_upload_presigned_url/ instead of /multipart_abort_presigned_url/.
Test Plan
Fixes issues identified in #11246
Summary by CodeRabbit
Release Notes
New Features
Improvements