URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders
jhrozek commented: """ Coverity seems to have detected a warning: ```` Error: CHECKED_RETURN (CWE-252): sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:323: check_return: Calling "sss_cmd_empty_packet" without checking return value (as is done elsewhere 4 out of 5 times). sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1401: example_assign: Example 1: Assigning: "ret" = return value from "sss_cmd_empty_packet(pctx->creq->out)". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1402: example_checked: Example 1 (cont.): "ret" has its value checked in "ret != 0". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1424: example_assign: Example 2: Assigning: "ret" = return value from "sss_cmd_empty_packet(pctx->creq->out)". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1425: example_checked: Example 2 (cont.): "ret" has its value checked in "ret != 0". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1097: example_assign: Example 3: Assigning: "ret" = return value from "sss_cmd_empty_packet(pctx->creq->out)". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1098: example_checked: Example 3 (cont.): "ret" has its value checked in "ret != 0". sssd-1.14.90/src/responder/common/responder_cmd.c:85: example_assign: Example 4: Assigning: "ret" = return value from "sss_cmd_empty_packet(pctx->creq->out)". sssd-1.14.90/src/responder/common/responder_cmd.c:86: example_checked: Example 4 (cont.): "ret" has its value checked in "ret != 0". # 321| DEBUG(SSSDBG_TRACE_FUNC, "setautomntent did not find requested map\n"); # 322| /* Notify the caller that this entry wasn't found */ # 323|-> sss_cmd_empty_packet(pctx->creq->out); # 324| } else { # 325| DEBUG(SSSDBG_TRACE_FUNC, "setautomntent found data\n"); ```` I'm not sure if it's legit or if we just passed a threshold of checked/unchecked ratio, but it would be nice to not add any new warnings with new commits.
Could you please add a more verbose comment to the commits that enable the responder socket activation (either the first one, autofs, or just copy to all) that explain why the BindsTo option was added and what the workflow is for socket-activated responders and starting and stopping the sssd service?
Similarly, can you please explain in the commit message that adds the `--unprivileged-start` option that the log files are chowned by the unit file and the socket files created by sssd in the most common scenario of this option? I was even wondering if the option would be better named `--socket-activated-start` but I don't have strong feelings about it.
There is a typo in the commit that changes the PAM responder `unprivileged_unprivileged_start`, which would be nice to fix in the commit where it was introduced, so that every commit can be compiled on its own and we can always use bisect.
I have a bit of trouble reading `client_registration()` after ` MONITOR: Deal with socket-activated responders`. Could we please change `client_registration()` so that the ifdefs are a bit less interleaved with the non-systemd code? Even at the cost of a little code duplication, I think this: ``` #ifdef HAVE_SYSTEMD systemd_client_registration(args..) #else managed_client_registration(args..) #endif /* HAVE_SYSTEMD */ ``` Is IMO preferred over the ifdefs being sprinkled around in the code.
About ` MAN: Mention that the services' list is optional`, are you sure just enabling the socket is all that is needed? Doesn't the admin also need to enable the service in addition to the socket?
`MONITOR: Let the responder know whether it was socket-activated`: do you think this commit is needed? Could the responder learn that it's socket activated when it goes through `activate_unix_sockets()` or is that too late? Please note I'm not against this commit per se, I'm just trying to see if we can simplify the code.
I'm not sure I understand the `time_t` pointer being added to the responder context. Shouldn't we only care about the requests from the client, like NSS or D-Bus?
I mostly just read the code, but I'm afraid I'm still having issues with the socket-activated PAM responder. My sssd.conf is as follows: ``` [sssd] services = nss user = sssd
domains = ipa.test ```
I enabled and started the sssd-pam responder socket, then tried to log in as an IPA user, but I'm getting: ``` Dec 21 09:57:04 client.ipa.test su[30415]: pam_sss(su-l:auth): Request to sssd failed. Public socket has wrong ownership or permissions ```
The socket was created as: ``` srw-rw-rw-. 1 sssd sssd 0 Dec 21 09:56 /var/lib/sss/pipes/pam ```
I built sssd from source. Please shout if you'd like to have access to my test machine. """
See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-268482769