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" <npmccal...@redhat.com>
To: "Matt Rogers" <mrog...@redhat.com>, freeipa-devel@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

--
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

Reply via email to