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(a)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(a)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