Add support for secondary certificates#3717
Open
pkhartsk wants to merge 4 commits into
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 optional alternate server TLS certificate/key directives, extends the TLS context struct, loads and validates an alternate certificate/key in the server SSL context (rejecting identical signatures), updates test cert generation, and adds unit tests and config documentation for dual-certificate setups. ChangesDual Certificate (PQC and RSA) Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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. 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 |
![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
🤖 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 `@valkey.conf`:
- Around line 218-228: The new config directives tls-alt-cert-file and
tls-alt-key-file in valkey.conf violate the "no new config options" policy;
either remove these two directives or rework them into a policy-compliant design
by tying them to an explicit CPU/memory trade-off (e.g., a single existing
toggle or heuristic that selects alternate certs based on resource constraints)
and document that behavior in the same file; update any parsing/loader logic
that referenced tls-alt-cert-file / tls-alt-key-file to use the existing option
or the new heuristic-controlled behavior, and add a short comment in valkey.conf
describing why the alternate cert selection is present and which trade-off or
feature-flag controls it.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 361d1ac7-0213-4455-ab88-7c49130a7f0b
📒 Files selected for processing (6)
src/config.csrc/server.hsrc/tls.ctests/unit/tls.tclutils/gen-test-certs.shvalkey.conf
👮 Files not reviewed due to content moderation or server errors (4)
- src/config.c
- utils/gen-test-certs.sh
- src/tls.c
- tests/unit/tls.tcl
yang-z-o
added
the
run-extra-tests
yang-z-o
reviewed
May 22, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.c`:
- Around line 556-557: The reload metadata and change detection currently only
track cert_file/client_cert_file/ca_cert_file and key_file/client_key_file but
not the alternate TLS materials; update the metadata fingerprinting and stats
logic used by tlsReconfigureIfNeeded() to also include alt_cert_file and
alt_key_file (the same way cert_file and key_file are handled) so that
createSSLContext()’s use of alt_cert_file/alt_key_file is respected during
background reloads, and mirror those same fields in metadataChanged() so changes
to the alternate certificate or key will trigger reconfigure.
In `@tests/unit/tls.tcl`:
- Around line 118-229: Each dual-cert test (e.g., test blocks "TLS: basic dual
certificates support", "TLS: alt cert and key files must be provided together",
"TLS: the same certificate twice not allowed", "TLS: Two certificates of the
same type not allowed", "TLS: Dual certificates with passphrases") should wrap
the configuration changes (calls to r CONFIG SET that set
tls-cert-file/tls-key-file/tls-alt-cert-file/tls-alt-key-file and related
validation calls like valkey_client/exec) in a try/finally (or equivalent) so
the original cert/key values
(orig_server_crt/orig_server_key/orig_server_alt_crt/orig_server_alt_key) are
always restored in the finally block; locate the r CONFIG SET calls and the
existing cleanup r CONFIG SET lines and move the cleanup into the finally to
guarantee restoration even if assertions (assert_equal/assert_match) or catches
fail.
- Around line 124-137: The openssl call uses the leaf cert ($valkey_crt) instead
of the CA cert (use the earlier ca_file) and ignores the catch return code, so
change the exec invocation in the s_client test to pass -CAfile $ca_file (not
$valkey_crt) and capture the catch return value (e.g. catch {exec ...} out rc);
then assert rc is 0 (test succeeded) before asserting the expected output
contains "Peer signature type: rsa_pss_rsae_sha256". Update the exec call and
assertions around the openssl s_client invocation in the TLS test (the block
that builds $valkey_crt/$valkey_pqc_crt and runs exec /usr/bin/openssl s_client)
accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50fe5814-aa26-4b2f-aa96-9e6fdb43eaca
📒 Files selected for processing (6)
src/config.csrc/server.hsrc/tls.ctests/unit/tls.tclutils/gen-test-certs.shvalkey.conf
✅ Files skipped from review due to trivial changes (1)
- valkey.conf
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/tls.c (1)
827-843:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack
tls-alt-key-filein reload metadata too.The alternate cert fingerprint is now tracked, but the alternate key still is not. Rotating only
tls-alt-key-fileat the same path leavesmetadataChanged()false, so background reload keeps serving the stale secondary key until a manual reconfigure or restart.Suggested fix
typedef struct { unsigned char cert_fingerprint[EVP_MAX_MD_SIZE]; unsigned int cert_fingerprint_len; unsigned char alt_cert_fingerprint[EVP_MAX_MD_SIZE]; unsigned int alt_cert_fingerprint_len; unsigned char client_cert_fingerprint[EVP_MAX_MD_SIZE]; unsigned int client_cert_fingerprint_len; unsigned char ca_cert_fingerprint[EVP_MAX_MD_SIZE]; unsigned int ca_cert_fingerprint_len; ino_t ca_cert_dir_inode; time_t ca_cert_dir_mtime; ino_t key_file_inode; time_t key_file_mtime; + ino_t alt_key_file_inode; + time_t alt_key_file_mtime; ino_t client_key_file_inode; time_t client_key_file_mtime; } tlsMaterialsMetadata;if (ctx_config->key_file && stat(ctx_config->key_file, &st) == 0) { metadata->key_file_inode = st.st_ino; metadata->key_file_mtime = st.st_mtime; } + if (ctx_config->alt_key_file && stat(ctx_config->alt_key_file, &st) == 0) { + metadata->alt_key_file_inode = st.st_ino; + metadata->alt_key_file_mtime = st.st_mtime; + } if (ctx_config->client_key_file && stat(ctx_config->client_key_file, &st) == 0) { metadata->client_key_file_inode = st.st_ino; metadata->client_key_file_mtime = st.st_mtime; }if (old->key_file_inode != new->key_file_inode || old->key_file_mtime != new->key_file_mtime) { return 1; } + if (old->alt_key_file_inode != new->alt_key_file_inode || old->alt_key_file_mtime != new->alt_key_file_mtime) { + return 1; + } if (old->client_key_file_inode != new->client_key_file_inode || old->client_key_file_mtime != new->client_key_file_mtime) { return 1; }Also applies to: 894-908, 933-942
🤖 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.c` around lines 827 - 843, The tlsMaterialsMetadata struct is missing tracking for the alternate key file so rotating tls-alt-key-file doesn't trigger metadataChanged(); add fields analogous to key_file_inode and key_file_mtime for the alternate key (e.g., alt_key_file_inode of type ino_t and alt_key_file_mtime of type time_t) to tlsMaterialsMetadata and update all places that initialize, copy, and compare metadata (functions/uses around tlsMaterialsMetadata, metadataChanged(), and related load/compare code that currently handle alt_cert_fingerprint and key_file_*/client_key_file_*/ca_cert_dir_*) to populate and check the new alt_key fields so changes to tls-alt-key-file cause metadataChanged() to return true.tests/unit/tls.tcl (1)
137-138:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the OpenSSL validation: use CA cert, check exit code, and avoid hard-coded path.
Three issues with the
openssl s_clientinvocation:
- Line 137 uses
-CAfile $valkey_crt(the server leaf certificate) instead of the CA certificate, which will fail verification.- The
catchcommand does not capture the return code, so the test cannot assert that the connection succeeded.- The hard-coded path
/usr/bin/opensslmay not exist on all systems.🔧 Proposed fix
- catch {exec /usr/bin/openssl s_client -connect localhost:$port -CAfile $valkey_crt -sigalgs "rsa_pss_pss_sha256:rsa_pss_rsae_sha256" < /dev/null} out - assert_match {*Peer signature type: rsa_pss_rsae_sha256*} $out + set rc [catch { + exec openssl s_client -connect localhost:$port -CAfile $ca_file \ + -sigalgs "rsa_pss_pss_sha256:rsa_pss_rsae_sha256" < /dev/null + } out] + assert_equal 0 $rc + assert_match {*Peer signature type: rsa_pss_rsae_sha256*} $out🤖 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/unit/tls.tcl` around lines 137 - 138, Replace the hard-coded openssl invocation and incorrect cert usage by calling "openssl" from PATH (not "/usr/bin/openssl"), pass the CA certificate variable instead of the server leaf (replace -CAfile $valkey_crt with the appropriate CA cert variable, e.g. -CAfile $valkey_ca or $ca_crt), and capture the catch result code so the test asserts the connection succeeded before checking output (use set rc [catch {exec openssl s_client -connect localhost:$port -CAfile $CA_VAR -sigalgs "rsa_pss_pss_sha256:rsa_pss_rsae_sha256" < /dev/null} out] and assert_equal 0 $rc, then assert_match the Peer signature text in $out); update references to $valkey_crt, the exec call, and the assert_match line accordingly.
🤖 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.c`:
- Around line 401-413: In tlsRefreshServerCertInfo move the null/disabled TLS
guard so you check (!valkey_tls_ctx || !(server.tls_port ||
server.tls_replication || server.tls_cluster)) before calling
SSL_CTX_set_current_cert; specifically, ensure tlsRefreshServerCertInfo tests
valkey_tls_ctx early and returns or calls tlsClearCertInfo for both cert slots
if the context is NULL or TLS is disabled, then only call
SSL_CTX_set_current_cert and tlsUpdateCertInfoFromCtx when valkey_tls_ctx is
valid; update logic around SSL_CTX_set_current_cert, tlsUpdateCertInfoFromCtx,
tlsClearCertInfo and the server.tls_server_* / server.tls_server_alt_* fields
accordingly.
In `@tests/unit/tls.tcl`:
- Line 158: The test assertion using assert_match is using a double-asterisk
pattern (**related to argument 'tls-alt-key-file'**) which is a typo; update the
pattern to use a single asterisk (*related to argument 'tls-alt-key-file'*) so
the glob-style match behaves as intended when matching $e in the tls.tcl test
(look for the assert_match call referencing $e and the 'tls-alt-key-file'
message).
- Around line 463-464: The restore step is copying from valkey_alt_crt and
valkey_alt_key which were redefined to the passphrase-protected versions; before
you redefine those two variables, save the original paths into new names (e.g.,
orig_valkey_alt_crt and orig_valkey_alt_key) and then change the restoration
file copy calls (the ones using valkey_alt_crt/valkey_alt_key around lines
463-464) to use the saved originals (orig_valkey_alt_crt and
orig_valkey_alt_key) so the temporary files are restored from the original
certificate/key paths.
---
Duplicate comments:
In `@src/tls.c`:
- Around line 827-843: The tlsMaterialsMetadata struct is missing tracking for
the alternate key file so rotating tls-alt-key-file doesn't trigger
metadataChanged(); add fields analogous to key_file_inode and key_file_mtime for
the alternate key (e.g., alt_key_file_inode of type ino_t and alt_key_file_mtime
of type time_t) to tlsMaterialsMetadata and update all places that initialize,
copy, and compare metadata (functions/uses around tlsMaterialsMetadata,
metadataChanged(), and related load/compare code that currently handle
alt_cert_fingerprint and key_file_*/client_key_file_*/ca_cert_dir_*) to populate
and check the new alt_key fields so changes to tls-alt-key-file cause
metadataChanged() to return true.
In `@tests/unit/tls.tcl`:
- Around line 137-138: Replace the hard-coded openssl invocation and incorrect
cert usage by calling "openssl" from PATH (not "/usr/bin/openssl"), pass the CA
certificate variable instead of the server leaf (replace -CAfile $valkey_crt
with the appropriate CA cert variable, e.g. -CAfile $valkey_ca or $ca_crt), and
capture the catch result code so the test asserts the connection succeeded
before checking output (use set rc [catch {exec openssl s_client -connect
localhost:$port -CAfile $CA_VAR -sigalgs
"rsa_pss_pss_sha256:rsa_pss_rsae_sha256" < /dev/null} out] and assert_equal 0
$rc, then assert_match the Peer signature text in $out); update references to
$valkey_crt, the exec call, and the assert_match line accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bd052c4-896c-491a-b3d1-f7fa79c805cb
📒 Files selected for processing (4)
src/server.csrc/server.hsrc/tls.ctests/unit/tls.tcl
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/unit/tls.tcl`:
- Around line 390-391: The test sets tls-key-file-pass via a CONFIG SET call
(tls-key-file-pass 1234) but the cleanup only restores certificate/key path
settings; update the cleanup CONFIG SET block that restores
temp_crt/temp_key/temp_alt_crt/temp_alt_key to also reset tls-key-file-pass back
to its original/default value (e.g., empty or the prior value) so the test does
not leak config across tests; locate the CONFIG SET usage in this file and add
tls-key-file-pass to the restore call that resets cert/key paths.
- Line 137: Replace the hard-coded OpenSSL binary path in the test command so
the test uses the openssl found on PATH; specifically update the exec invocation
that currently starts with "/usr/bin/openssl s_client -connect localhost:$port
-CAfile $valkey_crt -sigalgs ..." to call "openssl s_client ..." (i.e., remove
the absolute "/usr/bin" prefix) so the runtime PATH lookup is used and the test
becomes portable across environments.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9a0b082-a54e-49f5-bdc6-0cd871b66ed2
📒 Files selected for processing (4)
src/server.csrc/server.hsrc/tls.ctests/unit/tls.tcl
pkhartsk
force-pushed
the
add-dual-certs
branch
4 times, most recently
from
dc0bac0 to
f46ad5a
Compare
Contributor
Author
|
(Hmm, seems that CI run failed cause it ran the previous version of |
Contributor
Author
|
Could I ask someone to approve the CI run? It's been hanging there since friday |
Contributor
|
Hey @ranshid @lucasyonge @PingXie, could someone please approve the CI runs and take a look at this PR? I think it's in good shape overall, though it may fail the tests in some environments since it requires OpenSSL 3.x. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #3717 +/- ##
============================================
+ Coverage 76.73% 76.80% +0.07%
============================================
Files 162 162
Lines 81029 81036 +7
============================================
+ Hits 62175 62238 +63
+ Misses 18854 18798 -56
🚀 New features to boost your workflow:
|
Contributor
Author
Ok I think I fixed it for compatibility w/ OpenSSL 1.1.1, it now builds on RHEL8 for me and |
Contributor
Author
|
Bump, can someone approve the CI run? |
Contributor
Author
Two new config options, tls-alt-cert-file and tls-alt-key-file, enable using two certificates with different algorithm types at once. E.g. one can be a PQC mldsa certificate and another can be RSA for backwards compatibility with older clients that don't support PQC yet. Signed-off-by: Petr Khartskhaev <pkhartsk@redhat.com>
Also add the alternate certificate info to `tls info` This makes auto-reload work and its tests pass Signed-off-by: Petr Khartskhaev <pkhartsk@redhat.com>
… passphrases Signed-off-by: Petr Khartskhaev <pkhartsk@redhat.com>
This way the `gen-test-certs.sh` script is compatible with older versions of openssl < 3.5 Signed-off-by: Petr Khartskhaev <pkhartsk@redhat.com>
Contributor
Author
|
All the tests seem to pass now |
Contributor
Author
|
@zuiderkwast @PingXie @madolson @ranshid Can I ask for a review please? I'd like this to be merged upstream before the end of summer if possible :) |
zuiderkwast
approved these changes
Jun 30, 2026
zuiderkwast
left a comment
zuiderkwast
left a comment
Contributor
There was a problem hiding this comment.
LGTM.
It's a very strait-forward PR.
There's a spellcheck issue. We need to ignore the word or the whole shell script in the spellchecker's config file.
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
3 participants
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.
Two new config options, tls-alt-cert-file and tls-alt-key-file, enable using two certificates with different algorithm types at once. E.g. one can be a PQC mldsa certificate and another can be RSA for backwards compatibility with older clients that don't support PQC yet.
Resolves #3403