Openssl 4 fixes

[simo5]

I think this is a better patch than #8614

[gemini-code-assist]

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.

[gemini-code-assist]

Using talloc_strndup to copy the SID value is a bug. SIDs are binary data and frequently contain null bytes (for example, in the Identifier Authority field of an NT SID). talloc_strndup will truncate the SID at the first null byte encountered, leading to an invalid or incomplete SID string. Since the destination sid_ext is a char *, the binary SID should be properly encoded into a string format (e.g., using a hex encoder or a dedicated SID-to-string conversion utility) instead of being copied as a raw string.

[simo5]

I am almost sure this is bogus, because in this case the SID is actually a string, not a binary encoded SID

[sssd-bot]

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()).

[simo5]

Should I drop the "(ASN1_TYPE *) cast " ?
I think I saw a build issus w/o that change, which is odd, I wonder if the HAVE_X400ADDRESS_STRING check failed there an I did not notice ...

[simo5]

Should I drop the "(ASN1_TYPE *) cast " ? I think I saw a build issus w/o that change, which is odd, I wonder if the HAVE_X400ADDRESS_STRING check failed there an I did not notice ...

Found out the issue, with OpenSSL 4 the config test fails like this:

conftest.c: In function 'main':
conftest.c:188:44: error: arithmetic on pointer to an incomplete type
  188 |                           gn.d.x400Address - (ASN1_STRING *)0;
      |                                            ^
conftest.c:188:44: warning: statement with no effect [-Wunused-value]
  188 |                           gn.d.x400Address - (ASN1_STRING *)0;
      |                           ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~

So it ws building the wrong branch, I will update this PR shortly with a different test

[simo5]

Updated the config test, I verified it now works correctly with openssl 4.0, so I also dropped the now unnecessary casts

[simo5]

Note that I am having two test failing on my test rawhide builds, a new libfido2 library version landed there and test_passkey seem to segfault on:

[ RUN      ] test_decode_public_key
[  ERROR   ] --- Test failed with exception: Segmentation fault(11)

and

[ RUN      ] test_authenticate_integration
...
[sssd] [authenticate] (0x0400): Decoding public key.
[  ERROR   ] --- Test failed with exception: Segmentation fault(11)

I do not think this is related to the changes in this PR, as a previous build ws passing.

[simo5]

The failure in test in Fedora rebuilds seem to be related to how libfido2 tries to extract public keys, by using legacy interfaces that do not work the same way in openssl4, the previous build that successfully completed was probably still using a libfido2 linked with openssl3.