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

Pavel Reichl preichl at redhat.com
Mon Oct 20 09:28:50 UTC 2014


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<jhrozek at redhat.com>  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<jhrozek at redhat.com>
> +
> +    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, see<http://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<jhrozek at redhat.com>
> 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<mzidek at redhat.com>
> 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?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141020/e21ae3b4/attachment.html>


More information about the sssd-devel mailing list