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

2016-05-02 Thread 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" <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",
_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

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

2016-05-02 Thread 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" <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 s

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

2016-05-02 Thread Sumit Bose 
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",
> > >_auth_str);
> > > 
> > > if (require_auth_str != NULL) {
> > > kerr = krb5_dbe_set_string(kcontext, entry, "require_auth", NULL);
> > > }
> > > 
> > &

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

2016-04-28 Thread Matt Rogers 
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",
> >_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)?
> > 
>

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

2016-04-27 Thread Alexander Bokovoy 

On Wed, 27 Apr 2016, 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",
   _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.

I think from the maintenance perspective it would be better to use the
recommended API.
--
/ Alexander Bokovoy

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

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

2016-04-27 Thread Matt Rogers 
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",
>_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

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

2016-04-27 Thread Sumit Bose 
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",
   _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)?

bye,
Sumit

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

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

2016-04-26 Thread Matt Rogers 
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.

I've attached the updated patch. Thanks for spotting that.

-- 
Matt Rogers
Red Hat, Inc
>From a75ca3d391554c0254c1b2d206f96edc29f11149 Mon Sep 17 00:00:00 2001
From: Matt Rogers <mrog...@redhat.com>
Date: Fri, 25 Mar 2016 17:01:40 -0400
Subject: [PATCH] ipa_kdb: add krbPrincipalAuthInd handling

Store and retrieve the authentication indicator "require_auth" string in
the krbPrincipalAuthInd attribute. Skip storing auth indicators to
krbExtraData.
---
 daemons/ipa-kdb/ipa_kdb_principals.c | 278 +++
 install/share/60kerberos.ldif|   6 +-
 2 files changed, 282 insertions(+), 2 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c 
b/daemons/ipa-kdb/ipa_kdb_principals.c
index 
629f8193223c924267f6d5f39d258cfbc51c7f63..fbfb15d179790e30ea69e43f693bd90fdd0ff2ec
 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -54,6 +54,7 @@ static char *std_principal_attrs[] = {
 "krbLastSuccessfulAuth",
 "krbLastFailedAuth",
 "krbLoginFailedCount",
+"krbPrincipalAuthInd",
 "krbExtraData",
 "krbLastAdminUnlock",
 "krbObjectReferences",
@@ -428,6 +429,85 @@ done:
 return kerr;
 }
 
+static void strv_free(char **strv)
+{
+int i;
+
+if (strv == NULL) {
+return;
+}
+
+for (i = 0; strv[i] != NULL; i++) {
+free(strv[i]);
+}
+
+free(strv);
+}
+
+static krb5_error_code ipadb_get_ldap_auth_ind(krb5_context kcontext,
+   LDAP *lcontext,
+   LDAPMessage *lentry,
+   krb5_db_entry *entry)
+{
+krb5_error_code ret = 0;
+char **authinds = NULL;
+char *aistr = NULL;
+char *ap = NULL;
+size_t len = 0;
+size_t l = 0;
+int count = 0;
+int i = 0;
+
+ret = ipadb_ldap_attr_to_strlist(lcontext, lentry, "krbPrincipalAuthInd",
+ );
+switch (ret) {
+case 0:
+break;
+case ENOENT:
+return 0;
+default:
+return ret;
+}
+
+for (count = 0; authinds != NULL && authinds[count] != NULL; count++) {
+len += strlen(authinds[count]) + 1;
+}
+
+if (len == 0) {
+strv_free(authinds);
+return 0;
+}
+
+aistr = malloc(len);
+if (aistr == NULL) {
+ret = errno;
+goto cleanup;
+}
+
+/* Create a space-separated string of authinds. */
+ap = aistr;
+l = len;
+for (i = 0; i < count; i++) {
+

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

2016-04-26 Thread Sumit Bose 
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()?

> +
>  static krb5_error_code ipadb_get_ldap_mod_extra_data(struct ipadb_mods 
> *imods,
>   krb5_tl_data *tl_data,
>   int mod_op)
>  {
>  krb5_error_code kerr;
>  krb5_tl_data *data;
> +krb5_tl_data *data_tmp = NULL;
>  struct berval **bvs = NULL;
>  krb5_int16 be_type;
>  int n, i;
> @@ -1463,6 +1663,20 @@ static krb5_error_code 
> ipadb_get_ldap_mod_extra_data(struct ipadb_mods *imods,
>  continue;
>  }
>  
> +/* Exclude any auth indicators from krbExtraData */
> +kerr = filter_authind_str_attrs(data, _tmp);
> +if (kerr) {
> +goto done;
> +}
> +if (data_tmp != NULL) {
> +if (data_tmp->tl_data_contents == NULL) {
> +free(data_tmp);
> +data_tmp = NULL;
> +continue;
> +}
> +data = data_tmp;
> +}
> +
>  be_type = htons(data->tl_data_type);
>  
>  bvs[i] = calloc(1, sizeof(struct berval));

bye,
Sumit

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

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

2016-04-14 Thread Matt Rogers 


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

-- 
Matt Rogers
Red Hat, Inc
From d9d57281014d2c153531d8dc9cd0c547306a5f8a Mon Sep 17 00:00:00 2001
From: Matt Rogers <mrog...@redhat.com>
Date: Fri, 25 Mar 2016 17:01:40 -0400
Subject: [PATCH] ipa_kdb: add krbPrincipalAuthInd handling

Store and retrieve the authentication indicator "require_auth" string in
the krbPrincipalAuthInd attribute. Skip storing auth indicators to
krbExtraData.
---
 daemons/ipa-kdb/ipa_kdb_principals.c | 284 +++
 install/share/60kerberos.ldif|   6 +-
 2 files changed, 288 insertions(+), 2 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index 629f8193223c924267f6d5f39d258cfbc51c7f63..7e9a7aefa814ee8b2f9787964789c9dc80fc06eb 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -54,6 +54,7 @@ static char *std_principal_attrs[] = {
 "krbLastSuccessfulAuth",
 "krbLastFailedAuth",
 "krbLoginFailedCount",
+"krbPrincipalAuthInd",
 "krbExtraData",
 "krbLastAdminUnlock",
 "krbObjectReferences",
@@ -428,6 +429,85 @@ done:
 return kerr;
 }
 
+static void strv_free(char **strv)
+{
+int i;
+
+if (strv == NULL) {
+return;
+}
+
+for (i = 0; strv[i] != NULL; i++) {
+free(strv[i]);
+}
+
+free(strv);
+}
+
+static krb5_error_code ipadb_get_ldap_auth_ind(krb5_context kcontext,
+   LDAP *lcontext,
+   LDAPMessage *lentry,
+   krb5_db_entry *entry)
+{
+krb5_error_code ret = 0;
+char **authinds = NULL;
+char *aistr = NULL;
+char *ap = NULL;
+size_t len = 0;
+size_t l = 0;
+int count = 0;
+int i = 0;
+
+ret = ipadb_ldap_attr_to_strlist(lcontext, lentry, "krbPrincipalAuthInd",
+ );
+switch (ret) {
+case 0:
+break;
+case ENOENT:
+return 0;
+default:
+return ret;
+}
+
+for (count = 0; authinds != NULL && authinds[count] != NULL; count++) {
+len += strlen(authinds[count]) + 1;
+}
+
+if (len == 0) {
+strv_free(authinds);
+return 0;
+}
+
+aistr = malloc(len);
+if (aistr == NULL) {
+ret = errno;
+goto cleanup;
+}
+
+/* Create a space-separated string of authinds. */
+ap = aistr;
+l = len;
+for (i = 0; i < count; i++) {
+ret = snprintf(ap, l, "%s ", authinds[i]);
+if (ret <= 0 || ret > l) {
+ret = ENOMEM;
+goto cleanup;
+}
+ap += ret;
+l -= ret;
+}
+aistr[len - 1] = '\0';
+
+ret = krb5_dbe_set_string(kcontext, entry, "require_auth",
+  aistr);
+
+cleanup:
+strv_free(authinds);
+free(aistr);
+
+return ret;
+}
+
 static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
   char *principal,
   LDAPMessage *lentry,
@@ -611,6 +691,10 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
 goto done;
 }
 
+ret = ipadb_get_ldap_auth_ind(kcontext, lcontext, lentry, entry);
+if (ret)
+goto done;
+
 ret = ipadb_ldap_attr_to_key_data(lcontext, lentry,
   "krbPrincipalKey",
   _key_data, , );
@@ -1422,12 +1506,128 @@ done:
 return kerr;
 }
 
+/* Find the next key and value pair in *pos and update *pos. */
+static bool next_attr(const char **pos, const char *end,
+  const char **key_out, const char **val_out)
+{
+const char *key, *key_end, *val, *val_end;
+
+*key_out = *val_out = NULL;
+
+if (*pos == end) {
+return false;
+}
+
+key = *pos;
+key_end = memchr(key, '\0', end - key);
+if (key_end == NULL) {  /* Malformed representation; give up. */
+return fa

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

2016-04-14 Thread Nathaniel McCallum 
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.

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

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

2016-04-14 Thread Nathaniel McCallum 
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

This patch is part of the authentication indicators work and is
parallel to my patches (also on this list). My patches cause the
OTP/RADIUS process to insert indicators in tickets. This patch allows
us to define required indicators for a service principal. After this
patch is merged, we will need CLI and UI to expose the manipulation of
this attribute.

A similar patch was already merged upstream. This patch matches the
behavior and schema of upstream in our own KDB module with one
exception: we do not store indicators in krbExtraData in parallel.

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

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

2016-04-11 Thread Matt Rogers 
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

Regards,
---
Matt Rogers
Red Hat, Inc
From 4aedaf64320b52485407f6798788ecd367c3a002 Mon Sep 17 00:00:00 2001
From: Matt Rogers 
Date: Fri, 25 Mar 2016 17:01:40 -0400
Subject: [PATCH] ipa_kdb: add krbPrincipalAuthInd handling

Store and retrieve the authentication indicator "require_auth" string in
the krbPrincipalAuthInd attribute. Skip storing auth indicators to
krbExtraData.
---
 daemons/ipa-kdb/ipa_kdb_principals.c | 277 +++
 install/share/60kerberos.ldif|   6 +-
 2 files changed, 281 insertions(+), 2 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index 629f8193223c924267f6d5f39d258cfbc51c7f63..f12339579e5a4127947fd9a9d6cebf7c0f80e1e2 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -54,6 +54,7 @@ static char *std_principal_attrs[] = {
 "krbLastSuccessfulAuth",
 "krbLastFailedAuth",
 "krbLoginFailedCount",
+"krbPrincipalAuthInd",
 "krbExtraData",
 "krbLastAdminUnlock",
 "krbObjectReferences",
@@ -428,6 +429,85 @@ done:
 return kerr;
 }
 
+static void strv_free(char **strv)
+{
+int i;
+
+if (strv == NULL) {
+return;
+}
+
+for (i = 0; strv[i] != NULL; i++) {
+free(strv[i]);
+}
+
+free(strv);
+}
+
+static krb5_error_code ipadb_get_ldap_auth_ind(krb5_context kcontext,
+   LDAP *lcontext,
+   LDAPMessage *lentry,
+   krb5_db_entry *entry)
+{
+krb5_error_code ret = 0;
+char **authinds = NULL;
+char *aistr = NULL;
+char *ap = NULL;
+size_t len = 0;
+size_t l = 0;
+int count = 0;
+int i = 0;
+
+ret = ipadb_ldap_attr_to_strlist(lcontext, lentry, "krbPrincipalAuthInd",
+ );
+switch (ret) {
+case 0:
+break;
+case ENOENT:
+return 0;
+default:
+return ret;
+}
+
+for (count = 0; authinds != NULL && authinds[count] != NULL; count++) {
+len += strlen(authinds[count]) + 1;
+}
+
+if (len == 0) {
+strv_free(authinds);
+return 0;
+}
+
+aistr = malloc(len);
+if (aistr == NULL) {
+ret = errno;
+goto cleanup;
+}
+
+/* Create a space-separated string of authinds. */
+ap = aistr;
+l = len;
+for (i = 0; i < count; i++) {
+ret = snprintf(ap, l, "%s ", authinds[i]);
+if (ret <= 0 || ret > l) {
+ret = ENOMEM;
+goto cleanup;
+}
+ap += ret;
+l -= ret;
+}
+aistr[len - 1] = '\0';
+
+ret = krb5_dbe_set_string(kcontext, entry, "require_auth",
+  aistr);
+
+cleanup:
+strv_free(authinds);
+free(aistr);
+
+return ret;
+}
+
 static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
   char *principal,
   LDAPMessage *lentry,
@@ -611,6 +691,10 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
 goto done;
 }
 
+ret = ipadb_get_ldap_auth_ind(kcontext, lcontext, lentry, entry);
+if (ret)
+goto done;
+
 ret = ipadb_ldap_attr_to_key_data(lcontext, lentry,
   "krbPrincipalKey",
   _key_data, , );
@@ -1422,12 +1506,121 @@ done:
 return kerr;
 }
 
+/* Find the next key and value pair in *pos and update *pos. */
+static bool next_attr(const char **pos, const char *end,
+  const char **key_out, const char **val_out)
+{
+const char *key, *key_end, *val, *val_end;
+
+*key_out = *val_out = NULL;
+if (*pos == end)
+return false;
+key = *pos;
+key_end = memchr(key, '\0', end - key);
+if (key_end == NULL)/* Malformed representation; give up. */
+return false;
+val = key_end + 1;
+val_end = memchr(val, '\0', end - val);
+if (val_end == NULL)/* Malformed representation; give up. */
+return false;
+
+*key_out = key;
+*val_out = val;
+*pos = val_end + 1;
+return true;
+}
+
+static krb5_error_code filter_authind_str_attrs(krb5_tl_data *data,
+krb5_tl_data **data_out)
+{
+krb5_error_code ret = 0;
+krb5_tl_data *new_data = NULL;
+char *new_content = NULL;
+char *new_cp = NULL;
+const char *pos = NULL;
+const char *end = NULL;
+const char *key = NULL;
+const char *val = NULL;
+size_t data_len = 0;
+size_t new_len = 0;
+
+*data_out = NULL;
+
+if (data->tl_data_type !=