tls: openssl: output: Handle certificates thumbprints #11009
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Windows-specific client certificate thumbprint selection for TLS on outputs. Introduces a new output instance field, a config property (tls.windows.client_thumbprints), a public TLS API to set thumbprints, and an OpenSSL backend implementation that parses, stores, and filters certificates by thumbprints from the Windows CertStore. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as Config
participant Out as flb_output_instance
participant Core as flb_output.c
participant TLS as flb_tls
participant BE as tls_openssl (Windows)
participant Store as Windows CertStore
rect rgb(245,248,255)
note right of User: Configure tls.windows.client_thumbprints
User->>Core: set property "tls.windows.client_thumbprints"
Core->>Out: store ins.tls_win_thumbprints
end
rect rgb(245,255,245)
note over Core,TLS: TLS initialization for output
Core->>TLS: flb_tls_set_client_thumbprints(tls, thumbprints)
TLS-->>BE: backend.set_client_thumbprints(...)
BE->>BE: parse hex thumbprints -> CRYPT_HASH_BLOB[]
BE->>Store: open cert store
BE->>Store: filter/load certs by thumbprints
Store-->>BE: matching certs (if any)
BE-->>TLS: status (0/-1)
TLS-->>Core: status
alt failure
Core->>Core: abort output init
else success
Core->>Core: continue TLS setup
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
@cosmo0920 is this for #11004 or some other issue? |
cosmo0920
force-pushed
the
cosmo0920-handle-certificates-thumbprints
branch
from
aa67c41 to
0b8d32a
Compare
Signed-off-by: Hiroshi Hatake <[email protected]>
Signed-off-by: Hiroshi Hatake <[email protected]>
cosmo0920
force-pushed
the
cosmo0920-handle-certificates-thumbprints
branch
from
0b8d32a to
fe10be5
Compare
cosmo0920
requested review from
edsiper,
fujimotos and
koleini
as code owners
![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.
ℹ️ 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 👍.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
include/fluent-bit/tls/flb_tls.h (1)
100-101: Windows TLS hook and public API look correct; clarify semanticsHook and API are properly guarded and wired. Please confirm/document that “client_thumbprints” are used to add trusted certificates from the Windows CertStore (not to select a client identity cert/private key). If that’s the intent, consider a brief comment to avoid confusion with client-auth selection wording.
Also applies to: 139-140
src/tls/openssl.c (2)
956-1024: Thumbprint parsing is robust; remove unused variable and minor nitscompact_hex + hex_to_bytes handle formats well. token_ctx is unused; drop it.
Apply this diff:
-static int windows_set_allowed_thumbprints(struct tls_context *ctx, const char *thumbprints) +static int windows_set_allowed_thumbprints(struct tls_context *ctx, const char *thumbprints) { - char *token_ctx = NULL, *tok = NULL; + char *tok = NULL; size_t cap = 4, count = 0;
956-1040: Scope check: this adds trust anchors, not client-cert selectionThis path adds matching certificates to the OpenSSL X509 store (verification trust). It does not select a client certificate/private key for mutual TLS. Please confirm that this is the intended scope for this PR; if the goal is client-auth selection (per linked issue), additional work is needed to bind a client cert + key from the Windows store.
Happy to help outline the client-auth flow on Windows if that’s in scope.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
include/fluent-bit/flb_output.h(1 hunks)include/fluent-bit/tls/flb_tls.h(2 hunks)src/flb_output.c(5 hunks)src/tls/flb_tls.c(1 hunks)src/tls/openssl.c(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
include/fluent-bit/tls/flb_tls.h (1)
src/tls/flb_tls.c (1)
flb_tls_set_client_thumbprints(318-323)
src/tls/openssl.c (2)
include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)lib/cfl/src/cfl_utils.c (2)
cfl_utils_split(260-263)cfl_utils_split_free(273-285)
src/flb_output.c (4)
src/flb_sds.c (1)
flb_sds_destroy(389-399)src/flb_config.c (1)
prop_key_check(624-633)src/flb_utils.c (1)
flb_utils_set_plugin_string_property(2200-2224)src/tls/flb_tls.c (1)
flb_tls_set_client_thumbprints(318-323)
🔇 Additional comments (13)
include/fluent-bit/flb_output.h (1)
375-379: LGTM: adds Windows thumbprints field with proper guardingField name and placement align with existing Windows TLS options.
src/tls/flb_tls.c (1)
318-324: Delegation is fine; return contract differs from other settersFunction correctly delegates to backend. Note it returns -1 when the hook is unavailable, whereas other Windows setters here return 0 when unset; intentional?
src/flb_output.c (4)
101-106: Config key addition reads wellProperty name and description are clear and consistent with other Windows TLS properties.
201-203: LGTM: free thumbprints on cleanupProperly frees tls_win_thumbprints alongside other TLS fields.
785-786: LGTM: initialize field to NULLMatches pattern for other TLS Windows fields.
1019-1021: LGTM: property wiring via helperUses flb_utils_set_plugin_string_property; consistent with peers.
src/tls/openssl.c (7)
43-48: Good defensive define for CERT_FIND_SHA256_HASHEnsures compatibility with older SDKs.
67-69: LGTM: context fields for Windows thumbprintsFields are appropriate and sized; count tracked separately for safety.
169-178: LGTM: frees per-thumbprint buffers and arrayMemory ownership handled correctly on destroy.
675-679: LGTM: initialize new Windows fieldsSafe defaults for certstore name, enterprise flag, and thumbprint array.
344-399: Verify certificate context lifetime with CertFindCertificateInStore loopCurrent pattern doesn’t explicitly CertFreeCertificateContext() cert contexts returned by CertFindCertificateInStore(). Please confirm this enumeration style doesn’t leak contexts; if needed, free the previous context on each iteration and the last one after the loop.
If adjustment is required, a safer pattern is:
- Maintain a separate prev pointer, free it after each successful next find.
- Free the last one after the loop.
I can provide a concrete patch if we confirm this is needed.
1026-1039: LGTM: thread-safe setter with single-apply guardMutex usage and idempotency check look good.
1470-1473: LGTM: backend hook wiredset_client_thumbprints exported in tls_openssl for Windows.
2 participants
This change implements Windows certificate thumbprint handling in Fluent Bit’s OpenSSL backend.
It allows specifying tls.windows.client_thumbprints to load additional trusted certificates from the Windows CertStore, matching Fluentd’s behavior.
The feature supports SHA1 and SHA256 hashes, multiple comma-separated thumbprints, and integrates cleanly into the existing TLS initialization flow.
Closes #11004.
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:
This part of the logs indicates that the newly implemented feature that loads certificates with their thumbprints.
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.