tls: openssl: Fail verification if client certificate is not provided #11254
Conversation
WalkthroughAdd support for requiring client certificates: new config flag and API to request client verification, plumbing from input config -> flb_tls API -> TLS backend -> OpenSSL SSL_CTX to set SSL_VERIFY_FAIL_IF_NO_PEER_CERT when in server mode. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Input (flb_input)
participant TLS as flb_tls
participant Backend as OpenSSL backend
participant SSL as OpenSSL SSL_CTX
Input->>TLS: flb_tls_set_verify_client(tls, verify_client)
TLS->>TLS: tls->verify_client = verify_client
TLS->>Backend: if (tls->ctx && api->context_set_verify_client) call context_set_verify_client(ctx, verify_client)
Backend->>SSL: compute verify_flags (SSL_VERIFY_PEER | maybe SSL_VERIFY_FAIL_IF_NO_PEER_CERT)
Backend->>SSL: SSL_CTX_set_verify(ctx, verify_flags, verify_cb)
SSL-->>Backend: return status
Backend-->>TLS: return status
TLS-->>Input: return status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Signed-off-by: Edward Lancaster <[email protected]>
💡 Codex ReviewLines 804 to 808 in 0819c37  mTLS now forced for TLS inputs using default verify
When ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
This is a breaking change as it would break already running deployments. On the other hand one could also argue that this is a security issue as people might expect the client certificate to be checked but it is not, which might be surprising. Should we consider a new option for this? e.g. something like |
Yes. We need to keep backward compatibility. So, introducing new option would be great. |
Signed-off-by: Edward Lancaster <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/tls/flb_tls.c (2)
38-42: Clarify help text fortls.verify_client_certConsider making the description more explicit, e.g., that it requires a client certificate when Fluent Bit acts as a TLS server and complements
tls.verify, to avoid confusing it with server‑certificate validation on client connections.
293-306:flb_tls_set_verify_clientcleanly extends the TLS APIThe function’s null‑check, internal state update, and conditional delegation to
context_set_verify_clientmatch existing patterns (e.g., ALPN / hostname setters) and correctly propagate backend errors. You might optionally normalize the flag withtls->verify_client = !!verify_client;for stricter boolean semantics, but it’s not required given the current callers.src/flb_input.c (2)
655-658:tls.verify_client_certproperty wiring is consistent with other TLS flagsHooking
tls.verify_client_certthroughflb_utils_bool()intoins->tls_verify_clientmatches howtls.verifyandtls.verify_hostnameare handled, so parsing and behavior are consistent. Note that invalid boolean strings result in a value different fromFLB_TRUE, effectively leaving the feature disabled—same as the existing TLS options.
1340-1348: Client‑cert verification hookup is correct; consider scoping it to server modeThe call to
flb_tls_set_verify_client()whenins->tls_verify_client == FLB_TRUEcleanly propagates the setting to the TLS backend and aborts initialization on failure, which is what you want.You might optionally:
- Apply this only when
tls_session_mode == FLB_TLS_SERVER_MODE, or- Log a warning when
tls.verify_client_certis set on a client‑mode input,to make it clearer to users that the option is intended for server‑side client‑certificate enforcement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
include/fluent-bit/flb_input.h(1 hunks)include/fluent-bit/tls/flb_tls.h(3 hunks)src/flb_input.c(3 hunks)src/tls/flb_tls.c(2 hunks)src/tls/openssl.c(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tls/openssl.c
🧰 Additional context used
🧬 Code graph analysis (2)
include/fluent-bit/tls/flb_tls.h (1)
src/tls/flb_tls.c (1)
flb_tls_set_verify_client(293-306)
src/flb_input.c (4)
src/flb_config.c (1)
prop_key_check(679-688)src/flb_output.c (1)
prop_key_check(830-840)src/flb_processor.c (1)
prop_key_check(1347-1358)src/flb_utils.c (1)
flb_utils_bool(757-771)
🔇 Additional comments (3)
include/fluent-bit/flb_input.h (1)
446-452: Newtls_verify_clientfield is consistent and backward‑compatibleThe new flag sits alongside existing TLS options and is defaulted to false in
flb_input_new, so it does not alter behavior unless explicitly configured.include/fluent-bit/tls/flb_tls.h (1)
79-80: TLS backend hook and state for client verification are well‑integratedAdding
context_set_verify_client, theverify_clientfield, and the publicflb_tls_set_verify_client()prototype follows the existing TLS API patterns and keeps older backends safe by allowing the hook to remain NULL.Also applies to: 107-109, 135-137
src/flb_input.c (1)
397-409: Explicitly defaultingtls_verify_clienttoFLB_FALSEpreserves behaviorInitializing
tls_verify_clientalongside the other TLS fields keeps existing configs unchanged and makes the default (“don’t require client certs”) explicit in the constructor.
|
added a new configuration option that can be activated as follows: tls: on
tls.debug: 1
tls.verify: on
tls.verify_client_cert: on
tls.crt_file: ./certs/server.crt
tls.key_file: ./certs/server.key
tls.ca_file: ./certs/ca.crt
|
cosmo0920
left a comment
cosmo0920
left a comment
There was a problem hiding this comment.
Yes, this patch is really what we want.
3 participants
We are interested in using fluentbit with mTLS. However, currently fluentbit accepts connections from clients who do not provide a certificate at all. This change to the openssl library forces the connection to fail if certificates are not provided when fluent-bit is in server mode.
Tested with following configuration:
Output without client certificate
Output with client certificate
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
✏️ Tip: You can customize this high-level summary in your review settings.