[Freeipa-devel] [RFC] Reload trust data in ipadb
Sumit Bose
sbose at redhat.com
Mon Oct 8 20:40:14 UTC 2012
On Fri, Oct 05, 2012 at 08:44:41AM -0400, Simo Sorce wrote:
> On Fri, 2012-10-05 at 13:32 +0200, Sumit Bose wrote:
> > From f8726fe1c4a2ab71ada1297003e3dbe6068e4207 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose <sbose at redhat.com>
> > Date: Fri, 5 Oct 2012 12:06:24 +0200
> > Subject: [PATCH] ipadb: reload trust information if domain is not know
> >
> > Currently the data about trusted domains is read once at startup. If a
> > new trust is added the KDC must be restarted to know about the new
> > trust. This patch reloads the trust data if there is a request from an
> > unknown domain. To make DOS attacks a bit harder the data can be
> > updated
> > only once in a minute.
> > ---
> > daemons/ipa-kdb/ipa_kdb_mspac.c | 44
> > ++++++++++++++++++++++++++++++++---------
> > 1 Datei geändert, 35 Zeilen hinzugefügt(+), 9 Zeilen entfernt(-)
> >
> > diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c
> > b/daemons/ipa-kdb/ipa_kdb_mspac.c
> > index
> > b5346fed1230d02a88c94ab913507112990a1651..f0bd3bfe913705abd44efb08325f54521533637e 100644
> > --- a/daemons/ipa-kdb/ipa_kdb_mspac.c
> > +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
> > @@ -40,6 +40,7 @@ struct ipadb_mspac {
> >
> > int num_trusts;
> > struct ipadb_adtrusts *trusts;
> > + time_t last_update;
> > };
> >
> >
> > @@ -983,6 +984,8 @@ static struct ipadb_adtrusts
> > *get_domain_from_realm(krb5_context context,
> > struct ipadb_context *ipactx;
> > struct ipadb_adtrusts *domain;
> > int i;
> > + krb5_error_code kerr;
> > + bool updated = false;
>
> The name of this variable sounds backwards, should be called need_update
> or similar.
this variable is not needed anymore
>
> > ipactx = ipadb_get_context(context);
> > if (!ipactx) {
> > @@ -993,17 +996,28 @@ static struct ipadb_adtrusts
> > *get_domain_from_realm(krb5_context context,
> > return NULL;
> > }
> >
> > - for (i = 0; i < ipactx->mspac->num_trusts; i++) {
> > - domain = &ipactx->mspac->trusts[i];
> > - if (strlen(domain->domain_name) != realm.length) {
> > - continue;
> > + do {
> > + for (i = 0; i < ipactx->mspac->num_trusts; i++) {
> > + domain = &ipactx->mspac->trusts[i];
> > + if (strlen(domain->domain_name) != realm.length) {
> > + continue;
> > + }
> > + if (strncasecmp(domain->domain_name, realm.data,
> > + realm.length) == 0) {
> > + return domain;
> > + }
> > }
> > - if (strncasecmp(domain->domain_name, realm.data,
> > realm.length) == 0) {
> > - return domain;
> > - }
> > - }
> >
> > - return NULL;
> > + if (updated) {
> > + return NULL;
> > + } else {
> > + kerr = ipadb_reinit_mspac(ipactx);
> > + if (kerr != 0) {
> > + return NULL;
> > + }
> > + updated = true;
> > + }
> > + } while(1);
> > }
>
> I do not think I like the do/while loop here, I would rather have
> another function that wraps the current get_domain_from_realm() like:
>
> static struct ipadb_adtrusts *get_domain_from_realm_update()
> {
> dom = get_domain_from_realm();
> if (dom == NULL) {
> /* no dom, try to see if a new trust appeared */
> kerr = ipadb_reinit_mspac(ipactx);
> dom = get_domain_from_realm();
> }
> return dom;
> }
>
> This is clearer imo, and allows you to decide if you really want to
> update or if you want to call the non-updating function later on, should
> we need it.
done
>
> > static krb5_error_code filter_logon_info(krb5_context context,
> > @@ -1550,6 +1564,16 @@ krb5_error_code ipadb_reinit_mspac(struct
> > ipadb_context *ipactx)
> > struct dom_sid gsid;
> > char *resstr;
> > int ret;
> > + time_t now;
> > +
> > + /* Do not update the mspac struct more than once a minute. This
> > would
> > + * avoid heavy load on the directory server if there are lots of
> > requests
> > + * from domains which we do not trust. */
> > + now = time(NULL);
> > + if (ipactx->mspac != NULL && now > ipactx->mspac->last_update &&
> > + (now - ipactx->mspac->last_update) < 60) {
> > + return 0;
> > + }
> >
> > /* clean up in case we had old values around */
> > ipadb_mspac_struct_free(&ipactx->mspac);
> > @@ -1560,6 +1584,8 @@ krb5_error_code ipadb_reinit_mspac(struct
> > ipadb_context *ipactx)
> > goto done;
> > }
> >
> > + ipactx->mspac->last_update = now;
> > +
> > kerr = ipadb_simple_search(ipactx, ipactx->base,
> > LDAP_SCOPE_SUBTREE,
> > "(objectclass=ipaNTDomainAttrs)",
> > dom_attrs,
> > &result);
>
> This part look fine, I wonder if we shouldn't make it even longer than 1
> min.
I would prefer to keep it 1 minute to make the user experience better in
the case where trust is added to more than one domain.
I have opened https://fedorahosted.org/freeipa/ticket/3156 to track the
reload issue.
New version attatched.
bye,
Sumit
>
> Simo.
>
> --
> Simo Sorce * Red Hat, Inc * New York
>
-------------- next part --------------
From 8c3eef19fb76e0f66d09a188946b7504696f284c Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Fri, 5 Oct 2012 12:06:24 +0200
Subject: [PATCH] ipadb: reload trust information if domain is not known
Currently the data about trusted domains is read once at startup. If a
new trust is added the KDC must be restarted to know about the new
trust. This patch reloads the trust data if there is a request from an
unknown domain. To make DOS attacks a bit harder the data can be updated
only once in a minute.
Fixes https://fedorahosted.org/freeipa/ticket/3156
---
daemons/ipa-kdb/ipa_kdb_mspac.c | 40 +++++++++++++++++++++++++++++++++++++++-
1 Datei geändert, 39 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index b3cef97db1bfe120d00b3bbfd30ed392dbb7b385..cf13ca37da96632caab6b2e4de6497184c4a51fa 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -40,6 +40,7 @@ struct ipadb_mspac {
int num_trusts;
struct ipadb_adtrusts *trusts;
+ time_t last_update;
};
#ifdef DEBUG
@@ -990,6 +991,31 @@ static struct ipadb_adtrusts *get_domain_from_realm(krb5_context context,
return NULL;
}
+static struct ipadb_adtrusts *get_domain_from_realm_update(krb5_context context,
+ krb5_data realm)
+{
+ struct ipadb_context *ipactx;
+ struct ipadb_adtrusts *domain;
+ krb5_error_code kerr;
+
+ domain = get_domain_from_realm(context, realm);
+ if (domain == NULL) {
+ ipactx = ipadb_get_context(context);
+ if (!ipactx) {
+ return NULL;
+ }
+
+ kerr = ipadb_reinit_mspac(ipactx);
+ if (kerr != 0) {
+ return NULL;
+ }
+
+ domain = get_domain_from_realm(context, realm);
+ }
+
+ return domain;
+}
+
static krb5_error_code filter_logon_info(krb5_context context,
TALLOC_CTX *memctx,
krb5_data realm,
@@ -1004,7 +1030,7 @@ static krb5_error_code filter_logon_info(krb5_context context,
struct ipadb_adtrusts *domain;
char *domsid;
- domain = get_domain_from_realm(context, realm);
+ domain = get_domain_from_realm_update(context, realm);
if (!domain) {
return EINVAL;
}
@@ -1534,6 +1560,16 @@ krb5_error_code ipadb_reinit_mspac(struct ipadb_context *ipactx)
struct dom_sid gsid;
char *resstr;
int ret;
+ time_t now;
+
+ /* Do not update the mspac struct more than once a minute. This would
+ * avoid heavy load on the directory server if there are lots of requests
+ * from domains which we do not trust. */
+ now = time(NULL);
+ if (ipactx->mspac != NULL && now > ipactx->mspac->last_update &&
+ (now - ipactx->mspac->last_update) < 60) {
+ return 0;
+ }
/* clean up in case we had old values around */
ipadb_mspac_struct_free(&ipactx->mspac);
@@ -1544,6 +1580,8 @@ krb5_error_code ipadb_reinit_mspac(struct ipadb_context *ipactx)
goto done;
}
+ ipactx->mspac->last_update = now;
+
kerr = ipadb_simple_search(ipactx, ipactx->base, LDAP_SCOPE_SUBTREE,
"(objectclass=ipaNTDomainAttrs)", dom_attrs,
&result);
--
1.7.11.4
More information about the Freeipa-devel
mailing list