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

Sumit Bose sbose at redhat.com
Wed Jun 25 15:09:36 UTC 2014


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. 

> 
> > 
> > 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



More information about the sssd-devel mailing list