oidc_child: add JWT and mTLS authentication

[sumit-bose]

With the new option --client-auth-method oidc_child can select between
authentication with a client secret, mutual-TLS/mTLS (RFC-8705) and JWT
client assertion (RFC-7523). The latter require a PKCS#12 file with the
client credentials (certificate and private key) and the password to
unlock the private key must be provided with the --client-secret or
--client-secret-stdin option.

[gemini-code-assist]

Code Review

This pull request implements support for MTLS (RFC 8705) and JWT-based (RFC 7523) client authentication within the oidc_child component. Key changes include the addition of CLI options for PKCS#12 credentials, the implementation of JWK generation from PKCS#12 files, and the integration of JWT signing using the Jose library. Review feedback highlights critical logic errors in Curl error handling, compatibility issues with OpenSSL versions prior to 3.0, and multiple memory leaks involving BIGNUM, EC_GROUP, and X509 objects.

[alexey-tikhonov]

@sumit-bose, could you please convert corresponding commit messages to release notes?

[sssd-bot]

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

Functional Issues

• EC JWK missing alg field breaks JWT signing with EC keys. ec_priv_key_jwk (src/util/cert/libcrypto/cert.c:388-395) emits kty, crv, x, y, d, and x5t#S256 but no "alg" field. The RSA counterpart rsa_priv_key_jwk includes "alg":"RS256" (line 602). In get_jwt() (src/oidc_child/oidc_child_json.c:1256-1260), alg is extracted via get_json_const_string_from_keys(jwk, "alg") and the function fails with "JWK is missing required attributes" if it is NULL. This means JWT client assertion with EC keys will always fail. The EC JWK should include an appropriate alg value: "ES256" for P-256, "ES384" for P-384, or "ES512" for P-521.

• No OpenSSL < 3.0 compatibility for new crypto functions. sss_rsa_get_priv_comps (src/util/cert/libcrypto/cert.c:417) and sss_ec_get_x_y_d (line 253) use EVP_PKEY_get_bn_param() and OSSL_PKEY_PARAM_* macros, which are OpenSSL 3.0+ only. Similarly, get_cert_sha256_hash (line 1038) uses EVP_MD_fetch() / EVP_MD_free(), also 3.0+ APIs. The <openssl/core_names.h> include is guarded by #if OPENSSL_VERSION_NUMBER >= 0x30000000L (line 24), so these functions will fail to compile on OpenSSL < 3.0. The existing functions in the same file (sss_rsa_get_key, sss_ec_get_key) have proper dual-path implementations for both pre- and post-3.0. Either add version guards/fallbacks, or guard get_jwk_from_pkcs12 and its callees with #if OPENSSL_VERSION_NUMBER >= 0x30000000L, and provide a stub returning ENOTSUP for older versions. (I note the gemini bot claimed these were fixed after "/gemini see update" replies, but the code at the current PR HEAD — commit 57117fd16 — still lacks version guards.)

• client_credentials_grant does not support JWT/mTLS auth methods. client_credentials_grant() (src/oidc_child/oidc_child_curl.c:858-906) always appends client_secret directly to POST data (line 881) rather than routing through append_to_creds_to_post_data(). When oidc_get_id() calls this function with CAM_JWT, the PKCS#12 password is incorrectly sent as client_secret in the POST body instead of a JWT client assertion. Similarly for CAM_MTLS — the client_secret (which is actually the PKCS#12 password) should not appear in the POST data at all.

• refresh_token does not support JWT/mTLS auth methods. refresh_token() (src/oidc_child/oidc_child_curl.c:939-946) directly appends client_secret to POST data rather than using append_to_creds_to_post_data(). Token refresh will fail when using JWT or mTLS client authentication.

• get_jwt_payload truncates timestamps to int. In get_jwt_payload() (src/oidc_child/oidc_child_json.c:1202-1209), time_t values (iat, nbf, exp) are cast to (int) and packed with json_pack format "s:i". On 32-bit platforms (or post-Y2038), time_t values will overflow int. Use "s:I" format (which expects json_int_t) instead.

Nits & Non-functional Issues

• Missing space in error message. In check_client_auth() (src/oidc_child/oidc_child.c:423-424), the string literals "not needed for" and "authentication with client secret.\n" are concatenated without a space, producing: "not needed forauthentication with client secret."

• Debug message copy-paste error. In sss_ec_get_x_y_d() (src/util/cert/libcrypto/cert.c:280), the debug message for the d (private key) parameter says "Failed to retrieve EC y coordinate." — should say "private key" or "d parameter".

• Duplicate JOSE_CFLAGS/JOSE_LIBS in Makefile.am. $(JOSE_CFLAGS) is added to both SSS_CERT_CFLAGS and directly to libsss_cert_la_CFLAGS. Since libsss_cert_la_CFLAGS already includes $(SSS_CERT_CFLAGS), the jose flags appear twice. Same for $(JOSE_LIBS) in libsss_cert_la_LIBADD.

• Dead code in append_jwt_to_postdata. At src/oidc_child/oidc_child_curl.c:567, talloc_free(out) is called when out is already NULL (since append_to_post_data returned NULL). The call is harmless but misleading.

• Missing :relnote: tags. Commits "oidc_child: add JWT authentication" and "oidc_child: add pkcs12-client-creds option" introduce new user-visible CLI options (--pkcs12-client-creds, --client-auth-method) and new authentication methods but lack :relnote: tags in their commit messages.

• static char curve_name[4096] in sss_ec_get_x_y_d. The curve_name buffer at src/util/cert/libcrypto/cert.c:261 is declared static, which makes the function non-reentrant. While unlikely to cause issues in the current single-threaded usage, a stack-allocated buffer would be safer.

Review of Existing Review Comments

• Gemini: curl ret/res variable mismatch in set_http_opts — The author responded with "/gemini see update" and gemini confirmed this was fixed. The current code at src/oidc_child/oidc_child_curl.c:377-397 correctly uses res for all curl_easy_setopt calls. Resolved.

• Gemini: OpenSSL 3.0+ API usage without version guards in sss_ec_get_x_y_d — The author responded with "/gemini see update" and gemini claimed the fix was applied. However, examining the current PR HEAD (57117fd16), sss_ec_get_x_y_d at line 253 still uses OpenSSL 3.0+ APIs without #if guards. Not resolved in the code I reviewed.

• Gemini: sss_rsa_get_priv_comps incomplete for OpenSSL < 3.0 — Similar situation: gemini claimed the fix was applied, but the current code at line 417 still has a single implementation using only 3.0+ APIs. Not resolved in the code I reviewed.

• Gemini: BIGNUM memory leaks in ec_priv_key_jwk — The current code at src/util/cert/libcrypto/cert.c:409-413 properly frees x, y, d via BN_free() and ec_group via EC_GROUP_free() in the cleanup block. Resolved.

• Gemini: BIGNUM memory leaks in rsa_priv_key_jwk — The current code at lines 626-633 properly frees all eight BIGNUMs. Resolved.

• Gemini: X509 cert memory leak in get_jwk_from_pkcs12 — The current code at line 1146 calls X509_free(cert) in the done block. Resolved.

• CodeQL: Poorly documented functions — sss_rsa_get_priv_comps (115 lines) and rsa_priv_key_jwk (113 lines) were flagged. SSSD coding style says "Avoid comments that do not add value to the code" — these functions are straightforward enough that their names describe their purpose. Not a concern per SSSD conventions.

[sumit-bose]

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

Functional Issues

* **EC JWK missing `alg` field breaks JWT signing with EC keys.** `ec_priv_key_jwk` (`src/util/cert/libcrypto/cert.c:388-395`) emits `kty`, `crv`, `x`, `y`, `d`, and `x5t#S256` but no `"alg"` field. The RSA counterpart `rsa_priv_key_jwk` includes `"alg":"RS256"` (line 602). In `get_jwt()` (`src/oidc_child/oidc_child_json.c:1256-1260`), `alg` is extracted via `get_json_const_string_from_keys(jwk, "alg")` and the function fails with "JWK is missing required attributes" if it is NULL. This means **JWT client assertion with EC keys will always fail**. The EC JWK should include an appropriate `alg` value: `"ES256"` for P-256, `"ES384"` for P-384, or `"ES512"` for P-521.

fixed

* **No OpenSSL < 3.0 compatibility for new crypto functions.** `sss_rsa_get_priv_comps` (`src/util/cert/libcrypto/cert.c:417`) and `sss_ec_get_x_y_d` (line 253) use `EVP_PKEY_get_bn_param()` and `OSSL_PKEY_PARAM_*` macros, which are OpenSSL 3.0+ only. Similarly, `get_cert_sha256_hash` (line 1038) uses `EVP_MD_fetch()` / `EVP_MD_free()`, also 3.0+ APIs. The `<openssl/core_names.h>` include is guarded by `#if OPENSSL_VERSION_NUMBER >= 0x30000000L` (line 24), so these functions will fail to compile on OpenSSL < 3.0. The existing functions in the same file (`sss_rsa_get_key`, `sss_ec_get_key`) have proper dual-path implementations for both pre- and post-3.0. Either add version guards/fallbacks, or guard `get_jwk_from_pkcs12` and its callees with `#if OPENSSL_VERSION_NUMBER >= 0x30000000L`, and provide a stub returning `ENOTSUP` for older versions. (I note the gemini bot claimed these were fixed after "/gemini see update" replies, but the code at the current PR HEAD — commit `57117fd16` — still lacks version guards.)

not fixed yet

* **`client_credentials_grant` does not support JWT/mTLS auth methods.** `client_credentials_grant()` (`src/oidc_child/oidc_child_curl.c:858-906`) always appends `client_secret` directly to POST data (line 881) rather than routing through `append_to_creds_to_post_data()`. When `oidc_get_id()` calls this function with `CAM_JWT`, the PKCS#12 password is incorrectly sent as `client_secret` in the POST body instead of a JWT client assertion. Similarly for `CAM_MTLS` — the `client_secret` (which is actually the PKCS#12 password) should not appear in the POST data at all.

fixed

* **`refresh_token` does not support JWT/mTLS auth methods.** `refresh_token()` (`src/oidc_child/oidc_child_curl.c:939-946`) directly appends `client_secret` to POST data rather than using `append_to_creds_to_post_data()`. Token refresh will fail when using JWT or mTLS client authentication.

fixed

* **`get_jwt_payload` truncates timestamps to `int`.** In `get_jwt_payload()` (`src/oidc_child/oidc_child_json.c:1202-1209`), `time_t` values (`iat`, `nbf`, `exp`) are cast to `(int)` and packed with `json_pack` format `"s:i"`. On 32-bit platforms (or post-Y2038), `time_t` values will overflow `int`. Use `"s:I"` format (which expects `json_int_t`) instead.

fixed

Nits & Non-functional Issues

* **Missing space in error message.** In `check_client_auth()` (`src/oidc_child/oidc_child.c:423-424`), the string literals `"not needed for"` and `"authentication with client secret.\n"` are concatenated without a space, producing: "not needed forauthentication with client secret."

fixed

* **Debug message copy-paste error.** In `sss_ec_get_x_y_d()` (`src/util/cert/libcrypto/cert.c:280`), the debug message for the `d` (private key) parameter says "Failed to retrieve EC y coordinate." — should say "private key" or "d parameter".

fixed

* **Duplicate `JOSE_CFLAGS`/`JOSE_LIBS` in Makefile.am.** `$(JOSE_CFLAGS)` is added to both `SSS_CERT_CFLAGS` and directly to `libsss_cert_la_CFLAGS`. Since `libsss_cert_la_CFLAGS` already includes `$(SSS_CERT_CFLAGS)`, the jose flags appear twice. Same for `$(JOSE_LIBS)` in `libsss_cert_la_LIBADD`.

fixed

* **Dead code in `append_jwt_to_postdata`.** At `src/oidc_child/oidc_child_curl.c:567`, `talloc_free(out)` is called when `out` is already NULL (since `append_to_post_data` returned NULL). The call is harmless but misleading.

fixed

* **Missing `:relnote:` tags.** Commits "oidc_child: add JWT authentication" and "oidc_child: add pkcs12-client-creds option" introduce new user-visible CLI options (`--pkcs12-client-creds`, `--client-auth-method`) and new authentication methods but lack `:relnote:` tags in their commit messages.

not yet added

* **`static char curve_name[4096]` in `sss_ec_get_x_y_d`.** The `curve_name` buffer at `src/util/cert/libcrypto/cert.c:261` is declared `static`, which makes the function non-reentrant. While unlikely to cause issues in the current single-threaded usage, a stack-allocated buffer would be safer.

fixed

Review of Existing Review Comments

* **Gemini: curl `ret`/`res` variable mismatch in `set_http_opts`** — The author responded with "/gemini see update" and gemini confirmed this was fixed. The current code at `src/oidc_child/oidc_child_curl.c:377-397` correctly uses `res` for all `curl_easy_setopt` calls. **Resolved.**

* **Gemini: OpenSSL 3.0+ API usage without version guards in `sss_ec_get_x_y_d`** — The author responded with "/gemini see update" and gemini claimed the fix was applied. However, examining the current PR HEAD (`57117fd16`), `sss_ec_get_x_y_d` at line 253 still uses OpenSSL 3.0+ APIs without `#if` guards. **Not resolved** in the code I reviewed.

* **Gemini: `sss_rsa_get_priv_comps` incomplete for OpenSSL < 3.0** — Similar situation: gemini claimed the fix was applied, but the current code at line 417 still has a single implementation using only 3.0+ APIs. **Not resolved** in the code I reviewed.

* **Gemini: BIGNUM memory leaks in `ec_priv_key_jwk`** — The current code at `src/util/cert/libcrypto/cert.c:409-413` properly frees `x`, `y`, `d` via `BN_free()` and `ec_group` via `EC_GROUP_free()` in the cleanup block. **Resolved.**

* **Gemini: BIGNUM memory leaks in `rsa_priv_key_jwk`** — The current code at lines 626-633 properly frees all eight BIGNUMs. **Resolved.**

* **Gemini: X509 cert memory leak in `get_jwk_from_pkcs12`** — The current code at line 1146 calls `X509_free(cert)` in the `done` block. **Resolved.**

* **CodeQL: Poorly documented functions** — `sss_rsa_get_priv_comps` (115 lines) and `rsa_priv_key_jwk` (113 lines) were flagged. SSSD coding style says "Avoid comments that do not add value to the code" — these functions are straightforward enough that their names describe their purpose. Not a concern per SSSD conventions.

[sumit-bose]

Hi,

I have a question about where to add the code form the first patch, i.e. get_jwk_from_pkcs12() and the new functions it calls.

src/util/cert/libcrypto/cert.c seems quite natural because it already contains get_ssh_key_from_cert() which also creates a different representation of a key from a certificates although here only the public one.

However, due to the requirement to do base64-url encoding the dependency to libjose is added and as a result libjose is now linked into various SSSD components which have no use for it.

As an alternative all the new functions can be move to a (new) file in oidc_child/ so that the libjose dependency can be kept with oidc_child.

What would you prefer?

bye,
Sumit

[alexey-tikhonov]

However, due to the requirement to do base64-url encoding the dependency to libjose is added and as a result libjose is now linked into various SSSD components which have no use for it.

Via libsss_cert.so?

As an alternative all the new functions can be move to a (new) file in oidc_child/ so that the libjose dependency can be kept with oidc_child.

Since oidc_child is the only user so far, this would be fine, imo.