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

Pavel Reichl preichl at redhat.com
Tue May 27 09:15:53 UTC 2014


On Tue, 2014-05-27 at 10:42 +0200, Lukas Slebodnik wrote:
> On (27/05/14 10:31), Pavel Reichl wrote:
> >On Tue, 2014-05-27 at 09:32 +0200, Lukas Slebodnik wrote:
> >> On (26/05/14 18:10), Pavel Reichl wrote:
> >> >On Fri, 2014-05-23 at 10:09 +0200, Lukas Slebodnik wrote:
> >> >> On (22/05/14 17:08), Pavel Reichl wrote:
> >> >> >Sorry Lukas, but the patches do not apply to master.
> >> >> >
> >> >> rebased patches are attached.
> >> >> 
> >> >> LS
> >> >> _______________________________________________
> >> >> sssd-devel mailing list
> >> >> sssd-devel at lists.fedorahosted.org
> >> >> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >> >
> >> >Sorry for broken formatting in prev mail.
> >> >
> >> >Hello Lukas,
> >> >
> >> >I'm having trouble with one of my testing configuration: 
> >> >
> >> >sssd is client of ipa which has AD trust.
> >> >
> >> >When I ask on users from AD trust domain specific homedir_substring is
> >> >not used. 
> >> >
> >> >Could you have a look?
> >> Thank you for catching this issue.
> >> Function new_subdomain is appropriately updated.
> >> 
> >> >I have also noticed following nitpicks, could you have a look as well
> >> >
> >> >> diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
> >> >> index c7c1232c378d7e641da678d853247ddfd31bb736..5e5a9284e9ad80d822fe1db4269353aeb7925682 100644
> >> >> --- a/src/config/etc/sssd.api.conf
> >> >> +++ b/src/config/etc/sssd.api.conf
> >> >> @@ -36,6 +36,7 @@ filter_users_in_groups = bool, None, false
> >> >>  pwfield = str, None, false
> >> >>  override_homedir = str, None, false
> >> >>  fallback_homedir = str, None, false
> >> >> +homedir_substring = str, None, false, /home
> >> >> 
> >> >I'm sorry, but I am not aware of the meaning of this file, could you explain to me? 
> >> >And I'm also curious about the '/home' value, as all other lines are having just 3 values...
> >> >
> >> sh-4.2$ head -n3 src/config/etc/sssd.api.conf
> >> # Format:
> >> # option = type, subtype, mandatory[, default]
> >> 
> >> >
> >> >> static const char *get_shell_override(TALLOC_CTX *mem_ctx,
> >> >> diff --git a/src/util/sss_nss.c b/src/util/sss_nss.c
> >> >> index 66b0e59..2698936 100644
> >> >> --- a/src/util/sss_nss.c
> >> >> +++ b/src/util/sss_nss.c
> >> >> @@ -136,6 +136,17 @@ char *expand_homedir_template(TALLOC_CTX *mem_ctx, const char *template,
> >> >>                                                  homedir_ctx->flatname);
> >> >>                  break;
> >> >>  
> >> >> +            case 'H':
> >> >> +                if (homedir_ctx->config_homedir_substr == NULL) {
> >> >> +                    DEBUG(SSSDBG_CRIT_FAILURE,
> >> >> +                          ("Cannot expand home directory substring template "
> >> >>                            ^
> >> >> +                           "substring is empty.\n"));
> >> >>                                                    ^
> >> >I think that the parenthesis are relic here.
> >> It is lefover after multiple rebases in last few months.
> >> Acctually, it is not problem, but it needn't be here.
> >> I fixed the same issue also in 2nd patch.
> >> 
> >> >> --- 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 == NULL) {
> >> >> +                    ret = ENOMEM;
> >> >> +                    goto done;
> >> >> +                }
> >> >> +                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,
> >> >>                           ^ 
> >> >could you remove the the extra whitespace if already changing nearby code?
> >> >...
> >> >
> >> sure, done
> >> 
> >> >>                                          struct ldb_message *msg,
> >> >>                                          struct nss_ctx *nctx,
> >> >>                                          struct sss_domain_info *dom,
> >> >> -                                        const char *orig_name,
> >> >> -                                        uint32_t uid)
> >> >> +                                        struct sss_nss_homedir_ctx *homedir_ctx)
> >> >>  {
> >> >>      const char *homedir;
> >> >> -    char *name;
> >> >> +    const char *orig_name = homedir_ctx->username;
> >> >>      errno_t ret;
> >> >>  
> >> >>      homedir = ldb_msg_find_attr_as_string(msg, SYSDB_HOMEDIR, NULL);
> >> >> +    homedir_ctx->original = homedir;
> >> >>  
> >> >>      /* Subdomain users store FQDN in their name attribute */
> >> >> -    ret = sss_parse_name(mem_ctx, dom->names, orig_name, NULL, &name);
> >> >> +    ret = sss_parse_name_const(mem_ctx, dom->names, orig_name,
> >> >> +                               NULL, &homedir_ctx->username);
> >> >>      if (ret != EOK) {
> >> >>          DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into "
> >> >>                "name-value components.\n", orig_name);
> >> >Do we really need orig_name? I think it is just obfuscating code here.
> >> Yes,
> >> 
> >> orig_name was parameter of function get_homedir_override,
> >> which was converted into "struct sss_nss_homedir_ctx"
> >> 
> >> I could use "homedir_ctx->username" for two purposes
> >> (input and outtupt argument). It seems strange to me.
> >Yes, it is strange and it's a potential source of bugs if order of
> >reading to and writing from would change in implementation of
> >sss_parse_name(). But hiding this potential issue via orig_name is not
> >helping anything.
> Could you ellaborate? What kind of potential bugs?
The fact of passing pointer to the same area in memory to 2 separate
arguments of sss_parse_name() is what I called potential source of bugs.
You said "It seems strange to me" so I hope you know what I mean. 

I'm not saying you can't do it, I just say I don't see any reason why to
hide it with usage of orig_name.

> 
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel





More information about the sssd-devel mailing list