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

Yassir Elley yelley at redhat.com
Mon Jun 16 23:15:04 UTC 2014



----- Original Message -----
> 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.

Since the new version of libini_config is only needed when libsmbclient is being used, I have added the configure check to samba.m4.

> 
> >* 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
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> 

I have attached a revised patch that addresses Lukas's comments. Only outstanding issue is regarding how to get the name of the DC that SSSD is currently connected to.

Thanks,
Yassir.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AD-GPO-Add-gpo-smb-implementation-in-gpo_child-proce.patch
Type: text/x-patch
Size: 52522 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140616/e6aed18a/attachment.bin>


More information about the sssd-devel mailing list