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) {
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel