tls: add tls.crl_file support for certificate revocation checks - #12091
tls: add tls.crl_file support for certificate revocation checks#12091egershonNvidia wants to merge 7 commits into
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 ChangesTLS CRL and ingress queue support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant InputOutput as Input/output initialization
participant TLSAPI as flb_tls_set_crl_file
participant OpenSSL as OpenSSL TLS backend
participant CertificateStore as Certificate store
InputOutput->>TLSAPI: pass tls.crl_file
TLSAPI->>OpenSSL: dispatch CRL file path
OpenSSL->>CertificateStore: load CRLs and enable checking
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ 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 |
![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: cd33dd40c4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Valgrind output for CRL tests: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/tls/flb_tls.c (1)
314-326: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winLog a warning when the backend doesn't support CRL.
When
tls->api->context_set_crl_fileis NULL (e.g., mbedtls backend),flb_tls_set_crl_filesilently returns 0. Since callers only invoke this when the user explicitly configuredtls.crl_file, a silent no-op gives a false sense of security — the user believes certificate revocation is being checked when it isn't. Consider emitting a warning so the misconfiguration is visible.♻️ Proposed refactor
int flb_tls_set_crl_file(struct flb_tls *tls, const char *crl_file) { if (!tls) { return -1; } if (tls->ctx && tls->api->context_set_crl_file) { return tls->api->context_set_crl_file(tls->ctx, crl_file); } + flb_warn("[tls] CRL file configured but TLS backend does not support CRL checking"); return 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 `@src/tls/flb_tls.c` around lines 314 - 326, Update flb_tls_set_crl_file to emit a warning when tls->api->context_set_crl_file is unavailable, while preserving the existing successful backend call and return behavior. Use the project’s established TLS logging mechanism and clearly state that CRL configuration is unsupported by the active backend before returning.src/tls/openssl.c (3)
501-502: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse block comment style, not
//.As per coding guidelines,
`**/*.{c,h,cc,cpp,hpp}`: ... Use /* ... */ comments, wrapping long comments.The//line here should be a/* ... */block comment.🖊️ Proposed fix
- // PEM_read_bio_X509_CRL leaves a benign EOF error on the stack + /* PEM_read_bio_X509_CRL leaves a benign EOF error on the stack */ ERR_clear_error();🤖 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/openssl.c` around lines 501 - 502, Replace the `//` comment immediately before `ERR_clear_error()` with a C-style `/* ... */` block comment, preserving its existing message and keeping the comment concise.Source: Coding guidelines
489-503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMid-stream CRL parse errors are silently swallowed once one entry loads.
If the file contains a valid CRL followed by corrupted/truncated data, the loop simply stops (treating the parse failure the same as clean EOF) and
ERR_clear_error()discards the error unconditionally. Sinceloaded > 0still returns success, operators get no signal that part of the file failed to parse. Consider checkingERR_GET_REASON()for the benignPEM_R_NO_START_LINE/EOF marker specifically, and warn distinctly on unexpected parse failures.🤖 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/openssl.c` around lines 489 - 503, Update the CRL-reading loop around PEM_read_bio_X509_CRL so it distinguishes clean EOF/PEM_R_NO_START_LINE from unexpected parse errors after the loop ends. Preserve unconditional error clearing only for the benign EOF case; otherwise log a distinct warning containing the CRL file and parse error, while retaining successfully loaded CRLs.
510-511: 🧹 Nitpick | 🔵 Trivial
X509_V_FLAG_CRL_CHECK_ALLrequires a CRL for every cert in the chain, not just the leaf.Per OpenSSL's documented semantics,
X509_V_FLAG_CRL_CHECKrequires a suitable CRL for the leaf, andX509_V_FLAG_CRL_CHECK_ALLextends that requirement to the entire chain — an error occurs if any certificate lacks a matching CRL. For deployments using an intermediate CA (as opposed to the single-CA test setup here), operators will need to supply CRLs covering every level of the chain, or handshakes will fail with "unable to get CRL" even for otherwise-valid, non-revoked certificates. Worth calling out explicitly in thetls.crl_filedocumentation since it's easy to hit in production with multi-tier PKI.🤖 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/openssl.c` around lines 510 - 511, Update the tls.crl_file documentation to explicitly state that X509_V_FLAG_CRL_CHECK_ALL requires a matching CRL for every certificate in the chain, including intermediate CAs, not only the leaf; note that multi-tier deployments must provide CRLs for each level or validation will fail with an unable-to-get-CRL error.
🤖 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/flb_input.c`:
- Around line 1686-1694: Update the CRL setup error check in the input
initialization path around flb_tls_set_crl_file to use ret != 0 instead of ret
== -1, while preserving the existing error log and return behavior.
In `@tests/runtime/in_tcp.c`:
- Line 630: Wrap the flb_tls_set_minmax_proto call in the same TEST_CHECK
assertion pattern used for flb_start, flb_tls_init, and flb_tls_create, so
TLSv1.2 pinning failures fail the test immediately.
- Around line 43-45: Add the three missing TLS fixture files under
tests/data/tls/—client_certificate_revoked.pem, client_private_key_revoked.pem,
and crl.pem—matching the filenames referenced by TLS_CLIENT_CERT_REVOKED_FILE,
TLS_CLIENT_KEY_REVOKED_FILE, and TLS_CRL_FILE in tests/runtime/in_tcp.c, so the
TLS/CRL test path can execute.
---
Nitpick comments:
In `@src/tls/flb_tls.c`:
- Around line 314-326: Update flb_tls_set_crl_file to emit a warning when
tls->api->context_set_crl_file is unavailable, while preserving the existing
successful backend call and return behavior. Use the project’s established TLS
logging mechanism and clearly state that CRL configuration is unsupported by the
active backend before returning.
In `@src/tls/openssl.c`:
- Around line 501-502: Replace the `//` comment immediately before
`ERR_clear_error()` with a C-style `/* ... */` block comment, preserving its
existing message and keeping the comment concise.
- Around line 489-503: Update the CRL-reading loop around PEM_read_bio_X509_CRL
so it distinguishes clean EOF/PEM_R_NO_START_LINE from unexpected parse errors
after the loop ends. Preserve unconditional error clearing only for the benign
EOF case; otherwise log a distinct warning containing the CRL file and parse
error, while retaining successfully loaded CRLs.
- Around line 510-511: Update the tls.crl_file documentation to explicitly state
that X509_V_FLAG_CRL_CHECK_ALL requires a matching CRL for every certificate in
the chain, including intermediate CAs, not only the leaf; note that multi-tier
deployments must provide CRLs for each level or validation will fail with an
unable-to-get-CRL error.
🪄 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: ce52444b-565f-4fb9-ad99-fcb4f5183e91
⛔ Files ignored due to path filters (6)
tests/runtime/data/tls/ca_certificate.pemis excluded by!**/*.pemtests/runtime/data/tls/client_certificate.pemis excluded by!**/*.pemtests/runtime/data/tls/client_certificate_revoked.pemis excluded by!**/*.pemtests/runtime/data/tls/client_private_key.pemis excluded by!**/*.pemtests/runtime/data/tls/client_private_key_revoked.pemis excluded by!**/*.pemtests/runtime/data/tls/crl.pemis excluded by!**/*.pem
📒 Files selected for processing (9)
include/fluent-bit/flb_input.hinclude/fluent-bit/flb_output.hinclude/fluent-bit/tls/flb_tls.hsrc/flb_input.csrc/flb_output.csrc/tls/flb_tls.csrc/tls/openssl.ctests/internal/upstream_tls.ctests/runtime/in_tcp.c
|
Debug log / test output for CRL changes Internal tests: Runtime tests (mTLS + CRL on in_tcp): Revoked client is rejected during the TLS handshake (certificate revoked / certificate verify failed). Valid client is accepted and the test passes. Build: |
|
Hi maintainers, friendly follow-up on this PR. It has been open for over a week, and the required workflows are still awaiting maintainer approval. The implementation, runtime tests, Valgrind output, and companion docs PR are ready. Could someone please approve the workflows and take a look when available? Thank you. |
|
We need to follow our commit style guide line at first. Our commit linter complains as follows: |
ee1458e to
edcba39
Compare
Signed-off-by: egershon <egershon@nvidia.com>
Signed-off-by: egershon <egershon@nvidia.com>
Signed-off-by: egershon <egershon@nvidia.com>
Signed-off-by: egershon <egershon@nvidia.com>
edcba39 to
3a6b7fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/flb_tls.c`:
- Around line 599-611: Persist the CRL path in struct flb_tls and update
flb_tls_set_crl_file to store it using the existing string-storage pattern
before or alongside applying it to the active backend context. Extend the TLS
context creation/reload path in openssl.c to reapply tls->crl_file when set,
matching the existing ca/crt/key restoration behavior so CRL enforcement
survives reloads.
🪄 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 Plus
Run ID: 470c1bc2-db00-4ecc-b740-0d3ab951c807
⛔ Files ignored due to path filters (6)
tests/runtime/data/tls/ca_certificate.pemis excluded by!**/*.pemtests/runtime/data/tls/client_certificate.pemis excluded by!**/*.pemtests/runtime/data/tls/client_certificate_revoked.pemis excluded by!**/*.pemtests/runtime/data/tls/client_private_key.pemis excluded by!**/*.pemtests/runtime/data/tls/client_private_key_revoked.pemis excluded by!**/*.pemtests/runtime/data/tls/crl.pemis excluded by!**/*.pem
📒 Files selected for processing (9)
include/fluent-bit/flb_input.hinclude/fluent-bit/flb_output.hinclude/fluent-bit/tls/flb_tls.hsrc/flb_input.csrc/flb_output.csrc/tls/flb_tls.csrc/tls/openssl.ctests/internal/upstream_tls.ctests/runtime/in_tcp.c
Signed-off-by: egershon <egershon@nvidia.com>
Signed-off-by: egershon <egershon@nvidia.com>
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)
510-512: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReturn an error when CRL verification flags cannot be enabled.
tls_context_set_crl_file()callsX509_STORE_set_flags()but ignores the return value; if it returns0, setup proceeds with CRLs loaded but without CRL checking enabled. Check the call, logERR_get_error(), and return-1on failure.🤖 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/openssl.c` around lines 510 - 512, Update tls_context_set_crl_file() to check the return value of X509_STORE_set_flags(); when it returns 0, log the error from ERR_get_error() and return -1 immediately, while preserving the existing successful setup path.Source: Coding guidelines
🧹 Nitpick comments (1)
src/tls/openssl.c (1)
501-502: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a block comment here.
Replace the
//comment with/* ... */. As per coding guidelines, use/* ... */comments.🤖 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/openssl.c` around lines 501 - 502, Replace the line comment immediately preceding ERR_clear_error() with an equivalent /* ... */ block comment, preserving its explanation that PEM_read_bio_X509_CRL leaves a benign EOF error on the stack.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.
Outside diff comments:
In `@src/tls/openssl.c`:
- Around line 510-512: Update tls_context_set_crl_file() to check the return
value of X509_STORE_set_flags(); when it returns 0, log the error from
ERR_get_error() and return -1 immediately, while preserving the existing
successful setup path.
---
Nitpick comments:
In `@src/tls/openssl.c`:
- Around line 501-502: Replace the line comment immediately preceding
ERR_clear_error() with an equivalent /* ... */ block comment, preserving its
explanation that PEM_read_bio_X509_CRL leaves a benign EOF error on the stack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a2912524-81a1-4369-8117-9b1bcc5df6f3
📒 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: egershon <egershon@nvidia.com>
2 participants
Summary
tls.crl_fileTLS configuration option for input and output plugins(
X509_V_FLAG_CRL_CHECKandX509_V_FLAG_CRL_CHECK_ALL)in_tcpmTLS:valid client accepted, revoked client rejected)
Fixes #12060
Because the CRL is loaded into the TLS context certificate store, verification
applies to any connection created from that context (including upstream HTTP
client connections used by HTTP-based output plugins).
Example configuration
[INPUT] Name tcp Listen 0.0.0.0 Port 5170 Format json tls On tls.verify On tls.crt_file /path/to/server.crt tls.key_file /path/to/server.key tls.ca_file /path/to/ca.crt tls.crl_file /path/to/crl.pemTesting
Before we can approve your change; please submit the following in a comment:
Build and run tests:
cmake .. -DFLB_TESTS_INTERNAL=On -DFLB_TESTS_RUNTIME=On
make -j$(nproc)
./bin/flb-it-upstream_tls
./bin/flb-rt-in_tcp tcp_with_tls_crl_valid_client
./bin/flb-rt-in_tcp tcp_with_tls_crl_revoked_client
Documentation
Docs PR: fluent/fluent-bit-docs#2622
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
Summary
New Features
tls.crl_file(PEM-formatted), enabling CRL-based certificate revocation enforcement.payload_sizeparameter for metrics/traces/profiles.Tests