Resolve OpenSSL 4.0 build issues - #13476
Conversation
|
Did you consider the |
|
[approve ci] |
|
@JosiahWI Thanks for the reminder. The use of |
|
I think |
|
Oh, I see. The |
There was a problem hiding this comment.
Pull request overview
This PR continues the effort to keep ATS building across OpenSSL versions (OpenSSL 1.1.1 through 4.0+/constified APIs and BoringSSL) by tightening const-correctness around X509/ASN1 access and updating code to use supported accessor functions instead of direct structure field access.
Changes:
- Replace direct
ASN1_STRING/ASN1_BIT_STRINGfield access (->data,->length,->type) with accessor APIs likeASN1_STRING_get0_data(),ASN1_STRING_length(), andASN1_STRING_type(). - Update several helper signatures and local variables to
constwhere the OpenSSL APIs now expose const data (e.g.,X509_NAME,X509_NAME_ENTRY,ASN1_STRING). - Adjust certificate subject-name manipulation in
certifierto operate on a duplicatedX509_NAMEand set it back onto the certificate.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tscore/X509HostnameValidator.cc | Uses ASN1 accessor APIs for hostname checks and adjusts CN parsing types. |
| src/iocore/net/SSLUtils.cc | Makes asn1_strdup() accept const ASN1_STRING* and propagates const correctness through subject CN extraction. |
| src/iocore/net/SSLNetVConnection.cc | Updates debug helper to accept const X509_NAME*. |
| src/iocore/net/OCSPStapling.cc | Replaces ASN1_BIT_STRING direct field access with ASN1 accessor APIs for digesting and const cleanup. |
| src/api/InkAPI.cc | Uses const-correct X509_NAME_ENTRY / ASN1_STRING pointers when extracting CN during cert update. |
| plugins/experimental/txn_box/plugin/src/ts_util.cc | Makes ssl_value_for() accept const X509_NAME* and uses a const_cast for OpenSSL 1.1.1 compatibility at one call site. |
| plugins/experimental/sslheaders/expand.cc | Makes subject/issuer names const and replaces signature byte access with ASN1 accessor APIs. |
| plugins/experimental/cert_reporting_tool/cert_reporting_tool.cc | Treats subject name as const X509_NAME*. |
| plugins/certifier/certifier.cc | Duplicates and sets the subject name before adding CN to avoid mutating internal OpenSSL-owned structures directly. |
|
I had independently worked up a fix for this (#13482) before seeing this PR — standing that down in favor of yours. Handing over what I found in case it's useful, since some of it isn't covered here yet. The diagnosis and the three I built this branch in a Fedora 45 / OpenSSL 4.0.1 container (
Specifically:
Two notes on approach, take or leave:
Diffs for all of the above are on |
OpenSSL 1.1.1 declares the first argument as non-const while newer releases declare it const, so building against different versions failed depending on which signature was in effect. Add a const_cast at each call site to keep the const-qualified variables introduced by this PR buildable against both, matching the existing precedent in OCSPStapling.cc.
X509_get_subject_name and X509_NAME_get_index_by_NID have their argument constness changed together across OpenSSL versions, so a local variable declared with auto tracks whatever type is correct for the OpenSSL version in use, without a cast. This applies only to the two purely local variables; ts_util.cc ssl_value_for keeps its const_cast since its parameter type is shared across multiple callers.
X509HostnameValidator.cc cast ASN1_STRING_get0_data return value to non-const before an ats_strndup call that only wants const char *; drop the const instead of adding it back needlessly. certifier.cc leaked the duplicated X509_NAME on the X509_NAME_add_entry_by_txt failure path, and never checked X509_NAME_dup for allocation failure.
CertBase::X509Value took hardcoded function pointer types for X509_get_subject_name, X509_get_issuer_name, X509_getm_notBefore, and X509_getm_notAfter, but those accessors change constness in different directions across OpenSSL versions, so no single hardcoded signature builds everywhere. Deduce the parameter type from the actual accessor via decltype instead. Signature::_load and _write_ip_address also read ASN1_STRING fields directly, which breaks once the struct is opaque; switch to the accessor functions used elsewhere in this codebase.
get_x509_name_string only reads through the name via X509_NAME_print_ex, so accept a const X509_NAME * to match callers that pass X509_get_subject_name/X509_get_issuer_name results directly. get_x509_signature_string read the ASN1_STRING struct fields directly, which breaks once the struct is opaque; use ASN1_STRING_get0_data/ASN1_STRING_length instead.
make_cert_and_key mutated the X509_NAME returned by X509_get_subject_name in place, which stops compiling once that accessor can return const, and was already fragile since the returned name is only a view into the certificate internal state. Duplicate it, add the CN to the duplicate, and set it back as both subject and issuer name since this is a self-signed certificate.
client_context_dump.cc, verify_cert.cc, and ssl_client_verify_test.cc all held X509_get_subject_name/X509_NAME_get_entry/ X509_NAME_ENTRY_get_data results in hardcoded non-const locals or parameters, which stops compiling once those accessors return const. Switch to auto for the local variables and const for the debug_certificate parameter, matching the read-only usage in each case.
ssl_value_for is shared by four callers, each already deducing its X509_NAME pointer type with auto from X509_get_subject_name or X509_get_issuer_name, so unlike the other three call sites fixed earlier in this series, a single hardcoded parameter type cannot track the underlying accessor across OpenSSL versions. Templating the parameter on the callers deduced type removes the cast entirely.
Bare decltype(&X509_get_subject_name) in a parameter list reads poorly at each of the four call sites; name each getter type once via using so the declarations and out-of-line definitions just say what kind of accessor they take.
certifier.cc freed the duplicated X509_NAME manually on every path; switch to a scoped_X509_NAME unique_ptr matching the file existing scoped_X509/scoped_EVP_PKEY/scoped_SSL_CTX aliases so no path can forget to free it. ts_util.cc templated ssl_value_for just to defer the parameter type to the caller; decltype(X509_get_subject_name(nullptr)) deduces the same pointer type directly without turning it into a template. Also drop the last C-style cast this series introduced in X509HostnameValidator.cc in favor of reinterpret_cast.
d31a219 to
01b9317
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
plugins/certifier/certifier.cc:411
X509_NAME_add_entry_by_txt()expects aconst unsigned char *, but this code uses a non-const cast(unsigned char *)commonName.c_str(). Avoid casting away constness here (and avoid C-style casts) to keep the code const-correct across OpenSSL variants.
if (X509_NAME_add_entry_by_txt(n.get(), "CN", MBSTRING_ASC, (unsigned char *)commonName.c_str(), -1, -1, 0) != 1) {
plugins/experimental/txn_box/plugin/src/ts_util.cc:1119
- The PR description says the OpenSSL 1.1.1 vs 3.x+
X509_NAME_get_index_by_NID()const-mismatch is handled by addingconst_cast<X509_NAME*>(...)at the affected call sites, but this change instead introduces adecltype(X509_get_subject_name(nullptr))alias and does not add any suchconst_cast(and none exist in-tree). Please update the PR description to match the implemented approach, or adjust the code to match the documented fix.
using X509_NAME_ptr = decltype(X509_get_subject_name(nullptr));
TextView
ssl_value_for(X509_NAME_ptr name, int nid)
{
if (int loc = X509_NAME_get_index_by_NID(name, nid, -1); loc >= 0) {
| unsigned char *astr; | ||
| str = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(name, i)); | ||
| // Convert to UTF-8 | ||
| astrlen = ASN1_STRING_to_UTF8(&astr, str); |
There was a problem hiding this comment.
Confirmed real, but pre-existing on master since the function's original addition in 2015, unrelated to this PR's OpenSSL 4 changes. Filed as #13483.
| const int pos = X509_NAME_get_index_by_NID(X509_get_subject_name(cert.get()), NID_commonName, -1); | ||
| const X509_NAME_ENTRY *common_name = X509_NAME_get_entry(X509_get_subject_name(cert.get()), pos); | ||
| const ASN1_STRING *common_name_asn1 = X509_NAME_ENTRY_get_data(common_name); | ||
| char *common_name_str = reinterpret_cast<char *>(const_cast<unsigned char *>(ASN1_STRING_get0_data(common_name_asn1))); | ||
| if (ASN1_STRING_length(common_name_asn1) != static_cast<int>(strlen(common_name_str))) { |
There was a problem hiding this comment.
Confirmed real, but pre-existing on master since 2024, unrelated to this PR's OpenSSL 4 changes. Filed as #13484.
|
[approve ci autest 2] |
|
I've updated the change and addressed all the comments. New code does not use const_cast at all now. I did see |
|
@jeredfloyd I added changes on top of your commits to cover the entire source tree. I'd appreciate it if you could test this change for Fedora 45. |
There was a problem hiding this comment.
This looks great to me. Will wait for @jeredfloyd to check before approving. I have one question about the certifier logic.
Summary
Continues #13440 ("Resolve OpenSSL 4.0 build issues"), fixing the remaining CI failures on Ubuntu 20.04 and FreeBSD 13.1 (OpenSSL 1.1.1) flagged in review.
X509_NAME_get_index_by_NID()takes a non-constX509_NAME *on OpenSSL 1.1.1 but aconst X509_NAME *on OpenSSL 3.0+ and post-constify OpenSSL/BoringSSL. The original PR widened severalX509_NAME-typed variables toconstto build against the newer, constified signature, which broke the older signature at three call sites:src/tscore/X509HostnameValidator.cc—validate_hostname()src/iocore/net/SSLUtils.cc—SSLMultiCertConfigLoader::load_certs_and_cross_reference_names()plugins/experimental/txn_box/plugin/src/ts_util.cc—ssl_value_for()Each call site now does
const_cast<X509_NAME *>(...), matching the existing precedent inOCSPStapling.ccfor the same kind of OpenSSL-version const mismatch.Test plan
tscore,inknet, andtxn_boxtargets locally against BoringSSL — all compile cleancmake --build build -t format— no additional changes