[SSSD] [PATCH] AD: Add initial gpo-smb implementation.

Lukas Slebodnik lslebodn at redhat.com
Mon Jun 16 13:22:40 UTC 2014


On (15/06/14 19:08), Yassir Elley wrote:
>----- Original Message -----
>> ----- Original Message -----
>> > On Thu, Jun 12, 2014 at 04:37:25PM -0400, Yassir Elley wrote:
>> > > 
>> > > 
>> > > ----- Original Message -----
>> > > > On Fri, May 30, 2014 at 12:35:32PM -0400, Yassir Elley wrote:
>> > > > > 
>> > > > > 
>> > > > > ----- Original Message -----
>> > > > > > On (29/05/14 09:06), Yassir Elley wrote:
>> > > > > > >----- Original Message -----
>> > > > > 
>> > 
>> > ...
>> > 
>> > > > 
>> > > > 
>> > > > I didn't had a chance to do functional testings yet, I'll send my
>> > > > results tomorrow. For a start please find my comments on the code
>> > > > inline.
>> > > 
>> > > Please let me know if you have additional comments after functional
>> > > testing.
>> > 
>> > Basic functional tests were working fine, I was able to reject access
>> > and grant it again by modifying the GPO on the AD DC.
>> > 
>> > While watching the logs I've seen the smb uri
>> > smb://domain.name/sysvol/... A DNS query for domain.name will return the
>> > list of all DCs in domain.name and libsmbclient will probably pick the
>> > first which might be in a completely different location than the client.
>> > I would suggest to replace domain.name with the name of the DC SSSD is
>> > currently connected to.
>> 
>> Is there an easy way to get the name of the DC that SSSD is currently
>> connected to?
>> 
>> > 
>> > As a general comment, I would prefer to wait until the file transfer is
>> > done in a child process before pushing this patch to master. Because
>> > otherwise imo the risk of blocking the backend for too long is too high.
>> 
>> That's fine. I wanted to facilitate the review of the basic gpo-smb code, by
>> not introducing the complexities of a child process. Now that this review is
>> complete, I'll modify the code per the review comments, and then integrate
>> the code into a gpo_child process patch.
>> 
>> > 
>> > > 
>> > > > 
>> > > > bye,
>> > > > Sumit
>> > > > 
>> > > > 
>> > > > 
>> > 
>> > ...
>> > 
>> > > > I have two comments here. First, is there a way to avoid having deny
>> > > > rules? The reason is that we had some troubles in the past here,
>> > > > because
>> > > > when checking deny rules all group memberships of a user must be
>> > > > available to properly deny access. If some are missing access might be
>> > > > granted although it should be rejected. I guess it is not possible to
>> > > > get rid of them because they will be set on the AD side and we have to
>> > > > respect them.
>> > > 
>> > > I agree that relying on deny rules for security is problematic, b/c it
>> > > can
>> > > be difficult to exhaustively prove a negative. Don't we have the same
>> > > challenge with the simple_access_provider's simple_deny_groups?
>> > 
>> > yes, but I guess there were request to add deny groups as well. But
>> > since this will be set in sssd.conf users are free to use it or not.
>> 
>> In theory, we could support just the Allow rules in the policy file (and not
>> support the Deny rules), but I think this would be confusing for AD admins.
>> We should support both Allow rules and Deny rules.
>> 
>> > 
>> > > 
>> > > In any case, I also agree with your conclusion that we have to respect
>> > > the
>> > > GPO policy settings defined on the AD side.
>> > > 
>> > > > 
>> > > > Second, you are only checking SeInteractiveLogonRight and
>> > > > SeDenyInteractiveLogonRight. What about
>> > > > Se[Deny][Network|Batch|Service|RemoteInteractive]LogonRight? Are you
>> > > > planning to integrate them as well and relate them to different PAM
>> > > > services?
>> > > 
>> > > The plan for the initial deployment is to only support
>> > > SeInteractiveLogonRight and SeDenyInteractiveLogonRight.
>> > 
>> > ok, but please keep in mind that it would be hard to add support for
>> > different login types, e.g. network vs. console, later on without
>> > breaking backward compatibility.
>> 
>> We are initially supporting console login as a feature. If we later decide to
>> support network login as a feature (in a subsequent release), what is the
>> concern regarding backward compatibility?
>> 
>> > 
>> > > 
>> > 
>> > ...
>> > 
>> > > > > +
>> > > > > +  ret = ini_get_config_valueobj(RIGHTS_SECTION, name, ini_config,
>> > > > > +                                INI_GET_FIRST_VALUE, &vobj);
>> > > > > +  if (vobj == NULL) {
>> > > > > +      DEBUG(SSSDBG_CRIT_FAILURE, "section/name not found:
>> > > > > [%s][%s]\n",
>> > > > > +            RIGHTS_SECTION, name);
>> > > > > +      return EOK;
>> > > > > +  }
>> > > > > +  sids = ini_get_string_config_array(vobj, NULL, &num_sids, &ret);
>> > > > > +
>> > > > > +  if (ret) {
>> > > > 
>> > > > Please do explicit checks, in this case (ret != 0) because according to
>> > > > the docs ini_get_string_config_array() returns 0 on success.
>> > > 
>> > > Doesn't C define "true" as a non-zero value? Or are you suggesting this
>> > > just for clarity?
>> > 
>> > For clarity, see e.g. Andreas' blog post
>> > http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/ .
>> > 
>> > I think we have the practice in SSSD to use 'if (a)' and 'if (!a)' only
>> > if 'a' is a bool or a pointer. Personally I prefer '!= NULL' and '==
>> > NULL' with pointers as well.
>> 
>> OK.
>> 
>> > 
>> > > 
>> > > > 
>> > > > > +      DEBUG(SSSDBG_CRIT_FAILURE,
>> > > > > +            "ini_get_string_config_array failed [%d][%s]\n", ret,
>> > > > > strerror(ret));
>> > > > > +      return ret;
>> > > > > +  }
>> > > > > +
>> > > > > +  for (i = 0; i <num_sids; i++) {
>> > > > > +    if (sids[i][0] == '*') {
>> > > > > +      sids[i]++;
>> > > > > +    }
>> > > > > +  }
>> > > > 
>> > > > What is this loop good for?
>> > > > 
>> > >  
>> > > The GptTmpl.inf policy files I have seen all include an asterisk before
>> > > the
>> > > user_sid or group_sid; this loop removes the asterisk so that the policy
>> > > file sids can be correctly compared against the cached sids (which don't
>> > > have asterisks).
>> > 
>> > Please add this as a comment for the loop.
>> > 
>> 
>> OK.
>> 
>> > > 
>> > 
>> > ...
>> > 
>> > > > > +}
>> > > > > +
>> > > > > +int ad_gpo_process_cse_recv(struct tevent_req *req,
>> > > > > +                            TALLOC_CTX *mem_ctx,
>> > > > > +                            char ***allowed_sids,
>> > > > > +                            int *allowed_size,
>> > > > > +                            char ***denied_sids,
>> > > > > +                            int *denied_size)
>> > > > > +{
>> > > > > +    struct ad_gpo_process_cse_state *state =
>> > > > > +        tevent_req_data(req, struct ad_gpo_process_cse_state);
>> > > > > +
>> > > > > +    TEVENT_REQ_RETURN_ON_ERROR(req);
>> > > > > +
>> > > > > +    *allowed_sids = state->allowed_sids;
>> > > > > +    *allowed_size = state->allowed_size;
>> > > > 
>> > > > I guess talloc_steal is missing here.
>> > > 
>> > > Both allowed_sids and denied_sids are populated using
>> > > ini_get_string_config_array(), which does not use talloc memory
>> > > allocation.
>> > 
>> > I see. I was confused by mem_ctx in the argument list. I would suggest
>> > to copy the lists to talloc arrays here or even at a lower level because
>> > it is confusing and the lists are currently not freed in
>> > ad_gpo_cse_done(). It would be even more difficult if you start to read
>> > the list from cache, because here talloc arrays would be used.
>> 
>> OK.
>> 
>> > 
>> > bye,
>> > Sumit
>> > 
>> 
>> Regards,
>> Yassir.
>
>I have attached a patch, which integrates the gpo-smb code into a gpo_child process. You may have additional code review comments on the gpo_child part of the code, but I think I have addressed all of the initial gpo-smb code review comments in this patch, with the following two exceptions (for which I need some more guidance from you):
>* You suggested adding the new minimal version (libini_config-devel >= 1.1) to the configure check. Can you clarify what you mean by this, or preferably provide a diff? We probably only want to enforce this minimal version configure check for the case in which the gpo code is being used, right? For example, if the ipa-provider is being used, then we wouldn't want this configure check.
I think Sumit meant PKG_CHECK_MODULES(INI_CONFIG, [ini_config >= 1.1]]) in file
src/external/libini_config.m4

ipa-provider and ad-provider are related due to ipa-server mode. On the other
side ldap-provider can work with older version of libini_config.
If you want to solve problem with your proposal you can add this autoconf test
into src/external/samba.m4.

ding-libs is a simple library without complex dependencies. So I don't know if
we want to complicate it but Sumit can have different preferences.

>* You suggested using the name of the DC that SSSD is currently connected to in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.
>
>Regards,
>Yassir.

>From 5567854a940cee094a832bcae7271faa81b63bd0 Mon Sep 17 00:00:00 2001
>From: Yassir Elley <yelley at redhat.com>
>Date: Wed, 21 May 2014 15:11:36 -0400
>Subject: [PATCH] AD-GPO: Add gpo-smb implementation in gpo_child process
>
>---
> Makefile.am                     |  20 +
> contrib/sssd.spec.in            |   2 +-
> src/providers/ad/ad_gpo.c       | 822 +++++++++++++++++++++++++++++++++++++++-
> src/providers/ad/ad_gpo_child.c | 620 ++++++++++++++++++++++++++++++
> 4 files changed, 1458 insertions(+), 6 deletions(-)
> create mode 100644 src/providers/ad/ad_gpo_child.c
>
>diff --git a/Makefile.am b/Makefile.am
>index f64c77a5bdd7f53b41bd37572b2d91434a9893a6..4c2f13b1f9d6631cde82d8e61b89eecf4d2fbec5 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -107,6 +107,7 @@ sssdlibexec_PROGRAMS = \
>     sssd_be \
>     krb5_child \
>     ldap_child \
>+    gpo_child \
>     proxy_child
> if BUILD_SUDO
> sssdlibexec_PROGRAMS += sssd_sudo
>@@ -2294,6 +2295,25 @@ ldap_child_LDADD = \
>     $(DHASH_LIBS) \
>     $(KRB5_LIBS)

It would be better if you could build gpo_child conditionally.

if BUILD_SAMBA
sssdlibexec_PROGRAMS += gpo_child
endif

> 
>+gpo_child_SOURCES = \
>+    src/providers/ad/ad_gpo_child.c \
>+    src/util/atomic_io.c \
>+    src/util/util.c \
>+    src/util/signal.c
>+gpo_child_CFLAGS = \
>+    $(AM_CFLAGS) \
>+    $(POPT_CFLAGS) \
>+    $(KRB5_CFLAGS) \
>+    $(INI_CONFIG_CFLAGS) \
>+    $(SMBCLIENT_CFLAGS)
>+gpo_child_LDADD = \
>+    libsss_debug.la \
>+    $(TALLOC_LIBS) \
>+    $(POPT_LIBS) \
>+    $(DHASH_LIBS) \
>+    $(INI_CONFIG_LIBS) \
>+    $(SMBCLIENT_LIBS)
>+
> proxy_child_SOURCES = \
>     src/providers/proxy/proxy_child.c \
>     src/providers/data_provider_iface_generated.c \
>diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
>index b5e7903713ccc6944fbed4f84c2f0413a257b760..10704ae48cf5636de2a5cbbb42fc00b99052144f 100644
>--- a/contrib/sssd.spec.in
>+++ b/contrib/sssd.spec.in
>@@ -97,7 +97,7 @@ BuildRequires: libtdb-devel
> BuildRequires: libldb-devel
> BuildRequires: libdhash-devel >= 0.4.2
> BuildRequires: libcollection-devel
>-BuildRequires: libini_config-devel
>+BuildRequires: libini_config-devel >= 1.1
> BuildRequires: dbus-devel
> BuildRequires: dbus-libs
> %if 0%{?rhel5_minor} >= 7
>diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
>index bb6896e70ae96036b1a9e58a6c8e25d3d8f1543f..78c2a7da0352e0ea464f01751b907dbc4e02e861 100644
>--- a/src/providers/ad/ad_gpo.c
>+++ b/src/providers/ad/ad_gpo.c
//snip

>+
>+int ad_gpo_process_cse_recv(struct tevent_req *req,
>+                            TALLOC_CTX *mem_ctx,
>+                            int *_allowed_size,
>+                            char ***_allowed_sids,
>+                            int *_denied_size,
>+                            char ***_denied_sids)
>+{
>+    char *smb_uri;
      ^^^^^^^^^^^^^^
      unused variable
>+    int ret;
>+    char **allowed_sids;
>+    int allowed_size;
>+    char **denied_sids;
>+    int denied_size;
>+    struct ad_gpo_process_cse_state *state;
>+
>+    state = tevent_req_data(req, struct ad_gpo_process_cse_state);
>+
>+    TEVENT_REQ_RETURN_ON_ERROR(req);
>+
>+    ret = parse_gpo_child_response(mem_ctx, state->buf, state->len,
>+                                   &allowed_sids,
>+                                   &allowed_size,
>+                                   &denied_sids,
>+                                   &denied_size);
>+
>+    if (ret != EOK) {
>+        DEBUG(SSSDBG_CRIT_FAILURE,
>+              "Cannot parse child response: [%d][%s]\n", ret, strerror(ret));
>+        return ret;
>+    }
>+
>+    *_allowed_size = allowed_size;
>+    *_allowed_sids = talloc_steal(mem_ctx, allowed_sids);
>+    *_denied_size = denied_size;
>+    *_denied_sids = talloc_steal(mem_ctx, denied_sids);
>+
>+    return EOK;
>+}
>+
>+static errno_t
>+gpo_fork_child(struct tevent_req *req)
>+{
>+    int pipefd_to_child[2];
>+    int pipefd_from_child[2];
>+    pid_t pid;
>+    int ret;
>+    errno_t err;
>+    struct ad_gpo_process_cse_state *state;
>+
>+    state = tevent_req_data(req, struct ad_gpo_process_cse_state);
>+
>+    ret = pipe(pipefd_from_child);
>+    if (ret == -1) {
>+        err = errno;
>+        DEBUG(SSSDBG_CRIT_FAILURE,
>+              "pipe failed [%d][%s].\n", errno, strerror(errno));
>+        return err;
>+    }
>+    ret = pipe(pipefd_to_child);
>+    if (ret == -1) {
>+        err = errno;
>+        DEBUG(SSSDBG_CRIT_FAILURE,
>+              "pipe failed [%d][%s].\n", errno, strerror(errno));
>+        return err;
>+    }
>+
>+    pid = fork();
>+
>+    if (pid == 0) { /* child */
>+        err = exec_child(state,
>+                         pipefd_to_child, pipefd_from_child,
>+                         GPO_CHILD, gpo_child_debug_fd);
>+        DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec gpo_child: [%d][%s].\n",
>+              err, strerror(err));
>+        return err;
>+    } else if (pid > 0) { /* parent */
>+        state->child_pid = pid;
>+        state->io->read_from_child_fd = pipefd_from_child[0];
>+        close(pipefd_from_child[1]);
>+        state->io->write_to_child_fd = pipefd_to_child[1];
>+        close(pipefd_to_child[0]);
>+        fd_nonblocking(state->io->read_from_child_fd);
>+        fd_nonblocking(state->io->write_to_child_fd);
>+
>+        ret = child_handler_setup(state->ev, pid, NULL, NULL, NULL);
>+        if (ret != EOK) {
>+            DEBUG(SSSDBG_CRIT_FAILURE,
>+                  "Could not set up child signal handler\n");
>+            return ret;
>+        }
>+    } else { /* error */
>+        err = errno;
>+        DEBUG(SSSDBG_CRIT_FAILURE,
>+              "fork failed [%d][%s].\n", errno, strerror(errno));
>+        return err;
>+    }
>+
>+    return EOK;
>+}
>diff --git a/src/providers/ad/ad_gpo_child.c b/src/providers/ad/ad_gpo_child.c
>new file mode 100644
>index 0000000000000000000000000000000000000000..4659d8c3666841ea7bd84194cc1fdcfb671851ed
>--- /dev/null
>+++ b/src/providers/ad/ad_gpo_child.c
>@@ -0,0 +1,620 @@
>+/*
>+    SSSD
>+
>+    AD GPO Backend Module -- perform SMB and CSE processing in a child process
>+
>+    Authors:
>+        Yassir Elley <yelley at redhat.com>
>+
>+    Copyright (C) 2013 Red Hat
>+
>+    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/>.
>+*/
>+
//snip

>+static errno_t
>+process_security_settings_cse(TALLOC_CTX *mem_ctx,
>+                              const char *smb_uri,
>+                              char ***_allowed_sids,
>+                              int *_allowed_size,
>+                              char ***_denied_sids,
>+                              int *_denied_size)
>+{
>+    SMBCCTX *context;
>+    int ret = 0;
>+    uint8_t *buf = NULL;
>+    int bytesread = 0;
>+    char **allowed_sids;
>+    char **denied_sids;
>+    int allowed_size = 0;
>+    int denied_size = 0;
>+    TALLOC_CTX *tmp_ctx = NULL;
>+
>+    tmp_ctx = talloc_new(NULL);
>+    if (tmp_ctx == NULL) {
>+        ret = ENOMEM;
>+        goto done;
You should immediatelly return ENOMEM, otherwise context will be used
unitialized in done section.

>+    }
>+
>+    DEBUG(SSSDBG_TRACE_ALL, "%s\n", smb_uri);
>+
>+    context = smbc_new_context();
>+    if (context == NULL) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "Could not allocate new smbc context\n");
>+        ret = ENOMEM;
>+        goto done;
>+    }
>+
>+    smbc_setFunctionAuthData(context, sssd_krb_get_auth_data_fn);
>+    smbc_setOptionUseKerberos(context, 1);
>+
>+    /* Initialize the context using the previously specified options */
>+    if (smbc_init_context(context) == NULL) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "Could not initialize smbc context\n");
>+        ret = ENOMEM;
>+        goto done;
>+    }
>+
>+    /* Tell the compatibility layer to use this context */
>+    smbc_set_context(context);
>+
>+    int remotehandle = smbc_open(smb_uri, O_RDONLY, 0755);
>+    if (remotehandle < 0) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "smbc_open failed\n");
>+        ret = EPIPE;
>+        goto done;
>+    }
>+
>+    buf = talloc_array(tmp_ctx, uint8_t, SMB_BUFFER_SIZE);
>+    bytesread = smbc_read(remotehandle, buf, SMB_BUFFER_SIZE);
>+    if(bytesread < 0) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "smbc_read failed\n");
>+        ret = EPIPE;
>+        goto done;
>+    }
>+
>+    DEBUG(SSSDBG_CRIT_FAILURE, "bytesread: %d\n", bytesread);
>+
>+    smbc_close(remotehandle);
>+
>+    ret = ad_gpo_parse_security_cse_buffer(tmp_ctx,
>+                                           buf,
>+                                           bytesread,
>+                                           &allowed_sids,
>+                                           &allowed_size,
>+                                           &denied_sids,
>+                                           &denied_size);
    Function ad_gpo_parse_security_cse_buffer can fail in many cases
    ENOMEM, problem with initialising ini_config_*
    and output variable will be uninitialized in next lines.

>+
>+    /* TBD: allowed/denied_sids/size should be stored in cache */
>+
>+    *_allowed_sids = talloc_steal(mem_ctx, allowed_sids);
>+    *_allowed_size = allowed_size;
>+    *_denied_sids = talloc_steal(mem_ctx, denied_sids);
>+    *_denied_size = denied_size;
>+
>+ done:
>+    smbc_free_context(context, 0);
>+    talloc_free(tmp_ctx);
>+    return ret;
>+}
>+
>+int
>+main(int argc, const char *argv[])
>+{
>+    int opt;
>+    poptContext pc;
>+    int debug_fd = -1;
>+    errno_t ret;
>+    int result;
>+    TALLOC_CTX *main_ctx = NULL;
>+    uint8_t *buf = NULL;
>+    ssize_t len = 0;
>+    struct input_buffer *ibuf = NULL;
>+    struct response *resp = NULL;
>+    size_t written;
>+    char **allowed_sids;
>+    int allowed_size;
>+    char **denied_sids;
>+    int denied_size;
>+    int j;
>+
>+    struct poptOption long_options[] = {
>+        POPT_AUTOHELP
>+        {"debug-level", 'd', POPT_ARG_INT, &debug_level, 0,
>+         _("Debug level"), NULL},
>+        {"debug-timestamps", 0, POPT_ARG_INT, &debug_timestamps, 0,
>+         _("Add debug timestamps"), NULL},
>+        {"debug-microseconds", 0, POPT_ARG_INT, &debug_microseconds, 0,
>+         _("Show timestamps with microseconds"), NULL},
>+        {"debug-fd", 0, POPT_ARG_INT, &debug_fd, 0,
>+         _("An open file descriptor for the debug logs"), NULL},
>+        POPT_TABLEEND
>+    };
>+
>+    /* Set debug level to invalid value so we can decide 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);
>+            _exit(-1);
>+        }
>+    }
>+
>+    poptFreeContext(pc);
>+
>+    DEBUG_INIT(debug_level);
>+
>+    debug_prg_name = talloc_asprintf(NULL, "[sssd[gpo_child[%d]]]", getpid());
>+    if (debug_prg_name == NULL) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
>+        ret = ENOMEM;
          ret is not used in fail. You can ammend previous debug message
          if you want.
>+        goto fail;
>+    }
>+
>+    if (debug_fd != -1) {
>+        ret = set_debug_file_from_fd(debug_fd);
>+        if (ret != EOK) {
>+            DEBUG(SSSDBG_CRIT_FAILURE, "set_debug_file_from_fd failed.\n");
>+        }
>+    }
>+
>+    DEBUG(SSSDBG_TRACE_FUNC, "gpo_child started.\n");
>+
>+    main_ctx = talloc_new(NULL);
>+    if (main_ctx == NULL) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new failed.\n");
>+        talloc_free(discard_const(debug_prg_name));
>+        goto fail;
>+    }
>+    talloc_steal(main_ctx, debug_prg_name);
>+
>+    buf = talloc_size(main_ctx, sizeof(uint8_t)*IN_BUF_SIZE);
>+    if (buf == NULL) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
>+        goto fail;
>+    }
>+
>+    ibuf = talloc_zero(main_ctx, struct input_buffer);
>+    if (ibuf == NULL) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n");
>+        goto fail;
>+    }
>+
>+    DEBUG(SSSDBG_TRACE_FUNC, "context initialized\n");
>+
>+    errno = 0;
>+    len = sss_atomic_read_s(STDIN_FILENO, buf, IN_BUF_SIZE);
>+    if (len == -1) {
>+        ret = errno;
>+        DEBUG(SSSDBG_CRIT_FAILURE, "read failed [%d][%s].\n", ret, strerror(ret));
>+        goto fail;
>+    }
>+
>+    close(STDIN_FILENO);
>+
>+    ret = unpack_buffer(buf, len, ibuf);
>+    if (ret != EOK) {
>+        DEBUG(SSSDBG_CRIT_FAILURE,
>+              "unpack_buffer failed.[%d][%s].\n", ret, strerror(ret));
>+        goto fail;
>+    }
>+
>+    DEBUG(SSSDBG_TRACE_FUNC, "processing security settings\n");
>+
>+    result = process_security_settings_cse(main_ctx,
>+                                           ibuf->smb_uri,
>+                                           &allowed_sids,
>+                                           &allowed_size,
>+                                           &denied_sids,
>+                                           &denied_size);
    You should test result of function process_security_settings_cse.
    It may happen that output variables will not be initialised.

>+
>+    DEBUG(SSSDBG_CRIT_FAILURE, "allowed_size = %d\n", allowed_size);
>+    for (j= 0; j < allowed_size; j++) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "allowed_sids[%d] = %s\n", j,
>+              allowed_sids[j]);
>+    }
>+
>+    DEBUG(SSSDBG_CRIT_FAILURE, "denied_size = %d\n", denied_size);
>+    for (j= 0; j < denied_size; j++) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, " denied_sids[%d] = %s\n", j,
>+              denied_sids[j]);
>+    }
>+
>+    result = EOK;
  Is there a reason for this assignment? Will it be solved with my previous
  comment?

>+    ret = prepare_response(main_ctx, result, allowed_size, allowed_sids, denied_size, denied_sids, &resp);
>+    if (ret != EOK) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "prepare_response failed. [%d][%s].\n",
>+                    ret, strerror(ret));
>+        goto fail;
>+    }
>+
>+    errno = 0;
>+    DEBUG(SSSDBG_TRACE_FUNC, "resp->size: %zu\n", resp->size);
>+
>+    written = sss_atomic_write_s(STDOUT_FILENO, resp->buf, resp->size);
>+    if (written == -1) {
>+        ret = errno;
>+        DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
>+                    strerror(ret));
>+        goto fail;
>+    }
>+
>+    if (written != resp->size) {
>+        DEBUG(SSSDBG_CRIT_FAILURE, "Expected to write %zu bytes, wrote %zu\n",
>+              resp->size, written);
>+        goto fail;
>+    }
>+
>+    DEBUG(SSSDBG_TRACE_FUNC, "gpo_child completed successfully\n");
>+    close(STDOUT_FILENO);
>+    talloc_free(main_ctx);
>+    _exit(0);
It will be better to use constant EXIT_SUCCESS

>+
>+fail:
>+    DEBUG(SSSDBG_CRIT_FAILURE, "gpo_child failed!\n");
>+    close(STDOUT_FILENO);
>+    talloc_free(main_ctx);
>+    _exit(-1);
and here EXIT_FAILURE

LS



More information about the sssd-devel mailing list