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

Pavel Reichl preichl at redhat.com
Thu Oct 16 08:31:15 UTC 2014


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 <jhrozek at redhat.com> 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
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list