On Mon, Jan 9, 2017 at 11:23 AM, Lukas Slebodnik <lslebodn(a)redhat.com> wrote:
On (08/01/17 21:44), Fabiano Fidêncio wrote:
>diff --git a/src/util/server.c b/src/util/server.c
>index c9a2726..d9da803 100644
>--- a/src/util/server.c
>+++ b/src/util/server.c
>@@ -445,6 +445,7 @@ static const char *get_pid_path(void)
>
> int server_setup(const char *name, int flags,
> uid_t uid, gid_t gid,
>+ char *sssd_user,
> const char *conf_entry,
> struct main_context **main_ctx)
> {
>@@ -463,6 +464,22 @@ int server_setup(const char *name, int flags,
>
> setenv("_SSS_LOOPS", "NO", 0);
>
>+ if (sssd_user != NULL) {
>+ struct passwd *pw;
>+
>+ pw = getpwnam(sssd_user);
>+ if (pw == NULL) {
>+ DEBUG(SSSDBG_MINOR_FAILURE,
>+ "Cannot get the information about the user %s. "
>+ "The process will be run as root\n", sssd_user);
BTW We should not fall back to root.
User might expect that service will be running as non-root
and due to typo it will fall back to root. They needn't notice it.
Okay, so aborting is the best thing to do?
I'm okay with that as well.
>+ uid = 0;
>+ gid = 0;
>+ } else {
>+ uid = pw->pw_uid;
>+ gid = pw->pw_gid;
>+ }
>+ }
>+
> ret = chown_debug_file(NULL, uid, gid);
>
>Okay, okay. It would work. Would it be okay for the rest of the developers?
>
//snip
>So, summing up the questions ...
>- Is there any problem on calling 'setenv("_SSS_LOOPS", "NO",
0)'
>earlier in the process?
>- Would be okay adding "--sssd-user" option for starting the responders?
This option is required just for nss responder due to circular dependency
with socket activation. Does it worth to add it to each service?
Because other services can be easily handled by systemd non-root solution
without any problem.
Yeah, they can, but then, IMO, it won't be consistent (considering
that we can have something a bt more consistent, I'd go for it).
>- Shall I get rid of the "--uid and --gid" options in order to only
>use the new added "--sssd-user" one?
>- Shall I add a new "--socket-activated" option for starting the
>responders or just abuse the "--sssd-user" one?
-1 for misusing other options
I completely agree here. So "--socket-activated" may be a good way to go.
.
If others developers prefer replacing --uid/gid with "--sssd-user"
then we would not be able to distinguish between socket activated
non-root and sssd monitor version of non-root.
Well. that's not true for two reasons:
- Currently we don't distinguish those and I don't see why we should.
By the code path taken in the monitor side it knows whether the
process is socket-activated or not and, actually, let the service know
whether it was socket-activated or not.
- Adding "---socket-activated" option would makes things simpler and
let me remove the patch where the monitor lets the responders know
whether they were socket-activated or not.
And we still need to
be able to handle current solution due backward compatibility.
Please, I don't completely understand here.
Which backward compatibility problem you see? Could you be a bit more verbose?
Best Regards,
--
Fabiano Fidêncio