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

[Martin Basti]

On 02.05.2016 18:45, Sumit Bose wrote:
> 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
Pushed to master: 8a2afcafee977675fc289acab50cc808b469a2b3