On 04/27, Sumit Bose wrote: > On Tue, Apr 26, 2016 at 02:02:04PM -0400, Matt Rogers wrote: > > On 04/26, Sumit Bose wrote: > > > On Thu, Apr 14, 2016 at 12:59:55PM -0400, Matt Rogers wrote: > > > > > > > > > > > > ----- Original Message ----- > > > > > From: "Nathaniel McCallum" <[email protected]> > > > > > To: "Matt Rogers" <[email protected]>, [email protected] > > > > > Sent: Thursday, April 14, 2016 10:32:15 AM > > > > > Subject: Re: [Freeipa-devel] [PATCH] 0001 ipa_kdb add > > > > > krbPrincipalAuthInd handling > > > > > > > > > > On Mon, 2016-04-11 at 10:41 -0400, Matt Rogers wrote: > > > > > > Hi, > > > > > > > > > > > > The attached patch is a part of the authentication indicator > > > > > > enhancements, > > > > > > adding indicator value storage and retrieval for the KDB driver. > > > > > > > > > > > > https://fedorahosted.org/freeipa/ticket/5782 > > > > > > > > > > Can you add some whitespace in next_attr()? The density of the code > > > > > there hurts readability. > > > > > > > > > Sure, I've attached the revised patch. > > > > > > Hi Matt, > > > > > > thank you for the patch. Currently I have the following question. > > > > > > You call krb5_dbe_set_string to remove the 'require_auth' data before > > > calling ipadb_get_ldap_mod_extra_data() > > > > > > > > > + /* Delete authinds from tl_data so it is not included in > > > > krbExtraData. */ > > > > + kerr = krb5_dbe_set_string(kcontext, entry, "require_auth", > > > > NULL); > > > > + if (kerr) { > > > > + goto done; > > > > + } > > > > + > > > > kerr = ipadb_get_ldap_mod_extra_data(imods, > > > > entry->tl_data, > > > > mod_op); > > > > > > > > > > Why it is needed to filter this data again in > > > ipadb_get_ldap_mod_extra_data()? > > > > > > > Hi Sumit, thanks for looking. The above krb5_dbe_set_string() call I > > believe I left there in error - We decided to operate on a filtered copy > > of the tl_data (which filter_authind_str_attrs() handles) rather than > > removing it completely with krb5_dbe_set_string(). I had tested with the > > above code commented out but forgot to remove it with the submitted patch. > > ok, makes sense. > > Nevertheless, comments in kdb.h like: > > /* String attributes (currently stored inside tl-data) map C string keys to > * values. They can be set via kadmin and consumed by KDC plugins. */ > > and > > /* String attributes may not always be represented in tl-data. kadmin clients > * must use the get_strings and set_string RPCs. */ > > make me wonder if it is a good idea to operate on the tl-data of type > KRB5_TL_STRING_ATTRS directly? I know we do this in other places as well > so I'm not insisting to change it, I'm just wondering about the reasons. > > Would something like (error checks are missing) > > kerr = krb5_dbe_get_string(kcontext, entry, "require_auth", > &require_auth_str); > > if (require_auth_str != NULL) { > kerr = krb5_dbe_set_string(kcontext, entry, "require_auth", NULL); > } > > kerr = ipadb_get_ldap_mod_extra_data(imods, > entry->tl_data, > mod_op); > > if (require_auth_str != NULL) { > kerr = krb5_dbe_set_string(kcontext, entry, "require_auth", > require_auth_str); > } > > have the same effect with only using the recommended API (and making the > patch smaller)? >
I believe that would be the same effect, just less efficient. But sticking to the API is probably safer in the long run. I am okay with changing it if you would prefer. -- Matt Rogers Red Hat, Inc -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
