[SSSD] NSS: Add config option to expand homedir template format

Pavel Reichl preichl at redhat.com
Wed Mar 12 13:52:46 UTC 2014


On Tue, 2014-03-04 at 10:29 +0100, Lukas Slebodnik wrote:
> On (03/03/14 13:35), Lukas Slebodnik wrote:
> >On (31/01/14 23:58), Jakub Hrozek wrote:
> >>On Fri, Jan 31, 2014 at 11:55:25AM +0100, Pavel Březina wrote:
> >>> On 12/02/2013 04:12 PM, Lukas Slebodnik wrote:
> >>> >ehlo,
> >>> >
> >>> >attached patches address ticket #1853
> >>> >
> >>> >LS
> >>> 
> >>> Hi,
> >>> these patches no longer apply.
> >>
> >>for patches targeting 1.12, i would prefer to rebase them on top of Nikolai's
> >                       ^^^^
> >                       1.11
> >>and Stephen's logging patches, those are the first to go to master now.
> >
> >Rebased patches are attached,
> >
> >LS
> 
> Sending rebased patches on top of "ipa-server-mode: use lower-case user name
> for home dir"
> 
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Hello Lukas,

I'm afraid I have some troubles witch patches:

ipa user-find test_home
--------------
1 user matched
--------------
  User login: test_home
  First name: test
  Last name: home
  Home directory: %H/test_home
  Login shell: /bin/bash
  Email address: test_home at ipa.work
  UID: 1994000007
  GID: 1994000007
  Account disabled: False
  Password: False
  Kerberos keys available: False
----------------------------
Number of entries returned 1
----------------------------
# getent passwd test_home
test_home:*:1994000007:1994000007:test home:/:/bin/bash]

I suppose homedir should rather be /home/test_home then /.


> 
> @@ -318,6 +316,7 @@ static int fill_pwent(struct sss_packet *packet,
>      bool packet_initialized = false;
>      int ncret;
>      TALLOC_CTX *tmp_ctx = NULL;
> +    struct sss_nss_homedir_ctx *homedir_ctx;
>  
>      to_sized_string(&pwfield, nctx->pwfield);
>  
> @@ -373,7 +372,19 @@ static int fill_pwent(struct sss_packet *packet,
>          } else {
>              to_sized_string(&gecos, tmpstr);
>          }
> -        tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
name.str, uid);
> +
> +        homedir_ctx = talloc_zero(tmp_ctx, struct
sss_nss_homedir_ctx);
> +        if (! homedir_ctx) {
> +            num = 0;
> +            ret = ENOMEM;
> +            goto done;;
> +        }
> +
> +        homedir_ctx->username = name.str;
> +        homedir_ctx->uid = uid;
> +        homedir_ctx->domain = dom->name;

Should not be homedir_ctx->config_homedir_substr set to
nctx->homedir_substring or rather domain->homedir_substring if not
empty?

> +
> +        tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
homedir_ctx);
>          if (!tmpstr) {
>              to_sized_string(&homedir, "/");
>          } else {
> }


I have noticed a few interesting construction regarding our code style:

patch 2:

> From f06de6cf3130d0b01b04152e11933d1c4d875ea6 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lslebodn at redhat.com>
> Date: Mon, 2 Dec 2013 13:03:33 +0100
> Subject: [PATCH 2/3] NSS: Refactor expand_homedir_template
> 
> Function expand_homedir_template had lot of parameters.
> After adding new expand option, all function call should be rewritten,
> (usually argument NULL will be added)
> 
> This patch wraps all necessary arguments to structure.
> ---
>  src/providers/ipa/ipa_s2n_exop.c      | 17 ++++++++++----
>  src/providers/ipa/ipa_subdomains_id.c | 27 ++++++++++++++--------
>  src/responder/nss/nsssrv_cmd.c        | 35
++++++++++++++++++----------
>  src/responder/pac/pacsrv_utils.c      | 14 ++++++++++--
>  src/util/sss_nss.c                    | 43
++++++++++++++++++++---------------
>  src/util/sss_nss.h                    | 12 +++++++---
>  6 files changed, 99 insertions(+), 49 deletions(-)
> 
> diff --git a/src/providers/ipa/ipa_s2n_exop.c
b/src/providers/ipa/ipa_s2n_exop.c
> index
59a99061ac3ad6f24a8af50e9c1d574282301a1d..02e87f86b75f4b5603b8be2e11e25c0a9fcdec7c 100644
> --- a/src/providers/ipa/ipa_s2n_exop.c
> +++ b/src/providers/ipa/ipa_s2n_exop.c
> @@ -648,6 +648,7 @@ static void ipa_s2n_get_user_done(struct
tevent_req *subreq)
>      struct resp_attrs *simple_attrs = NULL;
>      time_t now;
>      uint64_t timeout = 10*60*60; /* FIXME: find a better timeout ! */
> +    struct sss_nss_homedir_ctx *homedir_ctx;
>      const char *homedir = NULL;
>      struct sysdb_attrs *user_attrs = NULL;
>      struct sysdb_attrs *group_attrs = NULL;
> @@ -738,13 +739,19 @@ static void ipa_s2n_get_user_done(struct
tevent_req *subreq)
>      switch (attrs->response_type) {
>               case RESP_USER:
>              if (state->dom->subdomain_homedir) {
>              +                homedir_ctx = talloc_zero(state, struct
sss_nss_homedir_ctx);
> +                if (! homedir_ctx) {
> +                    ret = ENOMEM;
> +                    goto done;
> +                }

I think it is now prefered to use if (homedir_ctx == NULL) to highlight
homedir_ctx is a pointer, but I defintly think that the space after ! is
redundant.

> +                homedir_ctx->username = attrs->a.user.pw_name;
> +                homedir_ctx->uid = attrs->a.user.pw_uid;
> +                homedir_ctx->domain = state->dom->name;
> +                homedir_ctx->flatname = state->dom->flat_name;
> +
>                  homedir =  expand_homedir_template(state,
>
state->dom->subdomain_homedir,
> -
attrs->a.user.pw_name,
> -
attrs->a.user.pw_uid,
> -                                                   NULL,
> -                                                   state->dom->name,
> -
state->dom->flat_name);
> +                                                   homedir_ctx);
>                  if (homedir == NULL) {
>                      ret = ENOMEM;
>                      goto done;
> diff --git a/src/providers/ipa/ipa_subdomains_id.c
b/src/providers/ipa/ipa_subdomains_id.c
> index
0adf806066ec34b8c42ddf4d0cbd2d23f05caa50..1766e33708cd29da18c3ac4d0b94ccc33583a851 100644
> --- a/src/providers/ipa/ipa_subdomains_id.c
> +++ b/src/providers/ipa/ipa_subdomains_id.c
> @@ -357,10 +357,10 @@ get_subdomain_homedir_of_user(TALLOC_CTX
*mem_ctx, struct sss_domain_info *dom,
>                                const char **_homedir)
>  {
>      errno_t ret;
> -    char *name;
> -    char *lc_name;
> +    const char *name;
>      const char *homedir;
>      TALLOC_CTX *tmp_ctx;
> +    struct sss_nss_homedir_ctx *homedir_ctx;
>  
>      tmp_ctx = talloc_new(mem_ctx);
>      if (tmp_ctx == NULL) {
>      @@ -368,22 +368,31 @@ get_subdomain_homedir_of_user(TALLOC_CTX
*mem_ctx, struct sss_domain_info *dom,
>          goto done;
>      }
>  
> -    ret = sss_parse_name(tmp_ctx, dom->names, fqname, NULL, &name);
> +    homedir_ctx = talloc_zero(tmp_ctx, struct sss_nss_homedir_ctx);
> +    if (! homedir_ctx) {
> +        ret = ENOMEM;
> +        goto done;
> +    }

same as above

> +
> +    homedir_ctx->uid = uid;
> +    homedir_ctx->domain = dom->name;
> +    homedir_ctx->flatname = dom->flat_name;
> +    ret = sss_parse_name_const(tmp_ctx, dom->names, fqname,
> +                               NULL, &name);
>      if (ret != EOK) {
>          goto done;
>      }
>  
>      /* To be compatible with the old winbind based user lookups and
IPA
>       * clients the user name in the home directory path will be
lower-case. */
> -    lc_name = sss_tc_utf8_str_tolower(tmp_ctx, name);
> -    if (lc_name == NULL) {
> -        ret =ENOMEM;
> +    homedir_ctx->username = sss_tc_utf8_str_tolower(tmp_ctx, name);
> +    if (homedir_ctx->username == NULL) {
> +        ret = ENOMEM;
>          goto done;
>      }
>  
> -    homedir = expand_homedir_template(tmp_ctx,
dom->subdomain_homedir, lc_name,
> -                                      uid, NULL, dom->name,
dom->flat_name);
> -
> +    homedir = expand_homedir_template(tmp_ctx,
dom->subdomain_homedir,
> +                                      homedir_ctx);
>      if (homedir == NULL) {
>          DEBUG(SSSDBG_OP_FAILURE, "expand_homedir_template failed\n");
>          ret = ENOMEM;
> @@ -373,7 +372,19 @@ static int fill_pwent(struct sss_packet *packet,
>          } else {
>              to_sized_string(&gecos, tmpstr);
>          }
> -        tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
name.str, uid);
> +
> +        homedir_ctx = talloc_zero(tmp_ctx, struct
sss_nss_homedir_ctx);
> +        if (! homedir_ctx) {
> +            num = 0;
> +            ret = ENOMEM;
> +            goto done;;
> +        }

same as above and please remove the redundant ;

> +
> +        homedir_ctx->username = name.str;
> +        homedir_ctx->uid = uid;
> +        homedir_ctx->domain = dom->name;
> +
> +        tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
homedir_ctx);
>          if (!tmpstr) {
>              to_sized_string(&homedir, "/");
>          } else {
> diff --git a/src/responder/pac/pacsrv_utils.c
b/src/responder/pac/pacsrv_utils.c
> index
035fe84c0f39aaa40a403a8ac5f19527b2191267..4bf5585e8de667850ef520fa372d7e114f2cab1a 100644
> --- a/src/responder/pac/pacsrv_utils.c
> +++ b/src/responder/pac/pacsrv_utils.c
> @@ -353,6 +353,7 @@ errno_t get_pwd_from_pac(TALLOC_CTX *mem_ctx,
>      char *upn;
>      hash_key_t key;
>      hash_value_t value;
> +    struct sss_nss_homedir_ctx *homedir_ctx;
>  
>      pwd = talloc_zero(mem_ctx, struct passwd);
>      if (pwd == NULL) {
> @@ -439,9 +440,18 @@ errno_t get_pwd_from_pac(TALLOC_CTX *mem_ctx,
>      /* Check if there is a special homedir template for sub-domains.
If not a
>       * fallback will be added by the NSS responder. */
>      if (IS_SUBDOMAIN(dom) && dom->subdomain_homedir) {
>      +        homedir_ctx = talloc_zero(pwd, struct
sss_nss_homedir_ctx);
> +        if (! homedir_ctx) {
> +            ret = ENOMEM;
> +            goto done;;
> +        }

same as above and please remove the redundant ;

> +        homedir_ctx->username = lname;
> +        homedir_ctx->uid = pwd->pw_uid;
> +        homedir_ctx->domain = dom->name;
> +        homedir_ctx->flatname = dom->flat_name;
> +
>          pwd->pw_dir = expand_homedir_template(pwd,
dom->subdomain_homedir,
> -                                              lname, pwd->pw_uid,
NULL,
> -                                              dom->name,
dom->flat_name);
> +                                              homedir_ctx);
>          if (pwd->pw_dir == NULL) {
>              ret = ENOMEM;
>              goto done;
> diff --git a/src/util/sss_nss.c b/src/util/sss_nss.c
> index
406c95cd0f76c9e73705bc49b4261160d26bfc58..efc6e3e2c66944c236a5d62df9e573b15a9f5840 100644
> --- a/src/util/sss_nss.c
> +++ b/src/util/sss_nss.c
> @@ -23,9 +23,7 @@
>  #include "util/sss_nss.h"
>  
>  char *expand_homedir_template(TALLOC_CTX *mem_ctx, const char
*template,
> -                              const char *username, uint32_t uid,
> -                              const char *original, const char
*domain,
> -                              const char *flatname)
> +                              struct sss_nss_homedir_ctx
*homedir_ctx)
>  {
>      char *copy;
>      char *p;
> @@ -40,6 +38,11 @@ char *expand_homedir_template(TALLOC_CTX *mem_ctx,
const char *template,
>          return NULL;
>      }
>  
> +    if (! homedir_ctx) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Missing home directory
data.\n"));
> +        return NULL;
> +    }

same as above



patch 3


> diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
> index
9a13f723de601413205cbb580a92791015b94aa1..cc8b2691262ed9b893bcb693eb1cbb2c05446a60 100644
> --- a/src/confdb/confdb.c
> +++ b/src/confdb/confdb.c
> @@ -1118,6 +1118,16 @@ static int confdb_get_domain_internal(struct
confdb_ctx *cdb,
>      }
>  
>      tmp = ldb_msg_find_attr_as_string(res->msgs[0],
> +                                      CONFDB_NSS_HOMEDIR_SUBSTRING,
NULL);
> +    if (tmp != NULL) {
> +        domain->homedir_substr = talloc_strdup(domain, tmp);
> +        if (!domain->homedir_substr) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +    }

Please use uniform form of testing to NULL IMO the first one is
prefered.

> diff --git a/src/man/include/homedir_substring.xml
b/src/man/include/homedir_substring.xml
> new file mode 100644
> index
0000000000000000000000000000000000000000..8ac83b7d665cfa8bc9b81bcbea780b40cb64ef62
> --- /dev/null
> +++ b/src/man/include/homedir_substring.xml
> @@ -0,0 +1,16 @@
> +<varlistentry>
> +    <term>homedir_substring (string)</term>
> +    <listitem>
> +        <para>
> +            Value of this option will be used in the expansion of
> +            <emphasis>override_homedir</emphasis> option if template
contains
> +            format string <emphasis>%H</emphasis>. LDAP directory can
directly
> +            contain template and this option can be used to expand
home
> +            directory for each machine (or Operating system).
> +            It can be set in the [nss] section or per-domain.

If this option is set in both [nss] and domain section which value is
then used?

> +        </para>
> +        <para>
> +            Default: /home
> +        </para>
> +    </listitem>
> +</varlistentry>

Thanks for reading.


Pavel Reichl




More information about the sssd-devel mailing list