oidc_child: add new option return-tokens

[sumit-bose]

oidc_child should only return access and refresh tokens during authentication if the new option '--return-tokens' is given.

Resolves: #8616

[sumit-bose]

Hi @eisenmann-b1 ,

can you check if this patch breaks any of your use-cases?

Thanks.

bye,
Sumit

[gemini-code-assist]

Code Review

This pull request introduces a --return-tokens command-line option to the OIDC child process, allowing token data to be returned conditionally. It also updates the IDP provider to pass this flag during PAM authentication. A review comment identified an issue in src/providers/idp/idp_auth.c where an incorrect debug level constant was used in a DEBUG macro.

[alexey-tikhonov]

oidc_child should only return access and refresh tokens during authentication if the new option '--return-tokens' is given.

Resolves: #8616

@sumit-bose, @abbra, wouldn't it make sense to make ipa-otpd more robust instead?

[sumit-bose]

oidc_child should only return access and refresh tokens during authentication if the new option '--return-tokens' is given.
Resolves: #8616

@sumit-bose, @abbra, wouldn't it make sense to make ipa-otpd more robust instead?

Hi,

no, ipa-otpd expects in this case just the identifier for the authenticated user to make sure the expected user is authenticated. Anything extra would make the evaluation more complex and as a result might offer more potential for misuse.

bye,
Sumit

[alexey-tikhonov]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a --return-tokens flag to the OIDC child process to control whether token data is returned in JSON format. A bug was identified in the SSS_CMD_RENEW case within src/providers/idp/idp_auth.c where the index c is not incremented after adding the new flag, causing it to be overwritten by the subsequent argument.

[sssd-bot]

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

Functional Issues

1. Missing c++ in SSS_CMD_RENEW case — --return-tokens is silently dropped

   src/providers/idp/idp_auth.c:74: After storing "--return-tokens" in extra_args[c] at line 68 and null-checking it, the index c is not incremented before assigning "--refresh-access-token" at line 74. This overwrites --return-tokens with --refresh-access-token, so oidc_child never receives the --return-tokens flag during token renewal.

   As a result, oidc_child will output \nuser_identifier (no JSON token data) but eval_access_token_buf() in src/providers/idp/idp_auth_eval.c:255 unconditionally calls json_loadb() on the (empty) content before the first newline, producing the exact error seen in issue #8616: "Failed to parse token data on line [1]: [No error]." This breaks all automatic token refresh.

   The SSS_PAM_AUTHENTICATE case (lines 58–65) correctly has c++; between the two assignments. The SSS_CMD_RENEW case needs the same fix:

   case SSS_CMD_RENEW:
       extra_args[c] = talloc_strdup(extra_args, "--return-tokens");
       if (extra_args[c] == NULL) {
           DEBUG(SSSDBG_OP_FAILURE, "Failed to add option.\n");
           ret = ENOMEM;
           goto done;
       }
       c++;  /* <-- missing */
       extra_args[c] = talloc_strdup(extra_args, "--refresh-access-token");
       break;

Nits & Non-functional Issues

1. No resilience in eval_access_token_buf for missing token data

   src/providers/idp/idp_auth_eval.c:248–260: When --return-tokens is not passed (or oidc_child is invoked outside the idp provider), the output format changes from {json}\nuser_identifier to \nuser_identifier. The function will attempt json_loadb("", 0, ...) which fails with a confusing "No error" message. Consider either: (a) detecting token_buflen == 0 and skipping token storage with a debug message, or (b) documenting that eval_access_token_buf requires --return-tokens to have been passed. This would make debugging easier if a similar issue recurs.

2. Test coverage: There are no unit or integration tests verifying the --return-tokens flag behavior — both the conditional output in oidc_child and the correct argument construction in set_oidc_auth_extra_args. A test for the SSS_CMD_RENEW argument building would have caught the c++ bug.

Confirmed Issues from Existing Review Comments

1. gemini-code-assist — missing c++ in SSS_CMD_RENEW: Confirmed. The index is not incremented after appending --return-tokens, causing --refresh-access-token to overwrite it at src/providers/idp/idp_auth.c:74. This is a real bug that will break token refresh (see Functional Issue #1 above).

2. gemini-code-assist — wrong debug level constant: This appears to have been fixed in a subsequent force-push. The current revision at src/providers/idp/idp_auth.c:60 and src/providers/idp/idp_auth.c:70 both correctly use SSSDBG_OP_FAILURE.

[eisenmann-b1]

@sumit-bose
AFAICS everything still works.
I added the missing c++ though, as noted above by gemini.

[sumit-bose]

@sumit-bose AFAICS everything still works. I added the missing c++ though, as noted above by gemini.

Hi,

thank you for the fast feedback. Sorry about the c++ I was in a hurry preparing the patch.

bye,
Sumit

[alexey-tikhonov]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new --return-tokens command-line option to the oidc_child utility, allowing it to optionally output OIDC token data in JSON format. The idp_auth provider is updated to utilize this flag during authentication and renewal processes. Additionally, the output formatting of the user identifier in oidc_child has been adjusted. I have no feedback to provide.

[alexey-tikhonov]

Note: Covscan is clean.

[sssd-bot]

The pull request was accepted by @alexey-tikhonov with the following PR CI status:

---

🟢 CodeQL (success)
🟢 osh-diff-scan:fedora-rawhide-x86_64:upstream (success)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-44-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / intgcheck (fedora-45) (success)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🟢 ci / system (fedora-44) (success)
🟢 ci / system (fedora-45) (success)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)

---

There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.