Open
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 PKCS#11 URI handling to OpenSSL TLS key loading: when Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant TLS as tls_context_create
participant OpenSSL as OpenSSL
participant Engine as ENGINE("pkcs11")
participant Token as PKCS#11 Token
App->>TLS: tls_context_create(key_file, key_passwd)
TLS->>OpenSSL: create SSL_CTX / load certificate
alt key_file starts with "pkcs11:"
TLS->>TLS: warn if key_passwd empty
TLS->>OpenSSL: ENGINE_by_id("pkcs11")
OpenSSL->>Engine: ENGINE_init()
TLS->>Engine: ENGINE_load_private_key(pkcs11_uri, key_passwd)
Engine->>Token: access token / PIN prompt
Token-->>Engine: EVP_PKEY
Engine-->>OpenSSL: EVP_PKEY
OpenSSL->>OpenSSL: SSL_CTX_use_PrivateKey(EVP_PKEY)
OpenSSL->>Engine: ENGINE_finish() / ENGINE_free()
OpenSSL->>OpenSSL: EVP_PKEY_free()
else Standard PEM file
TLS->>OpenSSL: set password callback if provided
OpenSSL->>OpenSSL: SSL_CTX_use_PrivateKey_file(path)
end
OpenSSL->>OpenSSL: SSL_CTX_check_private_key()
OpenSSL-->>TLS: success / error
TLS-->>App: return result
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 |
![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: 979ffa13b5
ℹ️ 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".
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tls/openssl.c`:
- Around line 958-964: The key loading branch using SSL_CTX_use_PrivateKey_file
logs the error into err_buf and calls flb_error but then continues execution,
causing a later misleading SSL_CTX_check_private_key error; after the flb_error
call in the failure path of SSL_CTX_use_PrivateKey_file, immediately jump to the
existing error cleanup path (use goto error) so the function exits with the
original key load failure rather than proceeding to SSL_CTX_check_private_key.
- Around line 924-950: The PKCS#11 branch uses deprecated ENGINE APIs
(ENGINE_by_id, ENGINE_init, ENGINE_load_private_key, ENGINE_finish) and
currently logs errors without OpenSSL error context; update the error logging
calls around ENGINE_by_id, ENGINE_init, ENGINE_load_private_key and
SSL_CTX_use_PrivateKey to append the OpenSSL error string using
ERR_error_string_n(err, err_buf, sizeof err_buf) (err_buf is already in scope)
so each flb_error includes the descriptive OpenSSL error, and keep note to plan
a future migration from ENGINE APIs to the Provider API for OpenSSL 3+/4.0
compatibility.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: toinux <26522723+itzwam@users.noreply.github.com>
itzwam
commented
Mar 23, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tls/openssl.c`:
- Around line 918-955: The OPENSSL_NO_ENGINE guard currently logs an error and
does a goto but leaves the ENGINE-related identifiers (ENGINE, ENGINE_by_id,
ENGINE_init, ENGINE_load_private_key, EVP_PKEY, SSL_CTX_use_PrivateKey,
EVP_PKEY_free, ENGINE_finish, ENGINE_free) unguarded, causing compile failures;
wrap the entire ENGINE usage block (the code that declares ENGINE *e, calls
ENGINE_by_id/ENGINE_init/ENGINE_load_private_key and uses EVP_PKEY and
SSL_CTX_use_PrivateKey, then frees with EVP_PKEY_free/ENGINE_finish/ENGINE_free)
inside an `#else` ... `#endif` so that when OPENSSL_NO_ENGINE is defined only the
flb_error and goto error are compiled, and when it is not defined the existing
ENGINE code is compiled as before.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/tls/openssl.c (1)
918-958:⚠️ Potential issue | 🔴 CriticalThe
OPENSSL_NO_ENGINEguard is still incomplete.Line 919 only gates the runtime error path. Lines 928-958 still reference
ENGINEsymbols outside an#else, so a no-engine build will fail at compile time before it can ever hitgoto error.🔧 Proposed fix
if (strncmp(key_file, "pkcs11:", 7) == 0) { `#ifdef` OPENSSL_NO_ENGINE flb_error("[tls] pkcs11_key_file '%s': requires OpenSSL ENGINE support", key_file); goto error; -#endif +#else /* PKCS#11 URI detected */ if (!key_passwd || strlen(key_passwd) == 0) { flb_warn("[tls] PKCS#11 URI may fail without a PIN/password (tls_key_passwd)"); } ENGINE *e = ENGINE_by_id("pkcs11"); @@ EVP_PKEY_free(pkey); ENGINE_finish(e); ENGINE_free(e); +#endif }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tls/openssl.c` around lines 918 - 958, The code references ENGINE symbols (ENGINE, ENGINE_by_id, ENGINE_init, ENGINE_load_private_key, ENGINE_finish, ENGINE_free, EVP_PKEY, SSL_CTX_use_PrivateKey) even when OPENSSL_NO_ENGINE is defined, causing compile errors; wrap the entire PKCS#11 branch that uses ENGINE (from the ENGINE *e = ENGINE_by_id(...) through ENGINE_free(e) and related EVP_PKEY handling) inside the complementary `#ifndef` OPENSSL_NO_ENGINE (or move it into the `#else` block of the existing guard) so that when OPENSSL_NO_ENGINE is defined the compiler sees only the error/warn path and not any ENGINE symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tls/openssl.c`:
- Around line 920-921: The log currently prints key_file verbatim which can leak
sensitive PKCS#11 URI query attributes; modify the logging in src/tls/openssl.c
(the flb_error calls that reference pkcs11_key_file/key_file, including the
block around lines 943-950) to first detect when key_file is a PKCS#11 URI
(scheme "pkcs11:"), parse its query string and produce a sanitized copy that
removes or replaces sensitive query parameters (at minimum "pin-value" and
"pin-source") before passing to flb_error; for non-pkcs11 paths log the original
value. Implement the sanitizer as a small helper (e.g., sanitize_pkcs11_uri) and
use it in all places that currently log key_file so the emitted message contains
the sanitized URI instead of the raw one.
- Around line 968-971: Cache the OpenSSL error value before it is consumed: call
ERR_get_error() once into a local unsigned long (e.g., "err") and pass that
cached value to ERR_error_string_n(err, err_buf, sizeof(err_buf)-1) and to the
flb_error() formatting call instead of calling ERR_get_error() twice; update the
block around the check (where ret != 1, involving err_buf, ERR_error_string_n,
and flb_error) to reuse that single cached error value and preserve correct
error/message pairing for key_file.
---
Duplicate comments:
In `@src/tls/openssl.c`:
- Around line 918-958: The code references ENGINE symbols (ENGINE, ENGINE_by_id,
ENGINE_init, ENGINE_load_private_key, ENGINE_finish, ENGINE_free, EVP_PKEY,
SSL_CTX_use_PrivateKey) even when OPENSSL_NO_ENGINE is defined, causing compile
errors; wrap the entire PKCS#11 branch that uses ENGINE (from the ENGINE *e =
ENGINE_by_id(...) through ENGINE_free(e) and related EVP_PKEY handling) inside
the complementary `#ifndef` OPENSSL_NO_ENGINE (or move it into the `#else` block of
the existing guard) so that when OPENSSL_NO_ENGINE is defined the compiler sees
only the error/warn path and not any ENGINE symbols.
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.
implements #10506
Summary by CodeRabbit
New Features
Bug Fixes