URL:
https://github.com/SSSD/sssd/pull/94
Title: #94: Enable {socket,dbus}-activation for responders
fidencio commented:
"""
On Wed, Dec 21, 2016 at 10:59 AM, Jakub Hrozek <notifications(a)github.com>
wrote:
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.
Thanks for catching this one. I'll take a look whether they make sense or
not.
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?
Sure.
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.
Sure.
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.
Oh, sorry, my fault messing up with rebase :-\
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.
Okay.
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?
In the testes I've done here (mainly generating the sssd tarball and
installing it on my VM) that's the only thing needed.
The services will be started by any activity on the sockets, doesn't matter
whether it's enabled or not.
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.
Hmm. I don't see how it could be done there, to be honest.
Correct me if I'm mistaken, but that code would be run even if the socket
was set in the services list, no?
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?
Pavel suggested to track internal communication as well. So, that's the
reason we update the "last_request_time" on every internal communication as
well.
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.
That's really really weird. I'll try to reproduce it locally and if not
possible I'll ask access to your test machine.
I'll submit another version soon,
Thanks for the review!
"""
See the full comment at
https://github.com/SSSD/sssd/pull/94#issuecomment-268489832