[Freeipa-devel] [PATCH] 0001 ipa_kdb add krbPrincipalAuthInd handling

[Sumit Bose]

On Mon, May 02, 2016 at 11:47:41AM -0400, Matt Rogers wrote:
> On 05/02, Sumit Bose wrote:
> > On Thu, Apr 28, 2016 at 02:58:07PM -0400, Matt Rogers wrote:
> > > On 04/27, Matt Rogers wrote:
> > > > 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" <npmccallum at redhat.com>
> > > > > > > > > To: "Matt Rogers" <mrogers at redhat.com>, freeipa-devel at redhat.com
> > > > > > > > > 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. 
> > > > 
> > > 
> > > Here's the updated patch. Thanks again for the review!
> > 
> > Hi Matt,
> > 
> > thank you for the new version. I played with/tested the patch with 'ipa
> > user-mod', ldapsearch, ldapmodify and kadmin.local and all was working
> > as expected.
> > 
> > I only found a minor issue:
> > 
> > > @@ -1428,6 +1512,7 @@ static krb5_error_code ipadb_get_ldap_mod_extra_data(struct ipadb_mods *imods,
> > >  {
> > >      krb5_error_code kerr;
> > >      krb5_tl_data *data;
> > > +    krb5_tl_data *data_tmp = NULL;
> > 
> > this is unused.
> > 
> > Coverity didn't found anything else as well.
> > 
> > Since Simo already added the new OID to
> > https://code.engineering.redhat.com/gerrit/p/rhanana.git, ACK, if you remove data_tmp.
> > 
> 
> Thanks! Here's the corrected patch.

ACK

bye,
Sumit

> 
> > bye,
> > Sumit
> 
> -- 
> Matt Rogers
> Red Hat, Inc