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!

-- 
Matt Rogers
Red Hat, Inc
>From c08bb73685b2f7a17a22e5ea066ad9b60b7c4163 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 | 171 +++++++++++++++++++++++++++++++++++
 install/share/60kerberos.ldif        |   6 +-
 2 files changed, 175 insertions(+), 2 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c 
b/daemons/ipa-kdb/ipa_kdb_principals.c
index 
629f8193223c924267f6d5f39d258cfbc51c7f63..837b4ad37f25aebbe61623e156544cf01d1c0c9e
 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",
+                                     &authinds);
+    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",
                                       &res_key_data, &result, &mkvno);
@@ -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;
     struct berval **bvs = NULL;
     krb5_int16 be_type;
     int n, i;
@@ -1649,6 +1734,62 @@ done:
     return kerr;
 }
 
+static krb5_error_code ipadb_get_ldap_mod_auth_ind(krb5_context kcontext,
+                                                   struct ipadb_mods *imods,
+                                                   krb5_db_entry *entry,
+                                                   int mod_op)
+{
+    krb5_error_code ret = 0;
+    char **strlist = NULL;
+    char *ais = NULL;
+    char *ai = NULL;
+    char *s = NULL;
+    size_t ai_size = 0;
+    int cnt = 0;
+    int i = 0;
+
+    ret = krb5_dbe_get_string(kcontext, entry, "require_auth", &ais);
+    if (ret) {
+        return ret;
+    }
+    if (ais == NULL) {
+        return 0;
+    }
+
+    ai_size = strlen(ais) + 1;
+
+    for (i = 0; i < ai_size; i++) {
+        if (ais[i] != ' ') {
+            continue;
+        }
+        if (i > 0 && ais[i - 1] != ' ') {
+            cnt++;
+        }
+    }
+
+    strlist = calloc(cnt + 2, sizeof(*strlist));
+    if (strlist == NULL) {
+        free(ais);
+        return errno;
+    }
+
+    cnt = 0;
+    ai = strtok_r(ais, " ", &s);
+    while (ai != NULL) {
+        if (ai[0] != '\0') {
+            strlist[cnt++] = ai;
+        }
+        ai = strtok_r(NULL, " ", &s);
+    }
+
+    ret = ipadb_get_ldap_mod_str_list(imods, "krbPrincipalAuthInd",
+                                      strlist, cnt, mod_op);
+
+    free(ais);
+    free(strlist);
+    return ret;
+}
+
 static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
                                            struct ipadb_mods *imods,
                                            krb5_db_entry *entry,
@@ -1657,6 +1798,7 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context 
kcontext,
     krb5_error_code kerr;
     krb5_int32 time32le;
     int mkvno;
+    char *req_auth_str = NULL;
 
     /* check each mask flag in order */
 
@@ -1835,6 +1977,10 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context 
kcontext,
         }
     }
 
+    kerr = ipadb_get_ldap_mod_auth_ind(kcontext, imods, entry, mod_op);
+    if (kerr)
+        goto done;
+
     /* KADM5_TL_DATA */
     if (entry->mask & KMASK_TL_DATA) {
         kerr = ipadb_get_tl_data(entry,
@@ -1854,12 +2000,36 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context 
kcontext,
             }
         }
 
+        kerr = krb5_dbe_get_string(kcontext, entry, "require_auth",
+                                   &req_auth_str);
+        if (kerr) {
+            goto done;
+        }
+
+        /* Do not store auth indicators from the string attribute in
+         * krbExtraData. Remove require_auth value from the entry temporarily. 
*/
+        if (req_auth_str != NULL) {
+            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);
         if (kerr && kerr != ENOENT) {
             goto done;
         }
+
+        /* Restore require_auth value */
+        if (req_auth_str != NULL) {
+            kerr = krb5_dbe_set_string(kcontext, entry, "require_auth",
+                                       req_auth_str);
+            if (kerr) {
+                goto done;
+            }
+        }
     }
 
     /* KADM5_LOAD */
@@ -1937,6 +2107,7 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context 
kcontext,
     kerr = 0;
 
 done:
+    free(req_auth_str);
     return kerr;
 }
 
diff --git a/install/share/60kerberos.ldif b/install/share/60kerberos.ldif
index 
8698e3a0538fd42443f4a3221fccfb739d4c104d..45bea2aefd3993a521e8ea732f902279622f8cb8
 100644
--- a/install/share/60kerberos.ldif
+++ b/install/share/60kerberos.ldif
@@ -266,7 +266,9 @@ attributetypes: ( 2.16.840.1.113719.1.301.4.53.1 NAME 
'krbPrincContainerRef' EQU
 attributetypes: ( 1.3.6.1.4.1.5322.21.2.5 NAME 'krbLastAdminUnlock' EQUALITY 
generalizedTimeMatch SYNTAX 1.3.6.1.4.1.1466.115.121.1.24 SINGLE-VALUE)
 ##### A list of services to which a service principal can delegate.
 attributetypes: ( 1.3.6.1.4.1.5322.21.2.4 NAME 'krbAllowedToDelegateTo' 
EQUALITY caseExactIA5Match SUBSTR caseExactSubstringsMatch SYNTAX 
1.3.6.1.4.1.1466.115.121.1.26)
-########################################################################
+##### A list of authentication indicator strings, one of which must be 
satisfied
+##### to authenticate to the principal as a service.
+attributetypes: ( 2.16.840.1.113730.3.8.15.2.1 NAME 'krbPrincipalAuthInd' 
EQUALITY caseExactMatch SYNTAX 1.3.6.1.4.1.1466.115.121.1.15)
 ########################################################################
 #                      Object Class Definitions                       #
 ########################################################################
@@ -294,7 +296,7 @@ objectClasses: ( 2.16.840.1.113719.1.301.6.4.1 NAME 
'krbKdcService' SUP ( krbSer
 objectClasses: ( 2.16.840.1.113719.1.301.6.5.1 NAME 'krbPwdService' SUP ( 
krbService ) )
 ###### The principal data auxiliary class. Holds principal information
 ###### and is used to store principal information for Person, Service objects.
-objectClasses: ( 2.16.840.1.113719.1.301.6.8.1 NAME 'krbPrincipalAux' 
AUXILIARY MAY ( krbPrincipalName $ krbCanonicalName $ krbUPEnabled $ 
krbPrincipalKey $ krbTicketPolicyReference $ krbPrincipalExpiration $ 
krbPasswordExpiration $ krbPwdPolicyReference $ krbPrincipalType $ 
krbPwdHistory $ krbLastPwdChange $ krbPrincipalAliases $ krbLastSuccessfulAuth 
$ krbLastFailedAuth $ krbLoginFailedCount $ krbExtraData $ krbLastAdminUnlock $ 
krbAllowedToDelegateTo ) )
+objectClasses: ( 2.16.840.1.113719.1.301.6.8.1 NAME 'krbPrincipalAux' 
AUXILIARY MAY ( krbPrincipalName $ krbCanonicalName $ krbUPEnabled $ 
krbPrincipalKey $ krbTicketPolicyReference $ krbPrincipalExpiration $ 
krbPasswordExpiration $ krbPwdPolicyReference $ krbPrincipalType $ 
krbPwdHistory $ krbLastPwdChange $ krbPrincipalAliases $ krbLastSuccessfulAuth 
$ krbLastFailedAuth $ krbLoginFailedCount $ krbExtraData $ krbLastAdminUnlock $ 
krbAllowedToDelegateTo $ krbPrincipalAuthInd ) )
 ###### This class is used to create additional principals and stand alone 
principals.
 objectClasses: ( 2.16.840.1.113719.1.301.6.9.1 NAME 'krbPrincipal' SUP ( top ) 
MUST ( krbPrincipalName ) MAY ( krbObjectReferences ) )
 ###### The principal references auxiliary class. Holds all principals referred
-- 
2.4.3

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