KRB5: let 'krb5_child' tolerate missing cap-set-id

[alexey-tikhonov]

If CAP_SETUID and/or CAP_SETGID are missing, 'krb5_child' will
skip operation that require those capabilities, namely any manipulations
with user ccache.

:packaging:This update makes it possible to not grant CAP_SETUID and CAP_SETGID
to 'krb5_child' binary in a situation where it is not required to store acquired
TGT after user authentication. Taking into account that it is already possible
to avoid using CAP_DAC_READ_SEARCH if keytab is readable by SSSD service user,
and usage of 'selinux_child' isn't always required, this allows to build a setup
with completely privilege-less SSSD to serve certain use cases. In particular,
this might be used to build a container running SSSD on OCP with a restricted
profile.

[alexey-tikhonov]

/gemini review

[gemini-code-assist]

Code Review

This pull request aims to make krb5_child tolerant of missing CAP_SETUID and CAP_SETGID capabilities, which is a valuable enhancement for running SSSD in restricted environments like containers. The approach of using a flag to guard capability-dependent operations is sound. However, my review identified two critical issues. First, the logic for checking capabilities and setting credentials can lead to a partial and inconsistent credential state for the process. Second, a recurring bug in error handling paths causes the child process to exit without responding to the parent, leading to timeouts. I have provided code suggestions to address both of these critical problems.

[gemini-code-assist]

The goto done; statement here will cause the child process to exit without sending a response to the parent process. The parent process will then time out waiting for a response. The error code should be sent back to the parent. This can be fixed by using break; to exit the switch statement, which will then allow k5c_send_data() to be called. This issue also exists in other error paths in this function, such as the offline check for SSS_CMD_RENEW.

        if (!krb5_child_has_setid_cap) {
            ret = KRB5_CC_NOTFOUND;
            break;
        }

[alexey-tikhonov]

The same is done in case if (offline)

[alexey-tikhonov]

Imo, having exit(error_code) is enough.

[alexey-tikhonov]

Note: Covscan is green.

[alexey-tikhonov]

As for testing:

• CI of this PR is to assure there is no regressions for regular packaging
• CI of Patched 'krb5_child' cap-set-id tests #8313 - for example system tests at F-42 - shows how this patch works when set-id bits are stripped: as expected, only tests, that explicitly expect ccache to be populated, fail.
• CI of Unpatched 'krb5_child' cap-set-id tests #8314 shows that without this patch and with set-id bits stripped, tons of tests fail of course.

[alexey-tikhonov]

/gemini review

[gemini-code-assist]

Code Review

The pull request aims to make krb5_child tolerate missing CAP_SETUID and CAP_SETGID capabilities, which is useful for running SSSD in restricted environments like containers. The changes correctly add checks for these capabilities and skip operations that require them, such as manipulating user ccache.

However, I've found a couple of critical issues. One is a potential use of an uninitialized variable in an offline authentication scenario without capabilities. The other is an incorrect use of goto done instead of break in the SSS_CMD_RENEW command handler, which would cause the child process to exit without sending a response. These issues need to be addressed to ensure the correctness and stability of the changes.

[gemini-code-assist]

The ret variable is not assigned a value if offline is true and kr->krb5_child_has_setid_caps is false. This leads to k5c_send_data() being called with an uninitialized variable, which is undefined behavior.

When no capabilities are present, no ccache is created, but the authentication should still be considered successful in an offline scenario. You should initialize ret to EOK in this case.

            if (kr->krb5_child_has_setid_caps) {
                ret = create_empty_ccache(kr);
            } else {
                ret = EOK;
            }

[alexey-tikhonov]

ret is known to be EOK at this point.

[sumit-bose]

Hi,

thank you for the updates, ACK.

bye,
Sumit

[alexey-tikhonov]

Note: Covscan is still green.