KRB5: let 'krb5_child' tolerate missing cap-set-id #8312
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
92a0aaf to
e8f7c10
Compare
alexey-tikhonov
changed the title
|
/gemini review |
There was a problem hiding this comment.
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.
| if (!krb5_child_has_setid_cap) { | ||
| ret = KRB5_CC_NOTFOUND; | ||
| goto done; | ||
| } |
There was a problem hiding this comment.
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;
}There was a problem hiding this comment.
The same is done in case if (offline)
There was a problem hiding this comment.
Imo, having exit(error_code) is enough.
|
Note: Covscan is green. |
|
As for testing:
|
e8f7c10 to
c331c8a
Compare
|
/gemini review |
There was a problem hiding this comment.
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.
| if (kr->krb5_child_has_setid_caps) { | ||
| ret = create_empty_ccache(kr); | ||
| } |
There was a problem hiding this comment.
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;
}There was a problem hiding this comment.
ret is known to be EOK at this point.
sumit-bose
left a comment
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
thank you for the updates, ACK.
bye,
Sumit
|
Note: Covscan is still green. |
alexey-tikhonov
added
Waiting for review
and removed
coverity
|
PR CI failure is not related. |
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. Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Sumit Bose <[email protected]>
c331c8a to
f668580
Compare
4 participants
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.