tls: openssl: don't recycle a connection after peer close_notify

[ku524]

Summary

out_forward (and every other plugin using the TLS-enabled upstream pool) can end up reusing a TLS connection that the peer already half-closed, causing the same connection to fail identically on every reuse until Retry_Limit is exhausted and records are dropped.

Root cause: tls_net_read() / tls_net_write() in src/tls/openssl.c only set connection->net_error when SSL_get_error() returns SSL_ERROR_SYSCALL. When the peer sends a TLS close_notify while the TCP socket itself stays open — e.g. a TLS-terminating proxy (istio/envoy) in front of the real upstream doing a graceful TLS-only teardown — SSL_get_error() returns SSL_ERROR_ZERO_RETURN instead, which falls through to the generic error path without setting net_error.

flb_upstream_conn_release() only refuses to recycle a connection when net_error is set, so this half-closed connection goes right back into the keepalive pool. The next time it's handed out, the cached TLS shutdown state makes the read fail again immediately (no new network I/O), it gets recycled again, and the cycle repeats indefinitely.

Fix: set connection->net_error = ECONNRESET on SSL_ERROR_ZERO_RETURN in both tls_net_read() and tls_net_write(), mirroring the existing SSL_ERROR_SYSCALL branch. This is the same convention already used for non-syscall connection errors elsewhere in the codebase — flb_io.c's net_io_propagate_critical_error() already treats ECONNRESET as a critical, non-recyclable error on the plain-socket path. Once set, flb_upstream_conn_release() destroys the connection instead of recycling it, so the next flush opens a fresh connection instead of repeating the same failure.

Scope: only SSL_ERROR_ZERO_RETURN (a clean close_notify) is handled here. The connection is torn down via flb_tls_session_invalidate(), which calls SSL_shutdown() to complete the bidirectional close — which is valid after ZERO_RETURN. I deliberately left SSL_ERROR_SSL (fatal protocol error) out: SSL_shutdown() must not be called after a fatal error, so making those sessions non-recyclable safely would need a separate change to the teardown path. Happy to follow up on that separately if maintainers want it.

Addresses #12076

Relation to #12046

#12046 force-closes the connection on any non-FLB_OK flush result in out_forward, which would incidentally also cover this case (a failed ack read returns non-OK). Its enumerated triggers are partial-write / handshake / WOULDBLOCK though, not the TLS-read ZERO_RETURN path, and it compensates at the plugin level. This PR fixes the same class of problem one layer down, at the TLS transport (net_error gating in flb_upstream_conn_release()), so it benefits every consumer of the upstream pool, not just out_forward. I see these as complementary rather than overlapping — happy to adjust scope if maintainers see it differently.

Verification

There's no packet-level capture of the close_notify itself (TLS 1.3 encrypts alerts) — SSL_ERROR_ZERO_RETURN is, by definition, the evidence that one was received. To verify the mechanism end-to-end without relying on a real proxy, I built a minimal TLS receiver that injects a real OpenSSL-encrypted close_notify alert on an established connection via ssl.MemoryBIO while leaving the underlying TCP socket open — the same condition a TLS-terminating proxy would produce.

Config used (sender):

[INPUT]
    Name   dummy
    Tag    test
    Rate   1

[OUTPUT]
    Name                  forward
    Match                 *
    Host                  127.0.0.1
    Port                  44444
    tls                   On
    tls.verify            Off
    Require_ack_response  On
    Retry_Limit           1

Before the fix — the poisoned connection is recycled and reused on every flush, failing identically each time:

[debug] [upstream] KA connection #51 to 127.0.0.1:44444 has been assigned (recycled)
[error] [output:forward:forward.0] cannot get ack
[debug] [upstream] KA connection #51 to 127.0.0.1:44444 is now available
[debug] [upstream] KA connection #51 to 127.0.0.1:44444 has been assigned (recycled)
[error] [output:forward:forward.0] cannot get ack
[debug] [upstream] KA connection #51 to 127.0.0.1:44444 is now available
... (repeats every cycle; 47 identical failures over 60s; eventually hits retry-attempts limit and drops)

After the fix — exactly one failure (the unavoidable in-flight loss), then the connection is destroyed and a fresh one takes over:

[debug] [upstream] KA connection #51 to 127.0.0.1:44444 has been assigned (recycled)
[debug] [tls] connection closed by the remote peer (close_notify)
[error] [output:forward:forward.0] cannot get ack
[debug] [upstream] KA connection #52 to 127.0.0.1:44444 is now available   <- fresh connection, no repeat failures after this

Valgrind (Debian/Docker, since Valgrind doesn't support Apple Silicon macOS): ran the same before/after repro under valgrind --leak-check=full against a debug build (-DFLB_VALGRIND=On):

==9== HEAP SUMMARY:
==9==     in use at exit: 0 bytes in 0 blocks
==9==   total heap usage: 21,767 allocs, 21,767 frees, 16,351,429 bytes allocated
==9== All heap blocks were freed -- no leaks are possible
==9== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Also ran the existing tests/internal/upstream_tls.c unit test against the change — passes unchanged.

I did not add a dedicated C regression test: tls_net_read()/tls_net_write() are static, and the existing upstream_tls.c exercises the release/destroy path through a mock TLS backend, so a deterministic test of this branch would need real-OpenSSL close_notify-while-TCP-open injection infrastructure that isn't present in the internal test suite today. The Python receiver above is that reproduction. Glad to contribute a reusable test harness for this if maintainers would like one.

---

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:

• Example configuration file for the change
• Debug log output from testing the change
• Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

• [N/A] Run local packaging test showing all targets (including any new ones) build.
• [N/A] Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

• [N/A] Documentation required for this feature

Backporting

• Backport to latest stable release.

The reported case reproduces on 3.2.3 as well as current master — happy to open a backport PR against the current stable branch if maintainers want one.

---

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

• Bug Fixes
  • Improved handling of clean TLS shutdowns so connections are now treated as failed instead of being reused unexpectedly.
  • Added clearer logging and error reporting when the TLS session is closed normally by the remote peer.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2a7d829d-1618-427d-80ee-2794cd856de0

📥 Commits

Reviewing files that changed from the base of the PR and between a6c3cf7 and 9c45844.

📒 Files selected for processing (1)

• src/tls/openssl.c

🚧 Files skipped from review as they are similar to previous changes (1)

• src/tls/openssl.c

---

📝 Walkthrough

Walkthrough

tls_net_read and tls_net_write now explicitly handle OpenSSL's clean close_notify, marking the connection errored and returning -1.

Changes

TLS close_notify handling

Layer / File(s)	Summary
Zero-return handling in read/write paths src/tls/openssl.c	Both functions log SSL_ERROR_ZERO_RETURN, set net_error to ECONNRESET, and return -1.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

• fluent/fluent-bit 12076: Addresses clean TLS shutdown handling that could allow poisoned keepalive connections to be reused.

Suggested reviewers: edsiper, cosmo0920, fujimotos

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: preventing recycled TLS connections after a peer close_notify.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[cosmo0920]

Our commit linter complains as below:

❌ Commit a6c3cf7205 failed:
Bad squash detected: Unexpected subject-like prefix in body: ['open: before this change the same poisoned connection is recycled']


Commit prefix validation failed.
Error: Process completed with exit code 1.

Commit subject and prefix are reasonable for us but we should avoid to use topic: style on the middle part of the commit messages. Could you change your descriptions on your commit message?

[ku524]

Thanks for the quick review! Good catch — that was a line in the commit body that wrapped to start with open:, which the linter (correctly) read as a mid-message topic: prefix.

I've reworded the body so no line starts with a word: prefix (rephrased the "...leaving the TCP socket open: before this change..." sentence) and force-pushed. The code change itself is unchanged. commit-lint should be green now.

[cosmo0920]

Looks good to me.
I'm also confusing why the TLS connections can be easily disconnected and reconnected.
This patch should resolve this phenomenon.