enable file caching in OpenSC

[mvogt1]

OpenSC sets use_file_caching to public. The speedup by file caching is around the factor of 10. Current sssd does not benefit from this speedup, leading to poor performance for smartcards.

This patch:

• enables file caching in OpenSC by exporting XDG_CACHE_HOME
• places the OpenSC cache to SSS_STATEDIR

Issue #8743

[gemini-code-assist]

Code Review

This pull request sets the XDG_CACHE_HOME environment variable to SSS_STATEDIR in both p11_child_common.c and krb5_child.c. However, the error handling for setenv is incorrect because setenv returns -1 on failure rather than a positive errno value. Comparing its return value directly against EOK or assigning it to ret (which expects a valid positive error code in SSSD) is improper. The reviewer suggests checking if the return value is non-zero and setting ret to errno on failure.

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.

[alexey-tikhonov]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces the setting of the XDG_CACHE_HOME environment variable to SSS_STATEDIR in both p11_child_common.c and krb5_child.c. The review feedback correctly identifies that the return value of setenv is being compared against EOK instead of standard POSIX return values (where setenv returns 0 on success and -1 on failure). Additionally, the feedback highlights code style inconsistencies, such as 3-space indentation instead of 4-space indentation and missing spaces after commas in function arguments, and provides code suggestions to fix these issues.

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]

The return value of setenv should be checked against 0 (or != 0) rather than EOK, as setenv is a standard POSIX function that returns -1 on failure, not an SSSD-specific function returning an errno.

Additionally, please fix the code style issues:

1. Use 4-space indentation for the body of the if statement (currently it uses 3 spaces).
2. Add spaces after commas in function calls (setenv and strerror).

    ret = setenv("XDG_CACHE_HOME", SSS_STATEDIR, 1);
    if (ret != 0) {
        ret = errno;
        DEBUG(SSSDBG_MINOR_FAILURE, "Cannot set XDG_CACHE_HOME (%s).\n", strerror(ret));
        goto done;
    }

[gemini-code-assist]

The return value of setenv should be checked against 0 (or != 0) rather than EOK, as setenv is a standard POSIX function that returns -1 on failure, not an SSSD-specific function returning an errno.

Additionally, please fix the code style issues:

1. Use 4-space indentation for the body of the if statement (currently it uses 3 spaces).
2. Add spaces after commas in function calls (setenv and strerror).
3. Remove the extra empty line added after the if block.

    ret = setenv("XDG_CACHE_HOME", SSS_STATEDIR, 1);
    if (ret != 0) {
        ret = errno;
        DEBUG(SSSDBG_MINOR_FAILURE, "Cannot set XDG_CACHE_HOME (%s).\n", strerror(ret));
        goto done;
    }

[sssd-bot]

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

Functional Issues

• Fatal abort on a non-critical optimization (p11_child_common.c:323, krb5_child.c:4410): Both call sites use goto done when setenv fails, which aborts the entire child process. Setting XDG_CACHE_HOME is purely a performance optimization — without it, OpenSC falls back to uncached operation (the current behavior). A setenv failure should log a warning and continue, not prevent authentication from working at all.

• Cache location mixes with SSSD state data: SSS_STATEDIR resolves to /var/lib/sss, which contains LDB caches, keytabs, and other critical SSSD state. OpenSC will write its file cache directly into this directory tree (e.g., /var/lib/sss/.cache/opensc/), co-mingling third-party cache files with SSSD internal data. Issue 8743 specifically recommends a dedicated directory. Consider using a subdirectory like SSS_STATEDIR"/opensc_cache" or a path under /var/cache/, and creating it at startup if it doesn't exist.

• Missing SELinux policy: Issue 8743 notes that p11_child runs in the sssd_t SELinux context, and OpenSC file caching requires write access to the cache directory. On SELinux-enforcing systems (RHEL/Fedora — the primary deployment targets), OpenSC cache writes will be denied by default, silently negating the intended speedup. The PR should include an SELinux policy update or at minimum document that one is needed.

• Unconditional setenv in krb5_child (krb5_child.c:4406): krb5_child handles all Kerberos authentication, not just PKINIT/smart card flows. Setting XDG_CACHE_HOME for every Kerberos authentication is unnecessary. This should be conditioned on PKINIT being active (e.g., checking IS_SC_AUTHTOK(kr->pd->authtok)), or at minimum placed closer to the PKINIT-specific code path to clarify intent.

• Commit message missing Resolves: and :relnote:: The commit message ("enable file caching in OpenSC") does not reference issue 8743 and is missing a :relnote: tag. This is a user-visible performance improvement for smart card authentication and warrants a release note.

Nits & Non-functional Issues

• 3-space indentation instead of 4 (p11_child_common.c:321-323, krb5_child.c:4408-4410): The lines inside the if block use 3-space indentation. The SSSD coding style requires 4 spaces.

• Missing spaces after commas (p11_child_common.c:319, krb5_child.c:4406): setenv("XDG_CACHE_HOME",SSS_STATEDIR,1) should be setenv("XDG_CACHE_HOME", SSS_STATEDIR, 1). Similarly, the DEBUG format string argument strerror(ret)) at p11_child_common.c:322 and krb5_child.c:4409 is missing a space after the comma separating the format string from strerror(ret).

• Extra blank line (krb5_child.c:4412-4413): There are two consecutive blank lines after the closing brace of the if block. Only one is needed.

• Semantic mismatch in return value check (p11_child_common.c:320, krb5_child.c:4407): setenv returns 0 on success and -1 on error (POSIX convention). Comparing against EOK (an SSSD error code macro that happens to equal 0) works numerically but is semantically misleading — ret != 0 would be more appropriate for a POSIX return value. The subsequent ret = errno assignment is correct.

Review of Existing Review Comments

• gemini-code-assist comments on setenv return value: These correctly identify that comparing setenv's return against EOK is non-idiomatic. However, the claim that the check is "incorrect" overstates the issue — since EOK is 0 and setenv returns 0 on success, the check functions correctly. The ret = errno assignment on the error path is also correct. The real concern is semantic clarity, not a bug.

• alexey-tikhonov comment on strerror(errno) vs strerror(ret): The current code already uses strerror(ret) after assigning ret = errno, so this appears to have been addressed. The request to "squash into a single patch" is already satisfied — the PR contains a single commit.

• gemini-code-assist style comments (indentation, comma spacing, extra blank line): All valid and confirmed above. These should be fixed.

• None of the existing comments address the most significant issue: the goto done causing a fatal abort for what should be a non-critical optimization failure. This is the highest-priority item to fix.

[sumit-bose]

Hi,

thank you for the patch. I think it would be beneficial for future readers to add a short comment here and in krb5_child.c because the connection to OpenSC caching is not obvious.

bye,
Sumit

[mvogt1]

When reading the gemini comments:

• The position of the setenv in krb5_child.c can be done somewhere else, gemini is correct. It's only an OpenSC optimisation. (Any suggestions for a different place?)

• If setenv fails, this is not a failure for the authentication, correct, there is no need to abort the authentication, gemini is correct too.

So the goto done is not needed in both cases. The case when this may happen is

• EINVAL name is NULL, points to a string of length 0, or contains an '=' character.
• ENOMEM Insufficient memory to add a new variable to the environment.

If the goto done; is removed, then the ret=errno is not needed too of course.

[mvogt1]

I will have a look for a better place for setenv in krb5_child.c

Q: How can I prepare such pull request better in the future?

The Gemini and Claude suggestions adds some noise here, I didn't know that they are that chatty. Any way to resolve this (in advance) in my fork?

[alexey-tikhonov]

The Gemini and Claude suggestions adds some noise here

Perhaps you can setup Gemini code review in your fork - see https://developers.google.com/gemini-code-assist/resources/faqs
But noise isn't a problem.