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(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel