On Tue, Oct 28, 2014 at 05:37:25AM +0100, Jakub Hrozek wrote:
On Fri, Oct 24, 2014 at 09:46:12PM +0200, Sumit Bose wrote:
> Hi,
> this two patches allow to have extra user attributes,
like e.g. a
> telephone number, for AD users of trusted domains on the IPA clients.
> This first makes it possible that the attributes given
in
> ldap_user_extra_attrs can be read from AD in ipa-server-mode. The second
> extends the list of attributes returned by sss_nss_getorigbyname().
> bye,
> Sumit
> P.S. in the first patch I used
sdap_extend_map_with_list() as it is used
> in other places as well where the maps in the 4th and 6th argument are
> the same. If I see it correctly we are leaking the old version of the
> map this way. Since this currently happen only once during init it does
> not do much harm, but maybe a ticket to fix this might be useful.
Ah, nice catch. Feel free to file the ticket, we should unit test that
function anyway and might as well include check_leaks_call to the test.
https://fedorahosted.org/freeipa/ticket/4667
...
> > ---
> > src/responder/nss/nsssrv_cmd.c | 170 ++++++++++++++++++++++++++++++++++------
> > src/tests/cmocka/test_nss_srv.c | 130 ++++++++++++++++++++++++++++++
> > 2 files changed, 278 insertions(+), 22 deletions(-)
>
> > diff --git a/src/responder/nss/nsssrv_cmd.c
b/src/responder/nss/nsssrv_cmd.c
> > index 9fca644..7481d49 100644
> > --- a/src/responder/nss/nsssrv_cmd.c
> > +++ b/src/responder/nss/nsssrv_cmd.c
> > @@ -4131,6 +4131,7 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx
*dctx)
> > SYSDB_AD_ACCOUNT_EXPIRES,
> > SYSDB_AD_USER_ACCOUNT_CONTROL,
> > SYSDB_DEFAULT_ATTRS, NULL};
> > + const char *all_attrs[] = { "*", NULL};
> Allowing the responder the retrieve /all/ attributes
seems quite
> dangerous to me, this allows to leak cachedPassword etc..
> What is the use-case?
My laziness :-). I changed this by using add_strings_lists() which I
send with the SSH pubkey patches.
> > bool user_found = false;
> > bool group_found = false;
> > struct ldb_message *msg = NULL;
> > @@ -4281,8 +4282,9 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx
*dctx)
> > }
> > } else {
> > ret = sysdb_search_user_by_name(cmdctx, dom,
> > - sysdb_name ? sysdb_name : name,
> > - attrs, &msg);
> > + sysdb_name ? sysdb_name : name,
> > + cmdctx->cmd == SSS_NSS_GETORIGBYNAME ? all_attrs
:attrs,
> > + &msg);
> > if (ret != EOK && ret != ENOENT) {
> > DEBUG(SSSDBG_CRIT_FAILURE,
> > "Failed to make request to our cache!\n");
> > @@ -4295,8 +4297,9 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx
*dctx)
> > } else {
> > talloc_free(msg);
> > ret = sysdb_search_group_by_name(cmdctx, dom,
> > - sysdb_name ? sysdb_name :
name,
> > - attrs, &msg);
> > + sysdb_name ? sysdb_name : name,
> > + cmdctx->cmd == SSS_NSS_GETORIGBYNAME ? all_attrs :
attrs,
> > + &msg);
> > if (ret != EOK && ret != ENOENT) {
> > DEBUG(SSSDBG_CRIT_FAILURE,
> > "Failed to make request to our cache!\n");
> > @@ -4563,17 +4566,100 @@ static errno_t fill_sid(struct sss_packet *packet,
> > return EOK;
> > }
> >
> > +static errno_t get_extra_attribute_list(TALLOC_CTX *mem_ctx,
> > + struct confdb_ctx *cdb,
> > + struct sss_domain_info *domain,
> > + size_t *new_attrs_count,
> > + char ***new_attrs_list)
> > +{
> > + int ret;
> > + TALLOC_CTX *tmp_ctx;
> > + char *confdb_path;
> > + char *extra_attrs;
> > + char **extra_attrs_list;
> > + size_t c;
> > + int count;
> > + char *sep;
> > + struct sss_domain_info *domain_head;
> > +
> > + tmp_ctx = talloc_new(NULL);
> > + if (tmp_ctx == NULL) {
> > + DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
> > + return ENOMEM;
> > + }
> > +
> > + domain_head = get_domains_head(domain);
> > + confdb_path = talloc_asprintf(tmp_ctx, CONFDB_DOMAIN_PATH_TMPL,
> > + domain_head->name);
> > + if (confdb_path == NULL) {
> > + DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n");
> > + ret = ENOMEM;
> > + goto done;
> > + }
> > +
> > + ret = confdb_get_string(cdb, tmp_ctx, confdb_path,
"ldap_user_extra_attrs",
> > + NULL, &extra_attrs);
> I don't like that we're leaking the knowledge
about an LDAP attribute to
> the responder. Can we use the 'user_attributes' parameter InfoPipe uses?
> I know there would be one more step for the admin to configure, but I
> would prefer that and keep the separated responder/backend architecture.
I agree, I allowed user_attributes for the nss responder as well with a
fallback to the InfoPipe value so the admin gets it for free if InfoPipe
is already configured but does not have to configure InfoPipe if not
needed. See man page change for details.
Thank you for the review, new version attached.
bye,
Sumit