On Wed, 2014-06-04 at 18:47 +0300, Alexander Bokovoy wrote:
> On Thu, 01 May 2014, Nathaniel McCallum wrote:
> >On Tue, 2014-03-11 at 11:09 -0400, Simo Sorce wrote:
> >> On Tue, 2014-03-11 at 16:05 +0200, Alexander Bokovoy wrote:
> >> > On Tue, 11 Mar 2014, Jan Pazdziora wrote:
> >> > >On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote:
> >> > >> Before this patch, ipa-kdb would load global configuration on startup
> >> > >> and never update it. This means that if global configuration is 
> >> > >> changed,
> >> > >> the KDC never receives the new configuration until it is restarted.
> >> > >>
> >> > >> This patch enables caching of the global configuration with a timeout 
> >> > >> of
> >> > >> 60 seconds.
> >> > >>
> >> > >> https://fedorahosted.org/freeipa/ticket/4153
> >> > >
> >> > >> >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 
> >> > >> >2001
> >> > >> From: Nathaniel McCallum <npmccal...@redhat.com>
> >> > >> Date: Mon, 24 Feb 2014 14:19:13 -0500
> >> > >> Subject: [PATCH] Periodically refresh global ipa-kdb configuration
> >> > >>
> >> > >> Before this patch, ipa-kdb would load global configuration on startup 
> >> > >> and
> >> > >> never update it. This means that if global configuration is changed, 
> >> > >> the
> >> > >> KDC never receives the new configuration until it is restarted.
> >> > >>
> >> > >> This patch enables caching of the global configuration with a timeout 
> >> > >> of
> >> > >> 60 seconds.
> >> > >>
> >> > >> https://fedorahosted.org/freeipa/ticket/4153
> >> > >
> >> > >I have only read the code and it looks sane, so depending on how much
> >> > >you consider my word about code-reading worth, ack.
> >> > >
> >> > >However, my gut feeling is that my preferred way of handling the issue
> >> > >(without knowing much about the background of the ticket) would
> >> > >probably be a HUP signal handler or something similar, rather than
> >> > >polling for current values with the value timeout. This patch
> >> > >introduces small nondeterminism to the behaviour when the usage of the
> >> > >new values cannot really be enfoced by the admin (without the daemon
> >> > >restart).
> >> > Thing is, we need the update to happen when other, non-root process
> >> > makes the changes -- in our case when IPA server running under httpd
> >> > privileges performs series of MS-RPC calls towards smbd which lead to
> >> > creating certain LDAP objects. httpd couldn't send SIGHUP to a process
> >> > not owned by httpd's effective user (non-root).
> >>
> >> Yes but this is not really the way to go.
> >>
> >> The proper fix is to use syncrepl/persistent search to get a
> >> notification of when we need to reload the configuration.
> >>
> >> On the patch itself I have a NACK due to this syntax used in various
> >> places: function()->attribute
> >>
> >> don't. do. that. ever.
> >>
> >> assign whatever come from the function to a local variable and *check*
> >> it is not null, *then* use it.
> >
> >Attached patch fixes the NACK issue, but does not address the question
> >of the larger approach. Do we need to take a different approach? If so,
> >what is it?
> 
> I've tested the patch but was unable to spot any activity to refresh the
> configuration after I've updated it with 
>     ipa config-mod --ipaconfigstring=KDC:Disable\ Lockout
> 
> At least, dirsrv logs didn't show me any attempt to re-read IPA config
> past the change.

I spent extensive time testing this today. I am unable to reproduce a
failure to reload data from LDAP. If I make the change using "ipa
config-mod", wait a minute and run a kinit the KDC always gets the new
values. So it works for me.

However, I did uncover a subtle bug with regard to ipaconfigstring which
would cause it to never to be unset once set. Perhaps you were running
into this?

The new attached version fixes this bug. Everything appears to work...

Nathaniel
From 34bd5bd7f7e846ee302ff54891dc42e6ac777690 Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum <npmccal...@redhat.com>
Date: Mon, 24 Feb 2014 14:19:13 -0500
Subject: [PATCH] Periodically refresh global ipa-kdb configuration

Before this patch, ipa-kdb would load global configuration on startup and
never update it. This means that if global configuration is changed, the
KDC never receives the new configuration until it is restarted.

This patch enables caching of the global configuration with a timeout of
60 seconds.

https://fedorahosted.org/freeipa/ticket/4153
---
 daemons/ipa-kdb/ipa_kdb.c            | 82 +++++++++++++++++++++---------------
 daemons/ipa-kdb/ipa_kdb.h            | 17 ++++++--
 daemons/ipa-kdb/ipa_kdb_audit_as.c   |  9 +++-
 daemons/ipa-kdb/ipa_kdb_mspac.c      | 10 ++++-
 daemons/ipa-kdb/ipa_kdb_principals.c | 11 ++++-
 5 files changed, 85 insertions(+), 44 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb.c b/daemons/ipa-kdb/ipa_kdb.c
index 0f3996cdfa35374c005bc1ed174dea0816a27747..e5101bdd0ad880888fd58fd93a5ca8133868db98 100644
--- a/daemons/ipa-kdb/ipa_kdb.c
+++ b/daemons/ipa-kdb/ipa_kdb.c
@@ -25,6 +25,8 @@
 
 #include "ipa_kdb.h"
 
+#define IPADB_GLOBAL_CONFIG_CACHE_TIME 60
+
 struct ipadb_context *ipadb_get_context(krb5_context kcontext)
 {
     void *db_ctx;
@@ -41,6 +43,7 @@ struct ipadb_context *ipadb_get_context(krb5_context kcontext)
 static void ipadb_context_free(krb5_context kcontext,
                                struct ipadb_context **ctx)
 {
+    struct ipadb_global_config *cfg;
     size_t c;
 
     if (*ctx != NULL) {
@@ -56,10 +59,11 @@ static void ipadb_context_free(krb5_context kcontext,
         ipadb_mspac_struct_free(&(*ctx)->mspac);
         krb5_free_default_realm(kcontext, (*ctx)->realm);
 
-        for (c = 0; (*ctx)->authz_data && (*ctx)->authz_data[c]; c++) {
-            free((*ctx)->authz_data[c]);
+        cfg = &(*ctx)->config;
+        for (c = 0; cfg->authz_data && cfg->authz_data[c]; c++) {
+            free(cfg->authz_data[c]);
         }
-        free((*ctx)->authz_data);
+        free(cfg->authz_data);
 
         free(*ctx);
         *ctx = NULL;
@@ -209,7 +213,7 @@ void ipadb_parse_user_auth(LDAP *lcontext, LDAPMessage *le,
     ldap_value_free_len(vals);
 }
 
-int ipadb_get_global_configs(struct ipadb_context *ipactx)
+static int ipadb_load_global_config(struct ipadb_context *ipactx)
 {
     char *attrs[] = { "ipaConfigString", IPA_KRB_AUTHZ_DATA_ATTR,
                       IPA_USER_AUTH_TYPE, NULL };
@@ -217,7 +221,6 @@ int ipadb_get_global_configs(struct ipadb_context *ipactx)
     LDAPMessage *res = NULL;
     LDAPMessage *first;
     char *base = NULL;
-    int i;
     int ret;
     char **authz_data_list;
 
@@ -241,45 +244,44 @@ int ipadb_get_global_configs(struct ipadb_context *ipactx)
     }
 
     /* Check for permitted authentication types. */
-    ipadb_parse_user_auth(ipactx->lcontext, res, &ipactx->user_auth);
+    ipadb_parse_user_auth(ipactx->lcontext, res, &ipactx->config.user_auth);
 
-    vals = ldap_get_values_len(ipactx->lcontext, first,
-                               "ipaConfigString");
-    if (!vals || !vals[0]) {
-        /* no config, set nothing */
-        ret = 0;
-        goto done;
-    }
+    /* Load config strings. */
+    vals = ldap_get_values_len(ipactx->lcontext, first, "ipaConfigString");
+    if (vals) {
+        ipactx->config.disable_last_success = false;
+        ipactx->config.disable_lockout = false;
+        for (int i = 0; vals[i]; i++) {
+            if (strncasecmp("KDC:Disable Last Success",
+                            vals[i]->bv_val, vals[i]->bv_len) == 0) {
+                ipactx->config.disable_last_success = true;
+                continue;
+            }
 
-    for (i = 0; vals[i]; i++) {
-        if (strncasecmp("KDC:Disable Last Success",
-                        vals[i]->bv_val, vals[i]->bv_len) == 0) {
-            ipactx->disable_last_success = true;
-            continue;
-        }
-        if (strncasecmp("KDC:Disable Lockout",
-                        vals[i]->bv_val, vals[i]->bv_len) == 0) {
-            ipactx->disable_lockout = true;
-            continue;
+            if (strncasecmp("KDC:Disable Lockout",
+                            vals[i]->bv_val, vals[i]->bv_len) == 0) {
+                ipactx->config.disable_lockout = true;
+                continue;
+            }
         }
     }
 
+	/* Load authz data. */
     ret = ipadb_ldap_attr_to_strlist(ipactx->lcontext, first,
                                      IPA_KRB_AUTHZ_DATA_ATTR, &authz_data_list);
-    if (ret != 0 && ret != ENOENT) {
-        goto done;
-    }
     if (ret == 0) {
-        if (ipactx->authz_data != NULL) {
-            for (i = 0; ipactx->authz_data[i]; i++) {
-                free(ipactx->authz_data[i]);
-            }
-            free(ipactx->authz_data);
+        if (ipactx->config.authz_data != NULL) {
+            for (int i = 0; ipactx->config.authz_data[i]; i++)
+                free(ipactx->config.authz_data[i]);
+            free(ipactx->config.authz_data);
         }
 
-        ipactx->authz_data = authz_data_list;
-    }
+        ipactx->config.authz_data = authz_data_list;
+    } else if (ret != ENOENT)
+        goto done;
 
+    /* Success! */
+    ipactx->config.last_update = time(NULL);
     ret = 0;
 
 done:
@@ -289,6 +291,18 @@ done:
     return ret;
 }
 
+const struct ipadb_global_config *
+ipadb_get_global_config(struct ipadb_context *ipactx)
+{
+    time_t now = 0;
+
+    if (time(&now) != (time_t)-1
+        && now - ipactx->config.last_update > IPADB_GLOBAL_CONFIG_CACHE_TIME)
+        ipadb_load_global_config(ipactx);
+
+    return &ipactx->config;
+}
+
 int ipadb_get_connection(struct ipadb_context *ipactx)
 {
     struct berval **vals = NULL;
@@ -390,7 +404,7 @@ int ipadb_get_connection(struct ipadb_context *ipactx)
     ipactx->n_supp_encs = n_kst;
 
     /* get additional options */
-    ret = ipadb_get_global_configs(ipactx);
+    ret = ipadb_load_global_config(ipactx);
     if (ret) {
         goto done;
     }
diff --git a/daemons/ipa-kdb/ipa_kdb.h b/daemons/ipa-kdb/ipa_kdb.h
index 6c036e3b6403a3b5fde544dc49c9d7efbaa6ca9b..b92107bab5a259601160a402c54fa8ed440925b3 100644
--- a/daemons/ipa-kdb/ipa_kdb.h
+++ b/daemons/ipa-kdb/ipa_kdb.h
@@ -87,6 +87,14 @@ enum ipadb_user_auth {
   IPADB_USER_AUTH_OTP      = 1 << 3,
 };
 
+struct ipadb_global_config {
+	time_t last_update;
+	bool disable_last_success;
+	bool disable_lockout;
+	char **authz_data;
+	enum ipadb_user_auth user_auth;
+};
+
 struct ipadb_context {
     char *uri;
     char *base;
@@ -99,10 +107,9 @@ struct ipadb_context {
     krb5_key_salt_tuple *supp_encs;
     int n_supp_encs;
     struct ipadb_mspac *mspac;
-    bool disable_last_success;
-    bool disable_lockout;
-    char **authz_data;
-    enum ipadb_user_auth user_auth;
+
+    /* Don't access this directly, use ipadb_get_global_config(). */
+    struct ipadb_global_config config;
 };
 
 #define IPA_E_DATA_MAGIC 0x0eda7a
@@ -277,3 +284,5 @@ void ipadb_audit_as_req(krb5_context kcontext,
 /* AUTH METHODS */
 void ipadb_parse_user_auth(LDAP *lcontext, LDAPMessage *le,
                            enum ipadb_user_auth *user_auth);
+const struct ipadb_global_config *
+ipadb_get_global_config(struct ipadb_context *ipactx);
diff --git a/daemons/ipa-kdb/ipa_kdb_audit_as.c b/daemons/ipa-kdb/ipa_kdb_audit_as.c
index 7596db0fae165efd21e7c24f9af97a200e99e624..52c165442bde61d3ce88843b122aae7fe0fae50b 100644
--- a/daemons/ipa-kdb/ipa_kdb_audit_as.c
+++ b/daemons/ipa-kdb/ipa_kdb_audit_as.c
@@ -30,6 +30,7 @@ void ipadb_audit_as_req(krb5_context kcontext,
                         krb5_timestamp authtime,
                         krb5_error_code error_code)
 {
+    const struct ipadb_global_config *gcfg;
     struct ipadb_context *ipactx;
     struct ipadb_e_data *ied;
     krb5_error_code kerr;
@@ -63,6 +64,10 @@ void ipadb_audit_as_req(krb5_context kcontext,
 
     client->mask = 0;
 
+    gcfg = ipadb_get_global_config(ipactx);
+    if (gcfg == NULL)
+        return;
+
     switch (error_code) {
     case 0:
         /* Check if preauth flag is specified (default), otherwise we have
@@ -72,7 +77,7 @@ void ipadb_audit_as_req(krb5_context kcontext,
                 client->fail_auth_count = 0;
                 client->mask |= KMASK_FAIL_AUTH_COUNT;
             }
-            if (ipactx->disable_last_success) {
+            if (gcfg->disable_last_success) {
                 break;
             }
             client->last_success = authtime;
@@ -83,7 +88,7 @@ void ipadb_audit_as_req(krb5_context kcontext,
     case KRB5KDC_ERR_PREAUTH_FAILED:
     case KRB5KRB_AP_ERR_BAD_INTEGRITY:
 
-        if (ipactx->disable_lockout) {
+        if (gcfg->disable_lockout) {
             break;
         }
 
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index a73a3cb46e104b43493177e333deb2b0d6226c2a..084b689d459f27e72d679d37e24650149973df61 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -1878,6 +1878,9 @@ void get_authz_data_types(krb5_context context, krb5_db_entry *entry,
     }
 
     if (ied == NULL || ied->authz_data == NULL) {
+        const struct ipadb_global_config *gcfg = NULL;
+        char **tmp = NULL;
+
         if (context == NULL) {
             krb5_klog_syslog(LOG_ERR, "Missing Kerberos context, no " \
                                       "authorization data will be added.");
@@ -1885,14 +1888,17 @@ void get_authz_data_types(krb5_context context, krb5_db_entry *entry,
         }
 
         ipactx = ipadb_get_context(context);
-        if (ipactx == NULL || ipactx->authz_data == NULL) {
+        gcfg = ipadb_get_global_config(ipactx);
+        if (gcfg != NULL)
+            tmp = gcfg->authz_data;
+        if (ipactx == NULL || tmp == NULL) {
             krb5_klog_syslog(LOG_ERR, "No default authorization data types " \
                                       "available, no authorization data will " \
                                       "be added.");
             goto done;
         }
 
-        authz_data_list = ipactx->authz_data;
+        authz_data_list = tmp;
     } else {
         authz_data_list = ied->authz_data;
     }
diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index 96f473e487ae50cf63528df2bf0630bfb159887b..e158c236eab5c7c5a7c12664dbde5d51cc55406d 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -320,18 +320,25 @@ static void ipadb_validate_password(struct ipadb_context *ipactx,
 static enum ipadb_user_auth ipadb_get_user_auth(struct ipadb_context *ipactx,
                                                 LDAPMessage *lentry)
 {
+    enum ipadb_user_auth gua = IPADB_USER_AUTH_NONE;
     enum ipadb_user_auth ua = IPADB_USER_AUTH_NONE;
+    const struct ipadb_global_config *gcfg = NULL;
 
     /* Get the user's user_auth settings. */
     ipadb_parse_user_auth(ipactx->lcontext, lentry, &ua);
 
+    /* Get the global user_auth settings. */
+    gcfg = ipadb_get_global_config(ipactx);
+    if (gcfg != NULL)
+        gua = gcfg->user_auth;
+
     /* If the disabled flag is set, ignore everything else. */
-    if ((ua | ipactx->user_auth) & IPADB_USER_AUTH_DISABLED)
+    if ((ua | gua) & IPADB_USER_AUTH_DISABLED)
         return IPADB_USER_AUTH_DISABLED;
 
     /* Determine which user_auth policy is active: user or global. */
     if (ua == IPADB_USER_AUTH_NONE)
-        ua = ipactx->user_auth;
+        ua = gua;
 
     /* Perform flag validation. */
     ipadb_validate_otp(ipactx, lentry, &ua);
-- 
2.0.0

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to