[SSSD] [PATCHES] IPA: check overrrides for IPA users as well

Jakub Hrozek jhrozek at redhat.com
Fri Nov 14 16:27:29 UTC 2014


On Thu, Nov 13, 2014 at 11:58:13AM +0100, Sumit Bose wrote:

Hi,

Coverity run was clean, CI run succeeded and testing against a recent
IPA server went OK. Before I ack, I also want to test against an old
server, but I'm running out of time today.

See some comments inline.

> From 9f807e92b8db786c331842117e1481dbb494a1f7 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Thu, 6 Nov 2014 13:13:27 +0100
> Subject: [PATCH 1/7] ipa: add split_ipa_anchor()

ACK, see one remark inline.

> +errno_t split_ipa_anchor(TALLOC_CTX *mem_ctx, const char *anchor,
> +                         char **_anchor_domain, char **_ipa_uuid)
> +{
> +    const char *sep;
> +
> +    if (anchor == NULL) {
> +        return EINVAL;
> +    }
> +    if (strncmp(OVERRIDE_ANCHOR_IPA_PREFIX, anchor,
> +                OVERRIDE_ANCHOR_IPA_PREFIX_LEN) != 0) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "No IPA anchor [%s].\n", anchor);
> +        return ENOMSG;

This is fine since we're only using the error as an..error..so the caller
will only test ret != EOK, but if we used it to differentiate between
different conditions, we should use a custom error code.

> +    }
> +
> +    sep = strchr(anchor + OVERRIDE_ANCHOR_IPA_PREFIX_LEN, ':');
> +    if (sep == NULL || sep[1] == '\0') {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Broken IPA anchor [%s].\n", anchor);
> +        return EINVAL;
> +    }

> From ec4cf492945cb8f30fcb2467e9d26836de7ff6d8 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 7 Nov 2014 13:55:01 +0100
> Subject: [PATCH 2/7] LDAP: add support for lookups by UUID
> 
> Related to https://fedorahosted.org/sssd/ticket/2481
> ---
>  src/providers/data_provider.h |  2 ++
>  src/providers/ldap/ldap_id.c  | 58 +++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h
> index e1cb4be..5df493e 100644
> --- a/src/providers/data_provider.h
> +++ b/src/providers/data_provider.h
> @@ -127,6 +127,7 @@
>  #define BE_FILTER_IDNUM 2
>  #define BE_FILTER_ENUM 3
>  #define BE_FILTER_SECID 4
> +#define BE_FILTER_UUID 5
>  
>  #define BE_REQ_USER          0x0001
>  #define BE_REQ_GROUP         0x0002
> @@ -139,6 +140,7 @@
>  #define BE_REQ_HOST          0x0010
>  #define BE_REQ_BY_SECID      0x0011
>  #define BE_REQ_USER_AND_GROUP 0x0012
> +#define BE_REQ_BY_UUID      0x0013
>  #define BE_REQ_TYPE_MASK     0x00FF
>  #define BE_REQ_FAST          0x1000
>  
> diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
> index e8b3a0e..2e58f4e 100644
> --- a/src/providers/ldap/ldap_id.c
> +++ b/src/providers/ldap/ldap_id.c
> @@ -179,6 +179,20 @@ struct tevent_req *users_get_send(TALLOC_CTX *memctx,
>              goto done;
>          }
>          break;
> +    case BE_FILTER_UUID:
> +        attr_name = ctx->opts->user_map[SDAP_AT_USER_UUID].name;
> +        if (attr_name == NULL) {
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  "UUID search not configured for this backend.\n");
> +            ret = EINVAL;

I wonder if failing is appropriate? I guess yes, but on the other hand,
calling an unconfigured back end handler (maybe an SSH handler with a
pure LDAP backend) is mostly a noop, right?

But I can see the point, just wanted to double check with you..

Otherwise ACK

> From 15ea0a84cc1d1ded27c6f84ac563b7902f203526 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 7 Nov 2014 21:33:36 +0100
> Subject: [PATCH 3/7] LDAP: always store UUID if available
> 
> Related to https://fedorahosted.org/sssd/ticket/2481
> ---
>  src/providers/ldap/sdap_async_groups.c | 20 ++++++++++++++++++++
>  src/providers/ldap/sdap_async_users.c  | 19 +++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c
> index a82d2aa..dc1b60d 100644
> --- a/src/providers/ldap/sdap_async_groups.c
> +++ b/src/providers/ldap/sdap_async_groups.c
> @@ -511,6 +511,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
>      bool posix_group;
>      bool use_id_mapping;
>      char *sid_str;
> +    const char *uuid;
>      struct sss_domain_info *subdomain;
>      int32_t ad_group_type;
>  
> @@ -547,6 +548,25 @@ static int sdap_save_group(TALLOC_CTX *memctx,
>          sid_str = NULL;
>      }
>  
> +    /* Always store UUID if available */
> +    ret = sysdb_attrs_get_string(attrs,
> +                                 opts->group_map[SDAP_AT_GROUP_UUID].sys_name,
> +                                 &uuid);
> +    if (ret == EOK) {
> +        ret = sysdb_attrs_add_string(group_attrs, SYSDB_UUID, uuid);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_MINOR_FAILURE, "Could not add UUID string: [%s]\n",
> +                                         strerror(ret));

Please use sss_strerror() in new code (also elsewhere in this patch).

Otherwise ACK

> From 3adf966d906cf491f7ca852bb36921fc05542dbf Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 7 Nov 2014 15:05:41 +0100
> Subject: [PATCH 4/7] ipa: add get_be_acct_req_for_uuid()

ACK

> From 9047069105444545a2f5dc0be3ef30d4c43689d1 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 7 Nov 2014 21:34:55 +0100
> Subject: [PATCH 5/7] IPA: make get_object_from_cache() public

ACK

I just noticed one oddity when reading the code. The function mixes if
and if-else statements, I think it would be nice to add another simple
patch that would converge to if-else style.

> From 9286597c2ed35db04403f72caa75bbfb966cae9b Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Wed, 5 Nov 2014 15:58:04 +0100
> Subject: [PATCH 6/7] IPA: check overrrides for IPA users as well
> 
> Currently overrides were only available for sub-domains, e.g. trusted AD
> domains. With this patch overrides can be used for IPA users as well.
> 
> Related to https://fedorahosted.org/sssd/ticket/2481
> ---

[...]

> +static errno_t ipa_id_get_account_info_get_original_step(struct tevent_req *req,
> +                                                         struct be_acct_req *ar)
> +{
> +    struct ipa_id_get_account_info_state *state = tevent_req_data(req,
> +                                          struct ipa_id_get_account_info_state);
> +    struct tevent_req *subreq;
> +    int ret;

ret is unused and triggers a compilation warning.

> +
> +    subreq = sdap_handle_acct_req_send(state, state->be_req, ar,
> +                                       state->ipa_ctx->sdap_id_ctx,
> +                                       state->ipa_ctx->sdap_id_ctx->opts->sdom,
> +                                       state->ipa_ctx->sdap_id_ctx->conn, true);
> +    if (subreq == NULL) {
> +        DEBUG(SSSDBG_OP_FAILURE, "sdap_handle_acct_req_send failed.\n");
> +        return ENOMEM;
> +    }
> +    tevent_req_set_callback(subreq, ipa_id_get_account_info_orig_done, req);
> +
> +    return EOK;
> +}
> +

The rest looks good.

> From d3d3d51a1d49936c9ad1a3733388bfd384fd07c4 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 7 Nov 2014 21:36:12 +0100
> Subject: [PATCH 7/7] Enable views for all domains

ACK



More information about the sssd-devel mailing list