tls: openssl: Fail verification if client certificate is not provided#11254
tls: openssl: Fail verification if client certificate is not provided#11254edsiper merged 2 commits intofluent:masterfrom
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 server-side client-certificate verification control: new input-level config flag, TLS API to set verify_client, and OpenSSL backend wiring to apply SSL_VERIFY_FAIL_IF_NO_PEER_CERT when enabled. Changes span input parsing, TLS core API, and the OpenSSL backend. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Input (flb_input)
participant TLS as flb_tls
participant Backend as OpenSSL backend
participant SSL as SSL_CTX (OpenSSL)
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: read current verify mode
Backend->>SSL: compute verify_flags (SSL_VERIFY_PEER [+ 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) | ⏱️ ~25 minutes Suggested reviewers
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 |
💡 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. |
![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.
|
@edbingo @cosmo0920 let's work on cleanup the git commit history to get this merged |
edbingo
force-pushed
the
master
branch
4 times, most recently
from
848595f to
e1e6657
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/flb_input.c`:
- Around line 655-658: The code assigns ins->tls_verify_client =
flb_utils_bool(tmp) without validating the return; instead, call
flb_utils_bool(tmp) into an int (e.g., int val = flb_utils_bool(tmp)),
flb_sds_destroy(tmp), check if val == -1 and if so emit an error (like
flb_plg_error or flb_error) indicating an invalid tls.verify_client_cert value
and return an error from the config parsing function; otherwise set
ins->tls_verify_client = val. Mirror the same validation pattern used for the
threaded property (the check around line 721) and reference prop_key_check,
flb_utils_bool, flb_sds_destroy, and ins->tls_verify_client when making the
change.
🧹 Nitpick comments (1)
src/flb_input.c (1)
1339-1348: Guardtls_verify_clientsetup on thetls_verifyflag in server mode, or document the OpenSSL dependency.This block runs whenever
tls.verify_client_cert=on, regardless oftls.verify. The OpenSSL backend does:mode = SSL_CTX_get_verify_mode(ctx->ctx); mode |= SSL_VERIFY_FAIL_IF_NO_PEER_CERT; SSL_CTX_set_verify(ctx->ctx, mode, NULL);When
tls.verify=off(sets initial mode toSSL_VERIFY_NONE), theSSL_VERIFY_FAIL_IF_NO_PEER_CERTflag has no effect in OpenSSL withoutSSL_VERIFY_PEER. The code works correctly but relies on this implicit OpenSSL behavior. Either add a guard (&& ins->tls_verify == FLB_TRUE) or explicitly document that this only applies when verify is enabled.
edbingo
force-pushed
the
master
branch
7 times, most recently
from
83bf8f0 to
4fcb274
Compare
Signed-off-by: Edward Lancaster <edward.lancaster@siemens.com>
Signed-off-by: Edward Lancaster <edward.lancaster@siemens.com>
|
@edsiper @cosmo0920 should be ready now 👍 |
4 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