Fix TLS module build with OpenSSL 4

[sarthakaggarwal97]

Failure link: https://github.com/valkey-io/valkey/actions/runs/27993975032/job/82852152994#step:8:746

Fedora Rawhide now treats the deprecated X509 APIs as build errors, so the TLS code uses the OpenSSL 4-= safe APIs where needed while keeping the existing paths for older OpenSSL versions.

The CI with run-extra-tests label will be blocked till this issue is resolved.

• test-fedorarawhide-tls-module passed
• test-fedorarawhide-tls-module-no-tls passed

[coderabbitai]

📝 Walkthrough

Walkthrough

Two functions in src/tls.c are updated with OpenSSL version guards (#if OPENSSL_VERSION_NUMBER >= 0x40000000L). isCertValid() uses X509_check_certificate_times() on OpenSSL 4.0+. getCertSubjectFieldByName() manually locates the subject entry by NID, converts to UTF-8, and validates it on OpenSSL 4.0+; both functions retain the pre-4.0 behavior via the existing API calls.

Changes

OpenSSL 4.0 API compatibility in src/tls.c

Layer / File(s)	Summary
isCertValid: OpenSSL 4.0+ time check src/tls.c	Wraps the certificate time validity check in a version guard: OpenSSL 4.0+ calls X509_check_certificate_times(); older builds retain the X509_cmp_current_time() notBefore/notAfter comparisons.
getCertSubjectFieldByName: OpenSSL 4.0+ manual entry lookup src/tls.c	Replaces the deprecated X509_NAME_get_text_by_NID() on OpenSSL 4.0+ with a manual lookup via X509_NAME_get_index_by_NID, X509_NAME_get_entry, ASN1_STRING_to_UTF8, embedded-NUL rejection, and buffer size enforcement; pre-4.0 path keeps the original call with a scoped const cast.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% 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
Title check	✅ Passed	The title 'Fix TLS module build with OpenSSL 4' directly and clearly summarizes the main change: addressing build compatibility issues with OpenSSL 4.
Description check	✅ Passed	The description is clearly related to the changeset, explaining the motivation (Fedora Rawhide's stricter handling of deprecated APIs), the solution (using OpenSSL 4-safe APIs), and verification (CI test results).
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.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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.c`:
- Around line 1313-1320: The code at lines 1313-1320 copies raw ASN1_STRING
bytes without checking for embedded NUL characters. Add a validation check after
retrieving the raw pointer and length (from ASN1_STRING_get0_data and
ASN1_STRING_length) to scan through the raw buffer and reject if any NUL byte is
found within the raw_len bytes. Return 0 immediately if an embedded NUL is
detected, before proceeding with the memcpy operation. This prevents certificate
names like "admin\0evil" from being truncated to "admin" when later used with
strlen() in authentication decisions.

🪄 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: b77d3cc3-342d-4ba8-a196-43343e44f875

📥 Commits

Reviewing files that changed from the base of the PR and between 25ada74 and a0b6352.

📒 Files selected for processing (1)

• src/tls.c

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.73%. Comparing base (ebe6215) to head (3561b45).
⚠️ Report is 1 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #4043      +/-   ##
============================================
- Coverage     76.75%   76.73%   -0.02%     
============================================
  Files           162      162              
  Lines         81021    81021              
============================================
- Hits          62185    62173      -12     
- Misses        18836    18848      +12     

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

... and 13 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.

[coderabbitai]

♻️ Duplicate comments (1)

src/tls.c (1)

1313-1320: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

OpenSSL 4.0+ path diverges from the legacy X509_NAME_get_text_by_NID semantics it replaces, affecting the auth lookup.

The legacy call (kept in the #else branch) retrieves the bytes encoded as UTF-8 from the first matching entry, and if the buffer is not large enough, or if the UTF-8 encoding would contain a NUL byte, no data is written and the call returns failure. The new branch copies ASN1_STRING_get0_data verbatim, which deviates in three ways, all of which feed ACLGetUserByName(field_value, strlen(field_value)) at Line 1401:

1. Encoding mismatch: raw bytes are not UTF-8-converted. Per OpenSSL docs, copying verbatim can be highly confusing if the target is a multicharacter string type like a BMPString or a UTF8String — a BMPString CN (UTF-16BE) yields wrong content (and interior NULs).
2. Embedded-NUL rejection still missing: despite being marked addressed previously, the current code has no memchr check, so admin\0evil truncates to admin under strlen().
3. Silent truncation vs. failure: on overflow this writes a truncated value and returns success, whereas the legacy API returns failure; a truncated CN could match a different ACL user.

Using ASN1_STRING_to_UTF8 restores legacy behavior and resolves all three.

🛡️ Proposed fix using ASN1_STRING_to_UTF8

-    const unsigned char *raw = ASN1_STRING_get0_data(data);
-    int raw_len = ASN1_STRING_length(data);
-    if (!raw || raw_len <= 0 || outlen == 0) return 0;
-
-    size_t copy_len = (size_t)raw_len < outlen ? (size_t)raw_len : outlen - 1;
-    memcpy(out, raw, copy_len);
-    out[copy_len] = '\0';
-    return copy_len > 0;
+    /* Convert to UTF-8 to match the legacy X509_NAME_get_text_by_NID() output. */
+    unsigned char *utf8 = NULL;
+    int utf8_len = ASN1_STRING_to_UTF8(&utf8, data);
+    if (utf8_len <= 0 || utf8 == NULL) {
+        OPENSSL_free(utf8);
+        return 0;
+    }
+    /* Reject embedded NUL and treat buffer overflow as failure (no truncation),
+     * mirroring the legacy API and protecting the downstream strlen()-based lookup. */
+    if (outlen == 0 || (size_t)utf8_len >= outlen ||
+        memchr(utf8, '\0', (size_t)utf8_len) != NULL) {
+        OPENSSL_free(utf8);
+        return 0;
+    }
+    memcpy(out, utf8, (size_t)utf8_len);
+    out[utf8_len] = '\0';
+    OPENSSL_free(utf8);
+    return 1;

Please confirm ASN1_STRING_to_UTF8 is available across the OpenSSL versions this branch targets (4.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.c` around lines 1313 - 1320, The code block extracting and copying
the ASN1 string data (using ASN1_STRING_get0_data followed by memcpy) does not
properly handle UTF-8 encoding conversion, embedded NUL byte validation, or
overflow semantics required by the legacy X509_NAME_get_text_by_NID API it
replaces. Replace the ASN1_STRING_get0_data and memcpy approach with
ASN1_STRING_to_UTF8, which automatically handles UTF-8 conversion for all string
types, includes embedded-NUL detection, and returns proper failure semantics on
buffer overflow. Verify that ASN1_STRING_to_UTF8 is available in all OpenSSL
versions this code targets (4.0+).

🤖 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.

Duplicate comments:
In `@src/tls.c`:
- Around line 1313-1320: The code block extracting and copying the ASN1 string
data (using ASN1_STRING_get0_data followed by memcpy) does not properly handle
UTF-8 encoding conversion, embedded NUL byte validation, or overflow semantics
required by the legacy X509_NAME_get_text_by_NID API it replaces. Replace the
ASN1_STRING_get0_data and memcpy approach with ASN1_STRING_to_UTF8, which
automatically handles UTF-8 conversion for all string types, includes
embedded-NUL detection, and returns proper failure semantics on buffer overflow.
Verify that ASN1_STRING_to_UTF8 is available in all OpenSSL versions this code
targets (4.0+).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c8309e43-f497-49cf-8ec2-a3fa5edae358

📥 Commits

Reviewing files that changed from the base of the PR and between 1abc0e3 and 3561b45.

📒 Files selected for processing (1)

• src/tls.c

[zuiderkwast]

Notice:

• Fix build warnings with OpenSSL 4.0 #4016

[sarthakaggarwal97]

Wow! Missed this. Let me close this PR