Code Review

This pull request introduces support for client authentication methods (MTLS and JWT client assertions) in oidc_child by extracting JWK from PKCS#12 files using OpenSSL's libcrypto. It updates the CLI options, HTTP request handling, and adds corresponding unit and system tests. The review highlights several critical issues: potential NULL pointer dereferences after PKCS12_parse and before open(), a double-free vulnerability when json_pack fails, and compilation breakage on platforms with OpenSSL 1.1.1 due to the unconditional use of OpenSSL 3.0+ APIs.

Review done using Claude Code / Opus 4.6

Functional Issues

Missing NULL checks after PKCS12_parse() — potential NULL pointer dereference

At src/oidc_child/libcrypto/oidc_child_get_jwk.c:574, PKCS12_parse() can return success (1) even when the PKCS#12 file does not contain a private key or certificate, leaving pkey or cert as NULL. The code proceeds to use both without NULL checks — cert at line 581 in get_cert_sha256_hash() and pkey at line 589 in EVP_PKEY_base_id(). This would cause a segfault on a malformed or certificate-only PKCS#12 file. Add NULL checks for both pkey and cert after the PKCS12_parse() call.

Missing NULL check before open() in get_jwk()

At src/oidc_child/oidc_child_json.c:1132, rest_ctx_get_pkcs12_client_creds(rest_ctx) is passed directly to open() without checking for NULL. While get_jwk() is currently only called from the CAM_JWT path where the PKCS#12 path is expected to be set, open(NULL, O_RDONLY) is undefined behavior. A defensive NULL check before the open() call would prevent a crash if this function is ever called from an unexpected path.

Potential double-free with json_pack "o" format on failure

At src/oidc_child/oidc_child_json.c:1235-1237, json_pack("{s:o}", "payload", payload_b64) uses the "o" format which steals the reference of payload_b64. Jansson's internal implementation calls json_object_set_new(), which decrements the reference count of the value even when the set operation fails. If json_pack fails, payload_b64 may already be freed, and the explicit json_decref(payload_b64) on line 1237 would cause a double-free. Consider switching to the "O" format (which increments the refcount) and always calling json_decref(payload_b64) afterward, or simply removing the json_decref(payload_b64) from the error block.

key_passwd stored as borrowed pointer without copy in rest_ctx

At src/oidc_child/oidc_child_curl.c:94, rest_ctx->key_passwd = key_passwd; stores a raw pointer without talloc_strdup. This is inconsistent with how ca_db (line 78) and pkcs12_client_creds (line 87) are handled in the same function — both are properly copied with talloc_strdup. In main() (src/oidc_child/oidc_child.c:986-991), free_cli_opts_members(&opts) frees opts.client_secret (which is the source of key_passwd) before talloc_free(main_ctx) frees the rest_ctx that still holds the pointer. While no code dereferences key_passwd during cleanup, this creates a dangling pointer window. The key_passwd should be talloc_strdup'd to match the pattern used for the other fields.

Nits & Non-Functional Issues

Test docstrings are generic and non-descriptive

The new system tests test_oidc_child__get_device_code, test_oidc_child__get_device_code_jwt, and test_oidc_child__get_device_code_mtls (at src/tests/system/tests/test_oidc_child.py:124,153,207) all have :title: Authenticate with default settings, which does not describe what the tests actually verify. The titles should reflect the specific feature being tested (e.g., "Request device code with client secret authentication", "Request device code with JWT client assertion", "Request device code with mTLS client authentication").

Function name append_to_creds_to_post_data has awkward phrasing

At src/oidc_child/oidc_child_curl.c:576, the function name reads awkwardly with the double "to". Consider append_creds_to_post_data for clarity.

PKCS#12 file buffer not securely erased in get_jwk()

At src/oidc_child/oidc_child_json.c:1159, buf holds the raw PKCS#12 file contents (encrypted key material) but is freed with plain talloc_free (line 1189) without setting sss_erase_talloc_mem_securely as a destructor. The rest of the code is careful about securely erasing sensitive data (e.g., sss_bn_to_base64 in oidc_child_get_jwk.c:94 and ec_priv_key_jwk at line 284). Consider adding talloc_set_destructor((void *) buf, sss_erase_talloc_mem_securely); after allocation for consistency.

mTLS test keystore cleanup fragile on failure

At src/tests/system/tests/test_oidc_child.py:260-262, the keystore cleanup (keytool -delete) is placed between the oidc_child invocation and the assertions. If the oidc_child command itself fails with an exception, the cleanup is skipped. The existing comment acknowledges this ("Currently the keystore is not restored by the framework"). A try/finally block or a pytest fixture with teardown would make this more robust.

Review of Existing Review Comments

Comment about NULL pointer dereferences after PKCS12_parse (gemini-code-assist) — Valid and confirmed. PKCS12_parse() can return success with NULL pkey/cert. This is covered above as a functional issue.

Comment about double-free with json_pack "o" format (gemini-code-assist) — Valid concern. The ownership semantics of the "o" format on json_pack failure make the explicit json_decref potentially dangerous. Covered above.

Comment about missing NULL check before open() (gemini-code-assist) — Valid. Covered above as a functional issue.

Comment about OpenSSL 3.0+ API compatibility (gemini-code-assist) — Addressed by the maintainer (@alexey-tikhonov) noting that support for older OpenSSL versions will be removed in a separate PR. No action needed here.