cmcfarlen commented on PR #13476: URL: https://github.com/apache/trafficserver/pull/13476#issuecomment-5172040067
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 `X509_NAME_get_index_by_NID` call sites match what I landed on exactly. The one thing worth flagging: **neither CI nor a BoringSSL build can validate the OpenSSL 4 side of this.** The Fedora CI job is Fedora 43 (OpenSSL 3.5), and per your own table in #13440 BoringSSL returns *non-const* from `X509_get_subject_name` — i.e. the same shape as 1.1.1. So a green CI run plus a BoringSSL build confirms the 1.1.1 regression is fixed, but can't exercise any of the const-*return* changes that OpenSSL 4 introduces. I built this branch in a Fedora 45 / OpenSSL 4.0.1 container (`BUILD_EXPERIMENTAL_PLUGINS=ON`, `ENABLE_CRIPTS=ON`, `BUILD_REGRESSION_TESTING=ON`, `ninja -k 0` to get past the first failure). Remaining errors: | File | Errors | | --- | --- | | `include/cripts/Certs.hpp` | 124 | | `src/cripts/Certs.cc` | 4 | | `plugins/lua/ts_lua_client_request.cc` | 4 | | `plugins/lua/ts_lua_client_cert_helpers.h` | 3 | | `src/iocore/net/unit_tests/test_SSLDHParams.cc` | 1 | Specifically: - **`plugins/lua`** — `get_x509_name_string(X509_NAME *)` needs a `const X509_NAME *` param (it only calls `X509_NAME_print_ex`, which is const on every version), and `ts_lua_client_cert_helpers.h` reads `sig->length` / `sig->data` directly, which breaks now that `ASN1_STRING` is opaque. Note this one builds by default — `plugins/lua` is gated on `if(USE_LUAJIT)`, and Fedora 45 has luajit — so it's not an opt-in path. - **`src/cripts`** — `Certs.cc` has the same opaque-`ASN1_STRING` issue in `Signature::_load` and `_write_ip_address`. `ENABLE_CRIPTS` defaults OFF so this is lower priority. - **`test_SSLDHParams.cc`** — `make_cert_and_key` mutates the subject name *in place*. Once the accessor returns const you have to dup, then call **both** `X509_set_subject_name` and `X509_set_issuer_name`, or the generated self-signed cert silently loses its CN. - Also broken on OpenSSL 4 but outside a default build: `example/plugins/c-api/client_context_dump/client_context_dump.cc` and `tests/tools/plugins/ssl_client_verify_test.cc`. Two notes on approach, take or leave: 1. `const_cast` handles the three call sites fine, but it can't handle Cripts. `CertBase::X509Value::_load_name(X509_NAME *(*getter)(const X509 *))` takes a **function pointer**, and `X509_get_subject_name`'s whole type changes in 4.0 — casting a function pointer and calling through it is UB. `X509_getm_notBefore`/`notAfter` also moved const the *opposite* direction (`const X509 *` in 1.1.1, `X509 *` in 4.0), so no single hardcoded spelling works there either. What worked for me was deducing the type from the accessor — `auto *` for locals, `decltype(&X509_get_subject_name)` for parameter and function-pointer types — which needs no casts and no version macros. 2. The `certifier.cc` change (carried from #13440) leaks the duplicated name on the `X509_NAME_add_entry_by_txt` failure path — only the `X509_set_subject_name` path frees it — and doesn't null-check `X509_NAME_dup`. Diffs for all of the above are on `cmcfarlen:ossl4-compat` (#13482) if you want to cherry-pick or copy from them; verified green on Fedora 45/OpenSSL 4.0.1, Fedora 43/OpenSSL 3.5.7, and Ubuntu 20.04/OpenSSL 1.1.1f + clang-12, with `test_tscore` and `test_net` passing on all three. Happy to just close mine and let you carry it however you prefer. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
