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 <notificati...@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.
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/SSSD/sssd/pull/94#issuecomment-268482769>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AAG4epea6S0_Rl39hsujP_N_VjmQLJzVks5rKPiBgaJpZM4K8AJs>
> .
>
I'll submit another version soon,
Thanks for the review!
"""
See the full comment at
https://github.com/SSSD/sssd/pull/94#issuecomment-268489832
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
