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

Sumit Bose sbose at redhat.com
Mon Sep 8 15:20:33 UTC 2014


On Mon, Sep 08, 2014 at 11:08:41AM -0400, Yassir Elley wrote:
> 
> 
> ----- Original Message -----
> > On (08/09/14 10:02), Yassir Elley wrote:
> > >----- Original Message -----
> > >> On Fri, Sep 05, 2014 at 06:49:30PM -0400, Yassir Elley wrote:
> > >> > 
> > >> > 
> > >> > ----- Original Message -----
> > >> > > On Wed, Aug 27, 2014 at 05:24:37PM -0400, Yassir Elley wrote:
> > >> > > > Hi,
> > >> > > > 
> > >> > 
> > >> > Thanks for the review! Revised patches attached.
> > >> 
> > >> Thank you, all my concerns are addresses, but I'm afaid I have some new
> > >> comments:
> > >
> > >Regards,
> > >Yassir.
> > 
> > just few nitpicks from static analysers.
> > 
> > >From f619ff819498b2d9eb3f492d9b3c9fac5cfb7be6 Mon Sep 17 00:00:00 2001
> > >From: Yassir Elley <yelley at redhat.com>
> > >Date: Fri, 5 Sep 2014 18:45:50 -0400
> > >Subject: [PATCH 2/2] AD-GPO: processing changes for gpo_map_* options
> > >
> > >---
> > > src/providers/ad/ad_access.c |   3 +-
> > > src/providers/ad/ad_access.h |  13 ++
> > > src/providers/ad/ad_gpo.c    | 442
> > > +++++++++++++++++++++++++++++++++++++++++--
> > > src/providers/ad/ad_gpo.h    |   3 +-
> > > src/providers/ad/ad_init.c   |  21 ++
> > > 5 files changed, 459 insertions(+), 23 deletions(-)
> > >
> > >diff --git a/src/providers/ad/ad_access.c b/src/providers/ad/ad_access.c
> > >index
> > >5b1792223c6e1d9fd6906851e8b32922fa88056d..00b5c9e834c975c538169cb8a12396f6a14fa3fe
> > >100644
> > >--- a/src/providers/ad/ad_access.c
> > >+++ b/src/providers/ad/ad_access.c
> > >@@ -402,7 +402,8 @@ ad_sdap_access_done(struct tevent_req *subreq)
> > >                                 state->be_ctx->ev,
> > >                                 state->domain,
> > >                                 state->ctx,
> > >-                                state->pd->user);
> > >+                                state->pd->user,
> > >+                                state->pd->service);
> > > 
> > >     if (!subreq) {
> > >         tevent_req_error(req, ENOMEM);
> > >diff --git a/src/providers/ad/ad_access.h b/src/providers/ad/ad_access.h
> > >index
> > >60ff5db66c9663e736338cdf7694eb75584c3674..34f5d2d1b6d8641536d2e1b78d686e27a0ff1585
> > >100644
> > >--- a/src/providers/ad/ad_access.h
> > >+++ b/src/providers/ad/ad_access.h
> > >@@ -34,6 +34,19 @@ struct ad_access_ctx {
> > >         GPO_ACCESS_CONTROL_ENFORCING
> > >     } gpo_access_control_mode;
> > >     int gpo_cache_timeout;
> > >+    /* supported GPO map options */
> > >+    enum gpo_map_type {
> > >+        GPO_MAP_INTERACTIVE = 0,
> > >+        GPO_MAP_REMOTE_INTERACTIVE,
> > >+        GPO_MAP_NETWORK,
> > >+        GPO_MAP_BATCH,
> > >+        GPO_MAP_SERVICE,
> > >+        GPO_MAP_PERMIT,
> > >+        GPO_MAP_DENY,
> > >+        GPO_MAP_NUM_OPTS
> > >+    } gpo_map_type;
> > >+    hash_table_t *gpo_map_options_table;
> > >+    enum gpo_map_type gpo_default_right;
> > > };
> > > 
> > > void
> > >diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
> > >index
> > >e0905a2df6fb8a4414b10e16dbdf9bb2258dad7e..8b74eeed7b7e70510e54f26ba80ed463e5eaeed0
> > >100644
> > >--- a/src/providers/ad/ad_gpo.c
> > >+++ b/src/providers/ad/ad_gpo.c
> > //snip
> > >+
> > >+errno_t
> > >+ad_gpo_parse_map_option_helper(enum gpo_map_type gpo_map_type,
> > >+                               hash_key_t key,
> > >+                               hash_table_t *options_table)
> > >+{
> > >+    hash_value_t val;
> > >+    int hret;
> > >+    int ret;
> > >+
> > >+    hret = hash_lookup(options_table, &key, &val);
> > >+    if (hret != HASH_SUCCESS && hret != HASH_ERROR_KEY_NOT_FOUND) {
> > >+        DEBUG(SSSDBG_OP_FAILURE, "Error checking hash table: [%s]\n",
> > >+              hash_error_string(hret));
> > >+        ret = EINVAL;
> > >+        goto done;
> > >+    } else if (hret == HASH_SUCCESS) {
> > >+        /* handle unexpected case where mapping for key already exists */
> > >+        if (val.i == gpo_map_type) {
> > >+            /* mapping for key exists for same map type; no error */
> > >+            DEBUG(SSSDBG_TRACE_FUNC,
> > >+                  "PAM service %s maps to %s multiple times\n", key.str,
> > >+                  gpo_map_type_string(gpo_map_type));
> > >+            ret = EOK;
> > >+        } else {
> > >+            /* mapping for key exists for different map type; error! */
> > >+            DEBUG(SSSDBG_CRIT_FAILURE,
> > >+                  "PAM service %s maps to both %s and %s\n", key.str,
> > >+                  gpo_map_type_string(val.i),
> > >gpo_map_type_string(gpo_map_type));
> > >+            ret = EINVAL;
> > >+        }
> > >+        goto done;
> > >+    } else {
> > >+        /* handle expected case where mapping for key doesn't already exist
> > >*/
> > >+        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));
> > >+            ret = EIO;
> > >+        }
> >           EIO is ignored here.
> >           missing goto done.
> > >+        ret = EOK;
> > >+    }
> > >+
> > >+done:
> > >+    return ret;
> > >+}
> > >+
> > 
> > //snip
> > 
> > >+
> > >+    hret = hash_lookup(ctx->gpo_map_options_table, &key, &val);
> > >+    if (hret != HASH_SUCCESS && hret != HASH_ERROR_KEY_NOT_FOUND) {
> > >+        DEBUG(SSSDBG_OP_FAILURE, "Error checking hash table: [%s]\n",
> > >+              hash_error_string(hret));
> > >+        ret = EINVAL;
> > >+        goto immediately;
> > >+    }
> > >+
> > >+    /* if service isn't mapped, map it to value of ad_gpo_default_right
> > >option */
> > >+    if (hret == HASH_ERROR_KEY_NOT_FOUND) {
> > >+        DEBUG(SSSDBG_TRACE_FUNC, "using default right\n");
> > >+        gpo_map_type = ctx->gpo_default_right;
> > >+    } else {
> > >+        gpo_map_type = val.i;
> > Could you use explicit casting?
> > -        gpo_map_type = val.i;
> > +        gpo_map_type = (enum gpo_map_type) val.i;
> > 
> > It will silence warning:
> > Error: PW.MIXED_ENUM_TYPE: [#def2]
> > sssd-1.12.1/src/providers/ad/ad_gpo.c:1257: mixed_enum_type: enumerated type
> > mixed with another type
> > 
> > >+    }
> > >+
> > >+    DEBUG(SSSDBG_TRACE_FUNC, "service %s maps to %s\n", service,
> > >+          gpo_map_type_string(gpo_map_type));
> > >+
> > >+    if (gpo_map_type == GPO_MAP_PERMIT) {
> > >+        ret = EOK;
> > >+        tevent_req_done(req);
> > >+        tevent_req_post(req, ev);
> > >+        goto immediately;
> > >+    }
> > 
> > LS
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > 
> 
> Revised patches attached.

All of Lukas' and my concerns are addressed, ACK.

bye,
Sumit

> 
> Regards,
> Yassir.



More information about the sssd-devel mailing list