Fix build warnings with OpenSSL 4.0 by pkhartsk · Pull Request #4016 · valkey-io/valkey

pkhartsk · 2026-06-22T14:41:50Z

Some functions are newly deprecated in OpenSSL 4.0, this commits works around those. More specifically:

X509_cmp_current_time() in isCertValid() is no longer necessary as X509_check_certificate_times() compares the notBefore and notAfter on its own. Had to add a version check since this function is new in OpenSSL 4.0.
Replace deprecated X509_NAME_get_text_by_NID. Not a perfect fix, because the new implementation still assumes that the name does not contain embedded null characters which may not be true, e.g., if the name is of type UniversalString or BMPString.
Also fix constness of X509_get_subject_name return value.

The latter two fixes are taken from this Fedora patch: https://src.fedoraproject.org/rpms/valkey/c/1a9c8847172ef3fb116a1e2fdb3871692378adae?branch=rawhide

Closes #4012

coderabbitai · 2026-06-22T14:42:07Z

📝 Walkthrough

Walkthrough

Two functions in src/tls.c are updated for OpenSSL 4.0 compatibility. isCertValid gains a preprocessor-guarded path that calls X509_check_certificate_times() on OpenSSL ≥ 4.0. getCertSubjectFieldByName tightens input validation to reject zero output length and replaces the deprecated X509_NAME_get_text_by_NID() with manual NID-based subject entry lookup and bounded ASN1_STRING copying.

Changes

OpenSSL 4.0 TLS Compatibility

Layer / File(s)	Summary
Certificate validity check and subject field extraction `src/tls.c`	`isCertValid` adds a `#if OPENSSL_VERSION_NUMBER >= 0x40000000L` branch calling `X509_check_certificate_times()` with failure handling and optional error logging; the pre-4.0 path using `X509_get0_notBefore/notAfter` and `X509_cmp_current_time()` is preserved. `getCertSubjectFieldByName` rejects calls with zero `outlen`, removes the deprecated `X509_NAME_get_text_by_NID()` call, and replaces it with `X509_get_subject_name()`, `X509_NAME_get_index_by_NID()`, `X509_NAME_get_entry()`, `X509_NAME_ENTRY_get_data()`, and an explicit `memcpy` with length bounds and null termination.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The description clearly matches the changes: OpenSSL 4.0 certificate-time checks, deprecated subject-name lookup replacement, and constness fixes.
Title check	✅ Passed	The title accurately reflects the OpenSSL 4.0 compatibility work, though the changes address more than warnings alone.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

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 `@src/tls.c`:
- Around line 1322-1325: The buffer copy logic in the null-termination code path
has an unsigned integer underflow vulnerability when outlen is 0, causing
SIZE_MAX to be used as copy_len. Add a guard check at the beginning of this code
block to return early or handle the case where outlen equals 0, before the
subtraction operation in the copy_len calculation occurs. This prevents the
expression outlen - 1 from underflowing and writing past the buffer bounds.
- Around line 469-477: The error variable is declared but not initialized before
being passed to X509_check_certificate_times(). Since this function only writes
to the error parameter when it returns 0 (indicating failure or invalidity), but
the code only reads the error variable when the function returns 1 (success),
the variable is accessed uninitialized. Initialize the error variable to 0
before the X509_check_certificate_times() function call to ensure it has a
defined value when read.

🪄 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: 76719a62-45c2-42d9-a91b-55da8c9a88d2

📥 Commits

Reviewing files that changed from the base of the PR and between bee30d4 and 1f62de1.

📒 Files selected for processing (1)

src/tls.c

zuiderkwast

LGTM, thanks!

Do we run with OpenSSL 4 in any of the CI jobs or daily jobs? If not, is there an easy way to update one of them to use a distro that does, or just update OpenSSL in one of them?

pkhartsk · 2026-06-23T10:22:56Z

Do we run with OpenSSL 4 in any of the CI jobs or daily jobs?

fedora rawhide (test-fedorarawhide-tls-module) has it, so should be good :)

codecov · 2026-06-23T10:36:48Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.78%. Comparing base (436dcae) to head (b3e7ab1).
⚠️ Report is 2 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #4016      +/-   ##
============================================
+ Coverage     76.75%   76.78%   +0.03%     
============================================
  Files           162      162              
  Lines         81017    81021       +4     
============================================
+ Hits          62187    62216      +29     
+ Misses        18830    18805      -25

Files with missing lines	Coverage Δ
src/tls.c	`17.64% <ø> (ø)`

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Some functions are newly deprecated in OpenSSL 4.0, this commits works around those. More specifically: - X509_cmp_current_time() in isCertValid() is no longer necessary as X509_check_certificate_times() compares the notBefore and notAfter on its own. Had to add a version check since this function is new in OpenSSL 4.0. - Replace deprecated X509_NAME_get_text_by_NID. Not a perfect fix, because the new implementation still assumes that the name does not contain embedded null characters which may not be true, e.g., if the name is of type UniversalString or BMPString. - Also fix constness of X509_get_subject_name return value for modern OpenSSL versions The latter two fixes are taken from this Fedora patch: https://src.fedoraproject.org/rpms/valkey/c/1a9c8847172ef3fb116a1e2fdb3871692378adae?branch=rawhide Signed-off-by: Petr Khartskhaev <pkhartsk@redhat.com>

pkhartsk · 2026-06-23T11:55:24Z

Yup, the fedora test passes, where openssl 4 is installed here

zuiderkwast · 2026-06-23T15:47:35Z

Changed the title "Fix build failures" -> "warnings". It's just deprecated functions - so unless compiled with -Werror it's just warnings, right?

zuiderkwast · 2026-06-23T15:50:59Z

Let's backport to all versions. Old versions can run on new OS versions with new OpenSSL versions too, so backporting prevents future CI problems for these branches. (Assuming it applies cleanly - if not we could skip it.)

pkhartsk · 2026-06-24T11:37:48Z

it's just warnings, right?

Yes, should I rename the commit too?

Assuming it applies cleanly - if not we could skip it.

I think the first hunk will fail as that function doesn't exist in any released version as of now, the second one seems to only be present in =>9.0

sarthakaggarwal97 · 2026-06-24T23:10:04Z

+        return false;
+    }
+    if (error > 0) {
+        serverLog(LL_WARNING, "X509_check_certificate_times() returned error %d", error);


I don't think we need this warning in logs. I could be wrong but seems like a specific error for a function.

The integer pointed to by error will be set to X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD or X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD if the certificate has an invalid notBefore or notAfter field, respectively.

The integer pointed to by error will be set to X509_V_ERR_CERT_NOT_YET_VALID or X509_V_ERR_CERT_HAS_EXPIRED if the verification time is outside of the certificate's correctly encoded validity window as per RFC5280.

I'd say this is actually pretty useful to have, though I agree that just outputting a random integer is hardly helpful. Also this is stupid since this if statement will never be reached. Could be solved by either switch casing it or outputting a better-worded general error BEFORE returning false, or just removing this statement completely.

Yeah, this is not logged when the certificate is simply expired. It's only on other errors, which are likely configuration/admin errors. I agree it can be good to have.

I see you've already merged (thanks!), so I put the code fixing this into #4064. This unreachable if statement does no harm, but still.. :D

sarthakaggarwal97 · 2026-06-24T23:12:39Z


    if (nid == -1) return 0;

+#if OPENSSL_VERSION_NUMBER >= 0x30000000L


Any harm in directly using const? without the version check?

Apparently, in 1.1.1 it's not const. Not sure if we have 1.1.1 in any of our CI jobs though - it would show a compile warning I guess. See #4016 (comment) above.

Threw a warning for me on RHEL8 w/ 1.1.1.

See this almalinux8 build before I pushed this version check https://github.com/valkey-io/valkey/actions/runs/28020409485/job/82935479930

zuiderkwast · 2026-06-25T03:41:10Z

it's just warnings, right?

Yes, should I rename the commit too?

No, it's not necessary. We squash-merge and use the PR title and description as commit message.

I think the first hunk will fail as that function doesn't exist in any released version as of now, the second one seems to only be present in =>9.0

Ok, so we should backport to 9.0 and 9.1 only then. Thanks for checking!

Some functions are newly deprecated in OpenSSL 4.0. This commit works around those. More specifically: - X509_cmp_current_time() in isCertValid() is no longer necessary as X509_check_certificate_times() compares the notBefore and notAfter on its own. Had to add a version check since this function is new in OpenSSL 4.0. - Replace deprecated X509_NAME_get_text_by_NID. Not a perfect fix, because the new implementation still assumes that the name does not contain embedded null characters which may not be true, e.g., if the name is of type UniversalString or BMPString. - Also fix constness of X509_get_subject_name return value. The latter two fixes are taken from this Fedora patch: https://src.fedoraproject.org/rpms/valkey/c/1a9c8847172ef3fb116a1e2fdb3871692378adae?branch=rawhide Closes #4012 Signed-off-by: Petr Khartskhaev <pkhartsk@redhat.com>

github-actions Bot assigned pkhartsk Jun 22, 2026

This was referenced Jun 22, 2026

Valkey fails to build against OpenSSL 4 #4012

Closed

Add support for secondary certificates #3717

Open

coderabbitai Bot reviewed Jun 22, 2026

View reviewed changes

Comment thread src/tls.c

Comment thread src/tls.c

pkhartsk force-pushed the support-openssl4 branch from 1f62de1 to b3e7ab1 Compare June 22, 2026 14:56

zuiderkwast approved these changes Jun 23, 2026

View reviewed changes

pkhartsk commented Jun 23, 2026

View reviewed changes

Comment thread src/tls.c

zuiderkwast added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jun 23, 2026

pkhartsk force-pushed the support-openssl4 branch from b3e7ab1 to ee48897 Compare June 23, 2026 10:57

zuiderkwast changed the title ~~Fix build failures with OpenSSL 4.0 (#4012)~~ Fix build warnings with OpenSSL 4.0 (#4012) Jun 23, 2026

zuiderkwast added this to Valkey 9.1, Valkey 8.1, Valkey 9.0, Valkey 8.0 and Valkey 7.2 Jun 23, 2026

zuiderkwast mentioned this pull request Jun 24, 2026

Fix TLS module build with OpenSSL 4 #4043

Closed

sarthakaggarwal97 reviewed Jun 24, 2026

View reviewed changes

zuiderkwast removed this from Valkey 7.2 Jun 26, 2026

zuiderkwast removed this from Valkey 8.1 Jun 26, 2026

zuiderkwast removed this from Valkey 8.0 Jun 26, 2026

zuiderkwast changed the title ~~Fix build warnings with OpenSSL 4.0 (#4012)~~ Fix build warnings with OpenSSL 4.0 Jun 26, 2026

zuiderkwast merged commit aaa859d into valkey-io:unstable Jun 26, 2026
72 of 74 checks passed

github-project-automation Bot moved this to To be backported in Valkey 9.1 Jun 26, 2026

github-project-automation Bot moved this to To be backported in Valkey 9.0 Jun 26, 2026

valkeyrie-ops Bot mentioned this pull request Jun 27, 2026

[backport] Backport sweep for 9.1 #3987

Open

valkeyrie-ops Bot mentioned this pull request Jun 27, 2026

[backport] Backport sweep for 9.0 #3879

Open

coderabbitai Bot mentioned this pull request Jun 29, 2026

Log certificate errors in isCertValid (OpenSSL 4.0 only) #4064

Merged

sarthakaggarwal97 added the test-failure An issue indicating a test failure label Jun 29, 2026


		if (nid == -1) return 0;

		#if OPENSSL_VERSION_NUMBER >= 0x30000000L

Uh oh!

Conversation

pkhartsk commented Jun 22, 2026 • edited by zuiderkwast Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

pkhartsk commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jun 23, 2026

Codecov Report

Uh oh!

pkhartsk commented Jun 23, 2026

Uh oh!

zuiderkwast commented Jun 23, 2026

Uh oh!

zuiderkwast commented Jun 23, 2026

Uh oh!

pkhartsk commented Jun 24, 2026

Uh oh!

sarthakaggarwal97 Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkhartsk Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

pkhartsk Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

sarthakaggarwal97 Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

pkhartsk Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

zuiderkwast commented Jun 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pkhartsk commented Jun 22, 2026 •

edited by zuiderkwast

Loading

coderabbitai Bot commented Jun 22, 2026 •

edited

Loading

pkhartsk commented Jun 23, 2026 •

edited

Loading

sarthakaggarwal97 Jun 24, 2026 •

edited

Loading