On Tue, Mar 11, 2014 at 07:09:42PM +0200, Alexander Bokovoy wrote:
> Hi,
> 
> 
> Add idmap_cache calls to ipa-sam to prevent huge numbers of LDAP calls
> to the
> directory service for gid/uid<->sid resolution.
> 
> Additionally, this patch further reduces number of queries by:
>  - fast fail on uidNumber=0 which doesn't exist in FreeIPA,
>  - return fallback group correctly when looking up user primary group as is
>    done during init,
>  - checking for group objectclass in case insensitive way
> 
> Based on the patch by Jason Woods <de...@jasonwoods.me.uk>
> 
> https://fedorahosted.org/freeipa/ticket/4234
> and
> https://bugzilla.redhat.com/show_bug.cgi?id=1073829
> https://bugzilla.redhat.com/show_bug.cgi?id=1074314

I didn't had a chance to run some test so far, but here are my comments
for the code. I will run some tests tomorrow.

bye,
Sumit

> 
> -- 
> / Alexander Bokovoy

> >From de5e03f7f7bf707c00b11569998b68b5c87744ed Mon Sep 17 00:00:00 2001
> From: Alexander Bokovoy <aboko...@redhat.com>
> Date: Fri, 7 Mar 2014 16:38:24 +0000
> Subject: [PATCH 2/2] ipa-sam: cache gid to sid and uid to sid requests in
>  idmap cache
> 
> Add idmap_cache calls to ipa-sam to prevent huge numbers of LDAP calls to the
> directory service for gid/uid<->sid resolution.
> 
> Additionally, this patch further reduces number of queries by:
>  - fast fail on uidNumber=0 which doesn't exist in FreeIPA,
>  - return fallback group correctly when looking up user primary group as is
>    done during init,
>  - checking for group objectclass in case insensitive way

I think this list and the ticket list below would justify a split in at
least two patches.

> 
> Based on the patch by Jason Woods <de...@jasonwoods.me.uk>
> 
> https://fedorahosted.org/freeipa/ticket/4234
> and
> https://bugzilla.redhat.com/show_bug.cgi?id=1073829
> https://bugzilla.redhat.com/show_bug.cgi?id=1074314
> ---
>  daemons/ipa-sam/ipa_sam.c | 125 
> +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 113 insertions(+), 12 deletions(-)
> 
> diff --git a/daemons/ipa-sam/ipa_sam.c b/daemons/ipa-sam/ipa_sam.c
> index 7a8eeb4..4eee3a6 100644
> --- a/daemons/ipa-sam/ipa_sam.c
> +++ b/daemons/ipa-sam/ipa_sam.c
> @@ -82,6 +82,28 @@ struct trustAuthInOutBlob {
>       struct AuthenticationInformationArray previous;/* 
> [subcontext(0),flag(LIBNDR_FLAG_REMAINING)] */
>  }/* [gensize,public,nopush] */;
>  
> +/* from generated idmap.h - hopefully OK */
> +enum id_type
> +#ifndef USE_UINT_ENUMS

I think we do not need this Samba-ism here becasue we do not define
USE_UINT_ENUMS anyways. Enums are fine imo, if you prefer defines I
dont't mind. Btw, USE_UINT_ENUMS is not available in Samba either
https://bugzilla.samba.org/show_bug.cgi?id=10477 :-)

> + {
> +     ID_TYPE_NOT_SPECIFIED,
> +     ID_TYPE_UID,
> +     ID_TYPE_GID,
> +     ID_TYPE_BOTH
> +}
> +#else
> + { __donnot_use_enum_id_type=0x7FFFFFFF}
> +#define ID_TYPE_NOT_SPECIFIED ( 0 )
> +#define ID_TYPE_UID ( 1 )
> +#define ID_TYPE_GID ( 2 )
> +#define ID_TYPE_BOTH ( 3 )
> +#endif
> +;
> +
> +struct unixid {
> +     uint32_t id;
> +     enum id_type type;
> +}/* [public] */;
>  
>  enum ndr_err_code ndr_pull_trustAuthInOutBlob(struct ndr_pull *ndr, int 
> ndr_flags, struct trustAuthInOutBlob *r); /*available in libndr-samba.so */
>  bool sid_check_is_builtin(const struct dom_sid *sid); /* available in 
> libpdb.so */
> @@ -91,6 +113,7 @@ char *sid_string_talloc(TALLOC_CTX *mem_ctx, const struct 
> dom_sid *sid); /* avai
>  char *sid_string_dbg(const struct dom_sid *sid); /* available in 
> libsmbconf.so */
>  char *escape_ldap_string(TALLOC_CTX *mem_ctx, const char *s); /* available 
> in libsmbconf.so */
>  bool secrets_store(const char *key, const void *data, size_t size); /* 
> available in libpdb.so */
> +void idmap_cache_set_sid2unixid(const struct dom_sid *sid, struct unixid 
> *unix_id); /* available in libsmbconf.so */
>  
>  #define LDAP_PAGE_SIZE 1024
>  #define LDAP_OBJ_SAMBASAMACCOUNT "ipaNTUserAttrs"
> @@ -750,8 +773,8 @@ static bool ldapsam_sid_to_id(struct pdb_methods *methods,
>       }
>  
>       for (c = 0; values[c] != NULL; c++) {
> -             if (strncmp(LDAP_OBJ_GROUPMAP, values[c]->bv_val,
> -                                            values[c]->bv_len) == 0) {
> +             if (strncasecmp(LDAP_OBJ_GROUPMAP, values[c]->bv_val,
> +                                                values[c]->bv_len) == 0) {
>                       break;
>               }
>       }
> @@ -769,6 +792,9 @@ static bool ldapsam_sid_to_id(struct pdb_methods *methods,
>               }
>  
>               unixid_from_gid(id, strtoul(gid_str, NULL, 10));
> +
> +             idmap_cache_set_sid2unixid(sid, id);
> +
>               ret = true;
>               goto done;
>       }
> @@ -785,8 +811,11 @@ static bool ldapsam_sid_to_id(struct pdb_methods 
> *methods,
>  
>       unixid_from_uid(id, strtoul(value, NULL, 10));
>  
> +     idmap_cache_set_sid2unixid(sid, id);
> +
>       ret = true;
>   done:
> +
>       TALLOC_FREE(mem_ctx);
>       return ret;
>  }
> @@ -806,6 +835,18 @@ static bool ldapsam_uid_to_sid(struct pdb_methods 
> *methods, uid_t uid,
>       int rc;
>       enum idmap_error_code err;
>       TALLOC_CTX *tmp_ctx = talloc_stackframe();
> +     struct unixid id;
> +
> +     /* Fast fail if we get a request for uidNumber=0 because it currently
> +      * will never exist in the directory
> +      * Saves an expensive LDAP call of which failure will never be cached
> +      */
> +     if (uid == 0) {
> +             DEBUG(3, ("ERROR: Received request for uid %u, "
> +                       "fast failing as it will never exist\n",
> +                       (unsigned int)uid));
> +             goto done;
> +     }
>  
>       filter = talloc_asprintf(tmp_ctx,
>                                "(&(uidNumber=%u)"
> @@ -852,6 +893,10 @@ static bool ldapsam_uid_to_sid(struct pdb_methods 
> *methods, uid_t uid,
>  
>       sid_copy(sid, user_sid);
>  
> +     unixid_from_uid(&id, uid);
> +
> +     idmap_cache_set_sid2unixid(sid, &id);
> +
>       ret = true;
>  
>  done:
> @@ -866,21 +911,30 @@ static bool ldapsam_gid_to_sid(struct pdb_methods 
> *methods, gid_t gid,
>       struct ldapsam_privates *priv =
>               (struct ldapsam_privates *)methods->private_data;
>       char *filter;
> -     const char *attrs[] = { LDAP_ATTRIBUTE_SID, NULL };
> +     const char *attrs[] = { LDAP_ATTRIBUTE_SID, LDAP_ATTRIBUTE_OBJECTCLASS, 
> NULL };
>       LDAPMessage *result = NULL;
>       LDAPMessage *entry = NULL;
>       bool ret = false;
> -     char *group_sid_string;
> +     char *group_sid_string = NULL;
>       struct dom_sid *group_sid = NULL;
> +     struct berval **values;
> +     size_t c;
>       int rc;
>       enum idmap_error_code err;
>       TALLOC_CTX *tmp_ctx = talloc_stackframe();
> +     struct unixid id;
>  
>       filter = talloc_asprintf(tmp_ctx,
> -                              "(&(gidNumber=%u)"
> -                              "(objectClass=%s))",
> +                              "(|(&(gidNumber=%u)"
> +                              "(objectClass=%s))"
> +                              "(&(uidNumber=%u)"
> +                              "(objectClass=%s)"
> +                              "(objectClass=%s)))",
> +                              (unsigned int)gid,

Can you add some indentation to make the grouping in the search filter
more clear?

> +                              LDAP_OBJ_GROUPMAP,
>                                (unsigned int)gid,
> -                              LDAP_OBJ_GROUPMAP);
> +                              LDAP_OBJ_POSIXACCOUNT,
> +                              LDAP_OBJ_SAMBASAMACCOUNT);
>       if (filter == NULL) {
>               DEBUG(3, ("talloc_asprintf failed\n"));
>               goto done;
> @@ -892,14 +946,46 @@ static bool ldapsam_gid_to_sid(struct pdb_methods 
> *methods, gid_t gid,
>       }
>       smbldap_talloc_autofree_ldapmsg(tmp_ctx, result);
>  
> -     if (ldap_count_entries(priv2ld(priv), result) != 1) {
> -             DEBUG(3, ("ERROR: Got %d entries for gid %u, expected one\n",
> +     if (ldap_count_entries(priv2ld(priv), result) == 0) {
> +             DEBUG(3, ("ERROR: Got %d entries for gid %u, expected at least 
> one\n",
>                          ldap_count_entries(priv2ld(priv), result),
>                          (unsigned int)gid));
>               goto done;
>       }
>  
> -     entry = ldap_first_entry(priv2ld(priv), result);
> +     for (entry = ldap_first_entry(priv2ld(priv), result);
> +              entry != NULL;
> +              entry = ldap_next_entry(priv2ld(priv), entry)) {
> +
> +             values = ldap_get_values_len(priv2ld(priv), entry, 
> "objectClass");
> +             if (values == NULL) {
> +                     DEBUG(10, ("Cannot find any objectclasses.\n"));
> +                     goto done;
> +             }
> +
> +             for (c = 0; values[c] != NULL; c++) {
> +                     if (strncasecmp(LDAP_OBJ_GROUPMAP, values[c]->bv_val,
> +                                                        values[c]->bv_len) 
> == 0) {
> +                             goto found;
> +                     }
> +             }
> +
> +     }
> +
> +found:
> +     // If we didn't find a group we found a user - so this is a primary 
> group
> +     // For user private group, use fallback group

iirc the coding style says that we do not like C++ style comments.

> +     if (entry == NULL) {
> +
> +             DEBUG(10, ("Did not find user private group %u, "
> +                        "returning fallback group.\n", (unsigned int)gid));
> +
> +             sid_copy(sid,
> +                      &priv->ipasam_privates->fallback_primary_group);
> +             ret = true;
> +             goto done;
> +
> +     }
>  
>       group_sid_string = get_single_attribute(tmp_ctx, priv2ld(priv), entry,
>                                               LDAP_ATTRIBUTE_SID);
> @@ -910,7 +996,7 @@ static bool ldapsam_gid_to_sid(struct pdb_methods 
> *methods, gid_t gid,
>       }
>  
>       err = sss_idmap_sid_to_smb_sid(priv->ipasam_privates->idmap_ctx,
> -                                    group_sid_string, &group_sid);
> +                                        group_sid_string, &group_sid);
>       if (err != IDMAP_SUCCESS) {
>               DEBUG(3, ("Error calling sid_string_talloc for sid '%s'\n",
>                         group_sid_string));
> @@ -919,6 +1005,10 @@ static bool ldapsam_gid_to_sid(struct pdb_methods 
> *methods, gid_t gid,
>  
>       sid_copy(sid, group_sid);
>  
> +     unixid_from_gid(&id, gid);
> +
> +     idmap_cache_set_sid2unixid(sid, &id);
> +
>       ret = true;
>  
>  done:
> @@ -2865,6 +2955,7 @@ static int ipasam_get_sid_by_gid(struct 
> ldapsam_privates *ldap_state,
>       struct dom_sid *sid = NULL;
>       int count;
>       enum idmap_error_code err;
> +     struct unixid id;
>  
>       tmp_ctx = talloc_new("ipasam_get_sid_by_gid");
>       if (tmp_ctx == NULL) {
> @@ -2919,6 +3010,10 @@ static int ipasam_get_sid_by_gid(struct 
> ldapsam_privates *ldap_state,
>       }
>       sid_copy(_sid, sid);
>  
> +     unixid_from_gid(&id, gid);
> +
> +     idmap_cache_set_sid2unixid(sid, &id);
> +
>       ret = 0;
>  
>  done:
> @@ -2938,6 +3033,7 @@ static int ipasam_get_primary_group_sid(TALLOC_CTX 
> *mem_ctx,
>       uint32_t uid;
>       uint32_t gid;
>       struct dom_sid *group_sid;
> +     struct unixid id;
>  
>       TALLOC_CTX *tmp_ctx = talloc_init("ipasam_get_primary_group_sid");
>       if (tmp_ctx == NULL) {
> @@ -2976,8 +3072,13 @@ static int ipasam_get_primary_group_sid(TALLOC_CTX 
> *mem_ctx,
>               }
>       }
>  
> -        ret = 0;
> +     unixid_from_gid(&id, gid);
> +
> +     idmap_cache_set_sid2unixid(group_sid, &id);
> +
> +     ret = 0;
>  done:
> +
>       if (ret == 0) {
>               *_group_sid = talloc_steal(mem_ctx, group_sid);
>       }
> -- 
> 1.8.3.1
> 

> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to