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 <yel...@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 <yel...@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@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
