tls: openssl: Reload and watch certificates mainly on Linux#11950
Open
cosmo0920 wants to merge 4 commits into
Open
tls: openssl: Reload and watch certificates mainly on Linux#11950cosmo0920 wants to merge 4 commits into
cosmo0920 wants to merge 4 commits into
Conversation
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds runtime TLS certificate reload support. A new ChangesTLS Runtime Certificate Reload
Sequence Diagram(s)sequenceDiagram
participant Caller
participant flb_tls_session_create
participant flb_tls_reload_if_needed
participant tls_should_reload_context
participant tls_context_reload
Caller->>flb_tls_session_create: initiate TLS session
flb_tls_session_create->>flb_tls_reload_if_needed: pre-handshake reload check
flb_tls_reload_if_needed->>flb_tls_reload_if_needed: lock reload_mutex
flb_tls_reload_if_needed->>tls_should_reload_context: file metadata changed?
tls_should_reload_context-->>flb_tls_reload_if_needed: true/false
alt reload needed
flb_tls_reload_if_needed->>tls_context_reload: context_reload(tls)
tls_context_reload->>tls_context_reload: build new SSL_CTX, apply settings
tls_context_reload->>tls_context_reload: mutex-swap ctx->ctx
tls_context_reload-->>flb_tls_reload_if_needed: 0 (success) or -1 (failure)
flb_tls_reload_if_needed->>flb_tls_reload_if_needed: refresh file statuses
end
flb_tls_reload_if_needed->>flb_tls_reload_if_needed: unlock reload_mutex
flb_tls_reload_if_needed-->>flb_tls_session_create: 1/0/-1
flb_tls_session_create-->>Caller: session handle or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 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 |
![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: 09c64c5705
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/internal/upstream_tls.c (1)
253-255: ⚡ Quick winAdd a post-reload assertion to verify status refresh behavior.
After asserting reload returns
1, add one moreflb_tls_reload_if_needed(tls) == 0check immediately. This locks in the contract that file status is refreshed after reload and prevents repeated false-positive reloads without further file changes.Suggested enhancement
TEST_CHECK(append_file(dst_key, "\n") == 0); ret = flb_tls_reload_if_needed(tls); TEST_CHECK(ret == 1); + TEST_CHECK(flb_tls_reload_if_needed(tls) == 0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/upstream_tls.c` around lines 253 - 255, After the existing TEST_CHECK assertion that verifies flb_tls_reload_if_needed(tls) returns 1, add an immediate follow-up TEST_CHECK assertion that calls flb_tls_reload_if_needed(tls) again and verifies it returns 0. This confirms that the file status has been refreshed after the reload and prevents false-positive repeated reloads without further file changes.src/tls/flb_tls.c (2)
340-342: 💤 Low valueConsider checking
pthread_mutex_initreturn value.While
pthread_mutex_initrarely fails, it can returnENOMEMorEAGAINunder resource pressure. Currently, a silent failure would leave the mutex in an undefined state, potentially causing undefined behavior whenpthread_mutex_lockis called inflb_tls_reload_if_needed.🔧 Suggested defensive check
tls->ctx = backend; tls->api = &tls_openssl; - pthread_mutex_init(&tls->reload_mutex, NULL); + if (pthread_mutex_init(&tls->reload_mutex, NULL) != 0) { + flb_errno(); + tls_context_destroy(backend); + flb_free(tls); + return NULL; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tls/flb_tls.c` around lines 340 - 342, The pthread_mutex_init call initializing tls->reload_mutex does not check its return value, which could leave the mutex in an undefined state if initialization fails due to resource constraints. Add a check for the return value of pthread_mutex_init in the initialization block and handle the error appropriately, such as by logging an error and returning a failure status to prevent subsequent mutex operations like pthread_mutex_lock in flb_tls_reload_if_needed from operating on an improperly initialized mutex.
595-595: ⚡ Quick winFunction opening brace should be on the next line.
Per the project's coding guidelines, function opening braces should be on a new line. The brace is currently on the same line as the function signature.
🔧 Suggested fix
-int flb_tls_set_client_thumbprints(struct flb_tls *tls, const char *thumbprints) { +int flb_tls_set_client_thumbprints(struct flb_tls *tls, const char *thumbprints) +{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tls/flb_tls.c` at line 595, Move the opening brace to a new line in the flb_tls_set_client_thumbprints function definition to comply with the project's coding guidelines. The function signature should end on one line, and the opening brace should be placed on the following line by itself.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tls/openssl.c`:
- Around line 1362-1366: The ALPN callback is registered on new_ctx during the
tls_context_alpn_set call with new_ctx as the user data pointer, but after the
context swap operation exchanges the SSL_CTX pointers and tls_context_destroy
frees new_ctx, the callback still references the freed pointer. Fix this by
re-registering the ALPN callback after the context swap (after the operation
that exchanges SSL_CTX pointers but before tls_context_destroy is called) so
that the callback's user data pointer references the correct active context
instead of the freed one. This ensures subsequent TLS handshakes with ALPN will
dereference valid memory.
In `@tests/internal/upstream_tls.c`:
- Around line 45-56: The `copy_file` function does not distinguish between
end-of-file and actual read errors when `fread()` returns 0. After the while
loop in `copy_file` exits, add an explicit check using `ferror(in)` to detect if
a read error occurred during the file reading process. If `ferror(in)` indicates
an error condition, close both file handles and return -1 to signal failure;
otherwise proceed with closing the files and returning 0 to indicate successful
completion.
---
Nitpick comments:
In `@src/tls/flb_tls.c`:
- Around line 340-342: The pthread_mutex_init call initializing
tls->reload_mutex does not check its return value, which could leave the mutex
in an undefined state if initialization fails due to resource constraints. Add a
check for the return value of pthread_mutex_init in the initialization block and
handle the error appropriately, such as by logging an error and returning a
failure status to prevent subsequent mutex operations like pthread_mutex_lock in
flb_tls_reload_if_needed from operating on an improperly initialized mutex.
- Line 595: Move the opening brace to a new line in the
flb_tls_set_client_thumbprints function definition to comply with the project's
coding guidelines. The function signature should end on one line, and the
opening brace should be placed on the following line by itself.
In `@tests/internal/upstream_tls.c`:
- Around line 253-255: After the existing TEST_CHECK assertion that verifies
flb_tls_reload_if_needed(tls) returns 1, add an immediate follow-up TEST_CHECK
assertion that calls flb_tls_reload_if_needed(tls) again and verifies it returns
0. This confirms that the file status has been refreshed after the reload and
prevents false-positive repeated reloads without further file changes.
🪄 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: f8758233-5637-4f8c-bfb7-970f1a7a4c3c
📒 Files selected for processing (4)
include/fluent-bit/tls/flb_tls.hsrc/tls/flb_tls.csrc/tls/openssl.ctests/internal/upstream_tls.c
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-reload-and-watch-certificates
branch
from
8ce04a2 to
78f867c
Compare
Contributor
Author
|
@patrick-stephens FYI: we finally revived the capability of executing package test on our workflows! 🙌 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On the current shorter validness of certificates proposals, we need to handle reloading certificates on Linux.
This is because shorter period of certificates" validness requests us to relaunching Fluent Bit executable before reaching the expire date of the certificates.
However, there is needed to take care of the possibility to care buffered chunks before flushing/terminating Fluent Bit executable.
Instead, if we had a capability to handle reloading certificates on changes, it'll be quite useful for users who need to handle short period of certificates.
But there is a limitation of this patch because we just storing with the same conditions the previous certificates like passphrase.
Closes #10692.
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