[SSSD] [PATCH] AD: add config and processing support gpo_map_* options

Sumit Bose sbose at redhat.com
Fri Sep 5 14:01:06 UTC 2014


On Wed, Aug 27, 2014 at 05:24:37PM -0400, Yassir Elley wrote:
> Hi,
> 
> The attached patches add configuring support (patch 1) and processing support (patch 2) for the gpo_map_* set of options. These options are used to map Linux PAM service names to GPO Logon Rights, which in turn map to specific key/value pairs (one for allow, one for deny) in the GptTmpl.inf file. I'm including my write-up (below) from the Design Page (https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryGPOIntegration)
> 
> Thanks,
> Yassir.
> 
> <<

Thank you, the patches work find in general although I have to admit
that I didn't test a large amount of different configuration.

Please find comments below. As a general comment to the man pages, I
think you missed to explain that '+' and '-' have to be used together
with the service name to add or delete it to a specific map.
Additionally I think it is not clear from reading the man page (in
contrast to the design page) that the ad_gpo_map_* options do not define
the list but only add or remove services from the default list.

bye,
Sumit


> +                        <para>
> +                            This option defines how access control is evaluated
> +                            for PAM service names that are not explicitly listed
> +                            in one of the ad_gpo_map_* options. This option can be
> +                            set in two different manners. First, this option can
> +                            be set to use a default logon right. For example, if
> +                            this option is set to 'interactive', it means that 

here is a trailing whitespace                                                   ^

> +                            unmapped PAM service names will be processed based on
> +                            the InteractiveLogonRight and DenyInteractiveLogonRight
> +                            policy settings. Alternatively, this option can be set
> +                            to either always permit or always deny access for
> +                            unmapped PAM service names.
> +                        </para>

...

> +
> +errno_t
> +ad_gpo_parse_map_option(TALLOC_CTX *mem_ctx,
> +                        enum gpo_map_type gpo_map_type,
> +                        hash_table_t *options_table,
> +                        char *conf_str,
> +                        const char **defaults)
> +{
> +    TALLOC_CTX *tmp_ctx;
> +    errno_t ret;
> +    char **conf_list = NULL;
> +    int conf_list_size = 0;
> +    char **allow = NULL;
> +    char **deny = NULL;
> +    int ai = 0, di = 0, li = 0;
> +    int i;
> +    hash_key_t key;
> +    hash_value_t val;
> +    int hret;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (tmp_ctx == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    DEBUG(SSSDBG_TRACE_FUNC, "gpo_map_type: %s\n",
> +          gpo_map_type_string(gpo_map_type));
> +
> +    if (conf_str) {
> +        ret = split_on_separator(tmp_ctx, conf_str, ',', true, true,
> +                                 &conf_list, &conf_list_size);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_OP_FAILURE,
> +                  "Cannot parse list of service names %s: %d\n", conf_str, ret);
> +            ret = EINVAL;
> +            goto done;
> +        }
> +
> +        allow = talloc_zero_array(tmp_ctx, char *, conf_list_size);
> +        deny = talloc_zero_array(tmp_ctx, char *, conf_list_size);
> +        if (allow == NULL || deny == NULL) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +    }
> +
> +    for (i = 0; i < conf_list_size; i++) {
> +        switch (conf_list[i][0]) {
> +        case '+':
> +            allow[ai] = conf_list[i] + 1;
> +            ai++;
> +            continue;
> +        case '-':
> +            deny[di] = conf_list[i] + 1;
> +            di++;
> +            continue;
> +        default:
> +            DEBUG(SSSDBG_CRIT_FAILURE, "ad_gpo_map values must start with "
> +                  "either '+' (allow) or '-' (deny), got '%s'\n",

For me 'allow' and 'deny' are a bit strange here. If I understand to
correctly it is about adding or removing a service for the map of a
specific login right.

> +                  conf_list[i]);
> +            ret = EINVAL;
> +            goto done;
> +        }
> +    }
> +
> +    /* Start by adding explicitly allowed attributes to hashtable */
> +    for (i = 0; i < ai; i++) {
> +        /* if the attribute is explicitly denied, skip it */
> +        if (ad_gpo_attr_in_list(deny, di, allow[i])) {
> +            continue;
> +        }
> +
> +        key.type = HASH_KEY_STRING;
> +        key.str = (char *)allow[i];
> +
> +        DEBUG(SSSDBG_TRACE_FUNC, "Explicitly allowed service: %s\n", key.str);
> +
> +        /* make sure mapping for key does not already exist */

I think you should only error out if the key exists and has a different
gpo_map_type.

> +        hret = hash_lookup(options_table, &key, &val);
> +        if (hret != HASH_ERROR_KEY_NOT_FOUND) {
> +            ret = EINVAL;
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  "Duplicate GPO mapping found for pam service %s\n.", key.str);
> +            goto done;
> +        }
> +
> +        val.type = HASH_VALUE_INT;
> +        val.i = gpo_map_type;
> +
> +        hret = hash_enter(options_table, &key, &val);
> +        if (hret != HASH_SUCCESS) {
> +            DEBUG(SSSDBG_OP_FAILURE, "Error checking hash table: [%s]\n",
> +                  hash_error_string(hret));
> +        }
> +
> +        li++;
> +
> +    }
> +
> +    /* Add defaults to hashtable */
> +    for (i = 0; defaults[i]; i++) {
> +        /* if the attribute is explicitly denied, skip it */
> +        if (ad_gpo_attr_in_list(deny, di, defaults[i])) {
> +            continue;
> +        }
> +
> +        key.type = HASH_KEY_STRING;
> +        key.str = talloc_strdup(mem_ctx, defaults[i]);
> +
> +
> +        DEBUG(SSSDBG_TRACE_FUNC, "Default service (not explicitly denied): %s\n",
> +              key.str);
> +
> +        /* make sure mapping for key does not already exist */
> +        hret = hash_lookup(options_table, &key, &val);
> +        if (hret != HASH_ERROR_KEY_NOT_FOUND) {
> +            ret = EINVAL;
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  "Duplicate GPO mapping found for pam service %s\n.", key.str);
> +            goto done;
> +        }
> +
> +        val.type = HASH_VALUE_INT;
> +        val.i = gpo_map_type;
> +
> +        hret = hash_enter(options_table, &key, &val);
> +
> +        li++;
> +
> +        DEBUG(SSSDBG_TRACE_INTERNAL,
> +              "Added default attr %s to whitelist\n", defaults[i]);
> +    }
> +
> +    DEBUG(SSSDBG_TRACE_FUNC, "num_service_names: %d\n", li);
> +
> +    ret = EOK;
> +done:
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}


> +
> +errno_t
> +ad_gpo_parse_map_options(struct ad_access_ctx *access_ctx)
> +{
> +    char *gpo_default_right_config;
> +    enum gpo_map_type gpo_default_right;
> +    errno_t ret;
> +    int i;
> +
> +    for (i = 0; i < GPO_MAP_NUM_OPTS; i++) {
> +
> +        struct gpo_map_option_entry entry = gpo_map_option_entries[i];
> +
> +        char *entry_config =  dp_opt_get_string(access_ctx->ad_options,
> +                                                entry.ad_basic_opt);
> +
> +        ret = ad_gpo_parse_map_option(access_ctx, entry.gpo_map_type,
> +                                      access_ctx->gpo_map_options_table,
> +                                      entry_config, entry.gpo_map_defaults);

ret is not checked here. As a result an invalid configuration where a
service is added to two different maps is not detected. I would suggest
to terminate SSSD here because the configuration is invalid and it is
not clear to the admin to which login right the service is added.

> +
> +        DEBUG(SSSDBG_TRACE_FUNC, "num_opts = %d\n", i);
> +
> +    }
> +
> +    /* default right (applicable for services without any mapping) */
> +    gpo_default_right_config =
> +        dp_opt_get_string(access_ctx->ad_options, AD_GPO_DEFAULT_RIGHT);
> +
> +    DEBUG(SSSDBG_TRACE_FUNC, "gpo_default_right_config: %s\n",
> +          gpo_default_right_config);
> +
> +    /* if default right not set in config, set them to DENY */
> +    if (gpo_default_right_config == NULL) {
> +        gpo_default_right = GPO_MAP_DENY;
> +    } else if (strncasecmp(gpo_default_right_config, "interactive",
> +                           strlen("interactive")) == 0) {
> +        gpo_default_right = GPO_MAP_INTERACTIVE;
> +    } else if (strncasecmp(gpo_default_right_config, "remote_interactive",
> +                           strlen("remote_interactive")) == 0) {
> +        gpo_default_right = GPO_MAP_REMOTE_INTERACTIVE;
> +    } else if (strncasecmp(gpo_default_right_config, "network",
> +                           strlen("network")) == 0) {
> +        gpo_default_right = GPO_MAP_NETWORK;
> +    } else if (strncasecmp(gpo_default_right_config, "batch",
> +                           strlen("batch")) == 0) {
> +        gpo_default_right = GPO_MAP_BATCH;
> +    } else if (strncasecmp(gpo_default_right_config, "service",
> +                           strlen("service")) == 0) {
> +        gpo_default_right = GPO_MAP_SERVICE;
> +    } else if (strncasecmp(gpo_default_right_config, "permit",
> +                           strlen("permit")) == 0) {
> +        gpo_default_right = GPO_MAP_PERMIT;
> +    } else if (strncasecmp(gpo_default_right_config, "deny",
> +                           strlen("deny")) == 0) {
> +        gpo_default_right = GPO_MAP_DENY;
> +    } else {
> +        ret = EINVAL;
> +        goto fail;
> +    }
> +
> +    DEBUG(SSSDBG_TRACE_FUNC, "gpo_default_right: %d\n", gpo_default_right);
> +    access_ctx->gpo_default_right = gpo_default_right;
> +
> +fail:
> +    return ret;
> +}

...

> @@ -3229,6 +3582,7 @@ parse_logon_right_with_libini(TALLOC_CTX *mem_ctx,
>  static errno_t
>  ad_gpo_parse_policy_file(TALLOC_CTX *mem_ctx,
>                           const char *filename,
> +                         enum gpo_map_type gpo_map_type,
>                           char ***allowed_sids,
>                           int *allowed_size,
>                           char ***denied_sids,
> @@ -3271,7 +3625,27 @@ ad_gpo_parse_policy_file(TALLOC_CTX *mem_ctx,
>          goto done;
>      }
>  
> -    key = ALLOW_LOGON_LOCALLY;
> +    switch (gpo_map_type) {
> +    case GPO_MAP_INTERACTIVE:
> +        key = ALLOW_LOGON_INTERACTIVE;
> +        break;
> +    case GPO_MAP_REMOTE_INTERACTIVE:
> +        key = ALLOW_LOGON_REMOTE_INTERACTIVE;
> +        break;
> +    case GPO_MAP_NETWORK:
> +        key = ALLOW_LOGON_NETWORK;
> +        break;
> +    case GPO_MAP_BATCH:
> +        key = ALLOW_LOGON_BATCH;
> +        break;
> +    case GPO_MAP_SERVICE:
> +        key = ALLOW_LOGON_SERVICE;
> +        break;
> +    default:
> +        ret = EINVAL;
> +        goto done;
> +    }
> +
>      ret = parse_logon_right_with_libini(tmp_ctx,
>                                          ini_config,
>                                          key,
> @@ -3284,10 +3658,30 @@ ad_gpo_parse_policy_file(TALLOC_CTX *mem_ctx,
>          goto done;
>      }
>  
> -    key = DENY_LOGON_LOCALLY;
> +    switch (gpo_map_type) {
> +    case GPO_MAP_INTERACTIVE:
> +        key = DENY_LOGON_INTERACTIVE;
> +        break;
> +    case GPO_MAP_REMOTE_INTERACTIVE:
> +        key = DENY_LOGON_REMOTE_INTERACTIVE;
> +        break;
> +    case GPO_MAP_NETWORK:
> +        key = DENY_LOGON_NETWORK;
> +        break;
> +    case GPO_MAP_BATCH:
> +        key = DENY_LOGON_BATCH;
> +        break;
> +    case GPO_MAP_SERVICE:
> +        key = DENY_LOGON_SERVICE;
> +        break;
> +    default:
> +        ret = EINVAL;
> +        goto done;
> +    }

I wonder if you can fold those two switch-blocks into one by introducing
a new variable like deny_key?

> +
>      ret = parse_logon_right_with_libini(tmp_ctx,
>                                          ini_config,
> -                                        DENY_LOGON_LOCALLY,
> +                                        key,
>                                          &deny_size,
>                                          &deny_sids);
>      if (ret != 0) {



More information about the sssd-devel mailing list