out_syslog: tls: io: Handle dtls protocol on out syslog#11728
out_syslog: tls: io: Handle dtls protocol on out syslog#11728
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:
📝 WalkthroughWalkthroughAdded DTLS-over-UDP transport support across IO, upstream, TLS, and out_syslog: new I/O flags and TLS datagram modes, transport-aware connection/handshake logic, syslog plugin DTLS handling and config/validation, and integration tests for UDP/TCP/DTLS flows. Several related code paths were adjusted for UDP/DTLS. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Fluent Bit Plugin (out_syslog)
participant Up as Upstream Manager
participant IO as flb_io / Socket Layer
participant TLS as OpenSSL TLS/DTLS
participant Receiver as Syslog Server
App->>Up: create upstream with flags (FLB_IO_UDP / FLB_IO_DTLS / FLB_IO_TLS / FLB_IO_TCP)
Up->>IO: flb_stream_setup(transport derived from flags)
App->>IO: connect() (UDP/TCP path selected)
alt transport == UDP/DTLS
IO->>IO: use UDP socket (non-blocking / recv timeout)
else
IO->>IO: use TCP connect (+proxy if TCP)
end
alt TLS or DTLS requested
IO->>TLS: establish TLS/DTLS session (client mode datagram/tcp as selected)
TLS-->>IO: handshake complete
end
IO->>Receiver: send datagram/stream (DTLS wraps datagram if used)
Receiver-->>App: response/ACK (captured by test receiver)
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 docstrings
🧪 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 |
cosmo0920
force-pushed
the
cosmo0920-handle-dtls-protocol-on-out_syslog
branch
from
3e51274 to
4c265e7
Compare
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-handle-dtls-protocol-on-out_syslog
branch
from
4c265e7 to
719fef2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
plugins/out_syslog/syslog.c (1)
825-836:⚠️ Potential issue | 🟠 MajorDTLS messages likely shouldn't have a trailing newline appended.
syslog_format()appends"\n"for any mode other than UDP. DTLS, like UDP, is a datagram transport (RFC 6012/5426): each syslog message is one datagram, and no octet-framing or trailing LF is expected by receivers. Appending\nto DTLS datagrams adds a spurious byte to every message and may confuse strict parsers.Consider treating DTLS the same as UDP for framing:
🛡️ Proposed fix
- if (ctx->parsed_mode != FLB_SYSLOG_UDP) { + if (ctx->parsed_mode != FLB_SYSLOG_UDP && + ctx->parsed_mode != FLB_SYSLOG_DTLS) { tmp = flb_sds_cat(*s, "\n", 1); if (!tmp) { ret_sds = NULL; goto clean; } *s = tmp; }RFC 6012 DTLS syslog message framing trailing newline🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_syslog/syslog.c` around lines 825 - 836, The code currently appends a trailing "\n" in syslog_format() for any mode other than UDP, but DTLS is a datagram transport too and should not get a trailing LF; update the conditional that decides to append the newline to also skip DTLS by checking ctx->parsed_mode against FLB_SYSLOG_DTLS in addition to FLB_SYSLOG_UDP, i.e. only append the "\n" when parsed_mode is neither UDP nor DTLS, and keep the existing flb_sds_cat error handling and ret_sds/goto clean behavior unchanged.
🧹 Nitpick comments (1)
plugins/out_syslog/syslog.c (1)
980-1009: Dead-code branch:FLB_SYSLOG_UDPis unreachable here; alsoflb_output_set_contextis already called inflb_syslog_config_create.Two small cleanups on this block:
- The outer
if (ctx->parsed_mode == FLB_SYSLOG_UDP)(line 972) already handles UDP and returns or falls through into the else. Inside the else,if (ctx->parsed_mode == FLB_SYSLOG_UDP) { io_flags = FLB_IO_UDP; }(lines 998-1000) can never be true, soFLB_IO_UDPis never actually selected. Drop that branch.flb_output_set_context(ins, ctx)at line 1025 duplicates the call at the end offlb_syslog_config_create()(syslog_conf.c line 172). Not harmful but redundant — pick one location.♻️ Proposed cleanup
- if (ctx->parsed_mode == FLB_SYSLOG_UDP) { - io_flags = FLB_IO_UDP; - } - else if (ctx->parsed_mode == FLB_SYSLOG_DTLS) { + if (ctx->parsed_mode == FLB_SYSLOG_DTLS) { io_flags = FLB_IO_DTLS; } else if (ins->use_tls == FLB_TRUE) { io_flags = FLB_IO_TLS; } else { io_flags = FLB_IO_TCP; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_syslog/syslog.c` around lines 980 - 1009, The else block after handling UDP contains an unreachable branch that checks ctx->parsed_mode == FLB_SYSLOG_UDP and sets io_flags = FLB_IO_UDP; remove that dead branch and the associated FLB_IO_UDP case so io_flags is chosen only from FLB_IO_DTLS, FLB_IO_TLS, or FLB_IO_TCP based on ctx->parsed_mode and ins->use_tls; also remove the redundant call to flb_output_set_context(ins, ctx) here since flb_syslog_config_create() already calls flb_output_set_context, leaving a single context-setter in flb_syslog_config_create to avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugins/out_syslog/syslog.c`:
- Around line 825-836: The code currently appends a trailing "\n" in
syslog_format() for any mode other than UDP, but DTLS is a datagram transport
too and should not get a trailing LF; update the conditional that decides to
append the newline to also skip DTLS by checking ctx->parsed_mode against
FLB_SYSLOG_DTLS in addition to FLB_SYSLOG_UDP, i.e. only append the "\n" when
parsed_mode is neither UDP nor DTLS, and keep the existing flb_sds_cat error
handling and ret_sds/goto clean behavior unchanged.
---
Nitpick comments:
In `@plugins/out_syslog/syslog.c`:
- Around line 980-1009: The else block after handling UDP contains an
unreachable branch that checks ctx->parsed_mode == FLB_SYSLOG_UDP and sets
io_flags = FLB_IO_UDP; remove that dead branch and the associated FLB_IO_UDP
case so io_flags is chosen only from FLB_IO_DTLS, FLB_IO_TLS, or FLB_IO_TCP
based on ctx->parsed_mode and ins->use_tls; also remove the redundant call to
flb_output_set_context(ins, ctx) here since flb_syslog_config_create() already
calls flb_output_set_context, leaving a single context-setter in
flb_syslog_config_create to avoid duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0df3fd59-1b23-43cf-bc6c-8c7d8b08c1f7
📒 Files selected for processing (3)
plugins/out_syslog/syslog.cplugins/out_syslog/syslog_conf.csrc/flb_io.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/flb_io.c
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/integration/scenarios/out_syslog/tests/test_out_syslog_001.py (1)
232-234:⚠️ Potential issue | 🟡 MinorDTLS assertion still only verifies handshake, not the syslog payload.
ACCEPT/DONEonly prove the DTLS server accepted a connection and completed a handshake; they do not proveout_syslogactually transmitted a syslog record over that channel. A broken encoder/writer path could still pass this test. Add a payload substring check (e.g.,"hello from out_syslog") so this test has parity with the UDP/TCP assertions.🧪 Proposed fix
def _assert_dtls_payload(output): assert "ACCEPT" in output assert "DONE" in output + assert "hello from out_syslog" in outputBased on learnings, add or update tests for behavior changes, especially protocol parsing and encoder/decoder paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/scenarios/out_syslog/tests/test_out_syslog_001.py` around lines 232 - 234, The current _assert_dtls_payload function only checks DTLS handshake markers; update it to also verify that the actual syslog payload was transmitted by asserting a payload substring (for example "hello from out_syslog") is present in output. Locate the _assert_dtls_payload helper and add an assertion that the expected message text appears (matching the UDP/TCP tests' payload check) so the test validates the encoder/writer path, not just the DTLS handshake.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/scenarios/out_syslog/tests/test_out_syslog_001.py`:
- Around line 149-155: DtlsReceiver.wait_ready currently only checks for early
process exit (via self.process.poll()) and doesn't verify that openssl s_server
is actually bound/listening; update wait_ready (and if needed start()) to wait
until the server is ready by either (a) probing the DTLS port in a short loop
(attempt a non-blocking connect/send or UDP socket send/recv to the target port
with a small timeout) until it succeeds or the deadline expires, or (b)
reading/parsing self.process stderr/stdout for the openssl "ACCEPT" banner
before returning; ensure you still raise RuntimeError with output from
_read_output if the process exits early. Reference: DtlsReceiver.wait_ready,
DtlsReceiver.start, self.process, self._read_output, and the openssl s_server
"ACCEPT" message.
- Around line 161-167: wait_message currently calls self._read_output which uses
process.communicate(timeout=...) and thus blocks until the openssl s_server
process exits; because the test runs openssl with -naccept 1 and -ign_eof the
server may not terminate and the call raises TimeoutError even after
handshake/payload were received. Change wait_message to read stderr/stdout
incrementally (non-blocking or with small timeouts) from the process used by
_read_output (or refactor _read_output to support streaming reads) and scan for
expected markers (DTLS handshake/payload logs); once markers are observed, call
self.stop() to terminate the server and return the captured output instead of
waiting for process termination. Ensure you still handle
subprocess.TimeoutExpired as fallback and reference wait_message, _read_output,
stop, and subprocess.TimeoutExpired in the change.
---
Duplicate comments:
In `@tests/integration/scenarios/out_syslog/tests/test_out_syslog_001.py`:
- Around line 232-234: The current _assert_dtls_payload function only checks
DTLS handshake markers; update it to also verify that the actual syslog payload
was transmitted by asserting a payload substring (for example "hello from
out_syslog") is present in output. Locate the _assert_dtls_payload helper and
add an assertion that the expected message text appears (matching the UDP/TCP
tests' payload check) so the test validates the encoder/writer path, not just
the DTLS handshake.
🪄 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: a6f7b1d9-7913-4110-bc7b-770ba5e6fa99
📒 Files selected for processing (1)
tests/integration/scenarios/out_syslog/tests/test_out_syslog_001.py
|
@codex review |
![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: 504b4b2c40
ℹ️ 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".
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
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)
plugins/out_syslog/syslog.c (1)
863-870:⚠️ Potential issue | 🟠 MajorTreat DTLS as datagram mode when formatting syslog payloads.
FLB_SYSLOG_DTLSnow uses datagram transport, but Line 863 still appends a TCP/TLS newline for every non-UDP mode. That changes DTLS wire payloads and can exceedctx->maxsizeby one byte because truncation happens before the newline append.🐛 Proposed fix
- if (ctx->parsed_mode != FLB_SYSLOG_UDP) { + if (ctx->parsed_mode != FLB_SYSLOG_UDP && + ctx->parsed_mode != FLB_SYSLOG_DTLS) { tmp = flb_sds_cat(*s, "\n", 1); if (!tmp) { ret_sds = NULL;Also applies to: 1035-1036
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_syslog/syslog.c` around lines 863 - 870, The code unconditionally treats non-UDP modes as stream-based when appending a trailing newline to the syslog payload; update the conditional that checks ctx->parsed_mode (used where *s is appended with "\n") to treat FLB_SYSLOG_DTLS as a datagram mode (i.e., only append the newline when parsed_mode is neither FLB_SYSLOG_UDP nor FLB_SYSLOG_DTLS). Apply the same change at the other occurrence that appends a newline (the second block referencing ctx->parsed_mode and appending "\n") so DTLS payloads are not altered or over-sized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugins/out_syslog/syslog.c`:
- Around line 863-870: The code unconditionally treats non-UDP modes as
stream-based when appending a trailing newline to the syslog payload; update the
conditional that checks ctx->parsed_mode (used where *s is appended with "\n")
to treat FLB_SYSLOG_DTLS as a datagram mode (i.e., only append the newline when
parsed_mode is neither FLB_SYSLOG_UDP nor FLB_SYSLOG_DTLS). Apply the same
change at the other occurrence that appends a newline (the second block
referencing ctx->parsed_mode and appending "\n") so DTLS payloads are not
altered or over-sized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff0cf485-40f8-4709-9fd2-dff4d889c32d
📒 Files selected for processing (2)
plugins/out_syslog/syslog.csrc/flb_io.c
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18dc607fed
ℹ️ 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".
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
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/tls/openssl.c (1)
845-858:⚠️ Potential issue | 🟠 MajorMirror the
OPENSSL_NO_DTLSguard in the OpenSSL ≥ 1.1.0 branch.The legacy branch at lines 823–837 correctly guards DTLS method calls with
#ifndef OPENSSL_NO_DTLSand fails gracefully with an error message when unsupported. The#elsebranch for OpenSSL ≥ 1.1.0 (lines 845–858) lacks this guard and unconditionally referencesDTLS_server_method()andDTLS_client_method(). When OpenSSL ≥ 1.1.0 is built withOPENSSL_NO_DTLS, these function symbols are declared but not implemented, causing a linker failure. Apply the same guard here for consistency and build robustness.🔧 Proposed fix
`#else` if (mode == FLB_TLS_SERVER_MODE) { ssl_ctx = SSL_CTX_new(TLS_server_method()); } - else if (mode == FLB_TLS_SERVER_MODE_DGRAM) { - ssl_ctx = SSL_CTX_new(DTLS_server_method()); - } - else if (mode == FLB_TLS_CLIENT_MODE_DGRAM) { - ssl_ctx = SSL_CTX_new(DTLS_client_method()); - } + else if (mode == FLB_TLS_SERVER_MODE_DGRAM || + mode == FLB_TLS_CLIENT_MODE_DGRAM) { +#ifndef OPENSSL_NO_DTLS + if (mode == FLB_TLS_SERVER_MODE_DGRAM) { + ssl_ctx = SSL_CTX_new(DTLS_server_method()); + } + else { + ssl_ctx = SSL_CTX_new(DTLS_client_method()); + } +#else + flb_error("[openssl] DTLS mode requested but this OpenSSL build " + "does not provide DTLS support"); + return NULL; +#endif + } else { ssl_ctx = SSL_CTX_new(TLS_client_method()); } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tls/openssl.c` around lines 845 - 858, The OpenSSL ≥1.1.0 branch unconditionally calls DTLS_server_method() and DTLS_client_method(), which can cause linker failures when built with OPENSSL_NO_DTLS; wrap the DTLS-specific branches (those comparing mode to FLB_TLS_SERVER_MODE_DGRAM and FLB_TLS_CLIENT_MODE_DGRAM) with the same `#ifndef` OPENSSL_NO_DTLS guard used in the legacy branch, and in the guarded-else provide the existing graceful fallback that logs an error and returns when DTLS is unavailable; keep TLS_server_method()/TLS_client_method() paths unchanged and ensure symbols SSL_CTX_new, DTLS_server_method, DTLS_client_method, TLS_server_method, TLS_client_method and the mode constants are referenced exactly as in the diff.
🧹 Nitpick comments (1)
src/tls/openssl.c (1)
823-843: Consider version-negotiating DTLS method for the pre-1.1.0 branch.
DTLSv1_server_method()/DTLSv1_client_method()pin the context to DTLS 1.0 only, while the >=1.1.0 branch uses the version-negotiatingDTLS_*_method(). SinceDTLS_*_method()is available starting in OpenSSL 1.0.2, you could use it here (with an additionalOPENSSL_VERSION_NUMBERcheck for 1.0.2+) to allow DTLS 1.2. This branch only targets very old OpenSSL releases, so this is optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tls/openssl.c` around lines 823 - 843, The DTLS branch currently uses DTLSv1_server_method()/DTLSv1_client_method() which pins to DTLS 1.0; change it to prefer the version‑negotiating DTLS_server_method()/DTLS_client_method() when building against OpenSSL 1.0.2+ by checking OPENSSL_VERSION_NUMBER (>= 0x10002000L) and calling DTLS_server_method()/DTLS_client_method() to create ssl_ctx for FLB_TLS_SERVER_MODE_DGRAM/FLB_TLS_CLIENT_MODE_DGRAM, and fall back to DTLSv1_server_method()/DTLSv1_client_method() when the version check fails or the symbols are unavailable; keep existing error handling (flb_error and return NULL) when OPENSSL_NO_DTLS is defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/tls/openssl.c`:
- Around line 845-858: The OpenSSL ≥1.1.0 branch unconditionally calls
DTLS_server_method() and DTLS_client_method(), which can cause linker failures
when built with OPENSSL_NO_DTLS; wrap the DTLS-specific branches (those
comparing mode to FLB_TLS_SERVER_MODE_DGRAM and FLB_TLS_CLIENT_MODE_DGRAM) with
the same `#ifndef` OPENSSL_NO_DTLS guard used in the legacy branch, and in the
guarded-else provide the existing graceful fallback that logs an error and
returns when DTLS is unavailable; keep TLS_server_method()/TLS_client_method()
paths unchanged and ensure symbols SSL_CTX_new, DTLS_server_method,
DTLS_client_method, TLS_server_method, TLS_client_method and the mode constants
are referenced exactly as in the diff.
---
Nitpick comments:
In `@src/tls/openssl.c`:
- Around line 823-843: The DTLS branch currently uses
DTLSv1_server_method()/DTLSv1_client_method() which pins to DTLS 1.0; change it
to prefer the version‑negotiating DTLS_server_method()/DTLS_client_method() when
building against OpenSSL 1.0.2+ by checking OPENSSL_VERSION_NUMBER (>=
0x10002000L) and calling DTLS_server_method()/DTLS_client_method() to create
ssl_ctx for FLB_TLS_SERVER_MODE_DGRAM/FLB_TLS_CLIENT_MODE_DGRAM, and fall back
to DTLSv1_server_method()/DTLSv1_client_method() when the version check fails or
the symbols are unavailable; keep existing error handling (flb_error and return
NULL) when OPENSSL_NO_DTLS is defined.
2 participants
And implemented dtls transport protocol in core.
RSyslog has DTLS capability to handle DTLS transportation protocol which is described in RFC 6012.
DTLS is handled over UDP and transport layer security(TLS).
Currently, when selecting UDP as a transport protocol, there's no TLS support on out_syslog plugin. So, if users wanted to use this plugin with UDP and TLS combination, there's no luck.
We should add a capability to handle this.
Related to #11703.
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:
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
Documentation
Behavior
Tests