krb5: Fix logic error in OAuth2 code verification

[kkzhsh]

Fixed a logic errors: a wrong boolean operator in OAuth2 code verification that could allow authentication to proceed with a mismatched user code.

krb5: Fixed a logic error that could lead to incorrect OAuth2 user code verification.

[gemini-code-assist]

Code Review

This pull request corrects logical errors in conditional checks within oidc_child_id.c and krb5_child.c. Specifically, it fixes the @ separator check in keycloak_lookup and ensures that user code validation in answer_idp_oauth2 correctly fails if either the length differs or the string comparison fails. The review feedback identifies a potential NULL pointer dereference vulnerability in krb5_child.c when data or data->user_code is NULL, and suggests adding a guard check to prevent a segmentation fault.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

If data or data->user_code is NULL, calling strlen(data->user_code) will result in a NULL pointer dereference and cause krb5_child to crash with a segmentation fault. Since oath2_data can be NULL if the challenge was not requested or failed to decode, we must safely guard against NULL pointers before performing string operations.

    if (data == NULL || data->user_code == NULL) {
        DEBUG(SSSDBG_OP_FAILURE, "OAuth2 data or user code is missing!\n");
        kerr = EINVAL;
        goto done;
    }

    if (strlen(data->user_code) != token_len || strcmp(data->user_code, token) != 0) {

[alexey-tikhonov]

/gemini review

[gemini-code-assist]

Code Review

This pull request fixes logical conditions in oidc_child_id.c and krb5_child.c, and adds NULL pointer checks to prevent potential crashes. Specifically, it corrects the separator check for group lookups and ensures that the user code validation fails if either the length or the content does not match. The reviewer recommends also checking if token is NULL in krb5_child.c before calling strcmp to avoid a potential segmentation fault.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

While checking data and data->user_code for NULL is a great improvement to prevent potential null-pointer dereferences, token should also be checked for NULL before calling strcmp on it. If token is NULL, strcmp(data->user_code, token) will result in a segmentation fault, crashing the krb5_child process.

    if (data == NULL || data->user_code == NULL || token == NULL) {
        DEBUG(SSSDBG_OP_FAILURE, "OAuth2 data, user code, or token is missing!\n");
        kerr = EINVAL;
        goto done;
    }

    if (strlen(data->user_code) != token_len || strcmp(data->user_code, token) != 0) {

[sumit-bose]

Hi,

thank you for the logic fixes I agree with them. Please consider Gemini's second suggestion as well.

bye,
Sumit

[sssd-bot]

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

Functional Issues

1. Missing NULL check for token in answer_idp_oauth2 — src/providers/krb5/krb5_child.c:1057

   The new NULL guard checks data and data->user_code, but token (returned by sss_authtok_get_oauth2 at line 1052) is not checked. If tok->data is NULL in an SSS_AUTHTOK_TYPE_OAUTH2 auth token, the subsequent strcmp(data->user_code, token) at line 1063 would segfault krb5_child. This was already flagged by the Gemini review and endorsed by @sumit-bose. The guard should be:

   if (data == NULL || data->user_code == NULL || token == NULL) {

2. data == NULL check at line 1057 may be unreachable, masking a real failure — src/providers/krb5/krb5_child.c:1057

   answer_idp_oauth2 is only called from sss_krb5_auth_methods_answer (line 1453) when the question is SSSD_IDP_OAUTH2_QUESTION. In the preceding request phase, request_idp_oauth2 decodes the challenge and returns EINVAL if data is NULL (line 1011–1013). However, sss_krb5_auth_methods_request loops over all questions and the final kerr reflects the last-processed question, so if another question succeeds after the OAuth2 request fails, oath2_data can remain NULL while the function returns EOK, allowing the answer phase to call answer_idp_oauth2 with data == NULL. The NULL check is therefore correct and necessary as a safety net — but consider whether the request loop should propagate the OAuth2 failure more explicitly rather than silently continuing.

3. Missing :relnote: commit message tag — Both fixes are user-visible: the &&-to-|| change in answer_idp_oauth2 is a security-relevant fix (same-length but different user codes would bypass OAuth2 verification), and the keycloak_lookup fix affects group lookup behavior. Per SSSD commit conventions, user-visible changes should carry a :relnote: tag.

Nits & Non-Functional Issues

1. Commit message style — The commit message fix: Fix logic errors in Keycloak lookup and OAuth2 code verification uses a fix: prefix. SSSD uses component: prefixes (e.g., krb5:, oidc_child:). Since this PR touches two separate components, consider splitting into two commits with appropriate prefixes, or using a shared prefix like oidc:.

2. No Resolves: references — If there are upstream issues tracking these bugs, the commit message should include Resolves: https://github.com/SSSD/sssd/issues/<NUMBER> references.

3. AI disclosure — Per the SSSD coding style guide, if generative AI assisted in producing this code, the commit message should include a trailer such as Assisted-By: <tool> (<model>).

4. Grammar in debug message — src/providers/krb5/krb5_child.c:1064: "User code do not match!" should be "User code does not match!" (or "User codes do not match!"). This is a pre-existing issue but is worth fixing while the surrounding code is being modified.

Review of Existing Review Comments

• Gemini comment (NULL check for data/data->user_code): This suggestion was adopted in the PR — the guard at line 1057 was added.

• Gemini comment (also check token for NULL): This suggestion has not been addressed yet. @sumit-bose explicitly endorsed it ("Please consider Gemini's second suggestion as well"). This should be incorporated before merge — see functional issue above.

• @sumit-bose comment: Agrees with the logic fixes and asks the contributor to also address the token NULL check. Still outstanding.

[alexey-tikhonov]

same-length but different user codes would bypass OAuth2 verification

As @sumit-bose explained, this statement is incorrect.

[kkzhsh]

same-length but different user codes would bypass OAuth2 verification

As @sumit-bose explained, this statement is incorrect.

@alexey-tikhonov Thanks for your review, This PR has been updated and another PR(#8950) is to fix the logic error in oidc_child

[sumit-bose]

Hi,

thank you for the fix, ACK.

The CI failures are not related imo.

bye,
Sumit