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
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
