[PATCH] Monitor and sbus changes for running SSSD as a non-privileged user

[Jakub Hrozek]

Hi,

Attached are patches that perform changes in the monitor process and the
low-level sbus and sysdb code required to run the NSS responder as a
non-privileged user. Some of the patches call chmod/chown on files owned
by the SSSD, so I'd like to request a very careful review.

The patches depend on patches from threads:
* SSSD: Add the options to specify a UID and GID to run as
* Packaging fixes related to non-root SSSD

I tried to explain the changes well in the commit messages. See below
for some more and especially questions.

[PATCH 1/7] SSSD: Load a user to run a service as from configuration
Pretty much just loads the sssd user from configuration and stores the
uid/gid. One question -- should the individual services or domain have a
option to ignore the sssd user? For instance:

[sssd]
user = sssd
group = sssd

[nss]
# Would run as SSSD

[pam]
# Would run as root/whatever user monitor runs as
ignore_sssd_user = True

This would mostly be a fallback for cases where we introduce a bug. I
don't think this kind of granularity is needed, the user can just
run sssd as root completely:

[sssd]
user = root
group = root

[PATCH 2/7] SBUS: Chown the sbus socket if needed
Chowns the monitor sbus socket if a non-root sssd user is specified. I
don't think we allow more privileges than before, the monitor socket has
permissions 0600 and is owned by sssd.sssd, so only the private user and
root (who bypasses DAC checks anyway) can access it.

In sbus_new_server(), we can open the socket after dbus creates it and
call fchmod() and fchown() to avoid some kind of TOCTOU races. But I'm
not sure this is needed given that a privileged process will create the
pipes.

[PATCH 3/7] SBUS: Allow connections from other UIDs
A simple patch, allows connections from the sssd user. Please see the
commit message for more details.

This patch (and the previous) should have a unit test, but currently our
sbus tests are tightly tied to the check framework. I need to change the
common code to be test framework-agnostic, but I don't want to delay the
inclusion of these patches.

[PATCH 4/7] BE: Own the sbus socket as the SSSD user
Same as the two previous patches, just for the sssd_be process.

[PATCH 5/7] MONITOR: Allow confdb to be accessed by nonroot user
Confdb is created by the privileged monitor process, but needs to be
accessed by nonprivileged responder processes. chown it after creation.

[PATCH 6/7] SYSDB: Allow calling chown on the sysdb file from monitor
Same as the previous one, just for the sysdb.

[PATCH 7/7] NSS: Run as a user specified by monitor
With this patch, it's possible to run the NSS responder as a
non-privileged user.

[Simo Sorce]

I am going to send a number of separate replies as I go and read about
the patches.

First on patch 01

On Wed, 15 Oct 2014 22:24:04 +0200
Jakub Hrozek  wrote:
> Adds two new options, user and group that are specified in the [sssd]
> section. When these options are specified, SSSD will run as the user
> and group. When these are not specified, SSSD will run as the
> configure-time user and group.

Do we really need to specify both a user and a group ?
In other projects specifying the user and using its primary group is
considered sufficient.
I think allowing to specify both can lead to potential issues if the
user is not member of the specified group.

Unless there is an actual need to specify the group explicitly I would
simplify and allow to specify only the user.

I can't seem to find where sss_user_from_string() is defined, is it in
a previous patch not yet committed to master ?
Why do we need this function when we can call directly getpwnam() ?

Simo.

[Jakub Hrozek]

On Wed, Oct 15, 2014 at 06:02:02PM -0400, Simo Sorce wrote:
> I am going to send a number of separate replies as I go and read about
> the patches.
>
> First on patch 01
>
> On Wed, 15 Oct 2014 22:24:04 +0200
> Jakub Hrozek  wrote:
> > Adds two new options, user and group that are specified in the [sssd]
> > section. When these options are specified, SSSD will run as the user
> > and group. When these are not specified, SSSD will run as the
> > configure-time user and group.
>
> Do we really need to specify both a user and a group ?
> In other projects specifying the user and using its primary group is
> considered sufficient.
> I think allowing to specify both can lead to potential issues if the
> user is not member of the specified group.
>
> Unless there is an actual need to specify the group explicitly I would
> simplify and allow to specify only the user.

Then I looked at different projects (nslcd, cockpit) :-)

I'm not against only specifying user, that would simplify the code a
bit. I'll change the patch accordingly.
> I can't seem to find where sss_user_from_string() is defined, is it in
> a previous patch not yet committed to master ?

Yes, sorry:
https://patchwork.acksyn.org/patch/8045/

Now, after stepping back from the code I realize the function is
misnamed as the input can be either a name ('sssd') or a UID in string
form ('123').

In short, the function tries getpwnam its input, then on failure tries
to getpwgid. I think allowing both name or ID is quite common.. Also,
this is how specifying users works in both the PAC and IFP responders and
I wanted to reuse the same code.
> Why do we need this function when we can call directly getpwnam() ?
>
> Simo.
>
> -- 
> Simo Sorce * Red Hat, Inc * New York

[Pavel Reichl]

On 10/16/2014 10:18 AM, Jakub Hrozek wrote:
> On Wed, Oct 15, 2014 at 06:02:02PM -0400, Simo Sorce wrote:
> > I am going to send a number of separate replies as I go and read about
> > the patches.
> >
> > First on patch 01
> >
> > On Wed, 15 Oct 2014 22:24:04 +0200
> > Jakub Hrozek  wrote:
> > > Adds two new options, user and group that are specified in the [sssd]
> > > section. When these options are specified, SSSD will run as the user
> > > and group. When these are not specified, SSSD will run as the
> > > configure-time user and group.
> >
> > Do we really need to specify both a user and a group ?
> > In other projects specifying the user and using its primary group is
> > considered sufficient.
> > I think allowing to specify both can lead to potential issues if the
> > user is not member of the specified group.
> >
> > Unless there is an actual need to specify the group explicitly I would
> > simplify and allow to specify only the user.
>
> Then I looked at different projects (nslcd, cockpit) :-)
>
> I'm not against only specifying user, that would simplify the code a
> bit. I'll change the patch accordingly.

Could you also fix 2 following nitpicks?
> +static int get_service_user(struct mt_ctx *ctx)
> +{
> errno_t ret;
> char *user_str;
> char *group_str;
> ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
>                        CONFDB_MONITOR_USER_RUNAS,
>                        NULL, &user_str);
> if (ret != EOK) {
>    DEBUG(SSSDBG_FATAL_FAILURE, "Failed to get the user to run as");

missing '\n'
>    return ret;
> }
> if (user_str != NULL) {
>    ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
>                            CONFDB_MONITOR_GROUP_RUNAS,
>                            user_str, &group_str);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_FATAL_FAILURE, "Failed to get the group to run as");

missing '\n'
>        return ret;
>    }
> } else {
>    user_str = SSSD_USER;
>    group_str = SSSD_GROUP;
> }
> ret = sss_user_from_string(user_str, &ctx->uid);
> talloc_free(user_str);
> if (ret != EOK) {
>    DEBUG(SSSDBG_FATAL_FAILURE, "Failed to set allowed UIDs.\n");
>    return ret;
> }
> ret = sss_group_from_string(group_str, &ctx->gid);
> talloc_free(group_str);
> if (ret != EOK) {
>    DEBUG(SSSDBG_FATAL_FAILURE, "Failed to set allowed UIDs.\n");
>    return ret;
> }
> return EOK;
>
> +}

Thanks!
> > I can't seem to find where sss_user_from_string() is defined, is it in
> > a previous patch not yet committed to master ?
>
> Yes, sorry:
> https://patchwork.acksyn.org/patch/8045/
>
> Now, after stepping back from the code I realize the function is
> misnamed as the input can be either a name ('sssd') or a UID in string
> form ('123').
>
> In short, the function tries getpwnam its input, then on failure tries
> to getpwgid. I think allowing both name or ID is quite common.. Also,
> this is how specifying users works in both the PAC and IFP responders and
> I wanted to reuse the same code.
> > Why do we need this function when we can call directly getpwnam() ?
> >
> > Simo.
> >
> > -- 
> > Simo Sorce * Red Hat, Inc * New York
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Simo Sorce]

On Wed, 15 Oct 2014 22:24:04 +0200
Jakub Hrozek  wrote:
> ret = check_file(filename,
>                 0, 0, S_IFSOCK|S_IRUSR|S_IWUSR, 0, NULL, true);
> /* check_file() uses -1 as a special value that means "no
>
> checks" */
> check_uid = (geteuid() == 0) ? (uid_t) -1 : geteuid();
> check_gid = (getegid() == 0) ? (gid_t) -1 : getegid();
> ret = check_file(filename, check_uid, check_gid,
>                 S_IFSOCK|S_IRUSR|S_IWUSR, 0, NULL, true);

It is difficult to read inlined ifs like this and in a critical
security case I would rather use some more real estate but make the
check crystal clear.

It would also be nice if the comment wouldn't state the obvious fact
that check_file() accepts -1 as a special value and rather explained
why you are using it to avoid checks.

I would do something like:

/* when running as root ignore file permission checks because .... */
check_uid = geteuid();
check_gid = getegid();
if (check_uid == 0) check_uid = -1;
if (check_gid == 0) check_gid = -1;

...

[Jakub Hrozek]

On Wed, Oct 15, 2014 at 06:12:42PM -0400, Simo Sorce wrote:
> On Wed, 15 Oct 2014 22:24:04 +0200
> Jakub Hrozek  wrote:
> > ret = check_file(filename,
> >                 0, 0, S_IFSOCK|S_IRUSR|S_IWUSR, 0, NULL, true);
> > /* check_file() uses -1 as a special value that means "no
> >
> > checks" */
> > check_uid = (geteuid() == 0) ? (uid_t) -1 : geteuid();
> > check_gid = (getegid() == 0) ? (gid_t) -1 : getegid();
> > ret = check_file(filename, check_uid, check_gid,
> >                 S_IFSOCK|S_IRUSR|S_IWUSR, 0, NULL, true);
>
> It is difficult to read inlined ifs like this and in a critical
> security case I would rather use some more real estate but make the
> check crystal clear.

OK, I'll change the patch.
> It would also be nice if the comment wouldn't state the obvious fact

Not sure I agree it's obvious that -1 means 'no checks', but fine..
> that check_file() accepts -1 as a special value and rather explained
> why you are using it to avoid checks.
>
> I would do something like:
>
> /* when running as root ignore file permission checks because .... */
> check_uid = geteuid();
> check_gid = getegid();
> if (check_uid == 0) check_uid = -1;
> if (check_gid == 0) check_gid = -1;

Thanks for the review!

[Simo Sorce]

On Wed, 15 Oct 2014 22:24:04 +0200
Jakub Hrozek  wrote:
> From c0385561ee5e9d050d2222aa43ebf46514f37dad Mon Sep 17 00:00:00 2001
> From: Michal Zidek 
> Date: Thu, 9 Oct 2014 17:15:56 +0200
> Subject: [PATCH 5/7] MONITOR: Allow confdb to be accessed by nonroot
> user
>
> src/monitor/monitor.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> index
> 44614be173325aa5b6f7ed03f00b6d4489ddf522..bd2c373008ef75ab46cf7dccdefd12468894f1ba
> 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c
> @@ -1718,7 +1718,6 @@ static errno_t load_configuration(TALLOC_CTX
> *mem_ctx, DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> confdb\n"); goto done;
>      }
>
> talloc_zfree(cdb_file);
>
> ret = confdb_init_db(config_file, ctx->cdb);
> if (ret != EOK) {
>
> @@ -1734,6 +1733,16 @@ static errno_t load_configuration(TALLOC_CTX
> *mem_ctx, goto done;
>      }
>
> /* Allow configuration database to be accessible
> * when SSSD runs as nonroot */
>
> ret = chown(cdb_file, ctx->uid, ctx->gid);
>
> if (ret != EOK) {
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "chown failed for [%s]: [%d][%s].\n",
>          cdb_file, ret, sss_strerror(ret));
>    goto done;
>
> }
>
> *monitor = ctx;
>
> ret = EOK;

I wonder if we shouldn't be more cautious here.
Do we need to give the sssd user write access ?
I think probably not, sounds like a great way to prevent "accidental"
changes would be to chown to (0, gid) and chmod so that the group can
only read, while root can read/write.

This way non-root process will be allowed to read but not change the
database.

Simo.

[Jakub Hrozek]

On Wed, Oct 15, 2014 at 06:17:55PM -0400, Simo Sorce wrote:
> On Wed, 15 Oct 2014 22:24:04 +0200
> Jakub Hrozek  wrote:
> > From c0385561ee5e9d050d2222aa43ebf46514f37dad Mon Sep 17 00:00:00 2001
> > From: Michal Zidek 
> > Date: Thu, 9 Oct 2014 17:15:56 +0200
> > Subject: [PATCH 5/7] MONITOR: Allow confdb to be accessed by nonroot
> > user
> >
> > src/monitor/monitor.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > index
> > 44614be173325aa5b6f7ed03f00b6d4489ddf522..bd2c373008ef75ab46cf7dccdefd12468894f1ba
> > 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c
> > @@ -1718,7 +1718,6 @@ static errno_t load_configuration(TALLOC_CTX
> > *mem_ctx, DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> > confdb\n"); goto done;
> >      }
> >
> > talloc_zfree(cdb_file);
> >
> > ret = confdb_init_db(config_file, ctx->cdb);
> > if (ret != EOK) {
> >
> > @@ -1734,6 +1733,16 @@ static errno_t load_configuration(TALLOC_CTX
> > *mem_ctx, goto done;
> >      }
> >
> > /* Allow configuration database to be accessible
> > * when SSSD runs as nonroot */
> >
> > ret = chown(cdb_file, ctx->uid, ctx->gid);
> >
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >          "chown failed for [%s]: [%d][%s].\n",
> >          cdb_file, ret, sss_strerror(ret));
> >    goto done;
> >
> > }
> >
> > *monitor = ctx;
> >
> > ret = EOK;
>
> I wonder if we shouldn't be more cautious here.
> Do we need to give the sssd user write access ?
> I think probably not, sounds like a great way to prevent "accidental"
> changes would be to chown to (0, gid) and chmod so that the group can
> only read, while root can read/write.
>
> This way non-root process will be allowed to read but not change the
> database.

But the sssd_be processes should also run as non-root if possible and it
needs to write to the db.

In general, the current plan is that sssd_be would start as root, do
whatever privileged initialization it needs and drop to sssd user, too.
Then, any operations that need to be done with different privileges will be
done either in setuid helper (ldap_child, krb5_child with validation, setting
the SELinux context) or in a helper running as the user on whose behalf
we are authenticating (krb5_child when keytab validation is not needed).

This way, only the monitor and the proxy_child will run privileged..

[Pavel Reichl]

On 10/16/2014 10:23 AM, Jakub Hrozek wrote:
> On Wed, Oct 15, 2014 at 06:17:55PM -0400, Simo Sorce wrote:
> > On Wed, 15 Oct 2014 22:24:04 +0200
> > Jakub Hrozek  wrote:
> > > From c0385561ee5e9d050d2222aa43ebf46514f37dad Mon Sep 17 00:00:00 2001
> > > From: Michal Zidek 
> > > Date: Thu, 9 Oct 2014 17:15:56 +0200
> > > Subject: [PATCH 5/7] MONITOR: Allow confdb to be accessed by nonroot
> > > user
> > >
> > > src/monitor/monitor.c | 11 ++++++++++-
> > >   1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > > index
> > > 44614be173325aa5b6f7ed03f00b6d4489ddf522..bd2c373008ef75ab46cf7dccdefd12468894f1ba
> > > 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c
> > > @@ -1718,7 +1718,6 @@ static errno_t load_configuration(TALLOC_CTX
> > > *mem_ctx, DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> > > confdb\n"); goto done;
> > >       }
> > >
> > > talloc_zfree(cdb_file);
> > >
> > > ret = confdb_init_db(config_file, ctx->cdb);
> > >  if (ret != EOK) {
> > >
> > > @@ -1734,6 +1733,16 @@ static errno_t load_configuration(TALLOC_CTX
> > > *mem_ctx, goto done;
> > >       }
> > > /* Allow configuration database to be accessible
> > > * when SSSD runs as nonroot */
> > > ret = chown(cdb_file, ctx->uid, ctx->gid);
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > >          "chown failed for [%s]: [%d][%s].\n",
> > >          cdb_file, ret, sss_strerror(ret));

errno should be used in debug message.
sss_strerror should be IMO replaced by strerror
ret should be set to some sensible value, as it is now -1.
> > >    goto done;
> > >
> > > }
> > > *monitor = ctx;
> > >
> > > ret = EOK;
> >
> > I wonder if we shouldn't be more cautious here.
> > Do we need to give the sssd user write access ?
> > I think probably not, sounds like a great way to prevent "accidental"
> > changes would be to chown to (0, gid) and chmod so that the group can
> > only read, while root can read/write.
> >
> > This way non-root process will be allowed to read but not change the
> > database.
>
> But the sssd_be processes should also run as non-root if possible and it
> needs to write to the db.
>
> In general, the current plan is that sssd_be would start as root, do
> whatever privileged initialization it needs and drop to sssd user, too.
> Then, any operations that need to be done with different privileges will be
> done either in setuid helper (ldap_child, krb5_child with validation, setting
> the SELinux context) or in a helper running as the user on whose behalf
> we are authenticating (krb5_child when keytab validation is not needed).
>
> This way, only the monitor and the proxy_child will run privileged..
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Michal Židek]

On 10/16/2014 11:01 AM, Pavel Reichl wrote:
> On 10/16/2014 10:23 AM, Jakub Hrozek wrote:
> > On Wed, Oct 15, 2014 at 06:17:55PM -0400, Simo Sorce wrote:
> > > On Wed, 15 Oct 2014 22:24:04 +0200
> > > Jakub Hrozek  wrote:
> > > > From c0385561ee5e9d050d2222aa43ebf46514f37dad Mon Sep 17 00:00:00 2001
> > > > From: Michal Zidek 
> > > > Date: Thu, 9 Oct 2014 17:15:56 +0200
> > > > Subject: [PATCH 5/7] MONITOR: Allow confdb to be accessed by nonroot
> > > > user
> > > >
> > > > src/monitor/monitor.c | 11 ++++++++++-
> > > >   1 file changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > > > index
> > > > 44614be173325aa5b6f7ed03f00b6d4489ddf522..bd2c373008ef75ab46cf7dccdefd12468894f1ba
> > > >
> > > > 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c
> > > > @@ -1718,7 +1718,6 @@ static errno_t load_configuration(TALLOC_CTX
> > > > *mem_ctx, DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> > > > confdb\n"); goto done;
> > > >       }
> > > > talloc_zfree(cdb_file);
> > > >  ret = confdb_init_db(config_file, ctx->cdb);
> > > >  if (ret != EOK) {
> > > >
> > > > @@ -1734,6 +1733,16 @@ static errno_t load_configuration(TALLOC_CTX
> > > > *mem_ctx, goto done;
> > > >       }
> > > > /* Allow configuration database to be accessible
> > > > * when SSSD runs as nonroot */
> > > > ret = chown(cdb_file, ctx->uid, ctx->gid);
> > > > if (ret != EOK) {
> > > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > > >          "chown failed for [%s]: [%d][%s].\n",
> > > >          cdb_file, ret, sss_strerror(ret));
>
> errno should be used in debug message.
> sss_strerror should be IMO replaced by strerror
> ret should be set to some sensible value, as it is now -1.

No. Please use sss_strerror where possible (not just where needed).

Michal

[Pavel Reichl]

On 10/16/2014 01:43 PM, Michal Židek wrote:
> On 10/16/2014 11:01 AM, Pavel Reichl wrote:
> > On 10/16/2014 10:23 AM, Jakub Hrozek wrote:
> > > On Wed, Oct 15, 2014 at 06:17:55PM -0400, Simo Sorce wrote:
> > > > On Wed, 15 Oct 2014 22:24:04 +0200
> > > > Jakub Hrozek  wrote:
> > > > > From c0385561ee5e9d050d2222aa43ebf46514f37dad Mon Sep 17 00:00:00 
> > > > > 2001
> > > > > From: Michal Zidek 
> > > > > Date: Thu, 9 Oct 2014 17:15:56 +0200
> > > > > Subject: [PATCH 5/7] MONITOR: Allow confdb to be accessed by nonroot
> > > > > user
> > > > >
> > > > > src/monitor/monitor.c | 11 ++++++++++-
> > > > >   1 file changed, 10 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > > > > index
> > > > > 44614be173325aa5b6f7ed03f00b6d4489ddf522..bd2c373008ef75ab46cf7dccdefd12468894f1ba
> > > > >
> > > > > 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c
> > > > > @@ -1718,7 +1718,6 @@ static errno_t load_configuration(TALLOC_CTX
> > > > > *mem_ctx, DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> > > > > confdb\n"); goto done;
> > > > >       }
> > > > > talloc_zfree(cdb_file);
> > > > >  ret = confdb_init_db(config_file, ctx->cdb);
> > > > >  if (ret != EOK) {
> > > > >
> > > > > @@ -1734,6 +1733,16 @@ static errno_t load_configuration(TALLOC_CTX
> > > > > *mem_ctx, goto done;
> > > > >       }
> > > > > /* Allow configuration database to be accessible
> > > > > * when SSSD runs as nonroot */
> > > > > ret = chown(cdb_file, ctx->uid, ctx->gid);
> > > > > if (ret != EOK) {
> > > > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > > > >          "chown failed for [%s]: [%d][%s].\n",
> > > > >          cdb_file, ret, sss_strerror(ret));
> >
> > errno should be used in debug message.
> > sss_strerror should be IMO replaced by strerror
> > ret should be set to some sensible value, as it is now -1.
>
> No. Please use sss_strerror where possible (not just where needed).

Sorry, you are right, but I believe that the rest of my previous comment 
is valid, right?
> Michal
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Michal Židek]

On 10/16/2014 01:49 PM, Pavel Reichl wrote:
> On 10/16/2014 01:43 PM, Michal Židek wrote:
> > On 10/16/2014 11:01 AM, Pavel Reichl wrote:
> > > On 10/16/2014 10:23 AM, Jakub Hrozek wrote:
> > > > On Wed, Oct 15, 2014 at 06:17:55PM -0400, Simo Sorce wrote:
> > > > > On Wed, 15 Oct 2014 22:24:04 +0200
> > > > > Jakub Hrozek  wrote:
> > > > > > From c0385561ee5e9d050d2222aa43ebf46514f37dad Mon Sep 17 00:00:00
> > > > > > 2001
> > > > > > From: Michal Zidek 
> > > > > > Date: Thu, 9 Oct 2014 17:15:56 +0200
> > > > > > Subject: [PATCH 5/7] MONITOR: Allow confdb to be accessed by nonroot
> > > > > > user
> > > > > >
> > > > > > src/monitor/monitor.c | 11 ++++++++++-
> > > > > >   1 file changed, 10 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > > > > > index
> > > > > > 44614be173325aa5b6f7ed03f00b6d4489ddf522..bd2c373008ef75ab46cf7dccdefd12468894f1ba
> > > > > >
> > > > > > 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c
> > > > > > @@ -1718,7 +1718,6 @@ static errno_t load_configuration(TALLOC_CTX
> > > > > > *mem_ctx, DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> > > > > > confdb\n"); goto done;
> > > > > >       }
> > > > > > talloc_zfree(cdb_file);
> > > > > >  ret = confdb_init_db(config_file, ctx->cdb);
> > > > > >  if (ret != EOK) {
> > > > > >
> > > > > > @@ -1734,6 +1733,16 @@ static errno_t load_configuration(TALLOC_CTX
> > > > > > *mem_ctx, goto done;
> > > > > >       }
> > > > > > /* Allow configuration database to be accessible
> > > > > > * when SSSD runs as nonroot */
> > > > > > ret = chown(cdb_file, ctx->uid, ctx->gid);
> > > > > > if (ret != EOK) {
> > > > > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > > > > >          "chown failed for [%s]: [%d][%s].\n",
> > > > > >          cdb_file, ret, sss_strerror(ret));
> > >
> > > errno should be used in debug message.
> > > sss_strerror should be IMO replaced by strerror
> > > ret should be set to some sensible value, as it is now -1.
> >
> > No. Please use sss_strerror where possible (not just where needed).
>
> Sorry, you are right, but I believe that the rest of my previous comment
> is valid, right?

Sure, I just reacted to the one thing I disagreed with :)

[Simo Sorce]

On Thu, 16 Oct 2014 10:23:35 +0200
Jakub Hrozek  wrote:
> On Wed, Oct 15, 2014 at 06:17:55PM -0400, Simo Sorce wrote:
> > On Wed, 15 Oct 2014 22:24:04 +0200
> > Jakub Hrozek  wrote:
> > > From c0385561ee5e9d050d2222aa43ebf46514f37dad Mon Sep 17 00:00:00
> > > 2001 From: Michal Zidek 
> > > Date: Thu, 9 Oct 2014 17:15:56 +0200
> > > Subject: [PATCH 5/7] MONITOR: Allow confdb to be accessed by
> > > nonroot user
> > >
> > > src/monitor/monitor.c | 11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > > index
> > > 44614be173325aa5b6f7ed03f00b6d4489ddf522..bd2c373008ef75ab46cf7dccdefd12468894f1ba
> > > 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c
> > > @@ -1718,7 +1718,6 @@ static errno_t load_configuration(TALLOC_CTX
> > > *mem_ctx, DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> > > confdb\n"); goto done;
> > >      }
> > >
> > > talloc_zfree(cdb_file);
> > >
> > > ret = confdb_init_db(config_file, ctx->cdb);
> > > if (ret != EOK) {
> > >
> > > @@ -1734,6 +1733,16 @@ static errno_t
> > > load_configuration(TALLOC_CTX *mem_ctx, goto done;
> > >      }
> > >
> > > /* Allow configuration database to be accessible
> > > * when SSSD runs as nonroot */
> > >
> > > ret = chown(cdb_file, ctx->uid, ctx->gid);
> > >
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > >          "chown failed for [%s]: [%d][%s].\n",
> > >          cdb_file, ret, sss_strerror(ret));
> > >    goto done;
> > >
> > > }
> > >
> > > *monitor = ctx;
> > >
> > > ret = EOK;
> >
> > I wonder if we shouldn't be more cautious here.
> > Do we need to give the sssd user write access ?
> > I think probably not, sounds like a great way to prevent
> > "accidental" changes would be to chown to (0, gid) and chmod so
> > that the group can only read, while root can read/write.
> >
> > This way non-root process will be allowed to read but not change the
> > database.
>
> But the sssd_be processes should also run as non-root if possible and
> it needs to write to the db.

To the configuration db ?
Why ?
> In general, the current plan is that sssd_be would start as root, do
> whatever privileged initialization it needs and drop to sssd user,
> too. Then, any operations that need to be done with different
> privileges will be done either in setuid helper (ldap_child,
> krb5_child with validation, setting the SELinux context) or in a
> helper running as the user on whose behalf we are authenticating
> (krb5_child when keytab validation is not needed).
>
> This way, only the monitor and the proxy_child will run privileged..

Yes, and the children usually do not read the confdb at all as they get
their parameters from the invoking process.

In what case does anything need to write to the confdb after the file
has been generated by the monitor ?

Simo.

[Jakub Hrozek]

On Thu, Oct 16, 2014 at 09:05:24AM -0400, Simo Sorce wrote:
> On Thu, 16 Oct 2014 10:23:35 +0200
> Jakub Hrozek  wrote:
> > On Wed, Oct 15, 2014 at 06:17:55PM -0400, Simo Sorce wrote:
> > > On Wed, 15 Oct 2014 22:24:04 +0200
> > > Jakub Hrozek  wrote:
> > > > From c0385561ee5e9d050d2222aa43ebf46514f37dad Mon Sep 17 00:00:00
> > > > 2001 From: Michal Zidek 
> > > > Date: Thu, 9 Oct 2014 17:15:56 +0200
> > > > Subject: [PATCH 5/7] MONITOR: Allow confdb to be accessed by
> > > > nonroot user
> > > >
> > > > src/monitor/monitor.c | 11 ++++++++++-
> > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > > > index
> > > > 44614be173325aa5b6f7ed03f00b6d4489ddf522..bd2c373008ef75ab46cf7dccdefd12468894f1ba
> > > > 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c
> > > > @@ -1718,7 +1718,6 @@ static errno_t load_configuration(TALLOC_CTX
> > > > *mem_ctx, DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> > > > confdb\n"); goto done;
> > > >      }
> > > >
> > > > talloc_zfree(cdb_file);
> > > >
> > > > ret = confdb_init_db(config_file, ctx->cdb);
> > > > if (ret != EOK) {
> > > >
> > > > @@ -1734,6 +1733,16 @@ static errno_t
> > > > load_configuration(TALLOC_CTX *mem_ctx, goto done;
> > > >      }
> > > >
> > > > /* Allow configuration database to be accessible
> > > > * when SSSD runs as nonroot */
> > > >
> > > > ret = chown(cdb_file, ctx->uid, ctx->gid);
> > > >
> > > > if (ret != EOK) {
> > > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > > >          "chown failed for [%s]: [%d][%s].\n",
> > > >          cdb_file, ret, sss_strerror(ret));
> > > >    goto done;
> > > >
> > > > }
> > > >
> > > > *monitor = ctx;
> > > >
> > > > ret = EOK;
> > >
> > > I wonder if we shouldn't be more cautious here.
> > > Do we need to give the sssd user write access ?
> > > I think probably not, sounds like a great way to prevent
> > > "accidental" changes would be to chown to (0, gid) and chmod so
> > > that the group can only read, while root can read/write.
> > >
> > > This way non-root process will be allowed to read but not change the
> > > database.
> >
> > But the sssd_be processes should also run as non-root if possible and
> > it needs to write to the db.
>
> To the configuration db ?
> Why ?

Aah, this is confdb.

Sorry, I was thinking sysdb. Then you are completely right.
> > In general, the current plan is that sssd_be would start as root, do
> > whatever privileged initialization it needs and drop to sssd user,
> > too. Then, any operations that need to be done with different
> > privileges will be done either in setuid helper (ldap_child,
> > krb5_child with validation, setting the SELinux context) or in a
> > helper running as the user on whose behalf we are authenticating
> > (krb5_child when keytab validation is not needed).
> >
> > This way, only the monitor and the proxy_child will run privileged..
>
> Yes, and the children usually do not read the confdb at all as they get
> their parameters from the invoking process.
>
> In what case does anything need to write to the confdb after the file
> has been generated by the monitor ?

None, I confused sysdb and confdb. Sorry about that.

When on the topic of setuid child processes, we currently pass quite a
lot of info through environment variables. Even though the variables are
private and namespaced (SSS_DEFAULT_REALM for example), I still think
we should get rid of the variables and either use a parameter
(--default-realm) or just pass the info through the pipe..

[Simo Sorce]

On Thu, 16 Oct 2014 16:24:36 +0200
Jakub Hrozek  wrote:
> When on the topic of setuid child processes, we currently pass quite a
> lot of info through environment variables. Even though the variables
> are private and namespaced (SSS_DEFAULT_REALM for example), I still
> think we should get rid of the variables and either use a parameter
> (--default-realm) or just pass the info through the pipe..

Yes and we should use secure_getent() throughout the code or we risk
opening up attacks against setuid binaries.

Simo.

[Michal Židek]

On 10/16/2014 03:05 PM, Simo Sorce wrote:
> On Thu, 16 Oct 2014 10:23:35 +0200
> Jakub Hrozek  wrote:
> > On Wed, Oct 15, 2014 at 06:17:55PM -0400, Simo Sorce wrote:
> > > On Wed, 15 Oct 2014 22:24:04 +0200
> > > Jakub Hrozek  wrote:
> > > > From c0385561ee5e9d050d2222aa43ebf46514f37dad Mon Sep 17 00:00:00
> > > > 2001 From: Michal Zidek 
> > > > Date: Thu, 9 Oct 2014 17:15:56 +0200
> > > > Subject: [PATCH 5/7] MONITOR: Allow confdb to be accessed by
> > > > nonroot user
> > > >
> > > > src/monitor/monitor.c | 11 ++++++++++-
> > > >   1 file changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > > > index
> > > > 44614be173325aa5b6f7ed03f00b6d4489ddf522..bd2c373008ef75ab46cf7dccdefd12468894f1ba
> > > > 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c
> > > > @@ -1718,7 +1718,6 @@ static errno_t load_configuration(TALLOC_CTX
> > > > *mem_ctx, DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> > > > confdb\n"); goto done;
> > > >       }
> > > >
> > > > talloc_zfree(cdb_file);
> > > >
> > > > ret = confdb_init_db(config_file, ctx->cdb);
> > > >  if (ret != EOK) {
> > > >
> > > > @@ -1734,6 +1733,16 @@ static errno_t
> > > > load_configuration(TALLOC_CTX *mem_ctx, goto done;
> > > >       }
> > > >
> > > > /* Allow configuration database to be accessible
> > > > * when SSSD runs as nonroot */
> > > >
> > > > ret = chown(cdb_file, ctx->uid, ctx->gid);
> > > >
> > > > if (ret != EOK) {
> > > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > > >          "chown failed for [%s]: [%d][%s].\n",
> > > >          cdb_file, ret, sss_strerror(ret));
> > > >    goto done;
> > > >
> > > > }
> > > > *monitor = ctx;
> > > >
> > > > ret = EOK;
> > >
> > > I wonder if we shouldn't be more cautious here.
> > > Do we need to give the sssd user write access ?
> > > I think probably not, sounds like a great way to prevent
> > > "accidental" changes would be to chown to (0, gid) and chmod so
> > > that the group can only read, while root can read/write.
> > >
> > > This way non-root process will be allowed to read but not change the
> > > database.
> >
> > But the sssd_be processes should also run as non-root if possible and
> > it needs to write to the db.
>
> To the configuration db ?
> Why ?
> > In general, the current plan is that sssd_be would start as root, do
> > whatever privileged initialization it needs and drop to sssd user,
> > too. Then, any operations that need to be done with different
> > privileges will be done either in setuid helper (ldap_child,
> > krb5_child with validation, setting the SELinux context) or in a
> > helper running as the user on whose behalf we are authenticating
> > (krb5_child when keytab validation is not needed).
> >
> > This way, only the monitor and the proxy_child will run privileged..
>
> Yes, and the children usually do not read the confdb at all as they get
> their parameters from the invoking process.
>
> In what case does anything need to write to the confdb after the file
> has been generated by the monitor ?
>
> Simo.

Hi,

I just tried to set new rights for confdb so that root can read/write
and responders (user sssd) can read. The solution was this:
file owned by user: root, group: sssd. File permissions 0640.

But than ldb failed to connect. I guess the problem was that I
did not pass LDB_FLAGS_READONLY to ldb_connect() call. So I
added new parameter to server_setup() and confdb_init(), so that
in responders I could init confdb with this parameter (not in monitor).

But ldb_connect() still fails. I am not sure what the problem is.
In logs I can see this:

(Thu Oct 16 20:23:38:525086 2014) [sssd[pam]] [ldb] (0x0010): ldb error 
(ldb_wait: Operations error (1)) occurred searching for modules, bailing out
(Thu Oct 16 20:23:38:525163 2014) [sssd[pam]] [ldb] (0x0010): Unable to 
load modules for /var/lib/sss/db/config.ldb: ldb_wait: Operations error (1)
(Thu Oct 16 20:23:38:525191 2014) [sssd[pam]] [confdb_init] (0x0010): 
Unable to open config database [/var/lib/sss/db/config.ldb]
(Thu Oct 16 20:23:38:525252 2014) [sssd[pam]] [server_setup] (0x0010): 
The confdb initialization failed

I am not really sure what the problem is right now. Would it
be OK, to have the confdb readable and writeable by responders
for now, and file a ticket to fix this later?

Michal

[Jakub Hrozek]

On Thu, Oct 16, 2014 at 08:39:16PM +0200, Michal Židek wrote:
> I am not really sure what the problem is right now. Would it
> be OK, to have the confdb readable and writeable by responders
> for now, and file a ticket to fix this later?

Fine by me, please file the ticket. In the first release(s), the
non-privileged services will be opt-in anyway, exactly to polish stuff
like this.

[Simo Sorce]

On Wed, 15 Oct 2014 22:24:04 +0200
Jakub Hrozek  wrote:
> Attached are patches that perform changes in the monitor process and
> the low-level sbus and sysdb code required to run the NSS responder
> as a non-privileged user. Some of the patches call chmod/chown on
> files owned by the SSSD, so I'd like to request a very careful review.

Aside from the points raised in the emails already sent the rest looks
good to me.

Simo.

[Jakub Hrozek]

On Wed, Oct 15, 2014 at 06:19:49PM -0400, Simo Sorce wrote:
> On Wed, 15 Oct 2014 22:24:04 +0200
> Jakub Hrozek  wrote:
> > Attached are patches that perform changes in the monitor process and
> > the low-level sbus and sysdb code required to run the NSS responder
> > as a non-privileged user. Some of the patches call chmod/chown on
> > files owned by the SSSD, so I'd like to request a very careful review.
>
> Aside from the points raised in the emails already sent the rest looks
> good to me.

Thank you very much for the review. I'll send out updated patches.

[Jakub Hrozek]

On Thu, Oct 16, 2014 at 10:25:12AM +0200, Jakub Hrozek wrote:
> On Wed, Oct 15, 2014 at 06:19:49PM -0400, Simo Sorce wrote:
> > On Wed, 15 Oct 2014 22:24:04 +0200
> > Jakub Hrozek  wrote:
> > > Attached are patches that perform changes in the monitor process and
> > > the low-level sbus and sysdb code required to run the NSS responder
> > > as a non-privileged user. Some of the patches call chmod/chown on
> > > files owned by the SSSD, so I'd like to request a very careful review.
> >
> > Aside from the points raised in the emails already sent the rest looks
> > good to me.
>
> Thank you very much for the review. I'll send out updated patches.

Attached are patches that implement the corrections Simo and Pavel asked
for with the exception of confdb being read-only for sssd processes,
Michal is still investigating that one.

I also merged some more Michal's patches that help spawn the PAM
privileged pipe before dropping privileges and also dropping privileges
in the other responders.

With this patchset, all responders are able to run as the SSSD user.

I have one question:
is it wise to also set the permissions of directories we create when
we "install -d" them? Or is this something typically done by the
downstream? Previously, we would just rely on the default system umask
when running "make install", maybe we should tighten the permissions in
Makefile.am as part of this effort?

And one comment:
One of Michal's patches moves creating the socket into a separate
function. This function also changes the umask, which I don't think
belongs into a utility function. I realize this is how the code worked
even before Michal's change, but I think it would be better to move the
umask setting into function like sss_process_init().

Thanks again for the review.

[Simo Sorce]

On Fri, 17 Oct 2014 18:47:28 +0200
Jakub Hrozek  wrote:
> On Thu, Oct 16, 2014 at 10:25:12AM +0200, Jakub Hrozek wrote:
> > On Wed, Oct 15, 2014 at 06:19:49PM -0400, Simo Sorce wrote:
> > > On Wed, 15 Oct 2014 22:24:04 +0200
> > > Jakub Hrozek  wrote:
> > > > Attached are patches that perform changes in the monitor
> > > > process and the low-level sbus and sysdb code required to run
> > > > the NSS responder as a non-privileged user. Some of the patches
> > > > call chmod/chown on files owned by the SSSD, so I'd like to
> > > > request a very careful review.
> > >
> > > Aside from the points raised in the emails already sent the rest
> > > looks good to me.
> >
> > Thank you very much for the review. I'll send out updated patches.
>
> Attached are patches that implement the corrections Simo and Pavel
> asked for with the exception of confdb being read-only for sssd
> processes, Michal is still investigating that one.
>
> I also merged some more Michal's patches that help spawn the PAM
> privileged pipe before dropping privileges and also dropping
> privileges in the other responders.
>
> With this patchset, all responders are able to run as the SSSD user.

Great!
> I have one question:
> is it wise to also set the permissions of directories we create when
> we "install -d" them? Or is this something typically done by the
> downstream? Previously, we would just rely on the default system umask
> when running "make install", maybe we should tighten the permissions
> in Makefile.am as part of this effort?

+1
> And one comment:
> One of Michal's patches moves creating the socket into a separate
> function. This function also changes the umask, which I don't think
> belongs into a utility function. I realize this is how the code worked
> even before Michal's change, but I think it would be better to move
> the umask setting into function like sss_process_init().

+1
> Thanks again for the review.

FYI the commit message for patch 2 still mentions the group option you
just removed :-)

Same for patch 4

The rest looks good.

Simo.

[Jakub Hrozek]

On Fri, Oct 17, 2014 at 01:29:42PM -0400, Simo Sorce wrote:
> On Fri, 17 Oct 2014 18:47:28 +0200
> Jakub Hrozek  wrote:
> > On Thu, Oct 16, 2014 at 10:25:12AM +0200, Jakub Hrozek wrote:
> > > On Wed, Oct 15, 2014 at 06:19:49PM -0400, Simo Sorce wrote:
> > > > On Wed, 15 Oct 2014 22:24:04 +0200
> > > > Jakub Hrozek  wrote:
> > > > > Attached are patches that perform changes in the monitor
> > > > > process and the low-level sbus and sysdb code required to run
> > > > > the NSS responder as a non-privileged user. Some of the patches
> > > > > call chmod/chown on files owned by the SSSD, so I'd like to
> > > > > request a very careful review.
> > > >
> > > > Aside from the points raised in the emails already sent the rest
> > > > looks good to me.
> > >
> > > Thank you very much for the review. I'll send out updated patches.
> >
> > Attached are patches that implement the corrections Simo and Pavel
> > asked for with the exception of confdb being read-only for sssd
> > processes, Michal is still investigating that one.
> >
> > I also merged some more Michal's patches that help spawn the PAM
> > privileged pipe before dropping privileges and also dropping
> > privileges in the other responders.
> >
> > With this patchset, all responders are able to run as the SSSD user.
>
> Great!

Well, I lied. InfoPipe still runs as root, but that effort is tracked
separately with:
    https://fedorahosted.org/sssd/ticket/2395
> > I have one question:
> > is it wise to also set the permissions of directories we create when
> > we "install -d" them? Or is this something typically done by the
> > downstream? Previously, we would just rely on the default system umask
> > when running "make install", maybe we should tighten the permissions
> > in Makefile.am as part of this effort?
>
> +1

I filed:
    https://fedorahosted.org/sssd/ticket/2467
> > And one comment:
> > One of Michal's patches moves creating the socket into a separate
> > function. This function also changes the umask, which I don't think
> > belongs into a utility function. I realize this is how the code worked
> > even before Michal's change, but I think it would be better to move
> > the umask setting into function like sss_process_init().
>
> +1

And here:
    https://fedorahosted.org/sssd/ticket/2468

Because both issues were already present in the code, just discovered
when changing the SSSD to run unprivileged.
> > Thanks again for the review.
>
> FYI the commit message for patch 2 still mentions the group option you
> just removed :-)
>
> Same for patch 4
>
> The rest looks good.

I found some other minor issues when I actually made the leap myself and
started running rootless SSSD on my laptop. I will fix those along with
your and Pavel's comments and re-send.

[Pavel Reichl]

On 10/17/2014 06:47 PM, Jakub Hrozek wrote:
> On Thu, Oct 16, 2014 at 10:25:12AM +0200, Jakub Hrozek wrote:
> > On Wed, Oct 15, 2014 at 06:19:49PM -0400, Simo Sorce wrote:
> > > On Wed, 15 Oct 2014 22:24:04 +0200
> > > Jakub Hrozek  wrote:
> > > > Attached are patches that perform changes in the monitor process and
> > > > the low-level sbus and sysdb code required to run the NSS responder
> > > > as a non-privileged user. Some of the patches call chmod/chown on
> > > > files owned by the SSSD, so I'd like to request a very careful review.
> > >
> > > Aside from the points raised in the emails already sent the rest looks
> > > good to me.
> >
> > Thank you very much for the review. I'll send out updated patches.
>
> Attached are patches that implement the corrections Simo and Pavel asked
> for with the exception of confdb being read-only for sssd processes,
> Michal is still investigating that one.
>
> I also merged some more Michal's patches that help spawn the PAM
> privileged pipe before dropping privileges and also dropping privileges
> in the other responders.
>
> With this patchset, all responders are able to run as the SSSD user.
>
> I have one question:
> is it wise to also set the permissions of directories we create when
> we "install -d" them? Or is this something typically done by the
> downstream? Previously, we would just rely on the default system umask
> when running "make install", maybe we should tighten the permissions in
> Makefile.am as part of this effort?
>
> And one comment:
> One of Michal's patches moves creating the socket into a separate
> function. This function also changes the umask, which I don't think
> belongs into a utility function. I realize this is how the code worked
> even before Michal's change, but I think it would be better to move the
> umask setting into function like sss_process_init().
>
> Thanks again for the review.

Sorry, but I can't apply these patches on top of the master.
I think you have to move the first patch.

git am patch/0001-UTIL-Add-a-function-to-convert-id_t-from-a-number-or.patch

Applying: UTIL: Add a function to convert id_t from a number or a name
error: patch failed: src/tests/cwrap/Makefile.am:45
error: src/tests/cwrap/Makefile.am: patch does not apply
Patch failed at 0001 UTIL: Add a function to convert id_t from a number 
or a name
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Mon, Oct 20, 2014 at 10:44:48AM +0200, Pavel Reichl wrote:
> On 10/17/2014 06:47 PM, Jakub Hrozek wrote:
> > On Thu, Oct 16, 2014 at 10:25:12AM +0200, Jakub Hrozek wrote:
> > > On Wed, Oct 15, 2014 at 06:19:49PM -0400, Simo Sorce wrote:
> > > > On Wed, 15 Oct 2014 22:24:04 +0200
> > > > Jakub Hrozek  wrote:
> > > > > Attached are patches that perform changes in the monitor process and
> > > > > the low-level sbus and sysdb code required to run the NSS responder
> > > > > as a non-privileged user. Some of the patches call chmod/chown on
> > > > > files owned by the SSSD, so I'd like to request a very careful review.
> > > >
> > > > Aside from the points raised in the emails already sent the rest looks
> > > > good to me.
> > >
> > > Thank you very much for the review. I'll send out updated patches.
> >
> > Attached are patches that implement the corrections Simo and Pavel asked
> > for with the exception of confdb being read-only for sssd processes,
> > Michal is still investigating that one.
> >
> > I also merged some more Michal's patches that help spawn the PAM
> > privileged pipe before dropping privileges and also dropping privileges
> > in the other responders.
> >
> > With this patchset, all responders are able to run as the SSSD user.
> >
> > I have one question:
> > is it wise to also set the permissions of directories we create when
> > we "install -d" them? Or is this something typically done by the
> > downstream? Previously, we would just rely on the default system umask
> > when running "make install", maybe we should tighten the permissions in
> > Makefile.am as part of this effort?
> >
> > And one comment:
> > One of Michal's patches moves creating the socket into a separate
> > function. This function also changes the umask, which I don't think
> > belongs into a utility function. I realize this is how the code worked
> > even before Michal's change, but I think it would be better to move the
> > umask setting into function like sss_process_init().
> >
> > Thanks again for the review.
>
> Sorry, but I can't apply these patches on top of the master.
> I think you have to move the first patch.
>
> git am patch/0001-UTIL-Add-a-function-to-convert-id_t-from-a-number-or.patch
>
> Applying: UTIL: Add a function to convert id_t from a number or a name
> error: patch failed: src/tests/cwrap/Makefile.am:45
> error: src/tests/cwrap/Makefile.am: patch does not apply
> Patch failed at 0001 UTIL: Add a function to convert id_t from a number or a
> name

You also need to apply the patches from the thread "[PATCH] SSSD: Add
the options to specify a UID and GID to run as" shall I also include
them here?

btw my whole branch is here (the top patches are not 100% tested yet,
although it works in general..)
    https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=nonroot

[Pavel Reichl]

On 10/17/2014 06:47 PM, Jakub Hrozek wrote:
> On Thu, Oct 16, 2014 at 10:25:12AM +0200, Jakub Hrozek wrote:
> > > On Wed, Oct 15, 2014 at 06:19:49PM -0400, Simo Sorce wrote:
> > > > > On Wed, 15 Oct 2014 22:24:04 +0200
> > > > > Jakub Hrozek  wrote:
> > > > > > >Attached are patches that perform changes in the monitor process and
> > > > > > >the low-level sbus and sysdb code required to run the NSS responder
> > > > > > >as a non-privileged user. Some of the patches call chmod/chown on
> > > > > > >files owned by the SSSD, so I'd like to request a very careful review.
> > > > >
> > > > > Aside from the points raised in the emails already sent the rest looks
> > > > > good to me.
> > >
> > > Thank you very much for the review. I'll send out updated patches.
>
> Attached are patches that implement the corrections Simo and Pavel asked
> for with the exception of confdb being read-only for sssd processes,
> Michal is still investigating that one.
>
> I also merged some more Michal's patches that help spawn the PAM
> privileged pipe before dropping privileges and also dropping privileges
> in the other responders.
>
> With this patchset, all responders are able to run as the SSSD user.
>
> I have one question:
> is it wise to also set the permissions of directories we create when
> we "install -d" them? Or is this something typically done by the
> downstream? Previously, we would just rely on the default system umask
> when running "make install", maybe we should tighten the permissions in
> Makefile.am as part of this effort?
>
> And one comment:
> One of Michal's patches moves creating the socket into a separate
> function. This function also changes the umask, which I don't think
> belongs into a utility function. I realize this is how the code worked
> even before Michal's change, but I think it would be better to move the
> umask setting into function like sss_process_init().
>
> Thanks again for the review.
>
> 0001-UTIL-Add-a-function-to-convert-id_t-from-a-number-or.patch

[snip]
> +++ b/src/tests/cwrap/test_usertools.c
> @@ -0,0 +1,101 @@
> +/*
> Authors:
>    Jakub Hrozek<[e-mail]>
> Copyright (C) 2014 Red Hat
> SSSD tests: User utilities
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 3 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program.  If not, seehttp://www.gnu.org/licenses/.
>
> +*/
>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
>
> +#include <popt.h>
> +#include "util/util.h"
> +#include "tests/cmocka/common_mock.h"
>
> +void test_get_user_num(void **state)
> +{
> uid_t uid;
> gid_t gid;
> errno_t ret;
> ret = sss_user_by_name_or_uid("123", &uid, &gid);
> assert_int_equal(ret, EOK);
> assert_int_equal(uid, 123);
> assert_int_equal(gid, 123);
>
> +}
>
> +void test_get_user_str(void **state)
> +{
> uid_t uid;
> gid_t gid;
> errno_t ret;
> ret = sss_user_by_name_or_uid("sssd", &uid, &gid);
> assert_int_equal(ret, EOK);
> assert_int_equal(uid, 123);
> assert_int_equal(gid, 456);
>
> +}

I wasn't able to run the tests now, but why is gid supposed to 456 and 
in prev test 123?
> +void test_get_user_nullparm(void **state)
> +{
> uid_t uid;
> errno_t ret;
> ret = sss_user_by_name_or_uid("sssd", &uid, NULL);
> assert_int_equal(ret, EOK);
> assert_int_equal(uid, 123);
>
> +}
>
> +int main(int argc, const char *argv[])
> +{
> poptContext pc;
> int opt;
> struct poptOption long_options[] = {
>    POPT_AUTOHELP
>    SSSD_DEBUG_OPTS
>    POPT_TABLEEND
> };
> const UnitTest tests[] = {
>    unit_test(test_get_user_num),
>    unit_test(test_get_user_str),
>    unit_test(test_get_user_nullparm),
> };
> /* Set debug level to invalid value so we can deside if -d 0 was used. */
> debug_level = SSSDBG_INVALID;
> pc = poptGetContext(argv[0], argc, argv, long_options, 0);
> while((opt = poptGetNextOpt(pc)) != -1) {
>    switch(opt) {
>    default:
>        fprintf(stderr, "\nInvalid option %s: %s\n\n",
>                poptBadOption(pc, 0), poptStrerror(opt));
>        poptPrintUsage(pc, stderr, 0);
>        return 1;
>    }
> }
> poptFreeContext(pc);
> DEBUG_CLI_INIT(debug_level);
> tests_set_cwd();
> return run_tests(tests);
>
> +}
>
> 0004-SSSD-Load-a-user-to-run-a-service-as-from-configurat.patch
>
> From 78066ed7cd43e0208f6d15b8b0489e3699b1b4cc Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek
> Date: Tue, 5 Aug 2014 13:52:48 +0200
> Subject: [PATCH 04/19] SSSD: Load a user to run a service as from
>   configuration
>
> Adds two new options, user and group that are specified in the [sssd]
> section. When these options are specified, SSSD will run as the user and
> group. When these are not specified, SSSD will run as the configure-time
> user and group.
>
> The group option only takes effect when user is also specified.
>
> Currently all services and providers are started as root. There is a
> temporary svc_supported_as_nonroot() function that returns true for a
> service if that service runs and was tested as nonroot and false
> otherwise. Currently this function always returns false, but will be
> amended in future patches.

[snip]
> +static int get_service_user(struct mt_ctx *ctx)
> +{
> errno_t ret;
> char *user_str;
> ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
>                        CONFDB_MONITOR_USER_RUNAS,
>                        NULL, &user_str);
> if (ret != EOK) {
>    DEBUG(SSSDBG_FATAL_FAILURE, "Failed to get the user to run as");

'\n'
>    return ret;
> }
> ret = sss_user_by_name_or_uid(user_str, &ctx->uid, &ctx->gid);
> talloc_free(user_str);
> if (ret != EOK) {
>    DEBUG(SSSDBG_FATAL_FAILURE, "Failed to set allowed UIDs.\n");
>    return ret;
> }
> return EOK;
>
> +}
>
> 0011-responder_common-Create-fd-for-pipe-in-helper.patch
>
> From 1a2d3bf47c0da1bb32be138fa8b2b33260f68a37 Mon Sep 17 00:00:00 2001
> From: Michal Zidek
> Date: Wed, 15 Oct 2014 17:35:12 +0200
> Subject: [PATCH 11/19] responder_common: Create fd for pipe in helper
>
> Move creating of file descriptor for pipes into
> helper function and make this function public.
>
> src/responder/common/responder.h        |   2 +
>   src/responder/common/responder_common.c | 129 ++++++++++++++------------------
>   2 files changed, 59 insertions(+), 72 deletions(-)
>
> diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
> index 97552ec472c5baa285b41cc48b51149f3ef6adb5..d233710782fe7df1bbcc338e3815d1701557519e 100644
> --- a/src/responder/common/responder.h
> +++ b/src/responder/common/responder.h
> @@ -176,6 +176,8 @@ responder_get_domain(struct resp_ctx *rctx, const char *domain);
>   errno_t responder_get_domain_by_id(struct resp_ctx *rctx, const char *id,
>                                      struct sss_domain_info **_ret_dom);
>
> +int create_pipe_fd(const char *sock_name, int *fd, mode_t umaskval);
> /* responder_cmd.c */
> int sss_cmd_empty_packet(struct sss_packet *packet);
> int sss_cmd_send_empty(struct cli_ctx *cctx, TALLOC_CTX *freectx);
>
> diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
> index 0ec2372e8d08f1002b303b5edc6897f17cee9699..3b880647e45cf4d4191ff7a9166e82b11288204d 100644
> --- a/src/responder/common/responder_common.c
> +++ b/src/responder/common/responder_common.c
> @@ -584,10 +584,63 @@ static int sss_dp_init(struct resp_ctx *rctx,
>       return EOK;
>   }
>
> +int create_pipe_fd(const char *sock_name, int *fd, mode_t umaskval)
> +{
> struct sockaddr_un addr;
> errno_t ret;
> *fd = socket(AF_UNIX, SOCK_STREAM, 0);
> if (*fd == -1) {
>    return EIO;
> }
> umask(umaskval);
> ret = set_nonblocking(*fd);
> if (ret != EOK) {
>    goto done;
> }
> ret = set_close_on_exec(*fd);
> if (ret != EOK) {
>    goto done;
> }
> memset(&addr, 0, sizeof(addr));
> addr.sun_family = AF_UNIX;
> strncpy(addr.sun_path, sock_name, sizeof(addr.sun_path)-1);

I think that spaces around '-' operator are more common in our 
codebase...but I don't argue about this.
> addr.sun_path[sizeof(addr.sun_path)-1] = '\0';

same here
> /* make sure we have no old sockets around */
> unlink(sock_name);

check for failure?
> if (bind(*fd, (struct sockaddr *)&addr, sizeof(addr)) == -1) {
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "Unable to bind on socket '%s'\n", sock_name);
>    ret = EIO;
>    goto done;
> }
> if (listen(*fd, 10) != 0) {
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "Unable to listen on socket '%s'\n", sock_name);
>    ret = EIO;
>    goto done;
> }

Why not to have check for listen and bind failure in the same form ('== 
-1') ?
> ret = EOK;

new line would be nice here
> +done:
> /* we want default permissions on created files to be very strict,
>   so set our umask to 0177 */
> umask(0177);
> if (ret != EOK) {
>    close(*fd);
> }
> return ret;
>
> +}

I think it would be nicer to use local varibale fd  in this function and 
return its vales using output parameter _fd. Do you agree?

[Jakub Hrozek]

On Mon, Oct 20, 2014 at 11:28:50AM +0200, Pavel Reichl wrote:
> On 10/17/2014 06:47 PM, Jakub Hrozek wrote:
> > On Thu, Oct 16, 2014 at 10:25:12AM +0200, Jakub Hrozek wrote:
> > > > On Wed, Oct 15, 2014 at 06:19:49PM -0400, Simo Sorce wrote:
> > > > > > On Wed, 15 Oct 2014 22:24:04 +0200
> > > > > > Jakub Hrozek  wrote:
> > > > > >
> > > > > > > >Attached are patches that perform changes in the monitor process and
> > > > > > > >the low-level sbus and sysdb code required to run the NSS responder
> > > > > > > >as a non-privileged user. Some of the patches call chmod/chown on
> > > > > > > >files owned by the SSSD, so I'd like to request a very careful review.
> > > > > >
> > > > > > Aside from the points raised in the emails already sent the rest looks
> > > > > > good to me.
> > > >
> > > > Thank you very much for the review. I'll send out updated patches.
> >
> > Attached are patches that implement the corrections Simo and Pavel asked
> > for with the exception of confdb being read-only for sssd processes,
> > Michal is still investigating that one.
> >
> > I also merged some more Michal's patches that help spawn the PAM
> > privileged pipe before dropping privileges and also dropping privileges
> > in the other responders.
> >
> > With this patchset, all responders are able to run as the SSSD user.
> >
> > I have one question:
> > is it wise to also set the permissions of directories we create when
> > we "install -d" them? Or is this something typically done by the
> > downstream? Previously, we would just rely on the default system umask
> > when running "make install", maybe we should tighten the permissions in
> > Makefile.am as part of this effort?
> >
> > And one comment:
> > One of Michal's patches moves creating the socket into a separate
> > function. This function also changes the umask, which I don't think
> > belongs into a utility function. I realize this is how the code worked
> > even before Michal's change, but I think it would be better to move the
> > umask setting into function like sss_process_init().
> >
> > Thanks again for the review.
> >
> > 0001-UTIL-Add-a-function-to-convert-id_t-from-a-number-or.patch
>
> [snip]
> > +++ b/src/tests/cwrap/test_usertools.c
> > @@ -0,0 +1,101 @@
> > +/*
> > Authors:
> >    Jakub Hrozek<[e-mail]>
> > Copyright (C) 2014 Red Hat
> > SSSD tests: User utilities
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License as published by
> > the Free Software Foundation; either version 3 of the License, or
> > (at your option) any later version.
> > This program is distributed in the hope that it will be useful,
> > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > GNU General Public License for more details.
> > You should have received a copy of the GNU General Public License
> > along with this program.  If not, seehttp://www.gnu.org/licenses/.
> >
> > +*/
> >
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> >
> > +#include <popt.h>
> > +#include "util/util.h"
> > +#include "tests/cmocka/common_mock.h"
> >
> > +void test_get_user_num(void **state)
> > +{
> > uid_t uid;
> > gid_t gid;
> > errno_t ret;
> > ret = sss_user_by_name_or_uid("123", &uid, &gid);
> > assert_int_equal(ret, EOK);
> > assert_int_equal(uid, 123);
> > assert_int_equal(gid, 123);
> >
> > +}
> >
> > +void test_get_user_str(void **state)
> > +{
> > uid_t uid;
> > gid_t gid;
> > errno_t ret;
> > ret = sss_user_by_name_or_uid("sssd", &uid, &gid);
> > assert_int_equal(ret, EOK);
> > assert_int_equal(uid, 123);
> > assert_int_equal(gid, 456);
> >
> > +}
>
> I wasn't able to run the tests now, but why is gid supposed to 456 and in
> prev test 123?

Check out the sssd users' primary GID -- it's 456. I changed it from the
previous iteration, because I wanted to check if SSSD correctly returns
the primary GID and not maybe the UID twice. In real world, the UID and
primary GID would probably be the same, but in unit test it's not.
> > +void test_get_user_nullparm(void **state)
> > +{
> > uid_t uid;
> > errno_t ret;
> > ret = sss_user_by_name_or_uid("sssd", &uid, NULL);
> > assert_int_equal(ret, EOK);
> > assert_int_equal(uid, 123);
> >
> > +}
> >
> > +int main(int argc, const char *argv[])
> > +{
> > poptContext pc;
> > int opt;
> > struct poptOption long_options[] = {
> >    POPT_AUTOHELP
> >    SSSD_DEBUG_OPTS
> >    POPT_TABLEEND
> > };
> > const UnitTest tests[] = {
> >    unit_test(test_get_user_num),
> >    unit_test(test_get_user_str),
> >    unit_test(test_get_user_nullparm),
> > };
> > /* Set debug level to invalid value so we can deside if -d 0 was used. */
> > debug_level = SSSDBG_INVALID;
> > pc = poptGetContext(argv[0], argc, argv, long_options, 0);
> > while((opt = poptGetNextOpt(pc)) != -1) {
> >    switch(opt) {
> >    default:
> >        fprintf(stderr, "\nInvalid option %s: %s\n\n",
> >                poptBadOption(pc, 0), poptStrerror(opt));
> >        poptPrintUsage(pc, stderr, 0);
> >        return 1;
> >    }
> > }
> > poptFreeContext(pc);
> > DEBUG_CLI_INIT(debug_level);
> > tests_set_cwd();
> > return run_tests(tests);
> >
> > +}
> >
> > 0004-SSSD-Load-a-user-to-run-a-service-as-from-configurat.patch
> >
> > From 78066ed7cd43e0208f6d15b8b0489e3699b1b4cc Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek
> > Date: Tue, 5 Aug 2014 13:52:48 +0200
> > Subject: [PATCH 04/19] SSSD: Load a user to run a service as from
> >  configuration
> >
> > Adds two new options, user and group that are specified in the [sssd]
> > section. When these options are specified, SSSD will run as the user and
> > group. When these are not specified, SSSD will run as the configure-time
> > user and group.
> >
> > The group option only takes effect when user is also specified.
> >
> > Currently all services and providers are started as root. There is a
> > temporary svc_supported_as_nonroot() function that returns true for a
> > service if that service runs and was tested as nonroot and false
> > otherwise. Currently this function always returns false, but will be
> > amended in future patches.
>
> [snip]
> > +static int get_service_user(struct mt_ctx *ctx)
> > +{
> > errno_t ret;
> > char *user_str;
> > ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
> >                        CONFDB_MONITOR_USER_RUNAS,
> >                        NULL, &user_str);

I fixed the default here from NULL to SSSD_USER to make sure that
everything works correctly if the user is not specified at all. (Sorry,
the patch regressed when I removed the group option..)
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_FATAL_FAILURE, "Failed to get the user to run as");
>
> '\n'

Fixed.
> >    return ret;
> > }
> > ret = sss_user_by_name_or_uid(user_str, &ctx->uid, &ctx->gid);
> > talloc_free(user_str);
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_FATAL_FAILURE, "Failed to set allowed UIDs.\n");
> >    return ret;
> > }
> > return EOK;
> >
> > +}
> >
> > 0011-responder_common-Create-fd-for-pipe-in-helper.patch
> >
> > From 1a2d3bf47c0da1bb32be138fa8b2b33260f68a37 Mon Sep 17 00:00:00 2001
> > From: Michal Zidek
> > Date: Wed, 15 Oct 2014 17:35:12 +0200
> > Subject: [PATCH 11/19] responder_common: Create fd for pipe in helper
> >
> > Move creating of file descriptor for pipes into
> > helper function and make this function public.
> >
> > src/responder/common/responder.h        |   2 +
> >  src/responder/common/responder_common.c | 129 ++++++++++++++------------------
> >  2 files changed, 59 insertions(+), 72 deletions(-)
> >
> > diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
> > index 97552ec472c5baa285b41cc48b51149f3ef6adb5..d233710782fe7df1bbcc338e3815d1701557519e 100644
> > --- a/src/responder/common/responder.h
> > +++ b/src/responder/common/responder.h
> > @@ -176,6 +176,8 @@ responder_get_domain(struct resp_ctx *rctx, const char *domain);
> >  errno_t responder_get_domain_by_id(struct resp_ctx *rctx, const char *id,
> >                                     struct sss_domain_info **_ret_dom);
> > +int create_pipe_fd(const char *sock_name, int *fd, mode_t umaskval);
> >
> > /* responder_cmd.c */
> >  int sss_cmd_empty_packet(struct sss_packet *packet);
> >  int sss_cmd_send_empty(struct cli_ctx *cctx, TALLOC_CTX *freectx);
> > diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
> > index 0ec2372e8d08f1002b303b5edc6897f17cee9699..3b880647e45cf4d4191ff7a9166e82b11288204d 100644
> > --- a/src/responder/common/responder_common.c
> > +++ b/src/responder/common/responder_common.c
> > @@ -584,10 +584,63 @@ static int sss_dp_init(struct resp_ctx *rctx,
> >      return EOK;
> >  }
> > +int create_pipe_fd(const char *sock_name, int *fd, mode_t umaskval)
> > +{
> > struct sockaddr_un addr;
> > errno_t ret;
> > *fd = socket(AF_UNIX, SOCK_STREAM, 0);
> > if (*fd == -1) {
> >    return EIO;
> > }
> > umask(umaskval);
> > ret = set_nonblocking(*fd);
> > if (ret != EOK) {
> >    goto done;
> > }
> > ret = set_close_on_exec(*fd);
> > if (ret != EOK) {
> >    goto done;
> > }
> > memset(&addr, 0, sizeof(addr));
> > addr.sun_family = AF_UNIX;
> > strncpy(addr.sun_path, sock_name, sizeof(addr.sun_path)-1);
>
> I think that spaces around '-' operator are more common in our
> codebase...but I don't argue about this.

Fixed
> > addr.sun_path[sizeof(addr.sun_path)-1] = '\0';
>
> same here

Fixed
> > /* make sure we have no old sockets around */
> > unlink(sock_name);
>
> check for failure?

Hmm, maybe, but we definitely need to ignore ENOENT and also make the
check non-fatal and still try to bind..

btw the check wasn't there even previously, this patch mostly moves code
around.
> > if (bind(*fd, (struct sockaddr *)&addr, sizeof(addr)) == -1) {
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >          "Unable to bind on socket '%s'\n", sock_name);
> >    ret = EIO;
> >    goto done;
> > }
> > if (listen(*fd, 10) != 0) {
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >          "Unable to listen on socket '%s'\n", sock_name);
> >    ret = EIO;
> >    goto done;
> > }
>
> Why not to have check for listen and bind failure in the same form ('== -1')
> ?

The old code didn't either :-)

Please file a ticket
> > ret = EOK;
>
> new line would be nice here
> > +done:
> > /* we want default permissions on created files to be very strict,
> >   so set our umask to 0177 */
> > umask(0177);

We already have a ticket to move umask elsewhere.
> > if (ret != EOK) {
> >    close(*fd);
> > }
> > return ret;
> >
> > +}
>
> I think it would be nicer to use local varibale fd  in this function and
> return its vales using output parameter _fd. Do you agree?

Yes, in a future commit, though. Please file one single ticket with
improvements for this function. In the attached patchset, I fixed the
small, low-risk changes, but the other changes should be done separately
(even though they are good ideas and we have a unit test..)

[Jakub Hrozek]

On Mon, Oct 20, 2014 at 03:25:52PM +0200, Jakub Hrozek wrote:
> > I think it would be nicer to use local varibale fd  in this function and
> > return its vales using output parameter _fd. Do you agree?
>
> Yes, in a future commit, though. Please file one single ticket with
> improvements for this function. In the attached patchset, I fixed the
> small, low-risk changes, but the other changes should be done separately
> (even though they are good ideas and we have a unit test..)

Attached are patches rebased on the current master, especially unit
tests didn't build after merging the views work.

[Pavel Reichl]

On 10/20/2014 10:42 PM, Jakub Hrozek wrote:
> +void test_get_user_num(void **state)
> +{
> uid_t uid;
> gid_t gid;
> errno_t ret;
> ret = sss_user_by_name_or_uid("123", &uid, &gid);
> assert_int_equal(ret, EOK);
> assert_int_equal(uid, 123);
> assert_int_equal(gid, 123);

gid 123 ?
> +}
>
> +void test_get_user_str(void **state)
> +{
> uid_t uid;
> gid_t gid;
> errno_t ret;
> ret = sss_user_by_name_or_uid("sssd", &uid, &gid);
> assert_int_equal(ret, EOK);
> assert_int_equal(uid, 123);
> assert_int_equal(gid, 456);

gid 456 ?
> +}

[snip]
> +errno_t sss_user_by_name_or_uid(const char *input, uid_t *_uid, gid_t *_gid)
> +{
> uid_t uid;
> gid_t gid;
> errno_t ret;
> char *endptr;
> struct passwd *pwd;

I believe we should null errno here.
> /* Try if it's an ID first */
> uid = strtouint32(input, &endptr, 10);
> gid = uid;

I'm sorry I don't understand why setting gid to uid in case that numeric 
value is passed is the right thing to do. I think we should get the 
value of gid from db all the time.
I suppose it's on purpose as we have tests (which I tried to ask you 
yesterday) that verify this behaviour, but I think that value of gid 
should not matter whether we ask for it using uid or username.
Thanks for explaining.
> if (errno != 0 || *endptr != '\0') {
>    ret = errno;
>    if (ret == ERANGE) {
>        DEBUG(SSSDBG_OP_FAILURE,
>              "UID [%s] is out of range.\n", input);
>        return ret;
>    }
>    /* Nope, maybe a username? */
>    pwd = getpwnam(input);
>    if (pwd == NULL) {
>        DEBUG(SSSDBG_OP_FAILURE,
>              "[%s] is neither a valid UID nor a user name which could be "
>              "resolved by getpwnam().\n", input);
>        return EINVAL;
>    }
>    uid = pwd->pw_uid;
>    gid = pwd->pw_gid;
> }
> if (_uid) {
>    *_uid = uid;
> }
> if (_gid) {
>    *_gid = gid;
> }
> return EOK;
>
> +}

[Jakub Hrozek]

On Tue, Oct 21, 2014 at 11:14:47AM +0200, Pavel Reichl wrote:
> On 10/20/2014 10:42 PM, Jakub Hrozek wrote:
> > +void test_get_user_num(void **state)
> > +{
> > uid_t uid;
> > gid_t gid;
> > errno_t ret;
> > ret = sss_user_by_name_or_uid("123", &uid, &gid);
> > assert_int_equal(ret, EOK);
> > assert_int_equal(uid, 123);
> > assert_int_equal(gid, 123);
>
> gid 123 ?
> > +}
> >
> > +void test_get_user_str(void **state)
> > +{
> > uid_t uid;
> > gid_t gid;
> > errno_t ret;
> > ret = sss_user_by_name_or_uid("sssd", &uid, &gid);
> > assert_int_equal(ret, EOK);
> > assert_int_equal(uid, 123);
> > assert_int_equal(gid, 456);
>
> gid 456 ?
> > +}
>
> [snip]
> > +errno_t sss_user_by_name_or_uid(const char *input, uid_t *_uid, gid_t *_gid)
> > +{
> > uid_t uid;
> > gid_t gid;
> > errno_t ret;
> > char *endptr;
> > struct passwd *pwd;
>
> I believe we should null errno here.

OK
> > /* Try if it's an ID first */
> > uid = strtouint32(input, &endptr, 10);
> > gid = uid;
>
> I'm sorry I don't understand why setting gid to uid in case that numeric
> value is passed is the right thing to do. I think we should get the value of
> gid from db all the time.

OK, calling getpwuid sounds like the right thing to do. I'll send new
patches -- thanks!
> I suppose it's on purpose as we have tests (which I tried to ask you
> yesterday) that verify this behaviour, but I think that value of gid should
> not matter whether we ask for it using uid or username.
> Thanks for explaining.
> > if (errno != 0 || *endptr != '\0') {
> >    ret = errno;
> >    if (ret == ERANGE) {
> >        DEBUG(SSSDBG_OP_FAILURE,
> >              "UID [%s] is out of range.\n", input);
> >        return ret;
> >    }
> >    /* Nope, maybe a username? */
> >    pwd = getpwnam(input);
> >    if (pwd == NULL) {
> >        DEBUG(SSSDBG_OP_FAILURE,
> >              "[%s] is neither a valid UID nor a user name which could be "
> >              "resolved by getpwnam().\n", input);
> >        return EINVAL;
> >    }
> >    uid = pwd->pw_uid;
> >    gid = pwd->pw_gid;
> > }
> > if (_uid) {
> >    *_uid = uid;
> > }
> > if (_gid) {
> >    *_gid = gid;
> > }
> > return EOK;
> >
> > +}
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Reichl]

Just last nitpicks before I dive into testing:
> Date: Tue, 5 Aug 2014 13:53:20 +0200
> Subject: [PATCH 03/19] RPM: Change file ownership to sssd.sssd
>
> Adds a private SSSD user in the %pre section of SSSD specfile. Also
> changes the ownsership of SSSD private directories to sssd.sssd.

s/ownsership/ownership
> Subject: [PATCH 05/19] SBUS: Chown the sbus socket if needed

[snip]
> @@ -268,6 +269,17 @@ int sbus_new_server(TALLOC_CTX *mem_ctx,
>          }
>      }
> if (stat_buf.st_uid != uid || stat_buf.st_gid != gid) {
>    ret = chown(filename, uid, gid);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "chown failed for [%s]: [%d][%s].\n", filename, errno,
> strerror(errno));

It would be nice to not use errno directly as strerror() itself may 
change errno value.
Just calling ret = errno and passing ret to strerror() would make me happy.
You may also consider using sss_strerror  instead of strerror.
>        ret = EIO;
>        goto done;
>    }
> }
> From a19db5f165a1d96fe159201df71b1b7d0f3aa1ff Mon Sep 17 00:00:00 2001
> From: Michal Zidek 
> Date: Thu, 9 Oct 2014 17:15:56 +0200
> Subject: [PATCH 08/19] MONITOR: Allow confdb to be accessed by nonroot 
> user
>
> src/monitor/monitor.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> index 
> 905e66f25601d155557487ae9c7eb6d3145d3a83..d430f96c5af9b45c9641d700d9fbcd3590bdaae6 
> 100644
> --- a/src/monitor/monitor.c
> +++ b/src/monitor/monitor.c
> @@ -1696,7 +1696,6 @@ static errno_t load_configuration(TALLOC_CTX 
> *mem_ctx,
>          DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing confdb\n");
>          goto done;
>      }
> talloc_zfree(cdb_file);

OK, cdf_file is used later in this function but still it should be freed 
in the end.
>  ret = confdb_init_db(config_file, ctx->cdb);
>  if (ret != EOK) {
>
> @@ -1712,6 +1711,17 @@ static errno_t load_configuration(TALLOC_CTX 
> *mem_ctx,
>          goto done;
>      }
>
> /* Allow configuration database to be accessible
> * when SSSD runs as nonroot */
>
> ret = chown(cdb_file, ctx->uid, ctx->gid);
>
> if (ret != 0) {
>    ret = errno;
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "chown failed for [%s]: [%d][%s].\n",
>          cdb_file, ret, sss_strerror(ret));
>    goto done;
>
> }
>
> *monitor = ctx;
>
> ret = EOK;
>
> -- 
> 1.9.3
> Subject: [PATCH 14/19] PAM: Create pipe file descriptors before 
> priviledges
>  are dropped

s/priviledges/privileges
> @@ -347,6 +350,24 @@ int main(int argc, const char *argv[])
>      /* set up things like debug, signals, daemonization, etc... */
>      debug_log_file = "sssd_pam";
> /* Crate pipe file descriptors here before priviledges are dropped

same typo
> * in server_setup() */
> ret = create_pipe_fd(SSS_PAM_SOCKET_NAME, &pipe_fd, 0111);
> if (ret != EOK) {
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "create_pipe_fd failed [%d]: %s.\n",
>          ret, sss_strerror(ret));
>    return 2;
> }
> ret = create_pipe_fd(SSS_PAM_PRIV_SOCKET_NAME, &priv_pipe_fd, 0177);
> if (ret != EOK) {
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "create_pipe_fd failed (priviledged pipe) [%d]: %s.\n",

same typo
>          ret, sss_strerror(ret));
>    return 2;
> }

[Jakub Hrozek]

On Tue, Oct 21, 2014 at 01:59:31PM +0200, Pavel Reichl wrote:
> Just last nitpicks before I dive into testing:

I hope I managed to fix them all, see the attached patches..

[Pavel Reichl]

On 10/21/2014 04:40 PM, Jakub Hrozek wrote:
> On Tue, Oct 21, 2014 at 01:59:31PM +0200, Pavel Reichl wrote:
> > Just last nitpicks before I dive into testing:
>
> I hope I managed to fix them all, see the attached patches..
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

The first patch no longer applies:

diff --cc src/tests/cwrap/Makefile.am
index d1f0e9e,d9eb5a8..0000000
--- a/src/tests/cwrap/Makefile.am
+++ b/src/tests/cwrap/Makefile.am
@@@ -42,10 -42,12 +42,19 @@@ check_PROGRAMS
   if HAVE_CMOCKA
   if HAVE_NSS_WRAPPER
   if HAVE_UID_WRAPPER
++<<<<<<< HEAD
  +check_PROGRAMS += \
  +    become_user-tests \
  +    server-tests \
  +    $(NULL)
++=======
+ check_PROGRAMS = \
+       become_user-tests \
+       server-tests \
+       usertools-tests \
+       responder_common-tests \
+       $(NULL)
++>>>>>>> UTIL: Add a function to convert id_t from a number or a name

I think it is because of the patch 
(5192d5db927d718e2bb1b6551753a836b2a3291a) that Lukas submited today.

Could you rebase please?

[Jakub Hrozek]

On Tue, Oct 21, 2014 at 04:40:46PM +0200, Jakub Hrozek wrote:
> On Tue, Oct 21, 2014 at 01:59:31PM +0200, Pavel Reichl wrote:
> > Just last nitpicks before I dive into testing:
>
> I hope I managed to fix them all, see the attached patches..

After today's fixes, there were some conflicts in
src/tests/cwrap/Makefile.am. Otherwise no changes..

[Pavel Reichl]

On 10/22/2014 02:17 PM, Jakub Hrozek wrote:
> On Tue, Oct 21, 2014 at 04:40:46PM +0200, Jakub Hrozek wrote:
> > On Tue, Oct 21, 2014 at 01:59:31PM +0200, Pavel Reichl wrote:
> > > Just last nitpicks before I dive into testing:
> >
> > I hope I managed to fix them all, see the attached patches..
>
> After today's fixes, there were some conflicts in
> src/tests/cwrap/Makefile.am. Otherwise no changes..
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Thanks.

ACK to all.

[Jakub Hrozek]

On Wed, Oct 22, 2014 at 02:25:20PM +0200, Pavel Reichl wrote:
> On 10/22/2014 02:17 PM, Jakub Hrozek wrote:
> > On Tue, Oct 21, 2014 at 04:40:46PM +0200, Jakub Hrozek wrote:
> > > On Tue, Oct 21, 2014 at 01:59:31PM +0200, Pavel Reichl wrote:
> > > > Just last nitpicks before I dive into testing:
> > >
> > > I hope I managed to fix them all, see the attached patches..
> >
> > After today's fixes, there were some conflicts in
> > src/tests/cwrap/Makefile.am. Otherwise no changes..
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Thanks.
>
> ACK to all.

Thank you for the careful review. I submitted the patches to CI before
pushing.

[Jakub Hrozek]

On Wed, Oct 22, 2014 at 02:30:44PM +0200, Jakub Hrozek wrote:
> On Wed, Oct 22, 2014 at 02:25:20PM +0200, Pavel Reichl wrote:
> > On 10/22/2014 02:17 PM, Jakub Hrozek wrote:
> > > On Tue, Oct 21, 2014 at 04:40:46PM +0200, Jakub Hrozek wrote:
> > > > On Tue, Oct 21, 2014 at 01:59:31PM +0200, Pavel Reichl wrote:
> > > > > Just last nitpicks before I dive into testing:
> > > >
> > > > I hope I managed to fix them all, see the attached patches..
> > >
> > > After today's fixes, there were some conflicts in
> > > src/tests/cwrap/Makefile.am. Otherwise no changes..
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Thanks.
> >
> > ACK to all.
>
> Thank you for the careful review. I submitted the patches to CI before
> pushing.

CI succeeded:
    http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/233/

Patches were pushed to master:
   * 76c8dafad2a18cf1514635aa766062085c23a5c8
   * 3f9e2c24dbc14b2eafbe4f5a5ee16fe9af3c3f75
   * 22f4bcbb211bf800af647ad1fc9595a8020a6fe6
   * 287cc55b9086dd3c4e2a5fb84784e09767860142
   * 4e1892cdfcc5300d6632200c38ba67f2783d15f2
   * b547bd685cb71bb450b0c86487767f02e66f6cea
   * 8bccd95e275fae760a991da394235e4e70e57bbd
   * 4a5cced91df68a85ef0b30de8efe104c8a0aab7a
   * 2ce29e05e62b2702ba4df5f3316eaf250b0ada7f
   * 5d19966eda424bd71964c6913b84d705dce3b350
   * 0887c35bdb85adf0a4376dc8963294ea5a9d6da6
   * 579e5d4b7a3ca161ea7518b2996905fa22c15995
   * 19e9c1c1a21790974400db9349637788727b6564
   * aa871e019f00493dfa53b48f906132bf94eeae9f
   * 5960687483a5d3d99093c9d6ab64e11c9bde7f7b
   * a10ac1d0a7210def232205a48c53a075930e82f6
   * fa24dabfd480e1ce346009336c7979ab59520c44
   * bc13c352ba9c2877f1e9bc62e55ad60fc000a55d
   * 5eda23c28c582b43b2a0a165b1750f3875c0fa84

[Pavel Reichl]

On 10/22/2014 02:17 PM, Jakub Hrozek wrote:
> +errno_t sss_user_by_name_or_uid(const char *input, uid_t *_uid, gid_t *_gid)
> +{
> uid_t uid;
> errno_t ret;
> char *endptr;
> struct passwd *pwd;
> /* Try if it's an ID first */
> errno = 0;
> uid = strtouint32(input, &endptr, 10);
> if (errno != 0 || *endptr != '\0') {
>    ret = errno;
>    if (ret == ERANGE) {
>        DEBUG(SSSDBG_OP_FAILURE,
>              "UID [%s] is out of range.\n", input);
>        return ret;
>    }
>    /* Nope, maybe a username? */
>    pwd = getpwnam(input);
> } else {
>    pwd = getpwuid(uid);
> }
> if (pwd == NULL) {
>    DEBUG(SSSDBG_OP_FAILURE,
>          "[%s] is neither a valid UID nor a user name which could be "
>          "resolved by getpwnam().\n", input);
>    return EINVAL;
> }
> if (_uid) {
>    *_uid = pwd->pw_uid;
> }
> if (_gid) {
>    *_gid = pwd->pw_gid;
> }
> return EOK;
>
> +}

Hello, sorry for opening an old thread.
I was just wondering if we support user names that are completely 
numerical - like 111 or 555?

This function would fail for such users if we added added test like this:

src/tests/cwrap/passwd

111:x:222:444:User whose name is numerical:/home/111:/bin/bash

src/tests/cwrap/test_usertools.c

+void test_get_user_with_numerical_username(void **state)
+{
+    uid_t uid;
+    gid_t gid;
+    errno_t ret;
+
+    ret = sss_user_by_name_or_uid("111", &uid, &gid);
+    assert_int_equal(ret, EOK);
+    assert_int_equal(uid, 222);
+    assert_int_equal(gid, 444);
+}

Fixing sss_user_by_name_or_uid() is a simple matter and I'm happy to do 
it, should I?

[Jakub Hrozek]

On Tue, Jan 27, 2015 at 11:12:44AM +0100, Pavel Reichl wrote:
> Hello, sorry for opening an old thread.
> I was just wondering if we support user names that are completely numerical
> like 111 or 555?

The responders should, because the local shadow-utils support numerical
username. But it's extremely poor practice and I don't think we should
care for setting the users to run as.

[Pavel Reichl]

On 10/15/2014 10:24 PM, Jakub Hrozek wrote:
> 0006-SYSDB-Allow-calling-chown-on-the-sysdb-file-from-mon.patch
>
> From 15386564a33d1461918b3f03a6eee0138a690055 Mon Sep 17 00:00:00 2001
> From: Michal Zidek
> Date: Thu, 9 Oct 2014 17:21:30 +0200
> Subject: [PATCH 6/7] SYSDB: Allow calling chown on the sysdb file from monitor
>
> Sysdb must be accessible for the nonroot sssd
> processes.
>
> src/db/sysdb.c        | 17 +++++++++++++++++
>   src/db/sysdb.h        |  9 +++++++++
>   src/monitor/monitor.c |  3 ++-
>   3 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/src/db/sysdb.c b/src/db/sysdb.c
> index 88cff241fab9022c3fed45dbb409393dbe01a590..2a7d38b372c4cbd69311d03f7cb5ca98f7057d19 100644
> --- a/src/db/sysdb.c
> +++ b/src/db/sysdb.c
> @@ -1286,6 +1286,16 @@ int sysdb_init(TALLOC_CTX *mem_ctx,
>                  struct sss_domain_info *domains,
>                  bool allow_upgrade)
>   {
> return sysdb_init_ext(mem_ctx, domains, allow_upgrade, false, 0, 0);
>
> +}
>
> +int sysdb_init_ext(TALLOC_CTX *mem_ctx,
>               struct sss_domain_info *domains,
>               bool allow_upgrade,
>               bool chown_dbfile,
>               uid_t uid,
>               gid_t gid)
>
> +{
>       struct sss_domain_info *dom;
>       struct sysdb_ctx *sysdb;
>       int ret;
> @@ -1307,6 +1317,13 @@ int sysdb_init(TALLOC_CTX *mem_ctx,
>               return ret;
>           }
>    if (chown_dbfile) {
>        chown(sysdb->ldb_file, uid, gid);

missing check on return value?
>        if (ret != EOK) {
>            return ret;
>        }
>    }
>     dom->sysdb = talloc_move(dom, &sysdb);
> }
>
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index c93119cf15a6ed0e2b0f026a67ab48e926151a0b..dfef1b5857c024e4a4c06d828650586acc33cc6f 100644
> --- a/src/db/sysdb.h
> +++ b/src/db/sysdb.h
> @@ -418,6 +418,15 @@ errno_t sysdb_update_ranges(struct sysdb_ctx *sysdb,
>   int sysdb_init(TALLOC_CTX *mem_ctx,
>                  struct sss_domain_info *domains,
>                  bool allow_upgrade);
>
> +/* Same as sysdb_init, but additionally allows to change
> file ownership of the sysdb databases. */
>
> +int sysdb_init_ext(TALLOC_CTX *mem_ctx,
>               struct sss_domain_info *domains,
>               bool allow_upgrade,
>               bool chown_dbfile,
>               uid_t uid, gid_t gid);
> /* used to initialize only one domain database.
> Do NOT use if sysdb_init has already been called */
> int sysdb_domain_init(TALLOC_CTX *mem_ctx,
>
> diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> index bd2c373008ef75ab46cf7dccdefd12468894f1ba..19f0db0830353015fb240033414f1d9b4ffc7dc0 100644
> --- a/src/monitor/monitor.c
> +++ b/src/monitor/monitor.c
> @@ -2332,7 +2332,8 @@ static int monitor_process_init(struct mt_ctx *ctx,
>       if (!tmp_ctx) {
>           return ENOMEM;
>       }
> ret = sysdb_init(tmp_ctx, ctx->domains, true);
> ret = sysdb_init_ext(tmp_ctx, ctx->domains, true,
>                     true, ctx->uid, ctx->gid);
> if (ret != EOK) {
>     SYSDB_VERSION_ERROR_DAEMON(ret);
>     return ret;
>
> -- 1.9.3