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