tls: support HTTPS proxy#11778
Open
antoniomrfranco wants to merge 5 commits intofluent:masterfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds HTTPS-proxy support with TLS-in-TLS chaining: proxy URL parsing accepts ChangesHTTPS Proxy with TLS-in-TLS Support
Sequence DiagramsequenceDiagram
participant Client
participant Proxy as HTTPS Proxy
participant Server
participant OpenSSL as OpenSSL Backend
Client->>Proxy: TCP connect
Client->>Proxy: TLS handshake (outer)
Proxy-->>Client: Outer TLS established
Note over Client,OpenSSL: Outer session available
Client->>Proxy: HTTP CONNECT (over outer TLS)
Proxy-->>Client: 200 OK
Client->>Server: TCP connect through tunnel
Client->>Server: TLS handshake for destination (inner)
OpenSSL->>OpenSSL: Chain inner TLS session to outer via session_set_outer/BIO
Server-->>Client: Inner TLS established
Client->>Server: Application traffic (nested TLS)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: daf51056d3
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/flb_io.c`:
- Around line 146-159: The proxy TLS session is created too late for the CONNECT
exchange; set the connection to use TLS before calling
flb_http_client_proxy_connect so flb_io_net_write()/flb_io_net_read() use the
TLS session (e.g., after flb_tls_session_create(...) succeeds, set
connection->io.flags |= FLB_IO_TLS), then call
flb_http_client_proxy_connect(...). If the ultimate upstream endpoint is plain
HTTP (no upstream TLS), clear the flag after a successful CONNECT
(connection->io.flags &= ~FLB_IO_TLS) so subsequent traffic uses plaintext
inside the established tunnel; reference flb_tls_session_create,
flb_http_client_proxy_connect, flb_io_net_write, flb_io_net_read, and the
FLB_IO_TLS flag when making this change.
In `@src/flb_upstream.c`:
- Around line 347-352: The proxy TLS context created by flb_tls_create for
u->proxy_tls_context is leaving hostname verification disabled, weakening the
proxy leg; after creating u->proxy_tls_context enable hostname verification (set
its verify_hostname flag) — e.g. call the appropriate API such as
flb_tls_set_verify_hostname(u->proxy_tls_context, FLB_TRUE) or directly set
u->proxy_tls_context->verify_hostname = FLB_TRUE if no setter exists — so the
proxy TLS hop enforces certificate name checks just like the destination leg.
In `@src/flb_utils.c`:
- Around line 1872-1875: The code currently allows "https" in the protocol
variable and logs only a generic error, causing broken behavior when the build
lacks TLS; update the conditional that checks protocol (the strcmp calls) to
also check the FLB_HAVE_TLS compile-time flag and, if FLB_HAVE_TLS is not
defined, treat "https" as unsupported: call flb_error with a descriptive message
and goto error (same error path). In other words, augment the existing protocol
check to reject "https" when FLB_HAVE_TLS is not enabled so CONNECT attempts
cannot proceed in non-TLS builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b65c4757-d5fe-4e45-b7a7-d44d20e2b6b9
📒 Files selected for processing (8)
include/fluent-bit/flb_upstream.hinclude/fluent-bit/tls/flb_tls.hsrc/flb_io.csrc/flb_upstream.csrc/flb_utils.csrc/tls/flb_tls.csrc/tls/openssl.ctests/internal/utils.c
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/flb_utils.c (1)
1833-1837:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale proxy parser comment to match new behavior.
The function header still says “currently only HTTP is supported,” but Line 1872+ now supports HTTPS when TLS is enabled. Please update this comment to avoid misleading future maintainers.
✏️ Suggested doc-comment fix
- * Note: currently only HTTP is supported. + * Note: HTTP is always supported; HTTPS is supported when built with TLS.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/flb_utils.c` around lines 1833 - 1837, The doc-comment for flb_utils_proxy_url_split is outdated stating “currently only HTTP is supported”; update the header comment to reflect that the parser now accepts HTTPS when TLS is enabled and clarify supported URL forms (e.g. `http://[user:pass@]host:port` and `https://[user:pass@]host:port` with TLS conditional support). Edit the comment above flb_utils_proxy_url_split to remove the HTTP-only note and add a brief line about HTTPS support when TLS is enabled and any relevant limitations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/flb_utils.c`:
- Around line 1833-1837: The doc-comment for flb_utils_proxy_url_split is
outdated stating “currently only HTTP is supported”; update the header comment
to reflect that the parser now accepts HTTPS when TLS is enabled and clarify
supported URL forms (e.g. `http://[user:pass@]host:port` and
`https://[user:pass@]host:port` with TLS conditional support). Edit the comment
above flb_utils_proxy_url_split to remove the HTTP-only note and add a brief
line about HTTPS support when TLS is enabled and any relevant limitations.
3 tasks
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/flb_upstream.c (1)
379-382:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
flb_free(u)here leaksproxy_tls_context(and pre-existing siblings) on the OOM path.If the
flb_strdup(proxy_host)at line 329 fails (OOM), control still falls through the new block at lines 339-366 and may allocateu->proxy_tls_context, plus the already-existingu->proxied_host,u->proxy_username,u->proxy_password. Then this branch onlyflb_free(u)s the struct, leaking all of them. Useflb_upstream_destroy(u)to centralize cleanup; it now also handlesproxy_tls_context(lines 720-725).🛡️ Proposed fix
if (!u->tcp_host) { - flb_free(u); + flb_upstream_destroy(u); return NULL; }Note:
flb_upstream_destroywalks the queues (initialized to empty here) and the TLS cleanup is NULL-guarded, so calling it on a partially-initialized upstream is safe. Verify the upstream isn't already linked intoconfig->upstreamsat this point (it isn't —mk_list_addhappens at line 389).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/flb_upstream.c` around lines 379 - 382, The free-on-OOM path currently calls flb_free(u) which leaks allocated fields like u->proxy_tls_context, u->proxied_host, u->proxy_username and u->proxy_password after a failed flb_strdup(proxy_host); replace the raw flb_free(u) call with flb_upstream_destroy(u) so the centralized destructor frees all subsidiary allocations (including proxy_tls_context), noting it is safe here because the upstream has not yet been linked via mk_list_add; ensure the conditional that checks !u->tcp_host uses flb_upstream_destroy(u) instead of flb_free(u).
🧹 Nitpick comments (1)
src/flb_upstream.c (1)
339-366: 🏗️ Heavy liftProxy TLS context has no user-configurable options for CA, client certs, or verification.
flb_tls_create()is called withNULLforca_path,ca_file,crt_file,key_file, andkey_passwd, andverify/verify_hostnameare hard-coded toFLB_TRUE. The proxy TLS context is intentionally separate from the destination TLS context (per the code comment), so destinationtls.*settings do not apply to the proxy leg either.Practical impact:
- Users behind a corporate HTTPS proxy with a private CA cannot point to a custom CA bundle for the proxy handshake; the connection will fail when OpenSSL falls back to the system default trust store.
- Mutual-TLS proxies are not supported (no client cert/key plumbing).
- No escape hatch exists to disable verification for testing or override CA validation.
Web search confirms Fluent Bit currently has no
proxy_tls.*ornet.proxy_tls.*configuration options to expose these settings.Since the PR is already marked
docs-required, at minimum document this limitation. For broader usability, consider either propagating the destinationtlscontext's CA/verify settings as sensible defaults, or introducing dedicatedproxy_tls.*config entries to allow per-proxy TLS customization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/flb_upstream.c` around lines 339 - 366, The proxy TLS context is created with flb_tls_create(...) using NULL for ca_path, ca_file, crt_file, key_file, key_passwd and hard-coded verification via flb_tls_set_verify_hostname(...), preventing custom CA, client certs, or verification control for HTTPS proxies; update the upstream/proxy config parsing to accept proxy-specific TLS options (e.g., proxy_ca_path, proxy_ca_file, proxy_crt_file, proxy_key_file, proxy_key_passwd, proxy_verify/verify_hostname) or, if not introducing new options, propagate the destination tls.* settings into the proxy leg, then pass those variables into flb_tls_create when creating u->proxy_tls_context and set verification with flb_tls_set_verify_hostname using the configured flag (and ensure proper freeing/error handling for the new strings like proxy_host, proxy_port, proxy_username, proxy_password on failure), and add docs noting the new options or the propagated behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/flb_upstream.c`:
- Around line 379-382: The free-on-OOM path currently calls flb_free(u) which
leaks allocated fields like u->proxy_tls_context, u->proxied_host,
u->proxy_username and u->proxy_password after a failed flb_strdup(proxy_host);
replace the raw flb_free(u) call with flb_upstream_destroy(u) so the centralized
destructor frees all subsidiary allocations (including proxy_tls_context),
noting it is safe here because the upstream has not yet been linked via
mk_list_add; ensure the conditional that checks !u->tcp_host uses
flb_upstream_destroy(u) instead of flb_free(u).
---
Nitpick comments:
In `@src/flb_upstream.c`:
- Around line 339-366: The proxy TLS context is created with flb_tls_create(...)
using NULL for ca_path, ca_file, crt_file, key_file, key_passwd and hard-coded
verification via flb_tls_set_verify_hostname(...), preventing custom CA, client
certs, or verification control for HTTPS proxies; update the upstream/proxy
config parsing to accept proxy-specific TLS options (e.g., proxy_ca_path,
proxy_ca_file, proxy_crt_file, proxy_key_file, proxy_key_passwd,
proxy_verify/verify_hostname) or, if not introducing new options, propagate the
destination tls.* settings into the proxy leg, then pass those variables into
flb_tls_create when creating u->proxy_tls_context and set verification with
flb_tls_set_verify_hostname using the configured flag (and ensure proper
freeing/error handling for the new strings like proxy_host, proxy_port,
proxy_username, proxy_password on failure), and add docs noting the new options
or the propagated behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9773efe6-4295-467e-80b9-538e5e705d9a
📒 Files selected for processing (2)
src/flb_io.csrc/flb_upstream.c
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/flb_upstream.c (1)
318-330: ⚡ Quick winUse centralized teardown in the proxy URL split failure branch as well.
At Line [329], this path still calls
flb_free(u)directly. It should follow the same centralized cleanup path (flb_upstream_destroy(u)) and free any partially filledproxy_*outputs first.Proposed patch
ret = flb_utils_proxy_url_split(config->http_proxy, &proxy_protocol, &proxy_username, &proxy_password, &proxy_host, &proxy_port); if (ret == -1) { - flb_free(u); + flb_free(proxy_protocol); + flb_free(proxy_host); + flb_free(proxy_port); + flb_free(proxy_username); + flb_free(proxy_password); + flb_upstream_destroy(u); return NULL; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/flb_upstream.c` around lines 318 - 330, The error path after flb_utils_proxy_url_split() failure currently calls flb_free(u) directly; change it to perform centralized teardown by freeing any partially populated proxy_* outputs (proxy_protocol, proxy_username, proxy_password, proxy_host, proxy_port) as appropriate and then call flb_upstream_destroy(u) before returning NULL. Locate the branch where flb_utils_proxy_url_split() returns -1 (within the flb_upstream_needs_proxy() block) and replace the flb_free(u) call with code that cleans up the proxy_* variables and invokes flb_upstream_destroy(u) to ensure consistent centralized cleanup started by flb_upstream_queue_init().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/flb_upstream.c`:
- Around line 318-330: The error path after flb_utils_proxy_url_split() failure
currently calls flb_free(u) directly; change it to perform centralized teardown
by freeing any partially populated proxy_* outputs (proxy_protocol,
proxy_username, proxy_password, proxy_host, proxy_port) as appropriate and then
call flb_upstream_destroy(u) before returning NULL. Locate the branch where
flb_utils_proxy_url_split() returns -1 (within the flb_upstream_needs_proxy()
block) and replace the flb_free(u) call with code that cleans up the proxy_*
variables and invokes flb_upstream_destroy(u) to ensure consistent centralized
cleanup started by flb_upstream_queue_init().
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/internal/upstream_tls.c (1)
154-156: ⚡ Quick winStrengthen the HTTP-proxy test by keeping upstream transport as TLS.
This case currently uses
FLB_IO_TCP, so it doesn’t fully isolate “proxy scheme decides proxy TLS context.” UsingFLB_IO_TLShere would better catch regressions wherehttp://might incorrectly create proxy TLS when destination transport is TLS.Suggested tweak
- u = flb_upstream_create(config, "dest.example.com", 80, - FLB_IO_TCP, NULL); + u = flb_upstream_create(config, "dest.example.com", 443, + FLB_IO_TLS, NULL);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/upstream_tls.c` around lines 154 - 156, The test creates the upstream with FLB_IO_TCP which doesn't exercise TLS behavior; change the transport passed to flb_upstream_create so the upstream uses TLS by replacing FLB_IO_TCP with FLB_IO_TLS in the flb_upstream_create call that assigns to variable u (destination "dest.example.com", port 80) so the test keeps the upstream transport as TLS and validates proxy-scheme TLS handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/internal/upstream_tls.c`:
- Around line 154-156: The test creates the upstream with FLB_IO_TCP which
doesn't exercise TLS behavior; change the transport passed to
flb_upstream_create so the upstream uses TLS by replacing FLB_IO_TCP with
FLB_IO_TLS in the flb_upstream_create call that assigns to variable u
(destination "dest.example.com", port 80) so the test keeps the upstream
transport as TLS and validates proxy-scheme TLS handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df0213b8-27f5-4429-836e-a5fac21761a8
📒 Files selected for processing (2)
tests/internal/upstream_tls.ctests/internal/utils.c
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/internal/utils.c
This was referenced May 6, 2026
Accept https:// in flb_utils_proxy_url_split alongside http://.
Default port for https:// proxies is 443. Error messages and the
default-port strdup calls are guarded behind FLB_HAVE_TLS so that
non-TLS builds retain the original behaviour and error message
('only HTTP proxy is supported.').
Signed-off-by: Antônio Franco <13881523+antoniomrfranco@users.noreply.github.com>
When the configured proxy URL uses https://, create a dedicated flb_tls context (proxy_tls_context) for the proxy TLS leg, stored in struct flb_upstream and freed in flb_upstream_destroy. The context uses the proxy hostname as its SNI vhost, and hostname verification is explicitly enabled via flb_tls_set_verify_hostname so the proxy certificate is fully validated. Also fixes two error-path bugs in flb_upstream_create: - flb_upstream_queue_init is now called immediately after flb_stream_setup so all subsequent error paths can safely call flb_upstream_destroy for centralised cleanup. - The unzip>tcp_host OOM check now calls flb_upstream_destroy instead of bare flb_free, preventing leaks of proxied_host, proxy_username, proxy_password, and proxy_tls_context. Signed-off-by: Antônio Franco <13881523+antoniomrfranco@users.noreply.github.com>
In flb_io_net_connect, when the upstream has a proxy_tls_context, perform a TLS handshake with the proxy before sending the HTTP CONNECT request. After a successful proxy TLS handshake, enable FLB_IO_TLS on the stream so that flb_io_net_write/read route all subsequent I/O (the CONNECT request and post-CONNECT data) through the proxy TLS session. Without this flag, plain-HTTP destinations would fall through to an unhandled path because their stream flags do not include FLB_IO_TLS. For HTTPS destinations the flag is already set, making this a no-op. Signed-off-by: Antônio Franco <13881523+antoniomrfranco@users.noreply.github.com>
Two related changes: SNI priority fix: when the TLS context carries an explicit vhost (e.g. the proxy hostname on a proxy TLS context), that vhost now takes priority over proxied_host in flb_tls_session_create. Previously proxied_host was used unconditionally for upstream connections, causing the proxy TLS handshake to advertise the destination hostname instead of the proxy hostname. TLS-in-TLS chaining: add session_set_outer to struct flb_tls_backend and implement tls_session_set_outer in the OpenSSL backend using BIO_f_ssl. When flb_tls_session_create detects an existing tls_session on the connection (the proxy TLS session), it chains the new inner session's I/O through the outer session via SSL_set_bio, so that the destination TLS handshake travels inside the already-established proxy TLS tunnel rather than going directly to the raw socket. Signed-off-by: Antônio Franco <13881523+antoniomrfranco@users.noreply.github.com>
Extend proxy_url_checks in utils.c with HTTPS proxy cases (explicit port, default port 443, credentials) and rejection cases for unsupported schemes (ftp://, socks5://). Add two tests to upstream_tls.c: one verifies that flb_upstream_create with an https:// proxy sets a non-NULL proxy_tls_context with verify_hostname enabled; another verifies that a plain http:// proxy leaves proxy_tls_context NULL. Signed-off-by: Antônio Franco <13881523+antoniomrfranco@users.noreply.github.com>
antoniomrfranco
force-pushed
the
https-proxy-support
branch
from
b89e9a7 to
5ce5a2a
Compare
Author
|
Hi @edsiper, @cosmo0920, Would this be something you'd consider for a future release? Happy to adjust scope, split further, or address any concerns before this is ready for a deeper review. Thanks! |
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
1 participant
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.
Add support for HTTPS proxy URLs (
https://) in Fluent Bit's proxy handling. Previously only plainhttp://proxy URLs were accepted.This change allows outbound connections to be routed through a TLS-secured proxy, including to TLS-enabled endpoints such as S3.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
Click to toggle debug output
Click to toggle Valgrind output
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Bug Fixes
Tests