Code Review

This pull request updates OpenSSL usage to improve compatibility with newer versions, primarily by adopting accessor functions and adding const qualifiers to various OpenSSL structures. However, several critical issues were identified in the feedback. The cast of x400Address to ASN1_TYPE * is problematic and could lead to memory corruption or incorrect DER encoding if the underlying type has changed to ASN1_STRING *. Furthermore, using talloc_strndup to copy binary SID data is a bug, as it will truncate the data at the first null byte; this data should be properly encoded or copied as a binary blob.

Review done using Claude Code / claude-opus-4-6

Functional Issues

1. Incomplete OpenSSL 4 fixes in p11_child_openssl.c — The PR updates get_issuer_subject_str but misses two other locations where X509_get_issuer_name() / X509_get_subject_name() return values are assigned to non-const pointers. In OpenSSL 4, these functions return const X509_NAME *, so these will produce compilation warnings (or errors with -Werror):

   • p11_child/p11_child_openssl.c:294 — X509_NAME *issuer_name = NULL; in do_ocsp(), assigned at p11_child/p11_child_openssl.c:330
   • p11_child/p11_child_openssl.c:969 — X509_NAME *tmp_name; in read_certs(), assigned at p11_child/p11_child_openssl.c:1096

   The competing PR (the one this claims to improve upon) fixes both of these.

2. Cast-away-const for X509_EXTENSION_get_data is fragile — At sss_cert_content_crypto.c:777, ext is declared const X509_EXTENSION * unconditionally, then const is cast away for the OpenSSL 3 call: X509_EXTENSION_get_data((X509_EXTENSION *)ext). This works but is the kind of pattern that silently hides real const-correctness bugs. The competing PR avoids this by using a version-conditional OSSL4_CONST macro so no cast is ever needed. If the unconditional-const approach is preferred, consider using #if OPENSSL_VERSION_NUMBER >= 0x40000000L around the cast instead of a comment-based reminder.

Nits & Non-Functional Issues

1. Comment has unmatched parenthesis — sss_cert_content_crypto.c:775-776:

   /* cast away const for openssl 3 compat, can be removed once v3 support is
    * over) */

   The closing ) has no matching opening paren.

2. Commit message typo — Second commit message says "compatibilitty" (double t).

3. Unnecessary (ASN1_TYPE *) cast — At sss_cert_content_crypto.c:512 and sss_cert_content_crypto.c:528, the cast (ASN1_TYPE *)current->d.x400Address is a no-op. This code is inside #else (i.e., HAVE_X400ADDRESS_STRING is NOT defined), which means x400Address is already ASN1_TYPE *. The cast adds noise without changing behavior. If the intent is defensive clarity, a comment would be clearer than a silent no-op cast.

Issues from Existing Review Comments

• (ASN1_TYPE *) cast concern (gemini-code-assist) — The bot flagged the cast at lines 512 and 528 as potentially causing memory corruption. This is a false positive: the #ifdef HAVE_X400ADDRESS_STRING / #else guard ensures the #else branch is only compiled when x400Address IS ASN1_TYPE *, so the cast is a no-op. However, the cast is still unnecessary (see nit above).

• talloc_strndup on SID data (gemini-code-assist) — The bot flagged talloc_strndup at line 815 as truncating binary SID data at null bytes. This is also a false positive: the NTDS CA Security Extension (OID 1.3.6.1.4.1.311.25.2.1) stores the SID as a text string (e.g., S-1-5-21-...), not as a binary blob. Additionally, this behavior is unchanged from the pre-PR code — the PR only replaces direct struct member access (->data, ->length) with accessor functions (ASN1_STRING_get0_data(), ASN1_STRING_length()).

Hi,

thank you for the patch, ACK.

Please let me know if you want to fix the deprecated X509_STORE_get0_objects() as well of if I shall replace it with X509_STORE_get1_objects() if OpenSSL 4 is used?

bye,
Sumit