Conversation
tlsRefreshServerCertInfo walked the SSL_CTX certificate cursor with SSL_CERT_SET_FIRST and SSL_CERT_SET_NEXT and assigned the results to tls_server_cert_serial and tls_server_alt_cert_serial in that order. OpenSSL keeps one certificate slot per key algorithm and orders the slots by algorithm, so ssl_cert_set_current(SSL_CERT_SET_FIRST) returns the lowest-numbered algorithm rather than the certificate configured first. SSL_PKEY_RSA is 0, so an RSA certificate always reported as the primary regardless of which config loaded it, and swapping tls-cert-file with tls-alt-cert-file produced identical INFO output. Select each certificate by the key algorithm of the file that configured it instead. The two are already required to use different key algorithms, so the algorithm identifies the slot. The single-certificate case keeps using the cursor, where there is nothing to disambiguate. Also drops the third SSL_CTX_set_current_cert block: ssl_cert_set_current rescans from slot 0 on SSL_CERT_SET_FIRST over an unchanged certificate set, so it could not fail once the first call had succeeded. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Starting each ordering in its own server drops the config backup and restore, and asserts the two orderings agree on which certificate belongs to which config. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Drop the single-certificate special case: the general path already handles it, since a NULL alt_cert_file yields NID_undef and clears the alt fields, and the primary lookup finds its own slot either way. Pull the pubkey extraction into tlsCertKeyAlgorithm() so the slot scan and the file read share it, and use it for the primary and alternate algorithm comparison in createSSLContext too, which drops two EVP_PKEY locals and the four EVP_PKEY_free calls on the two exit paths. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
madolson
force-pushed
the
fix-alt-cert-info-mapping
branch
from
September 11, 2026 17:18
f3693ef to
566c3e6
Compare
The refresh was reading the configured PEM file to learn its key algorithm, which made the INFO fields track the file on disk rather than the loaded certificate. tls-auto-reload-interval defaults to 0 and applyTLSPort calls tlsResetCertInfo() without rebuilding the context, so the two can diverge indefinitely: deleting tls-cert-file and setting tls-port reported tls_server_cert_serial:none for a certificate the server was still serving, and rotating tls-cert-file to the alternate's key algorithm pointed both fields at the alternate. Capture both algorithms in createSSLContext, where the certificate each file produced is unambiguous, and swap them in with the context. Also lands the cursor on tls-cert-file's slot rather than the algorithm-first slot. A TLS 1.2 peer picks its client certificate from the cursor, and valkey_tls_ctx doubles as the client context when tls-client-cert-file is unset, so a replica was presenting the alternate certificate. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
The background reload builds its context off the main thread and carries the key algorithms through pending_reload, but nothing exercised that. Deleting the swap at the apply site left the whole unit/tls suite green while a real reload reported the mapping inverted. Rewrite the two configured files so their key algorithms trade places and assert the mapping still follows the config after the reload. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
The auto-reload cron is only installed when TLS is built in (server.c:1805 is USE_OPENSSL == 1), so under --tls-module the wait for the reload log times out. The assertions before it hold either way, so guard only that block. Also drop the two NID_undef checks in createSSLContext. ssl_set_cert rejects a certificate whose public key does not resolve to an SSL_PKEY slot, so SSL_CTX_use_certificate_chain_file cannot return success and then leave EVP_PKEY_base_id undefined. Name the reload fixtures for their role rather than hanging all four off the EC certificate's path. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
EVP_PKEY_base_id() returns NID_undef for a provider-only algorithm, so keying the slot lookup on it reported tls_server_cert_serial:none for an ML-DSA certificate that OpenSSL 3.5 loads and serves happily. ssl_cert_lookup_by_pkey matches by name via EVP_PKEY_is_a and has a second loop over provider registered sigalgs, so the certificate resolves to a slot fine while the legacy pkey->type behind EVP_PKEY_base_id does not. PQC is the case this feature exists for, so that is the wrong key. Capture each certificate's serial and expiry in createSSLContext, where the file it came from is still known, and carry those with the context. Nothing has to identify a slot afterwards, so the lookup, the algorithm helper and the createSSLContext algorithm changes all go away. This drops the cursor repositioning. Landing the cursor on tls-cert-file's slot needs the same slot identification, and the TLS 1.2 client certificate behaviour it addressed is pre-existing rather than something this change introduces. Also splits the reload coverage into its own test so it can use the file's existing module-mode skip instead of a silent guard. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Loading the alternate certificate moves the context's current certificate to its slot, and nothing moved it back once the refresh stopped walking it. An outgoing TLS 1.2 connection picks its client certificate from there, so a replica with no tls-client-cert-file started presenting the alternate: tls-cert-file / tls-alt-cert-file 0e36225 before this commit RSA / EC Generic-cert EC-cert EC / RSA Generic-cert Generic-cert Reset it to the lowest slot at the end of createSSLContext, which is where 0e36225 left it after every refresh. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Deleting the cursor reset in createSSLContext left the whole suite green, which is how it regressed once already. Authenticate the replica by the CN of the certificate it presents and assert it is the one tls-cert-file configured. The comment on the captured certificate info claimed a failure was unreachable. isCertValid() accepts an unparseable notAfter on OpenSSL 3.x, because X509_cmp_time returns 0 on a format error and 0 is neither > 0 nor < 0, so tlsUpdateCertInfoFromCtx can fail. The stated outcome, a field left as none, is right. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
A TLS 1.3 client picks its certificate from the peer's signature algorithm preference and never reads the cursor, so tls-cert-file does not decide this at the default protocol version. The test pins TLS 1.2 and the name now says so. The primary side tls-protocols pin was redundant: the replica caps the handshake with its own. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
The capture ran before any private key loaded, so a certificate whose OpenSSL slot never receives a key still got a serial published. With tls-cert-file an ECDSA certificate and tls-key-file the RSA key, both keys land in the RSA slot and only the RSA certificate is servable, yet INFO named the ECDSA one. Load each certificate with its key and capture after the key, so the recorded certificate is the one the server can present. That config now reports none for the primary, where 0e36225 reported the alternate's certificate. Also renames tlsFreeServerCertInfo to match the tlsClear* helpers around it, and frees the captured info at tlsCreateContexts' error label alongside the two contexts instead of at both call sites. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Moving the key load ahead of the alternate certificate load made a mismatched primary pair loadable that 0e36225 refused, because ssl_set_cert unloads the key already sitting in a slot when the certificate handed to it does not match. Put the load order back and check the outcome instead: count the slots holding both a certificate and a key, and require one per configured certificate. OpenSSL routes a key to the slot for its own algorithm and only checks it against whatever certificate is already there, so a mismatched pair can leave a certificate with no key rather than failing. That also closes two shapes 0e36225 accepted: an ECDSA tls-cert-file with the RSA key alongside an RSA tls-alt-cert-file, which it reported as the alternate's serial, and the same mismatch with no alternate at all. The capture goes back to being per certificate, which is unambiguous again now that every accepted certificate is guaranteed a key. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Counting usable slots says a certificate is unpaired, not which one, so with a good tls-cert-file and tls-key-file and a mismatched alternate the old message sent the operator to the two files that were fine. Also pins each case in the test to its own log line rather than letting the pattern match an earlier one, and covers the client context, whose branch of the message had no test. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
The EVP_PKEY hoist was left over from a shared helper that no longer exists, and the alt_key_file to alt_cert_file swap was a style change to base's code with no behavior difference. Both are hunks a backport has to carry for nothing. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Rejecting a certificate with no matching private key is a new startup refusal, and it is not needed to make the reporting correct: in that configuration 0e36225 also reported the wrong certificate, and unstable already starts with a mismatched pair. It is a second bug, so it goes in its own change. The TLS 1.2 client certificate test goes with it. It covers pre-existing cursor behaviour rather than anything this change introduces. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
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
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.
tls_server_cert_serialreports whichever certificate OpenSSL gave the lower slot index, not the one intls-cert-file. Slot order is by key algorithm, so with an RSA and an ECDSA certificate configured, swapping the two config values produces identical INFO output. Anyone alerting ontls_server_cert_expires_in_secondssilently starts watching a different file the moment they add an alt certificate. This captures each certificate's serial and expiry while the context is being built, which is the last point where the file it came from is known, and carries those with the context.Based on valkey-io/valkey#3717 head
0e36225, so the base branch here ispr3717-alt-cert-base.Details
Problem
tlsRefreshServerCertInfowalks the cursor withSSL_CERT_SET_FIRSTthenSSL_CERT_SET_NEXTand assigns the results to the primary and alt fields in that order (src/tls.c:401-417). OpenSSL orders slots by algorithm, not configuration order:SSL_PKEY_RSAis0(ssl/ssl_local.h:319).Two configs, byte-identical output:
Not RSA-specific. Ed25519 (slot 7) with ECDSA (slot 3) and no RSA anywhere still reports the EC certificate as primary in both orders. Single-certificate configs report correctly, which is why this is quiet.
Fix
createSSLContextloads each certificate with its key and records the serial and expiry of whichever certificate that key activated, into atlsServerCertInfothat travels with the context through both the synchronous and background reload paths. The refresh publishes those. Nothing has to identify a slot afterwards.Alternatives considered
Read the configured PEM at refresh time to recover its key algorithm, then find the matching slot. Much less plumbing. It loses because it makes INFO follow the file on disk rather than the loaded certificate, and
tls-auto-reload-intervaldefaults to 0 whileapplyTLSPort(src/config.c:2939) callstlsResetCertInfo()without rebuilding the context, so the two diverge indefinitely. Deletingtls-cert-fileand then settingtls-portreportedtls_server_cert_serial:nonefor a certificate still being served.Record the key algorithm at load time and use that as the slot key. This is what the first version of this PR did, and it is wrong for the case the feature exists for.
EVP_PKEY_base_idgoes through the legacypkey->type, which is -1 for a provider-only key, so it returnsNID_undeffor ML-DSA.ssl_cert_lookup_by_pkeymatches by name viaEVP_PKEY_is_aand has a second loop over provider-registered sigalgs (ssl/ssl_cert.c:1325), so the certificate resolves to a slot and loads fine. Result on OpenSSL 3.6.2 with the stock default provider:Read the two files outright with
tlsUpdateCertInfoFromFile(), the waytlsRefreshCACertInfo()does for CA certificates. Loses for the same reason as the first alternative.Certificate cursor
Base reset the current certificate to the lowest slot at the end of every refresh. The refresh no longer walks the cursor, so
createSSLContextresets it instead, leaving outgoing TLS 1.2 connections picking the same client certificate they picked before. No behaviour change, and no test: it covers pre-existing behaviour, so it belongs with the fix for it rather than here.Not fixed here
A certificate whose private key does not match it still loads, because OpenSSL routes a key to the slot for its own algorithm and only checks it against whatever certificate is already there.
tls_server_cert_serialthen names a certificate the server cannot present.0e36225got that case wrong too, naming the alternate's certificate instead, andunstablealready starts with a mismatched pair and completes no handshake at all. Separate bug, separate change.Two provider-only certificates, ML-DSA-44 with ML-DSA-65, are rejected as "the same key algorithm" because
EVP_PKEY_base_idisNID_undeffor both. Pre-existing in #3717 and untouched here.Testing
The mapping test pins each serial to the file that configured it, using the single-certificate case to establish which serial is which. Asserting only that the two serials differ, or that they swap, passes while the mapping is inverted.
Fails on
0e36225with onlysrc/tls.creverted:The reload test covers the background path, which the mapping test does not reach. Deleting the swap in
tlsApplyPendingReloadleaves the mapping test green and fails only this one.ML-DSA is verified by hand, not in the suite:
utils/gen-test-certs.shno longer generates a PQC certificate as of0e36225.This was generated by AI but verified, with love, by a human.