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

Sumit Bose sbose at redhat.com
Fri Jun 27 12:12:56 UTC 2014


On Wed, Jun 25, 2014 at 11:33:47PM -0400, Yassir Elley wrote:
> 
> 
> ----- Original Message -----
> > On Wed, Jun 18, 2014 at 01:50:17PM -0400, Yassir Elley wrote:
> > > 
> > > 
> > > ----- Original Message -----
> > > > On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
> > > > > 
> > > > > 
> > > > > ----- Original Message -----
> > > > > > 
> > > > > > 
> > > > > > ----- Original Message -----
> > > > > > > On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > * 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.
> > > > > > > > 
> > > > > > > 
> > > > > > > In struct ad_gpo_access_state you have a member struct
> > > > > > > sdap_id_conn_ctx
> > > > > > > *conn. conn->service->uri is the LDAP uri for the current
> > > > > > > connection.
> > > > > > > You can use calls from OpenLDAP or ldb to split it into components,
> > > > > > > picj
> > > > > > > the hostname and create the smb uri.
> > > > > > > 
> > > > > > > In general the uri should always be available since you read the
> > > > > > > GPO
> > > > > > > data from LDAP before doing the smb operations. Nevertheless you
> > > > > > > can
> > > > > > > call be_resolve_server_send() to make sure it is set, see e.g.
> > > > > > > auth_get_server() how to use it.
> > > > > > > 
> > > > > > > HTH
> > > > > > > 
> > > > > > > bye,
> > > > > > > Sumit
> > > > > > > _______________________________________________
> > > > > > > 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 modifies the smb uri to use the
> > > > > > server
> > > > > > name rather than the domain name.
> > > > > > 
> > > > > > Thanks,
> > > > > > Yassir.
> > > > > > _______________________________________________
> > > > > > sssd-devel mailing list
> > > > > > sssd-devel at lists.fedorahosted.org
> > > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > > > 
> > > > > 
> > > > > Oops. Forgot to attach the patch.
> > > > > 
> > > > > Yassir.
> > > > 
> > > > Thank you, the patch is working as expected and now uses the hostname to
> > > > connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to
> > > > split the url and take the hostname from the lud_host member of typedef
> > > > struct ldap_url_desc. The LDAP url can contain port numbers which would
> > > > currently cause troubles with your scheme.
> > > >
> > > > As a general comment, please try to split your patches into smaller
> > > > units. This would help to review them especially to compare multiple
> > > > versions of a patch.
> > > 
> > > I thought you had previously indicated a preference for a single patch
> > > (with the understanding that this makes it more difficult for the
> > > reviewer). Perhaps I misunderstood. In any case, I have attached two
> > > patches to this email: the previous patch, and a new patch to address your
> > > comment about using ldap_url_parse().
> > 
> > ah, sorry, I think I didn't explain it well. My point was to split the
> > patch into smaller self contained unit, e.g. one patch for the smb
> > operation and a second one for parsing the data and extracting the SID
> > lists. Maybe there are even more opportunities to split the patch. Then
> > each patch get its comments and the changes can be squashed into the
> > patch again.
> 
> Ah, that makes sense. Thanks.
> 
> > 
> > > 
> > > > 
> > > > I have not looked at the child code in details yet, but I would like to
> > > > suggest a change in the workflow. I think the child should only download
> > > > the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and
> > > > then the backend will read an process it. This way you already have the
> > > > file available in the offline case. When calling the child the backend
> > > > should provide the smb url and a location to store the result. The child
> > > > can e.g. return a checksum for the file which the backend can save
> > > > together with the download time in the sysdb cache in a subtree below
> > > > cn=custom (grep sysdb.h for 'custom' to find the related sysdb calls).
> > > > With the download time it would be even possible to specific cache
> > > > lifetime during which the gpo file will not be downloaded again to save
> > > > bandwidth. But this should be optional.
> > > > 
> > > > What do you think?
> > > > 
> > > > bye,
> > > > Sumit
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > sssd-devel at lists.fedorahosted.org
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > 
> > > 
> > > With regard to your suggestion to store the policy data in a local file
> > > (and store the file's metadata in the cache), I think it would be
> > > preferrable to have the child parse the file, and store the relevant
> > > data/metadata directly to the cache. I believe this is how IPA HBAC
> > > currently works (i.e. storing to cache, rather than to file). This would
> > > allow the file parsing to be done once (when it is written by the child),
> > > rather than the file parsing being done every time the policy is processed
> > > (when it is read by the backend). Additionally, keep in mind that we are
> > > interested in only a subset of the stanzas (i.e. "Privilege Rights"
> > > stanza) in the Security Settings policy file (GptTmpl.inf). In other
> > > words, only a small portion of the file contains data that is relevant for
> > > us.
> > > 
> > > For this initial patch, I am having the child return the allowed_sids and
> > > denied_sids to the backend for further processing. However, in subsequent
> > > patches, I am planning on having the child store the relevant policy data
> > > to the cache, and then have the backend retrieve the relevant policy data
> > > from the cache. In other words, the proposed logic for future patches is:
> > > * if we are online, the backend calls the child (which stores the parsed
> > > data to cache)
> > > * regardless of whether we are online or offline, the backend retrieves the
> > > parsed data from the cache and makes a policy decision.
> > > 
> > > With regard to reducing unnecessary downloads, I am planning on
> > > implementing the logic suggested by [MS-GPOL] of comparing the cached gpo
> > > version against the current gpo version (by downloading only the GPT.INI
> > > file). If the gpo versions are the same, there is no need to download the
> > > policy file. Again, this is intended for a subsequent patch.
> > > 
> > > Regards,
> > > Yassir.
> > 
> > > From c2047c17b1fc8dc6ab0df337a3a0fa0af9cc22e4 Mon Sep 17 00:00:00 2001
> > > From: Yassir Elley <yelley at redhat.com>
> > > Date: Wed, 18 Jun 2014 13:42:50 -0400
> > > Subject: [PATCH 2/2] Use ldap_url_parse to extract hostname from ldap uri
> > > 
> > 
> > ACK
> > 
> > > From d0e3541cd2c0be7ca6c511bc40dd37fe569c3a29 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 1/2] AD-GPO: Add gpo-smb implementation in gpo_child
> > > process
> > > 
> > > ---
> > 
> > ...
> > 
> > > +/* == ad_gpo_process_cse_send/recv helpers
> > > ================================= */
> > > +
> > > +static errno_t
> > > +create_cse_send_buffer(TALLOC_CTX *mem_ctx,
> > > +                       char *smb_uri,
> > > +                       struct io_buffer **io_buf)
> > > +{
> > > +    struct io_buffer *buf;
> > > +    size_t rp;
> > > +
> > > +    DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
> > > +    DEBUG(SSSDBG_TRACE_FUNC, "strlen(smb_uri): %zu\n", strlen(smb_uri));
> > > +
> > > +    buf = talloc(mem_ctx, struct io_buffer);
> > > +    if (buf == NULL) {
> > > +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
> > > +        return ENOMEM;
> > > +    }
> > > +
> > > +    buf->size = 1 * sizeof(uint32_t);
> > > +    if (smb_uri) {
> > > +        buf->size += strlen(smb_uri);
> > > +    }
> > > +
> > > +    DEBUG(SSSDBG_TRACE_ALL, "buffer size: %zu\n", buf->size);
> > > +
> > > +    buf->data = talloc_size(buf, buf->size);
> > > +    if (buf->data == NULL) {
> > > +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
> > > +        talloc_free(buf);
> > > +        return ENOMEM;
> > > +    }
> > > +
> > > +    rp = 0;
> > > +    /* smb_uri */
> > > +    if (smb_uri) {
> > > +        SAFEALIGN_SET_UINT32(&buf->data[rp], strlen(smb_uri), &rp);
> > > +        safealign_memcpy(&buf->data[rp], smb_uri, strlen(smb_uri), &rp);
> > 
> > you are calling strlen(smb_uri) 4 time. Although it is a highly
> > optimized call it will need some time and since I guess that compilers
> > are not smart enough to optimize this into a single call I would suggest
> > to put the length into variable and calculate it only once.
> > 
> > > +    } else {
> > > +        SAFEALIGN_SET_UINT32(&buf->data[rp], 0, &rp);
> > > +    }
> > > +
> > > +    *io_buf = buf;
> > > +    return EOK;
> > > +}
> > > +
> > > +static errno_t
> > > +parse_gpo_child_response(TALLOC_CTX *mem_ctx,
> > > +                         uint8_t *buf, ssize_t size,
> > > +                         char ***_allowed_sids,
> > > +                         int *_allowed_size,
> > > +                         char ***_denied_sids,
> > > +                         int *_denied_size)
> > > +{
> > > +    size_t p = 0;
> > > +    uint32_t res;
> > > +    int allowed_size = 0;
> > > +    int denied_size = 0;
> > > +    int i = 0;
> > > +    int sid_len = 0;
> > > +    char **allowed_sids;
> > > +    char *allowed_sid;
> > > +    char **denied_sids;
> > > +    char *denied_sid;
> > > +
> > > +    /* operation result code */
> > > +    SAFEALIGN_COPY_UINT32_CHECK(&res, buf + p, size, &p);
> > > +
> > > +    /* allowed_size */
> > > +    SAFEALIGN_COPY_UINT32_CHECK(&allowed_size, buf + p, size, &p);
> > > +    DEBUG(SSSDBG_TRACE_FUNC, "child response allowed_size: %d\n",
> > > allowed_size);
> > > +
> > > +    allowed_sids = talloc_array(mem_ctx, char *, allowed_size + 1);
> > 
> > Why do you need '+ 1'? If you planned a NULL-terminated array you forgot
> > to set the last element to NULL.
> > 
> > > +    if (allowed_sids == NULL) {
> > > +        return ENOMEM;
> > > +    }
> > > +
> > > +    for (i = 0; i < allowed_size; i++) {
> > > +        SAFEALIGN_COPY_UINT32_CHECK(&sid_len, buf + p, size, &p);
> > > +        if ((p + sid_len ) > size) return EINVAL;
> > > +        allowed_sid = talloc_size(mem_ctx, sizeof(char) * (sid_len + 1));
> > > +        if (allowed_sid == NULL) {
> > > +            DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
> > > +            return ENOMEM;
> > > +        }
> > > +        safealign_memcpy(allowed_sid, buf+p, sizeof(char) * sid_len, &p);
> > > +        allowed_sid[sid_len] = '\0';
> > > +
> > > +        allowed_sids[i] = talloc_strdup(allowed_sids, allowed_sid);
> > > +        if (allowed_sids[i] == NULL) {
> > > +            return ENOMEM;
> > > +        }
> > 
> > I think you can simplify this by calling talloc_strndup() to copy the
> > data from the buffer and advance p manually.
> > 
> > > +    }
> > > +
> > > +    /* denied_size */
> > > +    SAFEALIGN_COPY_UINT32_CHECK(&denied_size, buf + p, size, &p);
> > > +    DEBUG(SSSDBG_TRACE_FUNC, "child response denied_size: %d\n",
> > > denied_size);
> > > +
> > > +    denied_sids = talloc_array(mem_ctx, char *, denied_size + 1);
> > 
> > see above
> > 
> > > +    if (denied_sids == NULL) {
> > > +        return ENOMEM;
> > > +    }
> > > +
> > > +    for (i = 0; i < denied_size; i++) {
> > > +        SAFEALIGN_COPY_UINT32_CHECK(&sid_len, buf + p, size, &p);
> > > +        if ((p + sid_len ) > size) return EINVAL;
> > > +        denied_sid = talloc_size(mem_ctx, sizeof(char) * (sid_len + 1));
> > > +        if (denied_sid == NULL) {
> > > +            DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
> > > +            return ENOMEM;
> > > +        }
> > > +        safealign_memcpy(denied_sid, buf+p, sizeof(char) * sid_len, &p);
> > > +        denied_sid[sid_len] = '\0';
> > > +
> > > +        denied_sids[i] = talloc_strdup(denied_sids, denied_sid);
> > > +        if (denied_sids[i] == NULL) {
> > > +            return ENOMEM;
> > > +        }
> > 
> > see above
> > 
> > > +    }
> > > +
> > > +    *_allowed_size = allowed_size;
> > > +    *_allowed_sids = talloc_steal(mem_ctx, allowed_sids);
> > 
> > you already allocated allocated_sids on mem_ctx. Nevertheless I think it
> > would be a good idea to use a tmp_ctx here and free all allocated memory
> > in the case of an error.
> > 
> > > +    *_denied_size = denied_size;
> > > +    *_denied_sids = talloc_steal(mem_ctx, denied_sids);
> > > +
> > > +    return EOK;
> > > +}
> > > +
> > > +
> > 
> > ...
> > 
> > > +static errno_t
> > > +unpack_buffer(uint8_t *buf,
> > > +              size_t size,
> > > +              struct input_buffer *ibuf)
> > > +{
> > > +    size_t p = 0;
> > > +    uint32_t len;
> > > +
> > > +    DEBUG(SSSDBG_TRACE_FUNC, "total buffer size: %zu\n", size);
> > > +
> > > +    /* smb_uri size and length */
> > > +    SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
> > > +
> > > +    DEBUG(SSSDBG_TRACE_FUNC, "smb_uri size: %d\n", len);
> > > +    if (len) {
> > > +        if ((p + len ) > size) return EINVAL;
> > > +        ibuf->smb_uri = talloc_strndup(ibuf, (char *)(buf + p), len);
> > > +        if (ibuf->smb_uri == NULL) return ENOMEM;
> > > +        DEBUG(SSSDBG_TRACE_FUNC, "got smb_uri: %s\n", ibuf->smb_uri);
> > > +        p += len;
> > > +    }
> > 
> > If I see is correctly you continue processing even if there is no
> > smb_uri maybe and error should be returned here if len == 0?
> > 
> > > +
> > > +    return EOK;
> > > +}
> > > +
> > > +
> > > +static errno_t
> > > +pack_buffer(struct response *r,
> > > +            int result,
> > > +            int allowed_size,
> > > +            char **allowed_sids,
> > > +            int denied_size,
> > > +            char **denied_sids)
> > > +{
> > > +    int len = 0;
> > > +    size_t p = 0;
> > > +    int i;
> > > +    int sid_len = 0;
> > > +
> > > +    /* A buffer with the following structure must be created:
> > > +     * int32_t status of the request (required)
> > 
> > uint32_t
> > 
> > > +     * int32_t allowed_size  (required)
> > > +     * uint8_t[allowed_size] (optional if allowed_size == 0)
> > 
> > the description is a bit misleading, you have a uint32_t sid_size,
> > uint8_t[sid_size] and this allowed_size times. I currently do not have a
> > good idea how to write it better.
> > 
> > > +     * int32_t denied_size   (required)
> > > +     * uint8_t[denied_size]  (optional if denied_size == 0)
> > > +     */
> > > +
> > > +    DEBUG(SSSDBG_TRACE_FUNC, "entering pack_buffer\n");
> > > +
> > > +    for (i = 0; i < allowed_size; i++) {
> > > +        len += strlen(allowed_sids[i]);
> > > +    }
> > > +
> > > +    for (i = 0; i < denied_size; i++) {
> > > +        len += strlen(denied_sids[i]);
> > > +    }
> > > +
> > > +    r->size = (3 + allowed_size + denied_size) * sizeof(uint32_t) + len;
> > > +
> > > +    DEBUG(SSSDBG_TRACE_FUNC, "response size: %zu\n",r->size);
> > > +
> > > +    r->buf = talloc_array(r, uint8_t, r->size);
> > > +    if(r->buf == NULL) {
> > > +        return ENOMEM;
> > > +    }
> > > +
> > > +    DEBUG(SSSDBG_TRACE_FUNC,
> > > +          "result [%d] allowed_size [%d] denied_size [%d]\n",
> > > +          result, allowed_size, denied_size);
> > > +
> > > +    /* result */
> > > +    SAFEALIGN_SET_UINT32(&r->buf[p], result, &p);
> > > +
> > > +    /* allowed_size */
> > > +    SAFEALIGN_SET_UINT32(&r->buf[p], allowed_size, &p);
> > > +
> > > +    /* allowed_sids */
> > > +    for (i = 0; i < allowed_size; i++) {
> > > +        sid_len = strlen(allowed_sids[i]);
> > > +        SAFEALIGN_SET_UINT32(&r->buf[p], sid_len, &p);
> > > +        safealign_memcpy(&r->buf[p], allowed_sids[i], sid_len, &p);
> > > +    }
> > > +
> > > +    /* denied_size */
> > > +    SAFEALIGN_SET_UINT32(&r->buf[p], denied_size, &p);
> > > +
> > > +    /* denied_sids */
> > > +    for (i = 0; i < denied_size; i++) {
> > > +        sid_len = strlen(denied_sids[i]);
> > > +        SAFEALIGN_SET_UINT32(&r->buf[p], sid_len, &p);
> > > +        safealign_memcpy(&r->buf[p], denied_sids[i], sid_len, &p);
> > > +    }
> > > +
> > > +    return EOK;
> > > +}
> > > +
> > 
> > ...
> > 
> > > +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");
> > > +        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);
> > > +    if (result != EOK) {
> > > +        DEBUG(SSSDBG_CRIT_FAILURE,
> > > +              "process_security_settings_cse failed.[%d][%s].\n",
> > > +              ret, strerror(ret));
> > > +        goto fail;
> > > +    }
> > > +
> > > +    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]);
> > > +    }
> > > +
> > > +    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);
> > > +    return EXIT_SUCCESS;
> > > +
> > > +fail:
> > > +    DEBUG(SSSDBG_CRIT_FAILURE, "gpo_child failed!\n");
> > > +    close(STDOUT_FILENO);
> > 
> > just a nitpick which needs no fixing. There are 'goto fail' before
> > STDIN_FILENO was closed. So in theory you might want to close this here
> > as well. But since close() will fail if STDIN_FILENO is already closed
> > it is not worth the effort to detect all of this properly since the
> > program exits anyway.
> > 
> > > +    talloc_free(main_ctx);
> > > +    return EXIT_FAILURE;
> > > +}
> > > --
> > > 1.8.5.3
> > > 
> > 
> > bye,
> > Sumit
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > 
> 
> I have attached a revised first patch which addresses the code review comments. I'm not sure if I needed to (since it has already been ACK'ed), but I have also attached an unchanged second patch.

ACK.

I will now look at the permissive mode patch.

bye,
Sumit
> 
> Thanks,
> Yassir.



More information about the sssd-devel mailing list