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